Re: [PATCHv2] mm: bail out when the PMD has been set in bloom filter

["David Hildenbrand (Arm)" <david@kernel.org>]

On 3/4/26 04:15, zhaoyang.huang wrote:
> From: Zhaoyang Huang <zhaoyang.huang@unisoc.com>
> 
> Part of bloom filter utilization in MGLRU are listed below, in which we
> can see that the step '3' will prevent the hot PMD to be carried to new
> gen since the new arrived rmap_walk clears the page's young flag.
> This commit would like to suggest to query the PMD in bloom filter
> before starting the rmap walk to improve this. In terms of the cost,
> test_bloom_filter only consume 20~30 instructions in modern processors(25
> instructions in ARM64).

This is hard to follow, for example because

* You forward-reference things
* You talk about "this commit". A commit does not suggest anything,
  people to.
* It's unclear what the real benefit of the patch is or which problem it
  tries to solve.


You could start with a template like the following, where you clearly
express what is currently problematic, and why, and how you are
intending to optimize:

"Currently, lru_gen_look_around() does not properly consider the bloom
filter when processing a PTE. This is suboptimal, because TODO ...

To improve performance, let's consult the bloom filter for the PMD we
are processing, to just skip processing all PTEs in the PMD table.
"

In general, for performance optimizations that are not completely
obvious, we prefer actual proof that it is worth the additional code.

If it's a correctness issue, we should spell that out. I'm still not
sure what it is :)

> 
> 1. rmap_walk set suitable PMD in filters[max_seq] while all page's turn
>    to be non-young status
>    * rmap_walk->lru_gen_look_around->update_bloom_filter(max_seq)
> 2. young pages gathering on the PMD if it is a hot VM area
> 3. newly arrived rmap_walk in the same PMD clears page's young which
>    are set in step 2
> 4. walk_mm test the PMD again during aging, which will bring the
>    suitable PMD to filters[max_seq+1]
>    * walk_mm->walk_pmd_range->test_bloom_fitler->update_bloom_filter(
>    walk->seq + 1)
> 
> Tested-by: syzbot@syzkaller.appspotmail.com

That doesn't really count as "Tested". It just verified that the problem
in v1 no longer results in a splat, but it didn't really test if the
patch does the right thing.


> Signed-off-by: Zhaoyang Huang <zhaoyang.huang@unisoc.com>
> ---
> v2: fix null-ptr-ref of mm_state and update commit message
> ---
> ---
>  mm/vmscan.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/mm/vmscan.c b/mm/vmscan.c
> index 10f1e7d716ca..5558a24d1564 100644
> --- a/mm/vmscan.c
> +++ b/mm/vmscan.c
> @@ -4234,6 +4234,10 @@ bool lru_gen_look_around(struct page_vma_mapped_walk *pvmw)
>  	/* avoid taking the LRU lock under the PTL when possible */
>  	walk = current->reclaim_state ? current->reclaim_state->mm_walk : NULL;
>  
> +	/* may the pmd has been set in bloom filter */

That comment is not really helpful given that the code does exactly that.

> +	if (mm_state && test_bloom_filter(mm_state, max_seq, pvmw->pmd))
> +		return true;


-- 
Cheers,

David