Messages in this thread | | | Date | Fri, 27 Jan 2012 15:58:34 -0800 | Subject | Re: [PATCH] Avoid mask based num_possible_cpus and num_online_cpus -v4 | From | 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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
| |