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;
MIME-Version: 1.0
References: <CAHbLzkoXzbHRJxb9DkjGkKQ8TAO08ctvz7wvjyPA8Gy2Skm+2g@mail.gmail.com>
 <Y1hM6sMRpBGUPRyo@cmpxchg.org> <CAHbLzkpaoN37camSLYVDU7p9AXzQjYcvHnWm3K87iwae-YyZiQ@mail.gmail.com>
 <Y1lvJBnVx1Fv5WHz@cmpxchg.org> <CAHbLzkqQ=6U3uerEkypsCHnmsXerEZi5erMSYK-kp8-vJNk89Q@mail.gmail.com>
 <CAJD7tkb5y9oqgVauVPiS0KbiL2Wnsu7jhK7Q44oUBZzBXwKUYA@mail.gmail.com>
 <Y1qSZHK/U0SpNqNa@cmpxchg.org> <CAJD7tkYt-KL=jDEy6pSOc5tDij=3SWmbhFeco39pjJuOmEAH0g@mail.gmail.com>
 <Y1vprODaLJLk0dka@cmpxchg.org> <CAJD7tkbBbh+uXe7S=a0E5=FBX4wVa7YDJDLmti370v2sVWVtWw@mail.gmail.com>
 <Y1/xBYZOyjoRrIoJ@cmpxchg.org>
In-Reply-To: <Y1/xBYZOyjoRrIoJ@cmpxchg.org>
From:   Yosry Ahmed <yosryahmed@google.com>
Date:   Mon, 31 Oct 2022 09:46:00 -0700
Message-ID: <CAJD7tkYTumMaromVY85Ncg_q6jK0rKkwUv2jRdW3Cdg0n3YJjg@mail.gmail.com>
Subject: Re: [PATCH] mm: vmscan: split khugepaged stats from direct reclaim stats
To:     Johannes Weiner <hannes@cmpxchg.org>
Cc:     Yang Shi <shy828301@gmail.com>,
        Andrew Morton <akpm@linux-foundation.org>, linux-mm@kvack.org,
        linux-kernel@vger.kernel.org, Eric Bergen <ebergen@meta.com>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Mon, Oct 31, 2022 at 9:00 AM Johannes Weiner <hannes@cmpxchg.org> wrote:
>
> On Fri, Oct 28, 2022 at 10:41:17AM -0700, Yosry Ahmed wrote:
> > On Fri, Oct 28, 2022 at 7:39 AM Johannes Weiner <hannes@cmpxchg.org> wrote:
> > > pgscan_user: User-requested reclaim. Could be confusing if we ever
> > > have an in-kernel proactive reclaim driver - unless that would then go
> > > to another counter (new or kswapd).
> > >
> > > pgscan_ext: Reclaim activity from extraordinary/external
> > > requests. External as in: outside the allocation context.
> >
> > I imagine if the kernel is doing proactive reclaim on its own, we
> > might want a separate counter for that anyway to monitor what the
> > kernel is doing. So maybe pgscan_user sounds nice for now, but I also
> > like that the latter explicitly says "this is external to the
> > allocation context". But we can just go with pgscan_user and document
> > it properly.
>
> Yes, I think you're right. pgscan_user sounds good to me.
>
> > How would khugepaged fit in this story? Seems like it would be part of
> > pgscan_ext but not pgscan_user. I imagine we also don't want to
> > pollute proactive reclaim counters with khugepaged reclaim (or other
> > non-direct reclaim).
> >
> > Maybe pgscan_user and pgscan_kernel/pgscan_indirect for things like khugepaged?
> > The problem with pgscan_kernel/indirect is that if we add a proactive
> > reclaim kthread in the future it would technically fit there but we
> > would want a separate counter for it.
> >
> > I am honestly not sure where to put khugepaged. The reasons I don't
> > like a dedicated counter for khugepaged are:
> > - What if other kthreads like khugepaged start doing the same, do we
> > add one counter per-thread?
>
> It's unlikely there will be more.
>
> The reason khugepaged doesn't rely on kswapd is unique to THP
> allocations: they can require an exorbitant amount of work to
> assemble, but due to fragmentation those requests may fail
> permanently. We don't want to burden a shared facility like kswapd
> with large amounts of speculative work on behalf of what are (still*)
> cornercase requests.
>
> This isn't true for other allocations. We do have __GFP_NORETRY sites
> here and there that rather fall back early than put in the full amount
> of work; but overall we expect allocations to succeed - and kswapd to
> be able to balance for them!!** - because the alternative tends to be
> OOMs, or drivers and workloads aborting on -ENOMEM.
>
> (* As we evolve the allocator and normalize huge page requests
>    (folios), kswapd may also eventually balance for THPs again. IOW,
>    it's more likely for this exception to disappear again than it is
>    that we'll see more of them.)
>
> (** This is also why it's no big deal if other kthreads that rely on
>     kswapd contribute to direct reclaim stats. First, it's highly
>     error prone to determine on a case by case basis whether userspace
>     could be waiting behind that direct reclaim - as Yang Shi's
>     writeback example demonstrates. Second, if kswapd is overwhelmed,
>     it's likely to impact userspace *anyway*! The benefit of this
>     classification work is questionable.)

Thanks for the explanation :)

>
> > - What if we deprecate khugepaged (or such threads)? Seems more likely
> > than deprecating kswapd.
>
> If that happens, we can remove the counter again. The bar isn't as
> high for vmstat as it for other ABI, and we've updated it plenty of
> times to reflect changes in the MM implementation.

Good to know! I thought we'd be stuck with it forever.

>
> > Looks like we want a stat that would group all of this reclaim coming
> > from non-direct kthreads, but would not include a future proactive
> > reclaim kthread.
>
> I think the desire to generalize overcomplicates things here in a way
> that isn't actually meaningful.
>
> Think of direct reclaim stats as a signal that either a) kswapd is
> broken or b) memory pressure is high enough to cause latencies in the
> class of requests that are of interest to userspace. This is true for
> all cases but khugepaged.

Agreed. I believe moving forward with pgscan_user and
pgscan_khugepaged style stats makes sense.

Thanks, Johannes!