On Fri, Jun 21, 2019 at 02:09:31PM +0200, Vlastimil Babka wrote: > On 6/21/19 1:43 PM, Alan Jenkins wrote: > > When setting the low and high watermarks we use min_wmark_pages(zone). > > I guess this is to reduce the line length. But we forgot that this macro > > includes zone->watermark_boost. We need to reset zone->watermark_boost > > first. Otherwise the watermarks will be set inconsistently. > > > > E.g. this could cause inconsistent values if the watermarks have been > > boosted, and then you change a sysctl which triggers > > __setup_per_zone_wmarks(). > > > > I strongly suspect this explains why I have seen slightly high watermarks. > > Suspicious-looking zoneinfo below - notice high-low != low-min. > > > > Node 0, zone Normal > > pages free 74597 > > min 9582 > > low 34505 > > high 36900 > > > > https://unix.stackexchange.com/questions/525674/my-low-and-high-watermarks-seem-higher-than-predicted-by-documentation-sysctl-vm/525687 > > > > Signed-off-by: Alan Jenkins <alan.christopher.jenkins@xxxxxxxxx> > > Fixes: 1c30844d2dfe ("mm: reclaim small amounts of memory when an external > > fragmentation event occurs") > > Cc: stable@xxxxxxxxxxxxxxx > > Nice catch, thanks! > > Acked-by: Vlastimil Babka <vbabka@xxxxxxx> > > Personally I would implement it a bit differently, see below. If you > agree, it's fine if you keep the authorship of the whole patch. > > > --- > > > > Tested by compiler :-). > > > > Ideally the commit message would be clear about what happens the > > *first* time __setup_per_zone_watermarks() is called. I guess that > > zone->watermark_boost is *usually* zero, or we would have noticed > > some wild problems :-). However I am not familiar with how the zone > > structures are allocated & initialized. Maybe there is a case where > > zone->watermark_boost could contain an arbitrary unitialized value > > at this point. Can we rule that out? > > Dunno if there's some arch override, but generic_alloc_nodedata() uses > kzalloc() so it's zeroed. > > -----8<----- > diff --git a/mm/page_alloc.c b/mm/page_alloc.c > index d66bc8abe0af..3b2f0cedf78e 100644 > --- a/mm/page_alloc.c > +++ b/mm/page_alloc.c > @@ -7624,6 +7624,7 @@ static void __setup_per_zone_wmarks(void) > > for_each_zone(zone) { > u64 tmp; > + unsigned long wmark_min; > > spin_lock_irqsave(&zone->lock, flags); > tmp = (u64)pages_min * zone_managed_pages(zone); > @@ -7642,13 +7643,13 @@ static void __setup_per_zone_wmarks(void) > > min_pages = zone_managed_pages(zone) / 1024; > min_pages = clamp(min_pages, SWAP_CLUSTER_MAX, 128UL); > - zone->_watermark[WMARK_MIN] = min_pages; > + wmark_min = min_pages; > } else { > /* > * If it's a lowmem zone, reserve a number of pages > * proportionate to the zone's size. > */ > - zone->_watermark[WMARK_MIN] = tmp; > + wmark_min = tmp; > } > > /* > @@ -7660,8 +7661,9 @@ static void __setup_per_zone_wmarks(void) > mult_frac(zone_managed_pages(zone), > watermark_scale_factor, 10000)); > > - zone->_watermark[WMARK_LOW] = min_wmark_pages(zone) + tmp; > - zone->_watermark[WMARK_HIGH] = min_wmark_pages(zone) + tmp * 2; > + zone->_watermark[WMARK_MIN] = wmark_min; > + zone->_watermark[WMARK_LOW] = wmark_min + tmp; > + zone->_watermark[WMARK_HIGH] = wmark_min + tmp * 2; > zone->watermark_boost = 0; Do you think this could cause a race condition between __setup_per_zone_wmarks and pgdat_watermark_boosted which checks whether the watermark_boost of each zone is non-zero? pgdat_watermark_boosted is not called with a zone lock. Here is a probable case scenario: watermarks are boosted in steal_suitable_fallback(which happens under a zone lock). After that kswapd is woken up by wakeup_kswapd(zone,0,0,zone_idx(zone)) in rmqueue without holding a zone lock. Lets say someone modified min_kfree_bytes, this would lead to all the zone->watermark_boost being set to 0. This may cause pgdat_watermark_boosted to return false, which would not wakeup kswapd as intended by boosting the watermark. This behaviour is similar to waking up kswapd for a balanced node. Also if kswapd was woken up successfully because of watermarks being boosted. In balance_pgdat, we use nr_boost_reclaim to count number of pages to reclaim because of boosting. nr_boost_reclaim is calculated as: nr_boost_reclaim = 0; for (i = 0; i <= classzone_idx; i++) { zone = pgdat->node_zones + i; if (!managed_zone(zone)) continue; nr_boost_reclaim += zone->watermark_boost; zone_boosts[i] = zone->watermark_boost; } boosted = nr_boost_reclaim; This is not under a zone_lock. This could lead to nr_boost_reclaim to be 0 if min_kfree_bytes is set to 0. Which would wake up kcompactd without reclaiming memory. kcompactd compaction might be spurious if the if the memory reclaim step is not happening? Any thoughts? > spin_unlock_irqrestore(&zone->lock, flags); >
