Re: global_reclaim() and code documentation (was: Re: [PATCH v3 3/3] mm: vmscan: ignore non-LRU-based reclaim in memcg reclaim

[Yu Zhao]

On Fri, Apr 7, 2023 at 12:03 AM Yosry Ahmed <yosryahmed@xxxxxxxxxx> wrote:
>
> On Thu, Apr 6, 2023 at 1:02 PM Johannes Weiner <hannes@xxxxxxxxxxx> wrote:
> >
> > On Wed, Apr 05, 2023 at 03:09:27PM -0600, Yu Zhao wrote:
> > > On Wed, Apr 5, 2023 at 2:01 PM Johannes Weiner <hannes@xxxxxxxxxxx> wrote:
> > > > static bool cgroup_reclaim(struct scan_control *sc)
> > > > {
> > > >         return sc->target_mem_cgroup;
> > > > }
> > > >
> > > > static bool global_reclaim(struct scan_control *sc)
> > > > {
> > > >         return !sc->target_mem_cgroup || mem_cgroup_is_root(sc->target_mem_cgroup);
> > > > }
> > > >
> > > > The name suggests it's the same thing twice, with opposite
> > > > polarity. But of course they're subtly different, and not documented.
> > > >
> > > > When do you use which?
> > >
> > > The problem I saw is that target_mem_cgroup is set when writing to the
> > > root memory.reclaim. And for this case, a few places might prefer
> > > global_reclaim(), e.g., in shrink_lruvec(), in addition to where it's
> > > being used.
> > >
> > > If this makes sense, we could 1) document it (or rename it) and apply
> > > it to those places, or 2) just unset target_mem_cgroup for root and
> > > delete global_reclaim(). Option 2 might break ABI but still be
> > > acceptable.
> >
> > Ah, cgroup_reclaim() tests whether it's limit/proactive reclaim or
> > allocator reclaim. global_reclaim() tests whether it's root reclaim
> > (which could be from either after memory.reclaim).
> >
> > I suppose we didn't clarify when introducing memory.reclaim what the
> > semantics should be on the root cgroup:
> >
> > - We currently exclude PGSCAN and PGSTEAL stats from /proc/vmstat for
> >   limit reclaim to tell cgroup constraints from physical pressure. We
> >   currently exclude root memory.reclaim activity as well. Seems okay.
> >
> > - The file_is_tiny heuristic is for allocator reclaim near OOM. It's
> >   currently excluded for root memory.reclaim, which seems okay too.
> >
> > - Like limit reclaim, root memory.reclaim currently NEVER swaps when
> >   global swappiness is 0. The whole cgroup-specific swappiness=0
> >   semantic is kind of quirky. But I suppose we can keep it as-is.
> >
> > - Proportional reclaim is disabled for everybody but direct reclaim
> >   from the allocator at initial priority. Effectively disabled for all
> >   situations where it might matter, including root memory.reclaim. We
> >   should also keep this as-is.
> >
> > - Writeback throttling is interesting. Historically, kswapd set the
> >   congestion state when running into lots of PG_reclaim pages, and
> >   clear it when the node is balanced. This throttles direct reclaim.
> >
> >   Cgroup limit reclaim would set and clear congestion on non-root only
> >   to do local limit-reclaim throttling. But now root memory.reclaim
> >   might clear a bit set by kswapd before the node is balanced, and
> >   release direct reclaimers from throttling prematurely. This seems
> >   wrong. However, the alternative is throttling memory.reclaim on
> >   subgroup congestion but not root, which seems also wrong.
> >
> > - Root memory.reclaim is exempted from the compaction_ready() bail
> >   condition as well as soft limit reclaim. But they'd both be no-ops
> >   anyway if we changed cgroup_reclaim() semantics.
> >
> > IMO we should either figure out what we want to do in those cases
> > above, at least for writeback throttling.
> >
> > Are you guys using root-level proactive reclaim?
> >
> > > If we don't want to decide right now, I can rename global_reclaim() to
> > > root_reclaim() and move it within #ifdef CONFIG_LRU_GEN and probably
> > > come back and revisit later.
> >
> > So conventional vmscan treats root-level memory.reclaim the same as
> > any other cgroup reclaim. And the cgroup_reclaim() checks are mostly
> > reserved for (questionable) allocator reclaim-specific heuristics.
> >
> > Is there a chance lrugen could do the same, and you'd be fine with
> > using cgroup_reclaim()? Or is it a data structure problem?
> >
> > If so, I agree it could be better to move it to CONFIG_LRU_GEN and
> > rename it for the time being. root_reclaim() SGTM.
>
> Oh and if we decide to keep the helper as root_reclaim I would prefer
> it to be outside CONFIG_LRU_GEN so that I can still use it in the
> patch series that this thread was originally a part of (ignoring
> non-LRU freed pages in memcg reclaim).

Thanks for the summaries. I think you both covered all the important details.

Just to recap -- it all comes down to how we want to define the
semantics for the root memory.reclaim for *future* use case, since
currently it has no known users (I personally use it for testing
purposes, which is not important at all). So I'm fine with whatever
you two see fit (delete, refactor or rename).