[PATCH] mm, page_alloc: fix calculation of pgdat->nr_zones

[PATCH] mm, page_alloc: fix calculation of pgdat->nr_zones

[Wei Yang]

Function init_currently_empty_zone() will adjust pgdat->nr_zones and set
it to 'zone_idx(zone) + 1' unconditionally. This is correct in the
normal case, while not exact in hot-plug situation.

This function is used in two places:

  * free_area_init_core()
  * move_pfn_range_to_zone()

In the first case, we are sure zone index increase monotonically. While
in the second one, this is under users control.

One way to reproduce this is:
----------------------------

1. create a virtual machine with empty node1

   -m 4G,slots=32,maxmem=32G \
   -smp 4,maxcpus=8          \
   -numa node,nodeid=0,mem=4G,cpus=0-3 \
   -numa node,nodeid=1,mem=0G,cpus=4-7

2. hot-add cpu 3-7

   cpu-add [3-7]

2. hot-add memory to nod1

   object_add memory-backend-ram,id=ram0,size=1G
   device_add pc-dimm,id=dimm0,memdev=ram0,node=1

3. online memory with following order

   echo online_movable > memory47/state
   echo online > memory40/state

After this, node1 will have its nr_zones equals to (ZONE_NORMAL + 1)
instead of (ZONE_MOVABLE + 1).

Signed-off-by: Wei Yang <richard.weiyang@gmail.com>
---
 mm/page_alloc.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/mm/page_alloc.c b/mm/page_alloc.c
index 5b7cd20dbaef..2d3c54201255 100644
--- a/mm/page_alloc.c
+++ b/mm/page_alloc.c
@@ -5823,8 +5823,10 @@ void __meminit init_currently_empty_zone(struct zone *zone,
 					unsigned long size)
 {
 	struct pglist_data *pgdat = zone->zone_pgdat;
+	int zone_idx = zone_idx(zone) + 1;
 
-	pgdat->nr_zones = zone_idx(zone) + 1;
+	if (zone_idx > pgdat->nr_zones)
+		pgdat->nr_zones = zone_idx;
 
 	zone->zone_start_pfn = zone_start_pfn;
 
-- 
2.15.1

Re: [PATCH] mm, page_alloc: fix calculation of pgdat->nr_zones

[Anshuman Khandual]

On 11/17/2018 07:50 AM, Wei Yang wrote:
> Function init_currently_empty_zone() will adjust pgdat->nr_zones and set
> it to 'zone_idx(zone) + 1' unconditionally. This is correct in the
> normal case, while not exact in hot-plug situation.
> 
> This function is used in two places:
> 
>   * free_area_init_core()
>   * move_pfn_range_to_zone()
> 
> In the first case, we are sure zone index increase monotonically. While
> in the second one, this is under users control.

So pgdat->nr_zones over counts the number of zones than what node has
really got ? Does it affect all online options (online/online_kernel
/online_movable) if there are other higher index zones present on the
node. 

> 
> One way to reproduce this is:
> ----------------------------
> 
> 1. create a virtual machine with empty node1
> 
>    -m 4G,slots=32,maxmem=32G \
>    -smp 4,maxcpus=8          \
>    -numa node,nodeid=0,mem=4G,cpus=0-3 \
>    -numa node,nodeid=1,mem=0G,cpus=4-7
> 
> 2. hot-add cpu 3-7
> 
>    cpu-add [3-7]
> 
> 2. hot-add memory to nod1
> 
>    object_add memory-backend-ram,id=ram0,size=1G
>    device_add pc-dimm,id=dimm0,memdev=ram0,node=1
> 
> 3. online memory with following order
> 
>    echo online_movable > memory47/state
>    echo online > memory40/state
> 
> After this, node1 will have its nr_zones equals to (ZONE_NORMAL + 1)
> instead of (ZONE_MOVABLE + 1).

Which prevents an over count I guess. Just wondering if you noticed this
causing any real problem or some other side effects.

> 
> Signed-off-by: Wei Yang <richard.weiyang@gmail.com>
> ---
>  mm/page_alloc.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/mm/page_alloc.c b/mm/page_alloc.c
> index 5b7cd20dbaef..2d3c54201255 100644
> --- a/mm/page_alloc.c
> +++ b/mm/page_alloc.c
> @@ -5823,8 +5823,10 @@ void __meminit init_currently_empty_zone(struct zone *zone,
>  					unsigned long size)
>  {
>  	struct pglist_data *pgdat = zone->zone_pgdat;
> +	int zone_idx = zone_idx(zone) + 1;
>  
> -	pgdat->nr_zones = zone_idx(zone) + 1;
> +	if (zone_idx > pgdat->nr_zones)
> +		pgdat->nr_zones = zone_idx;

This seems to be correct if we try to init a zone (due to memory hotplug)
in between index 0 and pgdat->nr_zones on an already populated node.

Re: [PATCH] mm, page_alloc: fix calculation of pgdat->nr_zones

[Wei Yang]

On Mon, Nov 19, 2018 at 12:08:54PM +0530, Anshuman Khandual wrote:
>
>
>On 11/17/2018 07:50 AM, Wei Yang wrote:
>> Function init_currently_empty_zone() will adjust pgdat->nr_zones and set
>> it to 'zone_idx(zone) + 1' unconditionally. This is correct in the
>> normal case, while not exact in hot-plug situation.
>> 
>> This function is used in two places:
>> 
>>   * free_area_init_core()
>>   * move_pfn_range_to_zone()
>> 
>> In the first case, we are sure zone index increase monotonically. While
>> in the second one, this is under users control.
>
>So pgdat->nr_zones over counts the number of zones than what node has
>really got ? Does it affect all online options (online/online_kernel

Yes, nr_zones is not the literal meaning.

>/online_movable) if there are other higher index zones present on the
>node. 
>

The sequence matters, while usually we online page to ZONE_NORMAL, if I
am correct.

I may not get your question clearly.

>> 
>> One way to reproduce this is:
>> ----------------------------
>> 
>> 1. create a virtual machine with empty node1
>> 
>>    -m 4G,slots=32,maxmem=32G \
>>    -smp 4,maxcpus=8          \
>>    -numa node,nodeid=0,mem=4G,cpus=0-3 \
>>    -numa node,nodeid=1,mem=0G,cpus=4-7
>> 
>> 2. hot-add cpu 3-7
>> 
>>    cpu-add [3-7]
>> 
>> 2. hot-add memory to nod1
>> 
>>    object_add memory-backend-ram,id=ram0,size=1G
>>    device_add pc-dimm,id=dimm0,memdev=ram0,node=1
>> 
>> 3. online memory with following order
>> 
>>    echo online_movable > memory47/state
>>    echo online > memory40/state
>> 
>> After this, node1 will have its nr_zones equals to (ZONE_NORMAL + 1)
>> instead of (ZONE_MOVABLE + 1).
>
>Which prevents an over count I guess. Just wondering if you noticed this
>causing any real problem or some other side effects.
>

Not from my side.

I think Michal's rely may answer your question.

>> 
>> Signed-off-by: Wei Yang <richard.weiyang@gmail.com>
>> ---
>>  mm/page_alloc.c | 4 +++-
>>  1 file changed, 3 insertions(+), 1 deletion(-)
>> 
>> diff --git a/mm/page_alloc.c b/mm/page_alloc.c
>> index 5b7cd20dbaef..2d3c54201255 100644
>> --- a/mm/page_alloc.c
>> +++ b/mm/page_alloc.c
>> @@ -5823,8 +5823,10 @@ void __meminit init_currently_empty_zone(struct zone *zone,
>>  					unsigned long size)
>>  {
>>  	struct pglist_data *pgdat = zone->zone_pgdat;
>> +	int zone_idx = zone_idx(zone) + 1;
>>  
>> -	pgdat->nr_zones = zone_idx(zone) + 1;
>> +	if (zone_idx > pgdat->nr_zones)
>> +		pgdat->nr_zones = zone_idx;
>
>This seems to be correct if we try to init a zone (due to memory hotplug)
>in between index 0 and pgdat->nr_zones on an already populated node.

Yes, you are right.

-- 
Wei Yang
Help you, Help me

Re: [PATCH] mm, page_alloc: fix calculation of pgdat->nr_zones

[Michal Hocko]

On Sat 17-11-18 10:20:22, Wei Yang wrote:
> Function init_currently_empty_zone() will adjust pgdat->nr_zones and set
> it to 'zone_idx(zone) + 1' unconditionally. This is correct in the
> normal case, while not exact in hot-plug situation.
>
> This function is used in two places:
> 
>   * free_area_init_core()
>   * move_pfn_range_to_zone()
> 
> In the first case, we are sure zone index increase monotonically. While
> in the second one, this is under users control.
> 
> One way to reproduce this is:
> ----------------------------
> 
> 1. create a virtual machine with empty node1
> 
>    -m 4G,slots=32,maxmem=32G \
>    -smp 4,maxcpus=8          \
>    -numa node,nodeid=0,mem=4G,cpus=0-3 \
>    -numa node,nodeid=1,mem=0G,cpus=4-7
> 
> 2. hot-add cpu 3-7
> 
>    cpu-add [3-7]
> 
> 2. hot-add memory to nod1
> 
>    object_add memory-backend-ram,id=ram0,size=1G
>    device_add pc-dimm,id=dimm0,memdev=ram0,node=1
> 
> 3. online memory with following order
> 
>    echo online_movable > memory47/state
>    echo online > memory40/state
> 
> After this, node1 will have its nr_zones equals to (ZONE_NORMAL + 1)
> instead of (ZONE_MOVABLE + 1).

Maybe it is just me but the above was quite hard to grasp. So just to
clarify. The underlying problem is that initialization of any existing
empty zone will override the previous node wide setting
(pgdat->nr_zones). The fix is to only update nr_zones when a higher zone
is added.

Fixes: f1dd2cd13c4b ("mm, memory_hotplug: do not associate hotadded memory to zones until online")

I haven't checked the code prior to this rework but I suspect it was
really the above one to change the picture.

> Signed-off-by: Wei Yang <richard.weiyang@gmail.com>

Acked-by: Michal Hocko <mhocko@suse.com>

Thanks!

> ---
>  mm/page_alloc.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/mm/page_alloc.c b/mm/page_alloc.c
> index 5b7cd20dbaef..2d3c54201255 100644
> --- a/mm/page_alloc.c
> +++ b/mm/page_alloc.c
> @@ -5823,8 +5823,10 @@ void __meminit init_currently_empty_zone(struct zone *zone,
>  					unsigned long size)
>  {
>  	struct pglist_data *pgdat = zone->zone_pgdat;
> +	int zone_idx = zone_idx(zone) + 1;
>  
> -	pgdat->nr_zones = zone_idx(zone) + 1;
> +	if (zone_idx > pgdat->nr_zones)
> +		pgdat->nr_zones = zone_idx;
>  
>  	zone->zone_start_pfn = zone_start_pfn;
>  
> -- 
> 2.15.1
> 

-- 
Michal Hocko
SUSE Labs

Re: [PATCH] mm, page_alloc: fix calculation of pgdat->nr_zones

[Michal Hocko]

On Mon 19-11-18 10:48:32, Michal Hocko wrote:
> On Sat 17-11-18 10:20:22, Wei Yang wrote:
> > Function init_currently_empty_zone() will adjust pgdat->nr_zones and set
> > it to 'zone_idx(zone) + 1' unconditionally. This is correct in the
> > normal case, while not exact in hot-plug situation.
> >
> > This function is used in two places:
> > 
> >   * free_area_init_core()
> >   * move_pfn_range_to_zone()
> > 
> > In the first case, we are sure zone index increase monotonically. While
> > in the second one, this is under users control.
> > 
> > One way to reproduce this is:
> > ----------------------------
> > 
> > 1. create a virtual machine with empty node1
> > 
> >    -m 4G,slots=32,maxmem=32G \
> >    -smp 4,maxcpus=8          \
> >    -numa node,nodeid=0,mem=4G,cpus=0-3 \
> >    -numa node,nodeid=1,mem=0G,cpus=4-7
> > 
> > 2. hot-add cpu 3-7
> > 
> >    cpu-add [3-7]
> > 
> > 2. hot-add memory to nod1
> > 
> >    object_add memory-backend-ram,id=ram0,size=1G
> >    device_add pc-dimm,id=dimm0,memdev=ram0,node=1
> > 
> > 3. online memory with following order
> > 
> >    echo online_movable > memory47/state
> >    echo online > memory40/state
> > 
> > After this, node1 will have its nr_zones equals to (ZONE_NORMAL + 1)
> > instead of (ZONE_MOVABLE + 1).
> 
> Maybe it is just me but the above was quite hard to grasp. So just to
> clarify. The underlying problem is that initialization of any existing
> empty zone will override the previous node wide setting
> (pgdat->nr_zones). The fix is to only update nr_zones when a higher zone
> is added.
> 
> Fixes: f1dd2cd13c4b ("mm, memory_hotplug: do not associate hotadded memory to zones until online")
> 
> I haven't checked the code prior to this rework but I suspect it was
> really the above one to change the picture.

Forgot to mention that this should probably go to stable. Having an
incorrect nr_zones might result in all sorts of problems which would be
quite hard to debug (e.g. reclaim not considering the movable zone).
I do not expect many users would suffer from this it but still this is
trivial and obviously right thing to do so backporting to the stable
tree shouldn't be harmful (last famous words).

Cc: stable # since 4.13

older tress would have to be checked explicitly.

> > Signed-off-by: Wei Yang <richard.weiyang@gmail.com>
> 
> Acked-by: Michal Hocko <mhocko@suse.com>
> 
> Thanks!
> 
> > ---
> >  mm/page_alloc.c | 4 +++-
> >  1 file changed, 3 insertions(+), 1 deletion(-)
> > 
> > diff --git a/mm/page_alloc.c b/mm/page_alloc.c
> > index 5b7cd20dbaef..2d3c54201255 100644
> > --- a/mm/page_alloc.c
> > +++ b/mm/page_alloc.c
> > @@ -5823,8 +5823,10 @@ void __meminit init_currently_empty_zone(struct zone *zone,
> >  					unsigned long size)
> >  {
> >  	struct pglist_data *pgdat = zone->zone_pgdat;
> > +	int zone_idx = zone_idx(zone) + 1;
> >  
> > -	pgdat->nr_zones = zone_idx(zone) + 1;
> > +	if (zone_idx > pgdat->nr_zones)
> > +		pgdat->nr_zones = zone_idx;
> >  
> >  	zone->zone_start_pfn = zone_start_pfn;
> >  
> > -- 
> > 2.15.1
> > 
> 
> -- 
> Michal Hocko
> SUSE Labs

-- 
Michal Hocko
SUSE Labs

Re: [PATCH] mm, page_alloc: fix calculation of pgdat->nr_zones

[Sasha Levin]

On Mon, Nov 19, 2018 at 02:38:51PM +0100, Michal Hocko wrote:
>Forgot to mention that this should probably go to stable. Having an
>incorrect nr_zones might result in all sorts of problems which would be
>quite hard to debug (e.g. reclaim not considering the movable zone).
>I do not expect many users would suffer from this it but still this is
>trivial and obviously right thing to do so backporting to the stable
>tree shouldn't be harmful (last famous words).
>
>Cc: stable # since 4.13
>
>older tress would have to be checked explicitly.

While the final commit included Michal's response, it didn't have a
stable tag. Could someone confirm it should go in stable?

--
Thanks,
Sasha

Re: [PATCH] mm, page_alloc: fix calculation of pgdat->nr_zones

[Wei Yang]

On Tue, Dec 04, 2018 at 04:05:04AM -0500, Sasha Levin wrote:
>On Mon, Nov 19, 2018 at 02:38:51PM +0100, Michal Hocko wrote:
>> Forgot to mention that this should probably go to stable. Having an
>> incorrect nr_zones might result in all sorts of problems which would be
>> quite hard to debug (e.g. reclaim not considering the movable zone).
>> I do not expect many users would suffer from this it but still this is
>> trivial and obviously right thing to do so backporting to the stable
>> tree shouldn't be harmful (last famous words).
>> 
>> Cc: stable # since 4.13
>> 
>> older tress would have to be checked explicitly.
>
>While the final commit included Michal's response, it didn't have a
>stable tag. Could someone confirm it should go in stable?
>

I think Michal wants it into stable.

>--
>Thanks,
>Sasha

-- 
Wei Yang
Help you, Help me

Re: [PATCH] mm, page_alloc: fix calculation of pgdat->nr_zones

[osalvador]

> Signed-off-by: Wei Yang <richard.weiyang@gmail.com>

Good catch.

One thing I was wondering is that if we also should re-adjust it when a
zone gets emptied during offlining memory.
I checked, and whenever we work wirh pgdat->nr_zones we seem to check
if the zone is populated in order to work with it.
But still, I wonder if we should re-adjust it.

Reviewed-by: Oscar Salvador <osalvador@suse.de>

Oscar Salvador

Re: [PATCH] mm, page_alloc: fix calculation of pgdat->nr_zones

[Michal Hocko]

On Mon 19-11-18 11:07:41, osalvador wrote:
[...]
> One thing I was wondering is that if we also should re-adjust it when a
> zone gets emptied during offlining memory.
> I checked, and whenever we work wirh pgdat->nr_zones we seem to check
> if the zone is populated in order to work with it.
> But still, I wonder if we should re-adjust it.

I would rather not because we are not really deallocating zones and once
you want to shrink it you have to linearize all the loops depending on
it.

-- 
Michal Hocko
SUSE Labs

Re: [PATCH] mm, page_alloc: fix calculation of pgdat->nr_zones

[Wei Yang]

On Mon, Nov 19, 2018 at 11:07:41AM +0100, osalvador wrote:
>
>> Signed-off-by: Wei Yang <richard.weiyang@gmail.com>
>
>Good catch.
>
>One thing I was wondering is that if we also should re-adjust it when a
>zone gets emptied during offlining memory.
>I checked, and whenever we work wirh pgdat->nr_zones we seem to check
>if the zone is populated in order to work with it.
>But still, I wonder if we should re-adjust it.

Well, thanks all for comments. I am glad you like it.

Actually, I have another proposal or I notice another potential issue.

In case user online pages in parallel, we may face a contention and get
a wrong nr_zones.

If this analysis is correct, I propose to have a lock around this.

Look forward your comments :-)

>
>Reviewed-by: Oscar Salvador <osalvador@suse.de>
>
>Oscar Salvador
>

-- 
Wei Yang
Help you, Help me

Re: [PATCH] mm, page_alloc: fix calculation of pgdat->nr_zones

[Michal Hocko]

On Mon 19-11-18 14:15:05, Wei Yang wrote:
> On Mon, Nov 19, 2018 at 11:07:41AM +0100, osalvador wrote:
> >
> >> Signed-off-by: Wei Yang <richard.weiyang@gmail.com>
> >
> >Good catch.
> >
> >One thing I was wondering is that if we also should re-adjust it when a
> >zone gets emptied during offlining memory.
> >I checked, and whenever we work wirh pgdat->nr_zones we seem to check
> >if the zone is populated in order to work with it.
> >But still, I wonder if we should re-adjust it.
> 
> Well, thanks all for comments. I am glad you like it.
> 
> Actually, I have another proposal or I notice another potential issue.
> 
> In case user online pages in parallel, we may face a contention and get
> a wrong nr_zones.

No, this should be protected by the global mem hotplug lock. Anyway a
dedicated lock would be much better. I would move it under
pgdat_resize_lock.
-- 
Michal Hocko
SUSE Labs

Re: [PATCH] mm, page_alloc: fix calculation of pgdat->nr_zones

[Wei Yang]

On Mon, Nov 19, 2018 at 03:23:25PM +0100, Michal Hocko wrote:
>On Mon 19-11-18 14:15:05, Wei Yang wrote:
>> On Mon, Nov 19, 2018 at 11:07:41AM +0100, osalvador wrote:
>> >
>> >> Signed-off-by: Wei Yang <richard.weiyang@gmail.com>
>> >
>> >Good catch.
>> >
>> >One thing I was wondering is that if we also should re-adjust it when a
>> >zone gets emptied during offlining memory.
>> >I checked, and whenever we work wirh pgdat->nr_zones we seem to check
>> >if the zone is populated in order to work with it.
>> >But still, I wonder if we should re-adjust it.
>> 
>> Well, thanks all for comments. I am glad you like it.
>> 
>> Actually, I have another proposal or I notice another potential issue.
>> 
>> In case user online pages in parallel, we may face a contention and get
>> a wrong nr_zones.
>
>No, this should be protected by the global mem hotplug lock. Anyway a
>dedicated lock would be much better. I would move it under
>pgdat_resize_lock.

This is what I think about.

Do you like me to send v2 with this change? Or you would like to add it
by yourself?

>-- 
>Michal Hocko
>SUSE Labs

-- 
Wei Yang
Help you, Help me

Re: [PATCH] mm, page_alloc: fix calculation of pgdat->nr_zones

[Michal Hocko]

On Mon 19-11-18 21:44:50, Wei Yang wrote:
> On Mon, Nov 19, 2018 at 03:23:25PM +0100, Michal Hocko wrote:
> >On Mon 19-11-18 14:15:05, Wei Yang wrote:
> >> On Mon, Nov 19, 2018 at 11:07:41AM +0100, osalvador wrote:
> >> >
> >> >> Signed-off-by: Wei Yang <richard.weiyang@gmail.com>
> >> >
> >> >Good catch.
> >> >
> >> >One thing I was wondering is that if we also should re-adjust it when a
> >> >zone gets emptied during offlining memory.
> >> >I checked, and whenever we work wirh pgdat->nr_zones we seem to check
> >> >if the zone is populated in order to work with it.
> >> >But still, I wonder if we should re-adjust it.
> >> 
> >> Well, thanks all for comments. I am glad you like it.
> >> 
> >> Actually, I have another proposal or I notice another potential issue.
> >> 
> >> In case user online pages in parallel, we may face a contention and get
> >> a wrong nr_zones.
> >
> >No, this should be protected by the global mem hotplug lock. Anyway a
> >dedicated lock would be much better. I would move it under
> >pgdat_resize_lock.
> 
> This is what I think about.
> 
> Do you like me to send v2 with this change? Or you would like to add it
> by yourself?

This is independent on this patch. So feel free to send a separate
patch.
-- 
Michal Hocko
SUSE Labs