Date:   Sun, 19 Feb 2023 12:05:41 +0000
From:   Matthew Wilcox <willy@infradead.org>
To:     Nhat Pham <nphamcs@gmail.com>
Cc:     akpm@linux-foundation.org, hannes@cmpxchg.org, linux-mm@kvack.org,
        linux-kernel@vger.kernel.org, bfoster@redhat.com, arnd@arndb.de,
        linux-api@vger.kernel.org, kernel-team@meta.com
Subject: Re: [PATCH v10 1/3] workingset: refactor LRU refault to expose
 refault recency check
Message-ID: <Y/IQlWdD1NvcUROv@casper.infradead.org>
References: <20230219073318.366189-1-nphamcs@gmail.com>
 <20230219073318.366189-2-nphamcs@gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20230219073318.366189-2-nphamcs@gmail.com>
Precedence: bulk

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.

> +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?
2. Again, don't use the term "side effect" here.  It's just one of
the things that the function _does_.