Skip to contentSkip to conversation

mm, swap: Enable THP SWAP for PowerPC Book3S64

RFC Superseded Ritesh Harjani (IBM) 5 participants 11 messages Jun 9 – Jun 11

Subsystems (3)
Memory management PowerPC MM swap

v1 of 4 Superseded Latest is v4 →

lore
Share & tools
lore

Ritesh Harjani (IBM)

author reviewer
#0 lore raw

cover mm, swap: Enable THP SWAP for PowerPC Book3S64

On PowerPC Book3S64, MMU is selected at runtime, so macros like PMD_SHIFT are effectively runtime variables in the Book3S64 code. THP swap code uses these macros for e.g. to size some of its array data structures based on PMD_ORDER. This patch series makes that usage dependent on the runtime variable.

Sayali did some performance runs of this on Book3S64 with Radix and it gives 40-50% performance improvement. We also plan to run it with Hash, will soon update the results.

Note that this patch series is based out of linux-next (next-20260608).

Ritesh Harjani (IBM) (4):
include/linux/swap.h: Remove unused leftovers
mm, swap: make SWAPFILE_CLUSTER runtime
mm, swap: make SWAP_NR_ORDERS runtime
powerpc: Kconfig: Enable THP_SWAP on Book3S64

 arch/powerpc/platforms/Kconfig.cputype |   1 +
 include/linux/swap.h                   |  17 +---
 mm/swap.h                              |   5 +-
 mm/swap_table.h                        |   6 +-
 mm/swapfile.c                          | 132 ++++++++++++++++++-------
 5 files changed, 106 insertions(+), 55 deletions(-)

--
2.39.5

author reviewer
#1 lore raw

RFC 1/4 include/linux/swap.h: Remove unused leftovers

This removed unused leftovers most of them are forward structure declarations. Also removes SWAP_BATCH macro which isn't used any where in the code.

Found these during manual code review.

Signed-off-by Ritesh Harjani (IBM) <[email protected]>
---
 include/linux/swap.h | 7 -------
 1 file changed, 7 deletions(-)
include/linux/swap.h +0 -7
index 8f0f68e245ba..46c25523d7b8 100644
@@ -16,10 +16,6 @@
 #include <uapi/linux/mempolicy.h>
 #include <asm/page.h>
 
-struct notifier_block;
-
-struct bio;
-
 #define SWAP_FLAG_PREFER	0x8000	/* set if swap priority specified */
 #define SWAP_FLAG_PRIO_MASK	0x7fff
 #define SWAP_FLAG_DISCARD	0x10000 /* enable discard for swap */

@@ -29,7 +25,6 @@ struct bio;
 #define SWAP_FLAGS_VALID	(SWAP_FLAG_PRIO_MASK | SWAP_FLAG_PREFER | \
 				 SWAP_FLAG_DISCARD | SWAP_FLAG_DISCARD_ONCE | \
 				 SWAP_FLAG_DISCARD_PAGES)
-#define SWAP_BATCH 64
 
 static inline int current_is_kswapd(void)
 {

@@ -175,7 +170,6 @@ static inline void mm_account_reclaimed_pages(unsigned long pages)
 
 struct address_space;
 struct sysinfo;
-struct writeback_control;
 struct zone;
 
 /*

@@ -442,7 +436,6 @@ extern sector_t swapdev_block(int, pgoff_t);
 extern int __swap_count(swp_entry_t entry);
 extern bool swap_entry_swapped(struct swap_info_struct *si, swp_entry_t entry);
 extern int swp_swapcount(swp_entry_t entry);
-struct backing_dev_info;
 extern struct swap_info_struct *get_swap_device(swp_entry_t entry);
 sector_t swap_folio_sector(struct folio *folio);
Signature · 1 line
2.39.5
reviewer
#2 lore raw

Re: [RFC 1/4] include/linux/swap.h: Remove unused leftovers

Quoted 11 lines · Ritesh Harjani (IBM)

This removed unused leftovers most of them are forward structure declarations. Also removes SWAP_BATCH macro which isn't used any where in the code.

Found these during manual code review.

Signed-off-by Ritesh Harjani (IBM) <[email protected]>
---
 include/linux/swap.h | 7 -------
 1 file changed, 7 deletions(-)

Seems like this can be submitted independently of this patch series right?

reviewer Reviewed-by
#3 lore raw

Re: [RFC 1/4] include/linux/swap.h: Remove unused leftovers

Quoted 8 lines · Nhat Pham
Quoted 5 lines · Ritesh Harjani (IBM)

This removed unused leftovers most of them are forward structure declarations. Also removes SWAP_BATCH macro which isn't used any where in the code.

Quoted 8 lines
Quoted 6 lines

Found these during manual code review.

Signed-off-by Ritesh Harjani (IBM) <[email protected]>
---
 include/linux/swap.h | 7 -------
 1 file changed, 7 deletions(-)

Seems like this can be submitted independently of this patch series right?

+1

Applied the patch and built the kernel. It seems to work. Feel free to add:

Reviewed-by Barry Song <[email protected]>
author reviewer
#4 lore raw

Re: [RFC 1/4] include/linux/swap.h: Remove unused leftovers

Quoted 16 lines · Barry Song
Quoted 15 lines · Nhat Pham
Quoted 11 lines · Ritesh Harjani (IBM)

This removed unused leftovers most of them are forward structure declarations. Also removes SWAP_BATCH macro which isn't used any where in the code.

Found these during manual code review.

Signed-off-by Ritesh Harjani (IBM) <[email protected]>
---
 include/linux/swap.h | 7 -------
 1 file changed, 7 deletions(-)
Quoted 7 lines
Quoted 1 line

Seems like this can be submitted independently of this patch series right?

+1

Applied the patch and built the kernel. It seems to work. Feel free to add:

Reviewed-by Barry Song <[email protected]>

Sure thanks Nhat & Barry. Yes, this patch can be picked up separately and it should not cause any conflict with the rest of the patch series either.

Let me split it and will send this one out separately with Barry's RB tag.

-ritesh

maintainer Acked-by on v1 1/4
#5 lore raw

Re: [RFC 1/4] include/linux/swap.h: Remove unused leftovers

In reply to Ritesh Harjani (IBM)
Quoted 6 lines · Ritesh Harjani (IBM)

This removed unused leftovers most of them are forward structure declarations. Also removes SWAP_BATCH macro which isn't used any where in the code.

Found these during manual code review.

Thanks for the patch.

Obvious correct.

Acked-by Chris Li <[email protected]>

Chris

Quoted 48 lines
Signed-off-by Ritesh Harjani (IBM) <[email protected]>
---
 include/linux/swap.h | 7 -------
 1 file changed, 7 deletions(-)
include/linux/swap.h +0 -8
index 8f0f68e245ba..46c25523d7b8 100644
@@ -16,10 +16,6 @@
 #include <uapi/linux/mempolicy.h>
 #include <asm/page.h>
 
-struct notifier_block;
-
-struct bio;
-
 #define SWAP_FLAG_PREFER       0x8000  /* set if swap priority specified */
 #define SWAP_FLAG_PRIO_MASK    0x7fff
 #define SWAP_FLAG_DISCARD      0x10000 /* enable discard for swap */

@@ -29,7 +25,6 @@ struct bio;
 #define SWAP_FLAGS_VALID       (SWAP_FLAG_PRIO_MASK | SWAP_FLAG_PREFER | \
                                 SWAP_FLAG_DISCARD | SWAP_FLAG_DISCARD_ONCE | \
                                 SWAP_FLAG_DISCARD_PAGES)
-#define SWAP_BATCH 64
 
 static inline int current_is_kswapd(void)
 {

@@ -175,7 +170,6 @@ static inline void mm_account_reclaimed_pages(unsigned long pages)
 
 struct address_space;
 struct sysinfo;
-struct writeback_control;
 struct zone;
 
 /*

@@ -442,7 +436,6 @@ extern sector_t swapdev_block(int, pgoff_t);
 extern int __swap_count(swp_entry_t entry);
 extern bool swap_entry_swapped(struct swap_info_struct *si, swp_entry_t entry);
 extern int swp_swapcount(swp_entry_t entry);
-struct backing_dev_info;
 extern struct swap_info_struct *get_swap_device(swp_entry_t entry);
 sector_t swap_folio_sector(struct folio *folio);
 
--

2.39.5

author reviewer
#6 lore raw

RFC 2/4 mm, swap: make SWAPFILE_CLUSTER runtime

In reply to Ritesh Harjani (IBM)

This makes SWAPFILE_CLUSTER as a runtime value. Architectures like powerpc book3s64 has HPAGE_PMD_NR, which is derived at runtime depending upon which chosen mmu.
Hence this patch initializes SWAPFILE_CLUSTER at runtime and also modifies swap_table and swap_memcg_table which were earlier using this macro for defining the number of table entries.

Signed-off-by Ritesh Harjani (IBM) <[email protected]>
---
 mm/swap.h       |  5 +++--
 mm/swap_table.h |  6 ++----
 mm/swapfile.c   | 27 ++++++++++++++++++++++-----
 3 files changed, 27 insertions(+), 11 deletions(-)
mm/swap.h +3 -2
index 77d2d14eda42..956879a69ddd 100644
@@ -26,11 +26,12 @@ extern int page_cluster;
 #define SWAP_TABLE_HAS_ZEROFLAG		((BITS_PER_LONG - SWAP_CACHE_PFN_MARK_BITS - \
 					  SWAP_CACHE_PFN_BITS) > SWAP_COUNT_MIN_BITS)
 
+extern unsigned int swap_slots_in_cluster __read_mostly;
+#define SWAPFILE_CLUSTER	swap_slots_in_cluster
+
 #ifdef CONFIG_THP_SWAP
-#define SWAPFILE_CLUSTER	HPAGE_PMD_NR
 #define swap_entry_order(order)	(order)
 #else
-#define SWAPFILE_CLUSTER	256
 #define swap_entry_order(order)	0
 #endif
mm/swap_table.h +2 -4
index e6613e62f8d0..90e2a7852300 100644
@@ -8,16 +8,14 @@
 
 /* A typical flat array in each cluster as swap table */
 struct swap_table {
-	atomic_long_t entries[SWAPFILE_CLUSTER];
+	DECLARE_FLEX_ARRAY(atomic_long_t, entries);
 };
 
 /* For storing memcg private id */
 struct swap_memcg_table {
-	unsigned short id[SWAPFILE_CLUSTER];
+	DECLARE_FLEX_ARRAY(unsigned short, id);
 };
 
-#define SWP_TABLE_USE_PAGE (sizeof(struct swap_table) == PAGE_SIZE)
-
 /*
  * A swap table entry represents the status of a swap slot on a swap
  * (physical or virtual) device. The swap table in each cluster is a
mm/swapfile.c +22 -5
index 78b49b0658ad..016a5aa0cb93 100644
@@ -129,6 +129,17 @@ static DEFINE_PER_CPU(struct percpu_swap_cluster, percpu_swap_cluster) = {
 	.lock = INIT_LOCAL_LOCK(),
 };
 
+unsigned int swap_slots_in_cluster __read_mostly;
+bool swap_table_use_page __read_mostly;
+
+static unsigned int generic_swap_slots_in_clusters(void)
+{
+	if (IS_ENABLED(CONFIG_THP_SWAP))
+		return HPAGE_PMD_NR;
+	else
+		return 256;
+}
+
 /* May return NULL on invalid type, caller must check for NULL return */
 static struct swap_info_struct *swap_type_to_info(int type)
 {

@@ -437,7 +448,7 @@ static void swap_cluster_free_table(struct swap_cluster_info *ci)
 		return;
 
 	rcu_assign_pointer(ci->table, NULL);
-	if (!SWP_TABLE_USE_PAGE) {
+	if (!swap_table_use_page) {
 		kmem_cache_free(swap_table_cachep, table);
 		return;
 	}

@@ -456,7 +467,7 @@ static int swap_cluster_alloc_table(struct swap_cluster_info *ci, gfp_t gfp)
 	if (rcu_access_pointer(ci->table))
 		return 0;
 
-	if (SWP_TABLE_USE_PAGE) {
+	if (swap_table_use_page) {
 		folio = folio_alloc(gfp | __GFP_ZERO, 0);
 		if (folio)
 			table = folio_address(folio);

@@ -471,7 +482,8 @@ static int swap_cluster_alloc_table(struct swap_cluster_info *ci, gfp_t gfp)
 #ifdef CONFIG_MEMCG
 	if (!mem_cgroup_disabled()) {
 		VM_WARN_ON_ONCE(ci->memcg_table);
-		ci->memcg_table = kzalloc_obj(*ci->memcg_table, gfp);
+		ci->memcg_table = kzalloc_flex(*ci->memcg_table, id,
+					       SWAPFILE_CLUSTER, gfp);
 		if (!ci->memcg_table) {
 			swap_cluster_free_table(ci);
 			return -ENOMEM;

@@ -3912,14 +3924,19 @@ static int __init swapfile_init(void)
 {
 	swapfile_maximum_size = arch_max_swapfile_size();
 
+	swap_slots_in_cluster = generic_swap_slots_in_clusters();
+	swap_table_use_page =
+		(swap_slots_in_cluster * sizeof(atomic_long_t) == PAGE_SIZE);
+
 	/*
 	 * Once a cluster is freed, it's swap table content is read
 	 * only, and all swap cache readers (swap_cache_*) verifies
 	 * the content before use. So it's safe to use RCU slab here.
 	 */
-	if (!SWP_TABLE_USE_PAGE)
+	if (!swap_table_use_page)
 		swap_table_cachep = kmem_cache_create("swap_table",
-				    sizeof(struct swap_table),
+				    struct_size_t(struct swap_table, entries,
+					    SWAPFILE_CLUSTER),
 				    0, SLAB_PANIC | SLAB_TYPESAFE_BY_RCU, NULL);
 
 #ifdef CONFIG_MIGRATION

--
2.39.5

author reviewer
#7 lore raw

RFC 3/4 mm, swap: make SWAP_NR_ORDERS runtime

In reply to Ritesh Harjani (IBM)

SWAP_NR_ORDERS is currently a compile-time constant defined as PMD_ORDER
+ 1 when CONFIG_THP_SWAP=y, else 1.
This patch converts SWAP_NR_ORDERS and all the relevant code paths to make it runtime dependent. This is needed for architectures like powerpc book3s64, where PMD_ORDER is decided at runtime depending upon which MMU is chosen (Radix / Hash).

One thing to note here is, if any of the allocations required in swapfile_init() call (which is a subsys_initcall) fails, then we have no option but to panic. This is inline with how memory allocation failures in other subsys_initcall() are handled.

Signed-off-by Ritesh Harjani (IBM) <[email protected]>
---
 include/linux/swap.h |  10 ++---
 mm/swapfile.c        | 105 ++++++++++++++++++++++++++++++-------------
 2 files changed, 78 insertions(+), 37 deletions(-)
include/linux/swap.h +5 -5
index 46c25523d7b8..063ab7c4d4a5 100644
@@ -224,9 +224,9 @@ enum {
 #define SWAP_ENTRY_INVALID	0
 
 #ifdef CONFIG_THP_SWAP
-#define SWAP_NR_ORDERS		(PMD_ORDER + 1)
+#define swap_nr_orders()	((unsigned int)(PMD_ORDER + 1))
 #else
-#define SWAP_NR_ORDERS		1
+#define swap_nr_orders()	(1U)
 #endif
 
 /*

@@ -234,7 +234,7 @@ enum {
  * The purpose is to optimize SWAP throughput on these device.
  */
 struct swap_sequential_cluster {
-	unsigned int next[SWAP_NR_ORDERS]; /* Likely next allocation offset */
+	DECLARE_FLEX_ARRAY(unsigned int, next); /* Likely next allocation offset */
 };
 
 /*

@@ -250,9 +250,9 @@ struct swap_info_struct {
 	struct swap_cluster_info *cluster_info; /* cluster info. Only for SSD */
 	struct list_head free_clusters; /* free clusters list */
 	struct list_head full_clusters; /* full clusters list */
-	struct list_head nonfull_clusters[SWAP_NR_ORDERS];
+	struct list_head *nonfull_clusters;
 					/* list of cluster that contains at least one free slot */
-	struct list_head frag_clusters[SWAP_NR_ORDERS];
+	struct list_head *frag_clusters;
 					/* list of cluster that are fragmented or contented */
 	unsigned int pages;		/* total of usable pages of swap */
 	atomic_long_t inuse_pages;	/* number of those currently in use */
mm/swapfile.c +73 -32

251 lines not on this page · raw message

--
2.39.5

author reviewer
#8 lore raw

RFC 4/4 powerpc: Kconfig: Enable THP_SWAP on Book3S64

In reply to Ritesh Harjani (IBM)

This enables THP_SWAP support for Book3S64.

The performance testing of this patch series on Book3S64 with zram has shown around 40-50% improvement in case of Radix. We will be doing some performance testing on Hash too and will soon update the results.

Signed-off-by Ritesh Harjani (IBM) <[email protected]>
---
 arch/powerpc/platforms/Kconfig.cputype | 1 +
 1 file changed, 1 insertion(+)
arch/powerpc/platforms/Kconfig.cputype +1 -0
index bac02c83bb3e..48f74bd22343 100644
@@ -113,6 +113,7 @@ config PPC_THP
        select HAVE_ARCH_TRANSPARENT_HUGEPAGE
        select HAVE_ARCH_TRANSPARENT_HUGEPAGE_PUD
        select ARCH_ENABLE_THP_MIGRATION if TRANSPARENT_HUGEPAGE
+       select ARCH_WANTS_THP_SWAP	if TRANSPARENT_HUGEPAGE
 
 choice
 	prompt "CPU selection"

--
2.39.5

reviewer
#9 lore raw
In reply to Ritesh Harjani (IBM)
Quoted 24 lines · Ritesh Harjani (IBM)

On PowerPC Book3S64, MMU is selected at runtime, so macros like PMD_SHIFT are effectively runtime variables in the Book3S64 code. THP swap code uses these macros for e.g. to size some of its array data structures based on PMD_ORDER. This patch series makes that usage dependent on the runtime variable.

Sayali did some performance runs of this on Book3S64 with Radix and it gives 40-50% performance improvement. We also plan to run it with Hash, will soon update the results.

Note that this patch series is based out of linux-next (next-20260608).

Ritesh Harjani (IBM) (4):
include/linux/swap.h: Remove unused leftovers
mm, swap: make SWAPFILE_CLUSTER runtime
mm, swap: make SWAP_NR_ORDERS runtime
powerpc: Kconfig: Enable THP_SWAP on Book3S64

 arch/powerpc/platforms/Kconfig.cputype |   1 +
 include/linux/swap.h                   |  17 +---
 mm/swap.h                              |   5 +-
 mm/swap_table.h                        |   6 +-
 mm/swapfile.c                          | 132 ++++++++++++++++++-------
 5 files changed, 106 insertions(+), 55 deletions(-)
Quoted 3 lines

--
2.39.5

Hello!

Instead of making SWAP_NR_ORDERS fully runtime, could we set it to the max PMD_ORDER possible on PowerPC Book3S64 as a compile-time constant in the swap.h ifdef block? (My assumtion is PMD_ORDER max not too big.)

I think the general runtime version adds cost. It impacts all other archs. percpu_swap_cluster needs a runtime alloc,
the si/offset and nonfull/frag arrays become separate pointers, and some accesses get one more indirection. And for nr_orders=1, the allocation itself is just waste.

With a compile-time possible max constant, the only downside is some acceptable amount of wasted bytes per CPU / per device on Book3S64 (the unused entries in the swap offset cache and the nonfull/frag lists), with no perf impact. the perf improvement comes from THP swap itself, right? Other arches see no
impact at all.

patch 2 looks fine as is. SWAPFILE_CLUSTER backs much bigger per-cluster arrays, so runtime sizing makes sense there, and it looks like no impact to other arches or the current code.

Thanks!
Youngjun Park

author reviewer
#10 lore raw
Quoted 25 lines · YoungJun Park
Quoted 24 lines · Ritesh Harjani (IBM)

On PowerPC Book3S64, MMU is selected at runtime, so macros like PMD_SHIFT are effectively runtime variables in the Book3S64 code. THP swap code uses these macros for e.g. to size some of its array data structures based on PMD_ORDER. This patch series makes that usage dependent on the runtime variable.

Sayali did some performance runs of this on Book3S64 with Radix and it gives 40-50% performance improvement. We also plan to run it with Hash, will soon update the results.

Note that this patch series is based out of linux-next (next-20260608).

Ritesh Harjani (IBM) (4):
include/linux/swap.h: Remove unused leftovers
mm, swap: make SWAPFILE_CLUSTER runtime
mm, swap: make SWAP_NR_ORDERS runtime
powerpc: Kconfig: Enable THP_SWAP on Book3S64

 arch/powerpc/platforms/Kconfig.cputype |   1 +
 include/linux/swap.h                   |  17 +---
 mm/swap.h                              |   5 +-
 mm/swap_table.h                        |   6 +-
 mm/swapfile.c                          | 132 ++++++++++++++++++-------
 5 files changed, 106 insertions(+), 55 deletions(-)
Quoted 5 lines
Quoted 3 lines

--
2.39.5

Hello!

Thanks for taking a look at this.

Quoted 10 lines

Instead of making SWAP_NR_ORDERS fully runtime, could we set it to the max PMD_ORDER possible on PowerPC Book3S64 as a compile-time constant in the swap.h ifdef block? (My assumtion is PMD_ORDER max not too big.)

I think the general runtime version adds cost. It impacts all other archs. percpu_swap_cluster needs a runtime alloc,
the si/offset and nonfull/frag arrays become separate pointers, and some accesses get one more indirection. And for nr_orders=1, the allocation itself is just waste.

Quoted 6 lines

With a compile-time possible max constant, the only downside is some acceptable amount of wasted bytes per CPU / per device on Book3S64 (the unused entries in the swap offset cache and the nonfull/frag lists), with no perf impact. the perf improvement comes from THP swap itself, right? Other arches see no
impact at all.

I looked into the memory waste comparison between static v/s runtime alloc. And the wastage for per-cpu alloc data structures (with Radix MMU) will be 0, because we use kcalloc_node() which will use kmalloc-64 slab. So slab padding would anyway add some memory waste. So it is as good as using static arrays with some max PMD_ORDER for the percpu_swap_cluster.

For the other lists you mentioned, it anyways adds a onetime negligible cost which isn't worth for making SWAP_NR_ORDERS runtime.

Quoted 4 lines

patch 2 looks fine as is. SWAPFILE_CLUSTER backs much bigger per-cluster arrays, so runtime sizing makes sense there, and it looks like no impact to other arches or the current code.

yup. That make sense.

So, unless someone else raises any objection - I will give this a try instead of patch-3 in this series and will get back with v2.

arch/powerpc/include/asm/book3s/64/pgtable.h +3 -0
index e67e64ac6e8c..57abd8b2c9a1 100644
@@ -204,6 +204,9 @@ extern unsigned long __pmd_frag_size_shift;
 #define MAX_PTRS_PER_PGD       (1 << (H_PGD_INDEX_SIZE > RADIX_PGD_INDEX_SIZE ? \
                                       H_PGD_INDEX_SIZE : RADIX_PGD_INDEX_SIZE))
 
+#define ARCH_MAX_PMD_ORDER ((H_PTE_INDEX_SIZE > RADIX_PTE_INDEX_SIZE) ? \
+                               H_PTE_INDEX_SIZE : RADIX_PTE_INDEX_SIZE)
+
 /* PMD_SHIFT determines what a second-level page table entry can map */
 #define PMD_SHIFT      (PAGE_SHIFT + PTE_INDEX_SIZE)
 #define PMD_SIZE       (1UL << PMD_SHIFT)
include/linux/swap.h +5 -2
index 46c25523d7b8..5f1451f8f266 100644
@@ -224,10 +224,14 @@ enum {
 #define SWAP_ENTRY_INVALID     0
 
 #ifdef CONFIG_THP_SWAP
+#ifdef ARCH_MAX_PMD_ORDER
+#define SWAP_NR_ORDERS         (ARCH_MAX_PMD_ORDER + 1)
+#else
 #define SWAP_NR_ORDERS         (PMD_ORDER + 1)
+#endif /* ARCH_MAX_PMD_ORDER */
 #else
 #define SWAP_NR_ORDERS         1
-#endif
+#endif /* CONFIG_THP_SWAP */
 
 
-ritesh