Re: [PATCH v2 2/4] workingset: refactor LRU refault to expose refault recency check

[Yu Zhao]

On Mon, Dec 5, 2022 at 6:19 PM Nhat Pham <nphamcs@xxxxxxxxx> wrote:
>
> On Mon, Dec 5, 2022 at 3:49 PM Yu Zhao <yuzhao@xxxxxxxxxx> wrote:
> >
> > On Mon, Dec 5, 2022 at 10:51 AM Nhat Pham <nphamcs@xxxxxxxxx> wrote:
> > >
> > > In preparation for computing recently evicted pages in cachestat,
> > > refactor workingset_refault and lru_gen_refault to expose a helper
> > > function that would test if an evicted page is recently evicted.
> > >
> > > Signed-off-by: Nhat Pham <nphamcs@xxxxxxxxx>
> > > ---
> > >  include/linux/swap.h |   1 +
> > >  mm/workingset.c      | 143 +++++++++++++++++++++++++++++--------------
> > >  2 files changed, 99 insertions(+), 45 deletions(-)
> > >
> > > diff --git a/include/linux/swap.h b/include/linux/swap.h
> > > index a18cf4b7c724..dae6f6f955eb 100644
> > > --- a/include/linux/swap.h
> > > +++ b/include/linux/swap.h
> > > @@ -361,6 +361,7 @@ static inline void folio_set_swap_entry(struct folio *folio, swp_entry_t entry)
> > >  }
> > >
> > >  /* linux/mm/workingset.c */
> > > +bool workingset_test_recent(void *shadow, bool file, bool *workingset);
> > >  void workingset_age_nonresident(struct lruvec *lruvec, unsigned long nr_pages);
> > >  void *workingset_eviction(struct folio *folio, struct mem_cgroup *target_memcg);
> > >  void workingset_refault(struct folio *folio, void *shadow);
> > > diff --git a/mm/workingset.c b/mm/workingset.c
> > > index 79585d55c45d..44b331ce3040 100644
> > > --- a/mm/workingset.c
> > > +++ b/mm/workingset.c
> > > @@ -244,6 +244,30 @@ static void *lru_gen_eviction(struct folio *folio)
> > >         return pack_shadow(mem_cgroup_id(memcg), pgdat, token, refs);
> > >  }
> > >
> > > +/*
> > > + * Test if the folio is recently evicted.
> > > + *
> > > + * As a side effect, also populates the references with
> > > + * values unpacked from the shadow of the evicted folio.
> > > + */
> > > +static bool lru_gen_test_recent(void *shadow, bool file, int *memcgid,
> > > +       struct pglist_data **pgdat, unsigned long *token, bool *workingset)
> > > +{
> > > +       struct mem_cgroup *eviction_memcg;
> > > +       struct lruvec *lruvec;
> > > +       struct lru_gen_struct *lrugen;
> > > +       unsigned long min_seq;
> > > +
> > > +       unpack_shadow(shadow, memcgid, pgdat, token, workingset);
> > > +       eviction_memcg = mem_cgroup_from_id(*memcgid);
> > > +
> > > +       lruvec = mem_cgroup_lruvec(eviction_memcg, *pgdat);
> > > +       lrugen = &lruvec->lrugen;
> > > +
> > > +       min_seq = READ_ONCE(lrugen->min_seq[file]);
> > > +       return !((*token >> LRU_REFS_WIDTH) != (min_seq & (EVICTION_MASK >> LRU_REFS_WIDTH)));
> > > +}
> >
> > Nit: not refactoring actually looks cleaner to me -- there are only a
> > few lines of duplicated code and you can get rid of 4 parameters
> > including the unused workingset in the next patch.
>
> (resending this because I forgot to forward to the rest of the
> group!)
>
> Thanks for the comment, Yu!
>
> Personally, I prefer refactoring this piece of logic - I do think that
> it is cleaner than copying the logic into the syscall implementation.

Let me clarify.

You can add
  lru_gen_test_recent(void *shadow, bool file)
without refactoring the existing
  lru_gen_refault().

Set the boilerplate aside, you only repeat one line of code, which is
the last line in the new function.

(The boilerplate code is repeated in many places, and that's why it's
called boilerplate.)

> I believe that if I don't do the refactoring, I'll have to repeat the
> unused parameters in the syscall, and make unpack_shadow
> a non-static function (along with all the locally defined macros like
> WORKINGSET_SHIFT). I think it would get quite messy there too.
>
> But more importantly, I'm a bit concerned that the logic for
> determining the recency of the eviction might change in the
> future, which would break the cachestat syscall unknowingly...
>
> Let me know what you think about this, Yu!

Your call :)