[PATCH 00/16] A few cleanup patches for huge_memory

[PATCH 00/16] A few cleanup patches for huge_memory

[Miaohe Lin]

Hi everyone,
This series contains a few cleaup patches to remove duplicated codes,
add/use helper functions, fix some obsolete comments and so on. More
details can be found in the respective changelogs. Thanks!
---
This patch set is based on linux-next-20220621.
---
Miaohe Lin (16):
  mm/huge_memory: use flush_pmd_tlb_range in move_huge_pmd
  mm/huge_memory: access vm_page_prot with READ_ONCE in
    remove_migration_pmd
  mm/huge_memory: fix comment of __pud_trans_huge_lock
  mm/huge_memory: use helper touch_pud in huge_pud_set_accessed
  mm/huge_memory: use helper touch_pmd in huge_pmd_set_accessed
  mm/huge_memory: rename mmun_start to haddr in remove_migration_pmd
  mm/huge_memory: minor cleanup for split_huge_pages_pid
  mm/huge_memory: use helper macro __ATTR_RW
  mm/huge_memory: fix comment in zap_huge_pud
  mm/huge_memory: check pmd_present first in is_huge_zero_pmd
  mm/huge_memory: try to free subpage in swapcache when possible
  mm/huge_memory: minor cleanup for split_huge_pages_all
  mm/huge_memory: add helper __get_deferred_split_queue
  mm/huge_memory: fix comment of page_deferred_list
  mm/huge_memory: correct comment of prep_transhuge_page
  mm/huge_memory: comment the subtle logic in __split_huge_pmd

 include/linux/huge_mm.h |   6 +-
 mm/huge_memory.c        | 121 ++++++++++++++++------------------------
 2 files changed, 52 insertions(+), 75 deletions(-)

-- 
2.23.0

[PATCH 01/16] mm/huge_memory: use flush_pmd_tlb_range in move_huge_pmd

[Miaohe Lin]

ARCHes with special requirements for evicting THP backing TLB entries can
implement flush_pmd_tlb_range. Otherwise also, it can help optimize TLB
flush in THP regime. Using flush_pmd_tlb_range to take advantage of this
in move_huge_pmd.

Signed-off-by: Miaohe Lin <linmiaohe@huawei.com>
---
 mm/huge_memory.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/mm/huge_memory.c b/mm/huge_memory.c
index af0751a79c19..fd6da053a13e 100644
--- a/mm/huge_memory.c
+++ b/mm/huge_memory.c
@@ -1746,7 +1746,7 @@ bool move_huge_pmd(struct vm_area_struct *vma, unsigned long old_addr,
 		pmd = move_soft_dirty_pmd(pmd);
 		set_pmd_at(mm, new_addr, new_pmd, pmd);
 		if (force_flush)
-			flush_tlb_range(vma, old_addr, old_addr + PMD_SIZE);
+			flush_pmd_tlb_range(vma, old_addr, old_addr + PMD_SIZE);
 		if (new_ptl != old_ptl)
 			spin_unlock(new_ptl);
 		spin_unlock(old_ptl);
-- 
2.23.0

Re: [PATCH 01/16] mm/huge_memory: use flush_pmd_tlb_range in move_huge_pmd

[Muchun Song]

On Thu, Jun 23, 2022 at 01:06:12AM +0800, Miaohe Lin wrote:
> ARCHes with special requirements for evicting THP backing TLB entries can
> implement flush_pmd_tlb_range. Otherwise also, it can help optimize TLB
> flush in THP regime. Using flush_pmd_tlb_range to take advantage of this
> in move_huge_pmd.
> 
> Signed-off-by: Miaohe Lin <linmiaohe@huawei.com>

LGTM.

Reviewed-by: Muchun Song <songmuchun@bytedance.com>

Re: [PATCH 01/16] mm/huge_memory: use flush_pmd_tlb_range in move_huge_pmd

[Zach O'Keefe]

On 23 Jun 14:30, Muchun Song wrote:
> On Thu, Jun 23, 2022 at 01:06:12AM +0800, Miaohe Lin wrote:
> > ARCHes with special requirements for evicting THP backing TLB entries can
> > implement flush_pmd_tlb_range. Otherwise also, it can help optimize TLB
> > flush in THP regime. Using flush_pmd_tlb_range to take advantage of this
> > in move_huge_pmd.
> > 
> > Signed-off-by: Miaohe Lin <linmiaohe@huawei.com>
> 
> LGTM.
> 
> Reviewed-by: Muchun Song <songmuchun@bytedace.com>

Reviewed-by: Zach O'Keefe <zokeefe@google.com>

[PATCH 02/16] mm/huge_memory: access vm_page_prot with READ_ONCE in remove_migration_pmd

[Miaohe Lin]

vma->vm_page_prot is read lockless from the rmap_walk, it may be updated
concurrently. Using READ_ONCE to prevent the risk of reading intermediate
values.

Signed-off-by: Miaohe Lin <linmiaohe@huawei.com>
---
 mm/huge_memory.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/mm/huge_memory.c b/mm/huge_memory.c
index fd6da053a13e..83fb6c3442ff 100644
--- a/mm/huge_memory.c
+++ b/mm/huge_memory.c
@@ -3202,7 +3202,7 @@ void remove_migration_pmd(struct page_vma_mapped_walk *pvmw, struct page *new)
 
 	entry = pmd_to_swp_entry(*pvmw->pmd);
 	get_page(new);
-	pmde = pmd_mkold(mk_huge_pmd(new, vma->vm_page_prot));
+	pmde = pmd_mkold(mk_huge_pmd(new, READ_ONCE(vma->vm_page_prot)));
 	if (pmd_swp_soft_dirty(*pvmw->pmd))
 		pmde = pmd_mksoft_dirty(pmde);
 	if (is_writable_migration_entry(entry))
-- 
2.23.0

Re: [PATCH 02/16] mm/huge_memory: access vm_page_prot with READ_ONCE in remove_migration_pmd

[Kirill A. Shutemov]

On Thu, Jun 23, 2022 at 01:06:13AM +0800, Miaohe Lin wrote:
> vma->vm_page_prot is read lockless from the rmap_walk, it may be updated
> concurrently. Using READ_ONCE to prevent the risk of reading intermediate
> values.

Have you checked all other vm_page_prot reads that they hold mmap_lock?

I think the right fix would be to provide a helper to read vm_page_prot
which does READ_ONCE() and use it everywhere. This seems more sustainable.

-- 
 Kirill A. Shutemov

Re: [PATCH 02/16] mm/huge_memory: access vm_page_prot with READ_ONCE in remove_migration_pmd

[Miaohe Lin]

On 2022/6/23 11:14, Kirill A. Shutemov wrote:
> On Thu, Jun 23, 2022 at 01:06:13AM +0800, Miaohe Lin wrote:
>> vma->vm_page_prot is read lockless from the rmap_walk, it may be updated
>> concurrently. Using READ_ONCE to prevent the risk of reading intermediate
>> values.
> 
> Have you checked all other vm_page_prot reads that they hold mmap_lock?

I took a glance when I made this patch.

> 
> I think the right fix would be to provide a helper to read vm_page_prot
> which does READ_ONCE() and use it everywhere. This seems more sustainable.
> 

This patch is inspired from the below commit
  6d2329f8872f ("mm: vm_page_prot: update with WRITE_ONCE/READ_ONCE")

It changed all the places that need to use READ_ONCE. But remove_migration_pmd
is missed due to it's introduced later. It looks fine to add a helper to read
vm_page_prot which does READ_ONCE() but READ_ONCE is unneeded while under the
mmap_lock, so might it be a little overkill to add a helper because the helper
is used iff mmap_lock is not held?

Thanks.

Re: [PATCH 02/16] mm/huge_memory: access vm_page_prot with READ_ONCE in remove_migration_pmd

[Zach O'Keefe]

On 23 Jun 20:03, Miaohe Lin wrote:
> On 2022/6/23 11:14, Kirill A. Shutemov wrote:
> > On Thu, Jun 23, 2022 at 01:06:13AM +0800, Miaohe Lin wrote:
> >> vma->vm_page_prot is read lockless from the rmap_walk, it may be updated
> >> concurrently. Using READ_ONCE to prevent the risk of reading intermediate
> >> values.
> > 
> > Have you checked all other vm_page_prot reads that they hold mmap_lock?
> 
> I took a glance when I made this patch.
> 
> > 
> > I think the right fix would be to provide a helper to read vm_page_prot
> > which does READ_ONCE() and use it everywhere. This seems more sustainable.
> > 
> 
> This patch is inspired from the below commit
>   6d2329f8872f ("mm: vm_page_prot: update with WRITE_ONCE/READ_ONCE")
> 
> It changed all the places that need to use READ_ONCE. But remove_migration_pmd
> is missed due to it's introduced later. It looks fine to add a helper to read
> vm_page_prot which does READ_ONCE() but READ_ONCE is unneeded while under the
> mmap_lock, so might it be a little overkill to add a helper because the helper
> is used iff mmap_lock is not held?
> 
> Thanks.

IMO adding the READ_ONCE() as proposed in fine. Adding a helper to be called
dependent on locking context still requires the caller / dev to know what the
locking context is - so I don't think it provides much benefit.

Re: [PATCH 02/16] mm/huge_memory: access vm_page_prot with READ_ONCE in remove_migration_pmd

[Miaohe Lin]

On 2022/6/25 2:40, Zach O'Keefe wrote:
> On 23 Jun 20:03, Miaohe Lin wrote:
>> On 2022/6/23 11:14, Kirill A. Shutemov wrote:
>>> On Thu, Jun 23, 2022 at 01:06:13AM +0800, Miaohe Lin wrote:
>>>> vma->vm_page_prot is read lockless from the rmap_walk, it may be updated
>>>> concurrently. Using READ_ONCE to prevent the risk of reading intermediate
>>>> values.
>>>
>>> Have you checked all other vm_page_prot reads that they hold mmap_lock?
>>
>> I took a glance when I made this patch.
>>
>>>
>>> I think the right fix would be to provide a helper to read vm_page_prot
>>> which does READ_ONCE() and use it everywhere. This seems more sustainable.
>>>
>>
>> This patch is inspired from the below commit
>>   6d2329f8872f ("mm: vm_page_prot: update with WRITE_ONCE/READ_ONCE")
>>
>> It changed all the places that need to use READ_ONCE. But remove_migration_pmd
>> is missed due to it's introduced later. It looks fine to add a helper to read
>> vm_page_prot which does READ_ONCE() but READ_ONCE is unneeded while under the
>> mmap_lock, so might it be a little overkill to add a helper because the helper
>> is used iff mmap_lock is not held?
>>
>> Thanks.
> 
> IMO adding the READ_ONCE() as proposed in fine. Adding a helper to be called
> dependent on locking context still requires the caller / dev to know what the
> locking context is - so I don't think it provides much benefit.

I tend to agree with Zach. Thanks!

> .
> 

[PATCH 03/16] mm/huge_memory: fix comment of __pud_trans_huge_lock

[Miaohe Lin]

__pud_trans_huge_lock returns page table lock pointer if a given pud maps
a thp instead of 'true' since introduced. Fix corresponding comments.

Signed-off-by: Miaohe Lin <linmiaohe@huawei.com>
---
 mm/huge_memory.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/mm/huge_memory.c b/mm/huge_memory.c
index 83fb6c3442ff..a26580da8011 100644
--- a/mm/huge_memory.c
+++ b/mm/huge_memory.c
@@ -1903,10 +1903,10 @@ spinlock_t *__pmd_trans_huge_lock(pmd_t *pmd, struct vm_area_struct *vma)
 }
 
 /*
- * Returns true if a given pud maps a thp, false otherwise.
+ * Returns page table lock pointer if a given pud maps a thp, NULL otherwise.
  *
- * Note that if it returns true, this routine returns without unlocking page
- * table lock. So callers must unlock it.
+ * Note that if it returns page table lock pointe, this routine returns without
+ * unlocking page table lock. So callers must unlock it.
  */
 spinlock_t *__pud_trans_huge_lock(pud_t *pud, struct vm_area_struct *vma)
 {
-- 
2.23.0

Re: [PATCH 03/16] mm/huge_memory: fix comment of __pud_trans_huge_lock

[Muchun Song]

On Thu, Jun 23, 2022 at 01:06:14AM +0800, Miaohe Lin wrote:
> __pud_trans_huge_lock returns page table lock pointer if a given pud maps
> a thp instead of 'true' since introduced. Fix corresponding comments.
> 
> Signed-off-by: Miaohe Lin <linmiaohe@huawei.com>

Acked-by: Muchun Song <songmuchun@bytedance.com>

Thanks.

Re: [PATCH 03/16] mm/huge_memory: fix comment of __pud_trans_huge_lock

[Zach O'Keefe]

On 23 Jun 01:06, Miaohe Lin wrote:
> __pud_trans_huge_lock returns page table lock pointer if a given pud maps
> a thp instead of 'true' since introduced. Fix corresponding comments.
> 
> Signed-off-by: Miaohe Lin <linmiaohe@huawei.com>
> ---
>  mm/huge_memory.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/mm/huge_memory.c b/mm/huge_memory.c
> index 83fb6c3442ff..a26580da8011 100644
> --- a/mm/huge_memory.c
> +++ b/mm/huge_memory.c
> @@ -1903,10 +1903,10 @@ spinlock_t *__pmd_trans_huge_lock(pmd_t *pmd, struct vm_area_struct *vma)
>  }
>  
>  /*
> - * Returns true if a given pud maps a thp, false otherwise.
> + * Returns page table lock pointer if a given pud maps a thp, NULL otherwise.
>   *
> - * Note that if it returns true, this routine returns without unlocking page
> - * table lock. So callers must unlock it.
> + * Note that if it returns page table lock pointe, this routine returns without

s/pointe/pointer

> + * unlocking page table lock. So callers must unlock it.
>   */
>  spinlock_t *__pud_trans_huge_lock(pud_t *pud, struct vm_area_struct *vma)
>  {
> -- 
> 2.23.0
> 

Re: [PATCH 03/16] mm/huge_memory: fix comment of __pud_trans_huge_lock

[Miaohe Lin]

On 2022/6/25 2:47, Zach O'Keefe wrote:
> On 23 Jun 01:06, Miaohe Lin wrote:
>> __pud_trans_huge_lock returns page table lock pointer if a given pud maps
>> a thp instead of 'true' since introduced. Fix corresponding comments.
>>
>> Signed-off-by: Miaohe Lin <linmiaohe@huawei.com>
>> ---
>>  mm/huge_memory.c | 6 +++---
>>  1 file changed, 3 insertions(+), 3 deletions(-)
>>
>> diff --git a/mm/huge_memory.c b/mm/huge_memory.c
>> index 83fb6c3442ff..a26580da8011 100644
>> --- a/mm/huge_memory.c
>> +++ b/mm/huge_memory.c
>> @@ -1903,10 +1903,10 @@ spinlock_t *__pmd_trans_huge_lock(pmd_t *pmd, struct vm_area_struct *vma)
>>  }
>>  
>>  /*
>> - * Returns true if a given pud maps a thp, false otherwise.
>> + * Returns page table lock pointer if a given pud maps a thp, NULL otherwise.
>>   *
>> - * Note that if it returns true, this routine returns without unlocking page
>> - * table lock. So callers must unlock it.
>> + * Note that if it returns page table lock pointe, this routine returns without
> 
> s/pointe/pointer

Thanks for catching this.

> 
>> + * unlocking page table lock. So callers must unlock it.
>>   */
>>  spinlock_t *__pud_trans_huge_lock(pud_t *pud, struct vm_area_struct *vma)
>>  {
>> -- 
>> 2.23.0
>>
> .
> 

Re: [PATCH 03/16] mm/huge_memory: fix comment of __pud_trans_huge_lock

[Matthew Wilcox]

On Thu, Jun 23, 2022 at 01:06:14AM +0800, Miaohe Lin wrote:
> __pud_trans_huge_lock returns page table lock pointer if a given pud maps
> a thp instead of 'true' since introduced. Fix corresponding comments.

I think the comments here are rather wasted.  I think this comment
should be moved to pud_trans_huge_lock() and turned into kernel-doc
format.

> Signed-off-by: Miaohe Lin <linmiaohe@huawei.com>
> ---
>  mm/huge_memory.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/mm/huge_memory.c b/mm/huge_memory.c
> index 83fb6c3442ff..a26580da8011 100644
> --- a/mm/huge_memory.c
> +++ b/mm/huge_memory.c
> @@ -1903,10 +1903,10 @@ spinlock_t *__pmd_trans_huge_lock(pmd_t *pmd, struct vm_area_struct *vma)
>  }
>  
>  /*
> - * Returns true if a given pud maps a thp, false otherwise.
> + * Returns page table lock pointer if a given pud maps a thp, NULL otherwise.
>   *
> - * Note that if it returns true, this routine returns without unlocking page
> - * table lock. So callers must unlock it.
> + * Note that if it returns page table lock pointe, this routine returns without
> + * unlocking page table lock. So callers must unlock it.
>   */
>  spinlock_t *__pud_trans_huge_lock(pud_t *pud, struct vm_area_struct *vma)
>  {
> -- 
> 2.23.0
> 

Re: [PATCH 03/16] mm/huge_memory: fix comment of __pud_trans_huge_lock

[Miaohe Lin]

On 2022/6/25 2:56, Matthew Wilcox wrote:
> On Thu, Jun 23, 2022 at 01:06:14AM +0800, Miaohe Lin wrote:
>> __pud_trans_huge_lock returns page table lock pointer if a given pud maps
>> a thp instead of 'true' since introduced. Fix corresponding comments.
> 
> I think the comments here are rather wasted.  I think this comment
> should be moved to pud_trans_huge_lock() and turned into kernel-doc
> format.

Do you mean something like below? If so, __pmd_trans_huge_lock might need to do the
similar work?

diff --git a/include/linux/huge_mm.h b/include/linux/huge_mm.h
index ae3d8e2fd9e2..b73fe864de13 100644
--- a/include/linux/huge_mm.h
+++ b/include/linux/huge_mm.h
@@ -238,6 +238,12 @@ static inline spinlock_t *pmd_trans_huge_lock(pmd_t *pmd,
        else
                return NULL;
 }
+/**
+ * Returns true if a given pud maps a thp, false otherwise.
+ *
+ * Note that if it returns true, this routine returns without unlocking page
+ * table lock. So callers must unlock it.
+ */
 static inline spinlock_t *pud_trans_huge_lock(pud_t *pud,
                struct vm_area_struct *vma)
 {
diff --git a/mm/huge_memory.c b/mm/huge_memory.c
index eb2e4ecb76aa..ae4c2116be07 100644
--- a/mm/huge_memory.c
+++ b/mm/huge_memory.c
@@ -2006,12 +2006,6 @@ spinlock_t *__pmd_trans_huge_lock(pmd_t *pmd, struct vm_area_struct *vma)
        return NULL;
 }

-/*
- * Returns true if a given pud maps a thp, false otherwise.
- *
- * Note that if it returns true, this routine returns without unlocking page
- * table lock. So callers must unlock it.
- */
 spinlock_t *__pud_trans_huge_lock(pud_t *pud, struct vm_area_struct *vma)
 {
        spinlock_t *ptl;

Thanks!
> 
>> Signed-off-by: Miaohe Lin <linmiaohe@huawei.com>
>> ---
>>  mm/huge_memory.c | 6 +++---
>>  1 file changed, 3 insertions(+), 3 deletions(-)
>>
>> diff --git a/mm/huge_memory.c b/mm/huge_memory.c
>> index 83fb6c3442ff..a26580da8011 100644
>> --- a/mm/huge_memory.c
>> +++ b/mm/huge_memory.c
>> @@ -1903,10 +1903,10 @@ spinlock_t *__pmd_trans_huge_lock(pmd_t *pmd, struct vm_area_struct *vma)
>>  }
>>  
>>  /*
>> - * Returns true if a given pud maps a thp, false otherwise.
>> + * Returns page table lock pointer if a given pud maps a thp, NULL otherwise.
>>   *
>> - * Note that if it returns true, this routine returns without unlocking page
>> - * table lock. So callers must unlock it.
>> + * Note that if it returns page table lock pointe, this routine returns without
>> + * unlocking page table lock. So callers must unlock it.
>>   */
>>  spinlock_t *__pud_trans_huge_lock(pud_t *pud, struct vm_area_struct *vma)
>>  {
>> -- 
>> 2.23.0
>>
> 
> .
> 

[PATCH 04/16] mm/huge_memory: use helper touch_pud in huge_pud_set_accessed

[Miaohe Lin]

Use helper touch_pud to set pud accessed to simplify the code and improve
the readability. No functional change intended.

Signed-off-by: Miaohe Lin <linmiaohe@huawei.com>
---
 mm/huge_memory.c | 14 ++++----------
 1 file changed, 4 insertions(+), 10 deletions(-)

diff --git a/mm/huge_memory.c b/mm/huge_memory.c
index a26580da8011..a0c0e4bf9c1e 100644
--- a/mm/huge_memory.c
+++ b/mm/huge_memory.c
@@ -1281,21 +1281,15 @@ int copy_huge_pud(struct mm_struct *dst_mm, struct mm_struct *src_mm,
 
 void huge_pud_set_accessed(struct vm_fault *vmf, pud_t orig_pud)
 {
-	pud_t entry;
-	unsigned long haddr;
-	bool write = vmf->flags & FAULT_FLAG_WRITE;
+	int flags = 0;
 
 	vmf->ptl = pud_lock(vmf->vma->vm_mm, vmf->pud);
 	if (unlikely(!pud_same(*vmf->pud, orig_pud)))
 		goto unlock;
 
-	entry = pud_mkyoung(orig_pud);
-	if (write)
-		entry = pud_mkdirty(entry);
-	haddr = vmf->address & HPAGE_PUD_MASK;
-	if (pudp_set_access_flags(vmf->vma, haddr, vmf->pud, entry, write))
-		update_mmu_cache_pud(vmf->vma, vmf->address, vmf->pud);
-
+	if (vmf->flags & FAULT_FLAG_WRITE)
+		flags = FOLL_WRITE;
+	touch_pud(vmf->vma, vmf->address, vmf->pud, flags);
 unlock:
 	spin_unlock(vmf->ptl);
 }
-- 
2.23.0

Re: [PATCH 04/16] mm/huge_memory: use helper touch_pud in huge_pud_set_accessed

[Muchun Song]

On Thu, Jun 23, 2022 at 01:06:15AM +0800, Miaohe Lin wrote:
> Use helper touch_pud to set pud accessed to simplify the code and improve
> the readability. No functional change intended.
> 
> Signed-off-by: Miaohe Lin <linmiaohe@huawei.com>
> ---
>  mm/huge_memory.c | 14 ++++----------
>  1 file changed, 4 insertions(+), 10 deletions(-)
> 
> diff --git a/mm/huge_memory.c b/mm/huge_memory.c
> index a26580da8011..a0c0e4bf9c1e 100644
> --- a/mm/huge_memory.c
> +++ b/mm/huge_memory.c
> @@ -1281,21 +1281,15 @@ int copy_huge_pud(struct mm_struct *dst_mm, struct mm_struct *src_mm,
>  
>  void huge_pud_set_accessed(struct vm_fault *vmf, pud_t orig_pud)
>  {
> -	pud_t entry;
> -	unsigned long haddr;
> -	bool write = vmf->flags & FAULT_FLAG_WRITE;
> +	int flags = 0;
>  
>  	vmf->ptl = pud_lock(vmf->vma->vm_mm, vmf->pud);
>  	if (unlikely(!pud_same(*vmf->pud, orig_pud)))
>  		goto unlock;
>  
> -	entry = pud_mkyoung(orig_pud);
> -	if (write)
> -		entry = pud_mkdirty(entry);
> -	haddr = vmf->address & HPAGE_PUD_MASK;
> -	if (pudp_set_access_flags(vmf->vma, haddr, vmf->pud, entry, write))
> -		update_mmu_cache_pud(vmf->vma, vmf->address, vmf->pud);
> -
> +	if (vmf->flags & FAULT_FLAG_WRITE)
> +		flags = FOLL_WRITE;

FOLL_* flags are used for follow_page(). But huge_pud_set_accessed() is used in mm fault
path. It is a little weird to me to use FOLL_WRITE here. I suggest replace the last
parameter of touch_pud() to "bool writable", then passing "vmf->flags & FAULT_FLAG_WRITE"
to it instead of converting from FAULT_FLAG* flag to FOLL* flag.

Thanks.

> +	touch_pud(vmf->vma, vmf->address, vmf->pud, flags);
>  unlock:
>  	spin_unlock(vmf->ptl);
>  }
> -- 
> 2.23.0
> 
> 

Re: [PATCH 04/16] mm/huge_memory: use helper touch_pud in huge_pud_set_accessed

[Miaohe Lin]

On 2022/6/23 14:42, Muchun Song wrote:
> On Thu, Jun 23, 2022 at 01:06:15AM +0800, Miaohe Lin wrote:
>> Use helper touch_pud to set pud accessed to simplify the code and improve
>> the readability. No functional change intended.
>>
>> Signed-off-by: Miaohe Lin <linmiaohe@huawei.com>
>> ---
>>  mm/huge_memory.c | 14 ++++----------
>>  1 file changed, 4 insertions(+), 10 deletions(-)
>>
>> diff --git a/mm/huge_memory.c b/mm/huge_memory.c
>> index a26580da8011..a0c0e4bf9c1e 100644
>> --- a/mm/huge_memory.c
>> +++ b/mm/huge_memory.c
>> @@ -1281,21 +1281,15 @@ int copy_huge_pud(struct mm_struct *dst_mm, struct mm_struct *src_mm,
>>  
>>  void huge_pud_set_accessed(struct vm_fault *vmf, pud_t orig_pud)
>>  {
>> -	pud_t entry;
>> -	unsigned long haddr;
>> -	bool write = vmf->flags & FAULT_FLAG_WRITE;
>> +	int flags = 0;
>>  
>>  	vmf->ptl = pud_lock(vmf->vma->vm_mm, vmf->pud);
>>  	if (unlikely(!pud_same(*vmf->pud, orig_pud)))
>>  		goto unlock;
>>  
>> -	entry = pud_mkyoung(orig_pud);
>> -	if (write)
>> -		entry = pud_mkdirty(entry);
>> -	haddr = vmf->address & HPAGE_PUD_MASK;
>> -	if (pudp_set_access_flags(vmf->vma, haddr, vmf->pud, entry, write))
>> -		update_mmu_cache_pud(vmf->vma, vmf->address, vmf->pud);
>> -
>> +	if (vmf->flags & FAULT_FLAG_WRITE)
>> +		flags = FOLL_WRITE;
> 
> FOLL_* flags are used for follow_page(). But huge_pud_set_accessed() is used in mm fault
> path. It is a little weird to me to use FOLL_WRITE here. I suggest replace the last
> parameter of touch_pud() to "bool writable", then passing "vmf->flags & FAULT_FLAG_WRITE"
> to it instead of converting from FAULT_FLAG* flag to FOLL* flag.

Sounds good. Thanks!

> 
> Thanks.
> 
>> +	touch_pud(vmf->vma, vmf->address, vmf->pud, flags);
>>  unlock:
>>  	spin_unlock(vmf->ptl);
>>  }
>> -- 
>> 2.23.0
>>
>>
> .
> 

[PATCH 05/16] mm/huge_memory: use helper touch_pmd in huge_pmd_set_accessed

[Miaohe Lin]

Use helper touch_pmd to set pmd accessed to simplify the code and improve
the readability. No functional change intended.

Signed-off-by: Miaohe Lin <linmiaohe@huawei.com>
---
 mm/huge_memory.c | 16 +++++-----------
 1 file changed, 5 insertions(+), 11 deletions(-)

diff --git a/mm/huge_memory.c b/mm/huge_memory.c
index a0c0e4bf9c1e..c6302fe6704b 100644
--- a/mm/huge_memory.c
+++ b/mm/huge_memory.c
@@ -1297,21 +1297,15 @@ void huge_pud_set_accessed(struct vm_fault *vmf, pud_t orig_pud)
 
 void huge_pmd_set_accessed(struct vm_fault *vmf)
 {
-	pmd_t entry;
-	unsigned long haddr;
-	bool write = vmf->flags & FAULT_FLAG_WRITE;
-	pmd_t orig_pmd = vmf->orig_pmd;
+	int flags = 0;
 
 	vmf->ptl = pmd_lock(vmf->vma->vm_mm, vmf->pmd);
-	if (unlikely(!pmd_same(*vmf->pmd, orig_pmd)))
+	if (unlikely(!pmd_same(*vmf->pmd, vmf->orig_pmd)))
 		goto unlock;
 
-	entry = pmd_mkyoung(orig_pmd);
-	if (write)
-		entry = pmd_mkdirty(entry);
-	haddr = vmf->address & HPAGE_PMD_MASK;
-	if (pmdp_set_access_flags(vmf->vma, haddr, vmf->pmd, entry, write))
-		update_mmu_cache_pmd(vmf->vma, vmf->address, vmf->pmd);
+	if (vmf->flags & FAULT_FLAG_WRITE)
+		flags = FOLL_WRITE;
+	touch_pmd(vmf->vma, vmf->address, vmf->pmd, flags);
 
 unlock:
 	spin_unlock(vmf->ptl);
-- 
2.23.0

Re: [PATCH 05/16] mm/huge_memory: use helper touch_pmd in huge_pmd_set_accessed

[Muchun Song]

On Thu, Jun 23, 2022 at 01:06:16AM +0800, Miaohe Lin wrote:
> Use helper touch_pmd to set pmd accessed to simplify the code and improve
> the readability. No functional change intended.
> 
> Signed-off-by: Miaohe Lin <linmiaohe@huawei.com>
> ---
>  mm/huge_memory.c | 16 +++++-----------
>  1 file changed, 5 insertions(+), 11 deletions(-)
> 
> diff --git a/mm/huge_memory.c b/mm/huge_memory.c
> index a0c0e4bf9c1e..c6302fe6704b 100644
> --- a/mm/huge_memory.c
> +++ b/mm/huge_memory.c
> @@ -1297,21 +1297,15 @@ void huge_pud_set_accessed(struct vm_fault *vmf, pud_t orig_pud)
>  
>  void huge_pmd_set_accessed(struct vm_fault *vmf)
>  {
> -	pmd_t entry;
> -	unsigned long haddr;
> -	bool write = vmf->flags & FAULT_FLAG_WRITE;
> -	pmd_t orig_pmd = vmf->orig_pmd;
> +	int flags = 0;
>  
>  	vmf->ptl = pmd_lock(vmf->vma->vm_mm, vmf->pmd);
> -	if (unlikely(!pmd_same(*vmf->pmd, orig_pmd)))
> +	if (unlikely(!pmd_same(*vmf->pmd, vmf->orig_pmd)))
>  		goto unlock;
>  
> -	entry = pmd_mkyoung(orig_pmd);
> -	if (write)
> -		entry = pmd_mkdirty(entry);
> -	haddr = vmf->address & HPAGE_PMD_MASK;
> -	if (pmdp_set_access_flags(vmf->vma, haddr, vmf->pmd, entry, write))
> -		update_mmu_cache_pmd(vmf->vma, vmf->address, vmf->pmd);
> +	if (vmf->flags & FAULT_FLAG_WRITE)
> +		flags = FOLL_WRITE;

Same suggestion as mentioned in previous thread.

Thanks.

> +	touch_pmd(vmf->vma, vmf->address, vmf->pmd, flags);
>  
>  unlock:
>  	spin_unlock(vmf->ptl);
> -- 
> 2.23.0
> 
> 

[PATCH 06/16] mm/huge_memory: rename mmun_start to haddr in remove_migration_pmd

[Miaohe Lin]

mmun_start indicates mmu_notifier start address but there's no mmu_notifier
stuff in remove_migration_pmd. This will make it hard to get the meaning of
mmun_start. Rename it to haddr to avoid confusing readers and also imporve
readability.

Signed-off-by: Miaohe Lin <linmiaohe@huawei.com>
---
 mm/huge_memory.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/mm/huge_memory.c b/mm/huge_memory.c
index c6302fe6704b..fb5c484dfa39 100644
--- a/mm/huge_memory.c
+++ b/mm/huge_memory.c
@@ -3181,7 +3181,7 @@ void remove_migration_pmd(struct page_vma_mapped_walk *pvmw, struct page *new)
 	struct vm_area_struct *vma = pvmw->vma;
 	struct mm_struct *mm = vma->vm_mm;
 	unsigned long address = pvmw->address;
-	unsigned long mmun_start = address & HPAGE_PMD_MASK;
+	unsigned long haddr = address & HPAGE_PMD_MASK;
 	pmd_t pmde;
 	swp_entry_t entry;
 
@@ -3204,12 +3204,12 @@ void remove_migration_pmd(struct page_vma_mapped_walk *pvmw, struct page *new)
 		if (!is_readable_migration_entry(entry))
 			rmap_flags |= RMAP_EXCLUSIVE;
 
-		page_add_anon_rmap(new, vma, mmun_start, rmap_flags);
+		page_add_anon_rmap(new, vma, haddr, rmap_flags);
 	} else {
 		page_add_file_rmap(new, vma, true);
 	}
 	VM_BUG_ON(pmd_write(pmde) && PageAnon(new) && !PageAnonExclusive(new));
-	set_pmd_at(mm, mmun_start, pvmw->pmd, pmde);
+	set_pmd_at(mm, haddr, pvmw->pmd, pmde);
 
 	/* No need to invalidate - it was non-present before */
 	update_mmu_cache_pmd(vma, address, pvmw->pmd);
-- 
2.23.0

Re: [PATCH 06/16] mm/huge_memory: rename mmun_start to haddr in remove_migration_pmd

[Muchun Song]

On Thu, Jun 23, 2022 at 01:06:17AM +0800, Miaohe Lin wrote:
> mmun_start indicates mmu_notifier start address but there's no mmu_notifier

Actually I don't know what "mmun" prefix means. But this change looks good to
me.

Reviewed-by: Muchun Song <songmuchun@bytedance.com>

> stuff in remove_migration_pmd. This will make it hard to get the meaning of
> mmun_start. Rename it to haddr to avoid confusing readers and also imporve
> readability.
> 
> Signed-off-by: Miaohe Lin <linmiaohe@huawei.com>
> ---
>  mm/huge_memory.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/mm/huge_memory.c b/mm/huge_memory.c
> index c6302fe6704b..fb5c484dfa39 100644
> --- a/mm/huge_memory.c
> +++ b/mm/huge_memory.c
> @@ -3181,7 +3181,7 @@ void remove_migration_pmd(struct page_vma_mapped_walk *pvmw, struct page *new)
>  	struct vm_area_struct *vma = pvmw->vma;
>  	struct mm_struct *mm = vma->vm_mm;
>  	unsigned long address = pvmw->address;
> -	unsigned long mmun_start = address & HPAGE_PMD_MASK;
> +	unsigned long haddr = address & HPAGE_PMD_MASK;
>  	pmd_t pmde;
>  	swp_entry_t entry;
>  
> @@ -3204,12 +3204,12 @@ void remove_migration_pmd(struct page_vma_mapped_walk *pvmw, struct page *new)
>  		if (!is_readable_migration_entry(entry))
>  			rmap_flags |= RMAP_EXCLUSIVE;
>  
> -		page_add_anon_rmap(new, vma, mmun_start, rmap_flags);
> +		page_add_anon_rmap(new, vma, haddr, rmap_flags);
>  	} else {
>  		page_add_file_rmap(new, vma, true);
>  	}
>  	VM_BUG_ON(pmd_write(pmde) && PageAnon(new) && !PageAnonExclusive(new));
> -	set_pmd_at(mm, mmun_start, pvmw->pmd, pmde);
> +	set_pmd_at(mm, haddr, pvmw->pmd, pmde);
>  
>  	/* No need to invalidate - it was non-present before */
>  	update_mmu_cache_pmd(vma, address, pvmw->pmd);
> -- 
> 2.23.0
> 
> 

[PATCH 07/16] mm/huge_memory: minor cleanup for split_huge_pages_pid

[Miaohe Lin]

Use helper function vma_lookup to lookup the needed vma and use help macro
IS_ERR_OR_NULL to check the validity of page to simplify the code. Minor
readability improvement.

Signed-off-by: Miaohe Lin <linmiaohe@huawei.com>
---
 mm/huge_memory.c | 8 +++-----
 1 file changed, 3 insertions(+), 5 deletions(-)

diff --git a/mm/huge_memory.c b/mm/huge_memory.c
index fb5c484dfa39..7cfa003b1789 100644
--- a/mm/huge_memory.c
+++ b/mm/huge_memory.c
@@ -2942,10 +2942,10 @@ static int split_huge_pages_pid(int pid, unsigned long vaddr_start,
 	 * table filled with PTE-mapped THPs, each of which is distinct.
 	 */
 	for (addr = vaddr_start; addr < vaddr_end; addr += PAGE_SIZE) {
-		struct vm_area_struct *vma = find_vma(mm, addr);
+		struct vm_area_struct *vma = vma_lookup(mm, addr);
 		struct page *page;
 
-		if (!vma || addr < vma->vm_start)
+		if (!vma)
 			break;
 
 		/* skip special VMA and hugetlb VMA */
@@ -2957,9 +2957,7 @@ static int split_huge_pages_pid(int pid, unsigned long vaddr_start,
 		/* FOLL_DUMP to ignore special (like zero) pages */
 		page = follow_page(vma, addr, FOLL_GET | FOLL_DUMP | FOLL_LRU);
 
-		if (IS_ERR(page))
-			continue;
-		if (!page)
+		if (IS_ERR_OR_NULL(page))
 			continue;
 
 		if (!is_transparent_hugepage(page))
-- 
2.23.0

Re: [PATCH 07/16] mm/huge_memory: minor cleanup for split_huge_pages_pid

[Kirill A. Shutemov]

On Thu, Jun 23, 2022 at 01:06:18AM +0800, Miaohe Lin wrote:
> Use helper function vma_lookup to lookup the needed vma and use help macro
> IS_ERR_OR_NULL to check the validity of page to simplify the code. Minor
> readability improvement.

Grouping of changing seems random, but okay.

-- 
 Kirill A. Shutemov

Re: [PATCH 07/16] mm/huge_memory: minor cleanup for split_huge_pages_pid

[Miaohe Lin]

On 2022/6/23 11:22, Kirill A. Shutemov wrote:
> On Thu, Jun 23, 2022 at 01:06:18AM +0800, Miaohe Lin wrote:
>> Use helper function vma_lookup to lookup the needed vma and use help macro
>> IS_ERR_OR_NULL to check the validity of page to simplify the code. Minor
>> readability improvement.
> 
> Grouping of changing seems random, but okay.

I group them because they belong to the same function. Should I split these two changes?

Many thanks for reviewing and comment!

> 

[PATCH 08/16] mm/huge_memory: use helper macro __ATTR_RW

[Miaohe Lin]

Use helper macro __ATTR_RW to define use_zero_page_attr, defrag_attr and
enabled_attr to make code more clear. Minor readability improvement.

Signed-off-by: Miaohe Lin <linmiaohe@huawei.com>
---
 mm/huge_memory.c | 10 ++++------
 1 file changed, 4 insertions(+), 6 deletions(-)

diff --git a/mm/huge_memory.c b/mm/huge_memory.c
index 7cfa003b1789..b42c8fa51e46 100644
--- a/mm/huge_memory.c
+++ b/mm/huge_memory.c
@@ -273,8 +273,8 @@ static ssize_t enabled_store(struct kobject *kobj,
 	}
 	return ret;
 }
-static struct kobj_attribute enabled_attr =
-	__ATTR(enabled, 0644, enabled_show, enabled_store);
+
+static struct kobj_attribute enabled_attr = __ATTR_RW(enabled);
 
 ssize_t single_hugepage_flag_show(struct kobject *kobj,
 				  struct kobj_attribute *attr, char *buf,
@@ -363,8 +363,7 @@ static ssize_t defrag_store(struct kobject *kobj,
 
 	return count;
 }
-static struct kobj_attribute defrag_attr =
-	__ATTR(defrag, 0644, defrag_show, defrag_store);
+static struct kobj_attribute defrag_attr = __ATTR_RW(defrag);
 
 static ssize_t use_zero_page_show(struct kobject *kobj,
 				  struct kobj_attribute *attr, char *buf)
@@ -378,8 +377,7 @@ static ssize_t use_zero_page_store(struct kobject *kobj,
 	return single_hugepage_flag_store(kobj, attr, buf, count,
 				 TRANSPARENT_HUGEPAGE_USE_ZERO_PAGE_FLAG);
 }
-static struct kobj_attribute use_zero_page_attr =
-	__ATTR(use_zero_page, 0644, use_zero_page_show, use_zero_page_store);
+static struct kobj_attribute use_zero_page_attr = __ATTR_RW(use_zero_page);
 
 static ssize_t hpage_pmd_size_show(struct kobject *kobj,
 				   struct kobj_attribute *attr, char *buf)
-- 
2.23.0

Re: [PATCH 08/16] mm/huge_memory: use helper macro __ATTR_RW

[Muchun Song]

On Thu, Jun 23, 2022 at 01:06:19AM +0800, Miaohe Lin wrote:
> Use helper macro __ATTR_RW to define use_zero_page_attr, defrag_attr and
> enabled_attr to make code more clear. Minor readability improvement.
> 
> Signed-off-by: Miaohe Lin <linmiaohe@huawei.com>

Reviewed-by: Muchun Song <songmuchun@bytedance.com>

Thanks.

[PATCH 09/16] mm/huge_memory: fix comment in zap_huge_pud

[Miaohe Lin]

The comment about deposited pgtable is borrowed from zap_huge_pmd but
there's no deposited pgtable stuff for huge pud in zap_huge_pud. Remove
it to avoid confusion.

Signed-off-by: Miaohe Lin <linmiaohe@huawei.com>
---
 mm/huge_memory.c | 7 +------
 1 file changed, 1 insertion(+), 6 deletions(-)

diff --git a/mm/huge_memory.c b/mm/huge_memory.c
index b42c8fa51e46..fd12fa930937 100644
--- a/mm/huge_memory.c
+++ b/mm/huge_memory.c
@@ -1914,12 +1914,7 @@ int zap_huge_pud(struct mmu_gather *tlb, struct vm_area_struct *vma,
 	ptl = __pud_trans_huge_lock(pud, vma);
 	if (!ptl)
 		return 0;
-	/*
-	 * For architectures like ppc64 we look at deposited pgtable
-	 * when calling pudp_huge_get_and_clear. So do the
-	 * pgtable_trans_huge_withdraw after finishing pudp related
-	 * operations.
-	 */
+
 	pudp_huge_get_and_clear_full(tlb->mm, addr, pud, tlb->fullmm);
 	tlb_remove_pud_tlb_entry(tlb, pud, addr);
 	if (vma_is_special_huge(vma)) {
-- 
2.23.0

[PATCH 10/16] mm/huge_memory: check pmd_present first in is_huge_zero_pmd

[Miaohe Lin]

When pmd is non-present, pmd_pfn returns an insane value. So we should
check pmd_present first to avoid acquiring such insane value and also
avoid touching possible cold huge_zero_pfn cache line when pmd isn't
present.

Signed-off-by: Miaohe Lin <linmiaohe@huawei.com>
---
 include/linux/huge_mm.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/include/linux/huge_mm.h b/include/linux/huge_mm.h
index ae3d8e2fd9e2..12b297f9951d 100644
--- a/include/linux/huge_mm.h
+++ b/include/linux/huge_mm.h
@@ -273,7 +273,7 @@ static inline bool is_huge_zero_page(struct page *page)
 
 static inline bool is_huge_zero_pmd(pmd_t pmd)
 {
-	return READ_ONCE(huge_zero_pfn) == pmd_pfn(pmd) && pmd_present(pmd);
+	return pmd_present(pmd) && READ_ONCE(huge_zero_pfn) == pmd_pfn(pmd);
 }
 
 static inline bool is_huge_zero_pud(pud_t pud)
-- 
2.23.0

Re: [PATCH 10/16] mm/huge_memory: check pmd_present first in is_huge_zero_pmd

[Muchun Song]

On Thu, Jun 23, 2022 at 01:06:21AM +0800, Miaohe Lin wrote:
> When pmd is non-present, pmd_pfn returns an insane value. So we should

Although it does not cause any problems, it is better to check pmd_present()
in advance. The change looks good to me. So

> check pmd_present first to avoid acquiring such insane value and also
> avoid touching possible cold huge_zero_pfn cache line when pmd isn't
> present.
> 
> Signed-off-by: Miaohe Lin <linmiaohe@huawei.com>

Reviewed-by: Muchun Song <songmuchun@bytedance.com>

[PATCH 11/16] mm/huge_memory: try to free subpage in swapcache when possible

[Miaohe Lin]

Subpages in swapcache won't be freed even if it is the last user of the
page until next time reclaim. It shouldn't hurt indeed, but we could try
to free these pages to save more memory for system.

Signed-off-by: Miaohe Lin <linmiaohe@huawei.com>
---
 mm/huge_memory.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/mm/huge_memory.c b/mm/huge_memory.c
index fd12fa930937..506e7a682780 100644
--- a/mm/huge_memory.c
+++ b/mm/huge_memory.c
@@ -2539,7 +2539,7 @@ static void __split_huge_page(struct page *page, struct list_head *list,
 		 * requires taking the lru_lock so we do the put_page
 		 * of the tail pages after the split is complete.
 		 */
-		put_page(subpage);
+		free_page_and_swap_cache(subpage);
 	}
 }
 
-- 
2.23.0

[PATCH 12/16] mm/huge_memory: minor cleanup for split_huge_pages_all

[Miaohe Lin]

There is nothing to do if a zone doesn't have any pages managed by the
buddy allocator. So we should check managed_zone instead. Also if a thp
is found, there's no need to traverse the subpages again.

Signed-off-by: Miaohe Lin <linmiaohe@huawei.com>
---
 mm/huge_memory.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/mm/huge_memory.c b/mm/huge_memory.c
index 506e7a682780..0030b4f67cd9 100644
--- a/mm/huge_memory.c
+++ b/mm/huge_memory.c
@@ -2858,9 +2858,12 @@ static void split_huge_pages_all(void)
 	unsigned long total = 0, split = 0;
 
 	pr_debug("Split all THPs\n");
-	for_each_populated_zone(zone) {
+	for_each_zone(zone) {
+		if (!managed_zone(zone))
+			continue;
 		max_zone_pfn = zone_end_pfn(zone);
 		for (pfn = zone->zone_start_pfn; pfn < max_zone_pfn; pfn++) {
+			int nr_pages;
 			if (!pfn_valid(pfn))
 				continue;
 
@@ -2876,8 +2879,10 @@ static void split_huge_pages_all(void)
 
 			total++;
 			lock_page(page);
+			nr_pages = thp_nr_pages(page);
 			if (!split_huge_page(page))
 				split++;
+			pfn += nr_pages - 1;
 			unlock_page(page);
 next:
 			put_page(page);
-- 
2.23.0

[PATCH 13/16] mm/huge_memory: add helper __get_deferred_split_queue

[Miaohe Lin]

Add helper __get_deferred_split_queue to remove the duplicated codes of
getting ds_queue. No functional change intended.

Signed-off-by: Miaohe Lin <linmiaohe@huawei.com>
---
 mm/huge_memory.c | 35 ++++++++++++-----------------------
 1 file changed, 12 insertions(+), 23 deletions(-)

diff --git a/mm/huge_memory.c b/mm/huge_memory.c
index 0030b4f67cd9..de8155ff584c 100644
--- a/mm/huge_memory.c
+++ b/mm/huge_memory.c
@@ -555,25 +555,23 @@ pmd_t maybe_pmd_mkwrite(pmd_t pmd, struct vm_area_struct *vma)
 	return pmd;
 }
 
-#ifdef CONFIG_MEMCG
-static inline struct deferred_split *get_deferred_split_queue(struct page *page)
+static inline struct deferred_split *__get_deferred_split_queue(struct pglist_data *pgdat,
+								struct mem_cgroup *memcg)
 {
-	struct mem_cgroup *memcg = page_memcg(compound_head(page));
-	struct pglist_data *pgdat = NODE_DATA(page_to_nid(page));
-
+#ifdef CONFIG_MEMCG
 	if (memcg)
 		return &memcg->deferred_split_queue;
-	else
-		return &pgdat->deferred_split_queue;
+#endif
+	return &pgdat->deferred_split_queue;
 }
-#else
+
 static inline struct deferred_split *get_deferred_split_queue(struct page *page)
 {
+	struct mem_cgroup *memcg = page_memcg(compound_head(page));
 	struct pglist_data *pgdat = NODE_DATA(page_to_nid(page));
 
-	return &pgdat->deferred_split_queue;
+	return __get_deferred_split_queue(pgdat, memcg);
 }
-#endif
 
 void prep_transhuge_page(struct page *page)
 {
@@ -2774,31 +2772,22 @@ void deferred_split_huge_page(struct page *page)
 static unsigned long deferred_split_count(struct shrinker *shrink,
 		struct shrink_control *sc)
 {
-	struct pglist_data *pgdata = NODE_DATA(sc->nid);
-	struct deferred_split *ds_queue = &pgdata->deferred_split_queue;
+	struct deferred_split *ds_queue;
 
-#ifdef CONFIG_MEMCG
-	if (sc->memcg)
-		ds_queue = &sc->memcg->deferred_split_queue;
-#endif
+	ds_queue = __get_deferred_split_queue(NODE_DATA(sc->nid), sc->memcg);
 	return READ_ONCE(ds_queue->split_queue_len);
 }
 
 static unsigned long deferred_split_scan(struct shrinker *shrink,
 		struct shrink_control *sc)
 {
-	struct pglist_data *pgdata = NODE_DATA(sc->nid);
-	struct deferred_split *ds_queue = &pgdata->deferred_split_queue;
+	struct deferred_split *ds_queue;
 	unsigned long flags;
 	LIST_HEAD(list), *pos, *next;
 	struct page *page;
 	int split = 0;
 
-#ifdef CONFIG_MEMCG
-	if (sc->memcg)
-		ds_queue = &sc->memcg->deferred_split_queue;
-#endif
-
+	ds_queue = __get_deferred_split_queue(NODE_DATA(sc->nid), sc->memcg);
 	spin_lock_irqsave(&ds_queue->split_queue_lock, flags);
 	/* Take pin on all head pages to avoid freeing them under us */
 	list_for_each_safe(pos, next, &ds_queue->split_queue) {
-- 
2.23.0

Re: [PATCH 13/16] mm/huge_memory: add helper __get_deferred_split_queue

[Muchun Song]

On Thu, Jun 23, 2022 at 01:06:24AM +0800, Miaohe Lin wrote:
> Add helper __get_deferred_split_queue to remove the duplicated codes of
> getting ds_queue. No functional change intended.
>

Sorry, I suggest dropping this change since I have reworked the code here [1].

[1] https://lore.kernel.org/all/20220621125658.64935-7-songmuchun@bytedance.com/

> Signed-off-by: Miaohe Lin <linmiaohe@huawei.com>
> ---
>  mm/huge_memory.c | 35 ++++++++++++-----------------------
>  1 file changed, 12 insertions(+), 23 deletions(-)
> 
> diff --git a/mm/huge_memory.c b/mm/huge_memory.c
> index 0030b4f67cd9..de8155ff584c 100644
> --- a/mm/huge_memory.c
> +++ b/mm/huge_memory.c
> @@ -555,25 +555,23 @@ pmd_t maybe_pmd_mkwrite(pmd_t pmd, struct vm_area_struct *vma)
>  	return pmd;
>  }
>  
> -#ifdef CONFIG_MEMCG
> -static inline struct deferred_split *get_deferred_split_queue(struct page *page)
> +static inline struct deferred_split *__get_deferred_split_queue(struct pglist_data *pgdat,
> +								struct mem_cgroup *memcg)
>  {
> -	struct mem_cgroup *memcg = page_memcg(compound_head(page));
> -	struct pglist_data *pgdat = NODE_DATA(page_to_nid(page));
> -
> +#ifdef CONFIG_MEMCG
>  	if (memcg)
>  		return &memcg->deferred_split_queue;
> -	else
> -		return &pgdat->deferred_split_queue;
> +#endif
> +	return &pgdat->deferred_split_queue;
>  }
> -#else
> +
>  static inline struct deferred_split *get_deferred_split_queue(struct page *page)
>  {
> +	struct mem_cgroup *memcg = page_memcg(compound_head(page));
>  	struct pglist_data *pgdat = NODE_DATA(page_to_nid(page));
>  
> -	return &pgdat->deferred_split_queue;
> +	return __get_deferred_split_queue(pgdat, memcg);
>  }
> -#endif
>  
>  void prep_transhuge_page(struct page *page)
>  {
> @@ -2774,31 +2772,22 @@ void deferred_split_huge_page(struct page *page)
>  static unsigned long deferred_split_count(struct shrinker *shrink,
>  		struct shrink_control *sc)
>  {
> -	struct pglist_data *pgdata = NODE_DATA(sc->nid);
> -	struct deferred_split *ds_queue = &pgdata->deferred_split_queue;
> +	struct deferred_split *ds_queue;
>  
> -#ifdef CONFIG_MEMCG
> -	if (sc->memcg)
> -		ds_queue = &sc->memcg->deferred_split_queue;
> -#endif
> +	ds_queue = __get_deferred_split_queue(NODE_DATA(sc->nid), sc->memcg);
>  	return READ_ONCE(ds_queue->split_queue_len);
>  }
>  
>  static unsigned long deferred_split_scan(struct shrinker *shrink,
>  		struct shrink_control *sc)
>  {
> -	struct pglist_data *pgdata = NODE_DATA(sc->nid);
> -	struct deferred_split *ds_queue = &pgdata->deferred_split_queue;
> +	struct deferred_split *ds_queue;
>  	unsigned long flags;
>  	LIST_HEAD(list), *pos, *next;
>  	struct page *page;
>  	int split = 0;
>  
> -#ifdef CONFIG_MEMCG
> -	if (sc->memcg)
> -		ds_queue = &sc->memcg->deferred_split_queue;
> -#endif
> -
> +	ds_queue = __get_deferred_split_queue(NODE_DATA(sc->nid), sc->memcg);
>  	spin_lock_irqsave(&ds_queue->split_queue_lock, flags);
>  	/* Take pin on all head pages to avoid freeing them under us */
>  	list_for_each_safe(pos, next, &ds_queue->split_queue) {
> -- 
> 2.23.0
> 
> 

Re: [PATCH 13/16] mm/huge_memory: add helper __get_deferred_split_queue

[Miaohe Lin]

On 2022/6/23 15:03, Muchun Song wrote:
> On Thu, Jun 23, 2022 at 01:06:24AM +0800, Miaohe Lin wrote:
>> Add helper __get_deferred_split_queue to remove the duplicated codes of
>> getting ds_queue. No functional change intended.
>>
> 
> Sorry, I suggest dropping this change since I have reworked the code here [1].

That's all right. Thanks for your work. :)

> 
> [1] https://lore.kernel.org/all/20220621125658.64935-7-songmuchun@bytedance.com/
> 
>> Signed-off-by: Miaohe Lin <linmiaohe@huawei.com>
>> ---
>>  mm/huge_memory.c | 35 ++++++++++++-----------------------
>>  1 file changed, 12 insertions(+), 23 deletions(-)
>>
>> diff --git a/mm/huge_memory.c b/mm/huge_memory.c
>> index 0030b4f67cd9..de8155ff584c 100644
>> --- a/mm/huge_memory.c
>> +++ b/mm/huge_memory.c
>> @@ -555,25 +555,23 @@ pmd_t maybe_pmd_mkwrite(pmd_t pmd, struct vm_area_struct *vma)
>>  	return pmd;
>>  }
>>  
>> -#ifdef CONFIG_MEMCG
>> -static inline struct deferred_split *get_deferred_split_queue(struct page *page)
>> +static inline struct deferred_split *__get_deferred_split_queue(struct pglist_data *pgdat,
>> +								struct mem_cgroup *memcg)
>>  {
>> -	struct mem_cgroup *memcg = page_memcg(compound_head(page));
>> -	struct pglist_data *pgdat = NODE_DATA(page_to_nid(page));
>> -
>> +#ifdef CONFIG_MEMCG
>>  	if (memcg)
>>  		return &memcg->deferred_split_queue;
>> -	else
>> -		return &pgdat->deferred_split_queue;
>> +#endif
>> +	return &pgdat->deferred_split_queue;
>>  }
>> -#else
>> +
>>  static inline struct deferred_split *get_deferred_split_queue(struct page *page)
>>  {
>> +	struct mem_cgroup *memcg = page_memcg(compound_head(page));
>>  	struct pglist_data *pgdat = NODE_DATA(page_to_nid(page));
>>  
>> -	return &pgdat->deferred_split_queue;
>> +	return __get_deferred_split_queue(pgdat, memcg);
>>  }
>> -#endif
>>  
>>  void prep_transhuge_page(struct page *page)
>>  {
>> @@ -2774,31 +2772,22 @@ void deferred_split_huge_page(struct page *page)
>>  static unsigned long deferred_split_count(struct shrinker *shrink,
>>  		struct shrink_control *sc)
>>  {
>> -	struct pglist_data *pgdata = NODE_DATA(sc->nid);
>> -	struct deferred_split *ds_queue = &pgdata->deferred_split_queue;
>> +	struct deferred_split *ds_queue;
>>  
>> -#ifdef CONFIG_MEMCG
>> -	if (sc->memcg)
>> -		ds_queue = &sc->memcg->deferred_split_queue;
>> -#endif
>> +	ds_queue = __get_deferred_split_queue(NODE_DATA(sc->nid), sc->memcg);
>>  	return READ_ONCE(ds_queue->split_queue_len);
>>  }
>>  
>>  static unsigned long deferred_split_scan(struct shrinker *shrink,
>>  		struct shrink_control *sc)
>>  {
>> -	struct pglist_data *pgdata = NODE_DATA(sc->nid);
>> -	struct deferred_split *ds_queue = &pgdata->deferred_split_queue;
>> +	struct deferred_split *ds_queue;
>>  	unsigned long flags;
>>  	LIST_HEAD(list), *pos, *next;
>>  	struct page *page;
>>  	int split = 0;
>>  
>> -#ifdef CONFIG_MEMCG
>> -	if (sc->memcg)
>> -		ds_queue = &sc->memcg->deferred_split_queue;
>> -#endif
>> -
>> +	ds_queue = __get_deferred_split_queue(NODE_DATA(sc->nid), sc->memcg);
>>  	spin_lock_irqsave(&ds_queue->split_queue_lock, flags);
>>  	/* Take pin on all head pages to avoid freeing them under us */
>>  	list_for_each_safe(pos, next, &ds_queue->split_queue) {
>> -- 
>> 2.23.0
>>
>>
> .
> 

[PATCH 14/16] mm/huge_memory: fix comment of page_deferred_list

[Miaohe Lin]

The current comment is confusing because if global or memcg deferred list
in the second tail page is occupied by compound_head, why we still use
page[2].deferred_list here? I think it wants to say that Global or memcg
deferred list in the first tail page is occupied by compound_mapcount and
compound_pincount so we use the second tail page's deferred_list instead.

Signed-off-by: Miaohe Lin <linmiaohe@huawei.com>
---
 include/linux/huge_mm.h | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/include/linux/huge_mm.h b/include/linux/huge_mm.h
index 12b297f9951d..2e8062b3417a 100644
--- a/include/linux/huge_mm.h
+++ b/include/linux/huge_mm.h
@@ -294,8 +294,8 @@ static inline bool thp_migration_supported(void)
 static inline struct list_head *page_deferred_list(struct page *page)
 {
 	/*
-	 * Global or memcg deferred list in the second tail pages is
-	 * occupied by compound_head.
+	 * Global or memcg deferred list in the first tail page is
+	 * occupied by compound_mapcount and compound_pincount.
 	 */
 	return &page[2].deferred_list;
 }
-- 
2.23.0

Re: [PATCH 14/16] mm/huge_memory: fix comment of page_deferred_list

[Muchun Song]

On Thu, Jun 23, 2022 at 01:06:25AM +0800, Miaohe Lin wrote:
> The current comment is confusing because if global or memcg deferred list
> in the second tail page is occupied by compound_head, why we still use
> page[2].deferred_list here? I think it wants to say that Global or memcg
> deferred list in the first tail page is occupied by compound_mapcount and
> compound_pincount so we use the second tail page's deferred_list instead.
> 
> Signed-off-by: Miaohe Lin <linmiaohe@huawei.com>
> ---
>  include/linux/huge_mm.h | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/include/linux/huge_mm.h b/include/linux/huge_mm.h
> index 12b297f9951d..2e8062b3417a 100644
> --- a/include/linux/huge_mm.h
> +++ b/include/linux/huge_mm.h
> @@ -294,8 +294,8 @@ static inline bool thp_migration_supported(void)
>  static inline struct list_head *page_deferred_list(struct page *page)
>  {
>  	/*
> -	 * Global or memcg deferred list in the second tail pages is
> -	 * occupied by compound_head.
> +	 * Global or memcg deferred list in the first tail page is
> +	 * occupied by compound_mapcount and compound_pincount.
>  	 */

The structure of "struct page" seems to have told us the information that
we resue the 2nd tail page to be used as deferred_list. I am not sure the
value of those comments. Maybe better to remove them?

Thanks.

>  	return &page[2].deferred_list;
>  }
> -- 
> 2.23.0
> 
> 

Re: [PATCH 14/16] mm/huge_memory: fix comment of page_deferred_list

[Miaohe Lin]

On 2022/6/23 15:24, Muchun Song wrote:
> On Thu, Jun 23, 2022 at 01:06:25AM +0800, Miaohe Lin wrote:
>> The current comment is confusing because if global or memcg deferred list
>> in the second tail page is occupied by compound_head, why we still use
>> page[2].deferred_list here? I think it wants to say that Global or memcg
>> deferred list in the first tail page is occupied by compound_mapcount and
>> compound_pincount so we use the second tail page's deferred_list instead.
>>
>> Signed-off-by: Miaohe Lin <linmiaohe@huawei.com>
>> ---
>>  include/linux/huge_mm.h | 4 ++--
>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/include/linux/huge_mm.h b/include/linux/huge_mm.h
>> index 12b297f9951d..2e8062b3417a 100644
>> --- a/include/linux/huge_mm.h
>> +++ b/include/linux/huge_mm.h
>> @@ -294,8 +294,8 @@ static inline bool thp_migration_supported(void)
>>  static inline struct list_head *page_deferred_list(struct page *page)
>>  {
>>  	/*
>> -	 * Global or memcg deferred list in the second tail pages is
>> -	 * occupied by compound_head.
>> +	 * Global or memcg deferred list in the first tail page is
>> +	 * occupied by compound_mapcount and compound_pincount.
>>  	 */
> 
> The structure of "struct page" seems to have told us the information that
> we resue the 2nd tail page to be used as deferred_list. I am not sure the

Yes, it does.

> value of those comments. Maybe better to remove them?

IMHO above comment tries to tell us why deferred list in the second tail page is used
instead of first tail page. But it should be fine to remove the above comments as they
don't seem to provide much info (thought I'm not really sure).

Thanks.

> 
> Thanks.
> 
>>  	return &page[2].deferred_list;
>>  }
>> -- 
>> 2.23.0
>>
>>
> .
> 

Re: [PATCH 14/16] mm/huge_memory: fix comment of page_deferred_list

[Zach O'Keefe]

On 23 Jun 20:26, Miaohe Lin wrote:
> On 2022/6/23 15:24, Muchun Song wrote:
> > On Thu, Jun 23, 2022 at 01:06:25AM +0800, Miaohe Lin wrote:
> >> The current comment is confusing because if global or memcg deferred list
> >> in the second tail page is occupied by compound_head, why we still use
> >> page[2].deferred_list here? I think it wants to say that Global or memcg
> >> deferred list in the first tail page is occupied by compound_mapcount and
> >> compound_pincount so we use the second tail page's deferred_list instead.
> >>
> >> Signed-off-by: Miaohe Lin <linmiaohe@huawei.com>
> >> ---
> >>  include/linux/huge_mm.h | 4 ++--
> >>  1 file changed, 2 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/include/linux/huge_mm.h b/include/linux/huge_mm.h
> >> index 12b297f9951d..2e8062b3417a 100644
> >> --- a/include/linux/huge_mm.h
> >> +++ b/include/linux/huge_mm.h
> >> @@ -294,8 +294,8 @@ static inline bool thp_migration_supported(void)
> >>  static inline struct list_head *page_deferred_list(struct page *page)
> >>  {
> >>  	/*
> >> -	 * Global or memcg deferred list in the second tail pages is
> >> -	 * occupied by compound_head.
> >> +	 * Global or memcg deferred list in the first tail page is
> >> +	 * occupied by compound_mapcount and compound_pincount.
> >>  	 */
> > 
> > The structure of "struct page" seems to have told us the information that
> > we resue the 2nd tail page to be used as deferred_list. I am not sure the
> 
> Yes, it does.
> 
> > value of those comments. Maybe better to remove them?
> 
> IMHO above comment tries to tell us why deferred list in the second tail page is used
> instead of first tail page. But it should be fine to remove the above comments as they
> don't seem to provide much info (thought I'm not really sure).
> 
> Thanks.
> 

Just a suggestion - feel free to disregard. Maybe we don't need to repeat the
comments in struct page, but maybe a "see organization of tail pages of compound
page in "struct page" definition" would at least point new people to where this
magic 2 comes from.  Maybe an obvious place to check after you're familiar with
overloading struct page data for compound pages - but IMO it's not obvious for
newcomers.

> > 
> > Thanks.
> > 
> >>  	return &page[2].deferred_list;
> >>  }
> >> -- 
> >> 2.23.0
> >>
> >>
> > .
> > 
> 

Re: [PATCH 14/16] mm/huge_memory: fix comment of page_deferred_list

[Miaohe Lin]

On 2022/6/25 1:09, Zach O'Keefe wrote:
> On 23 Jun 20:26, Miaohe Lin wrote:
>> On 2022/6/23 15:24, Muchun Song wrote:
>>> On Thu, Jun 23, 2022 at 01:06:25AM +0800, Miaohe Lin wrote:
>>>> The current comment is confusing because if global or memcg deferred list
>>>> in the second tail page is occupied by compound_head, why we still use
>>>> page[2].deferred_list here? I think it wants to say that Global or memcg
>>>> deferred list in the first tail page is occupied by compound_mapcount and
>>>> compound_pincount so we use the second tail page's deferred_list instead.
>>>>
>>>> Signed-off-by: Miaohe Lin <linmiaohe@huawei.com>
>>>> ---
>>>>  include/linux/huge_mm.h | 4 ++--
>>>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/include/linux/huge_mm.h b/include/linux/huge_mm.h
>>>> index 12b297f9951d..2e8062b3417a 100644
>>>> --- a/include/linux/huge_mm.h
>>>> +++ b/include/linux/huge_mm.h
>>>> @@ -294,8 +294,8 @@ static inline bool thp_migration_supported(void)
>>>>  static inline struct list_head *page_deferred_list(struct page *page)
>>>>  {
>>>>  	/*
>>>> -	 * Global or memcg deferred list in the second tail pages is
>>>> -	 * occupied by compound_head.
>>>> +	 * Global or memcg deferred list in the first tail page is
>>>> +	 * occupied by compound_mapcount and compound_pincount.
>>>>  	 */
>>>
>>> The structure of "struct page" seems to have told us the information that
>>> we resue the 2nd tail page to be used as deferred_list. I am not sure the
>>
>> Yes, it does.
>>
>>> value of those comments. Maybe better to remove them?
>>
>> IMHO above comment tries to tell us why deferred list in the second tail page is used
>> instead of first tail page. But it should be fine to remove the above comments as they
>> don't seem to provide much info (thought I'm not really sure).
>>
>> Thanks.
>>
> 
> Just a suggestion - feel free to disregard. Maybe we don't need to repeat the
> comments in struct page, but maybe a "see organization of tail pages of compound
> page in "struct page" definition" would at least point new people to where this
> magic 2 comes from.  Maybe an obvious place to check after you're familiar with
> overloading struct page data for compound pages - but IMO it's not obvious for
> newcomers.

This is a good suggestion to me. Thanks!

> 
>>>
>>> Thanks.
>>>
>>>>  	return &page[2].deferred_list;
>>>>  }
>>>> -- 
>>>> 2.23.0
>>>>
>>>>
>>> .
>>>
>>
> .
> 

[PATCH 15/16] mm/huge_memory: correct comment of prep_transhuge_page

[Miaohe Lin]

We use page->mapping and page->index, instead of page->indexlru in second
tail page as list_head. Correct it.

Signed-off-by: Miaohe Lin <linmiaohe@huawei.com>
---
 mm/huge_memory.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/mm/huge_memory.c b/mm/huge_memory.c
index de8155ff584c..8bd937cc1f74 100644
--- a/mm/huge_memory.c
+++ b/mm/huge_memory.c
@@ -576,7 +576,7 @@ static inline struct deferred_split *get_deferred_split_queue(struct page *page)
 void prep_transhuge_page(struct page *page)
 {
 	/*
-	 * we use page->mapping and page->indexlru in second tail page
+	 * we use page->mapping and page->index in second tail page
 	 * as list_head: assuming THP order >= 2
 	 */
 
-- 
2.23.0

Re: [PATCH 15/16] mm/huge_memory: correct comment of prep_transhuge_page

[Muchun Song]

On Thu, Jun 23, 2022 at 01:06:26AM +0800, Miaohe Lin wrote:
> We use page->mapping and page->index, instead of page->indexlru in second
> tail page as list_head. Correct it.
> 
> Signed-off-by: Miaohe Lin <linmiaohe@huawei.com>

Reviewed-by: Muchun Song <songmuchun@bytedance.com>

Thanks.

[PATCH 16/16] mm/huge_memory: comment the subtle logic in __split_huge_pmd

[Miaohe Lin]

It's dangerous and wrong to call page_folio(pmd_page(*pmd)) when pmd isn't
present. But the caller guarantees pmd is present when folio is set. So we
should be safe here. Add comment to make it clear.

Signed-off-by: Miaohe Lin <linmiaohe@huawei.com>
---
 mm/huge_memory.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/mm/huge_memory.c b/mm/huge_memory.c
index 8bd937cc1f74..b98b97592bd3 100644
--- a/mm/huge_memory.c
+++ b/mm/huge_memory.c
@@ -2234,6 +2234,10 @@ void __split_huge_pmd(struct vm_area_struct *vma, pmd_t *pmd,
 
 	if (pmd_trans_huge(*pmd) || pmd_devmap(*pmd) ||
 	    is_pmd_migration_entry(*pmd)) {
+		/*
+		 * It's safe to call pmd_page when folio is set because it's
+		 * guaranteed that pmd is present.
+		 */
 		if (folio && folio != page_folio(pmd_page(*pmd)))
 			goto out;
 		__split_huge_pmd_locked(vma, pmd, range.start, freeze);
-- 
2.23.0