linux-kernel - Re: [v3 PATCH 1/2] mm: thp: make transhuge_vma

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date:   Wed, 17 Jul 2019 14:03:55 -0700
From:   Yang Shi <yang.shi@...ux.alibaba.com>
To:     Hugh Dickins <hughd@...gle.com>
Cc:     kirill.shutemov@...ux.intel.com, mhocko@...e.com, vbabka@...e.cz,
        rientjes@...gle.com, akpm@...ux-foundation.org, linux-mm@...ck.org,
        linux-kernel@...r.kernel.org
Subject: Re: [v3 PATCH 1/2] mm: thp: make transhuge_vma_suitable available for
 anonymous THP



On 7/17/19 12:43 PM, Hugh Dickins wrote:
> On Thu, 13 Jun 2019, Yang Shi wrote:
>
>> The transhuge_vma_suitable() was only available for shmem THP, but
>> anonymous THP has the same check except pgoff check.  And, it will be
>> used for THP eligible check in the later patch, so make it available for
>> all kind of THPs.  This also helps reduce code duplication slightly.
>>
>> Since anonymous THP doesn't have to check pgoff, so make pgoff check
>> shmem vma only.
> Yes, I think you are right to avoid the pgoff check on anonymous.
> I had originally thought that it would work out okay even with the
> pgoff check on anonymous, and usually it would: but could give the
> wrong answer on an mremap-moved anonymous area.
>
>> Cc: Hugh Dickins <hughd@...gle.com>
>> Cc: Kirill A. Shutemov <kirill.shutemov@...ux.intel.com>
>> Cc: Michal Hocko <mhocko@...e.com>
>> Cc: Vlastimil Babka <vbabka@...e.cz>
>> Cc: David Rientjes <rientjes@...gle.com>
>> Signed-off-by: Yang Shi <yang.shi@...ux.alibaba.com>
> Almost Acked-by me, but there's one nit I'd much prefer to change:
> sorry for being such a late nuisance...
>
>> ---
>>   mm/huge_memory.c |  2 +-
>>   mm/internal.h    | 25 +++++++++++++++++++++++++
>>   mm/memory.c      | 13 -------------
>>   3 files changed, 26 insertions(+), 14 deletions(-)
>>
>> diff --git a/mm/huge_memory.c b/mm/huge_memory.c
>> index 9f8bce9..4bc2552 100644
>> --- a/mm/huge_memory.c
>> +++ b/mm/huge_memory.c
>> @@ -691,7 +691,7 @@ vm_fault_t do_huge_pmd_anonymous_page(struct vm_fault *vmf)
>>   	struct page *page;
>>   	unsigned long haddr = vmf->address & HPAGE_PMD_MASK;
>>   
>> -	if (haddr < vma->vm_start || haddr + HPAGE_PMD_SIZE > vma->vm_end)
>> +	if (!transhuge_vma_suitable(vma, haddr))
>>   		return VM_FAULT_FALLBACK;
>>   	if (unlikely(anon_vma_prepare(vma)))
>>   		return VM_FAULT_OOM;
>> diff --git a/mm/internal.h b/mm/internal.h
>> index 9eeaf2b..7f096ba 100644
>> --- a/mm/internal.h
>> +++ b/mm/internal.h
>> @@ -555,4 +555,29 @@ static inline bool is_migrate_highatomic_page(struct page *page)
>>   
>>   void setup_zone_pageset(struct zone *zone);
>>   extern struct page *alloc_new_node_page(struct page *page, unsigned long node);
>> +
>> +#ifdef CONFIG_TRANSPARENT_HUGEPAGE
>> +#define HPAGE_CACHE_INDEX_MASK (HPAGE_PMD_NR - 1)
>> +static inline bool transhuge_vma_suitable(struct vm_area_struct *vma,
>> +		unsigned long haddr)
>> +{
>> +	/* Don't have to check pgoff for anonymous vma */
>> +	if (!vma_is_anonymous(vma)) {
>> +		if (((vma->vm_start >> PAGE_SHIFT) & HPAGE_CACHE_INDEX_MASK) !=
>> +			(vma->vm_pgoff & HPAGE_CACHE_INDEX_MASK))
>> +			return false;
>> +	}
>> +
>> +	if (haddr < vma->vm_start || haddr + HPAGE_PMD_SIZE > vma->vm_end)
>> +		return false;
>> +	return true;
>> +}
>> +#else
>> +static inline bool transhuge_vma_suitable(struct vma_area_struct *vma,
>> +		unsigned long haddr)
>> +{
>> +	return false;
>> +}
>> +#endif
>> +
>>   #endif	/* __MM_INTERNAL_H */
> ... maybe I'm just not much of a fan of mm/internal.h (where at last you
> find odd bits and pieces which you had expected to find elsewhere), and
> maybe others will disagree: but I'd say transhuge_vma_suitable() surely
> belongs in include/linux/huge_mm.h, near __transparent_hugepage_enabled().
>
> But then your correct use of vma_is_anonymous() gets more complicated:
> because that declaration is over in include/linux/mm.h; and although
> linux/mm.h includes linux/huge_mm.h, vma_is_anonymous() comes lower down.
>
> However... linux/mm.h's definition of vma_set_anonymous() comes higher
> up, and it would make perfect sense to move vma_is_anonymous up to just
> after vma_set_anonymous(), wouldn't it?  Should vma_is_shmem() and
> vma_is_stack_for_current() declarations move with it? Probably yes:
> they make more sense near vma_is_anonymous() than where they were.

Thanks for the thorough instructions. Will fix this in v4.

>
> Hugh
>
>> diff --git a/mm/memory.c b/mm/memory.c
>> index 96f1d47..2286424 100644
>> --- a/mm/memory.c
>> +++ b/mm/memory.c
>> @@ -3205,19 +3205,6 @@ static vm_fault_t pte_alloc_one_map(struct vm_fault *vmf)
>>   }
>>   
>>   #ifdef CONFIG_TRANSPARENT_HUGE_PAGECACHE
>> -
>> -#define HPAGE_CACHE_INDEX_MASK (HPAGE_PMD_NR - 1)
>> -static inline bool transhuge_vma_suitable(struct vm_area_struct *vma,
>> -		unsigned long haddr)
>> -{
>> -	if (((vma->vm_start >> PAGE_SHIFT) & HPAGE_CACHE_INDEX_MASK) !=
>> -			(vma->vm_pgoff & HPAGE_CACHE_INDEX_MASK))
>> -		return false;
>> -	if (haddr < vma->vm_start || haddr + HPAGE_PMD_SIZE > vma->vm_end)
>> -		return false;
>> -	return true;
>> -}
>> -
>>   static void deposit_prealloc_pte(struct vm_fault *vmf)
>>   {
>>   	struct vm_area_struct *vma = vmf->vma;
>> -- 
>> 1.8.3.1