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

[Andrew Morton]

On Tue, 4 Apr 2023 14:49:13 -0700 Yosry Ahmed <yosryahmed@xxxxxxxxxx> wrote:

> On Tue, Apr 4, 2023 at 2:38 PM Andrew Morton <akpm@xxxxxxxxxxxxxxxxxxxx> wrote:
> >
> > On Tue,  4 Apr 2023 00:13:50 +0000 Yosry Ahmed <yosryahmed@xxxxxxxxxx> wrote:
> >
> > > Upon running some proactive reclaim tests using memory.reclaim, we
> > > noticed some tests flaking where writing to memory.reclaim would be
> > > successful even though we did not reclaim the requested amount fully.
> > > Looking further into it, I discovered that *sometimes* we over-report
> > > the number of reclaimed pages in memcg reclaim.
> > >
> > > Reclaimed pages through other means than LRU-based reclaim are tracked
> > > through reclaim_state in struct scan_control, which is stashed in
> > > current task_struct. These pages are added to the number of reclaimed
> > > pages through LRUs. For memcg reclaim, these pages generally cannot be
> > > linked to the memcg under reclaim and can cause an overestimated count
> > > of reclaimed pages. This short series tries to address that.
> > >
> > > Patches 1-2 are just refactoring, they add helpers that wrap some
> > > operations on current->reclaim_state, and rename
> > > reclaim_state->reclaimed_slab to reclaim_state->reclaimed.
> > >
> > > Patch 3 ignores pages reclaimed outside of LRU reclaim in memcg reclaim.
> > > The pages are uncharged anyway, so even if we end up under-reporting
> > > reclaimed pages we will still succeed in making progress during
> > > charging.
> > >
> > > Do not let the diff stat deceive you, the core of this series is patch 3,
> > > which has one line of code change. All the rest is refactoring and one
> > > huge comment.
> > >
> >
> > Wouldn't it be better to do this as a single one-line patch for
> > backportability?  Then all the refactoring etcetera can be added on
> > later.
> 
> 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.