Re: [Intel-gfx] [PATCH] drm/i915: Selectively enable self-reclaim

["M. Vefa Bicakci" <bicave@superonline.com>]

On 01/07/10 04:24 AM, Linus Torvalds wrote:
> On Wed, Jun 30, 2010 at 4:07 PM, Linus Torvalds
> <torvalds@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

Dear Linus,

I made the code change you documented below to a vanilla 2.6.34 tree,
compiled it and tested hibernate/thaw cycles. In total, I tested
16 cycles, with 8 consecutive cycles in one installation (Debian Sid)
and 8 consecutive cycles in another one (Fedora 13). For every cycle,
I tried to run "old" and "new" programs, in terms of whether they were
run in previous cycles. I tried a few extra cycles with uswsusp as well.

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.

Now, the obligatory question: Could we have this fix applied to 2.6.32,
2.6.33 and 2.6.34 ?

Thanks a lot again!

M. Vefa Bicakci

--- linux-2.6.34/drivers/gpu/drm/i915/i915_gem.c.orig	2010-07-01 17:47:30.000000000 +0000
+++ linux-2.6.34/drivers/gpu/drm/i915/i915_gem.c	2010-07-01 17:54:03.000000000 +0000
@@ -2312,7 +2312,7 @@
 	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))