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

[Nina Schoetterl-Glausch <nsg@linux.ibm.com>]

On Mon, 2025-09-15 at 13:33 +0200, Claudio Imbrenda wrote:
> 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);

Should we do a WARN_ONCE? Would we be notified if it shows up in CI?
Is the "if" even meaningful? That is, if the pmd is not the same for what
ever reason, are we guaranteed to see it here.
Does taking the page table lock also lock the pmd table? (I assume not)
What are the consequences of the or if the pmd is not the same, arbitrary memory corruption?

> > > +		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)
> > >  {  
> > 

-- 
IBM Deutschland Research & Development GmbH
Vorsitzender des Aufsichtsrats: Wolfgang Wendt
Geschäftsführung: David Faller
Sitz der Gesellschaft: Böblingen / Registergericht: Amtsgericht Stuttgart, HRB 243294