On Fri, Jan 20, 2023 at 9:26 AM Johannes Weiner <hannes@xxxxxxxxxxx> 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@xxxxxxxxxxxxxx/ 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.
