Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1410656 > unrolled thread

Re: BUG: scheduling while atomic: cron/668/0x10c9a0c0 (was: Re: mm, page_alloc: avoid looking up the first zone in a zonelist twice)

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: BUG: scheduling while atomic: cron/668/0x10c9a0c0 (was: Re: mm,  page_alloc: avoid looking up the first zone in a zonelist twice) Vlastimil Babka <vbabka@suse.cz> - 2016-05-31 23:50 +0200
    Re: BUG: scheduling while atomic: cron/668/0x10c9a0c0 (was: Re: mm,  page_alloc: avoid looking up the first zone in a zonelist twice) Mel Gorman <mgorman@techsingularity.net> - 2016-06-01 11:20 +0200
      Re: BUG: scheduling while atomic: cron/668/0x10c9a0c0 Vlastimil Babka <vbabka@suse.cz> - 2016-06-01 12:10 +0200
        Re: BUG: scheduling while atomic: cron/668/0x10c9a0c0 Mel Gorman <mgorman@techsingularity.net> - 2016-06-02 12:40 +0200
          Re: BUG: scheduling while atomic: cron/668/0x10c9a0c0 Vlastimil Babka <vbabka@suse.cz> - 2016-06-02 14:10 +0200
            Re: BUG: scheduling while atomic: cron/668/0x10c9a0c0 Mel Gorman <mgorman@techsingularity.net> - 2016-06-02 14:20 +0200
              Re: BUG: scheduling while atomic: cron/668/0x10c9a0c0 Andrew Morton <akpm@linux-foundation.org> - 2016-06-02 20:50 +0200
                Re: BUG: scheduling while atomic: cron/668/0x10c9a0c0 Stephen Rothwell <sfr@canb.auug.org.au> - 2016-06-03 06:00 +0200
                Re: BUG: scheduling while atomic: cron/668/0x10c9a0c0 Geert Uytterhoeven <geert@linux-m68k.org> - 2016-06-03 10:00 +0200
                  Re: BUG: scheduling while atomic: cron/668/0x10c9a0c0 Mel Gorman <mgorman@techsingularity.net> - 2016-06-03 10:50 +0200
                    Re: BUG: scheduling while atomic: cron/668/0x10c9a0c0 Geert Uytterhoeven <geert@linux-m68k.org> - 2016-06-03 11:10 +0200
                      Re: BUG: scheduling while atomic: cron/668/0x10c9a0c0 Andrew Morton <akpm@linux-foundation.org> - 2016-06-03 18:40 +0200
                        Re: BUG: scheduling while atomic: cron/668/0x10c9a0c0 Mel Gorman <mgorman@techsingularity.net> - 2016-06-03 18:50 +0200
                          Re: BUG: scheduling while atomic: cron/668/0x10c9a0c0 Andrew Morton <akpm@linux-foundation.org> - 2016-06-03 19:00 +0200

#1410656 — Re: BUG: scheduling while atomic: cron/668/0x10c9a0c0 (was: Re: mm, page_alloc: avoid looking up the first zone in a zonelist twice)

On 05/30/2016 05:56 PM, Mel Gorman wrote:
> diff --git a/mm/page_alloc.c b/mm/page_alloc.c
> index dba8cfd0b2d6..f2c1e47adc11 100644
> --- a/mm/page_alloc.c
> +++ b/mm/page_alloc.c
> @@ -3232,6 +3232,9 @@ __alloc_pages_slowpath(gfp_t gfp_mask, unsigned int order,
>  		 * allocations are system rather than user orientated
>  		 */
>  		ac->zonelist = node_zonelist(numa_node_id(), gfp_mask);
> +		ac->preferred_zoneref = first_zones_zonelist(ac->zonelist,
> +					ac->high_zoneidx, ac->nodemask);
> +		ac->classzone_idx = zonelist_zone_idx(ac->preferred_zoneref);
>  		page = get_page_from_freelist(gfp_mask, order,
>  						ALLOC_NO_WATERMARKS, ac);
>  		if (page)
> 

Even if that didn't help for this report, I think it's needed too
(except the classzone_idx which doesn't exist anymore?).

And I think the following as well. (the changed comment could be also
just deleted).


diff --git a/mm/page_alloc.c b/mm/page_alloc.c
index f8f3bfc435ee..0a8d8e2bf331 100644
--- a/mm/page_alloc.c
+++ b/mm/page_alloc.c
@@ -3808,7 +3808,7 @@ __alloc_pages_nodemask(gfp_t gfp_mask, unsigned
int order,
        /* Dirty zone balancing only done in the fast path */
        ac.spread_dirty_pages = (gfp_mask & __GFP_WRITE);

-       /* The preferred zone is used for statistics later */
+       /* The preferred zone is crucial for get_page_from_freelist */
        ac.preferred_zoneref = first_zones_zonelist(ac.zonelist,
                                        ac.high_zoneidx, ac.nodemask);
        if (!ac.preferred_zoneref) {
@@ -3832,8 +3832,11 @@ __alloc_pages_nodemask(gfp_t gfp_mask, unsigned
int order,
         * Restore the original nodemask if it was potentially replaced with
         * &cpuset_current_mems_allowed to optimize the fast-path attempt.
         */
-       if (cpusets_enabled())
+       if (cpusets_enabled()) {
                ac.nodemask = nodemask;
+               ac.preferred_zoneref = first_zones_zonelist(ac.zonelist,
+                                       ac.high_zoneidx, ac.nodemask);
+       }
        page = __alloc_pages_slowpath(alloc_mask, order, &ac);

[toc] | [next] | [standalone]

#1411010

On Tue, May 31, 2016 at 11:44:24PM +0200, Vlastimil Babka wrote:
> On 05/30/2016 05:56 PM, Mel Gorman wrote:
> > diff --git a/mm/page_alloc.c b/mm/page_alloc.c
> > index dba8cfd0b2d6..f2c1e47adc11 100644
> > --- a/mm/page_alloc.c
> > +++ b/mm/page_alloc.c
> > @@ -3232,6 +3232,9 @@ __alloc_pages_slowpath(gfp_t gfp_mask, unsigned int order,
> >  		 * allocations are system rather than user orientated
> >  		 */
> >  		ac->zonelist = node_zonelist(numa_node_id(), gfp_mask);
> > +		ac->preferred_zoneref = first_zones_zonelist(ac->zonelist,
> > +					ac->high_zoneidx, ac->nodemask);
> > +		ac->classzone_idx = zonelist_zone_idx(ac->preferred_zoneref);
> >  		page = get_page_from_freelist(gfp_mask, order,
> >  						ALLOC_NO_WATERMARKS, ac);
> >  		if (page)
> > 
> 
> Even if that didn't help for this report, I think it's needed too
> (except the classzone_idx which doesn't exist anymore?).
> 
> And I think the following as well. (the changed comment could be also
> just deleted).
> 

Why?

The comment is fine but I do not see why the recalculation would occur.

In the original code, the preferred_zoneref for statistics is calculated
based on either the supplied nodemask or cpuset_current_mems_allowed during
the initial attempt. It then relies on the cpuset checks in the slowpath
to encorce mems_allowed but the preferred zone doesn't change.

With your proposed change, it's possible that the
preferred_zoneref recalculation points to a zoneref disallowed by
cpuset_current_mems_sllowed. While it'll be skipped during allocation,
the statistics will still be against a zone that is potentially outside
what is allowed.

-- 
Mel Gorman
SUSE Labs

[toc] | [prev] | [next] | [standalone]

#1411053 — Re: BUG: scheduling while atomic: cron/668/0x10c9a0c0

On 06/01/2016 11:19 AM, Mel Gorman wrote:
> On Tue, May 31, 2016 at 11:44:24PM +0200, Vlastimil Babka wrote:
>> On 05/30/2016 05:56 PM, Mel Gorman wrote:
>>> diff --git a/mm/page_alloc.c b/mm/page_alloc.c
>>> index dba8cfd0b2d6..f2c1e47adc11 100644
>>> --- a/mm/page_alloc.c
>>> +++ b/mm/page_alloc.c
>>> @@ -3232,6 +3232,9 @@ __alloc_pages_slowpath(gfp_t gfp_mask, unsigned int order,
>>>  		 * allocations are system rather than user orientated
>>>  		 */
>>>  		ac->zonelist = node_zonelist(numa_node_id(), gfp_mask);
>>> +		ac->preferred_zoneref = first_zones_zonelist(ac->zonelist,
>>> +					ac->high_zoneidx, ac->nodemask);
>>> +		ac->classzone_idx = zonelist_zone_idx(ac->preferred_zoneref);
>>>  		page = get_page_from_freelist(gfp_mask, order,
>>>  						ALLOC_NO_WATERMARKS, ac);
>>>  		if (page)
>>>
>>
>> Even if that didn't help for this report, I think it's needed too
>> (except the classzone_idx which doesn't exist anymore?).

But you agree that the hunk above should be merged?

>> And I think the following as well. (the changed comment could be also
>> just deleted).
>>
> 
> Why?
> 
> The comment is fine but I do not see why the recalculation would occur.
> 
> In the original code, the preferred_zoneref for statistics is calculated
> based on either the supplied nodemask or cpuset_current_mems_allowed during
> the initial attempt. It then relies on the cpuset checks in the slowpath
> to encorce mems_allowed but the preferred zone doesn't change.
> 
> With your proposed change, it's possible that the
> preferred_zoneref recalculation points to a zoneref disallowed by
> cpuset_current_mems_sllowed. While it'll be skipped during allocation,
> the statistics will still be against a zone that is potentially outside
> what is allowed.

Hmm that's true and I was ready to agree. But then I noticed  that
gfp_to_alloc_flags() can mask out ALLOC_CPUSET for GFP_ATOMIC. So it's
like a lighter version of the ALLOC_NO_WATERMARKS situation. In that
case it's wrong if we leave ac->preferred_zoneref at a position that has
skipped some zones due to mempolicies?

[toc] | [prev] | [next] | [standalone]

#1412111 — Re: BUG: scheduling while atomic: cron/668/0x10c9a0c0

On Wed, Jun 01, 2016 at 12:01:24PM +0200, Vlastimil Babka wrote:
> > Why?
> > 
> > The comment is fine but I do not see why the recalculation would occur.
> > 
> > In the original code, the preferred_zoneref for statistics is calculated
> > based on either the supplied nodemask or cpuset_current_mems_allowed during
> > the initial attempt. It then relies on the cpuset checks in the slowpath
> > to encorce mems_allowed but the preferred zone doesn't change.
> > 
> > With your proposed change, it's possible that the
> > preferred_zoneref recalculation points to a zoneref disallowed by
> > cpuset_current_mems_sllowed. While it'll be skipped during allocation,
> > the statistics will still be against a zone that is potentially outside
> > what is allowed.
> 
> Hmm that's true and I was ready to agree. But then I noticed  that
> gfp_to_alloc_flags() can mask out ALLOC_CPUSET for GFP_ATOMIC. So it's
> like a lighter version of the ALLOC_NO_WATERMARKS situation. In that
> case it's wrong if we leave ac->preferred_zoneref at a position that has
> skipped some zones due to mempolicies?
> 

So both options are wrong then. How about this?

---8<---
mm, page_alloc: Recalculate the preferred zoneref if the context can ignore memory policies

The optimistic fast path may use cpuset_current_mems_allowed instead of
of a NULL nodemask supplied by the caller for cpuset allocations. The
preferred zone is calculated on this basis for statistic purposes and
as a starting point in the zonelist iterator.

However, if the context can ignore memory policies due to being atomic or
being able to ignore watermarks then the starting point in the zonelist
iterator is no longer correct. This patch resets the zonelist iterator in
the allocator slowpath if the context can ignore memory policies. This will
alter the zone used for statistics but only after it is known that it makes
sense for that context. Resetting it before entering the slowpath would
potentially allow an ALLOC_CPUSET allocation to be accounted for against
the wrong zone. Note that while nodemask is not explicitly set to the
original nodemask, it would only have been overwritten if cpuset_enabled()
and it was reset before the slowpath was entered.

Signed-off-by: Mel Gorman <mgorman@techsingularity.net>
---
 mm/page_alloc.c | 23 ++++++++++++++++-------
 1 file changed, 16 insertions(+), 7 deletions(-)

diff --git a/mm/page_alloc.c b/mm/page_alloc.c
index 557549c81083..b17358617a1b 100644
--- a/mm/page_alloc.c
+++ b/mm/page_alloc.c
@@ -3598,6 +3598,17 @@ __alloc_pages_slowpath(gfp_t gfp_mask, unsigned int order,
 	 */
 	alloc_flags = gfp_to_alloc_flags(gfp_mask);
 
+	/*
+	 * Reset the zonelist iterators if memory policies can be ignored.
+	 * These allocations are high priority and system rather than user
+	 * orientated.
+	 */
+	if ((alloc_flags & ALLOC_NO_WATERMARKS) || !(alloc_flags & ALLOC_CPUSET)) {
+		ac->zonelist = node_zonelist(numa_node_id(), gfp_mask);
+		ac->preferred_zoneref = first_zones_zonelist(ac->zonelist,
+					ac->high_zoneidx, ac->nodemask);
+	}
+
 	/* This is the last chance, in general, before the goto nopage. */
 	page = get_page_from_freelist(gfp_mask, order,
 				alloc_flags & ~ALLOC_NO_WATERMARKS, ac);
@@ -3606,12 +3617,6 @@ __alloc_pages_slowpath(gfp_t gfp_mask, unsigned int order,
 
 	/* Allocate without watermarks if the context allows */
 	if (alloc_flags & ALLOC_NO_WATERMARKS) {
-		/*
-		 * Ignore mempolicies if ALLOC_NO_WATERMARKS on the grounds
-		 * the allocation is high priority and these type of
-		 * allocations are system rather than user orientated
-		 */
-		ac->zonelist = node_zonelist(numa_node_id(), gfp_mask);
 		page = get_page_from_freelist(gfp_mask, order,
 						ALLOC_NO_WATERMARKS, ac);
 		if (page)
@@ -3810,7 +3815,11 @@ __alloc_pages_nodemask(gfp_t gfp_mask, unsigned int order,
 	/* Dirty zone balancing only done in the fast path */
 	ac.spread_dirty_pages = (gfp_mask & __GFP_WRITE);
 
-	/* The preferred zone is used for statistics later */
+	/*
+	 * The preferred zone is used for statistics but crucially it is
+	 * also used as the starting point for the zonelist iterator. It
+	 * may get reset for allocations that ignore memory policies.
+	 */
 	ac.preferred_zoneref = first_zones_zonelist(ac.zonelist,
 					ac.high_zoneidx, ac.nodemask);
 	if (!ac.preferred_zoneref) {

[toc] | [prev] | [next] | [standalone]

#1412175 — Re: BUG: scheduling while atomic: cron/668/0x10c9a0c0

On 06/02/2016 12:39 PM, Mel Gorman wrote:
> On Wed, Jun 01, 2016 at 12:01:24PM +0200, Vlastimil Babka wrote:
>>> Why?
>>>
>>> The comment is fine but I do not see why the recalculation would occur.
>>>
>>> In the original code, the preferred_zoneref for statistics is calculated
>>> based on either the supplied nodemask or cpuset_current_mems_allowed during
>>> the initial attempt. It then relies on the cpuset checks in the slowpath
>>> to encorce mems_allowed but the preferred zone doesn't change.
>>>
>>> With your proposed change, it's possible that the
>>> preferred_zoneref recalculation points to a zoneref disallowed by
>>> cpuset_current_mems_sllowed. While it'll be skipped during allocation,
>>> the statistics will still be against a zone that is potentially outside
>>> what is allowed.
>>
>> Hmm that's true and I was ready to agree. But then I noticed  that
>> gfp_to_alloc_flags() can mask out ALLOC_CPUSET for GFP_ATOMIC. So it's
>> like a lighter version of the ALLOC_NO_WATERMARKS situation. In that
>> case it's wrong if we leave ac->preferred_zoneref at a position that has
>> skipped some zones due to mempolicies?
>>
>
> So both options are wrong then. How about this?

I wonder if the original patch we're fixing was worth all this trouble 
(and more
for my compaction priority series :), but yeah this should work.

> ---8<---
> mm, page_alloc: Recalculate the preferred zoneref if the context can ignore memory policies
>
> The optimistic fast path may use cpuset_current_mems_allowed instead of
> of a NULL nodemask supplied by the caller for cpuset allocations. The
> preferred zone is calculated on this basis for statistic purposes and
> as a starting point in the zonelist iterator.
>
> However, if the context can ignore memory policies due to being atomic or
> being able to ignore watermarks then the starting point in the zonelist
> iterator is no longer correct. This patch resets the zonelist iterator in
> the allocator slowpath if the context can ignore memory policies. This will
> alter the zone used for statistics but only after it is known that it makes
> sense for that context. Resetting it before entering the slowpath would
> potentially allow an ALLOC_CPUSET allocation to be accounted for against
> the wrong zone. Note that while nodemask is not explicitly set to the
> original nodemask, it would only have been overwritten if cpuset_enabled()
> and it was reset before the slowpath was entered.
>
> Signed-off-by: Mel Gorman <mgorman@techsingularity.net>

Acked-by: Vlastimil Babka <vbabka@suse.cz>

[toc] | [prev] | [next] | [standalone]

#1412178 — Re: BUG: scheduling while atomic: cron/668/0x10c9a0c0

On Thu, Jun 02, 2016 at 02:04:42PM +0200, Vlastimil Babka wrote:
> On 06/02/2016 12:39 PM, Mel Gorman wrote:
> >On Wed, Jun 01, 2016 at 12:01:24PM +0200, Vlastimil Babka wrote:
> >>>Why?
> >>>
> >>>The comment is fine but I do not see why the recalculation would occur.
> >>>
> >>>In the original code, the preferred_zoneref for statistics is calculated
> >>>based on either the supplied nodemask or cpuset_current_mems_allowed during
> >>>the initial attempt. It then relies on the cpuset checks in the slowpath
> >>>to encorce mems_allowed but the preferred zone doesn't change.
> >>>
> >>>With your proposed change, it's possible that the
> >>>preferred_zoneref recalculation points to a zoneref disallowed by
> >>>cpuset_current_mems_sllowed. While it'll be skipped during allocation,
> >>>the statistics will still be against a zone that is potentially outside
> >>>what is allowed.
> >>
> >>Hmm that's true and I was ready to agree. But then I noticed  that
> >>gfp_to_alloc_flags() can mask out ALLOC_CPUSET for GFP_ATOMIC. So it's
> >>like a lighter version of the ALLOC_NO_WATERMARKS situation. In that
> >>case it's wrong if we leave ac->preferred_zoneref at a position that has
> >>skipped some zones due to mempolicies?
> >>
> >
> >So both options are wrong then. How about this?
> 
> I wonder if the original patch we're fixing was worth all this trouble (and
> more
> for my compaction priority series :), but yeah this should work.
> 

I considered that option when the bug report first came in. It was a 2%
hit to the page allocator microbenchmark to revert it. More than I expected
but enough to care. If this causes another problem, I'll revert it as
there will be other options later.

> >---8<---
> >mm, page_alloc: Recalculate the preferred zoneref if the context can ignore memory policies
> >
> >The optimistic fast path may use cpuset_current_mems_allowed instead of
> >of a NULL nodemask supplied by the caller for cpuset allocations. The
> >preferred zone is calculated on this basis for statistic purposes and
> >as a starting point in the zonelist iterator.
> >
> >However, if the context can ignore memory policies due to being atomic or
> >being able to ignore watermarks then the starting point in the zonelist
> >iterator is no longer correct. This patch resets the zonelist iterator in
> >the allocator slowpath if the context can ignore memory policies. This will
> >alter the zone used for statistics but only after it is known that it makes
> >sense for that context. Resetting it before entering the slowpath would
> >potentially allow an ALLOC_CPUSET allocation to be accounted for against
> >the wrong zone. Note that while nodemask is not explicitly set to the
> >original nodemask, it would only have been overwritten if cpuset_enabled()
> >and it was reset before the slowpath was entered.
> >
> >Signed-off-by: Mel Gorman <mgorman@techsingularity.net>
> 
> Acked-by: Vlastimil Babka <vbabka@suse.cz>
> 

Thanks.

-- 
Mel Gorman
SUSE Labs

[toc] | [prev] | [next] | [standalone]

#1412497 — Re: BUG: scheduling while atomic: cron/668/0x10c9a0c0

On Thu, 2 Jun 2016 13:19:36 +0100 Mel Gorman <mgorman@techsingularity.net> wrote:

> > >Signed-off-by: Mel Gorman <mgorman@techsingularity.net>
> > 
> > Acked-by: Vlastimil Babka <vbabka@suse.cz>
> > 
> 
> Thanks.

I queued this.  A tested-by:Geert would be nice?


From: Mel Gorman <mgorman@techsingularity.net>
Subject: mm, page_alloc: recalculate the preferred zoneref if the context can ignore memory policies

The optimistic fast path may use cpuset_current_mems_allowed instead of of
a NULL nodemask supplied by the caller for cpuset allocations.  The
preferred zone is calculated on this basis for statistic purposes and as a
starting point in the zonelist iterator.

However, if the context can ignore memory policies due to being atomic or
being able to ignore watermarks then the starting point in the zonelist
iterator is no longer correct.  This patch resets the zonelist iterator in
the allocator slowpath if the context can ignore memory policies.  This
will alter the zone used for statistics but only after it is known that it
makes sense for that context.  Resetting it before entering the slowpath
would potentially allow an ALLOC_CPUSET allocation to be accounted for
against the wrong zone.  Note that while nodemask is not explicitly set to
the original nodemask, it would only have been overwritten if
cpuset_enabled() and it was reset before the slowpath was entered.

Link: http://lkml.kernel.org/r/20160602103936.GU2527@techsingularity.net
Fixes: c33d6c06f60f710 ("mm, page_alloc: avoid looking up the first zone in a zonelist twice")
Signed-off-by: Mel Gorman <mgorman@techsingularity.net>
Reported-by: Geert Uytterhoeven <geert@linux-m68k.org>
Acked-by: Vlastimil Babka <vbabka@suse.cz>
Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
---

 mm/page_alloc.c |   23 ++++++++++++++++-------
 1 file changed, 16 insertions(+), 7 deletions(-)

diff -puN mm/page_alloc.c~mm-page_alloc-recalculate-the-preferred-zoneref-if-the-context-can-ignore-memory-policies mm/page_alloc.c
--- a/mm/page_alloc.c~mm-page_alloc-recalculate-the-preferred-zoneref-if-the-context-can-ignore-memory-policies
+++ a/mm/page_alloc.c
@@ -3604,6 +3604,17 @@ retry:
 	 */
 	alloc_flags = gfp_to_alloc_flags(gfp_mask);
 
+	/*
+	 * Reset the zonelist iterators if memory policies can be ignored.
+	 * These allocations are high priority and system rather than user
+	 * orientated.
+	 */
+	if ((alloc_flags & ALLOC_NO_WATERMARKS) || !(alloc_flags & ALLOC_CPUSET)) {
+		ac->zonelist = node_zonelist(numa_node_id(), gfp_mask);
+		ac->preferred_zoneref = first_zones_zonelist(ac->zonelist,
+					ac->high_zoneidx, ac->nodemask);
+	}
+
 	/* This is the last chance, in general, before the goto nopage. */
 	page = get_page_from_freelist(gfp_mask, order,
 				alloc_flags & ~ALLOC_NO_WATERMARKS, ac);
@@ -3612,12 +3623,6 @@ retry:
 
 	/* Allocate without watermarks if the context allows */
 	if (alloc_flags & ALLOC_NO_WATERMARKS) {
-		/*
-		 * Ignore mempolicies if ALLOC_NO_WATERMARKS on the grounds
-		 * the allocation is high priority and these type of
-		 * allocations are system rather than user orientated
-		 */
-		ac->zonelist = node_zonelist(numa_node_id(), gfp_mask);
 		page = get_page_from_freelist(gfp_mask, order,
 						ALLOC_NO_WATERMARKS, ac);
 		if (page)
@@ -3816,7 +3821,11 @@ retry_cpuset:
 	/* Dirty zone balancing only done in the fast path */
 	ac.spread_dirty_pages = (gfp_mask & __GFP_WRITE);
 
-	/* The preferred zone is used for statistics later */
+	/*
+	 * The preferred zone is used for statistics but crucially it is
+	 * also used as the starting point for the zonelist iterator. It
+	 * may get reset for allocations that ignore memory policies.
+	 */
 	ac.preferred_zoneref = first_zones_zonelist(ac.zonelist,
 					ac.high_zoneidx, ac.nodemask);
 	if (!ac.preferred_zoneref) {
_

[toc] | [prev] | [next] | [standalone]

#1412773 — Re: BUG: scheduling while atomic: cron/668/0x10c9a0c0

Hi Andrew,

On Thu, 2 Jun 2016 11:43:41 -0700 Andrew Morton <akpm@linux-foundation.org> wrote:
>
> On Thu, 2 Jun 2016 13:19:36 +0100 Mel Gorman <mgorman@techsingularity.net> wrote:
> 
> > > >Signed-off-by: Mel Gorman <mgorman@techsingularity.net>  
> > > 
> > > Acked-by: Vlastimil Babka <vbabka@suse.cz>
> > >   
> > 
> > Thanks.  
> 
> I queued this.  A tested-by:Geert would be nice?
> 
> 
> From: Mel Gorman <mgorman@techsingularity.net>
> Subject: mm, page_alloc: recalculate the preferred zoneref if the context can ignore memory policies

I dumped that into linux-next today as well.

-- 
Cheers,
Stephen Rothwell

[toc] | [prev] | [next] | [standalone]

#1412881 — Re: BUG: scheduling while atomic: cron/668/0x10c9a0c0

Hi Andrew, Mel,

On Thu, Jun 2, 2016 at 8:43 PM, Andrew Morton <akpm@linux-foundation.org> wrote:
> On Thu, 2 Jun 2016 13:19:36 +0100 Mel Gorman <mgorman@techsingularity.net> wrote:
>> > >Signed-off-by: Mel Gorman <mgorman@techsingularity.net>
>> >
>> > Acked-by: Vlastimil Babka <vbabka@suse.cz>
>> >
>>
>> Thanks.
>
> I queued this.  A tested-by:Geert would be nice?
>
>
> From: Mel Gorman <mgorman@techsingularity.net>
> Subject: mm, page_alloc: recalculate the preferred zoneref if the context can ignore memory policies
>
> The optimistic fast path may use cpuset_current_mems_allowed instead of of
> a NULL nodemask supplied by the caller for cpuset allocations.  The
> preferred zone is calculated on this basis for statistic purposes and as a
> starting point in the zonelist iterator.
>
> However, if the context can ignore memory policies due to being atomic or
> being able to ignore watermarks then the starting point in the zonelist
> iterator is no longer correct.  This patch resets the zonelist iterator in
> the allocator slowpath if the context can ignore memory policies.  This
> will alter the zone used for statistics but only after it is known that it
> makes sense for that context.  Resetting it before entering the slowpath
> would potentially allow an ALLOC_CPUSET allocation to be accounted for
> against the wrong zone.  Note that while nodemask is not explicitly set to
> the original nodemask, it would only have been overwritten if
> cpuset_enabled() and it was reset before the slowpath was entered.
>
> Link: http://lkml.kernel.org/r/20160602103936.GU2527@techsingularity.net
> Fixes: c33d6c06f60f710 ("mm, page_alloc: avoid looking up the first zone in a zonelist twice")

My understanding was that this was an an additional patch, not fixing
the problem in-se?

Indeed, after applying this patch (without the other one that added
"z = ac->preferred_zoneref;" to the reset_fair block of
get_page_from_freelist()) I still get crashes...

Now testing with both applied...

> Signed-off-by: Mel Gorman <mgorman@techsingularity.net>
> Reported-by: Geert Uytterhoeven <geert@linux-m68k.org>
> Acked-by: Vlastimil Babka <vbabka@suse.cz>
> Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
> ---
>
>  mm/page_alloc.c |   23 ++++++++++++++++-------
>  1 file changed, 16 insertions(+), 7 deletions(-)
>
> diff -puN mm/page_alloc.c~mm-page_alloc-recalculate-the-preferred-zoneref-if-the-context-can-ignore-memory-policies mm/page_alloc.c
> --- a/mm/page_alloc.c~mm-page_alloc-recalculate-the-preferred-zoneref-if-the-context-can-ignore-memory-policies
> +++ a/mm/page_alloc.c
> @@ -3604,6 +3604,17 @@ retry:
>          */
>         alloc_flags = gfp_to_alloc_flags(gfp_mask);
>
> +       /*
> +        * Reset the zonelist iterators if memory policies can be ignored.
> +        * These allocations are high priority and system rather than user
> +        * orientated.
> +        */
> +       if ((alloc_flags & ALLOC_NO_WATERMARKS) || !(alloc_flags & ALLOC_CPUSET)) {
> +               ac->zonelist = node_zonelist(numa_node_id(), gfp_mask);
> +               ac->preferred_zoneref = first_zones_zonelist(ac->zonelist,
> +                                       ac->high_zoneidx, ac->nodemask);
> +       }
> +
>         /* This is the last chance, in general, before the goto nopage. */
>         page = get_page_from_freelist(gfp_mask, order,
>                                 alloc_flags & ~ALLOC_NO_WATERMARKS, ac);
> @@ -3612,12 +3623,6 @@ retry:
>
>         /* Allocate without watermarks if the context allows */
>         if (alloc_flags & ALLOC_NO_WATERMARKS) {
> -               /*
> -                * Ignore mempolicies if ALLOC_NO_WATERMARKS on the grounds
> -                * the allocation is high priority and these type of
> -                * allocations are system rather than user orientated
> -                */
> -               ac->zonelist = node_zonelist(numa_node_id(), gfp_mask);
>                 page = get_page_from_freelist(gfp_mask, order,
>                                                 ALLOC_NO_WATERMARKS, ac);
>                 if (page)
> @@ -3816,7 +3821,11 @@ retry_cpuset:
>         /* Dirty zone balancing only done in the fast path */
>         ac.spread_dirty_pages = (gfp_mask & __GFP_WRITE);
>
> -       /* The preferred zone is used for statistics later */
> +       /*
> +        * The preferred zone is used for statistics but crucially it is
> +        * also used as the starting point for the zonelist iterator. It
> +        * may get reset for allocations that ignore memory policies.
> +        */
>         ac.preferred_zoneref = first_zones_zonelist(ac.zonelist,
>                                         ac.high_zoneidx, ac.nodemask);
>         if (!ac.preferred_zoneref) {
> _
>



-- 
Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

[toc] | [prev] | [next] | [standalone]

#1412934 — Re: BUG: scheduling while atomic: cron/668/0x10c9a0c0

On Fri, Jun 03, 2016 at 09:57:22AM +0200, Geert Uytterhoeven wrote:
> Hi Andrew, Mel,
> 
> On Thu, Jun 2, 2016 at 8:43 PM, Andrew Morton <akpm@linux-foundation.org> wrote:
> > On Thu, 2 Jun 2016 13:19:36 +0100 Mel Gorman <mgorman@techsingularity.net> wrote:
> >> > >Signed-off-by: Mel Gorman <mgorman@techsingularity.net>
> >> >
> >> > Acked-by: Vlastimil Babka <vbabka@suse.cz>
> >> >
> >>
> >> Thanks.
> >
> > I queued this.  A tested-by:Geert would be nice?
> >
> >
> > From: Mel Gorman <mgorman@techsingularity.net>
> > Subject: mm, page_alloc: recalculate the preferred zoneref if the context can ignore memory policies
> >
> > The optimistic fast path may use cpuset_current_mems_allowed instead of of
> > a NULL nodemask supplied by the caller for cpuset allocations.  The
> > preferred zone is calculated on this basis for statistic purposes and as a
> > starting point in the zonelist iterator.
> >
> > However, if the context can ignore memory policies due to being atomic or
> > being able to ignore watermarks then the starting point in the zonelist
> > iterator is no longer correct.  This patch resets the zonelist iterator in
> > the allocator slowpath if the context can ignore memory policies.  This
> > will alter the zone used for statistics but only after it is known that it
> > makes sense for that context.  Resetting it before entering the slowpath
> > would potentially allow an ALLOC_CPUSET allocation to be accounted for
> > against the wrong zone.  Note that while nodemask is not explicitly set to
> > the original nodemask, it would only have been overwritten if
> > cpuset_enabled() and it was reset before the slowpath was entered.
> >
> > Link: http://lkml.kernel.org/r/20160602103936.GU2527@techsingularity.net
> > Fixes: c33d6c06f60f710 ("mm, page_alloc: avoid looking up the first zone in a zonelist twice")
> 
> My understanding was that this was an an additional patch, not fixing
> the problem in-se?
> 

It doesn't fix the problem you had, it is a follow-on patch that
potentially affects.

> Indeed, after applying this patch (without the other one that added
> "z = ac->preferred_zoneref;" to the reset_fair block of
> get_page_from_freelist()) I still get crashes...
> 

The patch you have is the only one required for the crash. This patch
handles a corner case with atomic allocations that can ignore memory
policies.

> Now testing with both applied...

Thanks.

-- 
Mel Gorman
SUSE Labs

[toc] | [prev] | [next] | [standalone]

#1412951 — Re: BUG: scheduling while atomic: cron/668/0x10c9a0c0

Hi Mel,

On Fri, Jun 3, 2016 at 10:41 AM, Mel Gorman <mgorman@techsingularity.net> wrote:
> On Fri, Jun 03, 2016 at 09:57:22AM +0200, Geert Uytterhoeven wrote:
>> On Thu, Jun 2, 2016 at 8:43 PM, Andrew Morton <akpm@linux-foundation.org> wrote:
>> > On Thu, 2 Jun 2016 13:19:36 +0100 Mel Gorman <mgorman@techsingularity.net> wrote:
>> >> > >Signed-off-by: Mel Gorman <mgorman@techsingularity.net>
>> >> >
>> >> > Acked-by: Vlastimil Babka <vbabka@suse.cz>
>> >>
>> >> Thanks.
>> >
>> > I queued this.  A tested-by:Geert would be nice?
>> >
>> > From: Mel Gorman <mgorman@techsingularity.net>
>> > Subject: mm, page_alloc: recalculate the preferred zoneref if the context can ignore memory policies
>> >
>> > The optimistic fast path may use cpuset_current_mems_allowed instead of of
>> > a NULL nodemask supplied by the caller for cpuset allocations.  The
>> > preferred zone is calculated on this basis for statistic purposes and as a
>> > starting point in the zonelist iterator.
>> >
>> > However, if the context can ignore memory policies due to being atomic or
>> > being able to ignore watermarks then the starting point in the zonelist
>> > iterator is no longer correct.  This patch resets the zonelist iterator in
>> > the allocator slowpath if the context can ignore memory policies.  This
>> > will alter the zone used for statistics but only after it is known that it
>> > makes sense for that context.  Resetting it before entering the slowpath
>> > would potentially allow an ALLOC_CPUSET allocation to be accounted for
>> > against the wrong zone.  Note that while nodemask is not explicitly set to
>> > the original nodemask, it would only have been overwritten if
>> > cpuset_enabled() and it was reset before the slowpath was entered.
>> >
>> > Link: http://lkml.kernel.org/r/20160602103936.GU2527@techsingularity.net
>> > Fixes: c33d6c06f60f710 ("mm, page_alloc: avoid looking up the first zone in a zonelist twice")
>>
>> My understanding was that this was an an additional patch, not fixing
>> the problem in-se?
>
> It doesn't fix the problem you had, it is a follow-on patch that
> potentially affects.

Thanks for confirming!

>> Indeed, after applying this patch (without the other one that added
>> "z = ac->preferred_zoneref;" to the reset_fair block of
>> get_page_from_freelist()) I still get crashes...
>
> The patch you have is the only one required for the crash. This patch
> handles a corner case with atomic allocations that can ignore memory
> policies.

OK.

In the mean time my tests completed successfully with both patches applied.

Thanks!

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

[toc] | [prev] | [next] | [standalone]

#1413353 — Re: BUG: scheduling while atomic: cron/668/0x10c9a0c0

On Fri, 3 Jun 2016 11:00:30 +0200 Geert Uytterhoeven <geert@linux-m68k.org> wrote:

> In the mean time my tests completed successfully with both patches applied.

Can we please identify "both patches" with specificity?  I have the
below one.


From: Mel Gorman <mgorman@techsingularity.net>
Subject: mm, page_alloc: recalculate the preferred zoneref if the context can ignore memory policies

The optimistic fast path may use cpuset_current_mems_allowed instead of of
a NULL nodemask supplied by the caller for cpuset allocations.  The
preferred zone is calculated on this basis for statistic purposes and as a
starting point in the zonelist iterator.

However, if the context can ignore memory policies due to being atomic or
being able to ignore watermarks then the starting point in the zonelist
iterator is no longer correct.  This patch resets the zonelist iterator in
the allocator slowpath if the context can ignore memory policies.  This
will alter the zone used for statistics but only after it is known that it
makes sense for that context.  Resetting it before entering the slowpath
would potentially allow an ALLOC_CPUSET allocation to be accounted for
against the wrong zone.  Note that while nodemask is not explicitly set to
the original nodemask, it would only have been overwritten if
cpuset_enabled() and it was reset before the slowpath was entered.

Link: http://lkml.kernel.org/r/20160602103936.GU2527@techsingularity.net
Fixes: c33d6c06f60f710 ("mm, page_alloc: avoid looking up the first zone in a zonelist twice")
Signed-off-by: Mel Gorman <mgorman@techsingularity.net>
Reported-by: Geert Uytterhoeven <geert@linux-m68k.org>
Acked-by: Vlastimil Babka <vbabka@suse.cz>
Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
---

 mm/page_alloc.c |   23 ++++++++++++++++-------
 1 file changed, 16 insertions(+), 7 deletions(-)

diff -puN mm/page_alloc.c~mm-page_alloc-recalculate-the-preferred-zoneref-if-the-context-can-ignore-memory-policies mm/page_alloc.c
--- a/mm/page_alloc.c~mm-page_alloc-recalculate-the-preferred-zoneref-if-the-context-can-ignore-memory-policies
+++ a/mm/page_alloc.c
@@ -3604,6 +3604,17 @@ retry:
 	 */
 	alloc_flags = gfp_to_alloc_flags(gfp_mask);
 
+	/*
+	 * Reset the zonelist iterators if memory policies can be ignored.
+	 * These allocations are high priority and system rather than user
+	 * orientated.
+	 */
+	if ((alloc_flags & ALLOC_NO_WATERMARKS) || !(alloc_flags & ALLOC_CPUSET)) {
+		ac->zonelist = node_zonelist(numa_node_id(), gfp_mask);
+		ac->preferred_zoneref = first_zones_zonelist(ac->zonelist,
+					ac->high_zoneidx, ac->nodemask);
+	}
+
 	/* This is the last chance, in general, before the goto nopage. */
 	page = get_page_from_freelist(gfp_mask, order,
 				alloc_flags & ~ALLOC_NO_WATERMARKS, ac);
@@ -3612,12 +3623,6 @@ retry:
 
 	/* Allocate without watermarks if the context allows */
 	if (alloc_flags & ALLOC_NO_WATERMARKS) {
-		/*
-		 * Ignore mempolicies if ALLOC_NO_WATERMARKS on the grounds
-		 * the allocation is high priority and these type of
-		 * allocations are system rather than user orientated
-		 */
-		ac->zonelist = node_zonelist(numa_node_id(), gfp_mask);
 		page = get_page_from_freelist(gfp_mask, order,
 						ALLOC_NO_WATERMARKS, ac);
 		if (page)
@@ -3816,7 +3821,11 @@ retry_cpuset:
 	/* Dirty zone balancing only done in the fast path */
 	ac.spread_dirty_pages = (gfp_mask & __GFP_WRITE);
 
-	/* The preferred zone is used for statistics later */
+	/*
+	 * The preferred zone is used for statistics but crucially it is
+	 * also used as the starting point for the zonelist iterator. It
+	 * may get reset for allocations that ignore memory policies.
+	 */
 	ac.preferred_zoneref = first_zones_zonelist(ac.zonelist,
 					ac.high_zoneidx, ac.nodemask);
 	if (!ac.preferred_zoneref) {
_

[toc] | [prev] | [next] | [standalone]

#1413358 — Re: BUG: scheduling while atomic: cron/668/0x10c9a0c0

On Fri, Jun 03, 2016 at 09:35:18AM -0700, Andrew Morton wrote:
> On Fri, 3 Jun 2016 11:00:30 +0200 Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> 
> > In the mean time my tests completed successfully with both patches applied.
> 
> Can we please identify "both patches" with specificity?  I have the
> below one.
> 

mm, page_alloc: Reset zonelist iterator after resetting fair zone allocation policy
mm, page_alloc: Recalculate the preferred zoneref if the context can ignore memory policies

-- 
Mel Gorman
SUSE Labs

[toc] | [prev] | [next] | [standalone]

#1413364 — Re: BUG: scheduling while atomic: cron/668/0x10c9a0c0

On Fri, 3 Jun 2016 17:46:25 +0100 Mel Gorman <mgorman@techsingularity.net> wrote:

> On Fri, Jun 03, 2016 at 09:35:18AM -0700, Andrew Morton wrote:
> > On Fri, 3 Jun 2016 11:00:30 +0200 Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> > 
> > > In the mean time my tests completed successfully with both patches applied.
> > 
> > Can we please identify "both patches" with specificity?  I have the
> > below one.
> > 
> 
> mm, page_alloc: Reset zonelist iterator after resetting fair zone allocation policy
> mm, page_alloc: Recalculate the preferred zoneref if the context can ignore memory policies

Cool, thanks.  I'll get both over to Linus today.

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web