Re: [PATCH v2 1/9] mm: introduce pmd_install() helper

[Qi Zheng <zhengqi.arch@xxxxxxxxxxxxx>]

On 2021/8/25 AM12:26, David Hildenbrand wrote:
> On 19.08.21 05:18, Qi Zheng wrote:
> > Currently we have three times the same few lines repeated in the
> > code. Deduplicate them by newly introduced pmd_install() helper.
> >
> > Signed-off-by: Qi Zheng <zhengqi.arch@xxxxxxxxxxxxx>
> > ---
> >   include/linux/mm.h |  1 +
> >   mm/filemap.c       | 11 ++---------
> >   mm/memory.c        | 34 ++++++++++++++++------------------
> >   3 files changed, 19 insertions(+), 27 deletions(-)
> >
> > diff --git a/include/linux/mm.h b/include/linux/mm.h
> > index ce8fc0fd6d6e..57e48217bd71 100644
> > --- a/include/linux/mm.h
> > +++ b/include/linux/mm.h
> > @@ -2471,6 +2471,7 @@ static inline spinlock_t *pud_lock(struct 
> > mm_struct *mm, pud_t *pud)
> >       return ptl;
> >   }
> > +extern void pmd_install(struct mm_struct *mm, pmd_t *pmd, pgtable_t 
> > *pte);
> >   extern void __init pagecache_init(void);
> >   extern void __init free_area_init_memoryless_node(int nid);
> >   extern void free_initmem(void);
> > diff --git a/mm/filemap.c b/mm/filemap.c
> > index 53913fced7ae..9f773059c6dc 100644
> > --- a/mm/filemap.c
> > +++ b/mm/filemap.c
> > @@ -3210,15 +3210,8 @@ static bool filemap_map_pmd(struct vm_fault 
> > *vmf, struct page *page)
> >           }
> >       }
> > -    if (pmd_none(*vmf->pmd)) {
> > -        vmf->ptl = pmd_lock(mm, vmf->pmd);
> > -        if (likely(pmd_none(*vmf->pmd))) {
> > -            mm_inc_nr_ptes(mm);
> > -            pmd_populate(mm, vmf->pmd, vmf->prealloc_pte);
> > -            vmf->prealloc_pte = NULL;
> > -        }
> > -        spin_unlock(vmf->ptl);
> > -    }
> > +    if (pmd_none(*vmf->pmd))
> > +        pmd_install(mm, vmf->pmd, &vmf->prealloc_pte);
> >       /* See comment in handle_pte_fault() */
> >       if (pmd_devmap_trans_unstable(vmf->pmd)) {
> > diff --git a/mm/memory.c b/mm/memory.c
> > index 39e7a1495c3c..ef7b1762e996 100644
> > --- a/mm/memory.c
> > +++ b/mm/memory.c
> > @@ -433,9 +433,20 @@ void free_pgtables(struct mmu_gather *tlb, struct 
> > vm_area_struct *vma,
> >       }
> >   }
> > +void pmd_install(struct mm_struct *mm, pmd_t *pmd, pgtable_t *pte)
> > +{
> > +    spinlock_t *ptl = pmd_lock(mm, pmd);
> > +
> > +    if (likely(pmd_none(*pmd))) {    /* Has another populated it ? */
> > +        mm_inc_nr_ptes(mm);
> > +        pmd_populate(mm, pmd, *pte);
> > +        *pte = NULL;
> > +    }
> > +    spin_unlock(ptl);
> > +}
> > +
> >   int __pte_alloc(struct mm_struct *mm, pmd_t *pmd)
> >   {
> > -    spinlock_t *ptl;
> >       pgtable_t new = pte_alloc_one(mm);
> >       if (!new)
> >           return -ENOMEM;
> > @@ -455,13 +466,7 @@ int __pte_alloc(struct mm_struct *mm, pmd_t *pmd)
> >        */
> >       smp_wmb(); /* Could be smp_wmb__xxx(before|after)_spin_lock */
> > -    ptl = pmd_lock(mm, pmd);
> > -    if (likely(pmd_none(*pmd))) {    /* Has another populated it ? */
> > -        mm_inc_nr_ptes(mm);
> > -        pmd_populate(mm, pmd, new);
> > -        new = NULL;
> > -    }
> > -    spin_unlock(ptl);
> > +    pmd_install(mm, pmd, &new);
> >       if (new)
> >           pte_free(mm, new);
> >       return 0;
> > @@ -4027,17 +4032,10 @@ vm_fault_t finish_fault(struct vm_fault *vmf)
> >                   return ret;
> >           }
> > -        if (vmf->prealloc_pte) {
> > -            vmf->ptl = pmd_lock(vma->vm_mm, vmf->pmd);
> > -            if (likely(pmd_none(*vmf->pmd))) {
> > -                mm_inc_nr_ptes(vma->vm_mm);
> > -                pmd_populate(vma->vm_mm, vmf->pmd, vmf->prealloc_pte);
> > -                vmf->prealloc_pte = NULL;
> > -            }
> > -            spin_unlock(vmf->ptl);
> > -        } else if (unlikely(pte_alloc(vma->vm_mm, vmf->pmd))) {
> > +        if (vmf->prealloc_pte)
> > +            pmd_install(vma->vm_mm, vmf->pmd, &vmf->prealloc_pte);
> > +        else if (unlikely(pte_alloc(vma->vm_mm, vmf->pmd)))
> >               return VM_FAULT_OOM;
> > -        }
> >       }
> >       /* See comment in handle_pte_fault() */
>
> Reviewed-by: David Hildenbrand <david@xxxxxxxxxx>

Thanks for your review, I will add this to the patch v3.

> That's mostly unrelated to the remaining part of the series and can be 
> picked up early.

The implementation of subsequent patches depends on pmd_install().
So I am worried that if this patch is submitted as a separate patch,
subsequent patches will not be updated until this patch is merged.
What do you think?

>