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

[David Hildenbrand (Arm)]

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.
> 

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.

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.


-- 
Cheers,

David