lkml.org 
[lkml]   [2019]   [Jul]   [17]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [v3 PATCH 1/2] mm: thp: make transhuge_vma_suitable available for anonymous THP
From
Date


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@google.com>
>> Cc: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
>> Cc: Michal Hocko <mhocko@suse.com>
>> Cc: Vlastimil Babka <vbabka@suse.cz>
>> Cc: David Rientjes <rientjes@google.com>
>> Signed-off-by: Yang Shi <yang.shi@linux.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

\
 
 \ /
  Last update: 2019-07-17 23:04    [W:0.068 / U:0.532 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site