From: Yang Shi <yang.shi@linux.alibaba.com>
To: Hugh Dickins <hughd@google.com>
Cc: kirill.shutemov@linux.intel.com, mhocko@suse.com, vbabka@suse.cz,
rientjes@google.com, akpm@linux-foundation.org,
linux-mm@kvack.org, linux-kernel@vger.kernel.org
Subject: Re: [v3 PATCH 1/2] mm: thp: make transhuge_vma_suitable available for anonymous THP
Date: Wed, 17 Jul 2019 14:03:55 -0700 [thread overview]
Message-ID: <85d8060b-76ab-76d8-1fc5-496e07378722@linux.alibaba.com> (raw)
In-Reply-To: <alpine.LSU.2.11.1907171207080.1177@eggly.anvils>
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
next prev parent reply other threads:[~2019-07-17 21:04 UTC|newest]
Thread overview: 12+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-06-13 4:43 [v3 PATCH 0/2] Fix false negative of shmem vma's THP eligibility Yang Shi
2019-06-13 4:44 ` [v3 PATCH 1/2] mm: thp: make transhuge_vma_suitable available for anonymous THP Yang Shi
2019-07-17 19:43 ` Hugh Dickins
2019-07-17 21:03 ` Yang Shi [this message]
2019-06-13 4:44 ` [v3 PATCH 2/2] mm: thp: fix false negative of shmem vma's THP eligibility Yang Shi
2019-06-19 12:12 ` Vlastimil Babka
2019-06-19 16:28 ` Yang Shi
2019-07-18 21:44 ` Andrew Morton
2019-07-18 21:52 ` Vlastimil Babka
2019-07-18 22:06 ` Hugh Dickins
2019-07-17 19:44 ` Hugh Dickins
2019-07-15 19:49 ` [v3 PATCH 0/2] Fix " Yang Shi
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=85d8060b-76ab-76d8-1fc5-496e07378722@linux.alibaba.com \
--to=yang.shi@linux.alibaba.com \
--cc=akpm@linux-foundation.org \
--cc=hughd@google.com \
--cc=kirill.shutemov@linux.intel.com \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-mm@kvack.org \
--cc=mhocko@suse.com \
--cc=rientjes@google.com \
--cc=vbabka@suse.cz \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).