From: Yu Zhao <yuzhao@google.com>
To: Johannes Weiner <hannes@cmpxchg.org>
Cc: Brian Foster <bfoster@redhat.com>, Nhat Pham <nphamcs@gmail.com>,
akpm@linux-foundation.org, linux-mm@kvack.org,
linux-kernel@vger.kernel.org, willy@infradead.org,
linux-api@vger.kernel.org, kernel-team@meta.com
Subject: Re: [PATCH v6 1/3] workingset: refactor LRU refault to expose refault recency check
Date: Fri, 20 Jan 2023 20:43:55 -0700 [thread overview]
Message-ID: <CAOUHufYYMDxHPFEAKaXbvse8bjSVJp51hmHBG+3u9mTTstjFow@mail.gmail.com> (raw)
In-Reply-To: <Y8rA4C6qnT5InHGc@cmpxchg.org>
On Fri, Jan 20, 2023 at 9:26 AM Johannes Weiner <hannes@cmpxchg.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.
next prev parent reply other threads:[~2023-01-21 3:44 UTC|newest]
Thread overview: 12+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-01-17 19:59 [PATCH v6 0/3] cachestat: a new syscall for page cache state of files Nhat Pham
2023-01-17 19:59 ` [PATCH v6 1/3] workingset: refactor LRU refault to expose refault recency check Nhat Pham
2023-01-20 14:34 ` Brian Foster
2023-01-20 16:27 ` Johannes Weiner
2023-01-20 18:53 ` Brian Foster
2023-01-21 3:43 ` Yu Zhao [this message]
2023-01-20 17:29 ` Nhat Pham
2023-01-20 18:56 ` Brian Foster
2023-01-17 19:59 ` [PATCH v6 2/3] cachestat: implement cachestat syscall Nhat Pham
2023-01-20 14:36 ` Brian Foster
2023-01-20 18:26 ` Nhat Pham
2023-01-17 19:59 ` [PATCH v6 3/3] selftests: Add selftests for cachestat Nhat Pham
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=CAOUHufYYMDxHPFEAKaXbvse8bjSVJp51hmHBG+3u9mTTstjFow@mail.gmail.com \
--to=yuzhao@google.com \
--cc=akpm@linux-foundation.org \
--cc=bfoster@redhat.com \
--cc=hannes@cmpxchg.org \
--cc=kernel-team@meta.com \
--cc=linux-api@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-mm@kvack.org \
--cc=nphamcs@gmail.com \
--cc=willy@infradead.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).