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@xxxxxxxxxx>
Cc: Kirill A. Shutemov <kirill.shutemov@xxxxxxxxxxxxxxx>
Cc: Michal Hocko <mhocko@xxxxxxxx>
Cc: Vlastimil Babka <vbabka@xxxxxxx>
Cc: David Rientjes <rientjes@xxxxxxxxxx>
Signed-off-by: Yang Shi <yang.shi@xxxxxxxxxxxxxxxxx>
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
