Re: [PATCH v6 0/6] ksm: support tracking KSM-placed zero-pages

[David Hildenbrand]

On 30.03.23 14:06, xu xin wrote:
> Hi, I'm sorry to reply so late because I was so busy with my job matters recently.
>
> I appreciate David's idea of simplifying the implement of tracking KSM-placed zero pages.
> But I'm confused with how to implement that via pte_mkdirty/pte_dirty without affecting
> other functions now and in the future.

No need to worry about too much about the future here :)

> > I already shared some feedback in [1]. I think we should try to simplify
> > this handling, as proposed in that mail. Still waiting for a reply.
> >
> > [1]
> > https://lore.kernel.org/all/9d7a8be3-ee9e-3492-841b-a0af9952ef36@xxxxxxxxxx/
>
> I have some questions about using pte_mkdirty to mark KSM-placed zero pages.
>
> (1) Will KSM using pte_mkdirty to mark KSM-placed zero pages collides with the existing
>      handling of the same pte in other featutes? And in the future, what if there are new
>      codes also using pte_mkdirty for other goals.

So far I am not aware of other users of the dirty bit for the shared zeropage. If ever
required (why?) we could try finding another PTE bit. Or use a completely separate set
of zeropages, if ever really running out of PTE bits.

I selected pte_dirty() because it's available on all architectures and should be unused
on the shared zeropage (always clean).

Until then, we only have to worry about architectures that treat R/O dirty PTEs as writable
(I only know sparc64), maybe a good justification to finally fix sparc64 and identify others.
Again, happy to help here. [1]

> (2) Can the literal meaning of pte_mkdiry represents a pte that points to ksm zero page?

I briefly scanned the code. pte_dirty() should mostly not matter for the shared zeropage.
We have to double check (will do as well).

> (3) Suppose we use the pte_mkdirty approach, how to update/decline the count of ksm_zero_pages
>      when upper app writting on the page triggers COW(Copy on Write)? In *mm_fault outside
>      mm/ksm.c ?

yes. Do it synchronously when unmapping the shared zeropage.


diff --git a/mm/memory.c b/mm/memory.c
index f456f3b5049c..78b6c60602dd 100644
--- a/mm/memory.c
+++ b/mm/memory.c
@@ -1351,6 +1351,8 @@ zap_install_uffd_wp_if_needed(struct vm_area_struct *vma,
        pte_install_uffd_wp_if_needed(vma, addr, pte, pteval);
 }
 
+#define is_ksm_zero_pte(pte) (is_zero_pfn(pte_pfn(pte)) && pte_dirty(pte))
+
 static unsigned long zap_pte_range(struct mmu_gather *tlb,
                                struct vm_area_struct *vma, pmd_t *pmd,
                                unsigned long addr, unsigned long end,
@@ -1392,8 +1394,11 @@ static unsigned long zap_pte_range(struct mmu_gather *tlb,
                        tlb_remove_tlb_entry(tlb, pte, addr);
                        zap_install_uffd_wp_if_needed(vma, addr, pte, details,
                                                      ptent);
-                       if (unlikely(!page))
+                       if (unlikely(!page)) {
+                               if (is_ksm_zero_pte(ptent))
+                                       /* TODO: adjust counter */
                                continue;
+                       }
 
                        delay_rmap = 0;
                        if (!PageAnon(page)) {
@@ -3111,6 +3116,8 @@ static vm_fault_t wp_page_copy(struct vm_fault *vmf)
                                inc_mm_counter(mm, MM_ANONPAGES);
                        }
                } else {
+                       if (is_ksm_zero_pte(orig_pte))
+                               /* TODO: adjust counter */
                        inc_mm_counter(mm, MM_ANONPAGES);
                }
                flush_cache_page(vma, vmf->address, pte_pfn(vmf->orig_pte));


The nice thing is, if we get it wrong we "only" get wrong counters.

A prototype for that should be fairly simple -- to see what we're missing.

> Move the previos message here to reply together.
> > The problem with this approach I see is that it fundamentally relies on
> > the rmap/stable-tree to detect whether a zeropage was placed or not.
> >
> > I was wondering, why we even need an rmap item *at all* anymore. Why
> > can't we place the shared zeropage an call it a day (remove the rmap
> > item)? Once we placed a shared zeropage, the next KSM scan should better
> > just ignore it, it's already deduplicated.
>
> The reason is as follows ...
> Initially, all scanned pages by ksmd will be assigned a rmap_item storing the page
> information and ksm information, which helps ksmd can know every scanned pages' status and
> update all counts especialy when COW happens. But since use_zero_pages feature was merged,
> the situation changed, ksm zero pages is the only exception of ksm-scanned page without owning
> a rmap_item in KSM, which leads to ksmd even don't know the existing of KSM-placed, and thus
> causes the problem of our patches aimed to solve.

Understood, so per-PTE information would similarly work.


[1] https://lkml.kernel.org/r/20221212130213.136267-1-david@xxxxxxxxxx

--
Thanks,

David / dhildenb