Re: [PATCH v3 2/2] ksm: Optimize rmap_walk_ksm by passing a suitable address range

[xu.xin16]

> On 4/9/26 13:59, David Hildenbrand (Arm) wrote:
> > On 4/9/26 12:56, xu.xin16@xxxxxxxxxx wrote:
> >>>
> >>> Hmm, maybe we could do the following. I think the other members are only
> >>> relevant for the unstable tree.
> >>
> >> Well, I suspect that "SmartScan-Related" members might be also needed and used even when
> >>
> >> it's a stable rmap_item. In should_skip_rmap_item(), if its page is KSM, it can't be skip.
> > 
> > Yes, needs some more thought on details. We might have to ignore/skip
> > the fields for stable tree entries that have not a KSM page.
> > 
> >>
> >> What if the rmap_item is stable, but its page is not KSM?
> > 
> > I guess that would happen if we had a rmap_item at that address, and
> > then changed the page (e.g., COW).
> > 
> > ksm_do_scan() would call cmp_and_merge_page() after obtaining such an
> > rmap item from scan_get_next_rmap_item().
> > 
> > In cmp_and_merge_page() we'd call remove_rmap_item_from_tree() and
> > recalculate the checksum.
> > 
> > In remove_rmap_item_from_tree() we remove the item from the stable tree.
> > 
> > So we'd want to ignore the entries in STABLE_FLAG in
> > scan_get_next_rmap_item() to then reinitialize the fields in
> > cmp_and_merge_page() after remove_rmap_item_from_tree() I guess.
> > 
> 

Yes

> Something like this on top:
> 
> diff --git a/mm/ksm.c b/mm/ksm.c
> index 0c6bfed280f7..51fd37ee24d6 100644
> --- a/mm/ksm.c
> +++ b/mm/ksm.c
> @@ -905,6 +905,8 @@ static void remove_node_from_stable_tree(struct ksm_stable_node *stable_node)
>                 VM_BUG_ON(stable_node->rmap_hlist_len <= 0);
>                 stable_node->rmap_hlist_len--;
>                 put_anon_vma(rmap_item->anon_vma);
> +               /* Reset pgoff that overlays age-related information. */
> +               rmap_item->pgoff = 0;
>                 rmap_item->address &= PAGE_MASK;
>                 cond_resched();
>         }
> @@ -1058,9 +1060,10 @@ static void remove_rmap_item_from_tree(struct ksm_rmap_item *rmap_item)
>                 stable_node->rmap_hlist_len--;
>  
>                 put_anon_vma(rmap_item->anon_vma);
> +               /* Reset pgoff that overlays age-related information. */
> +               rmap_item->pgoff = 0;
>                 rmap_item->head = NULL;
>                 rmap_item->address &= PAGE_MASK;
> -
>         } else if (rmap_item->address & UNSTABLE_FLAG) {
>                 unsigned char age;
>                 /*
> @@ -2465,6 +2468,10 @@ static bool should_skip_rmap_item(struct folio *folio,
>         if (folio_test_ksm(folio))
>                 return false;
>  
> +       /* There is no age information in stable-tree nodes. */
> +       if (rmap_item->address & STABLE_FLAG)
> +               return false;
> +
>         age = rmap_item->age;
>         if (age != U8_MAX)
>                 rmap_item->age++;
> 
> 
> But it's all confusing. Because we might temporarily have rmap_item->anon_vma
> set on an rmap_entry that does not yet have the STABLE_FLAG flag set through
> stable_tree_append().
> 
> And then we magically call break_cow() which does a magical
> 
> 	put_anon_vma(rmap_item->anon_vma);
> 
> (this doesn't look correct in once case) ... anyhow.
> 

It looks confusing indeed, but it does break_cow because when merging two pages into
one KSM, the first one succeeds but the second one fails, so it's necessary to
restore the state of the first successful rmap_item.

> So we might want to reset the pgoff there as well, OR only store
> the pgoff in stable_tree_append() where we actually set STABLE_FLAG.
> 

Yes, I agree. Please allow me to reorganize all the key points we've discussed, and
update and optimize the patch along with the test program.

I will try to release v4 for review as soon as possible