Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1233003 > unrolled thread

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

Back to article view | Back to linux.kernel

Contents

  [PATCH 0/5] kernel/cpu.c: eliminate some indirection Rasmus Villemoes <linux@rasmusvillemoes.dk> - 2015-09-25 20:30 +0200
    [PATCH 1/5] kernel/cpu.c: change type of cpu_possible_bits and friends Rasmus Villemoes <linux@rasmusvillemoes.dk> - 2015-09-25 20:30 +0200
    [PATCH 5/5] kernel/cpu.c: make set_cpu_* static inlines Rasmus Villemoes <linux@rasmusvillemoes.dk> - 2015-09-25 20:30 +0200
    Re: [PATCH 0/5] kernel/cpu.c: eliminate some indirection Rusty Russell <rusty@rustcorp.com.au> - 2015-09-27 08:40 +0200
      Re: [PATCH 0/5] kernel/cpu.c: eliminate some indirection Rasmus Villemoes <linux@rasmusvillemoes.dk> - 2015-09-28 08:30 +0200
        Re: [PATCH 0/5] kernel/cpu.c: eliminate some indirection Rusty Russell <rusty@rustcorp.com.au> - 2015-09-29 01:40 +0200

#1233003 — [PATCH 0/5] kernel/cpu.c: eliminate some indirection

Maybe third time's the charm...

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
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
3/5).

The first 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, bloat-o-meter says we save ~3000 bytes.

Rasmus Villemoes (5):
  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

 drivers/base/cpu.c      | 10 ++++----
 include/linux/cpumask.h | 55 +++++++++++++++++++++++++++++++++++-------
 kernel/cpu.c            | 64 ++++++++++---------------------------------------
 3 files changed, 65 insertions(+), 64 deletions(-)

-- 
2.1.3

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [next] | [standalone]

#1233004 — [PATCH 1/5] kernel/cpu.c: change type of cpu_possible_bits and friends

Change cpu_possible_bits and friends (online, present, active) from
being bitmaps that happen to have the right size to actually being
struct cpumasks. Also rename them to __cpu_xyz_mask. This is mostly a
small cleanup in preparation for exporting them and, eventually,
eliminating the extra indirection through the cpu_xyz_mask variables.

Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk>
---
 kernel/cpu.c | 44 ++++++++++++++++++++++----------------------
 1 file changed, 22 insertions(+), 22 deletions(-)

diff --git a/kernel/cpu.c b/kernel/cpu.c
index 82cf9dff4295..fea4a3ce3011 100644
--- a/kernel/cpu.c
+++ b/kernel/cpu.c
@@ -772,71 +772,71 @@ const DECLARE_BITMAP(cpu_all_bits, NR_CPUS) = CPU_BITS_ALL;
 EXPORT_SYMBOL(cpu_all_bits);
 
 #ifdef CONFIG_INIT_ALL_POSSIBLE
-static DECLARE_BITMAP(cpu_possible_bits, CONFIG_NR_CPUS) __read_mostly
-	= CPU_BITS_ALL;
+static struct cpumask __cpu_possible_mask __read_mostly
+	= {CPU_BITS_ALL};
 #else
-static DECLARE_BITMAP(cpu_possible_bits, CONFIG_NR_CPUS) __read_mostly;
+static struct cpumask __cpu_possible_mask __read_mostly;
 #endif
-const struct cpumask *const cpu_possible_mask = to_cpumask(cpu_possible_bits);
+const struct cpumask *const cpu_possible_mask = &__cpu_possible_mask;
 EXPORT_SYMBOL(cpu_possible_mask);
 
-static DECLARE_BITMAP(cpu_online_bits, CONFIG_NR_CPUS) __read_mostly;
-const struct cpumask *const cpu_online_mask = to_cpumask(cpu_online_bits);
+static struct cpumask __cpu_online_mask __read_mostly;
+const struct cpumask *const cpu_online_mask = &__cpu_online_mask;
 EXPORT_SYMBOL(cpu_online_mask);
 
-static DECLARE_BITMAP(cpu_present_bits, CONFIG_NR_CPUS) __read_mostly;
-const struct cpumask *const cpu_present_mask = to_cpumask(cpu_present_bits);
+static struct cpumask __cpu_present_mask __read_mostly;
+const struct cpumask *const cpu_present_mask = &__cpu_present_mask;
 EXPORT_SYMBOL(cpu_present_mask);
 
-static DECLARE_BITMAP(cpu_active_bits, CONFIG_NR_CPUS) __read_mostly;
-const struct cpumask *const cpu_active_mask = to_cpumask(cpu_active_bits);
+static struct cpumask __cpu_active_mask __read_mostly;
+const struct cpumask *const cpu_active_mask = &__cpu_active_mask;
 EXPORT_SYMBOL(cpu_active_mask);
 
 void set_cpu_possible(unsigned int cpu, bool possible)
 {
 	if (possible)
-		cpumask_set_cpu(cpu, to_cpumask(cpu_possible_bits));
+		cpumask_set_cpu(cpu, &__cpu_possible_mask);
 	else
-		cpumask_clear_cpu(cpu, to_cpumask(cpu_possible_bits));
+		cpumask_clear_cpu(cpu, &__cpu_possible_mask);
 }
 
 void set_cpu_present(unsigned int cpu, bool present)
 {
 	if (present)
-		cpumask_set_cpu(cpu, to_cpumask(cpu_present_bits));
+		cpumask_set_cpu(cpu, &__cpu_present_mask);
 	else
-		cpumask_clear_cpu(cpu, to_cpumask(cpu_present_bits));
+		cpumask_clear_cpu(cpu, &__cpu_present_mask);
 }
 
 void set_cpu_online(unsigned int cpu, bool online)
 {
 	if (online) {
-		cpumask_set_cpu(cpu, to_cpumask(cpu_online_bits));
-		cpumask_set_cpu(cpu, to_cpumask(cpu_active_bits));
+		cpumask_set_cpu(cpu, &__cpu_online_mask);
+		cpumask_set_cpu(cpu, &__cpu_active_mask);
 	} else {
-		cpumask_clear_cpu(cpu, to_cpumask(cpu_online_bits));
+		cpumask_clear_cpu(cpu, &__cpu_online_mask);
 	}
 }
 
 void set_cpu_active(unsigned int cpu, bool active)
 {
 	if (active)
-		cpumask_set_cpu(cpu, to_cpumask(cpu_active_bits));
+		cpumask_set_cpu(cpu, &__cpu_active_mask);
 	else
-		cpumask_clear_cpu(cpu, to_cpumask(cpu_active_bits));
+		cpumask_clear_cpu(cpu, &__cpu_active_mask);
 }
 
 void init_cpu_present(const struct cpumask *src)
 {
-	cpumask_copy(to_cpumask(cpu_present_bits), src);
+	cpumask_copy(&__cpu_present_mask, src);
 }
 
 void init_cpu_possible(const struct cpumask *src)
 {
-	cpumask_copy(to_cpumask(cpu_possible_bits), src);
+	cpumask_copy(&__cpu_possible_mask, src);
 }
 
 void init_cpu_online(const struct cpumask *src)
 {
-	cpumask_copy(to_cpumask(cpu_online_bits), src);
+	cpumask_copy(&__cpu_online_mask, src);
 }
-- 
2.1.3

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1233005 — [PATCH 5/5] kernel/cpu.c: make set_cpu_* static inlines

Almost all callers of the set_cpu_* functions pass an explicit true
or false. Making them static inline thus replaces the function calls
with a simple set_bit/clear_bit, saving some .text.

Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk>
---
 include/linux/cpumask.h | 43 +++++++++++++++++++++++++++++++++++++++----
 kernel/cpu.c            | 34 ----------------------------------
 2 files changed, 39 insertions(+), 38 deletions(-)

diff --git a/include/linux/cpumask.h b/include/linux/cpumask.h
index 52ab539aefce..fc14275ff34e 100644
--- a/include/linux/cpumask.h
+++ b/include/linux/cpumask.h
@@ -720,14 +720,49 @@ extern const DECLARE_BITMAP(cpu_all_bits, NR_CPUS);
 #define for_each_present_cpu(cpu)  for_each_cpu((cpu), cpu_present_mask)
 
 /* Wrappers for arch boot code to manipulate normally-constant masks */
-void set_cpu_possible(unsigned int cpu, bool possible);
-void set_cpu_present(unsigned int cpu, bool present);
-void set_cpu_online(unsigned int cpu, bool online);
-void set_cpu_active(unsigned int cpu, bool active);
 void init_cpu_present(const struct cpumask *src);
 void init_cpu_possible(const struct cpumask *src);
 void init_cpu_online(const struct cpumask *src);
 
+static inline void
+set_cpu_possible(unsigned int cpu, bool possible)
+{
+	if (possible)
+		cpumask_set_cpu(cpu, &__cpu_possible_mask);
+	else
+		cpumask_clear_cpu(cpu, &__cpu_possible_mask);
+}
+
+static inline void
+set_cpu_present(unsigned int cpu, bool present)
+{
+	if (present)
+		cpumask_set_cpu(cpu, &__cpu_present_mask);
+	else
+		cpumask_clear_cpu(cpu, &__cpu_present_mask);
+}
+
+static inline void
+set_cpu_online(unsigned int cpu, bool online)
+{
+	if (online) {
+		cpumask_set_cpu(cpu, &__cpu_online_mask);
+		cpumask_set_cpu(cpu, &__cpu_active_mask);
+	} else {
+		cpumask_clear_cpu(cpu, &__cpu_online_mask);
+	}
+}
+
+static inline void
+set_cpu_active(unsigned int cpu, bool active)
+{
+	if (active)
+		cpumask_set_cpu(cpu, &__cpu_active_mask);
+	else
+		cpumask_clear_cpu(cpu, &__cpu_active_mask);
+}
+
+
 /**
  * to_cpumask - convert an NR_CPUS bitmap to a struct cpumask *
  * @bitmap: the bitmap
diff --git a/kernel/cpu.c b/kernel/cpu.c
index dd70f600442f..5210d80efc28 100644
--- a/kernel/cpu.c
+++ b/kernel/cpu.c
@@ -788,40 +788,6 @@ EXPORT_SYMBOL(__cpu_present_mask);
 struct cpumask __cpu_active_mask __read_mostly;
 EXPORT_SYMBOL(__cpu_active_mask);
 
-void set_cpu_possible(unsigned int cpu, bool possible)
-{
-	if (possible)
-		cpumask_set_cpu(cpu, &__cpu_possible_mask);
-	else
-		cpumask_clear_cpu(cpu, &__cpu_possible_mask);
-}
-
-void set_cpu_present(unsigned int cpu, bool present)
-{
-	if (present)
-		cpumask_set_cpu(cpu, &__cpu_present_mask);
-	else
-		cpumask_clear_cpu(cpu, &__cpu_present_mask);
-}
-
-void set_cpu_online(unsigned int cpu, bool online)
-{
-	if (online) {
-		cpumask_set_cpu(cpu, &__cpu_online_mask);
-		cpumask_set_cpu(cpu, &__cpu_active_mask);
-	} else {
-		cpumask_clear_cpu(cpu, &__cpu_online_mask);
-	}
-}
-
-void set_cpu_active(unsigned int cpu, bool active)
-{
-	if (active)
-		cpumask_set_cpu(cpu, &__cpu_active_mask);
-	else
-		cpumask_clear_cpu(cpu, &__cpu_active_mask);
-}
-
 void init_cpu_present(const struct cpumask *src)
 {
 	cpumask_copy(&__cpu_present_mask, src);
-- 
2.1.3

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1233624

Rasmus Villemoes <linux@rasmusvillemoes.dk> writes:
> Maybe third time's the charm...
>
> 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
> simply a useless indirection.

Thanks, consider all patches Acked-by: Rusty Russell <rusty@rustcorp.com.au>

But to be clear, it has outlived its usefulness, but it was not useless.

In particular, there used to be a debug config where 'struct cpumask'
wasn't defined, so we could catch people declaring 'struct cpumask' on
the stack (or passing by value).

There was a plan to remove CONFIG_NR_CPUS (ie. having no compile-time
cpu limit), but it seemed overkill and was abandoned.  But avoiding
'struct cpumask' (not struct cpumask *) in the core wherever possible
was a step towards it.

Hope that clarifies,
Rusty.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1233874

On Sun, Sep 27 2015, Rusty Russell <rusty@rustcorp.com.au> wrote:

> But to be clear, it has outlived its usefulness, but it was not useless.
>
> In particular, there used to be a debug config where 'struct cpumask'
> wasn't defined, so we could catch people declaring 'struct cpumask' on
> the stack (or passing by value).
>
> There was a plan to remove CONFIG_NR_CPUS (ie. having no compile-time
> cpu limit), but it seemed overkill and was abandoned.  But avoiding
> 'struct cpumask' (not struct cpumask *) in the core wherever possible
> was a step towards it.
>
> Hope that clarifies,

It does, thanks! Should some of that be edited into one of the
changelogs?

Rasmus
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1234571

Rasmus Villemoes <linux@rasmusvillemoes.dk> writes:
> On Sun, Sep 27 2015, Rusty Russell <rusty@rustcorp.com.au> wrote:
>
>> But to be clear, it has outlived its usefulness, but it was not useless.
>>
>> In particular, there used to be a debug config where 'struct cpumask'
>> wasn't defined, so we could catch people declaring 'struct cpumask' on
>> the stack (or passing by value).
>>
>> There was a plan to remove CONFIG_NR_CPUS (ie. having no compile-time
>> cpu limit), but it seemed overkill and was abandoned.  But avoiding
>> 'struct cpumask' (not struct cpumask *) in the core wherever possible
>> was a step towards it.
>>
>> Hope that clarifies,
>
> It does, thanks! Should some of that be edited into one of the
> changelogs?

Well, you could describe it as "now-useless", since it looks like you've
got another rev coming anyway?

Cheers,
Rusty.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web