Re: [PATCH 5/7] mm: free user PTE page table pages

[Mika Penttilä]

On 19.7.2021 17.12, Qi Zheng wrote:
> On 7/19/21 9:55 PM, Mika Penttilä wrote:
> > On 19.7.2021 15.56, Qi Zheng wrote:
> > > On 7/18/21 2:19 PM, Mika Penttilä wrote:
> > >
> > > > > +
> > > > > +/*
> > > > > + * returns true if the pmd has been populated with PTE page table,
> > > > > + * or false for all other cases.
> > > > > + */
> > > > > +bool pte_install_try_get(struct mm_struct *mm, pmd_t *pmd, 
> > > > > pgtable_t *pte)
> > > > > +{
> > > > > +    spinlock_t *ptl;
> > > > > +    bool retval = true;
> > > > > +
> > > > > +retry:
> > > > > +    ptl = pmd_lock(mm, pmd);
> > > > > +    if (likely(pmd_none(*pmd))) {
> > > > > +        __pte_install(mm, pmd, pte);
> > > > > +    } else if (pmd_leaf(*pmd) || !pmd_present(*pmd)) {
> > > > > +        retval = false;
> > > > > +    } else if (!pte_get_unless_zero(pmd)) {
> > > > > +        spin_unlock(ptl);
> > > > > +        goto retry;
> > > > > +    }
> > > > > +    spin_unlock(ptl);
> > > > > +    return retval;
> > > > > +}
> > > > > +
> > > >
> > > > Can pte_get_unless_zero() return true above? Can the pmd have been 
> > > > by populated by others? In that case the ref count is wrongly 
> > > > incremented.
> > >
> > > Here we only have mmap_read_lock(mm), so the pmd can be populated with
> > > other PTE page table page after a page fault in a different thread B 
> > > of this mm. In this case, thread B already hold a pte_refcount of 
> > > the PTE page table page populated in the pmd, so 
> > > pte_get_unless_zero() can
> > > return true above.
> >
> > Yes but if thread B populates the page table page and pte, then we 
> > also increase the refcount with pte_get_unless_zero() , but dont 
> > decrease it when notice !pte_none().
> > And in the pte_none() case, the refcount is increased again, so 
> > double accounting. see finish_fault().
>
> The semantics of this function is to hold the pte_refcount count when it
> returns 1, and its caller is responsible for decrease the pte_refcount
> by calling pte_put(), like the following pattern:
>
>     do_anonymous_page()
>
>     --> pte_alloc_try_get()
>
>         do something about pte
>
>         pte_put()
>
> Similarly in finish_fault().
>
> Thanks,
>
> Qi

Ok I didn't notice the added pte_put()'s at the end of these funcs.

Thanks,
Mika



> > > Similarly, if THP is enabled, the pmd also can be populated with a 
> > > THP page, we can see more detail in comment in handle_pte_fault(). The
> > > pmd_leaf() above is to detect this situation.