Re: [PATCH v4 0/3] Ignore non-LRU-based reclaim in memcg reclaim

[Yosry Ahmed]

On Tue, Apr 4, 2023 at 3:31 PM Andrew Morton <akpm@xxxxxxxxxxxxxxxxxxxx> wrote:
>
> On Tue, 4 Apr 2023 15:28:16 -0700 Andrew Morton <akpm@xxxxxxxxxxxxxxxxxxxx> wrote:
>
> > On Tue, 4 Apr 2023 15:00:57 -0700 Yosry Ahmed <yosryahmed@xxxxxxxxxx> wrote:
> >
> > > ...
> > >
> > > > >
> > > > > Without refactoring the code that adds reclaim_state->reclaimed to
> > > > > scan_control->nr_reclaimed into a helper (flush_reclaim_state()), the
> > > > > change would need to be done in two places instead of one, and I
> > > > > wouldn't know where to put the huge comment.
> > > >
> > > > Well, all depends on how desirable it it that we backport.  If "not
> > > > desirable" then leave things as-is.  If at least "possibly desirable"
> > > > then a simple patch with the two changes and no elaborate comment will
> > > > suit.
> > > >
> > >
> > > I would rather leave the current series as-is with an elaborate
> > > comment. I can send a separate single patch as a backport to stable if
> > > this is something that we usually do (though I am not sure how to
> > > format such patch).
> >
> > -stable maintainers prefer to take something which has already been
> > accepted by Linus.
> >
> > The series could be as simple as
> >
> > simple-two-liner.patch
> > revert-simple-two-liner.patch
> > this-series-as-is.patch
> >
> > simple-two-liner.patch goes into 6.3-rcX and -stable.  The other
> > patches into 6.4-rc1.
>
> But the key question remains: how desirable is a backport?
>
> Looking at the changelogs I'm not seeing a clear statement of the
> impact upon real-world users' real-world workloads.  (This is a hint).
> So I am unable to judge.
>
> Please share your thoughts on this.

I think it's nice to have but not really important. It occasionally
causes writes to memory.reclaim to report false positives and *might*
cause unnecessary retrying when charging memory, but probably too rare
to be a practical problem.

Personally, I intend to backport to our kernel at Google because it's
a simple enough fix and we have occasionally seen test flakiness
without it.

I have a reworked version of the series that only has 2 patches:
- simple-two-liner-patch (actually 5 lines)
- one patch including all refactoring squashed (introducing
flush_reclaim_state() with the huge comment, introducing
mm_account_reclaimed_pages(), and moving set_task_reclaim_state()
around).

Let me know if you want me to send it as v5, or leave the current v4
if you think backporting is not generally important.

>