Re: [Intel-gfx] [PATCH] drm/i915: Selectively enable self-reclaim
On Sun, Jul 18, 2010 at 7:27 AM, M. Vefa Bicakci bic...@superonline.com wrote: After hours of testing I came up with the following result: We need to have the __GFP_RECLAIMABLE flag in addition to GFP_HIGHUSER. Thanks for the extensive testing, and I'm committing the one-liner to add it, and cc'ing it to stable. I'm pretty certain that there is something overly fragile in the i915 driver that this flag makes so much of a difference, but at the same time I'm actually happy that it's that reclaimable flag, because at least that one was always the conceptually makes sense one. So I suspected it would be some low-memory issue and the flag that woudl turn out to matter would be the NOMEMALLOC one, but I'm happy to have been wrong. Adding __GFP_RECLAIMABLE is sane, although I really would like to understand why the i915 driver apparently cares so deeply about the allocation/freeing patterns. But whatever. Thanks again for being such a thorough tester, Linus ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm/i915: Selectively enable self-reclaim
On Thu, Jul 1, 2010 at 11:24 AM, Linus Torvalds
torva...@linux-foundation.org wrote:
On Wed, Jun 30, 2010 at 4:07 PM, Linus Torvalds
torva...@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 majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm/i915: Selectively enable self-reclaim
On Thu, Jul 1, 2010 at 3:34 PM, M. Vefa Bicakci bic...@superonline.com wrote: Based on my testing, I am happy to report that the change you suggest fixes the memory corruption (segfaults) after thaw issue for me. I can't thank you enough times for this. Hey, goodie. And you're the one to be thanked - bisecting it down to that commit that wasn't _meant_ to have any real semantic changes (except for the bug-fix of racy mapping gfp-flags update) is what really cracked the lid on the problem. Now, the obligatory question: Could we have this fix applied to 2.6.32, 2.6.33 and 2.6.34 ? No problem, except we should first determine exactly what flags are the appropriate ones. My original patch was obviously not even compile-tested, and I actually meant for people to use GFP_HIGHUSER rather than __GFP_HIGHMEM. That contains all the regular allocation flags (but not the __GFP_MOVABLE, which is still just a suspicion of being the core reason for the problem). And the original DRM code had: GFP_HIGHUSER | __GFP_COLD | __GFP_FS | __GFP_RECLAIMABLE | __GFP_NORETRY | __GFP_NOWARN | __GFP_NOMEMALLOC which is not entirely sensible (__GFP_FS is already part of GFP_HIGHUSER, for example), and two of the flags (NORETRY and NOWARN) are the ones the driver wants to do conditionally. But that still leaves the question about __GFP_COLD (probably sane), __GFP_RECLAIMABLE (I wonder about that one) and __GFP_NOMEMALLOC (usually used together with NORETRY, and I'm not at all sure it makes sense as a base flag). So I suspect the final patch should not look like the one you tested, but instead likely have GFP_HIGHUSER | __GFP_COLD and possibly the __GFP_RECLAIMABLE flag too instead of just the bare __GFP_HIGHMEM.. (Well, we already had that __GFP_COLD there from before, so it's really about replacing __GFP_HIGHMEM with something like GFP_HIGHUSER | __GFP_RECLAIMABLE). But its' great to hear that this does seem to be the underlying cause. If you could test with that GFP_HIGHUSER | __GFP_RECLAIMABLE, that would be a good thing. After all - maybe the problem was triggered by some other flag than __GFP_MOVABLE, and as such, having some additional testing with a bigger set of allocation flags would be a really good thing. Linus ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm/i915: Selectively enable self-reclaim
On Fri, Jul 2, 2010 at 9:59 AM, Linus Torvalds torva...@linux-foundation.org wrote: On Thu, Jul 1, 2010 at 3:34 PM, M. Vefa Bicakci bic...@superonline.com wrote: Based on my testing, I am happy to report that the change you suggest fixes the memory corruption (segfaults) after thaw issue for me. I can't thank you enough times for this. Hey, goodie. And you're the one to be thanked - bisecting it down to that commit that wasn't _meant_ to have any real semantic changes (except for the bug-fix of racy mapping gfp-flags update) is what really cracked the lid on the problem. Now, the obligatory question: Could we have this fix applied to 2.6.32, 2.6.33 and 2.6.34 ? No problem, except we should first determine exactly what flags are the appropriate ones. My original patch was obviously not even compile-tested, and I actually meant for people to use GFP_HIGHUSER rather than __GFP_HIGHMEM. That contains all the regular allocation flags (but not the __GFP_MOVABLE, which is still just a suspicion of being the core reason for the problem). And the original DRM code had: GFP_HIGHUSER | __GFP_COLD | __GFP_FS | __GFP_RECLAIMABLE | __GFP_NORETRY | __GFP_NOWARN | __GFP_NOMEMALLOC which is not entirely sensible (__GFP_FS is already part of GFP_HIGHUSER, for example), and two of the flags (NORETRY and NOWARN) are the ones the driver wants to do conditionally. But that still leaves the question about __GFP_COLD (probably sane), __GFP_RECLAIMABLE (I wonder about that one) and __GFP_NOMEMALLOC (usually used together with NORETRY, and I'm not at all sure it makes sense as a base flag). So I suspect the final patch should not look like the one you tested, but instead likely have GFP_HIGHUSER | __GFP_COLD and possibly the __GFP_RECLAIMABLE flag too instead of just the bare __GFP_HIGHMEM.. (Well, we already had that __GFP_COLD there from before, so it's really about replacing __GFP_HIGHMEM with something like GFP_HIGHUSER | __GFP_RECLAIMABLE). But its' great to hear that this does seem to be the underlying cause. If you could test with that GFP_HIGHUSER | __GFP_RECLAIMABLE, that would be a good thing. After all - maybe the problem was triggered by some other flag than __GFP_MOVABLE, and as such, having some additional testing with a bigger set of allocation flags would be a really good thing. I just sent a patch I tested here with GFP_HIGHUSER | __GFP_COLD instead, and it resumes okay for me, I'll play with GFP_RECLAIMABLE now, If anyone wants to know why nobody uses hibernate, this laptop with a 4200rpm driver boots faster than the hibernate cycle. Dave. ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm/i915: Selectively enable self-reclaim
On Thu, Jul 1, 2010 at 5:49 PM, Dave Airlie airl...@gmail.com wrote: RECLAIMABLE added also seems fine, of course you can't have RECLAIMABLE and MOVABLE (I find this out when it oopses on boot). Yes. They are both flags for the anti-fragmentation code, and I think I'll leave the decision as to whether the i915 driver should use __GFP_RECLAIMABLE to the people who work with and care about the fragmentation issues. I doubt it matters much in practice, at least not for the loads that the fragmentation people tend to care most about. So I suspect MOVABLE is the problem. but I don't know enough about gfp flags to know what RECLAIMABLE buys us, and where it might bite us so I can test some more. I think I'll just apply your previous tested patch - GFP_HIGHUSER should take care of all the flags that matter fundamentally, and then the reclaimable flag is really just a small detail for others to worry about. Linus ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm/i915: Selectively enable self-reclaim
On Thu, Jan 28, 2010 at 3:19 AM, Linus Torvalds torva...@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. ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm/i915: Selectively enable self-reclaim
On Wed, 30 Jun 2010 16:54:07 +1000, Dave Airlie airl...@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 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm/i915: Selectively enable self-reclaim
On Wed, Jun 30, 2010 at 4:07 PM, Linus Torvalds
torva...@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))
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
