linux-kernel - Re: [PATCH v6 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] [day] [month] [year] [list]

Message-ID: <CAOUHufYYMDxHPFEAKaXbvse8bjSVJp51hmHBG+3u9mTTstjFow@mail.gmail.com>
Date:   Fri, 20 Jan 2023 20:43:55 -0700
From:   Yu Zhao <yuzhao@...gle.com>
To:     Johannes Weiner <hannes@...xchg.org>
Cc:     Brian Foster <bfoster@...hat.com>, Nhat Pham <nphamcs@...il.com>,
        akpm@...ux-foundation.org, linux-mm@...ck.org,
        linux-kernel@...r.kernel.org, willy@...radead.org,
        linux-api@...r.kernel.org, kernel-team@...a.com
Subject: Re: [PATCH v6 1/3] workingset: refactor LRU refault to expose refault
 recency check

On Fri, Jan 20, 2023 at 9:26 AM Johannes Weiner <hannes@...xchg.org> wrote:
>
> On Fri, Jan 20, 2023 at 09:34:18AM -0500, Brian Foster wrote:
> > On Tue, Jan 17, 2023 at 11:59:57AM -0800, Nhat Pham wrote:
> > > +   int memcgid;
> > > +   struct pglist_data *pgdat;
> > > +   unsigned long token;
> > > +
> > > +   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)));
> >
> > I think this might be more readable without the double negative.
> >
> > Also it looks like this logic is pulled from lru_gen_refault(). Any
> > reason the caller isn't refactored to use this helper, similar to how
> > workingset_refault() is modified? It seems like a potential landmine to
> > duplicate the logic here for cachestat purposes and somewhere else for
> > actual workingset management.
>
> The initial version was refactored. Yu explicitly requested it be
> duplicated [1] to cut down on some boiler plate.
>
> I have to agree with Brian on this one, though. The factored version
> is better for maintenance than duplicating the core logic here. Even
> if it ends up a bit more boiler plate - it's harder to screw that up,
> and easier to catch at compile time, than the duplicates diverging.
>
> [1] https://lore.kernel.org/lkml/CAOUHufZKTqoD2rFwrX9-eCknBmeWqP88rZ7X7A_5KHHbGBUP=A@mail.gmail.com/

No objections to either way. I'll take a look at the final version and
we are good as long as it works as intended.