mm: page_alloc: fix zone allocation fairness on UP
Subsystems (2)
Johannes Weiner
reviewerPATCH RESEND 1/1 mm: page_alloc: fix zone allocation fairness on UP
The zone allocation batches can easily underflow due to higher-order allocations or spills to remote nodes. On SMP that's fine, because underflows are expected from concurrency and dealt with by returning
0. But on UP, zone_page_state will just return a wrapped unsigned long, which will get past the <= 0 check and then consider the zone eligible until its watermarks are hit.
3a025760fc15 ("mm: page_alloc: spill to remote nodes before waking kswapd") already made the counter-resetting use atomic_long_read() to accomodate underflows from remote spills, but it didn't go all the way with it. Make it clear that these batches are expected to go negative regardless of concurrency, and use atomic_long_read() everywhere.
--- mm/page_alloc.c | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-)
Sorry I forgot to CC you, Leon. Resend with updated Tags.
mm/page_alloc.c +3 -4
@@ -1612,7 +1612,7 @@ again: }__mod_zone_page_state (zone,NR_ALLOC_BATCH , -(1 << order)); -if (zone_page_state (zone,NR_ALLOC_BATCH ) ==0 && +if (atomic_long_read (&zone->vm_stat[NR_ALLOC_BATCH ]) <=0 && !zone_is_fair_depleted (zone))zone_set_flag (zone,ZONE_FAIR_DEPLETED );
@@ -5701,9 +5701,8 @@ static void __setup_per_zone_wmarks(void) zone->watermark[WMARK_HIGH ] =min_wmark_pages (zone) + (tmp >>1 );__mod_zone_page_state (zone,NR_ALLOC_BATCH , -high_wmark_pages (zone) - -low_wmark_pages (zone) - -zone_page_state (zone,NR_ALLOC_BATCH )); +high_wmark_pages (zone) -low_wmark_pages (zone) - +atomic_long_read (&zone->vm_stat[NR_ALLOC_BATCH ]));setup_zone_migrate_reserve (zone);spin_unlock_irqrestore (&zone->lock, flags);
Signature · 7 lines
2.0.4 -- To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM, see: http://www.linux-mm.org/ . Don't email: <a href=mailto:"[email protected]"> [email protected] </a>
Hi Johaness,
Quoted 50 lines · Johannes Weiner
The zone allocation batches can easily underflow due to higher-order allocations or spills to remote nodes. On SMP that's fine, because underflows are expected from concurrency and dealt with by returning
0. But on UP,zone_page_statewill just return a wrapped unsigned long, which will get past the <= 0 check and then consider the zone eligible until its watermarks are hit.
3a025760fc15("mm:page_alloc: spill to remote nodes before waking kswapd") already made the counter-resetting useatomic_long_read()to accomodate underflows from remote spills, but it didn't go all the way with it. Make it clear that these batches are expected to go negative regardless of concurrency, and useatomic_long_read()everywhere.Fixes 81c0a2bb515f ("mm: page_alloc: fair zone allocator policy")Reported-by Vlastimil Babka <[email protected]>Reported-by Leon Romanovsky <[email protected]>Signed-off-by Johannes Weiner <[email protected]>Acked-by Mel Gorman <[email protected]>Cc "3.12+" <[email protected]>--- mm/page_alloc.c | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-)Sorry I forgot to CC you, Leon. Resend with updated Tags.
mm/page_alloc.c +3
-4@@ -1612,7 +1612,7 @@ again: }__mod_zone_page_state (zone,NR_ALLOC_BATCH , -(1 << order)); -if (zone_page_state (zone,NR_ALLOC_BATCH ) ==0 && +if (atomic_long_read (&zone->vm_stat[NR_ALLOC_BATCH ]) <=0 && !zone_is_fair_depleted (zone))zone_set_flag (zone,ZONE_FAIR_DEPLETED );@@ -5701,9 +5701,8 @@ static void __setup_per_zone_wmarks(void) zone->watermark[WMARK_HIGH ] =min_wmark_pages (zone) + (tmp >>1 );__mod_zone_page_state (zone,NR_ALLOC_BATCH , -high_wmark_pages (zone) - -low_wmark_pages (zone) - -zone_page_state (zone, NR_ALLOC_BATCH)); +high_wmark_pages (zone) -low_wmark_pages (zone) - + atomic_long_read(&zone->vm_stat[NR_ALLOC_BATCH]));
Quoted 3 lines
setup_zone_migrate_reserve(zone);
spin_unlock_irqrestore(&zone->lock, flags);I think the better way will be to apply Mel's patch
https://lkml.org/lkml/2014/9/8/214 which fix zone_page_state shadow casting issue and convert all atomic_long_read(&zone->vm_stat[NR_ALLOC_BATCH])) to zone_page__state(zone, NR_ALLOC_BATCH). This move will unify access to vm_stat.
Quoted 9 lines
--
2.0.4--
To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>
Signature · 2 lines
Leon Romanovsky | Independent Linux Consultant
www.leon.nu | [email protected]Johannes Weiner
reviewerQuoted 59 lines · Leon Romanovsky
Hi Johaness,
Quoted 50 lines · Johannes Weiner
The zone allocation batches can easily underflow due to higher-order allocations or spills to remote nodes. On SMP that's fine, because underflows are expected from concurrency and dealt with by returning
0. But on UP,zone_page_statewill just return a wrapped unsigned long, which will get past the <= 0 check and then consider the zone eligible until its watermarks are hit.
3a025760fc15("mm:page_alloc: spill to remote nodes before waking kswapd") already made the counter-resetting useatomic_long_read()to accomodate underflows from remote spills, but it didn't go all the way with it. Make it clear that these batches are expected to go negative regardless of concurrency, and useatomic_long_read()everywhere.Fixes 81c0a2bb515f ("mm: page_alloc: fair zone allocator policy")Reported-by Vlastimil Babka <[email protected]>Reported-by Leon Romanovsky <[email protected]>Signed-off-by Johannes Weiner <[email protected]>Acked-by Mel Gorman <[email protected]>Cc "3.12+" <[email protected]>--- mm/page_alloc.c | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-)Sorry I forgot to CC you, Leon. Resend with updated Tags.
mm/page_alloc.c +3
-4@@ -1612,7 +1612,7 @@ again: }__mod_zone_page_state (zone,NR_ALLOC_BATCH , -(1 << order)); -if (zone_page_state (zone,NR_ALLOC_BATCH ) ==0 && +if (atomic_long_read (&zone->vm_stat[NR_ALLOC_BATCH ]) <=0 && !zone_is_fair_depleted (zone))zone_set_flag (zone,ZONE_FAIR_DEPLETED );@@ -5701,9 +5701,8 @@ static void __setup_per_zone_wmarks(void) zone->watermark[WMARK_HIGH ] =min_wmark_pages (zone) + (tmp >>1 );__mod_zone_page_state (zone,NR_ALLOC_BATCH , -high_wmark_pages (zone) - -low_wmark_pages (zone) - -zone_page_state (zone, NR_ALLOC_BATCH)); +high_wmark_pages (zone) -low_wmark_pages (zone) - + atomic_long_read(&zone->vm_stat[NR_ALLOC_BATCH]));Quoted 3 lines
setup_zone_migrate_reserve(zone); spin_unlock_irqrestore(&zone->lock, flags);
Quoted 5 lines
I think the better way will be to apply Mel's patch
https://lkml.org/lkml/2014/9/8/214 which fixzone_page_stateshadow casting issue and convert allatomic_long_read(&zone->vm_stat[NR_ALLOC_BATCH])) tozone_page__state(zone, NR_ALLOC_BATCH). This move will unify access tovm_stat.
It's not as simple. The counter can go way negative and we need that negative number, not 0, to calculate the reset delta. As I said in response to Mel's patch, we could make the vmstat API signed but I'm not convinced that is reasonable, given the 99% majority of usecases.
Quoted 66 lines · Johannes Weiner
Quoted 59 lines · Leon Romanovsky
Hi Johaness,
Quoted 50 lines · Johannes Weiner
The zone allocation batches can easily underflow due to higher-order allocations or spills to remote nodes. On SMP that's fine, because underflows are expected from concurrency and dealt with by returning
0. But on UP,zone_page_statewill just return a wrapped unsigned long, which will get past the <= 0 check and then consider the zone eligible until its watermarks are hit.
3a025760fc15("mm:page_alloc: spill to remote nodes before waking kswapd") already made the counter-resetting useatomic_long_read()to accomodate underflows from remote spills, but it didn't go all the way with it. Make it clear that these batches are expected to go negative regardless of concurrency, and useatomic_long_read()everywhere.Fixes 81c0a2bb515f ("mm: page_alloc: fair zone allocator policy")Reported-by Vlastimil Babka <[email protected]>Reported-by Leon Romanovsky <[email protected]>Signed-off-by Johannes Weiner <[email protected]>Acked-by Mel Gorman <[email protected]>Cc "3.12+" <[email protected]>--- mm/page_alloc.c | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-)Sorry I forgot to CC you, Leon. Resend with updated Tags.
mm/page_alloc.c +3
-4@@ -1612,7 +1612,7 @@ again: }__mod_zone_page_state (zone,NR_ALLOC_BATCH , -(1 << order)); -if (zone_page_state (zone,NR_ALLOC_BATCH ) ==0 && +if (atomic_long_read (&zone->vm_stat[NR_ALLOC_BATCH ]) <=0 && !zone_is_fair_depleted (zone))zone_set_flag (zone,ZONE_FAIR_DEPLETED );@@ -5701,9 +5701,8 @@ static void __setup_per_zone_wmarks(void) zone->watermark[WMARK_HIGH ] =min_wmark_pages (zone) + (tmp >>1 );__mod_zone_page_state (zone,NR_ALLOC_BATCH , -high_wmark_pages (zone) - -low_wmark_pages (zone) - -zone_page_state (zone, NR_ALLOC_BATCH)); +high_wmark_pages (zone) -low_wmark_pages (zone) - + atomic_long_read(&zone->vm_stat[NR_ALLOC_BATCH]));Quoted 3 lines
setup_zone_migrate_reserve(zone); spin_unlock_irqrestore(&zone->lock, flags);Quoted 5 lines
I think the better way will be to apply Mel's patch
https://lkml.org/lkml/2014/9/8/214 which fixzone_page_stateshadow casting issue and convert allatomic_long_read(&zone->vm_stat[NR_ALLOC_BATCH])) tozone_page__state(zone, NR_ALLOC_BATCH). This move will unify access tovm_stat.
Quoted 4 lines
It's not as simple. The counter can go way negative and we need that negative number, not 0, to calculate the reset delta. As I said in response to Mel's patch, we could make the vmstat API signed but I'm not convinced that is reasonable, given the 99% majority of usecases.
You are right, I missed that NR_ALLOC_BATCH is in use as a part of calculations
+ high_wmark_pages(zone) - low_wmark_pages(zone) -
+ atomic_long_read(&zone->vm_stat[NR_ALLOC_BATCH]));
Sorry
Signature · 8 lines
Leon Romanovsky | Independent Linux Consultant
www.leon.nu | [email protected]
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>mm/page_alloc.c static void __setup_per_zone_wmarks(void) · line 5705
Quoted 13 lines · Leon Romanovsky
Quoted 10 lines
Quoted 5 lines
I think the better way will be to apply Mel's patch
https://lkml.org/lkml/2014/9/8/214 which fixzone_page_stateshadow casting issue and convert allatomic_long_read(&zone->vm_stat[NR_ALLOC_BATCH])) tozone_page__state(zone, NR_ALLOC_BATCH). This move will unify access tovm_stat.It's not as simple. The counter can go way negative and we need that negative number, not 0, to calculate the reset delta. As I said in response to Mel's patch, we could make the vmstat API signed but I'm not convinced that is reasonable, given the 99% majority of usecases.
You are right, I missed that
NR_ALLOC_BATCHis in use as a part of calculationsmm/page_alloc.c +2
-0+high_wmark_pages (zone) -low_wmark_pages (zone) - + atomic_long_read(&zone->vm_stat[NR_ALLOC_BATCH]));
How about creating __zone_page_state for zone_page_state without the 0 check? That would be much nicer and would move the stuff to a central place. Given the nastiness of this issue there are bound to be more fixes coming up.
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>