Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Subject: Re: [PATCH v2 1/9] mm: introduce pmd_install() helper
To:     David Hildenbrand <david@redhat.com>, akpm@linux-foundation.org,
        tglx@linutronix.de, hannes@cmpxchg.org, mhocko@kernel.org,
        vdavydov.dev@gmail.com, kirill.shutemov@linux.intel.com,
        mika.penttila@nextfour.com
Cc:     linux-doc@vger.kernel.org, linux-kernel@vger.kernel.org,
        linux-mm@kvack.org, songmuchun@bytedance.com
References: <20210819031858.98043-1-zhengqi.arch@bytedance.com>
 <20210819031858.98043-2-zhengqi.arch@bytedance.com>
 <edd82170-7149-1abf-6204-f1d665a5fca2@redhat.com>
From:   Qi Zheng <zhengqi.arch@bytedance.com>
Message-ID: <c650ef29-0f9e-02a0-9426-b406a4608c8f@bytedance.com>
Date:   Thu, 26 Aug 2021 00:20:31 +0800
User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:78.0)
 Gecko/20100101 Thunderbird/78.13.0
MIME-Version: 1.0
In-Reply-To: <edd82170-7149-1abf-6204-f1d665a5fca2@redhat.com>
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Transfer-Encoding: 8bit
Precedence: bulk


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@bytedance.com>
>> ---
>>   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@redhat.com>

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?

>