Re: [PATCH v2 03/20] KVM: s390: Add gmap_helper_set_unused()

Claudio Imbrenda
2025-09-15T11:50:01Z

On Fri, 12 Sep 2025 11:17:02 +0200
Nina Schoetterl-Glausch <nsg@linux.ibm.com> wrote:

> On Wed, 2025-09-10 at 20:07 +0200, Claudio Imbrenda wrote:
> > Add gmap_helper_set_unused() to mark userspace ptes as unused.
> >
> > Core mm code will use that information to discard unused pages instead
> > of attempting to swap them.
> >
> > Signed-off-by: Claudio Imbrenda <imbrenda@linux.ibm.com>
>
> LGTM
> > ---
> > arch/s390/include/asm/gmap_helpers.h | 1 +
> > arch/s390/mm/gmap_helpers.c | 64 ++++++++++++++++++++++++++++
> > 2 files changed, 65 insertions(+)
> >
> > diff --git a/arch/s390/include/asm/gmap_helpers.h b/arch/s390/include/asm/gmap_helpers.h
> > index 5356446a61c4..459bd39d0887 100644
> > --- a/arch/s390/include/asm/gmap_helpers.h
> > +++ b/arch/s390/include/asm/gmap_helpers.h
> > @@ -11,5 +11,6 @@
> > void gmap_helper_zap_one_page(struct mm_struct *mm, unsigned long vmaddr);
> > void gmap_helper_discard(struct mm_struct *mm, unsigned long vmaddr, unsigned long end);
> > int gmap_helper_disable_cow_sharing(void);
> > +void gmap_helper_set_unused(struct mm_struct *mm, unsigned long vmaddr);
> >
> > #endif /* _ASM_S390_GMAP_HELPERS_H */
> > diff --git a/arch/s390/mm/gmap_helpers.c b/arch/s390/mm/gmap_helpers.c
> > index a45d417ad951..69ffc0c6b654 100644
> > --- a/arch/s390/mm/gmap_helpers.c
> > +++ b/arch/s390/mm/gmap_helpers.c
> > @@ -91,6 +91,70 @@ void gmap_helper_discard(struct mm_struct *mm, unsigned long vmaddr, unsigned lo
> > }
> > EXPORT_SYMBOL_GPL(gmap_helper_discard);
> >
> > +/**
> > + * gmap_helper_set_unused() - mark a pte entry as unused
> > + * @mm: the mm
> > + * @vmaddr: the userspace address whose pte is to be marked
> > + *
> > + * Mark the pte corresponding the given address as unused. This will cause
> > + * core mm code to just drop this page instead of swapping it.
> > + *
> > + * This function needs to be called with interrupts disabled (for example
> > + * while holding a spinlock), or while holding the mmap lock. Normally this
> > + * function is called as a result of an unmap operation, and thus KVM common
> > + * code will already hold kvm->mmu_lock in write mode.
> > + *
> > + * Context: Needs to be called while holding the mmap lock or with interrupts
> > + * disabled.
> > + */
> > +void gmap_helper_set_unused(struct mm_struct *mm, unsigned long vmaddr)
>
> Can you give this a better name? E.g. gmap_helper_try_set_pte_unused

yes

>
> > +{
> > + pmd_t *pmdp, pmd, pmdval;
> > + pud_t *pudp, pud;
> > + p4d_t *p4dp, p4d;
> > + pgd_t *pgdp, pgd;
> > + spinlock_t *ptl;
> > + pte_t *ptep;
> > +
> > + pgdp = pgd_offset(mm, vmaddr);
> > + pgd = pgdp_get(pgdp);
> > + if (pgd_none(pgd) || !pgd_present(pgd))
> > + return;
> > +
> > + p4dp = p4d_offset(pgdp, vmaddr);
> > + p4d = p4dp_get(p4dp);
> > + if (p4d_none(p4d) || !p4d_present(p4d))
> > + return;
> > +
> > + pudp = pud_offset(p4dp, vmaddr);
> > + pud = pudp_get(pudp);
> > + if (pud_none(pud) || pud_leaf(pud) || !pud_present(pud))
> > + return;
> > +
> > + pmdp = pmd_offset(pudp, vmaddr);
> > + pmd = pmdp_get_lockless(pmdp);
> > + if (pmd_none(pmd) || pmd_leaf(pmd) || !pmd_present(pmd))
> > + return;
> > +
> > + ptep = pte_offset_map_rw_nolock(mm, pmdp, vmaddr, &pmdval, &ptl);
> > + if (!ptep)
> > + return;
> > +
> > + if (spin_trylock(ptl)) {
>
> Missing the comment you promised :) about deadlock prevention.

Ooops! will fix

>
> > + /*
> > + * Make sure the pte we are touching is still the correct
> > + * one. In theory this check should not be needed, but
>
> Why should it not be needed? I.e. why should we be protected against modification?

I will add this in a comment:

a pmd pointing to a page table can change in only very few cases, and
all cases will take the mm->mmap_lock in write mode and require IPC
synchronization, which means that as long as interrupts are disabled or
we are holding the mmap_lock, the pmd cannot change under our feet.

by keeping interrupts disabled, we are basically stalling any remote
CPUs that might want to change the pmd, as the IPC will not complete
until we re-enable them.

in our case, we call this function after calling pgste_get_lock(),
which will disable interrupts until pgste_set_unlock() is called.

> > + * better safe than sorry.
> > + */
> > + if (likely(pmd_same(pmdval, pmdp_get_lockless(pmdp))))
> > + __atomic64_or(_PAGE_UNUSED, (long *)ptep);
> > + spin_unlock(ptl);
> > + }
> > +
> > + pte_unmap(ptep);
> > +}
> > +EXPORT_SYMBOL_GPL(gmap_helper_set_unused);
> > +
> > static int find_zeropage_pte_entry(pte_t *pte, unsigned long addr,
> > unsigned long end, struct mm_walk *walk)
> > {
>