Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Thu, 6 Apr 2023 16:02:13 -0400
From:   Johannes Weiner <hannes@cmpxchg.org>
To:     Yu Zhao <yuzhao@google.com>
Cc:     Yosry Ahmed <yosryahmed@google.com>,
        Andrew Morton <akpm@linux-foundation.org>,
        linux-kernel@vger.kernel.org, linux-mm@kvack.org,
        Shakeel Butt <shakeelb@google.com>,
        Michal Hocko <mhocko@suse.com>
Subject: Re: global_reclaim() and code documentation (was: Re: [PATCH v3 3/3]
 mm: vmscan: ignore non-LRU-based reclaim in memcg reclaim
Message-ID: <20230406200213.GA50192@cmpxchg.org>
References: <20230405200150.GA35884@cmpxchg.org>
 <CAOUHufYCxdAXtw_b1qFT_02L3gWcs3of6Y0-5mmLwr+CeWvLQQ@mail.gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <CAOUHufYCxdAXtw_b1qFT_02L3gWcs3of6Y0-5mmLwr+CeWvLQQ@mail.gmail.com>
Precedence: bulk

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@cmpxchg.org> 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.