[PATCH] Avoid mask based num_possible_cpus and num_online_cpus

[PATCH] Avoid mask based num_possible_cpus and num_online_cpus

[Venkatesh Pallipadi]

Kernel's notion of possible cpus (from include/linux/cpumask.h)
 *     cpu_possible_mask- has bit 'cpu' set iff cpu is populatable

 *  The cpu_possible_mask is fixed at boot time, as the set of CPU id's
 *  that it is possible might ever be plugged in at anytime during the
 *  life of that system boot.

 #define num_possible_cpus()     cpumask_weight(cpu_possible_mask)

and on x86 cpumask_weight() calls hweight64 and hweight64 (on older kernels
and systems with !X86_FEATURE_POPCNT) or a popcnt based alternative.

i.e, We needlessly go through this mask based calculation everytime
num_possible_cpus() is called.

The problem is there with cpu_online_mask() as well, which is fixed value at
boot time in !CONFIG_HOTPLUG_CPU case and should not change that often even
in HOTPLUG case.

Though most of the callers of these two routines are init time (with few
exceptions of runtime calls), it is cleaner to use variables
and not go through this repeated mask based calculation.

Signed-off-by: Venkatesh Pallipadi <venki@google.com>
---
 include/linux/cpumask.h |    8 ++++++--
 kernel/cpu.c            |    9 +++++++++
 2 files changed, 15 insertions(+), 2 deletions(-)

diff --git a/include/linux/cpumask.h b/include/linux/cpumask.h
index 4f7a632..2eb04dd 100644
--- a/include/linux/cpumask.h
+++ b/include/linux/cpumask.h
@@ -80,9 +80,13 @@ extern const struct cpumask *const cpu_online_mask;
 extern const struct cpumask *const cpu_present_mask;
 extern const struct cpumask *const cpu_active_mask;
 
+extern int nr_online_cpus;
+
 #if NR_CPUS > 1
-#define num_online_cpus()	cpumask_weight(cpu_online_mask)
-#define num_possible_cpus()	cpumask_weight(cpu_possible_mask)
+
+#define num_online_cpus()	(nr_online_cpus)
+#define num_possible_cpus()	(nr_cpu_ids)
+
 #define num_present_cpus()	cpumask_weight(cpu_present_mask)
 #define num_active_cpus()	cpumask_weight(cpu_active_mask)
 #define cpu_online(cpu)		cpumask_test_cpu((cpu), cpu_online_mask)
diff --git a/kernel/cpu.c b/kernel/cpu.c
index 2060c6e..eed2169 100644
--- a/kernel/cpu.c
+++ b/kernel/cpu.c
@@ -622,6 +622,13 @@ static DECLARE_BITMAP(cpu_active_bits, CONFIG_NR_CPUS) __read_mostly;
 const struct cpumask *const cpu_active_mask = to_cpumask(cpu_active_bits);
 EXPORT_SYMBOL(cpu_active_mask);
 
+#ifdef CONFIG_HOTPLUG_CPU
+int nr_online_cpus;
+#else
+int nr_online_cpus __read_mostly;
+#endif
+EXPORT_SYMBOL(nr_online_cpus);
+
 void set_cpu_possible(unsigned int cpu, bool possible)
 {
 	if (possible)
@@ -644,6 +651,8 @@ void set_cpu_online(unsigned int cpu, bool online)
 		cpumask_set_cpu(cpu, to_cpumask(cpu_online_bits));
 	else
 		cpumask_clear_cpu(cpu, to_cpumask(cpu_online_bits));
+
+	nr_online_cpus = cpumask_weight(cpu_online_mask);
 }
 
 void set_cpu_active(unsigned int cpu, bool active)
-- 
1.7.7.3

Re: [PATCH] Avoid mask based num_possible_cpus and num_online_cpus

[KOSAKI Motohiro]

(1/17/12 9:07 PM), Venkatesh Pallipadi wrote:
> Kernel's notion of possible cpus (from include/linux/cpumask.h)
>   *     cpu_possible_mask- has bit 'cpu' set iff cpu is populatable
> 
>   *  The cpu_possible_mask is fixed at boot time, as the set of CPU id's
>   *  that it is possible might ever be plugged in at anytime during the
>   *  life of that system boot.
> 
>   #define num_possible_cpus()     cpumask_weight(cpu_possible_mask)
> 
> and on x86 cpumask_weight() calls hweight64 and hweight64 (on older kernels
> and systems with !X86_FEATURE_POPCNT) or a popcnt based alternative.
> 
> i.e, We needlessly go through this mask based calculation everytime
> num_possible_cpus() is called.
> 
> The problem is there with cpu_online_mask() as well, which is fixed value at
> boot time in !CONFIG_HOTPLUG_CPU case and should not change that often even
> in HOTPLUG case.
> 
> Though most of the callers of these two routines are init time (with few
> exceptions of runtime calls), it is cleaner to use variables
> and not go through this repeated mask based calculation.
> 
> Signed-off-by: Venkatesh Pallipadi<venki@google.com>
> ---
>   include/linux/cpumask.h |    8 ++++++--
>   kernel/cpu.c            |    9 +++++++++
>   2 files changed, 15 insertions(+), 2 deletions(-)
> 
> diff --git a/include/linux/cpumask.h b/include/linux/cpumask.h
> index 4f7a632..2eb04dd 100644
> --- a/include/linux/cpumask.h
> +++ b/include/linux/cpumask.h
> @@ -80,9 +80,13 @@ extern const struct cpumask *const cpu_online_mask;
>   extern const struct cpumask *const cpu_present_mask;
>   extern const struct cpumask *const cpu_active_mask;
> 
> +extern int nr_online_cpus;
> +
>   #if NR_CPUS>  1
> -#define num_online_cpus()	cpumask_weight(cpu_online_mask)
> -#define num_possible_cpus()	cpumask_weight(cpu_possible_mask)
> +
> +#define num_online_cpus()	(nr_online_cpus)
> +#define num_possible_cpus()	(nr_cpu_ids)

nr_cpu_ids mean maximum cpu id of cpus. if cpu id are sparse, maximum id
doesn't match number of cpus.



> +
>   #define num_present_cpus()	cpumask_weight(cpu_present_mask)
>   #define num_active_cpus()	cpumask_weight(cpu_active_mask)
>   #define cpu_online(cpu)		cpumask_test_cpu((cpu), cpu_online_mask)
> diff --git a/kernel/cpu.c b/kernel/cpu.c
> index 2060c6e..eed2169 100644
> --- a/kernel/cpu.c
> +++ b/kernel/cpu.c
> @@ -622,6 +622,13 @@ static DECLARE_BITMAP(cpu_active_bits, CONFIG_NR_CPUS) __read_mostly;
>   const struct cpumask *const cpu_active_mask = to_cpumask(cpu_active_bits);
>   EXPORT_SYMBOL(cpu_active_mask);
> 
> +#ifdef CONFIG_HOTPLUG_CPU
> +int nr_online_cpus;
> +#else
> +int nr_online_cpus __read_mostly;
> +#endif
> +EXPORT_SYMBOL(nr_online_cpus);

You can always mark this to __read_mostly. other cpu hotplug stuff do so.
Because of, I guess, cpu hotplug developers don't think hotplugging is
frequently event.


>   void set_cpu_possible(unsigned int cpu, bool possible)
>   {
>   	if (possible)
> @@ -644,6 +651,8 @@ void set_cpu_online(unsigned int cpu, bool online)
>   		cpumask_set_cpu(cpu, to_cpumask(cpu_online_bits));
>   	else
>   		cpumask_clear_cpu(cpu, to_cpumask(cpu_online_bits));
> +
> +	nr_online_cpus = cpumask_weight(cpu_online_mask);
>   }

I like this change. :)

Re: [PATCH] Avoid mask based num_possible_cpus and num_online_cpus

[Venki Pallipadi]

On Tue, Jan 17, 2012 at 9:55 PM, KOSAKI Motohiro
<kosaki.motohiro@gmail.com> wrote:
> (1/17/12 9:07 PM), Venkatesh Pallipadi wrote:
>> Kernel's notion of possible cpus (from include/linux/cpumask.h)
>>   *     cpu_possible_mask- has bit 'cpu' set iff cpu is populatable
>>
>>   *  The cpu_possible_mask is fixed at boot time, as the set of CPU id's
>>   *  that it is possible might ever be plugged in at anytime during the
>>   *  life of that system boot.
>>
>>   #define num_possible_cpus()     cpumask_weight(cpu_possible_mask)
>>
>> and on x86 cpumask_weight() calls hweight64 and hweight64 (on older kernels
>> and systems with !X86_FEATURE_POPCNT) or a popcnt based alternative.
>>
>> i.e, We needlessly go through this mask based calculation everytime
>> num_possible_cpus() is called.
>>
>> The problem is there with cpu_online_mask() as well, which is fixed value at
>> boot time in !CONFIG_HOTPLUG_CPU case and should not change that often even
>> in HOTPLUG case.
>>
>> Though most of the callers of these two routines are init time (with few
>> exceptions of runtime calls), it is cleaner to use variables
>> and not go through this repeated mask based calculation.
>>
>> Signed-off-by: Venkatesh Pallipadi<venki@google.com>
>> ---
>>   include/linux/cpumask.h |    8 ++++++--
>>   kernel/cpu.c            |    9 +++++++++
>>   2 files changed, 15 insertions(+), 2 deletions(-)
>>
>> diff --git a/include/linux/cpumask.h b/include/linux/cpumask.h
>> index 4f7a632..2eb04dd 100644
>> --- a/include/linux/cpumask.h
>> +++ b/include/linux/cpumask.h
>> @@ -80,9 +80,13 @@ extern const struct cpumask *const cpu_online_mask;
>>   extern const struct cpumask *const cpu_present_mask;
>>   extern const struct cpumask *const cpu_active_mask;
>>
>> +extern int nr_online_cpus;
>> +
>>   #if NR_CPUS>  1
>> -#define num_online_cpus()    cpumask_weight(cpu_online_mask)
>> -#define num_possible_cpus()  cpumask_weight(cpu_possible_mask)
>> +
>> +#define num_online_cpus()    (nr_online_cpus)
>> +#define num_possible_cpus()  (nr_cpu_ids)
>
> nr_cpu_ids mean maximum cpu id of cpus. if cpu id are sparse, maximum id
> doesn't match number of cpus.
>

Yes. But will it be sparse in any arch? I saw some of the users of
num_possible_cpus() doing things like allocating a buffer for that
size and then indexing it using get_cpu(). So, I thought it would be
better to use the existing nr_cpu_ids instead of inventing another
variable. If indeed any arch is depending on this being sparse, we can
have a new variable similar to num_possible_cpus and also audit all
users of num_possible_cpus to see whether they should be using
nr_cpu_ids instead.

>
>
>> +
>>   #define num_present_cpus()  cpumask_weight(cpu_present_mask)
>>   #define num_active_cpus()   cpumask_weight(cpu_active_mask)
>>   #define cpu_online(cpu)             cpumask_test_cpu((cpu), cpu_online_mask)
>> diff --git a/kernel/cpu.c b/kernel/cpu.c
>> index 2060c6e..eed2169 100644
>> --- a/kernel/cpu.c
>> +++ b/kernel/cpu.c
>> @@ -622,6 +622,13 @@ static DECLARE_BITMAP(cpu_active_bits, CONFIG_NR_CPUS) __read_mostly;
>>   const struct cpumask *const cpu_active_mask = to_cpumask(cpu_active_bits);
>>   EXPORT_SYMBOL(cpu_active_mask);
>>
>> +#ifdef CONFIG_HOTPLUG_CPU
>> +int nr_online_cpus;
>> +#else
>> +int nr_online_cpus __read_mostly;
>> +#endif
>> +EXPORT_SYMBOL(nr_online_cpus);
>
> You can always mark this to __read_mostly. other cpu hotplug stuff do so.
> Because of, I guess, cpu hotplug developers don't think hotplugging is
> frequently event.
>

OK. Agree.

>
>>   void set_cpu_possible(unsigned int cpu, bool possible)
>>   {
>>       if (possible)
>> @@ -644,6 +651,8 @@ void set_cpu_online(unsigned int cpu, bool online)
>>               cpumask_set_cpu(cpu, to_cpumask(cpu_online_bits));
>>       else
>>               cpumask_clear_cpu(cpu, to_cpumask(cpu_online_bits));
>> +
>> +     nr_online_cpus = cpumask_weight(cpu_online_mask);
>>   }
>
> I like this change. :)
>
>

Thanks,
Venki

Re: [PATCH] Avoid mask based num_possible_cpus and num_online_cpus

[KOSAKI Motohiro]

(1/18/12 1:52 PM), Venki Pallipadi wrote:
> On Tue, Jan 17, 2012 at 9:55 PM, KOSAKI Motohiro
> <kosaki.motohiro@gmail.com>  wrote:
>> (1/17/12 9:07 PM), Venkatesh Pallipadi wrote:
>>> Kernel's notion of possible cpus (from include/linux/cpumask.h)
>>>    *     cpu_possible_mask- has bit 'cpu' set iff cpu is populatable
>>>
>>>    *  The cpu_possible_mask is fixed at boot time, as the set of CPU id's
>>>    *  that it is possible might ever be plugged in at anytime during the
>>>    *  life of that system boot.
>>>
>>>    #define num_possible_cpus()     cpumask_weight(cpu_possible_mask)
>>>
>>> and on x86 cpumask_weight() calls hweight64 and hweight64 (on older kernels
>>> and systems with !X86_FEATURE_POPCNT) or a popcnt based alternative.
>>>
>>> i.e, We needlessly go through this mask based calculation everytime
>>> num_possible_cpus() is called.
>>>
>>> The problem is there with cpu_online_mask() as well, which is fixed value at
>>> boot time in !CONFIG_HOTPLUG_CPU case and should not change that often even
>>> in HOTPLUG case.
>>>
>>> Though most of the callers of these two routines are init time (with few
>>> exceptions of runtime calls), it is cleaner to use variables
>>> and not go through this repeated mask based calculation.
>>>
>>> Signed-off-by: Venkatesh Pallipadi<venki@google.com>
>>> ---
>>>    include/linux/cpumask.h |    8 ++++++--
>>>    kernel/cpu.c            |    9 +++++++++
>>>    2 files changed, 15 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/include/linux/cpumask.h b/include/linux/cpumask.h
>>> index 4f7a632..2eb04dd 100644
>>> --- a/include/linux/cpumask.h
>>> +++ b/include/linux/cpumask.h
>>> @@ -80,9 +80,13 @@ extern const struct cpumask *const cpu_online_mask;
>>>    extern const struct cpumask *const cpu_present_mask;
>>>    extern const struct cpumask *const cpu_active_mask;
>>>
>>> +extern int nr_online_cpus;
>>> +
>>>    #if NR_CPUS>    1
>>> -#define num_online_cpus()    cpumask_weight(cpu_online_mask)
>>> -#define num_possible_cpus()  cpumask_weight(cpu_possible_mask)
>>> +
>>> +#define num_online_cpus()    (nr_online_cpus)
>>> +#define num_possible_cpus()  (nr_cpu_ids)
>>
>> nr_cpu_ids mean maximum cpu id of cpus. if cpu id are sparse, maximum id
>> doesn't match number of cpus.
>>
>
> Yes. But will it be sparse in any arch? I saw some of the users of
> num_possible_cpus() doing things like allocating a buffer for that
> size and then indexing it using get_cpu(). So, I thought it would be
> better to use the existing nr_cpu_ids instead of inventing another
> variable. If indeed any arch is depending on this being sparse, we can
> have a new variable similar to num_possible_cpus and also audit all
> users of num_possible_cpus to see whether they should be using
> nr_cpu_ids instead.

If my remember is correct, sparc does.

[PATCH] Avoid mask based num_possible_cpus and num_online_cpus

[Venkatesh Pallipadi]

Does this look better? Will send separate patch to fix code
using num_possible_cpus() when they actually need nr_cpu_ids.



Kernel's notion of possible cpus (from include/linux/cpumask.h)
 *     cpu_possible_mask- has bit 'cpu' set iff cpu is populatable

 *  The cpu_possible_mask is fixed at boot time, as the set of CPU id's
 *  that it is possible might ever be plugged in at anytime during the
 *  life of that system boot.

 #define num_possible_cpus()     cpumask_weight(cpu_possible_mask)

and on x86 cpumask_weight() calls hweight64 and hweight64 (on older kernels
and systems with !X86_FEATURE_POPCNT) or a popcnt based alternative.

i.e, We needlessly go through this mask based calculation everytime
num_possible_cpus() is called.

The problem is there with cpu_online_mask() as well, which is fixed value at
boot time in !CONFIG_HOTPLUG_CPU case and should not change that often even
in HOTPLUG case.

Though most of the callers of these two routines are init time (with few
exceptions of runtime calls), it is cleaner to use variables
and not go through this repeated mask based calculation.

Signed-off-by: Venkatesh Pallipadi <venki@google.com>
---
 include/linux/cpumask.h |   10 ++++++++--
 kernel/cpu.c            |    5 +++++
 kernel/smp.c            |    4 ++++
 3 files changed, 17 insertions(+), 2 deletions(-)

diff --git a/include/linux/cpumask.h b/include/linux/cpumask.h
index 4f7a632..ac3113b 100644
--- a/include/linux/cpumask.h
+++ b/include/linux/cpumask.h
@@ -23,10 +23,14 @@ typedef struct cpumask { DECLARE_BITMAP(bits, NR_CPUS); } cpumask_t;
 
 #if NR_CPUS == 1
 #define nr_cpu_ids		1
+#define nr_possible_cpus	1
 #else
 extern int nr_cpu_ids;
+extern int nr_possible_cpus;
 #endif
 
+extern int nr_online_cpus;
+
 #ifdef CONFIG_CPUMASK_OFFSTACK
 /* Assuming NR_CPUS is huge, a runtime limit is more efficient.  Also,
  * not all bits may be allocated. */
@@ -81,8 +85,10 @@ extern const struct cpumask *const cpu_present_mask;
 extern const struct cpumask *const cpu_active_mask;
 
 #if NR_CPUS > 1
-#define num_online_cpus()	cpumask_weight(cpu_online_mask)
-#define num_possible_cpus()	cpumask_weight(cpu_possible_mask)
+
+#define num_online_cpus()	(nr_online_cpus)
+#define num_possible_cpus()	(nr_possible_cpus)
+
 #define num_present_cpus()	cpumask_weight(cpu_present_mask)
 #define num_active_cpus()	cpumask_weight(cpu_active_mask)
 #define cpu_online(cpu)		cpumask_test_cpu((cpu), cpu_online_mask)
diff --git a/kernel/cpu.c b/kernel/cpu.c
index 2060c6e..f179baa 100644
--- a/kernel/cpu.c
+++ b/kernel/cpu.c
@@ -622,6 +622,9 @@ static DECLARE_BITMAP(cpu_active_bits, CONFIG_NR_CPUS) __read_mostly;
 const struct cpumask *const cpu_active_mask = to_cpumask(cpu_active_bits);
 EXPORT_SYMBOL(cpu_active_mask);
 
+int nr_online_cpus __read_mostly;
+EXPORT_SYMBOL(nr_online_cpus);
+
 void set_cpu_possible(unsigned int cpu, bool possible)
 {
 	if (possible)
@@ -644,6 +647,8 @@ void set_cpu_online(unsigned int cpu, bool online)
 		cpumask_set_cpu(cpu, to_cpumask(cpu_online_bits));
 	else
 		cpumask_clear_cpu(cpu, to_cpumask(cpu_online_bits));
+
+	nr_online_cpus = cpumask_weight(cpu_online_mask);
 }
 
 void set_cpu_active(unsigned int cpu, bool active)
diff --git a/kernel/smp.c b/kernel/smp.c
index db197d6..106e519 100644
--- a/kernel/smp.c
+++ b/kernel/smp.c
@@ -658,10 +658,14 @@ early_param("maxcpus", maxcpus);
 int nr_cpu_ids __read_mostly = NR_CPUS;
 EXPORT_SYMBOL(nr_cpu_ids);
 
+int nr_possible_cpus __read_mostly = NR_CPUS;
+EXPORT_SYMBOL(nr_possible_cpus);
+
 /* An arch may set nr_cpu_ids earlier if needed, so this would be redundant */
 void __init setup_nr_cpu_ids(void)
 {
 	nr_cpu_ids = find_last_bit(cpumask_bits(cpu_possible_mask),NR_CPUS) + 1;
+	nr_possible_cpus = cpumask_weight(cpu_possible_mask);
 }
 
 /* Called by boot processor to activate the rest. */
-- 
1.7.7.3

Re: [PATCH] Avoid mask based num_possible_cpus and num_online_cpus

[KOSAKI Motohiro]

On 1/19/2012 3:01 PM, Venkatesh Pallipadi wrote:
> Does this look better? Will send separate patch to fix code
> using num_possible_cpus() when they actually need nr_cpu_ids.

Sound ok to me. but I have two requests.

 - Please mesure how much time (or cycle) spented by cpumask_weight(cpu_possible_mask).
 - After your patch, nr_possible_cpus() return different value from before.
   Please verify this change doesn't makes any side effect.

Re: [PATCH] Avoid mask based num_possible_cpus and num_online_cpus

[Srivatsa S. Bhat]

Hi,

On 01/20/2012 01:31 AM, Venkatesh Pallipadi wrote:

> Does this look better? Will send separate patch to fix code
> using num_possible_cpus() when they actually need nr_cpu_ids.
> 
> 
> 
> Kernel's notion of possible cpus (from include/linux/cpumask.h)
>  *     cpu_possible_mask- has bit 'cpu' set iff cpu is populatable
> 
>  *  The cpu_possible_mask is fixed at boot time, as the set of CPU id's
>  *  that it is possible might ever be plugged in at anytime during the
>  *  life of that system boot.
> 
>  #define num_possible_cpus()     cpumask_weight(cpu_possible_mask)
> 
> and on x86 cpumask_weight() calls hweight64 and hweight64 (on older kernels
> and systems with !X86_FEATURE_POPCNT) or a popcnt based alternative.
> 
> i.e, We needlessly go through this mask based calculation everytime
> num_possible_cpus() is called.
> 
> The problem is there with cpu_online_mask() as well, which is fixed value at
> boot time in !CONFIG_HOTPLUG_CPU case and should not change that often even
> in HOTPLUG case.
> 
> Though most of the callers of these two routines are init time (with few
> exceptions of runtime calls), it is cleaner to use variables
> and not go through this repeated mask based calculation.
> 
> Signed-off-by: Venkatesh Pallipadi <venki@google.com>
> ---
>  include/linux/cpumask.h |   10 ++++++++--
>  kernel/cpu.c            |    5 +++++
>  kernel/smp.c            |    4 ++++
>  3 files changed, 17 insertions(+), 2 deletions(-)
> 
> diff --git a/include/linux/cpumask.h b/include/linux/cpumask.h
> index 4f7a632..ac3113b 100644
> --- a/include/linux/cpumask.h
> +++ b/include/linux/cpumask.h
> @@ -23,10 +23,14 @@ typedef struct cpumask { DECLARE_BITMAP(bits, NR_CPUS); } cpumask_t;
> 
>  #if NR_CPUS == 1
>  #define nr_cpu_ids		1
> +#define nr_possible_cpus	1
>  #else
>  extern int nr_cpu_ids;
> +extern int nr_possible_cpus;
>  #endif
> 
> +extern int nr_online_cpus;
> +
>  #ifdef CONFIG_CPUMASK_OFFSTACK
>  /* Assuming NR_CPUS is huge, a runtime limit is more efficient.  Also,
>   * not all bits may be allocated. */
> @@ -81,8 +85,10 @@ extern const struct cpumask *const cpu_present_mask;
>  extern const struct cpumask *const cpu_active_mask;
> 
>  #if NR_CPUS > 1
> -#define num_online_cpus()	cpumask_weight(cpu_online_mask)
> -#define num_possible_cpus()	cpumask_weight(cpu_possible_mask)
> +
> +#define num_online_cpus()	(nr_online_cpus)
> +#define num_possible_cpus()	(nr_possible_cpus)
> +
>  #define num_present_cpus()	cpumask_weight(cpu_present_mask)
>  #define num_active_cpus()	cpumask_weight(cpu_active_mask)
>  #define cpu_online(cpu)		cpumask_test_cpu((cpu), cpu_online_mask)
> diff --git a/kernel/cpu.c b/kernel/cpu.c
> index 2060c6e..f179baa 100644
> --- a/kernel/cpu.c
> +++ b/kernel/cpu.c
> @@ -622,6 +622,9 @@ static DECLARE_BITMAP(cpu_active_bits, CONFIG_NR_CPUS) __read_mostly;
>  const struct cpumask *const cpu_active_mask = to_cpumask(cpu_active_bits);
>  EXPORT_SYMBOL(cpu_active_mask);
> 
> +int nr_online_cpus __read_mostly;
> +EXPORT_SYMBOL(nr_online_cpus);
> +
>  void set_cpu_possible(unsigned int cpu, bool possible)
>  {
>  	if (possible)


Did you forget to add:

	nr_possible_cpus = cpumask_weight(cpu_possible_mask);

inside set_cpu_possible() ?


> @@ -644,6 +647,8 @@ void set_cpu_online(unsigned int cpu, bool online)
>  		cpumask_set_cpu(cpu, to_cpumask(cpu_online_bits));
>  	else
>  		cpumask_clear_cpu(cpu, to_cpumask(cpu_online_bits));
> +
> +	nr_online_cpus = cpumask_weight(cpu_online_mask);
>  }
> 
>  void set_cpu_active(unsigned int cpu, bool active)
> diff --git a/kernel/smp.c b/kernel/smp.c
> index db197d6..106e519 100644
> --- a/kernel/smp.c
> +++ b/kernel/smp.c
> @@ -658,10 +658,14 @@ early_param("maxcpus", maxcpus);
>  int nr_cpu_ids __read_mostly = NR_CPUS;
>  EXPORT_SYMBOL(nr_cpu_ids);
> 
> +int nr_possible_cpus __read_mostly = NR_CPUS;
> +EXPORT_SYMBOL(nr_possible_cpus);
> +
>  /* An arch may set nr_cpu_ids earlier if needed, so this would be redundant */
>  void __init setup_nr_cpu_ids(void)
>  {
>  	nr_cpu_ids = find_last_bit(cpumask_bits(cpu_possible_mask),NR_CPUS) + 1;
> +	nr_possible_cpus = cpumask_weight(cpu_possible_mask);
>  }
> 
>  /* Called by boot processor to activate the rest. */


Regards,
Srivatsa S. Bhat

Re: [PATCH] Avoid mask based num_possible_cpus and num_online_cpus

[Venki Pallipadi]

On Thu, Jan 19, 2012 at 12:43 PM, Srivatsa S. Bhat
<srivatsa.bhat@linux.vnet.ibm.com> wrote:
> Hi,
>
> On 01/20/2012 01:31 AM, Venkatesh Pallipadi wrote:
>
>> Does this look better? Will send separate patch to fix code
>> using num_possible_cpus() when they actually need nr_cpu_ids.
>>
>>
>>
>> Kernel's notion of possible cpus (from include/linux/cpumask.h)
>>  *     cpu_possible_mask- has bit 'cpu' set iff cpu is populatable
>>
>>  *  The cpu_possible_mask is fixed at boot time, as the set of CPU id's
>>  *  that it is possible might ever be plugged in at anytime during the
>>  *  life of that system boot.
>>
>>  #define num_possible_cpus()     cpumask_weight(cpu_possible_mask)
>>
>> and on x86 cpumask_weight() calls hweight64 and hweight64 (on older kernels
>> and systems with !X86_FEATURE_POPCNT) or a popcnt based alternative.
>>
>> i.e, We needlessly go through this mask based calculation everytime
>> num_possible_cpus() is called.
>>
>> The problem is there with cpu_online_mask() as well, which is fixed value at
>> boot time in !CONFIG_HOTPLUG_CPU case and should not change that often even
>> in HOTPLUG case.
>>
>> Though most of the callers of these two routines are init time (with few
>> exceptions of runtime calls), it is cleaner to use variables
>> and not go through this repeated mask based calculation.
>>
>> Signed-off-by: Venkatesh Pallipadi <venki@google.com>
>> ---
>>  include/linux/cpumask.h |   10 ++++++++--
>>  kernel/cpu.c            |    5 +++++
>>  kernel/smp.c            |    4 ++++
>>  3 files changed, 17 insertions(+), 2 deletions(-)
>>
>> diff --git a/include/linux/cpumask.h b/include/linux/cpumask.h
>> index 4f7a632..ac3113b 100644
>> --- a/include/linux/cpumask.h
>> +++ b/include/linux/cpumask.h
>> @@ -23,10 +23,14 @@ typedef struct cpumask { DECLARE_BITMAP(bits, NR_CPUS); } cpumask_t;
>>
>>  #if NR_CPUS == 1
>>  #define nr_cpu_ids           1
>> +#define nr_possible_cpus     1
>>  #else
>>  extern int nr_cpu_ids;
>> +extern int nr_possible_cpus;
>>  #endif
>>
>> +extern int nr_online_cpus;
>> +
>>  #ifdef CONFIG_CPUMASK_OFFSTACK
>>  /* Assuming NR_CPUS is huge, a runtime limit is more efficient.  Also,
>>   * not all bits may be allocated. */
>> @@ -81,8 +85,10 @@ extern const struct cpumask *const cpu_present_mask;
>>  extern const struct cpumask *const cpu_active_mask;
>>
>>  #if NR_CPUS > 1
>> -#define num_online_cpus()    cpumask_weight(cpu_online_mask)
>> -#define num_possible_cpus()  cpumask_weight(cpu_possible_mask)
>> +
>> +#define num_online_cpus()    (nr_online_cpus)
>> +#define num_possible_cpus()  (nr_possible_cpus)
>> +
>>  #define num_present_cpus()   cpumask_weight(cpu_present_mask)
>>  #define num_active_cpus()    cpumask_weight(cpu_active_mask)
>>  #define cpu_online(cpu)              cpumask_test_cpu((cpu), cpu_online_mask)
>> diff --git a/kernel/cpu.c b/kernel/cpu.c
>> index 2060c6e..f179baa 100644
>> --- a/kernel/cpu.c
>> +++ b/kernel/cpu.c
>> @@ -622,6 +622,9 @@ static DECLARE_BITMAP(cpu_active_bits, CONFIG_NR_CPUS) __read_mostly;
>>  const struct cpumask *const cpu_active_mask = to_cpumask(cpu_active_bits);
>>  EXPORT_SYMBOL(cpu_active_mask);
>>
>> +int nr_online_cpus __read_mostly;
>> +EXPORT_SYMBOL(nr_online_cpus);
>> +
>>  void set_cpu_possible(unsigned int cpu, bool possible)
>>  {
>>       if (possible)
>
>
> Did you forget to add:
>
>        nr_possible_cpus = cpumask_weight(cpu_possible_mask);
>
> inside set_cpu_possible() ?

No. That was intentional as I have that coupled with nr_cpu_ids and
set once after all the bits are set in setup_nr_cpu_ids() instead of
doing for each bit set.

>
>
>> @@ -644,6 +647,8 @@ void set_cpu_online(unsigned int cpu, bool online)
>>               cpumask_set_cpu(cpu, to_cpumask(cpu_online_bits));
>>       else
>>               cpumask_clear_cpu(cpu, to_cpumask(cpu_online_bits));
>> +
>> +     nr_online_cpus = cpumask_weight(cpu_online_mask);
>>  }
>>
>>  void set_cpu_active(unsigned int cpu, bool active)
>> diff --git a/kernel/smp.c b/kernel/smp.c
>> index db197d6..106e519 100644
>> --- a/kernel/smp.c
>> +++ b/kernel/smp.c
>> @@ -658,10 +658,14 @@ early_param("maxcpus", maxcpus);
>>  int nr_cpu_ids __read_mostly = NR_CPUS;
>>  EXPORT_SYMBOL(nr_cpu_ids);
>>
>> +int nr_possible_cpus __read_mostly = NR_CPUS;
>> +EXPORT_SYMBOL(nr_possible_cpus);
>> +
>>  /* An arch may set nr_cpu_ids earlier if needed, so this would be redundant */
>>  void __init setup_nr_cpu_ids(void)
>>  {
>>       nr_cpu_ids = find_last_bit(cpumask_bits(cpu_possible_mask),NR_CPUS) + 1;
>> +     nr_possible_cpus = cpumask_weight(cpu_possible_mask);
>>  }
>>
>>  /* Called by boot processor to activate the rest. */
>
>
> Regards,
> Srivatsa S. Bhat
>

Re: [PATCH] Avoid mask based num_possible_cpus and num_online_cpus

[KOSAKI Motohiro]

>>> +int nr_online_cpus __read_mostly;
>>> +EXPORT_SYMBOL(nr_online_cpus);
>>> +
>>>  void set_cpu_possible(unsigned int cpu, bool possible)
>>>  {
>>>       if (possible)
>>
>>
>> Did you forget to add:
>>
>>        nr_possible_cpus = cpumask_weight(cpu_possible_mask);
>>
>> inside set_cpu_possible() ?
>
> No. That was intentional as I have that coupled with nr_cpu_ids and
> set once after all the bits are set in setup_nr_cpu_ids() instead of
> doing for each bit set.

But, Srivatsa's way seems more safer, no? Is there any advantage to make couple
with nr_cpu_ids?

Re: [PATCH] Avoid mask based num_possible_cpus and num_online_cpus

[Venki Pallipadi]

On Fri, Jan 20, 2012 at 3:45 PM, KOSAKI Motohiro
<kosaki.motohiro@gmail.com> wrote:
>>>> +int nr_online_cpus __read_mostly;
>>>> +EXPORT_SYMBOL(nr_online_cpus);
>>>> +
>>>>  void set_cpu_possible(unsigned int cpu, bool possible)
>>>>  {
>>>>       if (possible)
>>>
>>>
>>> Did you forget to add:
>>>
>>>        nr_possible_cpus = cpumask_weight(cpu_possible_mask);
>>>
>>> inside set_cpu_possible() ?
>>
>> No. That was intentional as I have that coupled with nr_cpu_ids and
>> set once after all the bits are set in setup_nr_cpu_ids() instead of
>> doing for each bit set.
>
> But, Srivatsa's way seems more safer, no? Is there any advantage to make couple
> with nr_cpu_ids?

I think it is a tradeoff between safer and cleaner :). infact, that's
how I had coded the patch first. But, then I changed it to be in sync
with nr_cpu_ids as it seemed a bit cleaner (and also to make sure 2048
CPU guys won't come after me for doing the mask calculation 2048 times
during the boot).

Re: [PATCH] Avoid mask based num_possible_cpus and num_online_cpus

[Venki Pallipadi]

On Thu, Jan 19, 2012 at 12:40 PM, KOSAKI Motohiro
<kosaki.motohiro@jp.fujitsu.com> wrote:
> On 1/19/2012 3:01 PM, Venkatesh Pallipadi wrote:
>> Does this look better? Will send separate patch to fix code
>> using num_possible_cpus() when they actually need nr_cpu_ids.
>
> Sound ok to me. but I have two requests.
>
>  - Please mesure how much time (or cycle) spented by cpumask_weight(cpu_possible_mask).

On two of the recentish x86 server CPUs, __sw_hweight64() base
mask_weight() takes ~300 cycles. popcnt instruction based weight takes
<10 cycles.

>  - After your patch, nr_possible_cpus() return different value from before.

Yes. I have tested this on x86 and haven't seen any problem.

>   Please verify this change doesn't makes any side effect.
>
>

Re: [PATCH] Avoid mask based num_possible_cpus and num_online_cpus

[Srivatsa S. Bhat]

On 01/21/2012 05:25 AM, Venki Pallipadi wrote:

> On Fri, Jan 20, 2012 at 3:45 PM, KOSAKI Motohiro
> <kosaki.motohiro@gmail.com> wrote:
>>>>> +int nr_online_cpus __read_mostly;
>>>>> +EXPORT_SYMBOL(nr_online_cpus);
>>>>> +
>>>>>  void set_cpu_possible(unsigned int cpu, bool possible)
>>>>>  {
>>>>>       if (possible)
>>>>
>>>>
>>>> Did you forget to add:
>>>>
>>>>        nr_possible_cpus = cpumask_weight(cpu_possible_mask);
>>>>
>>>> inside set_cpu_possible() ?
>>>
>>> No. That was intentional as I have that coupled with nr_cpu_ids and
>>> set once after all the bits are set in setup_nr_cpu_ids() instead of
>>> doing for each bit set.
>>
>> But, Srivatsa's way seems more safer, no? Is there any advantage to make couple
>> with nr_cpu_ids?
> 
> I think it is a tradeoff between safer and cleaner :). infact, that's
> how I had coded the patch first. But, then I changed it to be in sync
> with nr_cpu_ids as it seemed a bit cleaner (and also to make sure 2048
> CPU guys won't come after me for doing the mask calculation 2048 times
> during the boot).
> 


I knew you were trying to optimize further when I saw your patch. That's
precisely the reason I cross-checked the code to ensure that the optimization
didn't go beyond correctness :)

And this is what I found:

start_kernel()
  setup_nr_cpu_ids() // This is not the end of setting up cpu_possible_mask
  rest_init()
    kernel_init()
      smp_prepare_cpus();

And on x86, this becomes:
      native_smp_prepare_cpus();
        smp_sanity_check(); // cpu_possible_mask & nr_cpu_ids can change here!
           ^^^^^^^^^

And there is another place where things can change:
prefill_possible_map(). But this is called in setup_arch(), which is called
before setup_nr_cpu_ids(). So we need not worry about this.

(Btw, I checked only the x86 arch. Not sure how other architectures handle
things.)

Regards,
Srivatsa S. Bhat
IBM Linux Technology Center

Re: [PATCH] Avoid mask based num_possible_cpus and num_online_cpus

[Venki Pallipadi]

On Sun, Jan 22, 2012 at 9:22 PM, Srivatsa S. Bhat
<srivatsa.bhat@linux.vnet.ibm.com> wrote:
> On 01/21/2012 05:25 AM, Venki Pallipadi wrote:
>
>> On Fri, Jan 20, 2012 at 3:45 PM, KOSAKI Motohiro
>> <kosaki.motohiro@gmail.com> wrote:
>>>>>> +int nr_online_cpus __read_mostly;
>>>>>> +EXPORT_SYMBOL(nr_online_cpus);
>>>>>> +
>>>>>>  void set_cpu_possible(unsigned int cpu, bool possible)
>>>>>>  {
>>>>>>       if (possible)
>>>>>
>>>>>
>>>>> Did you forget to add:
>>>>>
>>>>>        nr_possible_cpus = cpumask_weight(cpu_possible_mask);
>>>>>
>>>>> inside set_cpu_possible() ?
>>>>
>>>> No. That was intentional as I have that coupled with nr_cpu_ids and
>>>> set once after all the bits are set in setup_nr_cpu_ids() instead of
>>>> doing for each bit set.
>>>
>>> But, Srivatsa's way seems more safer, no? Is there any advantage to make couple
>>> with nr_cpu_ids?
>>
>> I think it is a tradeoff between safer and cleaner :). infact, that's
>> how I had coded the patch first. But, then I changed it to be in sync
>> with nr_cpu_ids as it seemed a bit cleaner (and also to make sure 2048
>> CPU guys won't come after me for doing the mask calculation 2048 times
>> during the boot).
>>
>
>
> I knew you were trying to optimize further when I saw your patch. That's
> precisely the reason I cross-checked the code to ensure that the optimization
> didn't go beyond correctness :)
>
> And this is what I found:
>
> start_kernel()
>  setup_nr_cpu_ids() // This is not the end of setting up cpu_possible_mask
>  rest_init()
>    kernel_init()
>      smp_prepare_cpus();
>
> And on x86, this becomes:
>      native_smp_prepare_cpus();
>        smp_sanity_check(); // cpu_possible_mask & nr_cpu_ids can change here!
>           ^^^^^^^^^
>

Ack. Thanks for pointing this out. I missed this in my testing as this is under
if !defined(CONFIG_X86_BIGSMP) && defined(CONFIG_X86_32)

Proper way to handle this would be to call
setup_nr_cpu_ids();
After set_cpu_possible() loop.

I did a grep for any other place where nr_cpu_ids may be getting
written. And only other place I find
arch/powerpc/platforms/iseries/setup.c: nr_cpu_ids = max(nr_cpu_ids, 64);
but it does not have a accompanying set_cpu_possible(). So, the change
here should not affect this.

-Venki

> And there is another place where things can change:
> prefill_possible_map(). But this is called in setup_arch(), which is called
> before setup_nr_cpu_ids(). So we need not worry about this.
>
> (Btw, I checked only the x86 arch. Not sure how other architectures handle
> things.)
>
> Regards,
> Srivatsa S. Bhat
> IBM Linux Technology Center
>

[PATCH] Avoid mask based num_possible_cpus and num_online_cpus -v3

[Venkatesh Pallipadi]

Kernel's notion of possible cpus (from include/linux/cpumask.h)
 *     cpu_possible_mask- has bit 'cpu' set iff cpu is populatable

 *  The cpu_possible_mask is fixed at boot time, as the set of CPU id's
 *  that it is possible might ever be plugged in at anytime during the
 *  life of that system boot.

 #define num_possible_cpus()     cpumask_weight(cpu_possible_mask)

and on x86 cpumask_weight() calls hweight64 and hweight64 (on older kernels
and systems with !X86_FEATURE_POPCNT) or a popcnt based alternative.

i.e, We needlessly go through this mask based calculation everytime
num_possible_cpus() is called.

The problem is there with cpu_online_mask() as well, which is fixed value at
boot time in !CONFIG_HOTPLUG_CPU case and should not change that often even
in HOTPLUG case.

Though most of the callers of these two routines are init time (with few
exceptions of runtime calls), it is cleaner to use variables
and not go through this repeated mask based calculation.

Signed-off-by: Venkatesh Pallipadi <venki@google.com>
---
 arch/x86/kernel/smpboot.c |    2 +-
 include/linux/cpumask.h   |   10 ++++++++--
 kernel/cpu.c              |    5 +++++
 kernel/smp.c              |    4 ++++
 4 files changed, 18 insertions(+), 3 deletions(-)

diff --git a/arch/x86/kernel/smpboot.c b/arch/x86/kernel/smpboot.c
index 66d250c..f87fcde 100644
--- a/arch/x86/kernel/smpboot.c
+++ b/arch/x86/kernel/smpboot.c
@@ -947,7 +947,7 @@ static int __init smp_sanity_check(unsigned max_cpus)
 			nr++;
 		}
 
-		nr_cpu_ids = 8;
+		setup_nr_cpu_ids();
 	}
 #endif
 
diff --git a/include/linux/cpumask.h b/include/linux/cpumask.h
index 4f7a632..ac3113b 100644
--- a/include/linux/cpumask.h
+++ b/include/linux/cpumask.h
@@ -23,10 +23,14 @@ typedef struct cpumask { DECLARE_BITMAP(bits, NR_CPUS); } cpumask_t;
 
 #if NR_CPUS == 1
 #define nr_cpu_ids		1
+#define nr_possible_cpus	1
 #else
 extern int nr_cpu_ids;
+extern int nr_possible_cpus;
 #endif
 
+extern int nr_online_cpus;
+
 #ifdef CONFIG_CPUMASK_OFFSTACK
 /* Assuming NR_CPUS is huge, a runtime limit is more efficient.  Also,
  * not all bits may be allocated. */
@@ -81,8 +85,10 @@ extern const struct cpumask *const cpu_present_mask;
 extern const struct cpumask *const cpu_active_mask;
 
 #if NR_CPUS > 1
-#define num_online_cpus()	cpumask_weight(cpu_online_mask)
-#define num_possible_cpus()	cpumask_weight(cpu_possible_mask)
+
+#define num_online_cpus()	(nr_online_cpus)
+#define num_possible_cpus()	(nr_possible_cpus)
+
 #define num_present_cpus()	cpumask_weight(cpu_present_mask)
 #define num_active_cpus()	cpumask_weight(cpu_active_mask)
 #define cpu_online(cpu)		cpumask_test_cpu((cpu), cpu_online_mask)
diff --git a/kernel/cpu.c b/kernel/cpu.c
index 2060c6e..f179baa 100644
--- a/kernel/cpu.c
+++ b/kernel/cpu.c
@@ -622,6 +622,9 @@ static DECLARE_BITMAP(cpu_active_bits, CONFIG_NR_CPUS) __read_mostly;
 const struct cpumask *const cpu_active_mask = to_cpumask(cpu_active_bits);
 EXPORT_SYMBOL(cpu_active_mask);
 
+int nr_online_cpus __read_mostly;
+EXPORT_SYMBOL(nr_online_cpus);
+
 void set_cpu_possible(unsigned int cpu, bool possible)
 {
 	if (possible)
@@ -644,6 +647,8 @@ void set_cpu_online(unsigned int cpu, bool online)
 		cpumask_set_cpu(cpu, to_cpumask(cpu_online_bits));
 	else
 		cpumask_clear_cpu(cpu, to_cpumask(cpu_online_bits));
+
+	nr_online_cpus = cpumask_weight(cpu_online_mask);
 }
 
 void set_cpu_active(unsigned int cpu, bool active)
diff --git a/kernel/smp.c b/kernel/smp.c
index db197d6..106e519 100644
--- a/kernel/smp.c
+++ b/kernel/smp.c
@@ -658,10 +658,14 @@ early_param("maxcpus", maxcpus);
 int nr_cpu_ids __read_mostly = NR_CPUS;
 EXPORT_SYMBOL(nr_cpu_ids);
 
+int nr_possible_cpus __read_mostly = NR_CPUS;
+EXPORT_SYMBOL(nr_possible_cpus);
+
 /* An arch may set nr_cpu_ids earlier if needed, so this would be redundant */
 void __init setup_nr_cpu_ids(void)
 {
 	nr_cpu_ids = find_last_bit(cpumask_bits(cpu_possible_mask),NR_CPUS) + 1;
+	nr_possible_cpus = cpumask_weight(cpu_possible_mask);
 }
 
 /* Called by boot processor to activate the rest. */
-- 
1.7.7.3

Re: [PATCH] Avoid mask based num_possible_cpus and num_online_cpus -v3

[Srivatsa S. Bhat]

On 01/24/2012 08:04 AM, Venkatesh Pallipadi wrote:

> Kernel's notion of possible cpus (from include/linux/cpumask.h)
>  *     cpu_possible_mask- has bit 'cpu' set iff cpu is populatable
> 
>  *  The cpu_possible_mask is fixed at boot time, as the set of CPU id's
>  *  that it is possible might ever be plugged in at anytime during the
>  *  life of that system boot.
> 
>  #define num_possible_cpus()     cpumask_weight(cpu_possible_mask)
> 
> and on x86 cpumask_weight() calls hweight64 and hweight64 (on older kernels
> and systems with !X86_FEATURE_POPCNT) or a popcnt based alternative.
> 
> i.e, We needlessly go through this mask based calculation everytime
> num_possible_cpus() is called.
> 
> The problem is there with cpu_online_mask() as well, which is fixed value at
> boot time in !CONFIG_HOTPLUG_CPU case and should not change that often even
> in HOTPLUG case.
> 
> Though most of the callers of these two routines are init time (with few
> exceptions of runtime calls), it is cleaner to use variables
> and not go through this repeated mask based calculation.
> 
> Signed-off-by: Venkatesh Pallipadi <venki@google.com>
> ---
>  arch/x86/kernel/smpboot.c |    2 +-
>  include/linux/cpumask.h   |   10 ++++++++--
>  kernel/cpu.c              |    5 +++++
>  kernel/smp.c              |    4 ++++
>  4 files changed, 18 insertions(+), 3 deletions(-)
> 
> diff --git a/arch/x86/kernel/smpboot.c b/arch/x86/kernel/smpboot.c
> index 66d250c..f87fcde 100644
> --- a/arch/x86/kernel/smpboot.c
> +++ b/arch/x86/kernel/smpboot.c
> @@ -947,7 +947,7 @@ static int __init smp_sanity_check(unsigned max_cpus)
>  			nr++;
>  		}
> 
> -		nr_cpu_ids = 8;
> +		setup_nr_cpu_ids();
>  	}
>  #endif
> 
> diff --git a/include/linux/cpumask.h b/include/linux/cpumask.h
> index 4f7a632..ac3113b 100644
> --- a/include/linux/cpumask.h
> +++ b/include/linux/cpumask.h
> @@ -23,10 +23,14 @@ typedef struct cpumask { DECLARE_BITMAP(bits, NR_CPUS); } cpumask_t;
> 
>  #if NR_CPUS == 1
>  #define nr_cpu_ids		1
> +#define nr_possible_cpus	1
>  #else
>  extern int nr_cpu_ids;
> +extern int nr_possible_cpus;
>  #endif
> 
> +extern int nr_online_cpus;
> +
>  #ifdef CONFIG_CPUMASK_OFFSTACK
>  /* Assuming NR_CPUS is huge, a runtime limit is more efficient.  Also,
>   * not all bits may be allocated. */
> @@ -81,8 +85,10 @@ extern const struct cpumask *const cpu_present_mask;
>  extern const struct cpumask *const cpu_active_mask;
> 
>  #if NR_CPUS > 1
> -#define num_online_cpus()	cpumask_weight(cpu_online_mask)
> -#define num_possible_cpus()	cpumask_weight(cpu_possible_mask)
> +
> +#define num_online_cpus()	(nr_online_cpus)
> +#define num_possible_cpus()	(nr_possible_cpus)
> +
>  #define num_present_cpus()	cpumask_weight(cpu_present_mask)
>  #define num_active_cpus()	cpumask_weight(cpu_active_mask)
>  #define cpu_online(cpu)		cpumask_test_cpu((cpu), cpu_online_mask)
> diff --git a/kernel/cpu.c b/kernel/cpu.c
> index 2060c6e..f179baa 100644
> --- a/kernel/cpu.c
> +++ b/kernel/cpu.c
> @@ -622,6 +622,9 @@ static DECLARE_BITMAP(cpu_active_bits, CONFIG_NR_CPUS) __read_mostly;
>  const struct cpumask *const cpu_active_mask = to_cpumask(cpu_active_bits);
>  EXPORT_SYMBOL(cpu_active_mask);
> 
> +int nr_online_cpus __read_mostly;
> +EXPORT_SYMBOL(nr_online_cpus);
> +
>  void set_cpu_possible(unsigned int cpu, bool possible)
>  {
>  	if (possible)
> @@ -644,6 +647,8 @@ void set_cpu_online(unsigned int cpu, bool online)
>  		cpumask_set_cpu(cpu, to_cpumask(cpu_online_bits));
>  	else
>  		cpumask_clear_cpu(cpu, to_cpumask(cpu_online_bits));
> +
> +	nr_online_cpus = cpumask_weight(cpu_online_mask);
>  }
> 
>  void set_cpu_active(unsigned int cpu, bool active)
> diff --git a/kernel/smp.c b/kernel/smp.c
> index db197d6..106e519 100644
> --- a/kernel/smp.c
> +++ b/kernel/smp.c
> @@ -658,10 +658,14 @@ early_param("maxcpus", maxcpus);
>  int nr_cpu_ids __read_mostly = NR_CPUS;
>  EXPORT_SYMBOL(nr_cpu_ids);
> 
> +int nr_possible_cpus __read_mostly = NR_CPUS;
> +EXPORT_SYMBOL(nr_possible_cpus);
> +
>  /* An arch may set nr_cpu_ids earlier if needed, so this would be redundant */
>  void __init setup_nr_cpu_ids(void)
>  {
>  	nr_cpu_ids = find_last_bit(cpumask_bits(cpu_possible_mask),NR_CPUS) + 1;
> +	nr_possible_cpus = cpumask_weight(cpu_possible_mask);
>  }
> 
>  /* Called by boot processor to activate the rest. */



This patch still has problems, IMHO.

A quick grep on the source for set_cpu_possible() revealed the following
problematic areas:
1. arch/alpha/kernel/process.c: common_shutdown_1()
   set_cpu_possible() with second parameter as 'false' is used here.
   Though this is the shutdown code, a disparity between what
   num_possible_cpus() reports and what is actually there in
   cpu_possible_mask is not such a good idea, at any point in time.

2. arch/cris/arch-v32/kernel/smp.c: smp_prepare_boot_cpu()
   smp_prepare_boot_cpu() is called after setup_nr_cpu_ids().

   (Btw, I think things are really messed up in cris as it is, if I am not
   totally mistaken).

3. arch/mn10300/kernel/smp.c: smp_prepare_cpus()
   This function calls set_cpu_possible(). smp_prepare_cpus() is called in
   kernel_init(), which is much later than setup_nr_cpu_ids().

4. arch/um/kernel/smp.c: smp_prepare_cpus()
   Same as in 3.

5. As far as arch/x86/xen is concerned, I can see code such as the following,
   among other things:

    xen_smp_prepare_cpus():

    /* Restrict the possible_map according to max_cpus. */
    while ((num_possible_cpus() > 1) && (num_possible_cpus() > max_cpus)) {
            for (cpu = nr_cpu_ids - 1; !cpu_possible(cpu); cpu--)
                    continue;
            set_cpu_possible(cpu, false);
    }

If I am not missing anything obvious, applying this patch can effectively
convert the above code into an infinite loop, among other damages!

I still feel it would be safer to edit set_cpu_possible() such that
nr_possible_cpus is updated whenever cpu_possible_mask is altered.

Regards,
Srivatsa S. Bhat
IBM Linux Technology Center

Re: [PATCH] Avoid mask based num_possible_cpus and num_online_cpus -v3

[KOSAKI Motohiro]

> If I am not missing anything obvious, applying this patch can effectively
> convert the above code into an infinite loop, among other damages!
>
> I still feel it would be safer to edit set_cpu_possible() such that
> nr_possible_cpus is updated whenever cpu_possible_mask is altered.

Yup, I agree with you. The important thing is, even if we use set_cpu_possible(),
x86 performance doesn't drop. So, I don't understand why we need to take
current strategy.

Re: [PATCH] Avoid mask based num_possible_cpus and num_online_cpus -v3

[Venki Pallipadi]

On Tue, Jan 24, 2012 at 11:22 AM, Srivatsa S. Bhat
<srivatsa.bhat@linux.vnet.ibm.com> wrote:
> On 01/24/2012 08:04 AM, Venkatesh Pallipadi wrote:
>
>> Kernel's notion of possible cpus (from include/linux/cpumask.h)
>>  *     cpu_possible_mask- has bit 'cpu' set iff cpu is populatable
>>
>>  *  The cpu_possible_mask is fixed at boot time, as the set of CPU id's
>>  *  that it is possible might ever be plugged in at anytime during the
>>  *  life of that system boot.
>>
>>  #define num_possible_cpus()     cpumask_weight(cpu_possible_mask)
>>
>> and on x86 cpumask_weight() calls hweight64 and hweight64 (on older kernels
>> and systems with !X86_FEATURE_POPCNT) or a popcnt based alternative.
>>
>> i.e, We needlessly go through this mask based calculation everytime
>> num_possible_cpus() is called.
>>
>> The problem is there with cpu_online_mask() as well, which is fixed value at
>> boot time in !CONFIG_HOTPLUG_CPU case and should not change that often even
>> in HOTPLUG case.
>>
>> Though most of the callers of these two routines are init time (with few
>> exceptions of runtime calls), it is cleaner to use variables
>> and not go through this repeated mask based calculation.
>>
>> Signed-off-by: Venkatesh Pallipadi <venki@google.com>
>> ---
>>  arch/x86/kernel/smpboot.c |    2 +-
>>  include/linux/cpumask.h   |   10 ++++++++--
>>  kernel/cpu.c              |    5 +++++
>>  kernel/smp.c              |    4 ++++
>>  4 files changed, 18 insertions(+), 3 deletions(-)
>>
>> diff --git a/arch/x86/kernel/smpboot.c b/arch/x86/kernel/smpboot.c
>> index 66d250c..f87fcde 100644
>> --- a/arch/x86/kernel/smpboot.c
>> +++ b/arch/x86/kernel/smpboot.c
>> @@ -947,7 +947,7 @@ static int __init smp_sanity_check(unsigned max_cpus)
>>                       nr++;
>>               }
>>
>> -             nr_cpu_ids = 8;
>> +             setup_nr_cpu_ids();
>>       }
>>  #endif
>>
>> diff --git a/include/linux/cpumask.h b/include/linux/cpumask.h
>> index 4f7a632..ac3113b 100644
>> --- a/include/linux/cpumask.h
>> +++ b/include/linux/cpumask.h
>> @@ -23,10 +23,14 @@ typedef struct cpumask { DECLARE_BITMAP(bits, NR_CPUS); } cpumask_t;
>>
>>  #if NR_CPUS == 1
>>  #define nr_cpu_ids           1
>> +#define nr_possible_cpus     1
>>  #else
>>  extern int nr_cpu_ids;
>> +extern int nr_possible_cpus;
>>  #endif
>>
>> +extern int nr_online_cpus;
>> +
>>  #ifdef CONFIG_CPUMASK_OFFSTACK
>>  /* Assuming NR_CPUS is huge, a runtime limit is more efficient.  Also,
>>   * not all bits may be allocated. */
>> @@ -81,8 +85,10 @@ extern const struct cpumask *const cpu_present_mask;
>>  extern const struct cpumask *const cpu_active_mask;
>>
>>  #if NR_CPUS > 1
>> -#define num_online_cpus()    cpumask_weight(cpu_online_mask)
>> -#define num_possible_cpus()  cpumask_weight(cpu_possible_mask)
>> +
>> +#define num_online_cpus()    (nr_online_cpus)
>> +#define num_possible_cpus()  (nr_possible_cpus)
>> +
>>  #define num_present_cpus()   cpumask_weight(cpu_present_mask)
>>  #define num_active_cpus()    cpumask_weight(cpu_active_mask)
>>  #define cpu_online(cpu)              cpumask_test_cpu((cpu), cpu_online_mask)
>> diff --git a/kernel/cpu.c b/kernel/cpu.c
>> index 2060c6e..f179baa 100644
>> --- a/kernel/cpu.c
>> +++ b/kernel/cpu.c
>> @@ -622,6 +622,9 @@ static DECLARE_BITMAP(cpu_active_bits, CONFIG_NR_CPUS) __read_mostly;
>>  const struct cpumask *const cpu_active_mask = to_cpumask(cpu_active_bits);
>>  EXPORT_SYMBOL(cpu_active_mask);
>>
>> +int nr_online_cpus __read_mostly;
>> +EXPORT_SYMBOL(nr_online_cpus);
>> +
>>  void set_cpu_possible(unsigned int cpu, bool possible)
>>  {
>>       if (possible)
>> @@ -644,6 +647,8 @@ void set_cpu_online(unsigned int cpu, bool online)
>>               cpumask_set_cpu(cpu, to_cpumask(cpu_online_bits));
>>       else
>>               cpumask_clear_cpu(cpu, to_cpumask(cpu_online_bits));
>> +
>> +     nr_online_cpus = cpumask_weight(cpu_online_mask);
>>  }
>>
>>  void set_cpu_active(unsigned int cpu, bool active)
>> diff --git a/kernel/smp.c b/kernel/smp.c
>> index db197d6..106e519 100644
>> --- a/kernel/smp.c
>> +++ b/kernel/smp.c
>> @@ -658,10 +658,14 @@ early_param("maxcpus", maxcpus);
>>  int nr_cpu_ids __read_mostly = NR_CPUS;
>>  EXPORT_SYMBOL(nr_cpu_ids);
>>
>> +int nr_possible_cpus __read_mostly = NR_CPUS;
>> +EXPORT_SYMBOL(nr_possible_cpus);
>> +
>>  /* An arch may set nr_cpu_ids earlier if needed, so this would be redundant */
>>  void __init setup_nr_cpu_ids(void)
>>  {
>>       nr_cpu_ids = find_last_bit(cpumask_bits(cpu_possible_mask),NR_CPUS) + 1;
>> +     nr_possible_cpus = cpumask_weight(cpu_possible_mask);
>>  }
>>
>>  /* Called by boot processor to activate the rest. */
>
>
>
> This patch still has problems, IMHO.
>
> A quick grep on the source for set_cpu_possible() revealed the following
> problematic areas:
> 1. arch/alpha/kernel/process.c: common_shutdown_1()
>   set_cpu_possible() with second parameter as 'false' is used here.
>   Though this is the shutdown code, a disparity between what
>   num_possible_cpus() reports and what is actually there in
>   cpu_possible_mask is not such a good idea, at any point in time.
>
> 2. arch/cris/arch-v32/kernel/smp.c: smp_prepare_boot_cpu()
>   smp_prepare_boot_cpu() is called after setup_nr_cpu_ids().
>
>   (Btw, I think things are really messed up in cris as it is, if I am not
>   totally mistaken).
>
> 3. arch/mn10300/kernel/smp.c: smp_prepare_cpus()
>   This function calls set_cpu_possible(). smp_prepare_cpus() is called in
>   kernel_init(), which is much later than setup_nr_cpu_ids().
>
> 4. arch/um/kernel/smp.c: smp_prepare_cpus()
>   Same as in 3.
>
> 5. As far as arch/x86/xen is concerned, I can see code such as the following,
>   among other things:
>
>    xen_smp_prepare_cpus():
>
>    /* Restrict the possible_map according to max_cpus. */
>    while ((num_possible_cpus() > 1) && (num_possible_cpus() > max_cpus)) {
>            for (cpu = nr_cpu_ids - 1; !cpu_possible(cpu); cpu--)
>                    continue;
>            set_cpu_possible(cpu, false);
>    }
>
> If I am not missing anything obvious, applying this patch can effectively
> convert the above code into an infinite loop, among other damages!
>
> I still feel it would be safer to edit set_cpu_possible() such that
> nr_possible_cpus is updated whenever cpu_possible_mask is altered.
>

With the xen example above you have easily convinced me to not mess
with this code too much.
Will go back to the drawing board looking for simplest possible change.
Thanks again for being vigilant about this.

-Venki

> Regards,
> Srivatsa S. Bhat
> IBM Linux Technology Center
>

[PATCH] Avoid mask based num_possible_cpus and num_online_cpus -v4

[Venkatesh Pallipadi]

Kernel's notion of possible cpus (from include/linux/cpumask.h)
 *     cpu_possible_mask- has bit 'cpu' set iff cpu is populatable

 *  The cpu_possible_mask is fixed at boot time, as the set of CPU id's
 *  that it is possible might ever be plugged in at anytime during the
 *  life of that system boot.

 #define num_possible_cpus()     cpumask_weight(cpu_possible_mask)

and on x86 cpumask_weight() calls hweight64 and hweight64 (on older kernels
and systems with !X86_FEATURE_POPCNT) or a popcnt based alternative.

i.e, We needlessly go through this mask based calculation everytime
num_possible_cpus() is called.

The problem is there with cpu_online_mask() as well, which is fixed value at
boot time in !CONFIG_HOTPLUG_CPU case and should not change that often even
in HOTPLUG case.

Though most of the callers of these two routines are init time (with few
exceptions of runtime calls), it is cleaner to use variables
and not go through this repeated mask based calculation.

Signed-off-by: Venkatesh Pallipadi <venki@google.com>
---
 include/linux/cpumask.h |    9 +++++++--
 kernel/cpu.c            |   13 +++++++++++++
 2 files changed, 20 insertions(+), 2 deletions(-)

diff --git a/include/linux/cpumask.h b/include/linux/cpumask.h
index 4f7a632..ec0f6c9 100644
--- a/include/linux/cpumask.h
+++ b/include/linux/cpumask.h
@@ -27,6 +27,9 @@ typedef struct cpumask { DECLARE_BITMAP(bits, NR_CPUS); } cpumask_t;
 extern int nr_cpu_ids;
 #endif
 
+extern int nr_online_cpus;
+extern int nr_possible_cpus;
+
 #ifdef CONFIG_CPUMASK_OFFSTACK
 /* Assuming NR_CPUS is huge, a runtime limit is more efficient.  Also,
  * not all bits may be allocated. */
@@ -81,8 +84,10 @@ extern const struct cpumask *const cpu_present_mask;
 extern const struct cpumask *const cpu_active_mask;
 
 #if NR_CPUS > 1
-#define num_online_cpus()	cpumask_weight(cpu_online_mask)
-#define num_possible_cpus()	cpumask_weight(cpu_possible_mask)
+
+#define num_online_cpus()	(nr_online_cpus)
+#define num_possible_cpus()	(nr_possible_cpus)
+
 #define num_present_cpus()	cpumask_weight(cpu_present_mask)
 #define num_active_cpus()	cpumask_weight(cpu_active_mask)
 #define cpu_online(cpu)		cpumask_test_cpu((cpu), cpu_online_mask)
diff --git a/kernel/cpu.c b/kernel/cpu.c
index 2060c6e..58894e0 100644
--- a/kernel/cpu.c
+++ b/kernel/cpu.c
@@ -604,16 +604,23 @@ EXPORT_SYMBOL(cpu_all_bits);
 #ifdef CONFIG_INIT_ALL_POSSIBLE
 static DECLARE_BITMAP(cpu_possible_bits, CONFIG_NR_CPUS) __read_mostly
 	= CPU_BITS_ALL;
+int nr_possible_cpus __read_mostly = NR_CPUS;
 #else
 static DECLARE_BITMAP(cpu_possible_bits, CONFIG_NR_CPUS) __read_mostly;
+int nr_possible_cpus __read_mostly;
 #endif
 const struct cpumask *const cpu_possible_mask = to_cpumask(cpu_possible_bits);
 EXPORT_SYMBOL(cpu_possible_mask);
 
+EXPORT_SYMBOL(nr_possible_cpus);
+
 static DECLARE_BITMAP(cpu_online_bits, CONFIG_NR_CPUS) __read_mostly;
 const struct cpumask *const cpu_online_mask = to_cpumask(cpu_online_bits);
 EXPORT_SYMBOL(cpu_online_mask);
 
+int nr_online_cpus __read_mostly;
+EXPORT_SYMBOL(nr_online_cpus);
+
 static DECLARE_BITMAP(cpu_present_bits, CONFIG_NR_CPUS) __read_mostly;
 const struct cpumask *const cpu_present_mask = to_cpumask(cpu_present_bits);
 EXPORT_SYMBOL(cpu_present_mask);
@@ -628,6 +635,8 @@ void set_cpu_possible(unsigned int cpu, bool possible)
 		cpumask_set_cpu(cpu, to_cpumask(cpu_possible_bits));
 	else
 		cpumask_clear_cpu(cpu, to_cpumask(cpu_possible_bits));
+
+	nr_possible_cpus = cpumask_weight(cpu_possible_mask);
 }
 
 void set_cpu_present(unsigned int cpu, bool present)
@@ -644,6 +653,8 @@ void set_cpu_online(unsigned int cpu, bool online)
 		cpumask_set_cpu(cpu, to_cpumask(cpu_online_bits));
 	else
 		cpumask_clear_cpu(cpu, to_cpumask(cpu_online_bits));
+
+	nr_online_cpus = cpumask_weight(cpu_online_mask);
 }
 
 void set_cpu_active(unsigned int cpu, bool active)
@@ -662,9 +673,11 @@ void init_cpu_present(const struct cpumask *src)
 void init_cpu_possible(const struct cpumask *src)
 {
 	cpumask_copy(to_cpumask(cpu_possible_bits), src);
+	nr_possible_cpus = cpumask_weight(cpu_possible_mask);
 }
 
 void init_cpu_online(const struct cpumask *src)
 {
 	cpumask_copy(to_cpumask(cpu_online_bits), src);
+	nr_online_cpus = cpumask_weight(cpu_online_mask);
 }
-- 
1.7.7.3

Re: [PATCH] Avoid mask based num_possible_cpus and num_online_cpus -v4

[Srivatsa S. Bhat]

On 01/25/2012 04:55 AM, Venkatesh Pallipadi wrote:

> Kernel's notion of possible cpus (from include/linux/cpumask.h)
>  *     cpu_possible_mask- has bit 'cpu' set iff cpu is populatable
> 
>  *  The cpu_possible_mask is fixed at boot time, as the set of CPU id's
>  *  that it is possible might ever be plugged in at anytime during the
>  *  life of that system boot.
> 
>  #define num_possible_cpus()     cpumask_weight(cpu_possible_mask)
> 
> and on x86 cpumask_weight() calls hweight64 and hweight64 (on older kernels
> and systems with !X86_FEATURE_POPCNT) or a popcnt based alternative.
> 
> i.e, We needlessly go through this mask based calculation everytime
> num_possible_cpus() is called.
> 
> The problem is there with cpu_online_mask() as well, which is fixed value at
> boot time in !CONFIG_HOTPLUG_CPU case and should not change that often even
> in HOTPLUG case.
> 
> Though most of the callers of these two routines are init time (with few
> exceptions of runtime calls), it is cleaner to use variables
> and not go through this repeated mask based calculation.
> 
> Signed-off-by: Venkatesh Pallipadi <venki@google.com>
> ---


Looks good to me now :-)

Acked-by: Srivatsa S. Bhat <srivatsa.bhat@linux.vnet.ibm.com>

Regards,
Srivatsa S. Bhat

Re: [PATCH] Avoid mask based num_possible_cpus and num_online_cpus -v4

[Srivatsa S. Bhat]

On 01/25/2012 04:55 AM, Venkatesh Pallipadi wrote:

> Kernel's notion of possible cpus (from include/linux/cpumask.h)
>  *     cpu_possible_mask- has bit 'cpu' set iff cpu is populatable
> 
>  *  The cpu_possible_mask is fixed at boot time, as the set of CPU id's
>  *  that it is possible might ever be plugged in at anytime during the
>  *  life of that system boot.
> 
>  #define num_possible_cpus()     cpumask_weight(cpu_possible_mask)
> 
> and on x86 cpumask_weight() calls hweight64 and hweight64 (on older kernels
> and systems with !X86_FEATURE_POPCNT) or a popcnt based alternative.
> 
> i.e, We needlessly go through this mask based calculation everytime
> num_possible_cpus() is called.
> 
> The problem is there with cpu_online_mask() as well, which is fixed value at
> boot time in !CONFIG_HOTPLUG_CPU case and should not change that often even
> in HOTPLUG case.
> 
> Though most of the callers of these two routines are init time (with few
> exceptions of runtime calls), it is cleaner to use variables
> and not go through this repeated mask based calculation.
> 
> Signed-off-by: Venkatesh Pallipadi <venki@google.com>
> ---


Looks good to me now :-)

Acked-by: Srivatsa S. Bhat <srivatsa.bhat@linux.vnet.ibm.com>

Regards,
Srivatsa S. Bhat

Re: [PATCH] Avoid mask based num_possible_cpus and num_online_cpus -v4

[KOSAKI Motohiro]

2012/1/26 Srivatsa S. Bhat <srivatsa.bhat@linux.vnet.ibm.com>:
> On 01/25/2012 04:55 AM, Venkatesh Pallipadi wrote:
>
>> Kernel's notion of possible cpus (from include/linux/cpumask.h)
>>  *     cpu_possible_mask- has bit 'cpu' set iff cpu is populatable
>>
>>  *  The cpu_possible_mask is fixed at boot time, as the set of CPU id's
>>  *  that it is possible might ever be plugged in at anytime during the
>>  *  life of that system boot.
>>
>>  #define num_possible_cpus()     cpumask_weight(cpu_possible_mask)
>>
>> and on x86 cpumask_weight() calls hweight64 and hweight64 (on older kernels
>> and systems with !X86_FEATURE_POPCNT) or a popcnt based alternative.
>>
>> i.e, We needlessly go through this mask based calculation everytime
>> num_possible_cpus() is called.
>>
>> The problem is there with cpu_online_mask() as well, which is fixed value at
>> boot time in !CONFIG_HOTPLUG_CPU case and should not change that often even
>> in HOTPLUG case.
>>
>> Though most of the callers of these two routines are init time (with few
>> exceptions of runtime calls), it is cleaner to use variables
>> and not go through this repeated mask based calculation.
>>
>> Signed-off-by: Venkatesh Pallipadi <venki@google.com>
>> ---
>
>
> Looks good to me now :-)
>
> Acked-by: Srivatsa S. Bhat <srivatsa.bhat@linux.vnet.ibm.com>

I'm ok too.

Acked-by: KOSAKI Motohiro <kosaki.motohiro@jp.fujitsu.com>

Re: [PATCH] Avoid mask based num_possible_cpus and num_online_cpus -v4

[Andrew Morton]

On Tue, 24 Jan 2012 15:25:41 -0800
Venkatesh Pallipadi <venki@google.com> wrote:

> Kernel's notion of possible cpus (from include/linux/cpumask.h)
>  *     cpu_possible_mask- has bit 'cpu' set iff cpu is populatable
> 
>  *  The cpu_possible_mask is fixed at boot time, as the set of CPU id's
>  *  that it is possible might ever be plugged in at anytime during the
>  *  life of that system boot.
> 
>  #define num_possible_cpus()     cpumask_weight(cpu_possible_mask)
> 
> and on x86 cpumask_weight() calls hweight64 and hweight64 (on older kernels
> and systems with !X86_FEATURE_POPCNT) or a popcnt based alternative.
> 
> i.e, We needlessly go through this mask based calculation everytime
> num_possible_cpus() is called.
> 
> The problem is there with cpu_online_mask() as well, which is fixed value at
> boot time in !CONFIG_HOTPLUG_CPU case and should not change that often even
> in HOTPLUG case.
> 
> Though most of the callers of these two routines are init time (with few
> exceptions of runtime calls), it is cleaner to use variables
> and not go through this repeated mask based calculation.
>
> ...
>
> +extern int nr_online_cpus;
> +extern int nr_possible_cpus;
> +
>  #ifdef CONFIG_CPUMASK_OFFSTACK
>  /* Assuming NR_CPUS is huge, a runtime limit is more efficient.  Also,
>   * not all bits may be allocated. */
> @@ -81,8 +84,10 @@ extern const struct cpumask *const cpu_present_mask;
>  extern const struct cpumask *const cpu_active_mask;
>  
>  #if NR_CPUS > 1
> -#define num_online_cpus()	cpumask_weight(cpu_online_mask)
> -#define num_possible_cpus()	cpumask_weight(cpu_possible_mask)
> +
> +#define num_online_cpus()	(nr_online_cpus)
> +#define num_possible_cpus()	(nr_possible_cpus)

This changes the return types from "unsigned int" to int.  Worse, the
return types become dependent upon CONFIG_SMP.

s/int/unsigned int/g, methinks.

>
> ...
>
> --- a/kernel/cpu.c
> +++ b/kernel/cpu.c
> @@ -604,16 +604,23 @@ EXPORT_SYMBOL(cpu_all_bits);
>  #ifdef CONFIG_INIT_ALL_POSSIBLE
>  static DECLARE_BITMAP(cpu_possible_bits, CONFIG_NR_CPUS) __read_mostly
>  	= CPU_BITS_ALL;
> +int nr_possible_cpus __read_mostly = NR_CPUS;

It looks strange to see cpu_possible_bits using CONFIG_NR_CPUS whereas
nr_possible_cpus uses NR_CPUS.  I suggest using CONFIG_NR_CPUS for
both.

Aside: that FIXME in include/linux/threads.h should get fixed - it's
stupid.  We should fix the Kconfigs.

And the legacy NR_CPUS should be banished from the kernel altogether.

>  #else
>  static DECLARE_BITMAP(cpu_possible_bits, CONFIG_NR_CPUS) __read_mostly;
> +int nr_possible_cpus __read_mostly;
>  #endif
>  const struct cpumask *const cpu_possible_mask = to_cpumask(cpu_possible_bits);
>  EXPORT_SYMBOL(cpu_possible_mask);
>  
> +EXPORT_SYMBOL(nr_possible_cpus);

It's better to place the export immediately following the nr_possible_cpus
definition(s).

Re: [PATCH] Avoid mask based num_possible_cpus and num_online_cpus -v4

[Venki Pallipadi]

On Thu, Jan 26, 2012 at 3:22 PM, Andrew Morton
<akpm@linux-foundation.org> wrote:
> On Tue, 24 Jan 2012 15:25:41 -0800
> Venkatesh Pallipadi <venki@google.com> wrote:
>
>> Kernel's notion of possible cpus (from include/linux/cpumask.h)
>>  *     cpu_possible_mask- has bit 'cpu' set iff cpu is populatable
>>
>>  *  The cpu_possible_mask is fixed at boot time, as the set of CPU id's
>>  *  that it is possible might ever be plugged in at anytime during the
>>  *  life of that system boot.
>>
>>  #define num_possible_cpus()     cpumask_weight(cpu_possible_mask)
>>
>> and on x86 cpumask_weight() calls hweight64 and hweight64 (on older kernels
>> and systems with !X86_FEATURE_POPCNT) or a popcnt based alternative.
>>
>> i.e, We needlessly go through this mask based calculation everytime
>> num_possible_cpus() is called.
>>
>> The problem is there with cpu_online_mask() as well, which is fixed value at
>> boot time in !CONFIG_HOTPLUG_CPU case and should not change that often even
>> in HOTPLUG case.
>>
>> Though most of the callers of these two routines are init time (with few
>> exceptions of runtime calls), it is cleaner to use variables
>> and not go through this repeated mask based calculation.
>>
>> ...
>>
>> +extern int nr_online_cpus;
>> +extern int nr_possible_cpus;
>> +
>>  #ifdef CONFIG_CPUMASK_OFFSTACK
>>  /* Assuming NR_CPUS is huge, a runtime limit is more efficient.  Also,
>>   * not all bits may be allocated. */
>> @@ -81,8 +84,10 @@ extern const struct cpumask *const cpu_present_mask;
>>  extern const struct cpumask *const cpu_active_mask;
>>
>>  #if NR_CPUS > 1
>> -#define num_online_cpus()    cpumask_weight(cpu_online_mask)
>> -#define num_possible_cpus()  cpumask_weight(cpu_possible_mask)
>> +
>> +#define num_online_cpus()    (nr_online_cpus)
>> +#define num_possible_cpus()  (nr_possible_cpus)
>
> This changes the return types from "unsigned int" to int.  Worse, the
> return types become dependent upon CONFIG_SMP.
>
> s/int/unsigned int/g, methinks.
>

It will be kind of confusing with nr_cpu_ids being int and online or
possible_cpus being unsigned int. But, I agree that for the sake of
existing callers, this should be unsigned int.

>>
>> ...
>>
>> --- a/kernel/cpu.c
>> +++ b/kernel/cpu.c
>> @@ -604,16 +604,23 @@ EXPORT_SYMBOL(cpu_all_bits);
>>  #ifdef CONFIG_INIT_ALL_POSSIBLE
>>  static DECLARE_BITMAP(cpu_possible_bits, CONFIG_NR_CPUS) __read_mostly
>>       = CPU_BITS_ALL;
>> +int nr_possible_cpus __read_mostly = NR_CPUS;
>
> It looks strange to see cpu_possible_bits using CONFIG_NR_CPUS whereas
> nr_possible_cpus uses NR_CPUS.  I suggest using CONFIG_NR_CPUS for
> both.
>

OK. Will change. I saw NR_CPUS generously sprinkled around in
include/linux/cpumask.h and thought that was the preferred one to use.

> Aside: that FIXME in include/linux/threads.h should get fixed - it's
> stupid.  We should fix the Kconfigs.
>
> And the legacy NR_CPUS should be banished from the kernel altogether.
>

Silly-grep shows
$ git grep NR_CPUS | grep -v CONFIG_NR_CPUS | wc -l
748
$ git grep CONFIG_NR_CPUS | wc -l
57

So, that will involve a huge patch :-).

>>  #else
>>  static DECLARE_BITMAP(cpu_possible_bits, CONFIG_NR_CPUS) __read_mostly;
>> +int nr_possible_cpus __read_mostly;
>>  #endif
>>  const struct cpumask *const cpu_possible_mask = to_cpumask(cpu_possible_bits);
>>  EXPORT_SYMBOL(cpu_possible_mask);
>>
>> +EXPORT_SYMBOL(nr_possible_cpus);
>
> It's better to place the export immediately following the nr_possible_cpus
> definition(s).

OK. Will do.

Thanks,
Venki

[PATCH] Avoid mask based num_possible_cpus and num_online_cpus -v5

[Venkatesh Pallipadi]

Kernel's notion of possible cpus (from include/linux/cpumask.h)
 *     cpu_possible_mask- has bit 'cpu' set iff cpu is populatable

 *  The cpu_possible_mask is fixed at boot time, as the set of CPU id's
 *  that it is possible might ever be plugged in at anytime during the
 *  life of that system boot.

 #define num_possible_cpus()     cpumask_weight(cpu_possible_mask)

and on x86 cpumask_weight() calls hweight64 and hweight64 (on older kernels
and systems with !X86_FEATURE_POPCNT) or a popcnt based alternative.

i.e, We needlessly go through this mask based calculation everytime
num_possible_cpus() is called.

The problem is there with cpu_online_mask() as well, which is fixed value at
boot time in !CONFIG_HOTPLUG_CPU case and should not change that often even
in HOTPLUG case.

Though most of the callers of these two routines are init time (with few
exceptions of runtime calls), it is cleaner to use variables
and not go through this repeated mask based calculation.

Signed-off-by: Venkatesh Pallipadi <venki@google.com>
Acked-by: Srivatsa S. Bhat <srivatsa.bhat@linux.vnet.ibm.com>
Acked-by: KOSAKI Motohiro <kosaki.motohiro@jp.fujitsu.com>
---
 include/linux/cpumask.h |    9 +++++++--
 kernel/cpu.c            |   13 +++++++++++++
 2 files changed, 20 insertions(+), 2 deletions(-)

diff --git a/include/linux/cpumask.h b/include/linux/cpumask.h
index 4f7a632..930e255 100644
--- a/include/linux/cpumask.h
+++ b/include/linux/cpumask.h
@@ -27,6 +27,9 @@ typedef struct cpumask { DECLARE_BITMAP(bits, NR_CPUS); } cpumask_t;
 extern int nr_cpu_ids;
 #endif
 
+extern unsigned int nr_online_cpus;
+extern unsigned int nr_possible_cpus;
+
 #ifdef CONFIG_CPUMASK_OFFSTACK
 /* Assuming NR_CPUS is huge, a runtime limit is more efficient.  Also,
  * not all bits may be allocated. */
@@ -81,8 +84,10 @@ extern const struct cpumask *const cpu_present_mask;
 extern const struct cpumask *const cpu_active_mask;
 
 #if NR_CPUS > 1
-#define num_online_cpus()	cpumask_weight(cpu_online_mask)
-#define num_possible_cpus()	cpumask_weight(cpu_possible_mask)
+
+#define num_online_cpus()	(nr_online_cpus)
+#define num_possible_cpus()	(nr_possible_cpus)
+
 #define num_present_cpus()	cpumask_weight(cpu_present_mask)
 #define num_active_cpus()	cpumask_weight(cpu_active_mask)
 #define cpu_online(cpu)		cpumask_test_cpu((cpu), cpu_online_mask)
diff --git a/kernel/cpu.c b/kernel/cpu.c
index 2060c6e..d520d34 100644
--- a/kernel/cpu.c
+++ b/kernel/cpu.c
@@ -604,9 +604,13 @@ EXPORT_SYMBOL(cpu_all_bits);
 #ifdef CONFIG_INIT_ALL_POSSIBLE
 static DECLARE_BITMAP(cpu_possible_bits, CONFIG_NR_CPUS) __read_mostly
 	= CPU_BITS_ALL;
+unsigned int nr_possible_cpus __read_mostly = CONFIG_NR_CPUS;
 #else
 static DECLARE_BITMAP(cpu_possible_bits, CONFIG_NR_CPUS) __read_mostly;
+unsigned int nr_possible_cpus __read_mostly;
 #endif
+EXPORT_SYMBOL(nr_possible_cpus);
+
 const struct cpumask *const cpu_possible_mask = to_cpumask(cpu_possible_bits);
 EXPORT_SYMBOL(cpu_possible_mask);
 
@@ -614,6 +618,9 @@ static DECLARE_BITMAP(cpu_online_bits, CONFIG_NR_CPUS) __read_mostly;
 const struct cpumask *const cpu_online_mask = to_cpumask(cpu_online_bits);
 EXPORT_SYMBOL(cpu_online_mask);
 
+unsigned int nr_online_cpus __read_mostly;
+EXPORT_SYMBOL(nr_online_cpus);
+
 static DECLARE_BITMAP(cpu_present_bits, CONFIG_NR_CPUS) __read_mostly;
 const struct cpumask *const cpu_present_mask = to_cpumask(cpu_present_bits);
 EXPORT_SYMBOL(cpu_present_mask);
@@ -628,6 +635,8 @@ void set_cpu_possible(unsigned int cpu, bool possible)
 		cpumask_set_cpu(cpu, to_cpumask(cpu_possible_bits));
 	else
 		cpumask_clear_cpu(cpu, to_cpumask(cpu_possible_bits));
+
+	nr_possible_cpus = cpumask_weight(cpu_possible_mask);
 }
 
 void set_cpu_present(unsigned int cpu, bool present)
@@ -644,6 +653,8 @@ void set_cpu_online(unsigned int cpu, bool online)
 		cpumask_set_cpu(cpu, to_cpumask(cpu_online_bits));
 	else
 		cpumask_clear_cpu(cpu, to_cpumask(cpu_online_bits));
+
+	nr_online_cpus = cpumask_weight(cpu_online_mask);
 }
 
 void set_cpu_active(unsigned int cpu, bool active)
@@ -662,9 +673,11 @@ void init_cpu_present(const struct cpumask *src)
 void init_cpu_possible(const struct cpumask *src)
 {
 	cpumask_copy(to_cpumask(cpu_possible_bits), src);
+	nr_possible_cpus = cpumask_weight(cpu_possible_mask);
 }
 
 void init_cpu_online(const struct cpumask *src)
 {
 	cpumask_copy(to_cpumask(cpu_online_bits), src);
+	nr_online_cpus = cpumask_weight(cpu_online_mask);
 }
-- 
1.7.7.3

Re: [PATCH] Avoid mask based num_possible_cpus and num_online_cpus -v5

[Andrew Morton]

On Tue, 31 Jan 2012 16:17:19 -0800
Venkatesh Pallipadi <venki@google.com> wrote:

> Kernel's notion of possible cpus (from include/linux/cpumask.h)
>  *     cpu_possible_mask- has bit 'cpu' set iff cpu is populatable
> 
>  *  The cpu_possible_mask is fixed at boot time, as the set of CPU id's
>  *  that it is possible might ever be plugged in at anytime during the
>  *  life of that system boot.
> 
>  #define num_possible_cpus()     cpumask_weight(cpu_possible_mask)
> 
> and on x86 cpumask_weight() calls hweight64 and hweight64 (on older kernels
> and systems with !X86_FEATURE_POPCNT) or a popcnt based alternative.
> 
> i.e, We needlessly go through this mask based calculation everytime
> num_possible_cpus() is called.
> 
> The problem is there with cpu_online_mask() as well, which is fixed value at
> boot time in !CONFIG_HOTPLUG_CPU case and should not change that often even
> in HOTPLUG case.
> 
> Though most of the callers of these two routines are init time (with few
> exceptions of runtime calls), it is cleaner to use variables
> and not go through this repeated mask based calculation.

Looks good to me.

> --- a/kernel/cpu.c
> +++ b/kernel/cpu.c
> @@ -604,9 +604,13 @@ EXPORT_SYMBOL(cpu_all_bits);
>  #ifdef CONFIG_INIT_ALL_POSSIBLE
>  static DECLARE_BITMAP(cpu_possible_bits, CONFIG_NR_CPUS) __read_mostly
>  	= CPU_BITS_ALL;
> +unsigned int nr_possible_cpus __read_mostly = CONFIG_NR_CPUS;
>  #else
>  static DECLARE_BITMAP(cpu_possible_bits, CONFIG_NR_CPUS) __read_mostly;
> +unsigned int nr_possible_cpus __read_mostly;
>  #endif
> +EXPORT_SYMBOL(nr_possible_cpus);

What the heck is CONFIG_INIT_ALL_POSSIBLE?

<blames Rusty>

:    1) Some archs (m32, parisc, s390) set possible_map to all 1, so we add a
:       CONFIG_INIT_ALL_POSSIBLE for this rather than break them.

Seems strange.  Do these architectures really need to initialise
cpu_possible_map at compile-time, when all the other architectures
manage to do it at runtime?

Re: [PATCH] Avoid mask based num_possible_cpus and num_online_cpus -v5

[Rusty Russell]

On Wed, 1 Feb 2012 14:01:25 -0800, Andrew Morton <akpm@linux-foundation.org> wrote:
> > Though most of the callers of these two routines are init time (with few
> > exceptions of runtime calls), it is cleaner to use variables
> > and not go through this repeated mask based calculation.
> 
> Looks good to me.

But, I wonder who's asking num_possible_cpus().  It's not a very useful
thing to know, though some arch's "know" it's contiguous, so can cheat.

Optimizing it seems particularly foolish.  We either audit and wean
everyone off who's using it incorrectly, or insist on contiguous CPU
numbers and drop the mask altogether.

> > --- a/kernel/cpu.c
> > +++ b/kernel/cpu.c
> > @@ -604,9 +604,13 @@ EXPORT_SYMBOL(cpu_all_bits);
> >  #ifdef CONFIG_INIT_ALL_POSSIBLE
> >  static DECLARE_BITMAP(cpu_possible_bits, CONFIG_NR_CPUS) __read_mostly
> >  	= CPU_BITS_ALL;
> > +unsigned int nr_possible_cpus __read_mostly = CONFIG_NR_CPUS;
> >  #else
> >  static DECLARE_BITMAP(cpu_possible_bits, CONFIG_NR_CPUS) __read_mostly;
> > +unsigned int nr_possible_cpus __read_mostly;
> >  #endif
> > +EXPORT_SYMBOL(nr_possible_cpus);
> 
> What the heck is CONFIG_INIT_ALL_POSSIBLE?
> 
> <blames Rusty>
> 
> :    1) Some archs (m32, parisc, s390) set possible_map to all 1, so we add a
> :       CONFIG_INIT_ALL_POSSIBLE for this rather than break them.
> 
> Seems strange.  Do these architectures really need to initialise
> cpu_possible_map at compile-time, when all the other architectures
> manage to do it at runtime?

IIRC playing with 3 archs boot code seemed like a recipe for disaster.
Feel free to try to fix this in -next though, and see what breaks...

Cheers,
Rusty.

Re: [PATCH] Avoid mask based num_possible_cpus and num_online_cpus -v5

[Andrew Morton]

On Fri, 03 Feb 2012 06:33:02 +1030
Rusty Russell <rusty@rustcorp.com.au> wrote:

> On Wed, 1 Feb 2012 14:01:25 -0800, Andrew Morton <akpm@linux-foundation.org> wrote:
> > > Though most of the callers of these two routines are init time (with few
> > > exceptions of runtime calls), it is cleaner to use variables
> > > and not go through this repeated mask based calculation.
> > 
> > Looks good to me.
> 
> But, I wonder who's asking num_possible_cpus().  It's not a very useful
> thing to know, though some arch's "know" it's contiguous, so can cheat.
> 
> Optimizing it seems particularly foolish.

We're fools for optimisations!

> We either audit and wean
> everyone off who's using it incorrectly, or insist on contiguous CPU
> numbers and drop the mask altogether.

drivers/block/nvme.c looks like it's assuming a contiguous map.  Maybe
also drivers/scsi/bnx2fc (wtf?).  I didn't see anything else outside
arch code.

Re: [PATCH] Avoid mask based num_possible_cpus and num_online_cpus -v5

[Venki Pallipadi]

On Thu, Feb 2, 2012 at 12:19 PM, Andrew Morton
<akpm@linux-foundation.org> wrote:
> On Fri, 03 Feb 2012 06:33:02 +1030
> Rusty Russell <rusty@rustcorp.com.au> wrote:
>
>> On Wed, 1 Feb 2012 14:01:25 -0800, Andrew Morton <akpm@linux-foundation.org> wrote:
>> > > Though most of the callers of these two routines are init time (with few
>> > > exceptions of runtime calls), it is cleaner to use variables
>> > > and not go through this repeated mask based calculation.
>> >
>> > Looks good to me.
>>
>> But, I wonder who's asking num_possible_cpus().  It's not a very useful
>> thing to know, though some arch's "know" it's contiguous, so can cheat.
>>
>> Optimizing it seems particularly foolish.
>
> We're fools for optimisations!
>

I would think that if we are giving an API people will abuse it sooner
or later :-).

>> We either audit and wean
>> everyone off who's using it incorrectly, or insist on contiguous CPU
>> numbers and drop the mask altogether.
>
> drivers/block/nvme.c looks like it's assuming a contiguous map.  Maybe
> also drivers/scsi/bnx2fc (wtf?).  I didn't see anything else outside
> arch code.
>

Yes. I found a bunch of them which seemed obviously wrong.  Doing
things like allocating space based on num_possible_cpus() and
accessing the space with get_cpu() or doing cou_online() check etc.

arch/x86/kernel/apic/x2apic_uv_x.c
arch/x86/kernel/cpu/mcheck/mce-inject.c
arch/x86/platform/uv/tlb_uv.c
drivers/scsi/bnx2fc/bnx2fc_io.c
kernel/debug/kdb/kdb_bt.c

I have a patch to fix these obvious ones. But, there are other users
which were not very obvious to me and also I am know of code in older
kernels (code which since have been rewritten) which can get benefit
of this API change.

Thanks,
Venki

Re: [PATCH] Avoid mask based num_possible_cpus and num_online_cpus -v5

[Tony Luck]

On Thu, Feb 2, 2012 at 12:03 PM, Rusty Russell <rusty@rustcorp.com.au> wrote:
> IIRC playing with 3 archs boot code seemed like a recipe for disaster.
> Feel free to try to fix this in -next though, and see what breaks...

ia64 is what breaks ... well not actually broken ... but some very
weird delays that
show up in different places depending on whether this patch is present.

First linux-next kernel to be blessed with this patch was
next-20120210. Booting it
I see:
[    7.164233] Switching to clocksource itc
[  146.077315] pnp: PnP ACPI init

An ugly 138.913 second delay.  Digging in the code showed that the bad bits
happened inside stop_machine()

Reverting just this patch makes this big delay disappear:

[   32.780232] Switching to clocksource itc
[   32.832100] pnp: PnP ACPI init

but notice that it takes 25 extra seconds to get to this point in the
boot (and while
we expect to save some time by not re-computing num_online_cpus each time we
need it ... this looks to be a lot more than I'd expect!)

-Tony

Re: [PATCH] Avoid mask based num_possible_cpus and num_online_cpus -v5

[Venki Pallipadi]

On Mon, Feb 13, 2012 at 11:54 AM, Tony Luck <tony.luck@gmail.com> wrote:
> On Thu, Feb 2, 2012 at 12:03 PM, Rusty Russell <rusty@rustcorp.com.au> wrote:
>> IIRC playing with 3 archs boot code seemed like a recipe for disaster.
>> Feel free to try to fix this in -next though, and see what breaks...
>
> ia64 is what breaks ... well not actually broken ... but some very
> weird delays that
> show up in different places depending on whether this patch is present.
>
> First linux-next kernel to be blessed with this patch was
> next-20120210. Booting it
> I see:
> [    7.164233] Switching to clocksource itc
> [  146.077315] pnp: PnP ACPI init
>
> An ugly 138.913 second delay.  Digging in the code showed that the bad bits
> happened inside stop_machine()
>
> Reverting just this patch makes this big delay disappear:
>
> [   32.780232] Switching to clocksource itc
> [   32.832100] pnp: PnP ACPI init
>
> but notice that it takes 25 extra seconds to get to this point in the
> boot (and while
> we expect to save some time by not re-computing num_online_cpus each time we
> need it ... this looks to be a lot more than I'd expect!)
>
> -Tony

What is NR_CPUS set to here? And how many actual CPUs are there on this machine?

Re: [PATCH] Avoid mask based num_possible_cpus and num_online_cpus -v5

[Srivatsa S. Bhat]

On 02/14/2012 01:24 AM, Tony Luck wrote:

> On Thu, Feb 2, 2012 at 12:03 PM, Rusty Russell <rusty@rustcorp.com.au> wrote:
>> IIRC playing with 3 archs boot code seemed like a recipe for disaster.
>> Feel free to try to fix this in -next though, and see what breaks...
> 
> ia64 is what breaks ... well not actually broken ... but some very
> weird delays that
> show up in different places depending on whether this patch is present.
> 
> First linux-next kernel to be blessed with this patch was
> next-20120210. Booting it
> I see:
> [    7.164233] Switching to clocksource itc
> [  146.077315] pnp: PnP ACPI init
> 
> An ugly 138.913 second delay.  Digging in the code showed that the bad bits
> happened inside stop_machine()
> 
> Reverting just this patch makes this big delay disappear:
> 
> [   32.780232] Switching to clocksource itc
> [   32.832100] pnp: PnP ACPI init
> 
> but notice that it takes 25 extra seconds to get to this point in the
> boot (and while
> we expect to save some time by not re-computing num_online_cpus each time we
> need it ... this looks to be a lot more than I'd expect!)
> 


Oh no!! ia64 directly uses cpu_set() and cpu_clear() on cpu_online_map!!
Grr.. It means num_online_cpus can be different from the actual number of
online cpus because it doesn't go through the set_cpu_online() path.. I haven't
yet pin-pointed the exact problem, but this definitely doesn't look good...
 
Regards,
Srivatsa S. Bhat

Re: [PATCH] Avoid mask based num_possible_cpus and num_online_cpus -v5

[Venki Pallipadi]

On Mon, Feb 13, 2012 at 12:25 PM, Srivatsa S. Bhat
<srivatsa.bhat@linux.vnet.ibm.com> wrote:
> On 02/14/2012 01:24 AM, Tony Luck wrote:
>
>> On Thu, Feb 2, 2012 at 12:03 PM, Rusty Russell <rusty@rustcorp.com.au> wrote:
>>> IIRC playing with 3 archs boot code seemed like a recipe for disaster.
>>> Feel free to try to fix this in -next though, and see what breaks...
>>
>> ia64 is what breaks ... well not actually broken ... but some very
>> weird delays that
>> show up in different places depending on whether this patch is present.
>>
>> First linux-next kernel to be blessed with this patch was
>> next-20120210. Booting it
>> I see:
>> [    7.164233] Switching to clocksource itc
>> [  146.077315] pnp: PnP ACPI init
>>
>> An ugly 138.913 second delay.  Digging in the code showed that the bad bits
>> happened inside stop_machine()
>>
>> Reverting just this patch makes this big delay disappear:
>>
>> [   32.780232] Switching to clocksource itc
>> [   32.832100] pnp: PnP ACPI init
>>
>> but notice that it takes 25 extra seconds to get to this point in the
>> boot (and while
>> we expect to save some time by not re-computing num_online_cpus each time we
>> need it ... this looks to be a lot more than I'd expect!)
>>
>
>
> Oh no!! ia64 directly uses cpu_set() and cpu_clear() on cpu_online_map!!
> Grr.. It means num_online_cpus can be different from the actual number of
> online cpus because it doesn't go through the set_cpu_online() path.. I haven't
> yet pin-pointed the exact problem, but this definitely doesn't look good...
>

This feels like a minefield in general. ia64, mips and um seems to
have cpu_set and cpu_clear of cpu_online_map and/or cpu_possible_map
in there.

Re: [PATCH] Avoid mask based num_possible_cpus and num_online_cpus -v5

[Srivatsa S. Bhat]

On 02/14/2012 01:55 AM, Srivatsa S. Bhat wrote:

> On 02/14/2012 01:24 AM, Tony Luck wrote:
> 
>> On Thu, Feb 2, 2012 at 12:03 PM, Rusty Russell <rusty@rustcorp.com.au> wrote:
>>> IIRC playing with 3 archs boot code seemed like a recipe for disaster.
>>> Feel free to try to fix this in -next though, and see what breaks...
>>
>> ia64 is what breaks ... well not actually broken ... but some very
>> weird delays that
>> show up in different places depending on whether this patch is present.
>>
>> First linux-next kernel to be blessed with this patch was
>> next-20120210. Booting it
>> I see:
>> [    7.164233] Switching to clocksource itc
>> [  146.077315] pnp: PnP ACPI init
>>
>> An ugly 138.913 second delay.  Digging in the code showed that the bad bits
>> happened inside stop_machine()
>>
>> Reverting just this patch makes this big delay disappear:
>>
>> [   32.780232] Switching to clocksource itc
>> [   32.832100] pnp: PnP ACPI init
>>
>> but notice that it takes 25 extra seconds to get to this point in the
>> boot (and while
>> we expect to save some time by not re-computing num_online_cpus each time we
>> need it ... this looks to be a lot more than I'd expect!)
>>
> 
> 
> Oh no!! ia64 directly uses cpu_set() and cpu_clear() on cpu_online_map!!
> Grr.. It means num_online_cpus can be different from the actual number of
> online cpus because it doesn't go through the set_cpu_online() path.. I haven't
> yet pin-pointed the exact problem, but this definitely doesn't look good...
>  


Hmm.. interesting.. The only calls that ia64 uses which updates the
num_online_cpus macro seem to be init_cpu_online(cpumask_of(0)); Atleast this
is what the mainline code tells me (haven't checked linux-next).

So, if I am not mistaken, is the value of num_online_cpus() always 1 when
Venki's patch is applied?

IOW, what output do you see from the following printk from
arch/ia64/kernel/smpboot.c?

printk(KERN_INFO "Total of %d processors activated (%lu.%02lu BogoMIPS).\n",
         (int)num_online_cpus(), bogosum/(500000/HZ), (bogosum/(5000/HZ))%100);


Regards,
Srivatsa S. Bhat

Re: [PATCH] Avoid mask based num_possible_cpus and num_online_cpus -v5

[Srivatsa S. Bhat]

On 02/14/2012 02:13 AM, Venki Pallipadi wrote:

> On Mon, Feb 13, 2012 at 12:25 PM, Srivatsa S. Bhat
> <srivatsa.bhat@linux.vnet.ibm.com> wrote:
>> On 02/14/2012 01:24 AM, Tony Luck wrote:
>>
>>> On Thu, Feb 2, 2012 at 12:03 PM, Rusty Russell <rusty@rustcorp.com.au> wrote:
>>>> IIRC playing with 3 archs boot code seemed like a recipe for disaster.
>>>> Feel free to try to fix this in -next though, and see what breaks...
>>>
>>> ia64 is what breaks ... well not actually broken ... but some very
>>> weird delays that
>>> show up in different places depending on whether this patch is present.
>>>
>>> First linux-next kernel to be blessed with this patch was
>>> next-20120210. Booting it
>>> I see:
>>> [    7.164233] Switching to clocksource itc
>>> [  146.077315] pnp: PnP ACPI init
>>>
>>> An ugly 138.913 second delay.  Digging in the code showed that the bad bits
>>> happened inside stop_machine()
>>>
>>> Reverting just this patch makes this big delay disappear:
>>>
>>> [   32.780232] Switching to clocksource itc
>>> [   32.832100] pnp: PnP ACPI init
>>>
>>> but notice that it takes 25 extra seconds to get to this point in the
>>> boot (and while
>>> we expect to save some time by not re-computing num_online_cpus each time we
>>> need it ... this looks to be a lot more than I'd expect!)
>>>
>>
>>
>> Oh no!! ia64 directly uses cpu_set() and cpu_clear() on cpu_online_map!!
>> Grr.. It means num_online_cpus can be different from the actual number of
>> online cpus because it doesn't go through the set_cpu_online() path.. I haven't
>> yet pin-pointed the exact problem, but this definitely doesn't look good...
>>
> 
> This feels like a minefield in general. ia64, mips and um seems to
> have cpu_set and cpu_clear of cpu_online_map and/or cpu_possible_map
> in there.
> 


Since I had almost never seen code using "cpu_online_map" instead of
"cpu_online_mask", I hadn't checked it while reviewing your patch... :-(
Honestly, it is only now that I realized that there is this other variant too!

Regards,
Srivatsa S. Bhat

Re: [PATCH] Avoid mask based num_possible_cpus and num_online_cpus -v5

[Tony Luck]

On Mon, Feb 13, 2012 at 12:44 PM, Srivatsa S. Bhat
<srivatsa.bhat@linux.vnet.ibm.com> wrote:
> IOW, what output do you see from the following printk from
> arch/ia64/kernel/smpboot.c?
>
> printk(KERN_INFO "Total of %d processors activated (%lu.%02lu BogoMIPS).\n",
>         (int)num_online_cpus(), bogosum/(500000/HZ), (bogosum/(5000/HZ))%100);

That is a complicated question - because linux-next also has patches
by Arjan that
change how (when) cpus are brought online. Initially I blamed his
patches and tried
reverting them ... and saw the symptom you are wondering about (message said
"Total of 1 processors", but the BogoMIPs was a number big enough to be all of
them. Thanks to you, I can now understand why.

Fix will be to stop ia64 from messing directly with cpu_online_map?

-Tony

Re: [PATCH] Avoid mask based num_possible_cpus and num_online_cpus -v5

[Rusty Russell]

On Mon, 13 Feb 2012 13:57:45 -0800, Tony Luck <tony.luck@gmail.com> wrote:
> On Mon, Feb 13, 2012 at 12:44 PM, Srivatsa S. Bhat
> <srivatsa.bhat@linux.vnet.ibm.com> wrote:
> > IOW, what output do you see from the following printk from
> > arch/ia64/kernel/smpboot.c?
> >
> > printk(KERN_INFO "Total of %d processors activated (%lu.%02lu BogoMIPS).\n",
> >         (int)num_online_cpus(), bogosum/(500000/HZ), (bogosum/(5000/HZ))%100);
> 
> That is a complicated question - because linux-next also has patches
> by Arjan that
> change how (when) cpus are brought online. Initially I blamed his
> patches and tried
> reverting them ... and saw the symptom you are wondering about (message said
> "Total of 1 processors", but the BogoMIPs was a number big enough to be all of
> them. Thanks to you, I can now understand why.
> 
> Fix will be to stop ia64 from messing directly with cpu_online_map?

Yes, and the other architectures.

We're well within reach of removing cpu_*_map now I think.

Cheers,
Rusty.

Re: [PATCH] Avoid mask based num_possible_cpus and num_online_cpus -v5

[Srivatsa S. Bhat]

On 02/14/2012 02:55 PM, Rusty Russell wrote:

> On Mon, 13 Feb 2012 13:57:45 -0800, Tony Luck <tony.luck@gmail.com> wrote:
>> On Mon, Feb 13, 2012 at 12:44 PM, Srivatsa S. Bhat
>> <srivatsa.bhat@linux.vnet.ibm.com> wrote:
>>> IOW, what output do you see from the following printk from
>>> arch/ia64/kernel/smpboot.c?
>>>
>>> printk(KERN_INFO "Total of %d processors activated (%lu.%02lu BogoMIPS).\n",
>>>         (int)num_online_cpus(), bogosum/(500000/HZ), (bogosum/(5000/HZ))%100);
>>
>> That is a complicated question - because linux-next also has patches
>> by Arjan that
>> change how (when) cpus are brought online. Initially I blamed his
>> patches and tried
>> reverting them ... and saw the symptom you are wondering about (message said
>> "Total of 1 processors", but the BogoMIPs was a number big enough to be all of
>> them. Thanks to you, I can now understand why.
>>
>> Fix will be to stop ia64 from messing directly with cpu_online_map?
> 
> Yes, and the other architectures.
> 


Right. And we should also ensure that nobody messes directly with
cpu_possible_map as well. I have written up a patch for ia64 (see below).
Sorry, I haven't even compile tested it - I neither have the toolchain nor the
hardware. I hope it works!

But don't expect the above printk statement to print the right value if you are
running a kernel which has the patch posted at https://lkml.org/lkml/2012/1/31/286
applied. That is another patch that can alter boot-up time and many related
things. So, it would be best to test Venki's patch + following fix without having
Arjan's patch applied.

---
From: Srivatsa S. Bhat <srivatsa.bhat@linux.vnet.ibm.com>
Subject: [PATCH] ia64: Don't use cpu_set()/cpu_clear() over cpu_[online|possible]_map

Directly using cpu_set() and cpu_clear() on cpu_online_map or cpu_possible_map
is strongly discouraged. Use the functions set_cpu_online() and
set_cpu_possible() instead. This also means that the new implementation of
num_[online|possible]_cpus can track all changes to cpu_[online|possible]_mask
and hence give the correct results always.

Reported-by: Tony Luck <tony.luck@intel.com>
Signed-off-by: Srivatsa S. Bhat <srivatsa.bhat@linux.vnet.ibm.com>
---

 0 files changed, 0 insertions(+), 0 deletions(-)

diff --git a/arch/ia64/kernel/setup.c b/arch/ia64/kernel/setup.c
index cd57d73..4d1a550 100644
--- a/arch/ia64/kernel/setup.c
+++ b/arch/ia64/kernel/setup.c
@@ -486,7 +486,7 @@ mark_bsp_online (void)
 {
 #ifdef CONFIG_SMP
 	/* If we register an early console, allow CPU 0 to printk */
-	cpu_set(smp_processor_id(), cpu_online_map);
+	set_cpu_online(smp_processor_id(), true);
 #endif
 }
 
diff --git a/arch/ia64/kernel/smp.c b/arch/ia64/kernel/smp.c
index 0bd537b..8551979 100644
--- a/arch/ia64/kernel/smp.c
+++ b/arch/ia64/kernel/smp.c
@@ -77,7 +77,7 @@ stop_this_cpu(void)
 	/*
 	 * Remove this CPU:
 	 */
-	cpu_clear(smp_processor_id(), cpu_online_map);
+	set_cpu_online(smp_processor_id(), false);
 	max_xtp();
 	local_irq_disable();
 	cpu_halt();
diff --git a/arch/ia64/kernel/smpboot.c b/arch/ia64/kernel/smpboot.c
index 5590979..35f6e3c 100644
--- a/arch/ia64/kernel/smpboot.c
+++ b/arch/ia64/kernel/smpboot.c
@@ -401,7 +401,7 @@ smp_callin (void)
 	/* Setup the per cpu irq handling data structures */
 	__setup_vector_irq(cpuid);
 	notify_cpu_starting(cpuid);
-	cpu_set(cpuid, cpu_online_map);
+	set_cpu_online(cpuid, true);
 	per_cpu(cpu_state, cpuid) = CPU_ONLINE;
 	spin_unlock(&vector_lock);
 	ipi_call_unlock_irq();
@@ -548,7 +548,7 @@ do_rest:
 	if (!cpu_isset(cpu, cpu_callin_map)) {
 		printk(KERN_ERR "Processor 0x%x/0x%x is stuck.\n", cpu, sapicid);
 		ia64_cpu_to_sapicid[cpu] = -1;
-		cpu_clear(cpu, cpu_online_map);  /* was set in smp_callin() */
+		set_cpu_online(cpu, false);  /* was set in smp_callin() */
 		return -EINVAL;
 	}
 	return 0;
@@ -609,7 +609,7 @@ smp_prepare_cpus (unsigned int max_cpus)
 	/*
 	 * We have the boot CPU online for sure.
 	 */
-	cpu_set(0, cpu_online_map);
+	set_cpu_online(0, true);
 	cpu_set(0, cpu_callin_map);
 
 	local_cpu_data->loops_per_jiffy = loops_per_jiffy;
@@ -633,7 +633,7 @@ smp_prepare_cpus (unsigned int max_cpus)
 
 void __devinit smp_prepare_boot_cpu(void)
 {
-	cpu_set(smp_processor_id(), cpu_online_map);
+	set_cpu_online(smp_processor_id(), true);
 	cpu_set(smp_processor_id(), cpu_callin_map);
 	set_numa_node(cpu_to_node_map[smp_processor_id()]);
 	per_cpu(cpu_state, smp_processor_id()) = CPU_ONLINE;
@@ -732,10 +732,10 @@ int __cpu_disable(void)
 			return -EBUSY;
 	}
 
-	cpu_clear(cpu, cpu_online_map);
+	set_cpu_online(cpu, false);
 
 	if (migrate_platform_irqs(cpu)) {
-		cpu_set(cpu, cpu_online_map);
+		set_cpu_online(cpu, true);
 		return -EBUSY;
 	}
 

[PATCH 0/3] Cleanup raw handling of online/possible map

[Venkatesh Pallipadi]

> Yes, and the other architectures.

Here are the patches for other instances I see in plain git grep.

I have been brave (foolish) enough to send this without any testing. So,
this comes with 'use it at your own risk' tag :-).

Thanks,
Venki

[PATCH 1/3] hexagon: Avoid raw handling of cpu_possible_map

[Venkatesh Pallipadi]

Use set_cpu_possible instead.

Signed-off-by: Venkatesh Pallipadi <venki@google.com>
---
 arch/hexagon/kernel/smp.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/arch/hexagon/kernel/smp.c b/arch/hexagon/kernel/smp.c
index c871a2c..8962705 100644
--- a/arch/hexagon/kernel/smp.c
+++ b/arch/hexagon/kernel/smp.c
@@ -272,5 +272,5 @@ void smp_start_cpus(void)
 	int i;
 
 	for (i = 0; i < NR_CPUS; i++)
-		cpu_set(i, cpu_possible_map);
+		set_cpu_possible(i, true);
 }
-- 
1.7.7.3

[PATCH 2/3] mips: Avoid raw handling of cpu_possible_map/cpu_online_map

[Venkatesh Pallipadi]

Use set_cpu_* and init_cpu_* variants instead.

Signed-off-by: Venkatesh Pallipadi <venki@google.com>
---
 arch/mips/cavium-octeon/smp.c       |    2 +-
 arch/mips/kernel/smp.c              |    4 ++--
 arch/mips/netlogic/xlr/smp.c        |    4 ++--
 arch/mips/pmc-sierra/yosemite/smp.c |    4 ++--
 arch/mips/sgi-ip27/ip27-smp.c       |    2 +-
 arch/mips/sibyte/bcm1480/smp.c      |    5 ++---
 arch/mips/sibyte/sb1250/smp.c       |    5 ++---
 7 files changed, 12 insertions(+), 14 deletions(-)

diff --git a/arch/mips/cavium-octeon/smp.c b/arch/mips/cavium-octeon/smp.c
index efcfff4..5cce09c 100644
--- a/arch/mips/cavium-octeon/smp.c
+++ b/arch/mips/cavium-octeon/smp.c
@@ -268,7 +268,7 @@ static int octeon_cpu_disable(void)
 
 	spin_lock(&smp_reserve_lock);
 
-	cpu_clear(cpu, cpu_online_map);
+	set_cpu_online(cpu, false);
 	cpu_clear(cpu, cpu_callin_map);
 	local_irq_disable();
 	fixup_irqs();
diff --git a/arch/mips/kernel/smp.c b/arch/mips/kernel/smp.c
index 32c1e95..28777ff 100644
--- a/arch/mips/kernel/smp.c
+++ b/arch/mips/kernel/smp.c
@@ -148,7 +148,7 @@ static void stop_this_cpu(void *dummy)
 	/*
 	 * Remove this CPU:
 	 */
-	cpu_clear(smp_processor_id(), cpu_online_map);
+	set_cpu_online(smp_processor_id(), false);
 	for (;;) {
 		if (cpu_wait)
 			(*cpu_wait)();		/* Wait if available. */
@@ -248,7 +248,7 @@ int __cpuinit __cpu_up(unsigned int cpu)
 	while (!cpu_isset(cpu, cpu_callin_map))
 		udelay(100);
 
-	cpu_set(cpu, cpu_online_map);
+	set_cpu_online(cpu, true);
 
 	return 0;
 }
diff --git a/arch/mips/netlogic/xlr/smp.c b/arch/mips/netlogic/xlr/smp.c
index 080284d..8084221 100644
--- a/arch/mips/netlogic/xlr/smp.c
+++ b/arch/mips/netlogic/xlr/smp.c
@@ -154,7 +154,7 @@ void __init nlm_smp_setup(void)
 	cpu_set(boot_cpu, phys_cpu_present_map);
 	__cpu_number_map[boot_cpu] = 0;
 	__cpu_logical_map[0] = boot_cpu;
-	cpu_set(0, cpu_possible_map);
+	set_cpu_possible(0, true);
 
 	num_cpus = 1;
 	for (i = 0; i < NR_CPUS; i++) {
@@ -166,7 +166,7 @@ void __init nlm_smp_setup(void)
 			cpu_set(i, phys_cpu_present_map);
 			__cpu_number_map[i] = num_cpus;
 			__cpu_logical_map[num_cpus] = i;
-			cpu_set(num_cpus, cpu_possible_map);
+			set_cpu_possible(num_cpus, true);
 			++num_cpus;
 		}
 	}
diff --git a/arch/mips/pmc-sierra/yosemite/smp.c b/arch/mips/pmc-sierra/yosemite/smp.c
index 2608752..b2b23eb 100644
--- a/arch/mips/pmc-sierra/yosemite/smp.c
+++ b/arch/mips/pmc-sierra/yosemite/smp.c
@@ -155,10 +155,10 @@ static void __init yos_smp_setup(void)
 {
 	int i;
 
-	cpus_clear(cpu_possible_map);
+	init_cpu_possible(cpumask_of(0));
 
 	for (i = 0; i < 2; i++) {
-		cpu_set(i, cpu_possible_map);
+		set_cpu_possible(i, true);
 		__cpu_number_map[i]	= i;
 		__cpu_logical_map[i]	= i;
 	}
diff --git a/arch/mips/sgi-ip27/ip27-smp.c b/arch/mips/sgi-ip27/ip27-smp.c
index c6851df..735b43b 100644
--- a/arch/mips/sgi-ip27/ip27-smp.c
+++ b/arch/mips/sgi-ip27/ip27-smp.c
@@ -76,7 +76,7 @@ static int do_cpumask(cnodeid_t cnode, nasid_t nasid, int highest)
 			/* Only let it join in if it's marked enabled */
 			if ((acpu->cpu_info.flags & KLINFO_ENABLE) &&
 			    (tot_cpus_found != NR_CPUS)) {
-				cpu_set(cpuid, cpu_possible_map);
+				set_cpu_possible(cpuid, true);
 				alloc_cpupda(cpuid, tot_cpus_found);
 				cpus_found++;
 				tot_cpus_found++;
diff --git a/arch/mips/sibyte/bcm1480/smp.c b/arch/mips/sibyte/bcm1480/smp.c
index d667875..63d2211 100644
--- a/arch/mips/sibyte/bcm1480/smp.c
+++ b/arch/mips/sibyte/bcm1480/smp.c
@@ -147,14 +147,13 @@ static void __init bcm1480_smp_setup(void)
 {
 	int i, num;
 
-	cpus_clear(cpu_possible_map);
-	cpu_set(0, cpu_possible_map);
+	init_cpu_possible(cpumask_of(0));
 	__cpu_number_map[0] = 0;
 	__cpu_logical_map[0] = 0;
 
 	for (i = 1, num = 0; i < NR_CPUS; i++) {
 		if (cfe_cpu_stop(i) == 0) {
-			cpu_set(i, cpu_possible_map);
+			set_cpu_possible(i, true);
 			__cpu_number_map[i] = ++num;
 			__cpu_logical_map[num] = i;
 		}
diff --git a/arch/mips/sibyte/sb1250/smp.c b/arch/mips/sibyte/sb1250/smp.c
index 38e7f6b..77f0df5 100644
--- a/arch/mips/sibyte/sb1250/smp.c
+++ b/arch/mips/sibyte/sb1250/smp.c
@@ -135,14 +135,13 @@ static void __init sb1250_smp_setup(void)
 {
 	int i, num;
 
-	cpus_clear(cpu_possible_map);
-	cpu_set(0, cpu_possible_map);
+	init_cpu_possible(cpumask_of(0));
 	__cpu_number_map[0] = 0;
 	__cpu_logical_map[0] = 0;
 
 	for (i = 1, num = 0; i < NR_CPUS; i++) {
 		if (cfe_cpu_stop(i) == 0) {
-			cpu_set(i, cpu_possible_map);
+			set_cpu_possible(i, true);
 			__cpu_number_map[i] = ++num;
 			__cpu_logical_map[num] = i;
 		}
-- 
1.7.7.3

[PATCH 3/3] um: Avoid raw handling of cpu_online_map

[Venkatesh Pallipadi]

Use init_cpu_online and set_cpu_online instead.

Signed-off-by: Venkatesh Pallipadi <venki@google.com>
---
 arch/um/kernel/skas/process.c |    2 +-
 arch/um/kernel/smp.c          |    8 ++++----
 2 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/arch/um/kernel/skas/process.c b/arch/um/kernel/skas/process.c
index 2e9852c..5daa0f5 100644
--- a/arch/um/kernel/skas/process.c
+++ b/arch/um/kernel/skas/process.c
@@ -41,7 +41,7 @@ static int __init start_kernel_proc(void *unused)
 	cpu_tasks[0].pid = pid;
 	cpu_tasks[0].task = current;
 #ifdef CONFIG_SMP
-	cpu_online_map = cpumask_of_cpu(0);
+	init_cpu_online(cpumask_of(0));
 #endif
 	start_kernel();
 	return 0;
diff --git a/arch/um/kernel/smp.c b/arch/um/kernel/smp.c
index 155206a..b5d2ca9 100644
--- a/arch/um/kernel/smp.c
+++ b/arch/um/kernel/smp.c
@@ -76,7 +76,7 @@ static int idle_proc(void *cpup)
 		cpu_relax();
 
 	notify_cpu_starting(cpu);
-	cpu_set(cpu, cpu_online_map);
+	set_cpu_online(cpu, true);
 	default_idle();
 	return 0;
 }
@@ -110,8 +110,8 @@ void smp_prepare_cpus(unsigned int maxcpus)
 	for (i = 0; i < ncpus; ++i)
 		set_cpu_possible(i, true);
 
-	cpu_clear(me, cpu_online_map);
-	cpu_set(me, cpu_online_map);
+	set_cpu_online(me, false);
+	set_cpu_online(me, true);
 	cpu_set(me, cpu_callin_map);
 
 	err = os_pipe(cpu_data[me].ipi_pipe, 1, 1);
@@ -138,7 +138,7 @@ void smp_prepare_cpus(unsigned int maxcpus)
 
 void smp_prepare_boot_cpu(void)
 {
-	cpu_set(smp_processor_id(), cpu_online_map);
+	set_cpu_online(smp_processor_id(), true);
 }
 
 int __cpu_up(unsigned int cpu)
-- 
1.7.7.3

Re: [PATCH] Avoid mask based num_possible_cpus and num_online_cpus -v5

[Tony Luck]

On Tue, Feb 14, 2012 at 1:35 PM, Srivatsa S. Bhat
<srivatsa.bhat@linux.vnet.ibm.com> wrote:
> Right. And we should also ensure that nobody messes directly with
> cpu_possible_map as well. I have written up a patch for ia64 (see below).
> Sorry, I haven't even compile tested it - I neither have the toolchain nor the
> hardware. I hope it works!

Thanks for doing this - compiles and seems to work.

Tested-by: Tony Luck <tony.luck@intel.com>

Can we get this added to the series - so it gets applied along with
Venki's patch.

>  0 files changed, 0 insertions(+), 0 deletions(-)

I think your patch generation script needs some attention - I see
 arch/ia64/kernel/setup.c   |    2 +-
 arch/ia64/kernel/smp.c     |    2 +-
 arch/ia64/kernel/smpboot.c |   12 ++++++------
 3 files changed, 8 insertions(+), 8 deletions(-)

> @@ -609,7 +609,7 @@ smp_prepare_cpus (unsigned int max_cpus)
>        /*
>         * We have the boot CPU online for sure.
>         */
> -       cpu_set(0, cpu_online_map);
> +       set_cpu_online(0, true);
>        cpu_set(0, cpu_callin_map);
>
>        local_cpu_data->loops_per_jiffy = loops_per_jiffy;

Generic code has already marked cpu 0 online ... so this one could
just be dropped (and the preceding comment too). Though it does no
harm to set it again.

-Tony

Re: [PATCH] Avoid mask based num_possible_cpus and num_online_cpus -v5

[David Daney]

On 01/31/2012 04:17 PM, Venkatesh Pallipadi wrote:
> Kernel's notion of possible cpus (from include/linux/cpumask.h)
>   *     cpu_possible_mask- has bit 'cpu' set iff cpu is populatable
>
>   *  The cpu_possible_mask is fixed at boot time, as the set of CPU id's
>   *  that it is possible might ever be plugged in at anytime during the
>   *  life of that system boot.
>
>   #define num_possible_cpus()     cpumask_weight(cpu_possible_mask)
>
> and on x86 cpumask_weight() calls hweight64 and hweight64 (on older kernels
> and systems with !X86_FEATURE_POPCNT) or a popcnt based alternative.
>
> i.e, We needlessly go through this mask based calculation everytime
> num_possible_cpus() is called.
>
> The problem is there with cpu_online_mask() as well, which is fixed value at
> boot time in !CONFIG_HOTPLUG_CPU case and should not change that often even
> in HOTPLUG case.
>
> Though most of the callers of these two routines are init time (with few
> exceptions of runtime calls), it is cleaner to use variables
> and not go through this repeated mask based calculation.
>
> Signed-off-by: Venkatesh Pallipadi<venki@google.com>
> Acked-by: Srivatsa S. Bhat<srivatsa.bhat@linux.vnet.ibm.com>
> Acked-by: KOSAKI Motohiro<kosaki.motohiro@jp.fujitsu.com>

How is it that this patch got merged to linux-next before all the 
cleanup patches for nr_online_cpus?

 From the looks of your follow-on patches it would seem that all MIPS, 
hexagon, and um are now broken.

I know for a fact that MIPS doesn't boot because of this.

David Daney


> ---
>   include/linux/cpumask.h |    9 +++++++--
>   kernel/cpu.c            |   13 +++++++++++++
>   2 files changed, 20 insertions(+), 2 deletions(-)
>
> diff --git a/include/linux/cpumask.h b/include/linux/cpumask.h
> index 4f7a632..930e255 100644
> --- a/include/linux/cpumask.h
> +++ b/include/linux/cpumask.h
> @@ -27,6 +27,9 @@ typedef struct cpumask { DECLARE_BITMAP(bits, NR_CPUS); } cpumask_t;
>   extern int nr_cpu_ids;
>   #endif
>
> +extern unsigned int nr_online_cpus;
> +extern unsigned int nr_possible_cpus;
> +
>   #ifdef CONFIG_CPUMASK_OFFSTACK
>   /* Assuming NR_CPUS is huge, a runtime limit is more efficient.  Also,
>    * not all bits may be allocated. */
> @@ -81,8 +84,10 @@ extern const struct cpumask *const cpu_present_mask;
>   extern const struct cpumask *const cpu_active_mask;
>
>   #if NR_CPUS>  1
> -#define num_online_cpus()	cpumask_weight(cpu_online_mask)
> -#define num_possible_cpus()	cpumask_weight(cpu_possible_mask)
> +
> +#define num_online_cpus()	(nr_online_cpus)
> +#define num_possible_cpus()	(nr_possible_cpus)
> +
>   #define num_present_cpus()	cpumask_weight(cpu_present_mask)
>   #define num_active_cpus()	cpumask_weight(cpu_active_mask)
>   #define cpu_online(cpu)		cpumask_test_cpu((cpu), cpu_online_mask)
> diff --git a/kernel/cpu.c b/kernel/cpu.c
> index 2060c6e..d520d34 100644
> --- a/kernel/cpu.c
> +++ b/kernel/cpu.c
> @@ -604,9 +604,13 @@ EXPORT_SYMBOL(cpu_all_bits);
>   #ifdef CONFIG_INIT_ALL_POSSIBLE
>   static DECLARE_BITMAP(cpu_possible_bits, CONFIG_NR_CPUS) __read_mostly
>   	= CPU_BITS_ALL;
> +unsigned int nr_possible_cpus __read_mostly = CONFIG_NR_CPUS;
>   #else
>   static DECLARE_BITMAP(cpu_possible_bits, CONFIG_NR_CPUS) __read_mostly;
> +unsigned int nr_possible_cpus __read_mostly;
>   #endif
> +EXPORT_SYMBOL(nr_possible_cpus);
> +
>   const struct cpumask *const cpu_possible_mask = to_cpumask(cpu_possible_bits);
>   EXPORT_SYMBOL(cpu_possible_mask);
>
> @@ -614,6 +618,9 @@ static DECLARE_BITMAP(cpu_online_bits, CONFIG_NR_CPUS) __read_mostly;
>   const struct cpumask *const cpu_online_mask = to_cpumask(cpu_online_bits);
>   EXPORT_SYMBOL(cpu_online_mask);
>
> +unsigned int nr_online_cpus __read_mostly;
> +EXPORT_SYMBOL(nr_online_cpus);
> +
>   static DECLARE_BITMAP(cpu_present_bits, CONFIG_NR_CPUS) __read_mostly;
>   const struct cpumask *const cpu_present_mask = to_cpumask(cpu_present_bits);
>   EXPORT_SYMBOL(cpu_present_mask);
> @@ -628,6 +635,8 @@ void set_cpu_possible(unsigned int cpu, bool possible)
>   		cpumask_set_cpu(cpu, to_cpumask(cpu_possible_bits));
>   	else
>   		cpumask_clear_cpu(cpu, to_cpumask(cpu_possible_bits));
> +
> +	nr_possible_cpus = cpumask_weight(cpu_possible_mask);
>   }
>
>   void set_cpu_present(unsigned int cpu, bool present)
> @@ -644,6 +653,8 @@ void set_cpu_online(unsigned int cpu, bool online)
>   		cpumask_set_cpu(cpu, to_cpumask(cpu_online_bits));
>   	else
>   		cpumask_clear_cpu(cpu, to_cpumask(cpu_online_bits));
> +
> +	nr_online_cpus = cpumask_weight(cpu_online_mask);
>   }
>
>   void set_cpu_active(unsigned int cpu, bool active)
> @@ -662,9 +673,11 @@ void init_cpu_present(const struct cpumask *src)
>   void init_cpu_possible(const struct cpumask *src)
>   {
>   	cpumask_copy(to_cpumask(cpu_possible_bits), src);
> +	nr_possible_cpus = cpumask_weight(cpu_possible_mask);
>   }
>
>   void init_cpu_online(const struct cpumask *src)
>   {
>   	cpumask_copy(to_cpumask(cpu_online_bits), src);
> +	nr_online_cpus = cpumask_weight(cpu_online_mask);
>   }

Re: [PATCH] Avoid mask based num_possible_cpus and num_online_cpus -v5

[Andrew Morton]

On Mon, 27 Feb 2012 13:55:55 -0800
David Daney <ddaney.cavm@gmail.com> wrote:

> On 01/31/2012 04:17 PM, Venkatesh Pallipadi wrote:
> > Kernel's notion of possible cpus (from include/linux/cpumask.h)
> >   *     cpu_possible_mask- has bit 'cpu' set iff cpu is populatable
> >
> >   *  The cpu_possible_mask is fixed at boot time, as the set of CPU id's
> >   *  that it is possible might ever be plugged in at anytime during the
> >   *  life of that system boot.
> >
> >   #define num_possible_cpus()     cpumask_weight(cpu_possible_mask)
> >
> > and on x86 cpumask_weight() calls hweight64 and hweight64 (on older kernels
> > and systems with !X86_FEATURE_POPCNT) or a popcnt based alternative.
> >
> > i.e, We needlessly go through this mask based calculation everytime
> > num_possible_cpus() is called.
> >
> > The problem is there with cpu_online_mask() as well, which is fixed value at
> > boot time in !CONFIG_HOTPLUG_CPU case and should not change that often even
> > in HOTPLUG case.
> >
> > Though most of the callers of these two routines are init time (with few
> > exceptions of runtime calls), it is cleaner to use variables
> > and not go through this repeated mask based calculation.
> >
> > Signed-off-by: Venkatesh Pallipadi<venki@google.com>
> > Acked-by: Srivatsa S. Bhat<srivatsa.bhat@linux.vnet.ibm.com>
> > Acked-by: KOSAKI Motohiro<kosaki.motohiro@jp.fujitsu.com>
> 
> How is it that this patch got merged to linux-next before all the 
> cleanup patches for nr_online_cpus?

<spends five minutes searching mailing list archives>

I for one do not have a clue what patches the term "cleanup patches for
nr_online_cpus" refers to.  Patches have names - please use them!

>  From the looks of your follow-on patches it would seem that all MIPS, 
> hexagon, and um are now broken.
> 
> I know for a fact that MIPS doesn't boot because of this.

I shall drop
cpumask-avoid-mask-based-num_possible_cpus-and-num_online_cpus.patch.

That patch was sent as a single standalone patch and the changelog had
no mention of any needed preparatory patches.  If resending, please
send *all* patches in a single sequence-numbered series.  We know how
to do this.

Re: [PATCH] Avoid mask based num_possible_cpus and num_online_cpus -v5

[David Daney]

On 02/27/2012 02:07 PM, Andrew Morton wrote:
> On Mon, 27 Feb 2012 13:55:55 -0800
> David Daney<ddaney.cavm@gmail.com>  wrote:
>
>> On 01/31/2012 04:17 PM, Venkatesh Pallipadi wrote:
>>> Kernel's notion of possible cpus (from include/linux/cpumask.h)
>>>    *     cpu_possible_mask- has bit 'cpu' set iff cpu is populatable
>>>
>>>    *  The cpu_possible_mask is fixed at boot time, as the set of CPU id's
>>>    *  that it is possible might ever be plugged in at anytime during the
>>>    *  life of that system boot.
>>>
>>>    #define num_possible_cpus()     cpumask_weight(cpu_possible_mask)
>>>
>>> and on x86 cpumask_weight() calls hweight64 and hweight64 (on older kernels
>>> and systems with !X86_FEATURE_POPCNT) or a popcnt based alternative.
>>>
>>> i.e, We needlessly go through this mask based calculation everytime
>>> num_possible_cpus() is called.
>>>
>>> The problem is there with cpu_online_mask() as well, which is fixed value at
>>> boot time in !CONFIG_HOTPLUG_CPU case and should not change that often even
>>> in HOTPLUG case.
>>>
>>> Though most of the callers of these two routines are init time (with few
>>> exceptions of runtime calls), it is cleaner to use variables
>>> and not go through this repeated mask based calculation.
>>>
>>> Signed-off-by: Venkatesh Pallipadi<venki@google.com>
>>> Acked-by: Srivatsa S. Bhat<srivatsa.bhat@linux.vnet.ibm.com>
>>> Acked-by: KOSAKI Motohiro<kosaki.motohiro@jp.fujitsu.com>
>>
>> How is it that this patch got merged to linux-next before all the
>> cleanup patches for nr_online_cpus?
>
> <spends five minutes searching mailing list archives>
>
> I for one do not have a clue what patches the term "cleanup patches for
> nr_online_cpus" refers to.  Patches have names - please use them!
>

Sorry about that.  I was a little hasty:

   From:	Venkatesh Pallipadi <venki@google.com>
   Subject: [PATCH 0/3] Cleanup raw handling of online/possible map
   Date:	Tue, 14 Feb 2012 14:49:41 -0800
   Message-Id: <1329259784-20592-1-git-send-email-venki@google.com>

I don't really know a better way to refer to them.

David Daney



>>    From the looks of your follow-on patches it would seem that all MIPS,
>> hexagon, and um are now broken.
>>
>> I know for a fact that MIPS doesn't boot because of this.
>
> I shall drop
> cpumask-avoid-mask-based-num_possible_cpus-and-num_online_cpus.patch.
>
> That patch was sent as a single standalone patch and the changelog had
> no mention of any needed preparatory patches.  If resending, please
> send *all* patches in a single sequence-numbered series.  We know how
> to do this.

It is possible that Venkatesh did not know about the breakage when the 
original patch was sent

Re: [PATCH 2/3] mips: Avoid raw handling of cpu_possible_map/cpu_online_map

[David Daney]

On 02/14/2012 02:49 PM, Venkatesh Pallipadi wrote:
> Use set_cpu_* and init_cpu_* variants instead.
>
> Signed-off-by: Venkatesh Pallipadi<venki@google.com>

I came up with the same thing, so...

Acked-by: David Daney <david.daney@cavium.com>

Ralf:  If you too were to Acknowledge the patch, we might get it merged.

David Daney

> ---
>   arch/mips/cavium-octeon/smp.c       |    2 +-
>   arch/mips/kernel/smp.c              |    4 ++--
>   arch/mips/netlogic/xlr/smp.c        |    4 ++--
>   arch/mips/pmc-sierra/yosemite/smp.c |    4 ++--
>   arch/mips/sgi-ip27/ip27-smp.c       |    2 +-
>   arch/mips/sibyte/bcm1480/smp.c      |    5 ++---
>   arch/mips/sibyte/sb1250/smp.c       |    5 ++---
>   7 files changed, 12 insertions(+), 14 deletions(-)
>
> diff --git a/arch/mips/cavium-octeon/smp.c b/arch/mips/cavium-octeon/smp.c
> index efcfff4..5cce09c 100644
> --- a/arch/mips/cavium-octeon/smp.c
> +++ b/arch/mips/cavium-octeon/smp.c
> @@ -268,7 +268,7 @@ static int octeon_cpu_disable(void)
>
>   	spin_lock(&smp_reserve_lock);
>
> -	cpu_clear(cpu, cpu_online_map);
> +	set_cpu_online(cpu, false);
>   	cpu_clear(cpu, cpu_callin_map);
>   	local_irq_disable();
>   	fixup_irqs();
> diff --git a/arch/mips/kernel/smp.c b/arch/mips/kernel/smp.c
> index 32c1e95..28777ff 100644
> --- a/arch/mips/kernel/smp.c
> +++ b/arch/mips/kernel/smp.c
> @@ -148,7 +148,7 @@ static void stop_this_cpu(void *dummy)
>   	/*
>   	 * Remove this CPU:
>   	 */
> -	cpu_clear(smp_processor_id(), cpu_online_map);
> +	set_cpu_online(smp_processor_id(), false);
>   	for (;;) {
>   		if (cpu_wait)
>   			(*cpu_wait)();		/* Wait if available. */
> @@ -248,7 +248,7 @@ int __cpuinit __cpu_up(unsigned int cpu)
>   	while (!cpu_isset(cpu, cpu_callin_map))
>   		udelay(100);
>
> -	cpu_set(cpu, cpu_online_map);
> +	set_cpu_online(cpu, true);
>
>   	return 0;
>   }
> diff --git a/arch/mips/netlogic/xlr/smp.c b/arch/mips/netlogic/xlr/smp.c
> index 080284d..8084221 100644
> --- a/arch/mips/netlogic/xlr/smp.c
> +++ b/arch/mips/netlogic/xlr/smp.c
> @@ -154,7 +154,7 @@ void __init nlm_smp_setup(void)
>   	cpu_set(boot_cpu, phys_cpu_present_map);
>   	__cpu_number_map[boot_cpu] = 0;
>   	__cpu_logical_map[0] = boot_cpu;
> -	cpu_set(0, cpu_possible_map);
> +	set_cpu_possible(0, true);
>
>   	num_cpus = 1;
>   	for (i = 0; i<  NR_CPUS; i++) {
> @@ -166,7 +166,7 @@ void __init nlm_smp_setup(void)
>   			cpu_set(i, phys_cpu_present_map);
>   			__cpu_number_map[i] = num_cpus;
>   			__cpu_logical_map[num_cpus] = i;
> -			cpu_set(num_cpus, cpu_possible_map);
> +			set_cpu_possible(num_cpus, true);
>   			++num_cpus;
>   		}
>   	}
> diff --git a/arch/mips/pmc-sierra/yosemite/smp.c b/arch/mips/pmc-sierra/yosemite/smp.c
> index 2608752..b2b23eb 100644
> --- a/arch/mips/pmc-sierra/yosemite/smp.c
> +++ b/arch/mips/pmc-sierra/yosemite/smp.c
> @@ -155,10 +155,10 @@ static void __init yos_smp_setup(void)
>   {
>   	int i;
>
> -	cpus_clear(cpu_possible_map);
> +	init_cpu_possible(cpumask_of(0));
>
>   	for (i = 0; i<  2; i++) {
> -		cpu_set(i, cpu_possible_map);
> +		set_cpu_possible(i, true);
>   		__cpu_number_map[i]	= i;
>   		__cpu_logical_map[i]	= i;
>   	}
> diff --git a/arch/mips/sgi-ip27/ip27-smp.c b/arch/mips/sgi-ip27/ip27-smp.c
> index c6851df..735b43b 100644
> --- a/arch/mips/sgi-ip27/ip27-smp.c
> +++ b/arch/mips/sgi-ip27/ip27-smp.c
> @@ -76,7 +76,7 @@ static int do_cpumask(cnodeid_t cnode, nasid_t nasid, int highest)
>   			/* Only let it join in if it's marked enabled */
>   			if ((acpu->cpu_info.flags&  KLINFO_ENABLE)&&
>   			(tot_cpus_found != NR_CPUS)) {
> -				cpu_set(cpuid, cpu_possible_map);
> +				set_cpu_possible(cpuid, true);
>   				alloc_cpupda(cpuid, tot_cpus_found);
>   				cpus_found++;
>   				tot_cpus_found++;
> diff --git a/arch/mips/sibyte/bcm1480/smp.c b/arch/mips/sibyte/bcm1480/smp.c
> index d667875..63d2211 100644
> --- a/arch/mips/sibyte/bcm1480/smp.c
> +++ b/arch/mips/sibyte/bcm1480/smp.c
> @@ -147,14 +147,13 @@ static void __init bcm1480_smp_setup(void)
>   {
>   	int i, num;
>
> -	cpus_clear(cpu_possible_map);
> -	cpu_set(0, cpu_possible_map);
> +	init_cpu_possible(cpumask_of(0));
>   	__cpu_number_map[0] = 0;
>   	__cpu_logical_map[0] = 0;
>
>   	for (i = 1, num = 0; i<  NR_CPUS; i++) {
>   		if (cfe_cpu_stop(i) == 0) {
> -			cpu_set(i, cpu_possible_map);
> +			set_cpu_possible(i, true);
>   			__cpu_number_map[i] = ++num;
>   			__cpu_logical_map[num] = i;
>   		}
> diff --git a/arch/mips/sibyte/sb1250/smp.c b/arch/mips/sibyte/sb1250/smp.c
> index 38e7f6b..77f0df5 100644
> --- a/arch/mips/sibyte/sb1250/smp.c
> +++ b/arch/mips/sibyte/sb1250/smp.c
> @@ -135,14 +135,13 @@ static void __init sb1250_smp_setup(void)
>   {
>   	int i, num;
>
> -	cpus_clear(cpu_possible_map);
> -	cpu_set(0, cpu_possible_map);
> +	init_cpu_possible(cpumask_of(0));
>   	__cpu_number_map[0] = 0;
>   	__cpu_logical_map[0] = 0;
>
>   	for (i = 1, num = 0; i<  NR_CPUS; i++) {
>   		if (cfe_cpu_stop(i) == 0) {
> -			cpu_set(i, cpu_possible_map);
> +			set_cpu_possible(i, true);
>   			__cpu_number_map[i] = ++num;
>   			__cpu_logical_map[num] = i;
>   		}

Re: [PATCH] Avoid mask based num_possible_cpus and num_online_cpus -v5

[Stephen Rothwell]

[-- Attachment #1: Type: text/plain, Size: 314 bytes --]

Hi Andrew,

On Mon, 27 Feb 2012 14:07:46 -0800 Andrew Morton <akpm@linux-foundation.org> wrote:
>
> I shall drop
> cpumask-avoid-mask-based-num_possible_cpus-and-num_online_cpus.patch.

I have dropped that from the akpm tree today.

-- 
Cheers,
Stephen Rothwell                    sfr@canb.auug.org.au

[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH] Avoid mask based num_possible_cpus and num_online_cpus -v5

[Venki Pallipadi]

On Mon, Feb 27, 2012 at 2:16 PM, David Daney <ddaney.cavm@gmail.com> wrote:
> On 02/27/2012 02:07 PM, Andrew Morton wrote:
>>
>> On Mon, 27 Feb 2012 13:55:55 -0800
>> David Daney<ddaney.cavm@gmail.com>  wrote:
>>
>>> On 01/31/2012 04:17 PM, Venkatesh Pallipadi wrote:
>>>>
>>>> Kernel's notion of possible cpus (from include/linux/cpumask.h)
>>>>   *     cpu_possible_mask- has bit 'cpu' set iff cpu is populatable
>>>>
>>>>   *  The cpu_possible_mask is fixed at boot time, as the set of CPU id's
>>>>   *  that it is possible might ever be plugged in at anytime during the
>>>>   *  life of that system boot.
>>>>
>>>>   #define num_possible_cpus()     cpumask_weight(cpu_possible_mask)
>>>>
>>>> and on x86 cpumask_weight() calls hweight64 and hweight64 (on older
>>>> kernels
>>>> and systems with !X86_FEATURE_POPCNT) or a popcnt based alternative.
>>>>
>>>> i.e, We needlessly go through this mask based calculation everytime
>>>> num_possible_cpus() is called.
>>>>
>>>> The problem is there with cpu_online_mask() as well, which is fixed
>>>> value at
>>>> boot time in !CONFIG_HOTPLUG_CPU case and should not change that often
>>>> even
>>>> in HOTPLUG case.
>>>>
>>>> Though most of the callers of these two routines are init time (with few
>>>> exceptions of runtime calls), it is cleaner to use variables
>>>> and not go through this repeated mask based calculation.
>>>>
>>>> Signed-off-by: Venkatesh Pallipadi<venki@google.com>
>>>> Acked-by: Srivatsa S. Bhat<srivatsa.bhat@linux.vnet.ibm.com>
>>>> Acked-by: KOSAKI Motohiro<kosaki.motohiro@jp.fujitsu.com>
>>>
>>>
>>> How is it that this patch got merged to linux-next before all the
>>> cleanup patches for nr_online_cpus?
>>
>>
>> <spends five minutes searching mailing list archives>
>>
>> I for one do not have a clue what patches the term "cleanup patches for
>> nr_online_cpus" refers to.  Patches have names - please use them!
>>
>
> Sorry about that.  I was a little hasty:
>
>  From: Venkatesh Pallipadi <venki@google.com>
>  Subject: [PATCH 0/3] Cleanup raw handling of online/possible map
>  Date: Tue, 14 Feb 2012 14:49:41 -0800
>  Message-Id: <1329259784-20592-1-git-send-email-venki@google.com>
>
> I don't really know a better way to refer to them.
>
> David Daney
>
>
>
>
>>>   From the looks of your follow-on patches it would seem that all MIPS,
>>> hexagon, and um are now broken.
>>>
>>> I know for a fact that MIPS doesn't boot because of this.
>>
>>
>> I shall drop
>> cpumask-avoid-mask-based-num_possible_cpus-and-num_online_cpus.patch.
>>
>> That patch was sent as a single standalone patch and the changelog had
>> no mention of any needed preparatory patches.  If resending, please
>> send *all* patches in a single sequence-numbered series.  We know how
>> to do this.
>
>
> It is possible that Venkatesh did not know about the breakage when the
> original patch was sent

Sorry about all the confusion with this change. The long story goes
something like this:

Yes. The problems with this patch "Avoid mask based num_possible_cpus
and num_online_cpus"
were uncovered after it went into mm. First report was on ia64 here
https://lkml.org/lkml/2012/2/13/327

And there were potential problems in other archs as well. Srivatsa and
I sent out a bunch of cleanups
https://lkml.org/lkml/2012/2/14/355
https://lkml.org/lkml/2012/2/14/380

Rusty followed up with a superset of these cleanups
https://lkml.org/lkml/2012/2/15/4

I asked Rusty to pick up my original patch as the last patch in his series.
https://lkml.org/lkml/2012/2/15/516

That's all the happenings that I remember. I am not sure what the
status of Rusty's or my cleanup patches is right now. I am looking at
various trees now trying to find these cleanup patches. I can resubmit
this particular change once the cleanups have made through.

Thanks,
Venki