[PATCH v2 0/6] kernel/cpu.c: eliminate some indirection

[PATCH v2 0/6] kernel/cpu.c: eliminate some indirection

[Rasmus Villemoes]

v2: fix build failure on ppc, add acks.

The four cpumasks cpu_{possible,online,present,active}_bits are
exposed readonly via the corresponding const variables
cpu_xyz_mask. But they are also accessible for arbitrary writing via
the exposed functions set_cpu_xyz. There's quite a bit of code
throughout the kernel which iterates over or otherwise accesses these
bitmaps, and having the access go via the cpu_xyz_mask variables is
nowadays [1] simply a useless indirection.

It may be that any problem in CS can be solved by an extra level of
indirection, but that doesn't mean every extra indirection solves a
problem. In this case, it even necessitates some minor ugliness (see
4/6).

Patch 1/6 is new in v2, and fixes a build failure on ppc by renaming a
struct member, to avoid problems when the identifier cpu_online_mask
becomes a macro later in the series. The next four patches eliminate
the cpu_xyz_mask variables by simply exposing the actual bitmaps,
after renaming them to discourage direct access - that still happens
through cpu_xyz_mask, which are now simply macros with the same type
and value as they used to have.

After that, there's no longer any reason to have the setter functions
be out-of-line: The boolean parameter is almost always a literal true
or false, so by making them static inlines they will usually compile
to one or two instructions.

For a defconfig build on x86_64, bloat-o-meter says we save ~3000
bytes. We also save a little stack (stackdelta says 127 functions have
a 16 byte smaller stack frame, while two grow by that amount). Mostly
because, when iterating over the mask, gcc typically loads the value
of cpu_xyz_mask into a callee-saved register and from there into %rdi
before each find_next_bit call - now it can just load the appropriate
immediate address into %rdi before each call.

[1] See Rusty's kind explanation
http://thread.gmane.org/gmane.linux.kernel/2047078/focus=2047722 for
some historic context.

Rasmus Villemoes (6):
  powerpc/fadump: rename cpu_online_mask member of struct
    fadump_crash_info_header
  kernel/cpu.c: change type of cpu_possible_bits and friends
  kernel/cpu.c: export __cpu_*_mask
  drivers/base/cpu.c: use __cpu_*_mask directly
  kernel/cpu.c: eliminate cpu_*_mask
  kernel/cpu.c: make set_cpu_* static inlines

 arch/powerpc/include/asm/fadump.h |  2 +-
 arch/powerpc/kernel/fadump.c      |  4 +--
 drivers/base/cpu.c                | 10 +++---
 include/linux/cpumask.h           | 55 ++++++++++++++++++++++++++++-----
 kernel/cpu.c                      | 64 ++++++++-------------------------------
 5 files changed, 68 insertions(+), 67 deletions(-)

-- 
2.1.3

[PATCH v2 1/6] powerpc/fadump: rename cpu_online_mask member of struct fadump_crash_info_header

[Rasmus Villemoes]

As preparation for eliminating the indirect access to the various
global cpu_*_bits bitmaps via the pointer variables cpu_*_mask, rename
the cpu_online_mask member of struct fadump_crash_info_header to
simply online_mask, thus allowing cpu_online_mask to become a macro.

Acked-by: Michael Ellerman <mpe@ellerman.id.au>
Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk>
---
 arch/powerpc/include/asm/fadump.h | 2 +-
 arch/powerpc/kernel/fadump.c      | 4 ++--
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/arch/powerpc/include/asm/fadump.h b/arch/powerpc/include/asm/fadump.h
index 493e72f64b35..b4407d0add27 100644
--- a/arch/powerpc/include/asm/fadump.h
+++ b/arch/powerpc/include/asm/fadump.h
@@ -191,7 +191,7 @@ struct fadump_crash_info_header {
 	u64		elfcorehdr_addr;
 	u32		crashing_cpu;
 	struct pt_regs	regs;
-	struct cpumask	cpu_online_mask;
+	struct cpumask	online_mask;
 };
 
 /* Crash memory ranges */
diff --git a/arch/powerpc/kernel/fadump.c b/arch/powerpc/kernel/fadump.c
index 26d091a1a54c..3cb3b02a13dd 100644
--- a/arch/powerpc/kernel/fadump.c
+++ b/arch/powerpc/kernel/fadump.c
@@ -415,7 +415,7 @@ void crash_fadump(struct pt_regs *regs, const char *str)
 	else
 		ppc_save_regs(&fdh->regs);
 
-	fdh->cpu_online_mask = *cpu_online_mask;
+	fdh->online_mask = *cpu_online_mask;
 
 	/* Call ibm,os-term rtas call to trigger firmware assisted dump */
 	rtas_os_term((char *)str);
@@ -646,7 +646,7 @@ static int __init fadump_build_cpu_notes(const struct fadump_mem_struct *fdm)
 		}
 		/* Lower 4 bytes of reg_value contains logical cpu id */
 		cpu = be64_to_cpu(reg_entry->reg_value) & FADUMP_CPU_ID_MASK;
-		if (fdh && !cpumask_test_cpu(cpu, &fdh->cpu_online_mask)) {
+		if (fdh && !cpumask_test_cpu(cpu, &fdh->online_mask)) {
 			SKIP_TO_NEXT_CPU(reg_entry);
 			continue;
 		}
-- 
2.1.3

Re: [PATCH v2 0/6] kernel/cpu.c: eliminate some indirection

[Rasmus Villemoes]

On Tue, Oct 06 2015, Rasmus Villemoes <linux@rasmusvillemoes.dk> wrote:

> v2: fix build failure on ppc, add acks.

Does anyone want to take these through their tree?

Rasmus

Re: [PATCH v2 0/6] kernel/cpu.c: eliminate some indirection

[Rusty Russell]

Rasmus Villemoes <rv@rasmusvillemoes.dk> writes:
> On Tue, Oct 06 2015, Rasmus Villemoes <linux@rasmusvillemoes.dk> wrote:
>
>> v2: fix build failure on ppc, add acks.
>
> Does anyone want to take these through their tree?

I think the x86 tree is the least illogical place, unless akpm wants it?

Cheers,
Rusty.

[PATCH v2, resend 0/6] kernel/cpu.c: eliminate some indirection

[Rasmus Villemoes]

Andrew, can I get you to take these through -mm? Noone else seems to
want to pick them up. They're rebased on top of 4.4-rc2 (and applied
cleanly), but otherwise identical to what I've sent previously.

=====

v2: fix build failure on ppc, add acks.

The four cpumasks cpu_{possible,online,present,active}_bits are
exposed readonly via the corresponding const variables
cpu_xyz_mask. But they are also accessible for arbitrary writing via
the exposed functions set_cpu_xyz. There's quite a bit of code
throughout the kernel which iterates over or otherwise accesses these
bitmaps, and having the access go via the cpu_xyz_mask variables is
nowadays [1] simply a useless indirection.

It may be that any problem in CS can be solved by an extra level of
indirection, but that doesn't mean every extra indirection solves a
problem. In this case, it even necessitates some minor ugliness (see
4/6).

Patch 1/6 is new in v2, and fixes a build failure on ppc by renaming a
struct member, to avoid problems when the identifier cpu_online_mask
becomes a macro later in the series. The next four patches eliminate
the cpu_xyz_mask variables by simply exposing the actual bitmaps,
after renaming them to discourage direct access - that still happens
through cpu_xyz_mask, which are now simply macros with the same type
and value as they used to have.

After that, there's no longer any reason to have the setter functions
be out-of-line: The boolean parameter is almost always a literal true
or false, so by making them static inlines they will usually compile
to one or two instructions.

For a defconfig build on x86_64, bloat-o-meter says we save ~3000
bytes. We also save a little stack (stackdelta says 127 functions have
a 16 byte smaller stack frame, while two grow by that amount). Mostly
because, when iterating over the mask, gcc typically loads the value
of cpu_xyz_mask into a callee-saved register and from there into %rdi
before each find_next_bit call - now it can just load the appropriate
immediate address into %rdi before each call.

[1] See Rusty's kind explanation
http://thread.gmane.org/gmane.linux.kernel/2047078/focus=2047722 for
some historic context.

Rasmus Villemoes (6):
  powerpc/fadump: rename cpu_online_mask member of struct
    fadump_crash_info_header
  kernel/cpu.c: change type of cpu_possible_bits and friends
  kernel/cpu.c: export __cpu_*_mask
  drivers/base/cpu.c: use __cpu_*_mask directly
  kernel/cpu.c: eliminate cpu_*_mask
  kernel/cpu.c: make set_cpu_* static inlines

 arch/powerpc/include/asm/fadump.h |  2 +-
 arch/powerpc/kernel/fadump.c      |  4 +--
 drivers/base/cpu.c                | 10 +++---
 include/linux/cpumask.h           | 55 ++++++++++++++++++++++++++++-----
 kernel/cpu.c                      | 64 ++++++++-------------------------------
 5 files changed, 68 insertions(+), 67 deletions(-)

-- 
2.6.1

[PATCH v2, resend 1/6] powerpc/fadump: rename cpu_online_mask member of struct fadump_crash_info_header

[Rasmus Villemoes]

As preparation for eliminating the indirect access to the various
global cpu_*_bits bitmaps via the pointer variables cpu_*_mask, rename
the cpu_online_mask member of struct fadump_crash_info_header to
simply online_mask, thus allowing cpu_online_mask to become a macro.

Acked-by: Michael Ellerman <mpe@ellerman.id.au>
Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk>
---
 arch/powerpc/include/asm/fadump.h | 2 +-
 arch/powerpc/kernel/fadump.c      | 4 ++--
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/arch/powerpc/include/asm/fadump.h b/arch/powerpc/include/asm/fadump.h
index 493e72f64b35..b4407d0add27 100644
--- a/arch/powerpc/include/asm/fadump.h
+++ b/arch/powerpc/include/asm/fadump.h
@@ -191,7 +191,7 @@ struct fadump_crash_info_header {
 	u64		elfcorehdr_addr;
 	u32		crashing_cpu;
 	struct pt_regs	regs;
-	struct cpumask	cpu_online_mask;
+	struct cpumask	online_mask;
 };
 
 /* Crash memory ranges */
diff --git a/arch/powerpc/kernel/fadump.c b/arch/powerpc/kernel/fadump.c
index 26d091a1a54c..3cb3b02a13dd 100644
--- a/arch/powerpc/kernel/fadump.c
+++ b/arch/powerpc/kernel/fadump.c
@@ -415,7 +415,7 @@ void crash_fadump(struct pt_regs *regs, const char *str)
 	else
 		ppc_save_regs(&fdh->regs);
 
-	fdh->cpu_online_mask = *cpu_online_mask;
+	fdh->online_mask = *cpu_online_mask;
 
 	/* Call ibm,os-term rtas call to trigger firmware assisted dump */
 	rtas_os_term((char *)str);
@@ -646,7 +646,7 @@ static int __init fadump_build_cpu_notes(const struct fadump_mem_struct *fdm)
 		}
 		/* Lower 4 bytes of reg_value contains logical cpu id */
 		cpu = be64_to_cpu(reg_entry->reg_value) & FADUMP_CPU_ID_MASK;
-		if (fdh && !cpumask_test_cpu(cpu, &fdh->cpu_online_mask)) {
+		if (fdh && !cpumask_test_cpu(cpu, &fdh->online_mask)) {
 			SKIP_TO_NEXT_CPU(reg_entry);
 			continue;
 		}
-- 
2.6.1