linux-kernel - Re: [PATCH v10 1/3] workingset: refactor LRU refault to expose refault recency check

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAKEwX=NfD-57zAgXBoCVFVHu_TDbbnng3piZTu7iGjALpj=Tug@mail.gmail.com>
Date:   Tue, 21 Feb 2023 00:49:00 -0800
From:   Nhat Pham <nphamcs@...il.com>
To:     Matthew Wilcox <willy@...radead.org>
Cc:     akpm@...ux-foundation.org, hannes@...xchg.org, linux-mm@...ck.org,
        linux-kernel@...r.kernel.org, bfoster@...hat.com, arnd@...db.de,
        linux-api@...r.kernel.org, kernel-team@...a.com
Subject: Re: [PATCH v10 1/3] workingset: refactor LRU refault to expose
 refault recency check

On Sun, Feb 19, 2023 at 4:05 AM Matthew Wilcox <willy@...radead.org> wrote:
>
> On Sat, Feb 18, 2023 at 11:33:16PM -0800, Nhat Pham wrote:
> > +++ 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.
> > + */
>
> I find this comment hard to understand.  First it talks about "the
> folio", but it doesn't pass a folio.  Then it talks about "the
> references", but I don't have any idea what those are either.
>
> I think what you mean is,
>
>  * Test if the shadow entry is for a folio which was recently evicted.
>  * Fills in @memcgid, @pgdat, @token and @workingset with values
>  * extracted from the shadow entry.

I'll fix this comment in the next version. Thanks for the suggestion!

>
> > +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));
> > +}
>
> [...]
>
> > +/*
> > + * Test if the folio is recently evicted by checking if
> > + * refault distance of shadow exceeds workingset size.
> >   *
> > - * Calculates and evaluates the refault distance of the previously
> > - * evicted folio in the context of the node and the memcg whose memory
> > - * pressure caused the eviction.
> > + * As a side effect, populate workingset with the value
> > + * unpacked from shadow.
> >   */
>
> 1. Shouldn't this be kernel-doc?

Hmm good point...

> 2. Again, don't use the term "side effect" here.  It's just one of
> the things that the function _does_.
>

I'll fix this! Thanks for pointing it out.