Message-ID: <ZVNsMVPJ5y8C_hBC@tiehlicka>
Date:   Tue, 14 Nov 2023 13:46:41 +0100
From:   Michal Hocko <mhocko@...e.com>
To:     Marcelo Tosatti <mtosatti@...hat.com>
Cc:     linux-kernel@...r.kernel.org, linux-mm@...ck.org,
        Vlastimil Babka <vbabka@...e.cz>,
        Andrew Morton <akpm@...ux-foundation.org>,
        David Hildenbrand <david@...hat.com>,
        Peter Xu <peterx@...hat.com>
Subject: Re: [patch 0/2] mm: too_many_isolated can stall due to out of sync
 VM counters

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

Powered by blists - more mailing lists