[PATCH -mm] mm, madvise, THP: Use THP aligned address in madvise_free_huge_pmd()

[PATCH -mm] mm, madvise, THP: Use THP aligned address in madvise_free_huge_pmd()

[Huang, Ying]

From: Huang Ying <ying.huang@intel.com>

The address argument passed in madvise_free_huge_pmd() may be not THP
aligned.  But some THP operations like pmdp_invalidate(),
set_pmd_at(), and tlb_remove_pmd_tlb_entry() need the address to be
THP aligned.  Fix this via using THP aligned address for these
functions in madvise_free_huge_pmd().

Signed-off-by: "Huang, Ying" <ying.huang@intel.com>
Cc: "Kirill A. Shutemov" <kirill.shutemov@linux.intel.com>
Cc: Michal Hocko <mhocko@suse.com>
Cc: Minchan Kim <minchan@kernel.org>
Cc: Shaohua Li <shli@kernel.org>
Cc: Zi Yan <zi.yan@cs.rutgers.edu>
Cc: jglisse@redhat.com
Cc: "Aneesh Kumar K.V" <aneesh.kumar@linux.vnet.ibm.com>
---
 mm/huge_memory.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/mm/huge_memory.c b/mm/huge_memory.c
index 0cc62405de9c..c5e1bfb08bd7 100644
--- a/mm/huge_memory.c
+++ b/mm/huge_memory.c
@@ -1617,6 +1617,7 @@ bool madvise_free_huge_pmd(struct mmu_gather *tlb, struct vm_area_struct *vma,
 	struct page *page;
 	struct mm_struct *mm = tlb->mm;
 	bool ret = false;
+	unsigned long haddr = addr & HPAGE_PMD_MASK;
 
 	tlb_remove_check_page_size_change(tlb, HPAGE_PMD_SIZE);
 
@@ -1663,12 +1664,12 @@ bool madvise_free_huge_pmd(struct mmu_gather *tlb, struct vm_area_struct *vma,
 	unlock_page(page);
 
 	if (pmd_young(orig_pmd) || pmd_dirty(orig_pmd)) {
-		pmdp_invalidate(vma, addr, pmd);
+		pmdp_invalidate(vma, haddr, pmd);
 		orig_pmd = pmd_mkold(orig_pmd);
 		orig_pmd = pmd_mkclean(orig_pmd);
 
-		set_pmd_at(mm, addr, pmd, orig_pmd);
-		tlb_remove_pmd_tlb_entry(tlb, pmd, addr);
+		set_pmd_at(mm, haddr, pmd, orig_pmd);
+		tlb_remove_pmd_tlb_entry(tlb, pmd, haddr);
 	}
 
 	mark_page_lazyfree(page);
-- 
2.16.1

Re: [PATCH -mm] mm, madvise, THP: Use THP aligned address in madvise_free_huge_pmd()

[Zi Yan]

[-- Attachment #1: Type: text/plain, Size: 2476 bytes --]

This cannot happen.

Two address parameters are passed: addr and next.
If “addr” is not aligned and “next” is aligned or the end of madvise range, which might not be aligned,
either way next - addr < HPAGE_PMD_SIZE.

This means the code in “if (next - addr != HPAGE_PMD_SIZE)”, which is above your second hunk,
will split the THP between “addr” and “next” and get out as long as “addr“ is not aligned.
Thus, the code in your second hunk should always get aligned “addr”.

Let me know if I miss anything.

—
Best Regards,
Yan Zi

On 14 Mar 2018, at 21:18, Huang, Ying wrote:

> From: Huang Ying <ying.huang@intel.com>
>
> The address argument passed in madvise_free_huge_pmd() may be not THP
> aligned.  But some THP operations like pmdp_invalidate(),
> set_pmd_at(), and tlb_remove_pmd_tlb_entry() need the address to be
> THP aligned.  Fix this via using THP aligned address for these
> functions in madvise_free_huge_pmd().
>
> Signed-off-by: "Huang, Ying" <ying.huang@intel.com>
> Cc: "Kirill A. Shutemov" <kirill.shutemov@linux.intel.com>
> Cc: Michal Hocko <mhocko@suse.com>
> Cc: Minchan Kim <minchan@kernel.org>
> Cc: Shaohua Li <shli@kernel.org>
> Cc: Zi Yan <zi.yan@cs.rutgers.edu>
> Cc: jglisse@redhat.com
> Cc: "Aneesh Kumar K.V" <aneesh.kumar@linux.vnet.ibm.com>
> ---
>  mm/huge_memory.c | 7 ++++---
>  1 file changed, 4 insertions(+), 3 deletions(-)
>
> diff --git a/mm/huge_memory.c b/mm/huge_memory.c
> index 0cc62405de9c..c5e1bfb08bd7 100644
> --- a/mm/huge_memory.c
> +++ b/mm/huge_memory.c
> @@ -1617,6 +1617,7 @@ bool madvise_free_huge_pmd(struct mmu_gather *tlb, struct vm_area_struct *vma,
>  	struct page *page;
>  	struct mm_struct *mm = tlb->mm;
>  	bool ret = false;
> +	unsigned long haddr = addr & HPAGE_PMD_MASK;
>
>  	tlb_remove_check_page_size_change(tlb, HPAGE_PMD_SIZE);
>
> @@ -1663,12 +1664,12 @@ bool madvise_free_huge_pmd(struct mmu_gather *tlb, struct vm_area_struct *vma,
>  	unlock_page(page);
>
>  	if (pmd_young(orig_pmd) || pmd_dirty(orig_pmd)) {
> -		pmdp_invalidate(vma, addr, pmd);
> +		pmdp_invalidate(vma, haddr, pmd);
>  		orig_pmd = pmd_mkold(orig_pmd);
>  		orig_pmd = pmd_mkclean(orig_pmd);
>
> -		set_pmd_at(mm, addr, pmd, orig_pmd);
> -		tlb_remove_pmd_tlb_entry(tlb, pmd, addr);
> +		set_pmd_at(mm, haddr, pmd, orig_pmd);
> +		tlb_remove_pmd_tlb_entry(tlb, pmd, haddr);
>  	}
>
>  	mark_page_lazyfree(page);
> -- 
> 2.16.1

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 557 bytes --]

Re: [PATCH -mm] mm, madvise, THP: Use THP aligned address in madvise_free_huge_pmd()

[Huang, Ying]

Zi Yan <zi.yan@cs.rutgers.edu> writes:

> This cannot happen.
>
> Two address parameters are passed: addr and next.
> If a??addra?? is not aligned and a??nexta?? is aligned or the end of madvise range, which might not be aligned,
> either way next - addr < HPAGE_PMD_SIZE.
>
> This means the code in a??if (next - addr != HPAGE_PMD_SIZE)a??, which is above your second hunk,
> will split the THP between a??addra?? and a??nexta?? and get out as long as a??addra?? is not aligned.
> Thus, the code in your second hunk should always get aligned a??addra??.
>
> Let me know if I miss anything.

Yes, you are right!  Thanks for pointing this out.

Sorry for bothering, please ignore this patch.

Best Regards,
Huang, Ying

> a??
> Best Regards,
> Yan Zi
>
> On 14 Mar 2018, at 21:18, Huang, Ying wrote:
>
>> From: Huang Ying <ying.huang@intel.com>
>>
>> The address argument passed in madvise_free_huge_pmd() may be not THP
>> aligned.  But some THP operations like pmdp_invalidate(),
>> set_pmd_at(), and tlb_remove_pmd_tlb_entry() need the address to be
>> THP aligned.  Fix this via using THP aligned address for these
>> functions in madvise_free_huge_pmd().
>>
>> Signed-off-by: "Huang, Ying" <ying.huang@intel.com>
>> Cc: "Kirill A. Shutemov" <kirill.shutemov@linux.intel.com>
>> Cc: Michal Hocko <mhocko@suse.com>
>> Cc: Minchan Kim <minchan@kernel.org>
>> Cc: Shaohua Li <shli@kernel.org>
>> Cc: Zi Yan <zi.yan@cs.rutgers.edu>
>> Cc: jglisse@redhat.com
>> Cc: "Aneesh Kumar K.V" <aneesh.kumar@linux.vnet.ibm.com>
>> ---
>>  mm/huge_memory.c | 7 ++++---
>>  1 file changed, 4 insertions(+), 3 deletions(-)
>>
>> diff --git a/mm/huge_memory.c b/mm/huge_memory.c
>> index 0cc62405de9c..c5e1bfb08bd7 100644
>> --- a/mm/huge_memory.c
>> +++ b/mm/huge_memory.c
>> @@ -1617,6 +1617,7 @@ bool madvise_free_huge_pmd(struct mmu_gather *tlb, struct vm_area_struct *vma,
>>  	struct page *page;
>>  	struct mm_struct *mm = tlb->mm;
>>  	bool ret = false;
>> +	unsigned long haddr = addr & HPAGE_PMD_MASK;
>>
>>  	tlb_remove_check_page_size_change(tlb, HPAGE_PMD_SIZE);
>>
>> @@ -1663,12 +1664,12 @@ bool madvise_free_huge_pmd(struct mmu_gather *tlb, struct vm_area_struct *vma,
>>  	unlock_page(page);
>>
>>  	if (pmd_young(orig_pmd) || pmd_dirty(orig_pmd)) {
>> -		pmdp_invalidate(vma, addr, pmd);
>> +		pmdp_invalidate(vma, haddr, pmd);
>>  		orig_pmd = pmd_mkold(orig_pmd);
>>  		orig_pmd = pmd_mkclean(orig_pmd);
>>
>> -		set_pmd_at(mm, addr, pmd, orig_pmd);
>> -		tlb_remove_pmd_tlb_entry(tlb, pmd, addr);
>> +		set_pmd_at(mm, haddr, pmd, orig_pmd);
>> +		tlb_remove_pmd_tlb_entry(tlb, pmd, haddr);
>>  	}
>>
>>  	mark_page_lazyfree(page);
>> -- 
>> 2.16.1

Re: [PATCH -mm] mm, madvise, THP: Use THP aligned address in madvise_free_huge_pmd()

[Michal Hocko]

On Wed 14-03-18 21:39:54, Zi Yan wrote:
> This cannot happen.
> 
> Two address parameters are passed: addr and next.
> If a??addra?? is not aligned and a??nexta?? is aligned or the end of madvise range, which might not be aligned,
> either way next - addr < HPAGE_PMD_SIZE.
> 
> This means the code in a??if (next - addr != HPAGE_PMD_SIZE)a??, which is above your second hunk,
> will split the THP between a??addra?? and a??nexta?? and get out as long as a??addra?? is not aligned.
> Thus, the code in your second hunk should always get aligned a??addra??.

OK, so what would happen if the above doesn't hold anymore after some
change up the call chain? Is it critical? If yes, do we want VM_BUG_ON
to detect that? Or at least document the asumption?
-- 
Michal Hocko
SUSE Labs