Re: [patch 0/2] mm: too_many_isolated can stall due to out of sync VM counters

[Marcelo Tosatti]

On Tue, Nov 14, 2023 at 01:46:41PM +0100, Michal Hocko wrote:
> On Tue 14-11-23 09:26:53, Marcelo Tosatti wrote:
> > Hi Michal,
> > 
> > On Tue, Nov 14, 2023 at 09:20:09AM +0100, Michal Hocko wrote:
> > > On Mon 13-11-23 20:34:20, Marcelo Tosatti wrote:
> > > > A customer reported seeing processes hung at too_many_isolated,
> > > > while analysis indicated that the problem occurred due to out
> > > > of sync per-CPU stats (see below).
> > > > 
> > > > Fix is to use node_page_state_snapshot to avoid the out of stale values.
> > > > 
> > > > 2136 static unsigned long
> > > >     2137 shrink_inactive_list(unsigned long nr_to_scan, struct lruvec *lruvec,
> > > >     2138                      struct scan_control *sc, enum lru_list lru)
> > > >     2139 {
> > > >     :
> > > >     2145         bool file = is_file_lru(lru);
> > > >     :
> > > >     2147         struct pglist_data *pgdat = lruvec_pgdat(lruvec);
> > > >     :
> > > >     2150         while (unlikely(too_many_isolated(pgdat, file, sc))) {
> > > >     2151                 if (stalled)
> > > >     2152                         return 0;
> > > >     2153
> > > >     2154                 /* wait a bit for the reclaimer. */
> > > >     2155                 msleep(100);   <--- some processes were sleeping here, with pending SIGKILL.
> > > >     2156                 stalled = true;
> > > >     2157
> > > >     2158                 /* We are about to die and free our memory. Return now. */
> > > >     2159                 if (fatal_signal_pending(current))
> > > >     2160                         return SWAP_CLUSTER_MAX;
> > > >     2161         }
> > > > 
> > > > msleep() must be called only when there are too many isolated pages:
> > > 
> > > What do you mean here?
> > 
> > That msleep() must not be called when
> > 
> > isolated > inactive
> > 
> > is false.
> 
> Well, but the code is structured in a way that this is simply true.
> too_many_isolated might be false positive because it is a very loose
> interface and the number of isolated pages can fluctuate depending on
> the number of direct reclaimers.
>  
> > > >     2019 static int too_many_isolated(struct pglist_data *pgdat, int file,
> > > >     2020                 struct scan_control *sc)
> > > >     2021 {
> > > >     :
> > > >     2030         if (file) {
> > > >     2031                 inactive = node_page_state(pgdat, NR_INACTIVE_FILE);
> > > >     2032                 isolated = node_page_state(pgdat, NR_ISOLATED_FILE);
> > > >     2033         } else {
> > > >     :
> > > >     2046         return isolated > inactive;
> > > > 
> > > > The return value was true since:
> > > > 
> > > >     crash> p ((struct pglist_data *) 0xffff00817fffe580)->vm_stat[NR_INACTIVE_FILE]
> > > >     $8 = {
> > > >       counter = 1
> > > >     }
> > > >     crash> p ((struct pglist_data *) 0xffff00817fffe580)->vm_stat[NR_ISOLATED_FILE]
> > > >     $9 = {
> > > >       counter = 2
> > > > 
> > > > while per_cpu stats had:
> > > > 
> > > >     crash> p ((struct pglist_data *) 0xffff00817fffe580)->per_cpu_nodestats
> > > >     $85 = (struct per_cpu_nodestat *) 0xffff8000118832e0
> > > >     crash> p/x 0xffff8000118832e0 + __per_cpu_offset[42]
> > > >     $86 = 0xffff00917fcc32e0
> > > >     crash> p ((struct per_cpu_nodestat *) 0xffff00917fcc32e0)->vm_node_stat_diff[NR_ISOLATED_FILE]
> > > >     $87 = -1 '\377'
> > > > 
> > > >     crash> p/x 0xffff8000118832e0 + __per_cpu_offset[44]
> > > >     $89 = 0xffff00917fe032e0
> > > >     crash> p ((struct per_cpu_nodestat *) 0xffff00917fe032e0)->vm_node_stat_diff[NR_ISOLATED_FILE]
> > > >     $91 = -1 '\377'
> > > 
> > > This doesn't really tell much. How much out of sync they really are
> > > cumulatively over all cpus?
> > 
> > This is the cumulative value over all CPUs (offsets for other CPUs 
> > have been omitted since they are zero).
> 
> OK, so that means the NR_ISOLATED_FILE is 0 while NR_INACTIVE_FILE is 1,
> correct? If that is the case then the value is indeed outdated but it
> also means that the NR_INACTIVE_FILE is so small that all but 1 (resp. 2
> as kswapd is never throttled) reclaimers will be stalled anyway. So does
> the exact snapshot really help? Do you have any means to reproduce this
> behavior and see that the patch actually changed the behavior?
> 
> [...]
> 
> > > With a very low NR_FREE_PAGES and many contending allocation the system
> > > could be easily stuck in reclaim. What are other reclaim
> > > characteristics? 
> > 
> > I can ask. What information in particular do you want to know?
> 
> When I am dealing with issues like this I heavily rely on /proc/vmstat
> counters and pgscan, pgsteal counters to see whether there is any
> progress over time.
> 
> > > Is the direct reclaim successful? 
> > 
> > Processes are stuck in too_many_isolated (unnecessarily). What do you mean when you ask
> > "Is the direct reclaim successful", precisely?
> 
> With such a small LRU list it is quite likely that many processes will
> be competing over last pages on the list while rest will be throttled
> because there is nothing to reclaim. It is quite possible that all
> reclaimers will be waiting for a single reclaimer (either kswapd or
> other direct reclaimer). I would like to understand whether the system
> is stuck in unproductive state where everybody just waits until the
> counter is synced or everything just progress very slowly because of the
> small LRU. 
> -- 
> Michal Hocko
> SUSE Labs

Michal,

I think this provides the data you are looking for:

It seems that the situation was invoking memory-consuming user program
in pallarel expecting that the system will kick oom-killer at the end.

The node 0-3 are small containing system data and almost all files.
The node 4-7 are large prepared to contain user data only. 
The issue described in above was observed on node 4-7, where
had very few memory for files.

The node 4-7 has more cpu than node 0-3.
Only cpus on node 4-7 are configuerd to be nohz_full.
So we often found unflushed percpu vmstat on cpus of node 4-7.