mm, page_alloc: fix calculation of pgdat->nr_zones
Subsystems (2)
PATCH 1/1 mm, page_alloc: fix calculation of pgdat->nr_zones
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).
--- mm/page_alloc.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)
mm/page_alloc.c +3 -1
@@ -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;
Signature · 1 line
2.15.1
Quoted 11 lines · Wei Yang
Function
init_currently_empty_zone()will adjustpgdat->nr_zonesand 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.
Quoted 20 lines
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-72. hot-add cpu 3-7
cpu-add [3-7]2. hot-add memory to nod1
object_addmemory-backend-ram,id=ram0,size=1G
device_addpc-dimm,id=dimm0,memdev=ram0,node=1
Quoted 7 lines
3. online memory with following order
echo
online_movable> memory47/state
echo online > memory40/stateAfter this, node1 will have its
nr_zonesequals 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.
Quoted 11 lines
Signed-off-by Wei Yang <[email protected]>--- mm/page_alloc.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)mm/page_alloc.c +0
-0
mm/page_alloc.c void __meminit init_currently_empty_zone(struct zone *zone, · line 5829
Quoted 8 lines
mm/page_alloc.c +3
-1@@ -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.
Quoted 9 lines · Anshuman Khandual
Quoted 6 lines · Wei Yang
Function
init_currently_empty_zone()will adjustpgdat->nr_zonesand 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:
Quoted 8 lines
Quoted 5 lines
*
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_zonesover 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.
Quoted 3 lines
/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.
Quoted 22 lines
Quoted 22 lines
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-72. hot-add cpu 3-7
cpu-add [3-7]2. hot-add memory to nod1
object_addmemory-backend-ram,id=ram0,size=1G
device_addpc-dimm,id=dimm0,memdev=ram0,node=13. online memory with following order
Quoted 9 lines
Quoted 5 lines
echo
online_movable> memory47/state
echo online > memory40/stateAfter this, node1 will have its
nr_zonesequals 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.
Quoted 16 lines
Quoted 16 lines
Signed-off-by Wei Yang <[email protected]>--- mm/page_alloc.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)mm/page_alloc.c +1
-0@@ -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 ;
Quoted 6 lines
mm/page_alloc.c void __meminit init_currently_empty_zone(struct zone *zone, · line 5829
Quoted 3 lines
mm/page_alloc.c +2
-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_zoneson an already populated node.
Yes, you are right.
Signature · 2 lines
Wei Yang Help you, Help me
Quoted 31 lines · Wei Yang
Function
init_currently_empty_zone()will adjustpgdat->nr_zonesand 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-72. hot-add cpu 3-7
cpu-add [3-7]2. hot-add memory to nod1
object_addmemory-backend-ram,id=ram0,size=1G
device_addpc-dimm,id=dimm0,memdev=ram0,node=1
Quoted 7 lines
3. online memory with following order
echo
online_movable> memory47/state
echo online > memory40/stateAfter this, node1 will have its
nr_zonesequals 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.
I haven't checked the code prior to this rework but I suspect it was really the above one to change the picture.
Quoted 1 line
Signed-off-by Wei Yang <[email protected]>
Thanks!
Quoted 23 lines
--- mm/page_alloc.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)mm/page_alloc.c +3
-1@@ -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;Signature · 1 line
2.15.1
Signature · 2 lines
Michal Hocko SUSE Labs
Michal Hocko
reviewerQuoted 46 lines · Michal Hocko
Quoted 31 lines · Wei Yang
Function
init_currently_empty_zone()will adjustpgdat->nr_zonesand 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-72. hot-add cpu 3-7
cpu-add [3-7]2. hot-add memory to nod1
object_addmemory-backend-ram,id=ram0,size=1G
device_addpc-dimm,id=dimm0,memdev=ram0,node=1Quoted 7 lines
3. online memory with following order
echo
online_movable> memory47/state
echo online > memory40/stateAfter this, node1 will have its
nr_zonesequals 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 updatenr_zoneswhen a higher zone is added.
Quoted 4 lines
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).
older tress would have to be checked explicitly.
Quoted 33 lines
Quoted 1 line
Signed-off-by Wei Yang <[email protected]>Acked-by Michal Hocko <[email protected]>Thanks!
Quoted 23 lines
--- mm/page_alloc.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)mm/page_alloc.c +3
-1@@ -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;Signature · 1 line
2.15.1Signature · 2 lines
Michal Hocko SUSE Labs
Signature · 2 lines
Michal Hocko SUSE Labs
Quoted 10 lines · Michal Hocko
Forgot to mention that this should probably go to stable. Having an incorrect
nr_zonesmight 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.13older 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
Quoted 8 lines · Sasha Levin
Quoted 7 lines · Michal Hocko
Forgot to mention that this should probably go to stable. Having an incorrect
nr_zonesmight 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).
Quoted 7 lines
Quoted 3 lines
Cc stable # since 4.13older 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.
Quoted 3 lines
--
Thanks,
Sasha
Signature · 2 lines
Wei Yang Help you, Help me
Quoted 2 lines
Signed-off-by Wei Yang <[email protected]>
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.
Oscar Salvador
Michal Hocko
reviewerOn Mon 19-11-18 11:07:41, osalvador wrote:
[...]
Quoted 5 lines
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 wirhpgdat->nr_zoneswe 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.
Signature · 2 lines
Michal Hocko SUSE Labs
Quoted 10 lines · osalvador
Quoted 2 lines
Signed-off-by Wei Yang <[email protected]>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 wirhpgdat->nr_zoneswe 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 :-)
Quoted 5 lines
Reviewed-by Oscar Salvador <[email protected]>Oscar Salvador
Signature · 2 lines
Wei Yang Help you, Help me
Michal Hocko
reviewerQuoted 12 lines · Wei Yang
Quoted 10 lines · osalvador
Quoted 2 lines
Signed-off-by Wei Yang <[email protected]>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 wirhpgdat->nr_zoneswe seem to check if the zone is populated in order to work with it.
But still, I wonder if we should re-adjust it.
Quoted 6 lines
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.
Signature · 2 lines
Michal Hocko SUSE Labs
Quoted 15 lines · Michal Hocko
Quoted 14 lines · Wei Yang
Quoted 10 lines · osalvador
Quoted 2 lines
Signed-off-by Wei Yang <[email protected]>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 wirhpgdat->nr_zoneswe 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.
Quoted 8 lines
Quoted 4 lines
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?
Quoted 3 lines
Signature · 2 lines
Michal Hocko SUSE Labs
Signature · 2 lines
Wei Yang Help you, Help me
Michal Hocko
reviewerQuoted 21 lines · Wei Yang
Quoted 20 lines · Michal Hocko
Quoted 18 lines · Wei Yang
Quoted 10 lines · osalvador
Quoted 2 lines
Signed-off-by Wei Yang <[email protected]>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 wirhpgdat->nr_zoneswe 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.
Quoted 8 lines
Quoted 3 lines
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.
Signature · 2 lines
Michal Hocko SUSE Labs