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

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

[Dave Airlie]

On Thu, Jan 28, 2010 at 3:19 AM, Linus Torvalds
<torvalds@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.

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

[Chris Wilson]

On Wed, 30 Jun 2010 16:54:07 +1000, Dave Airlie <airlied@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

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

[Linus Torvalds]

On Wed, Jun 30, 2010 at 12:05 AM, Chris Wilson <chris@chris-wilson.co.uk> wrote:
>
> 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.

Hmm. More likely is the __GFP_MOVABLE flag, I think.

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.

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:

-       /* Basically we want to disable the OOM killer and handle ENOMEM
-        * ourselves by sacrificing pages from cached buffers.
-        * XXX shmem_file_[gs]et_gfp_mask()
-        */
-       mapping_set_gfp_mask(obj->filp->f_path.dentry->d_inode->i_mapping,
-                            GFP_HIGHUSER |
-                            __GFP_COLD |
-                            __GFP_FS |
-                            __GFP_RECLAIMABLE |
-                            __GFP_NORETRY |
-                            __GFP_NOWARN |
-                            __GFP_NOMEMALLOC);

(and note how it doesn't have __GFP_MOVABLE set).

So I wonder if we shouldn't re-instate that mapping_set_gfp_mask() for
the _initial_ setting when the file descriptor is created. That part
wasn't the bug - the bug was the code that used to try to do that
whole per-allocation dance with the bits incorrectly (ie this part of
the change:

-               gfp = i915_gem_object_get_page_gfp_mask(obj);
-               i915_gem_object_set_page_gfp_mask(obj, gfp & ~__GFP_NORETRY);
-               ret = i915_gem_object_get_pages(obj);
-               i915_gem_object_set_page_gfp_mask (obj, gfp);

in that patch).

I could easily see that something would get very unhappy and corrupt
memory if the suspend-to-disk phase ends up compacting memory and
moving the pages around from under the i915 driver.

I dunno. But that seems more likely than the __GFP_NORETRY flag, which
should have no semantic meaning (except making it more likely for
allocations to fail, of course, but that's what the i915 code
_wanted_).

                      Linus

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

[Linus Torvalds]

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

---
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))

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

[KOSAKI Motohiro]

> 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?

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@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

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

[Dave Airlie]

On Thu, Jul 1, 2010 at 11:24 AM, Linus Torvalds
<torvalds@linux-foundation.org> 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?
>

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@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
>

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

[Chris Wilson]

On Wed, 30 Jun 2010 18:24:04 -0700, Linus Torvalds <torvalds@linux-foundation.org> 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:

That code I added with the original shrinker patch, and the flags lifted
from the shmem defaults, tweaked to what seemed sane with the addition of
NORETRY and friends. I see that i915 is unique in using shmem as the page
allocator, which perhaps explains why this failure is not observed with
the ttm drivers. ttm uses two sets of gfp mask: HIGHUSER and USER | DMA32.
So replacing the mapping_gfp_mask() with HIGHUSER would seem appropriate.
And the interaction of MOVABLE could explain why hibernate broke with the
introduction of GEM.

* turns to his trusty copy of LDD to explain the various meanings of
gfp_t...
-- 
Chris Wilson, Intel Open Source Technology Centre

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

[M. Vefa Bicakci]

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))

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

[Linus Torvalds]

On Thu, Jul 1, 2010 at 3:34 PM, M. Vefa Bicakci <bicave@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

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

[Dave Airlie]

On Fri, Jul 2, 2010 at 9:59 AM, Linus Torvalds
<torvalds@linux-foundation.org> wrote:
> On Thu, Jul 1, 2010 at 3:34 PM, M. Vefa Bicakci <bicave@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.

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

[Dave Airlie]

On Fri, Jul 2, 2010 at 10:06 AM, Dave Airlie <airlied@gmail.com> wrote:
> On Fri, Jul 2, 2010 at 9:59 AM, Linus Torvalds
> <torvalds@linux-foundation.org> wrote:
>> On Thu, Jul 1, 2010 at 3:34 PM, M. Vefa Bicakci <bicave@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.

RECLAIMABLE added also seems fine, of course you can't have
RECLAIMABLE and MOVABLE (I find this out when it oopses on boot).

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.

Dave.

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

[Linus Torvalds]

On Thu, Jul 1, 2010 at 5:49 PM, Dave Airlie <airlied@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

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

[M. Vefa Bicakci]

On 02/07/10 04:28 AM, Linus Torvalds wrote:
> On Thu, Jul 1, 2010 at 5:49 PM, Dave Airlie <airlied@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.
> 

Dear Linus,

I have bad news regarding your fix for self-reclaim and i915.

Apparently, I haven't tried enough hibernate/thaw cycles while
initially testing your fix.

After applying your fix to 2.6.34.1 and using it for two weeks,
I noticed that every now and then I get a black screen or random
kernel errors after thawing. I thought maybe this might be the
same problem caused by d8e0902806c0bd2ccc4f6a267ff52565a3ec933b .
(It turns out that my guess was right.)

So I compiled two vanilla 2.6.34.1 kernels. One with
d8e0902806c0bd2ccc4f6a267ff52565a3ec933b reverted to get back
to pre 2.6.32.8 state, and another one with your fix applied.

Then I set up an automated process where the computer would
hibernate, and reboot at the end of the hibernation sequence
(by setting /sys/power/disk to reboot) and then thaw back.
I made this process loop at least 20 times.

The kernel with d8e0902806c0bd2ccc4f6a267ff52565a3ec933b reverted
was able to hibernate/thaw at least 40 times in one go, while
the one with your fix applied was able to hibernate/thaw at most
17 times (in two separate trials) after which it crashed during
the next thaw.

Is there anything I can do find out the correct flags to use
in addition to GFP_HIGHUSER ? Can I do something like a bisection
for the flags one by one starting from the pre 2.6.32.8 state?
If you could outline a procedure to do this, I would be glad to
follow it.

Sorry to bug you again about this problem because of incomplete
testing on my part.

Regards,

M. Vefa Bicakci

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

[Linus Torvalds]

[-- Attachment #1: Type: text/plain, Size: 1998 bytes --]

On Sat, Jul 17, 2010 at 11:58 AM, M. Vefa Bicakci
<bicave@superonline.com> wrote:
>
> The kernel with d8e0902806c0bd2ccc4f6a267ff52565a3ec933b reverted
> was able to hibernate/thaw at least 40 times in one go, while
> the one with your fix applied was able to hibernate/thaw at most
> 17 times (in two separate trials) after which it crashed during
> the next thaw.

Ok. I do wonder if the bug is possibly something entirely different,
and the allocation patterns just happen to expose/hide it. Reverting
the original commit should be pretty darn close to applying my fix.
Any remaining issues would seem to be more about the actual bug in the
original code (racing on changing that mapping->gfp_mask witthout any
locking) than about anything else.

> Is there anything I can do find out the correct flags to use
> in addition to GFP_HIGHUSER ? Can I do something like a bisection
> for the flags one by one starting from the pre 2.6.32.8 state?
> If you could outline a procedure to do this, I would be glad to
> follow it.

You can try adding __GFP_RECLAIMABLE | __GFP_NOMEMALLOC to the set of
flags in i915_gem_object_get_pages(). That's what the old code had
(and then it played games with NORETRY|NOWARN). I've attached a patch
(UNTESTED! Maybe it won't compile).

Now, I don't see why those flags would matter, but NOMEMALLOC in
particular does make a difference for memory allocation patterns under
low memory conditions, so maybe it could make a difference.

And if it _does_ make a difference, it would be interesting to know
which of the two flags matter. So try both flags first, and see if
that gets you something reliable. And if it does, remove one of them
and try again - just to see _which_ flag it is that the i915 driver
would care about. That would hopefully give us a hint.

> Sorry to bug you again about this problem because of incomplete
> testing on my part.

Oh, never be sorry for testing even more, and testing something nobody
else bothered to test.

             Linus

[-- Attachment #2: diff --]
[-- Type: application/octet-stream, Size: 560 bytes --]

 drivers/gpu/drm/i915/i915_gem.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 0743858..98496d4 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -2241,6 +2241,8 @@ i915_gem_object_get_pages(struct drm_gem_object *obj,
 		page = read_cache_page_gfp(mapping, i,
 					   GFP_HIGHUSER |
 					   __GFP_COLD |
+					   __GFP_RECLAIMABLE |
+					   __GFP_NOMEMALLOC |
 					   gfpmask);
 		if (IS_ERR(page))
 			goto err_pages;

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

[M. Vefa Bicakci]

On 17/07/10 10:15 PM, Linus Torvalds wrote:
> On Sat, Jul 17, 2010 at 11:58 AM, M. Vefa Bicakci
> <bicave@superonline.com> wrote:
>>
>> The kernel with d8e0902806c0bd2ccc4f6a267ff52565a3ec933b reverted
>> was able to hibernate/thaw at least 40 times in one go, while
>> the one with your fix applied was able to hibernate/thaw at most
>> 17 times (in two separate trials) after which it crashed during
>> the next thaw.
> 
> Ok. I do wonder if the bug is possibly something entirely different,
> and the allocation patterns just happen to expose/hide it. Reverting
> the original commit should be pretty darn close to applying my fix.
> Any remaining issues would seem to be more about the actual bug in the
> original code (racing on changing that mapping->gfp_mask witthout any
> locking) than about anything else.
> 
>> Is there anything I can do find out the correct flags to use
>> in addition to GFP_HIGHUSER ? Can I do something like a bisection
>> for the flags one by one starting from the pre 2.6.32.8 state?
>> If you could outline a procedure to do this, I would be glad to
>> follow it.
> 
> You can try adding __GFP_RECLAIMABLE | __GFP_NOMEMALLOC to the set of
> flags in i915_gem_object_get_pages(). That's what the old code had
> (and then it played games with NORETRY|NOWARN). I've attached a patch
> (UNTESTED! Maybe it won't compile).
> 
> Now, I don't see why those flags would matter, but NOMEMALLOC in
> particular does make a difference for memory allocation patterns under
> low memory conditions, so maybe it could make a difference.
> 
> And if it _does_ make a difference, it would be interesting to know
> which of the two flags matter. So try both flags first, and see if
> that gets you something reliable. And if it does, remove one of them
> and try again - just to see _which_ flag it is that the i915 driver
> would care about. That would hopefully give us a hint.

Dear Linus,

After hours of testing I came up with the following result: We need
to have the __GFP_RECLAIMABLE flag in addition to GFP_HIGHUSER.

First I tested a kernel with both flags added to your fix. I was able
to get more than 60 hibernate/thaw cycles without any errors, so
I thought that was good.

Then I tried a kernel with __GFP_NOMEMALLOC, and I found out that
this kernel wasn't very reliable. In the first trial run, I got a
crash in the second thaw. (Magic Sys-Rq did work.) In the second
trial run, I got a Xorg related kernel Oops in the 12th thaw.
Therefore I concluded that having only __GFP_NOMEMALLOC in addition
to GFP_HIGHUSER was not good enough.

Finally, I tested a kernel with __GFP_RECLAIMABLE. For this one, I did
two trial runs, each with 60 hibernate/thaw cycles. I had no problems
during these runs, so I concluded that __GFP_RECLAIMABLE is the key
flag to use in addition to GFP_HIGHUSER and __GFP_COLD.

I think in a previous e-mail you were suggesting that __GFP_RECLAIMABLE
could be optionally needed for a few technical reasons. To be honest, I
have no idea why it looks like it is needed for proper operation.

As always, it is great to report test results. Hopefully this time I did
enough amount of tests.

Regards,

M. Vefa Bicakci

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

[Linus Torvalds]

On Sun, Jul 18, 2010 at 7:27 AM, M. Vefa Bicakci <bicave@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