[PATCH] drm/i915: Selectively enable self-reclaim
in [Kernel]
|
Prev: 2.6.34 Northbridge Chipset Errors on HP Proliant 4 x Opteron in x86_64 mode
Next: x86: enlightenment for ticket spin locks - eliminate NOPs introduced by first patch
From: Dave Airlie on 30 Jun 2010 03:00 On Thu, Jan 28, 2010 at 3:19 AM, Linus Torvalds <torvalds(a)linux-foundation.org> wrote: > > > On Wed, 27 Jan 2010, Chris Wilson wrote: >> >> Yes, it survives a short torture test that leaks lots of bo objects from >> X. Obviously this patch depends upon the new interface. > > All right. I'll apply my patch, since i'd rather have any half-way complex > logic for handling mappings in the generic mm code. And then I'll apply > yours, because now I can look at it without wanting to dig my eyes out. > Chris's patch has been reported to cause a regression in hibernate, I haven't set up a reproducer yet, and it might be a stable issue (someone bisected it in stable). https://bugzilla.kernel.org/show_bug.cgi?id=13811 Hopefully I can spend some time tomorrow setting up a machine to test. Dave. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
From: Chris Wilson on 30 Jun 2010 03:10 On Wed, 30 Jun 2010 16:54:07 +1000, Dave Airlie <airlied(a)gmail.com> wrote: > Chris's patch has been reported to cause a regression in hibernate, Reviewing the patch again, we no longer set the default gfpmask on the inode to contain NORETRY and instead add the NORETRY at the one spot in the code where we are trying to do a large allocation and our shrinker would be prevented from running (due to contention on struct_mutex). I do not know how this causes memory corruption across hibernate and would appreciate any insights. -- Chris Wilson, Intel Open Source Technology Centre -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
From: Linus Torvalds on 30 Jun 2010 21:30 On Wed, Jun 30, 2010 at 4:07 PM, Linus Torvalds <torvalds(a)linux-foundation.org> wrote: > > That commit changes the page cache allocation to use > > + � � � � � � � � � � � � � � � � � � � � �mapping_gfp_mask (mapping) | > + � � � � � � � � � � � � � � � � � � � � �__GFP_COLD | > + � � � � � � � � � � � � � � � � � � � � �gfpmask); > > if I read it right. And the default mapping_gfp_mask() is > GFP_HIGHUSER_MOVABLE, so I think you get all of > (__GFP_WAIT | __GFP_IO | __GFP_FS | __GFP_HARDWALL | __GFP_HIGHMEM) > set by default. ... and then I left out the one flag I _meant_ to have there, namely __GFP_MOVABLE. > The old code didn't just play games with ~__GFP_NORETRY and change > that at runtime (which was buggy - no locking, no protection, no > nothing), it also initialized the gfp mask. And that code also got > removed: In fact, I don't really see why we should use that mapping_gfp_mask() at all, since all allocations should be going through that i915_gem_object_get_pages() function anyway. So why not just change that function to ignore the default gfp mask for the mapping, and just use the mask that the o915 driver wants? Btw, why did it want to mark the pages reclaimable? Anyway, what I'm suggesting somebody who sees this test is just something like the patch below (whitespace-damage - I'm cutting and pasting, it's a trivial one-liner). Does this change any behavior? Vefa? Linus --- diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c index 9ded3da..ec8ed6b 100644 --- a/drivers/gpu/drm/i915/i915_gem.c +++ b/drivers/gpu/drm/i915/i915_gem.c @@ -2239,7 +2239,7 @@ i915_gem_object_get_pages(struct drm_gem_object *obj, mapping = inode->i_mapping; for (i = 0; i < page_count; i++) { page = read_cache_page_gfp(mapping, i, - mapping_gfp_mask (mapping) | + GFP_HIGHMEM | __GFP_COLD | gfpmask); if (IS_ERR(page)) -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
From: KOSAKI Motohiro on 30 Jun 2010 22:00 > On Wed, Jun 30, 2010 at 4:07 PM, Linus Torvalds > <torvalds(a)linux-foundation.org> wrote: > > > > That commit changes the page cache allocation to use > > > > + � � � � � � � � � � � � � � � � � � � � �mapping_gfp_mask (mapping) | > > + � � � � � � � � � � � � � � � � � � � � �__GFP_COLD | > > + � � � � � � � � � � � � � � � � � � � � �gfpmask); > > > > if I read it right. And the default mapping_gfp_mask() is > > GFP_HIGHUSER_MOVABLE, so I think you get all of > > (__GFP_WAIT | __GFP_IO | __GFP_FS | __GFP_HARDWALL | __GFP_HIGHMEM) > > set by default. > > .. and then I left out the one flag I _meant_ to have there, namely > __GFP_MOVABLE. > > > The old code didn't just play games with ~__GFP_NORETRY and change > > that at runtime (which was buggy - no locking, no protection, no > > nothing), it also initialized the gfp mask. And that code also got > > removed: > > In fact, I don't really see why we should use that mapping_gfp_mask() > at all, since all allocations should be going through that > i915_gem_object_get_pages() function anyway. So why not just change > that function to ignore the default gfp mask for the mapping, and just > use the mask that the o915 driver wants? > > Btw, why did it want to mark the pages reclaimable? I'm not GEM expert at all. but as far as I read following documentation, http://lwn.net/Articles/283798/ GEM memory have pin and unpin state and unpined memory can be reclaimed. but it's just guess. So, I wonder if your patch solve the issue. I don't imazine a memory state which "swap-out is safe, but compaction is unsafe". Dave, if you have good documentation which we understand GEM memory management, could you send us? - kosaki > > Anyway, what I'm suggesting somebody who sees this test is just > something like the patch below (whitespace-damage - I'm cutting and > pasting, it's a trivial one-liner). Does this change any behavior? > Vefa? > > Linus > > --- > diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c > index 9ded3da..ec8ed6b 100644 > --- a/drivers/gpu/drm/i915/i915_gem.c > +++ b/drivers/gpu/drm/i915/i915_gem.c > @@ -2239,7 +2239,7 @@ i915_gem_object_get_pages(struct drm_gem_object *obj, > mapping = inode->i_mapping; > for (i = 0; i < page_count; i++) { > page = read_cache_page_gfp(mapping, i, > - mapping_gfp_mask (mapping) | > + GFP_HIGHMEM | > __GFP_COLD | > gfpmask); > if (IS_ERR(page)) > -- > To unsubscribe from this list: send the line "unsubscribe linux-kernel" in > the body of a message to majordomo(a)vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > Please read the FAQ at http://www.tux.org/lkml/ -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
From: Dave Airlie on 1 Jul 2010 06:20
On Thu, Jul 1, 2010 at 11:24 AM, Linus Torvalds <torvalds(a)linux-foundation.org> wrote: > On Wed, Jun 30, 2010 at 4:07 PM, Linus Torvalds > <torvalds(a)linux-foundation.org> wrote: >> >> That commit changes the page cache allocation to use >> >> + � � � � � � � � � � � � � � � � � � � � �mapping_gfp_mask (mapping) | >> + � � � � � � � � � � � � � � � � � � � � �__GFP_COLD | >> + � � � � � � � � � � � � � � � � � � � � �gfpmask); >> >> if I read it right. And the default mapping_gfp_mask() is >> GFP_HIGHUSER_MOVABLE, so I think you get all of >> (__GFP_WAIT | __GFP_IO | __GFP_FS | __GFP_HARDWALL | __GFP_HIGHMEM) >> set by default. > > .. and then I left out the one flag I _meant_ to have there, namely > __GFP_MOVABLE. > >> The old code didn't just play games with ~__GFP_NORETRY and change >> that at runtime (which was buggy - no locking, no protection, no >> nothing), it also initialized the gfp mask. And that code also got >> removed: > > In fact, I don't really see why we should use that mapping_gfp_mask() > at all, since all allocations should be going through that > i915_gem_object_get_pages() function anyway. So why not just change > that function to ignore the default gfp mask for the mapping, and just > use the mask that the o915 driver wants? > > Btw, why did it want to mark the pages reclaimable? > > Anyway, what I'm suggesting somebody who sees this test is just > something like the patch below (whitespace-damage - I'm cutting and > pasting, it's a trivial one-liner). �Does this change any behavior? > Vefa? > I think Linus is on to something, I'll finish my testing tomorrow, I'm stuck testing this on a laptop with a 4200rpm driver, hibernating takes quite a long time ;-( But I have reproduced the initial failure,reverted the patch reproduced success, and then did a couple of cycles with Linus patch before I left. Tomorrow I'll do another 3-4 cycles to confirm. the patch also needs a couple of __ before GFP_HIGHMEM, in case anyone else was hacking it. Dave. > � � � � � � � � � �Linus > > --- > diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c > index 9ded3da..ec8ed6b 100644 > --- a/drivers/gpu/drm/i915/i915_gem.c > +++ b/drivers/gpu/drm/i915/i915_gem.c > @@ -2239,7 +2239,7 @@ i915_gem_object_get_pages(struct drm_gem_object *obj, > � � � �mapping = inode->i_mapping; > � � � �for (i = 0; i < page_count; i++) { > � � � � � � � �page = read_cache_page_gfp(mapping, i, > - � � � � � � � � � � � � � � � � � � � � �mapping_gfp_mask (mapping) | > + � � � � � � � � � � � � � � � � � � � � �GFP_HIGHMEM | > � � � � � � � � � � � � � � � � � � � � � __GFP_COLD | > � � � � � � � � � � � � � � � � � � � � � gfpmask); > � � � � � � � �if (IS_ERR(page)) > -- > To unsubscribe from this list: send the line "unsubscribe linux-kernel" in > the body of a message to majordomo(a)vger.kernel.org > More majordomo info at �http://vger.kernel.org/majordomo-info.html > Please read the FAQ at �http://www.tux.org/lkml/ > -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/ |