diff mbox

drm/i915: Do not drop pagetables when empty

Message ID 1501153556-19944-2-git-send-email-rex.tsai@canonical.com
State New
Headers show

Commit Message

Rex Tsai July 27, 2017, 11:05 a.m. UTC
From: Chris Wilson <chris@chris-wilson.co.uk>

From: Chris Wilson <chris@chris-wilson.co.uk>

BugLink: https://bugs.launchpad.net/bugs/1680904

This is the minimal backport for stable of the upstream commit:

commit dd19674bacba227ae5d3ce680cbc5668198894dc
Author: Chris Wilson <chris@chris-wilson.co.uk>
Date:   Wed Feb 15 08:43:46 2017 +0000

    drm/i915: Remove bitmap tracking for used-ptes

Due to a race with the shrinker, when we try to allocate a pagetable, we
may end up shrinking it instead. This comes as a nasty surprise as we
try to dereference it to fill in the pagetable entries for the object.

In linus/master this is fixed by pinning the pagetables prior to
allocation, but that backport is roughly
 drivers/gpu/drm/i915/i915_gem_gtt.c |   10 ----------
 1 file changed, 10 deletions(-)
i.e. unsuitable for stable. Instead we neuter the code that tried to
free the pagetables.

Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=99295
Fixes: 2ce5179fe826 ("drm/i915/gtt: Free unused lower-level page tables")
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Michel Thierry <michel.thierry@intel.com>
Cc: Mika Kuoppala <mika.kuoppala@intel.com>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
Cc: Michał Winiarski <michal.winiarski@intel.com>
Cc: Daniel Vetter <daniel.vetter@intel.com>
Cc: Jani Nikula <jani.nikula@linux.intel.com>
Cc: intel-gfx@lists.freedesktop.org
Cc: <stable@vger.kernel.org> # v4.10+
Tested-by: Maël Lavault <mael.lavault@protonmail.com>
Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
(cherry picked from commit 92220696d5d07525443d9280c08c498e77d0386c)
Signed-off-by: Rex Tsai (蔡志展) <rex.tsai@canonical.com>
---
 drivers/gpu/drm/i915/i915_gem_gtt.c | 10 ----------
 1 file changed, 10 deletions(-)

Comments

Stefan Bader July 27, 2017, 3:19 p.m. UTC | #1
On 27.07.2017 13:05, Rex Tsai (蔡志展) wrote:
> From: Chris Wilson <chris@chris-wilson.co.uk>
> 
> From: Chris Wilson <chris@chris-wilson.co.uk>
> 
> BugLink: https://bugs.launchpad.net/bugs/1680904
> 
> This is the minimal backport for stable of the upstream commit:
> 
> commit dd19674bacba227ae5d3ce680cbc5668198894dc
> Author: Chris Wilson <chris@chris-wilson.co.uk>
> Date:   Wed Feb 15 08:43:46 2017 +0000
> 
>     drm/i915: Remove bitmap tracking for used-ptes
> 
> Due to a race with the shrinker, when we try to allocate a pagetable, we
> may end up shrinking it instead. This comes as a nasty surprise as we
> try to dereference it to fill in the pagetable entries for the object.
> 
> In linus/master this is fixed by pinning the pagetables prior to
> allocation, but that backport is roughly
>  drivers/gpu/drm/i915/i915_gem_gtt.c |   10 ----------
>  1 file changed, 10 deletions(-)

^Funnily that is the size of this backport...

> i.e. unsuitable for stable. Instead we neuter the code that tried to
> free the pagetables.
> 
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=99295
> Fixes: 2ce5179fe826 ("drm/i915/gtt: Free unused lower-level page tables")
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Michel Thierry <michel.thierry@intel.com>
> Cc: Mika Kuoppala <mika.kuoppala@intel.com>
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> Cc: Michał Winiarski <michal.winiarski@intel.com>
> Cc: Daniel Vetter <daniel.vetter@intel.com>
> Cc: Jani Nikula <jani.nikula@linux.intel.com>
> Cc: intel-gfx@lists.freedesktop.org
> Cc: <stable@vger.kernel.org> # v4.10+
> Tested-by: Maël Lavault <mael.lavault@protonmail.com>
> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> (cherry picked from commit 92220696d5d07525443d9280c08c498e77d0386c)

Not upstream linux tree, where was the patch picked from? I assume one of the
stable trees, but which one?

> Signed-off-by: Rex Tsai (蔡志展) <rex.tsai@canonical.com>
> ---
>  drivers/gpu/drm/i915/i915_gem_gtt.c | 10 ----------
>  1 file changed, 10 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
> index 6924a8e..b2d57d1 100644
> --- a/drivers/gpu/drm/i915/i915_gem_gtt.c
> +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
> @@ -735,10 +735,6 @@ static bool gen8_ppgtt_clear_pt(struct i915_address_space *vm,
>  	GEM_BUG_ON(pte_end > GEN8_PTES);
>  
>  	bitmap_clear(pt->used_ptes, pte, num_entries);
> -	if (USES_FULL_PPGTT(vm->i915)) {
> -		if (bitmap_empty(pt->used_ptes, GEN8_PTES))
> -			return true;
> -	}
>  
>  	pt_vaddr = kmap_px(pt);
>  
> @@ -778,9 +774,6 @@ static bool gen8_ppgtt_clear_pd(struct i915_address_space *vm,
>  		}
>  	}
>  
> -	if (bitmap_empty(pd->used_pdes, I915_PDES))
> -		return true;
> -
>  	return false;
>  }
>  
> @@ -816,9 +809,6 @@ static bool gen8_ppgtt_clear_pdp(struct i915_address_space *vm,
>  
>  	mark_tlbs_dirty(ppgtt);
>  
> -	if (bitmap_empty(pdp->used_pdpes, I915_PDPES_PER_PDP(dev_priv)))
> -		return true;
> -
>  	return false;
>  }
>  
>
Rex Tsai July 28, 2017, 3:16 a.m. UTC | #2
Stefan Bader <stefan.bader@canonical.com> 於 2017年7月27日 週四 下午11:19寫道:

> > i.e. unsuitable for stable. Instead we neuter the code that tried to
> > free the pagetables.
> >
> > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=99295
> > Fixes: 2ce5179fe826 ("drm/i915/gtt: Free unused lower-level page tables")
> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > Cc: Michel Thierry <michel.thierry@intel.com>
> > Cc: Mika Kuoppala <mika.kuoppala@intel.com>
> > Cc: Chris Wilson <chris@chris-wilson.co.uk>
> > Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> > Cc: Michał Winiarski <michal.winiarski@intel.com>
> > Cc: Daniel Vetter <daniel.vetter@intel.com>
> > Cc: Jani Nikula <jani.nikula@linux.intel.com>
> > Cc: intel-gfx@lists.freedesktop.org
> > Cc: <stable@vger.kernel.org> # v4.10+
> > Tested-by: Maël Lavault <mael.lavault@protonmail.com>
> > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> > Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> > (cherry picked from commit 92220696d5d07525443d9280c08c498e77d0386c)
>
> Not upstream linux tree, where was the patch picked from? I assume one of
> the
> stable trees, but which one?
>

As I explained in cover letter[1], it's from stable tree[2].

% git remote -v
origin git://git.kernel.org/pub/scm/linux/kernel/git/stable/linux-stable.git
(fetch)
origin git://git.kernel.org/pub/scm/linux/kernel/git/stable/linux-stable.git
(push)
% git tag --contains=92220696d5d07525443d9280c08c498e77d0386c
v4.11.10
v4.11.11
v4.11.12
v4.11.6
v4.11.7
v4.11.8
v4.11.9
% git branch --contains=92220696d5d07525443d9280c08c498e77d0386c
* linux-4.11.y

[1] https://lists.ubuntu.com/archives/kernel-team/2017-July/085964.html
[2]
https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux-stable.git/commit/?id=92220696d5d07525443d9280c08c498e77d0386c
Stefan Bader July 28, 2017, 8:11 a.m. UTC | #3
On 28.07.2017 05:16, Rex Tsai wrote:
> Stefan Bader <stefan.bader@canonical.com <mailto:stefan.bader@canonical.com>> 於
> 2017年7月27日 週四 下午11:19寫道:
> 
>     > i.e. unsuitable for stable. Instead we neuter the code that tried to
>     > free the pagetables.
>     >
>     > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=99295
>     > Fixes: 2ce5179fe826 ("drm/i915/gtt: Free unused lower-level page tables")
>     > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk
>     <mailto:chris@chris-wilson.co.uk>>
>     > Cc: Michel Thierry <michel.thierry@intel.com
>     <mailto:michel.thierry@intel.com>>
>     > Cc: Mika Kuoppala <mika.kuoppala@intel.com <mailto:mika.kuoppala@intel.com>>
>     > Cc: Chris Wilson <chris@chris-wilson.co.uk <mailto:chris@chris-wilson.co.uk>>
>     > Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com
>     <mailto:joonas.lahtinen@linux.intel.com>>
>     > Cc: Michał Winiarski <michal.winiarski@intel.com
>     <mailto:michal.winiarski@intel.com>>
>     > Cc: Daniel Vetter <daniel.vetter@intel.com <mailto:daniel.vetter@intel.com>>
>     > Cc: Jani Nikula <jani.nikula@linux.intel.com
>     <mailto:jani.nikula@linux.intel.com>>
>     > Cc: intel-gfx@lists.freedesktop.org <mailto:intel-gfx@lists.freedesktop.org>
>     > Cc: <stable@vger.kernel.org <mailto:stable@vger.kernel.org>> # v4.10+
>     > Tested-by: Maël Lavault <mael.lavault@protonmail.com
>     <mailto:mael.lavault@protonmail.com>>
>     > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com
>     <mailto:daniel.vetter@intel.com>>
>     > Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org
>     <mailto:gregkh@linuxfoundation.org>>
>     > (cherry picked from commit 92220696d5d07525443d9280c08c498e77d0386c)
> 
>     Not upstream linux tree, where was the patch picked from? I assume one of the
>     stable trees, but which one?
> 
> 
> As I explained in cover letter[1], it's from stable tree[2].

Ah ok, I missed that. Still this should be hinted in the cherry-pick line of the
patch itself like:

(cherry picked from commit 92220696d5d07525443d9280c08c498e77d0386c linux-4.11.y)

This can be added when applying, so no need to do a v2 for this.

Acked-by: Stefan Bader <stefan.bader@canonical.com>

> 
> % git remote -v
> origingit://git.kernel.org/pub/scm/linux/kernel/git/stable/linux-stable.git
> <http://git.kernel.org/pub/scm/linux/kernel/git/stable/linux-stable.git> (fetch)
> origingit://git.kernel.org/pub/scm/linux/kernel/git/stable/linux-stable.git
> <http://git.kernel.org/pub/scm/linux/kernel/git/stable/linux-stable.git> (push)
> % git tag --contains=92220696d5d07525443d9280c08c498e77d0386c
> v4.11.10
> v4.11.11
> v4.11.12
> v4.11.6
> v4.11.7
> v4.11.8
> v4.11.9
> % git branch --contains=92220696d5d07525443d9280c08c498e77d0386c
> * linux-4.11.y
> 
> [1] https://lists.ubuntu.com/archives/kernel-team/2017-July/085964.html
> [2]
> https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux-stable.git/commit/?id=92220696d5d07525443d9280c08c498e77d0386c
>
Seth Forshee July 28, 2017, 6:23 p.m. UTC | #4
On Thu, Jul 27, 2017 at 07:05:56PM +0800, Rex Tsai (蔡志展) wrote:
> From: Chris Wilson <chris@chris-wilson.co.uk>
> 
> From: Chris Wilson <chris@chris-wilson.co.uk>
> 
> BugLink: https://bugs.launchpad.net/bugs/1680904
> 
> This is the minimal backport for stable of the upstream commit:
> 
> commit dd19674bacba227ae5d3ce680cbc5668198894dc
> Author: Chris Wilson <chris@chris-wilson.co.uk>
> Date:   Wed Feb 15 08:43:46 2017 +0000
> 
>     drm/i915: Remove bitmap tracking for used-ptes
> 
> Due to a race with the shrinker, when we try to allocate a pagetable, we
> may end up shrinking it instead. This comes as a nasty surprise as we
> try to dereference it to fill in the pagetable entries for the object.
> 
> In linus/master this is fixed by pinning the pagetables prior to
> allocation, but that backport is roughly
>  drivers/gpu/drm/i915/i915_gem_gtt.c |   10 ----------
>  1 file changed, 10 deletions(-)
> i.e. unsuitable for stable. Instead we neuter the code that tried to
> free the pagetables.
> 
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=99295
> Fixes: 2ce5179fe826 ("drm/i915/gtt: Free unused lower-level page tables")
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Michel Thierry <michel.thierry@intel.com>
> Cc: Mika Kuoppala <mika.kuoppala@intel.com>
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> Cc: Michał Winiarski <michal.winiarski@intel.com>
> Cc: Daniel Vetter <daniel.vetter@intel.com>
> Cc: Jani Nikula <jani.nikula@linux.intel.com>
> Cc: intel-gfx@lists.freedesktop.org
> Cc: <stable@vger.kernel.org> # v4.10+
> Tested-by: Maël Lavault <mael.lavault@protonmail.com>
> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> (cherry picked from commit 92220696d5d07525443d9280c08c498e77d0386c)
> Signed-off-by: Rex Tsai (蔡志展) <rex.tsai@canonical.com>

With the provenance clarification suggested by smb:

Acked-by: Seth Forshee <seth.forshee@canonical.com>
Kleber Sacilotto de Souza Aug. 7, 2017, 2:02 p.m. UTC | #5
Applied on zesty/master-next branch, with the provenance clarification
suggested by smb. Thanks.
diff mbox

Patch

diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
index 6924a8e..b2d57d1 100644
--- a/drivers/gpu/drm/i915/i915_gem_gtt.c
+++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
@@ -735,10 +735,6 @@  static bool gen8_ppgtt_clear_pt(struct i915_address_space *vm,
 	GEM_BUG_ON(pte_end > GEN8_PTES);
 
 	bitmap_clear(pt->used_ptes, pte, num_entries);
-	if (USES_FULL_PPGTT(vm->i915)) {
-		if (bitmap_empty(pt->used_ptes, GEN8_PTES))
-			return true;
-	}
 
 	pt_vaddr = kmap_px(pt);
 
@@ -778,9 +774,6 @@  static bool gen8_ppgtt_clear_pd(struct i915_address_space *vm,
 		}
 	}
 
-	if (bitmap_empty(pd->used_pdes, I915_PDES))
-		return true;
-
 	return false;
 }
 
@@ -816,9 +809,6 @@  static bool gen8_ppgtt_clear_pdp(struct i915_address_space *vm,
 
 	mark_tlbs_dirty(ppgtt);
 
-	if (bitmap_empty(pdp->used_pdpes, I915_PDPES_PER_PDP(dev_priv)))
-		return true;
-
 	return false;
 }