percpu: some trivial cleanup patches

The conditional check "(ai->dyn_size < PERCPU_DYNAMIC_EARLY_SIZE) has covered the check '(!ai->dyn_size)'.

Signed-off-by Baoquan He <[email protected]>

---
 mm/percpu.c | 1 -
 1 file changed, 1 deletion(-)

mm/percpu.c +0 -1

index 28e07ede46f6..1480bf283d11 100644
@@ -2615,7 +2615,6 @@ void __init pcpu_setup_first_chunk(const struct pcpu_alloc_info *ai,
 	PCPU_SETUP_BUG_ON(ai->unit_size < PCPU_MIN_UNIT_SIZE);
 	PCPU_SETUP_BUG_ON(!IS_ALIGNED(ai->unit_size, PCPU_BITMAP_BLOCK_SIZE));
 	PCPU_SETUP_BUG_ON(ai->dyn_size < PERCPU_DYNAMIC_EARLY_SIZE);
-	PCPU_SETUP_BUG_ON(!ai->dyn_size);
 	PCPU_SETUP_BUG_ON(!IS_ALIGNED(ai->reserved_size, PCPU_MIN_ALLOC_SIZE));
 	PCPU_SETUP_BUG_ON(!(IS_ALIGNED(PCPU_BITMAP_BLOCK_SIZE, PAGE_SIZE) ||
 			    IS_ALIGNED(PAGE_SIZE, PCPU_BITMAP_BLOCK_SIZE)));

Signature · 1 line

2.34.1

This removes the need of local varibale 'chunk', and optimize the code calling pcpu_alloc_first_chunk() to initialize reserved chunk and dynamic chunk to make it simpler.

Signed-off-by Baoquan He <[email protected]>

---
 mm/percpu.c | 32 +++++++++++++-------------------
 1 file changed, 13 insertions(+), 19 deletions(-)

mm/percpu.c +1 -2

index 1480bf283d11..c25b058a46ad 100644
@@ -2581,7 +2581,6 @@ void __init pcpu_setup_first_chunk(const struct pcpu_alloc_info *ai,
 {
 	size_t size_sum = ai->static_size + ai->reserved_size + ai->dyn_size;
 	size_t static_size, dyn_size;
-	struct pcpu_chunk *chunk;
 	unsigned long *group_offsets;
 	size_t *group_sizes;
 	unsigned long *unit_off;

@@ -2697,7 +2696,7 @@ void __init pcpu_setup_first_chunk(const struct pcpu_alloc_info *ai,
 	pcpu_unit_pages = ai->unit_size >> PAGE_SHIFT;
 	pcpu_unit_size = pcpu_unit_pages << PAGE_SHIFT;
 	pcpu_atom_size = ai->atom_size;
-	pcpu_chunk_struct_size = struct_size(chunk, populated,
+	pcpu_chunk_struct_size = struct_size((struct pcpu_chunk *)0, populated,
 					     BITS_TO_LONGS(pcpu_unit_pages));
 
 	pcpu_stats_save_ai(ai);

mm/percpu.c +0 -8

 	 */
 	tmp_addr = (unsigned long)base_addr + static_size;
-	map_size = ai->reserved_size ?: dyn_size;
-	chunk = pcpu_alloc_first_chunk(tmp_addr, map_size);
-
-	/* init dynamic chunk if necessary */
 	if (ai->reserved_size) {
-		pcpu_reserved_chunk = chunk;
-
-		tmp_addr = (unsigned long)base_addr + static_size +
-			   ai->reserved_size;

mm/percpu.c +3 -0

+	map_size = dyn_size;
+	pcpu_first_chunk = pcpu_alloc_first_chunk(tmp_addr, map_size);
+
 	pcpu_nr_empty_pop_pages = pcpu_first_chunk->nr_empty_pop_pages;
 	pcpu_chunk_relocate(pcpu_first_chunk, -1);
 

Signature · 1 line

2.34.1

Hello,

Quoted 31 lines · Baoquan He

This removes the need of local varibale 'chunk', and optimize the code calling pcpu_alloc_first_chunk() to initialize reserved chunk and dynamic chunk to make it simpler.

Signed-off-by Baoquan He <[email protected]>

---
 mm/percpu.c | 32 +++++++++++++-------------------
 1 file changed, 13 insertions(+), 19 deletions(-)

mm/percpu.c +1 -2

index 1480bf283d11..c25b058a46ad 100644
@@ -2581,7 +2581,6 @@ void __init pcpu_setup_first_chunk(const struct pcpu_alloc_info *ai,
 {
 	size_t size_sum = ai->static_size + ai->reserved_size + ai->dyn_size;
 	size_t static_size, dyn_size;
-	struct pcpu_chunk *chunk;
 	unsigned long *group_offsets;
 	size_t *group_sizes;
 	unsigned long *unit_off;

@@ -2697,7 +2696,7 @@ void __init pcpu_setup_first_chunk(const struct pcpu_alloc_info *ai,
 	pcpu_unit_pages = ai->unit_size >> PAGE_SHIFT;
 	pcpu_unit_size = pcpu_unit_pages << PAGE_SHIFT;
 	pcpu_atom_size = ai->atom_size;
-	pcpu_chunk_struct_size = struct_size(chunk, populated,
+	pcpu_chunk_struct_size = struct_size((struct pcpu_chunk *)0, populated,
 					     BITS_TO_LONGS(pcpu_unit_pages));
 
 	pcpu_stats_save_ai(ai);

mm/percpu.c void __init pcpu_setup_first_chunk(const struct pcpu_alloc_info *ai, · line 2737

Quoted 9 lines

mm/percpu.c +1 -5

@@ -2735,28 +2734,23 @@ void __init pcpu_setup_first_chunk(const struct pcpu_alloc_info *ai,
 
 	/*
 	 * Initialize first chunk.
-	 * If the reserved_size is non-zero, this initializes the reserved
-	 * chunk.  If the reserved_size is zero, the reserved chunk is NULL
-	 * and the dynamic region is initialized here.  The first chunk,
-	 * pcpu_first_chunk, will always point to the chunk that serves
-	 * the dynamic region.
+	 * If the reserved_size is non-zero, initializes the reserved chunk

^initialize

mm/percpu.c void __init pcpu_setup_first_chunk(const struct pcpu_alloc_info *ai, · line 2738

Quoted 1 line

mm/percpu.c +1 -0

+	 * firstly. If the reserved_size is zero, the reserved chunk is NULL

^ can remove firstly.

mm/percpu.c void __init pcpu_setup_first_chunk(const struct pcpu_alloc_info *ai, · line 2741

Quoted 3 lines

mm/percpu.c +3 -0

+	 * and the dynamic region is initialized directly. The first chunk,
+	 * pcpu_first_chunk, will always point to the chunk that serves the
+	 * dynamic region.

Reading this, I'll probably reword this comment to explain the reserved chunk better.

Quoted 18 lines

mm/percpu.c +2 -10

 	 */
 	tmp_addr = (unsigned long)base_addr + static_size;
-	map_size = ai->reserved_size ?: dyn_size;
-	chunk = pcpu_alloc_first_chunk(tmp_addr, map_size);
-
-	/* init dynamic chunk if necessary */
 	if (ai->reserved_size) {
-		pcpu_reserved_chunk = chunk;
-
-		tmp_addr = (unsigned long)base_addr + static_size +
-			   ai->reserved_size;
-		map_size = dyn_size;
-		chunk = pcpu_alloc_first_chunk(tmp_addr, map_size);
+		map_size = ai->reserved_size;
+		pcpu_reserved_chunk = pcpu_alloc_first_chunk(tmp_addr, map_size);
 	}
 

mm/percpu.c void __init pcpu_setup_first_chunk(const struct pcpu_alloc_info *ai, · line 2750

Quoted 4 lines

mm/percpu.c +2 -2

-	/* link the first chunk in */
-	pcpu_first_chunk = chunk;
+	/* init dynamic chunk if necessary */
+	tmp_addr += (unsigned long)ai->reserved_size;

I'm not a big fan of += the tmp_addr as I personally find it easier to read if it's just laid out explicitly.

Quoted 7 lines

mm/percpu.c +3 -0

+	map_size = dyn_size;
+	pcpu_first_chunk = pcpu_alloc_first_chunk(tmp_addr, map_size);
+
 	pcpu_nr_empty_pop_pages = pcpu_first_chunk->nr_empty_pop_pages;
 	pcpu_chunk_relocate(pcpu_first_chunk, -1);
 

Quoted 3 lines

Signature · 1 line

2.34.1

Overall, I think this is good, but I'd go 1 step further and get rid of map_size. Regarding tmp_addr, I'd prefer if we kept all the math together.

The variable 'err' is assgigned to an error message if atomic alloc failed, while it has no chance to be printed if is_atomic is true.

Here change to print error message too if atomic alloc failed, while avoid to call dump_stack() if that case.

Signed-off-by Baoquan He <[email protected]>

---
 mm/percpu.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

mm/percpu.c +0 -0

index c25b058a46ad..74f75ef0ad58 100644

mm/percpu.c +1 -0

 		if (!--warn_limit)
 			pr_info("limit reached, disable warning\n");
 	}
+
 	if (is_atomic) {
 		/* see the flag handling in pcpu_balance_workfn() */
 		pcpu_atomic_alloc_failed = true;

Signature · 1 line

2.34.1

Quoted 19 lines · Baoquan He

The variable 'err' is assgigned to an error message if atomic alloc failed, while it has no chance to be printed if is_atomic is true.

Here change to print error message too if atomic alloc failed, while avoid to call dump_stack() if that case.

Signed-off-by Baoquan He <[email protected]>

---
 mm/percpu.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

mm/percpu.c +0 -0

index c25b058a46ad..74f75ef0ad58 100644
@@ -1890,13 +1890,15 @@ static void __percpu *pcpu_alloc(size_t size, size_t align, bool reserved,
 fail:
 	trace_percpu_alloc_percpu_fail(reserved, is_atomic, size, align);
 

mm/percpu.c static void __percpu *pcpu_alloc(size_t size, size_t align, bool reserved, · line 1897

Quoted 7 lines

mm/percpu.c +3 -2

-	if (!is_atomic && do_warn && warn_limit) {
+	if (do_warn && warn_limit) {
 		pr_warn("allocation failed, size=%zu align=%zu atomic=%d, %s\n",
 			size, align, is_atomic, err);
-		dump_stack();
+		if (is_atomic)
+			dump_stack();

This should be (!is_atomic) to preserve the current logic?

Quoted 11 lines

mm/percpu.c +1 -0

 		if (!--warn_limit)
 			pr_info("limit reached, disable warning\n");
 	}
+
 	if (is_atomic) {
 		/* see the flag handling in pcpu_balance_workfn() */
 		pcpu_atomic_alloc_failed = true;

Signature · 1 line

2.34.1

Thanks,
Dennis

There's a left issue in my mailbox about percpu code at below. When I rechecked it and considered Dennis's comment, I made an attmept to fix it with patch 3.

https://lore.kernel.org/all/Y407wDMKq5ibE9sc@fedora/T/#u

Patch 1 and 2 are trivial clean up patches when reading percpu code.

Baoquan He (3):
mm/percpu.c: remove redundant check
mm/percpu.c: optimize the code in pcpu_setup_first_chunk() a little
bit
mm/percpu.c: print error message too if atomic alloc failed

 mm/percpu.c | 39 +++++++++++++++++----------------------
 1 file changed, 17 insertions(+), 22 deletions(-)

Signature · 1 line

2.34.1

Hello,

Quoted 17 lines · Baoquan He

There's a left issue in my mailbox about percpu code at below. When I rechecked it and considered Dennis's comment, I made an attmept to fix it with patch 3.

https://lore.kernel.org/all/Y407wDMKq5ibE9sc@fedora/T/#u

Patch 1 and 2 are trivial clean up patches when reading percpu code.

Baoquan He (3):
mm/percpu.c: remove redundant check
mm/percpu.c: optimize the code in pcpu_setup_first_chunk() a little
bit
mm/percpu.c: print error message too if atomic alloc failed

 mm/percpu.c | 39 +++++++++++++++++----------------------
 1 file changed, 17 insertions(+), 22 deletions(-)

Quoted 3 lines

Signature · 1 line

2.34.1

Thanks for these. I left a few comments. I think I might have some stuff for v6.6, I'll figure that out in a couple days. If that's so, I can pull 1, probably massage 2 and send that out again, and then I think you'll need to resend 3.

Quoted 24 lines · Dennis Zhou

Hello,

Quoted 20 lines · Baoquan He

There's a left issue in my mailbox about percpu code at below. When I rechecked it and considered Dennis's comment, I made an attmept to fix it with patch 3.

https://lore.kernel.org/all/Y407wDMKq5ibE9sc@fedora/T/#u

Patch 1 and 2 are trivial clean up patches when reading percpu code.

Baoquan He (3):
mm/percpu.c: remove redundant check
mm/percpu.c: optimize the code in pcpu_setup_first_chunk() a little
bit
mm/percpu.c: print error message too if atomic alloc failed

 mm/percpu.c | 39 +++++++++++++++++----------------------
 1 file changed, 17 insertions(+), 22 deletions(-)

Signature · 1 line

2.34.1

Quoted 4 lines

Thanks for these. I left a few comments. I think I might have some stuff for v6.6, I'll figure that out in a couple days. If that's so, I can pull 1, probably massage 2 and send that out again, and then I think you'll need to resend 3.

Sure, thanks for careful reviewing and great suggestion. So I only need to send v2 of patch 3, right? Or I should change and send v2 of the whold series? I may not get it clear.

Hi,

Quoted 30 lines · Baoquan He

Quoted 28 lines · Dennis Zhou

Hello,

Quoted 8 lines · Baoquan He

There's a left issue in my mailbox about percpu code at below. When I rechecked it and considered Dennis's comment, I made an attmept to fix it with patch 3.

https://lore.kernel.org/all/Y407wDMKq5ibE9sc@fedora/T/#u

Patch 1 and 2 are trivial clean up patches when reading percpu code.

Quoted 12 lines

Baoquan He (3):
mm/percpu.c: remove redundant check
mm/percpu.c: optimize the code in pcpu_setup_first_chunk() a little
bit
mm/percpu.c: print error message too if atomic alloc failed

 mm/percpu.c | 39 +++++++++++++++++----------------------
 1 file changed, 17 insertions(+), 22 deletions(-)

Signature · 1 line

2.34.1

Thanks for these. I left a few comments. I think I might have some stuff for v6.6, I'll figure that out in a couple days. If that's so, I can pull 1, probably massage 2 and send that out again, and then I think you'll need to resend 3.

Quoted 4 lines

Sure, thanks for careful reviewing and great suggestion. So I only need to send v2 of patch 3, right? Or I should change and send v2 of the whold series? I may not get it clear.

Sorry for the delay. I've pulled 1 and 2 (reworded the comment). Can you please resend patch 3.