mm: thp: support "THPeligible" semantics for mTHP with anonymous shmem

PATCH Superseded Bang Li 3 participants 11 messages Jul 2, 2024 – Jul 4, 2024

Subsystems (3)

Memory management THP tmpfs (shmem filesystem)

lore

Share & tools

lore

Bang Li

author

2024-07-02 02:37 UTC #0 lore raw

PATCH v2 1/1 mm: thp: support "THPeligible" semantics for mTHP with anonymous shmem

After the commit 7fb1b252afb5 ("mm: shmem: add mTHP support for anonymous shmem"), we can configure different policies through the multi-size THP sysfs interface for anonymous shmem. But currently "THPeligible" indicates only whether the mapping is eligible for allocating THP-pages as well as the THP is PMD mappable or not for anonymous shmem, we need to support semantics for mTHP with anonymous shmem similar to those for mTHP with anonymous memory.

Signed-off-by Bang Li <[email protected]>

---
Changes since v1 [1]:
- Put anonymous shmem mthp related logic into
thp_vma_allowable_orders() (per David)

[1] https://lore.kernel.org/linux-mm/[email protected]/

---
 include/linux/huge_mm.h | 11 +++++++++++
 mm/huge_memory.c        | 13 +++++++++----
 mm/shmem.c              |  9 +--------
 3 files changed, 21 insertions(+), 12 deletions(-)

include/linux/huge_mm.h +11 -0

index 212cca384d7e..f87136f38aa1 100644
@@ -267,6 +267,10 @@ unsigned long thp_vma_allowable_orders(struct vm_area_struct *vma,
 	return __thp_vma_allowable_orders(vma, vm_flags, tva_flags, orders);
 }
 
+unsigned long shmem_allowable_huge_orders(struct inode *inode,
+				struct vm_area_struct *vma, pgoff_t index,
+				bool global_huge);
+
 struct thpsize {
 	struct kobject kobj;
 	struct list_head node;

@@ -460,6 +464,13 @@ static inline unsigned long thp_vma_allowable_orders(struct vm_area_struct *vma,
 	return 0;
 }
 
+static inline unsigned long shmem_allowable_huge_orders(struct inode *inode,
+				struct vm_area_struct *vma, pgoff_t index,
+				bool global_huge)
+{
+	return 0;
+}
+
 #define transparent_hugepage_flags 0UL
 
 #define thp_get_unmapped_area	NULL

mm/huge_memory.c +9 -4

index c7ce28f6b7f3..ea377bb4af91 100644
@@ -151,10 +151,15 @@ unsigned long __thp_vma_allowable_orders(struct vm_area_struct *vma,
 	 * Must be done before hugepage flags check since shmem has its
 	 * own flags.
 	 */
-	if (!in_pf && shmem_file(vma->vm_file))
-		return shmem_is_huge(file_inode(vma->vm_file), vma->vm_pgoff,
-				     !enforce_sysfs, vma->vm_mm, vm_flags)
-			? orders : 0;
+	if (!in_pf && shmem_file(vma->vm_file)) {
+		bool global_huge = shmem_is_huge(file_inode(vma->vm_file), vma->vm_pgoff,
+							!enforce_sysfs, vma->vm_mm, vm_flags);
+
+		if (!vma_is_anon_shmem(vma))
+			return global_huge? orders : 0;
+		return shmem_allowable_huge_orders(file_inode(vma->vm_file),
+							vma, vma->vm_pgoff, global_huge);
+	}
 
 	if (!vma_is_anonymous(vma)) {
 		/*

mm/shmem.c +1 -8

index d495c0701a83..aa85df9c662a 100644
@@ -1622,7 +1622,7 @@ static gfp_t limit_gfp_mask(gfp_t huge_gfp, gfp_t limit_gfp)
 }
 
 #ifdef CONFIG_TRANSPARENT_HUGEPAGE
-static unsigned long shmem_allowable_huge_orders(struct inode *inode,
+unsigned long shmem_allowable_huge_orders(struct inode *inode,
 				struct vm_area_struct *vma, pgoff_t index,
 				bool global_huge)
 {

@@ -1707,13 +1707,6 @@ static unsigned long shmem_suitable_orders(struct inode *inode, struct vm_fault
 	return orders;
 }
 #else
-static unsigned long shmem_allowable_huge_orders(struct inode *inode,
-				struct vm_area_struct *vma, pgoff_t index,
-				bool global_huge)
-{
-	return 0;
-}
-
 static unsigned long shmem_suitable_orders(struct inode *inode, struct vm_fault *vmf,
 					   struct address_space *mapping, pgoff_t index,
 					   unsigned long orders)

Signature · 1 line

2.19.1.6.gb485710b

Baolin Wang

reviewer

2024-07-02 06:14 UTC #1 lore raw

Quoted 38 lines · Bang Li

After the commit 7fb1b252afb5 ("mm: shmem: add mTHP support for anonymous shmem"), we can configure different policies through the multi-size THP sysfs interface for anonymous shmem. But currently "THPeligible" indicates only whether the mapping is eligible for allocating THP-pages as well as the THP is PMD mappable or not for anonymous shmem, we need to support semantics for mTHP with anonymous shmem similar to those for mTHP with anonymous memory.
Signed-off-by Bang Li <[email protected]>
---
Changes since v1 [1]:
- Put anonymous shmem mthp related logic into
thp_vma_allowable_orders() (per David)
[1] https://lore.kernel.org/linux-mm/[email protected]/
---
  include/linux/huge_mm.h | 11 +++++++++++
  mm/huge_memory.c        | 13 +++++++++----
  mm/shmem.c              |  9 +--------
  3 files changed, 21 insertions(+), 12 deletions(-)
include/linux/huge_mm.h +4 -0
index 212cca384d7e..f87136f38aa1 100644
@@ -267,6 +267,10 @@ unsigned long thp_vma_allowable_orders(struct vm_area_struct *vma,
  	return __thp_vma_allowable_orders(vma, vm_flags, tva_flags, orders);
  }
 
+unsigned long shmem_allowable_huge_orders(struct inode *inode,
+				struct vm_area_struct *vma, pgoff_t index,
+				bool global_huge);
+
  struct thpsize {
  	struct kobject kobj;
  	struct list_head node;

include/linux/huge_mm.h static inline unsigned long thp_vma_allowable_orders(struct vm_area_struct *vma, · line 472

Quoted 9 lines

include/linux/huge_mm.h +6 -0

@@ -460,6 +464,13 @@ static inline unsigned long thp_vma_allowable_orders(struct vm_area_struct *vma,
  	return 0;
  }
 
+static inline unsigned long shmem_allowable_huge_orders(struct inode *inode,
+				struct vm_area_struct *vma, pgoff_t index,
+				bool global_huge)
+{
+	return 0;
+}

This function should be placed in the ‘shmem_fs.h’ header file, just like shmem_is_huge().

Bang Li

author

2024-07-03 07:31 UTC #2 lore raw

Hi Baolin,

Quoted 45 lines · Baolin Wang

Quoted 42 lines · Bang Li
After the commit 7fb1b252afb5 ("mm: shmem: add mTHP support for anonymous shmem"), we can configure different policies through the multi-size THP sysfs interface for anonymous shmem. But currently "THPeligible" indicates only whether the mapping is eligible for allocating THP-pages as well as the THP is PMD mappable or not for anonymous shmem, we need to support semantics for mTHP with anonymous shmem similar to those for mTHP with anonymous memory.
Signed-off-by Bang Li <[email protected]>
---
Changes since v1 [1]:
- Put anonymous shmem mthp related logic into
    thp_vma_allowable_orders() (per David)
[1] https://lore.kernel.org/linux-mm/[email protected]/
---
include/linux/huge_mm.h | 11 +++++++++++
mm/huge_memory.c        | 13 +++++++++----
mm/shmem.c              | 9 +--------
  3 files changed, 21 insertions(+), 12 deletions(-)
include/linux/huge_mm.h +5 -0
index 212cca384d7e..f87136f38aa1 100644
@@ -267,6 +267,10 @@ unsigned long thp_vma_allowable_orders(struct 
 vm_area_struct *vma,
       return __thp_vma_allowable_orders(vma, vm_flags, tva_flags, orders);
   }
+unsigned long shmem_allowable_huge_orders(struct inode *inode,
+                struct vm_area_struct *vma, pgoff_t index,
+                bool global_huge);
+
   struct thpsize {
       struct kobject kobj;
       struct list_head node;
@@ -460,6 +464,13 @@ static inline unsigned long 
 thp_vma_allowable_orders(struct vm_area_struct *vma,
       return 0;
   }
+static inline unsigned long shmem_allowable_huge_orders(struct inode 

Quoted 8 lines

include/linux/huge_mm.h static inline unsigned long thp_vma_allowable_orders(struct vm_area_struct *vma, · line 472
Quoted 6 lines
include/linux/huge_mm.h +5 -0
 *inode,
+                struct vm_area_struct *vma, pgoff_t index,
+                bool global_huge)
+{
+    return 0;
+}
This function should be placed in the ‘shmem_fs.h’ header file, just like shmem_is_huge().

Looks more reasonable，thank you for your review.

Thanks,
Bang

Ryan Roberts

reviewer

2024-07-02 08:18 UTC #3 lore raw

In reply to Bang Li

Quoted 59 lines · Bang Li

After the commit 7fb1b252afb5 ("mm: shmem: add mTHP support for anonymous shmem"), we can configure different policies through the multi-size THP sysfs interface for anonymous shmem. But currently "THPeligible" indicates only whether the mapping is eligible for allocating THP-pages as well as the THP is PMD mappable or not for anonymous shmem, we need to support semantics for mTHP with anonymous shmem similar to those for mTHP with anonymous memory.
Signed-off-by Bang Li <[email protected]>
---
Changes since v1 [1]:
- Put anonymous shmem mthp related logic into
thp_vma_allowable_orders() (per David)
[1] https://lore.kernel.org/linux-mm/[email protected]/
---
 include/linux/huge_mm.h | 11 +++++++++++
 mm/huge_memory.c        | 13 +++++++++----
 mm/shmem.c              |  9 +--------
 3 files changed, 21 insertions(+), 12 deletions(-)
include/linux/huge_mm.h +11 -0
index 212cca384d7e..f87136f38aa1 100644
@@ -267,6 +267,10 @@ unsigned long thp_vma_allowable_orders(struct vm_area_struct *vma,
 	return __thp_vma_allowable_orders(vma, vm_flags, tva_flags, orders);
 }
 
+unsigned long shmem_allowable_huge_orders(struct inode *inode,
+				struct vm_area_struct *vma, pgoff_t index,
+				bool global_huge);
+
 struct thpsize {
 	struct kobject kobj;
 	struct list_head node;
@@ -460,6 +464,13 @@ static inline unsigned long thp_vma_allowable_orders(struct vm_area_struct *vma,
 	return 0;
 }
 
+static inline unsigned long shmem_allowable_huge_orders(struct inode *inode,
+				struct vm_area_struct *vma, pgoff_t index,
+				bool global_huge)
+{
+	return 0;
+}
+
 #define transparent_hugepage_flags 0UL
 
 #define thp_get_unmapped_area	NULL
mm/huge_memory.c +0 -0
index c7ce28f6b7f3..ea377bb4af91 100644
@@ -151,10 +151,15 @@ unsigned long __thp_vma_allowable_orders(struct vm_area_struct *vma,
 	 * Must be done before hugepage flags check since shmem has its
 	 * own flags.
 	 */

mm/huge_memory.c unsigned long __thp_vma_allowable_orders(struct vm_area_struct *vma, · line 159

Quoted 10 lines

mm/huge_memory.c +6 -4

-	if (!in_pf && shmem_file(vma->vm_file))
-		return shmem_is_huge(file_inode(vma->vm_file), vma->vm_pgoff,
-				     !enforce_sysfs, vma->vm_mm, vm_flags)
-			? orders : 0;
+	if (!in_pf && shmem_file(vma->vm_file)) {
+		bool global_huge = shmem_is_huge(file_inode(vma->vm_file), vma->vm_pgoff,
+							!enforce_sysfs, vma->vm_mm, vm_flags);
+
+		if (!vma_is_anon_shmem(vma))
+			return global_huge? orders : 0;

nit: missing space before '?'

mm/huge_memory.c unsigned long __thp_vma_allowable_orders(struct vm_area_struct *vma, · line 161

Quoted 2 lines

mm/huge_memory.c +2 -0

+		return shmem_allowable_huge_orders(file_inode(vma->vm_file),
+							vma, vma->vm_pgoff, global_huge);

What's the rationale for splitting these functions into shmem_is_huge() and shmem_allowable_huge_orders()? Why not just have a single shmem_allowable_huge_orders() that tells you the answer?

Quoted 31 lines

mm/huge_memory.c +1 -0

+	}
 
 	if (!vma_is_anonymous(vma)) {
 		/*

mm/shmem.c +1 -8

index d495c0701a83..aa85df9c662a 100644
@@ -1622,7 +1622,7 @@ static gfp_t limit_gfp_mask(gfp_t huge_gfp, gfp_t limit_gfp)
 }
 
 #ifdef CONFIG_TRANSPARENT_HUGEPAGE
-static unsigned long shmem_allowable_huge_orders(struct inode *inode,
+unsigned long shmem_allowable_huge_orders(struct inode *inode,
 				struct vm_area_struct *vma, pgoff_t index,
 				bool global_huge)
 {

@@ -1707,13 +1707,6 @@ static unsigned long shmem_suitable_orders(struct inode *inode, struct vm_fault
 	return orders;
 }
 #else
-static unsigned long shmem_allowable_huge_orders(struct inode *inode,
-				struct vm_area_struct *vma, pgoff_t index,
-				bool global_huge)
-{
-	return 0;
-}
-
 static unsigned long shmem_suitable_orders(struct inode *inode, struct vm_fault *vmf,
 					   struct address_space *mapping, pgoff_t index,
 					   unsigned long orders)

Bang Li

author

2024-07-03 07:34 UTC #4 lore raw

Hi Ryan,

Thanks for the review!

Quoted 64 lines · Ryan Roberts

Quoted 63 lines · Bang Li
After the commit 7fb1b252afb5 ("mm: shmem: add mTHP support for anonymous shmem"), we can configure different policies through the multi-size THP sysfs interface for anonymous shmem. But currently "THPeligible" indicates only whether the mapping is eligible for allocating THP-pages as well as the THP is PMD mappable or not for anonymous shmem, we need to support semantics for mTHP with anonymous shmem similar to those for mTHP with anonymous memory.
Signed-off-by Bang Li <[email protected]>
---
Changes since v1 [1]:
- Put anonymous shmem mthp related logic into
thp_vma_allowable_orders() (per David)
[1] https://lore.kernel.org/linux-mm/[email protected]/
---
  include/linux/huge_mm.h | 11 +++++++++++
  mm/huge_memory.c        | 13 +++++++++----
  mm/shmem.c              |  9 +--------
  3 files changed, 21 insertions(+), 12 deletions(-)
include/linux/huge_mm.h +11 -0
index 212cca384d7e..f87136f38aa1 100644
@@ -267,6 +267,10 @@ unsigned long thp_vma_allowable_orders(struct vm_area_struct *vma,
  	return __thp_vma_allowable_orders(vma, vm_flags, tva_flags, orders);
  }
 
+unsigned long shmem_allowable_huge_orders(struct inode *inode,
+				struct vm_area_struct *vma, pgoff_t index,
+				bool global_huge);
+
  struct thpsize {
  	struct kobject kobj;
  	struct list_head node;
@@ -460,6 +464,13 @@ static inline unsigned long thp_vma_allowable_orders(struct vm_area_struct *vma,
  	return 0;
  }
 
+static inline unsigned long shmem_allowable_huge_orders(struct inode *inode,
+				struct vm_area_struct *vma, pgoff_t index,
+				bool global_huge)
+{
+	return 0;
+}
+
  #define transparent_hugepage_flags 0UL
 
  #define thp_get_unmapped_area	NULL
mm/huge_memory.c +0 -4
index c7ce28f6b7f3..ea377bb4af91 100644
@@ -151,10 +151,15 @@ unsigned long __thp_vma_allowable_orders(struct vm_area_struct *vma,
  	 * Must be done before hugepage flags check since shmem has its
  	 * own flags.
  	 */
-	if (!in_pf && shmem_file(vma->vm_file))
-		return shmem_is_huge(file_inode(vma->vm_file), vma->vm_pgoff,
-				     !enforce_sysfs, vma->vm_mm, vm_flags)
-			? orders : 0;

Quoted 8 lines

mm/huge_memory.c unsigned long __thp_vma_allowable_orders(struct vm_area_struct *vma, · line 159
Quoted 6 lines
mm/huge_memory.c +6 -0
+	if (!in_pf && shmem_file(vma->vm_file)) {
+		bool global_huge = shmem_is_huge(file_inode(vma->vm_file), vma->vm_pgoff,
+							!enforce_sysfs, vma->vm_mm, vm_flags);
+
+		if (!vma_is_anon_shmem(vma))
+			return global_huge? orders : 0;
nit: missing space before '?'

Yes, thanks.

Quoted 8 lines

mm/huge_memory.c unsigned long __thp_vma_allowable_orders(struct vm_area_struct *vma, · line 161
Quoted 3 lines
mm/huge_memory.c +2 -0
+		return shmem_allowable_huge_orders(file_inode(vma->vm_file),
+							vma, vma->vm_pgoff, global_huge);
What's the rationale for splitting these functions into shmem_is_huge() and shmem_allowable_huge_orders()? Why not just have a single shmem_allowable_huge_orders() that tells you the answer?

Currently, shmem_is_huge() is used for all shmem implementations to determine whether the conditions for using THP are met. And shmem_allowable_huge_orders() is currently mainly used for anonymous shmem's mTHP to obtain all orders that meet the conditions. In my opinion, there is no problem in separating these two functions. In the future, when mTHP of other shmem types is also implemented, will shmem_is_huge() be unnecessary?

Thanks,
Bang

Quoted 31 lines

mm/huge_memory.c +1 -0

+	}
 
  	if (!vma_is_anonymous(vma)) {
  		/*

mm/shmem.c +1 -8

index d495c0701a83..aa85df9c662a 100644
@@ -1622,7 +1622,7 @@ static gfp_t limit_gfp_mask(gfp_t huge_gfp, gfp_t limit_gfp)
  }
 
  #ifdef CONFIG_TRANSPARENT_HUGEPAGE
-static unsigned long shmem_allowable_huge_orders(struct inode *inode,
+unsigned long shmem_allowable_huge_orders(struct inode *inode,
  				struct vm_area_struct *vma, pgoff_t index,
  				bool global_huge)
  {

@@ -1707,13 +1707,6 @@ static unsigned long shmem_suitable_orders(struct inode *inode, struct vm_fault
  	return orders;
  }
  #else
-static unsigned long shmem_allowable_huge_orders(struct inode *inode,
-				struct vm_area_struct *vma, pgoff_t index,
-				bool global_huge)
-{
-	return 0;
-}
-
  static unsigned long shmem_suitable_orders(struct inode *inode, struct vm_fault *vmf,
  					   struct address_space *mapping, pgoff_t index,
  					   unsigned long orders)

Ryan Roberts

reviewer

2024-07-03 10:25 UTC #5 lore raw

Quoted 91 lines · Bang Li

Hi Ryan,
Thanks for the review!
Quoted 66 lines · Ryan Roberts
Quoted 65 lines · Bang Li
After the commit 7fb1b252afb5 ("mm: shmem: add mTHP support for anonymous shmem"), we can configure different policies through the multi-size THP sysfs interface for anonymous shmem. But currently "THPeligible" indicates only whether the mapping is eligible for allocating THP-pages as well as the THP is PMD mappable or not for anonymous shmem, we need to support semantics for mTHP with anonymous shmem similar to those for mTHP with anonymous memory.
Signed-off-by Bang Li <[email protected]>
---
Changes since v1 [1]:
- Put anonymous shmem mthp related logic into
    thp_vma_allowable_orders() (per David)
[1]
https://lore.kernel.org/linux-mm/[email protected]/
---
include/linux/huge_mm.h | 11 +++++++++++
mm/huge_memory.c        | 13 +++++++++----
mm/shmem.c              | 9 +--------
  3 files changed, 21 insertions(+), 12 deletions(-)
include/linux/huge_mm.h +3 -0
index 212cca384d7e..f87136f38aa1 100644
@@ -267,6 +267,10 @@ unsigned long thp_vma_allowable_orders(struct
 vm_area_struct *vma,
       return __thp_vma_allowable_orders(vma, vm_flags, tva_flags, orders);
   }
   +unsigned long shmem_allowable_huge_orders(struct inode *inode,
+                struct vm_area_struct *vma, pgoff_t index,
+                bool global_huge);
+
   struct thpsize {
       struct kobject kobj;
struct list_head node;
include/linux/huge_mm.h +6 -0
@@ -460,6 +464,13 @@ static inline unsigned long
 thp_vma_allowable_orders(struct vm_area_struct *vma,
       return 0;
   }
   +static inline unsigned long shmem_allowable_huge_orders(struct inode *inode,
+                struct vm_area_struct *vma, pgoff_t index,
+                bool global_huge)
+{
+    return 0;
+}
+
   #define transparent_hugepage_flags 0UL
     #define thp_get_unmapped_area    NULL
mm/huge_memory.c +1 -4
index c7ce28f6b7f3..ea377bb4af91 100644
@@ -151,10 +151,15 @@ unsigned long __thp_vma_allowable_orders(struct
 vm_area_struct *vma,
        * Must be done before hugepage flags check since shmem has its
        * own flags.
        */
-    if (!in_pf && shmem_file(vma->vm_file))
-        return shmem_is_huge(file_inode(vma->vm_file), vma->vm_pgoff,
-                     !enforce_sysfs, vma->vm_mm, vm_flags)
-            ? orders : 0;
+    if (!in_pf && shmem_file(vma->vm_file)) {
Quoted 8 lines
mm/huge_memory.c unsigned long __thp_vma_allowable_orders(struct vm_area_struct *vma, · line 159
Quoted 6 lines
mm/huge_memory.c +5 -0
+        bool global_huge = shmem_is_huge(file_inode(vma->vm_file),
 vma->vm_pgoff,
+                            !enforce_sysfs, vma->vm_mm, vm_flags);
+
+        if (!vma_is_anon_shmem(vma))
+            return global_huge? orders : 0;
nit: missing space before '?'
Yes, thanks.
Quoted 8 lines
mm/huge_memory.c unsigned long __thp_vma_allowable_orders(struct vm_area_struct *vma, · line 161
Quoted 3 lines
mm/huge_memory.c +2 -0
+        return shmem_allowable_huge_orders(file_inode(vma->vm_file),
+                            vma, vma->vm_pgoff, global_huge);
What's the rationale for splitting these functions into shmem_is_huge() and shmem_allowable_huge_orders()? Why not just have a single shmem_allowable_huge_orders() that tells you the answer?

Quoted 6 lines

Currently, shmem_is_huge() is used for all shmem implementations to determine whether the conditions for using THP are met. And shmem_allowable_huge_orders() is currently mainly used for anonymous shmem's mTHP to obtain all orders that meet the conditions. In my opinion, there is no problem in separating these two functions. In the future, when mTHP of other shmem types is also implemented, will shmem_is_huge() be unnecessary?

Personally, I consider shmem_is_huge() to be superfluous; If you have one function, shmem_allowable_huge_orders(), that gives you all the information you need. If the inode only allows PMD-size, then only return that bit in the field. IMHO removing shmem_is_huge() would make things more readable.

Thanks,
Ryan

Quoted 36 lines

Thanks,
Bang

Quoted 32 lines

mm/huge_memory.c +1 -0

+    }
         if (!vma_is_anonymous(vma)) {

mm/shmem.c +1 -1

index d495c0701a83..aa85df9c662a 100644
@@ -1622,7 +1622,7 @@ static gfp_t limit_gfp_mask(gfp_t huge_gfp, gfp_t
 limit_gfp)
   }
     #ifdef CONFIG_TRANSPARENT_HUGEPAGE
-static unsigned long shmem_allowable_huge_orders(struct inode *inode,
+unsigned long shmem_allowable_huge_orders(struct inode *inode,
                   struct vm_area_struct *vma, pgoff_t index,
                   bool global_huge)
   {

@@ -1707,13 +1707,6 @@ static unsigned long shmem_suitable_orders(struct

inode *inode, struct vm_fault
return orders;
}
#else

mm/shmem.c +0 -7

-static unsigned long shmem_allowable_huge_orders(struct inode *inode,
-                struct vm_area_struct *vma, pgoff_t index,
-                bool global_huge)
-{
-    return 0;
-}
-

static unsigned long shmem_suitable_orders(struct inode *inode, struct vm_fault *vmf,
struct address_space *mapping, pgoff_t index, unsigned long orders)

Bang Li

author

2024-07-03 16:02 UTC #6 lore raw

Quoted 99 lines · Ryan Roberts

Quoted 91 lines · Bang Li
Hi Ryan,
Thanks for the review!
Quoted 66 lines · Ryan Roberts
Quoted 65 lines · Bang Li
After the commit 7fb1b252afb5 ("mm: shmem: add mTHP support for anonymous shmem"), we can configure different policies through the multi-size THP sysfs interface for anonymous shmem. But currently "THPeligible" indicates only whether the mapping is eligible for allocating THP-pages as well as the THP is PMD mappable or not for anonymous shmem, we need to support semantics for mTHP with anonymous shmem similar to those for mTHP with anonymous memory.
Signed-off-by Bang Li <[email protected]>
---
Changes since v1 [1]:
- Put anonymous shmem mthp related logic into
thp_vma_allowable_orders() (per David)
[1]
https://lore.kernel.org/linux-mm/[email protected]/
---
   include/linux/huge_mm.h | 11 +++++++++++
   mm/huge_memory.c        | 13 +++++++++----
   mm/shmem.c              |  9 +--------
   3 files changed, 21 insertions(+), 12 deletions(-)
include/linux/huge_mm.h +9 -0
index 212cca384d7e..f87136f38aa1 100644
@@ -267,6 +267,10 @@ unsigned long thp_vma_allowable_orders(struct
 vm_area_struct *vma,
       return __thp_vma_allowable_orders(vma, vm_flags, tva_flags, orders);
   }
   +unsigned long shmem_allowable_huge_orders(struct inode *inode,
+                struct vm_area_struct *vma, pgoff_t index,
+                bool global_huge);
+
   struct thpsize {
       struct kobject kobj;
       struct list_head node;
@@ -460,6 +464,13 @@ static inline unsigned long
 thp_vma_allowable_orders(struct vm_area_struct *vma,
       return 0;
   }
   +static inline unsigned long shmem_allowable_huge_orders(struct inode *inode,
+                struct vm_area_struct *vma, pgoff_t index,
+                bool global_huge)
+{
+    return 0;
+}
+
   #define transparent_hugepage_flags 0UL
     #define thp_get_unmapped_area    NULL
mm/huge_memory.c +1 -4
index c7ce28f6b7f3..ea377bb4af91 100644
@@ -151,10 +151,15 @@ unsigned long __thp_vma_allowable_orders(struct
 vm_area_struct *vma,
        * Must be done before hugepage flags check since shmem has its
        * own flags.
        */
-    if (!in_pf && shmem_file(vma->vm_file))
-        return shmem_is_huge(file_inode(vma->vm_file), vma->vm_pgoff,
-                     !enforce_sysfs, vma->vm_mm, vm_flags)
-            ? orders : 0;
+    if (!in_pf && shmem_file(vma->vm_file)) {
Quoted 8 lines
mm/huge_memory.c unsigned long __thp_vma_allowable_orders(struct vm_area_struct *vma, · line 159
Quoted 6 lines
mm/huge_memory.c +5 -0
+        bool global_huge = shmem_is_huge(file_inode(vma->vm_file),
 vma->vm_pgoff,
+                            !enforce_sysfs, vma->vm_mm, vm_flags);
+
+        if (!vma_is_anon_shmem(vma))
+            return global_huge? orders : 0;
nit: missing space before '?'
Yes, thanks.
Quoted 8 lines
mm/huge_memory.c unsigned long __thp_vma_allowable_orders(struct vm_area_struct *vma, · line 161
Quoted 3 lines
mm/huge_memory.c +2 -0
+        return shmem_allowable_huge_orders(file_inode(vma->vm_file),
+                            vma, vma->vm_pgoff, global_huge);
What's the rationale for splitting these functions into shmem_is_huge() and shmem_allowable_huge_orders()? Why not just have a single shmem_allowable_huge_orders() that tells you the answer?
Quoted 6 lines
Currently, shmem_is_huge() is used for all shmem implementations to determine whether the conditions for using THP are met. And shmem_allowable_huge_orders() is currently mainly used for anonymous shmem's mTHP to obtain all orders that meet the conditions. In my opinion, there is no problem in separating these two functions. In the future, when mTHP of other shmem types is also implemented, will shmem_is_huge() be unnecessary?

Quoted 4 lines

Personally, I consider shmem_is_huge() to be superfluous; If you have one function, shmem_allowable_huge_orders(), that gives you all the information you need. If the inode only allows PMD-size, then only return that bit in the field. IMHO removing shmem_is_huge() would make things more readable.

Thank you very much for your opinion. I think there is no big problem in keeping the current interface status quo. In my opinion, when we implement mTHP for all shmems, it may be better to remove shmem_is_huge() and use shmem_allowable_huge_orders() uniformly. I believe that it won't be long before someone proposes a non-anonymous shared mTHP patchset. Anyway, thank you for your suggestion :).

Thanks,
Bang

Quoted 40 lines

Thanks,
Ryan

Quoted 36 lines

Thanks,
Bang

Quoted 32 lines

mm/huge_memory.c +1 -0

+    }
         if (!vma_is_anonymous(vma)) {

mm/shmem.c +1 -1

index d495c0701a83..aa85df9c662a 100644
@@ -1622,7 +1622,7 @@ static gfp_t limit_gfp_mask(gfp_t huge_gfp, gfp_t
 limit_gfp)
   }
     #ifdef CONFIG_TRANSPARENT_HUGEPAGE
-static unsigned long shmem_allowable_huge_orders(struct inode *inode,
+unsigned long shmem_allowable_huge_orders(struct inode *inode,
                   struct vm_area_struct *vma, pgoff_t index,
                   bool global_huge)
   {

@@ -1707,13 +1707,6 @@ static unsigned long shmem_suitable_orders(struct

inode *inode, struct vm_fault
return orders;
}
#else

mm/shmem.c +0 -7

-static unsigned long shmem_allowable_huge_orders(struct inode *inode,
-                struct vm_area_struct *vma, pgoff_t index,
-                bool global_huge)
-{
-    return 0;
-}
-
   static unsigned long shmem_suitable_orders(struct inode *inode, struct

vm_fault *vmf,
struct address_space *mapping, pgoff_t index, unsigned long orders)

Ryan Roberts

reviewer

2024-07-04 09:46 UTC #7 lore raw

Quoted 106 lines · Bang Li

Quoted 100 lines · Ryan Roberts
Quoted 92 lines · Bang Li
Hi Ryan,
Thanks for the review!
Quoted 67 lines · Ryan Roberts
Quoted 66 lines · Bang Li
After the commit 7fb1b252afb5 ("mm: shmem: add mTHP support for anonymous shmem"), we can configure different policies through the multi-size THP sysfs interface for anonymous shmem. But currently "THPeligible" indicates only whether the mapping is eligible for allocating THP-pages as well as the THP is PMD mappable or not for anonymous shmem, we need to support semantics for mTHP with anonymous shmem similar to those for mTHP with anonymous memory.
Signed-off-by Bang Li <[email protected]>
---
Changes since v1 [1]:
   - Put anonymous shmem mthp related logic into
     thp_vma_allowable_orders() (per David)
[1]
https://lore.kernel.org/linux-mm/[email protected]/
---
   include/linux/huge_mm.h | 11 +++++++++++
   mm/huge_memory.c        | 13 +++++++++----
   mm/shmem.c              | 9 +--------
   3 files changed, 21 insertions(+), 12 deletions(-)
include/linux/huge_mm.h +3 -0
index 212cca384d7e..f87136f38aa1 100644
@@ -267,6 +267,10 @@ unsigned long thp_vma_allowable_orders(struct
 vm_area_struct *vma,
        return __thp_vma_allowable_orders(vma, vm_flags, tva_flags, orders);
    }
    +unsigned long shmem_allowable_huge_orders(struct inode *inode,
+                struct vm_area_struct *vma, pgoff_t index,
+                bool global_huge);
+
    struct thpsize {
        struct kobject kobj;
struct list_head node;
include/linux/huge_mm.h +0 -0
@@ -460,6 +464,13 @@ static inline unsigned long
 thp_vma_allowable_orders(struct vm_area_struct *vma,
        return 0;
    }
+static inline unsigned long shmem_allowable_huge_orders(struct inode *inode,
+ struct vm_area_struct *vma, pgoff_t index, + bool global_huge)
include/linux/huge_mm.h +4 -0
+{
+    return 0;
+}
+
#define transparent_hugepage_flags 0UL
#define thp_get_unmapped_area NULL
mm/huge_memory.c +1 -4
index c7ce28f6b7f3..ea377bb4af91 100644
@@ -151,10 +151,15 @@ unsigned long __thp_vma_allowable_orders(struct
 vm_area_struct *vma,
         * Must be done before hugepage flags check since shmem has its
         * own flags.
         */
-    if (!in_pf && shmem_file(vma->vm_file))
-        return shmem_is_huge(file_inode(vma->vm_file), vma->vm_pgoff,
-                     !enforce_sysfs, vma->vm_mm, vm_flags)
-            ? orders : 0;
+    if (!in_pf && shmem_file(vma->vm_file)) {
Quoted 8 lines
mm/huge_memory.c unsigned long __thp_vma_allowable_orders(struct vm_area_struct *vma, · line 159
Quoted 6 lines
mm/huge_memory.c +5 -0
+        bool global_huge = shmem_is_huge(file_inode(vma->vm_file),
 vma->vm_pgoff,
+                            !enforce_sysfs, vma->vm_mm, vm_flags);
+
+        if (!vma_is_anon_shmem(vma))
+            return global_huge? orders : 0;
nit: missing space before '?'
Yes, thanks.
Quoted 8 lines
mm/huge_memory.c unsigned long __thp_vma_allowable_orders(struct vm_area_struct *vma, · line 161
Quoted 3 lines
mm/huge_memory.c +2 -0
+        return shmem_allowable_huge_orders(file_inode(vma->vm_file),
+                            vma, vma->vm_pgoff, global_huge);
What's the rationale for splitting these functions into shmem_is_huge() and shmem_allowable_huge_orders()? Why not just have a single shmem_allowable_huge_orders() that tells you the answer?
Quoted 6 lines
Currently, shmem_is_huge() is used for all shmem implementations to determine whether the conditions for using THP are met. And shmem_allowable_huge_orders() is currently mainly used for anonymous shmem's mTHP to obtain all orders that meet the conditions. In my opinion, there is no problem in separating these two functions. In the future, when mTHP of other shmem types is also implemented, will shmem_is_huge() be unnecessary?
Quoted 4 lines
Personally, I consider shmem_is_huge() to be superfluous; If you have one function, shmem_allowable_huge_orders(), that gives you all the information you need. If the inode only allows PMD-size, then only return that bit in the field. IMHO removing shmem_is_huge() would make things more readable.

Quoted 6 lines

Thank you very much for your opinion. I think there is no big problem in keeping the current interface status quo. In my opinion, when we implement mTHP for all shmems, it may be better to remove shmem_is_huge() and use shmem_allowable_huge_orders() uniformly. I believe that it won't be long before someone proposes a non-anonymous shared mTHP patchset. Anyway, thank you for your suggestion :).

See [1] for an expanded list of concerns I have about the current state of the interface and implementation.

Thanks,
Ryan

Quoted 44 lines

Thanks,
Bang

Quoted 40 lines

Thanks,
Ryan

Quoted 36 lines

Thanks,
Bang

Quoted 32 lines

mm/huge_memory.c +1 -0

+    }
          if (!vma_is_anonymous(vma)) {

mm/shmem.c +1 -1

index d495c0701a83..aa85df9c662a 100644
@@ -1622,7 +1622,7 @@ static gfp_t limit_gfp_mask(gfp_t huge_gfp, gfp_t
 limit_gfp)
    }
      #ifdef CONFIG_TRANSPARENT_HUGEPAGE
-static unsigned long shmem_allowable_huge_orders(struct inode *inode,
+unsigned long shmem_allowable_huge_orders(struct inode *inode,
                    struct vm_area_struct *vma, pgoff_t index,
                    bool global_huge)
    {

@@ -1707,13 +1707,6 @@ static unsigned long shmem_suitable_orders(struct

inode *inode, struct vm_fault
       return orders;
   }
   #else

mm/shmem.c +0 -7

-static unsigned long shmem_allowable_huge_orders(struct inode *inode,
-                struct vm_area_struct *vma, pgoff_t index,
-                bool global_huge)
-{
-    return 0;
-}
-

static unsigned long shmem_suitable_orders(struct inode *inode, struct vm_fault *vmf,
struct address_space *mapping, pgoff_t index, unsigned long orders)

Bang Li

author

2024-07-04 10:05 UTC #8 lore raw

Hi Ryan,

Quoted 114 lines · Ryan Roberts

Quoted 106 lines · Bang Li
Quoted 100 lines · Ryan Roberts
Quoted 92 lines · Bang Li
Hi Ryan,
Thanks for the review!
Quoted 67 lines · Ryan Roberts
Quoted 66 lines · Bang Li
After the commit 7fb1b252afb5 ("mm: shmem: add mTHP support for anonymous shmem"), we can configure different policies through the multi-size THP sysfs interface for anonymous shmem. But currently "THPeligible" indicates only whether the mapping is eligible for allocating THP-pages as well as the THP is PMD mappable or not for anonymous shmem, we need to support semantics for mTHP with anonymous shmem similar to those for mTHP with anonymous memory.
Signed-off-by Bang Li <[email protected]>
---
Changes since v1 [1]:
- Put anonymous shmem mthp related logic into
thp_vma_allowable_orders() (per David)
[1]
https://lore.kernel.org/linux-mm/[email protected]/
---
    include/linux/huge_mm.h | 11 +++++++++++
    mm/huge_memory.c        | 13 +++++++++----
    mm/shmem.c              |  9 +--------
    3 files changed, 21 insertions(+), 12 deletions(-)
include/linux/huge_mm.h +9 -0
index 212cca384d7e..f87136f38aa1 100644
@@ -267,6 +267,10 @@ unsigned long thp_vma_allowable_orders(struct
 vm_area_struct *vma,
        return __thp_vma_allowable_orders(vma, vm_flags, tva_flags, orders);
    }
    +unsigned long shmem_allowable_huge_orders(struct inode *inode,
+                struct vm_area_struct *vma, pgoff_t index,
+                bool global_huge);
+
    struct thpsize {
        struct kobject kobj;
        struct list_head node;
@@ -460,6 +464,13 @@ static inline unsigned long
 thp_vma_allowable_orders(struct vm_area_struct *vma,
        return 0;
    }
    +static inline unsigned long shmem_allowable_huge_orders(struct inode
 *inode,
+                struct vm_area_struct *vma, pgoff_t index,
+                bool global_huge)
+{
+    return 0;
+}
+
    #define transparent_hugepage_flags 0UL
      #define thp_get_unmapped_area    NULL
mm/huge_memory.c +1 -4
index c7ce28f6b7f3..ea377bb4af91 100644
@@ -151,10 +151,15 @@ unsigned long __thp_vma_allowable_orders(struct
 vm_area_struct *vma,
         * Must be done before hugepage flags check since shmem has its
         * own flags.
         */
-    if (!in_pf && shmem_file(vma->vm_file))
-        return shmem_is_huge(file_inode(vma->vm_file), vma->vm_pgoff,
-                     !enforce_sysfs, vma->vm_mm, vm_flags)
-            ? orders : 0;
+    if (!in_pf && shmem_file(vma->vm_file)) {
Quoted 8 lines
mm/huge_memory.c unsigned long __thp_vma_allowable_orders(struct vm_area_struct *vma, · line 159
Quoted 6 lines
mm/huge_memory.c +5 -0
+        bool global_huge = shmem_is_huge(file_inode(vma->vm_file),
 vma->vm_pgoff,
+                            !enforce_sysfs, vma->vm_mm, vm_flags);
+
+        if (!vma_is_anon_shmem(vma))
+            return global_huge? orders : 0;
nit: missing space before '?'
Yes, thanks.
Quoted 8 lines
mm/huge_memory.c unsigned long __thp_vma_allowable_orders(struct vm_area_struct *vma, · line 161
Quoted 3 lines
mm/huge_memory.c +2 -0
+        return shmem_allowable_huge_orders(file_inode(vma->vm_file),
+                            vma, vma->vm_pgoff, global_huge);
What's the rationale for splitting these functions into shmem_is_huge() and shmem_allowable_huge_orders()? Why not just have a single shmem_allowable_huge_orders() that tells you the answer?
Quoted 6 lines
Currently, shmem_is_huge() is used for all shmem implementations to determine whether the conditions for using THP are met. And shmem_allowable_huge_orders() is currently mainly used for anonymous shmem's mTHP to obtain all orders that meet the conditions. In my opinion, there is no problem in separating these two functions. In the future, when mTHP of other shmem types is also implemented, will shmem_is_huge() be unnecessary?
Quoted 4 lines
Personally, I consider shmem_is_huge() to be superfluous; If you have one function, shmem_allowable_huge_orders(), that gives you all the information you need. If the inode only allows PMD-size, then only return that bit in the field. IMHO removing shmem_is_huge() would make things more readable.
Quoted 6 lines
Thank you very much for your opinion. I think there is no big problem in keeping the current interface status quo. In my opinion, when we implement mTHP for all shmems, it may be better to remove shmem_is_huge() and use shmem_allowable_huge_orders() uniformly. I believe that it won't be long before someone proposes a non-anonymous shared mTHP patchset. Anyway, thank you for your suggestion :).

Quoted 2 lines

See [1] for an expanded list of concerns I have about the current state of the interface and implementation.

OK, could you please send me the corresponding link ;)？

Thanks，
Bang

Quoted 48 lines

Thanks,
Ryan

Quoted 44 lines

Thanks,
Bang

Quoted 40 lines

Thanks,
Ryan

Quoted 36 lines

Thanks,
Bang

Quoted 32 lines

mm/huge_memory.c +1 -0

+    }
          if (!vma_is_anonymous(vma)) {

mm/shmem.c +1 -1

index d495c0701a83..aa85df9c662a 100644
@@ -1622,7 +1622,7 @@ static gfp_t limit_gfp_mask(gfp_t huge_gfp, gfp_t
 limit_gfp)
    }
      #ifdef CONFIG_TRANSPARENT_HUGEPAGE
-static unsigned long shmem_allowable_huge_orders(struct inode *inode,
+unsigned long shmem_allowable_huge_orders(struct inode *inode,
                    struct vm_area_struct *vma, pgoff_t index,
                    bool global_huge)
    {

@@ -1707,13 +1707,6 @@ static unsigned long shmem_suitable_orders(struct

inode *inode, struct vm_fault
       return orders;
   }
   #else

mm/shmem.c +0 -7

-static unsigned long shmem_allowable_huge_orders(struct inode *inode,
-                struct vm_area_struct *vma, pgoff_t index,
-                bool global_huge)
-{
-    return 0;
-}
-
    static unsigned long shmem_suitable_orders(struct inode *inode, struct

vm_fault *vmf,
struct address_space *mapping, pgoff_t index, unsigned long orders)

Ryan Roberts

reviewer

2024-07-04 10:25 UTC #9 lore raw

Quoted 5 lines

Quoted 3 lines
See [1] for an expanded list of concerns I have about the current state of the interface and implementation.
OK, could you please send me the corresponding link ;)？

Oops!

https://lore.kernel.org/linux-mm/[email protected]/

Bang Li

author

2024-07-04 12:03 UTC #10 lore raw

Quoted 9 lines · Ryan Roberts

Quoted 5 lines
Quoted 3 lines
See [1] for an expanded list of concerns I have about the current state of the interface and implementation.
OK, could you please send me the corresponding link ;)？
Oops!
https://lore.kernel.org/linux-mm/[email protected]/

Thank you for providing the link. I will review the details of your concerns.

Best regards,
Bang