[PATCH] another minor bit of cpumask cleanup

[PATCH] another minor bit of cpumask cleanup

[Paul Jackson]

Ingo Oeser pointed out to me in private email that one of the cpumask
macros was broken - the macro for for_each_online_cpu() starts its loop
with _any_ cpu from the provided mask, and only worries about restricting
itself to _online_ cpus when looping to the next cpu:

include/linux/cpumask.h:
> #define for_each_online_cpu(cpu, map)                                   \
>         for (cpu = first_cpu_const(mk_cpumask_const(map));              \
>                 cpu < NR_CPUS;                                          \
>                 cpu = next_online_cpu(cpu,map))

Looking further, I see this macro is never used, and its subordinate
inline macro next_online_cpu() used no where else.  What's more, it's
redundant.  Calling it with a map of "cpu_online_map" (which you have to
do, given it's broken thus) is just as good as calling the macro right
above, "for_each_cpu()", with that same "cpu_online_map". Indeed the
only uses of "for_each_cpu()", in arch/i386/mach-voyager/voyager_smp.c,
do pass "cpu_online_map" explicitly, in 5 of 6 calls there from.

So, having found a piece of code that is broken, redundant and unused,
I hereby off the following patch to remove it.

Thank-you, Ingo.


# This is a BitKeeper generated patch for the following project:
# Project Name: Linux kernel tree
# This patch format is intended for GNU patch command version 2.5 or higher.
# This patch includes the following deltas:
#	           ChangeSet	1.1497  -> 1.1498 
#	include/linux/cpumask.h	1.2     -> 1.3    
#
# The following is the BitKeeper ChangeSet Log
# --------------------------------------------
# 03/12/21	pj@sgi.com	1.1498
# Remove a couple unused, redundant, confused macros
# --------------------------------------------
#
diff -Nru a/include/linux/cpumask.h b/include/linux/cpumask.h
--- a/include/linux/cpumask.h	Sun Dec 21 17:37:14 2003
+++ b/include/linux/cpumask.h	Sun Dec 21 17:37:14 2003
@@ -17,22 +17,9 @@
 #define cpu_online(cpu)			({ BUG_ON((cpu) != 0); 1; })
 #endif
 
-static inline int next_online_cpu(int cpu, cpumask_t map)
-{
-	do
-		cpu = next_cpu_const(cpu, map);
-	while (cpu < NR_CPUS && !cpu_online(cpu));
-	return cpu;
-}
-
 #define for_each_cpu(cpu, map)						\
 	for (cpu = first_cpu_const(map);				\
 		cpu < NR_CPUS;						\
 		cpu = next_cpu_const(cpu,map))
-
-#define for_each_online_cpu(cpu, map)					\
-	for (cpu = first_cpu_const(map);				\
-		cpu < NR_CPUS;						\
-		cpu = next_online_cpu(cpu,map))
 
 #endif /* __LINUX_CPUMASK_H */


-- 
                          I won't rest till it's the best ...
                          Programmer, Linux Scalability
                          Paul Jackson <pj@sgi.com> 1.650.933.1373

Re: [PATCH] another minor bit of cpumask cleanup

[William Lee Irwin III]

On Sun, Dec 21, 2003 at 06:00:44PM -0800, Paul Jackson wrote:
> Looking further, I see this macro is never used, and its subordinate
> inline macro next_online_cpu() used no where else.  What's more, it's
> redundant.  Calling it with a map of "cpu_online_map" (which you have to
> do, given it's broken thus) is just as good as calling the macro right
> above, "for_each_cpu()", with that same "cpu_online_map". Indeed the
> only uses of "for_each_cpu()", in arch/i386/mach-voyager/voyager_smp.c,
> do pass "cpu_online_map" explicitly, in 5 of 6 calls there from.

Callers couldn't be converted without risking a "cleanup factor". It's
not terribly surprising some issue might appear since it wasn't used.

I don't honestly care if it lives or dies; it appeared to make more
sense as a generic macro than a voyager-specific macro at the time.


-- wli

Re: [PATCH] another minor bit of cpumask cleanup

[Andrew Morton]

Paul Jackson <pj@sgi.com> wrote:
>
> Ingo Oeser pointed out to me in private email that one of the cpumask
> macros was broken - the macro for for_each_online_cpu() starts its loop
> with _any_ cpu from the provided mask, and only worries about restricting
> itself to _online_ cpus when looping to the next cpu:
> 
> include/linux/cpumask.h:
> > #define for_each_online_cpu(cpu, map)                                   \
> >         for (cpu = first_cpu_const(mk_cpumask_const(map));              \
> >                 cpu < NR_CPUS;                                          \
> >                 cpu = next_online_cpu(cpu,map))
> 
> Looking further, I see this macro is never used, and its subordinate
> inline macro next_online_cpu() used no where else.  What's more, it's
> redundant.  Calling it with a map of "cpu_online_map" (which you have to
> do, given it's broken thus) is just as good as calling the macro right
> above, "for_each_cpu()", with that same "cpu_online_map". Indeed the
> only uses of "for_each_cpu()", in arch/i386/mach-voyager/voyager_smp.c,
> do pass "cpu_online_map" explicitly, in 5 of 6 calls there from.
> 
> So, having found a piece of code that is broken, redundant and unused,
> I hereby off the following patch to remove it.

Generates rejects for my tree.  I already have three patches which alter
cpumask.h.

Please, hang onto it until we get things synced up a bit more.

Re: [PATCH] another minor bit of cpumask cleanup

[Paul Jackson]

> Please, hang onto it until we get things synced up a bit more.

Ok - good idea.  I'll resend later on.  There is no hurry on this one.

-- 
                          I won't rest till it's the best ...
                          Programmer, Linux Scalability
                          Paul Jackson <pj@sgi.com> 1.650.933.1373

Re: [PATCH] another minor bit of cpumask cleanup

[William Lee Irwin III]

At some point in the past, akpm wrote:
>> Please, hang onto it until we get things synced up a bit more.

On Sun, Dec 21, 2003 at 11:19:18PM -0800, Paul Jackson wrote:
> Ok - good idea.  I'll resend later on.  There is no hurry on this one.

A rereading of this thread reveals the point of the thing was missed.
It's supposed to iterate over online cpus in an given bitmap. It was
meant to replace iterations like:

for_each_cpu(cpu, mask) {
	if (!cpu_online(cpu))
		continue;
	do_something(cpu);
}

with

for_each_online_cpu(cpu, mask)
	do_something(cpu);

Using any_online_cpu() as the starting point repairs it, since that
properly ands mask with cpu_online_map and hands back the first cpu,
though it's only a coincidence it hands back the first such cpu. There
isn't a a first_online_cpu() in the API, that's just effectively what
any_online_cpu() does at the moment.

On the other hand, I just don't care anymore, apart from clarifying
intent so as to counter the implication that all I did back then was
crap gibberish all over the tree. I personally have received zero
recognition or other return on my efforts in this area apart from the
mere fact it was merged. In fact, rather the opposite.


-- wli

Re: [PATCH] another minor bit of cpumask cleanup

[Paul Jackson]

wli wrote:
> so as to counter the implication that all I did back then was
> crap gibberish all over the tree

Sorry you're feeling picked on and under appreciated.

It really doesn't matter to me by whom or how this came to be.  More
than once in my career, I have cleaned up some stuff, then out of
curiosity gone back to see who might have written such crap -- only to
discover it was I.  I've learned to not ask such questions.  Just keep
on trying to make things better.

We are working on a large and ever changing body of code with thousands
of others active at any point.  The opportunities for refinement and
improvement are endless.  Don't sweat how we got here; those who have
done the most (such as yourself) probably have created the most
"opportunities" for further refinment.  Just judge each change on its
current merits, and on whether it moves in the right long term
directions.

See further:
    Ten Commandments of Egoless Programming
    http://builder.com.com/5100-6404-1045782.html

-- 
                          I won't rest till it's the best ...
                          Programmer, Linux Scalability
                          Paul Jackson <pj@sgi.com> 1.650.933.1373

Re: [PATCH] another minor bit of cpumask cleanup

[Rusty Russell]

On Sun, 21 Dec 2003 23:19:18 -0800
Paul Jackson <pj@sgi.com> wrote:

> > Please, hang onto it until we get things synced up a bit more.
> 
> Ok - good idea.  I'll resend later on.  There is no hurry on this one.

For the hotplug CPU stuff, I have a patch which changes these iterators anyway,
as discussed with wli and Patrick Mochel.  I'll push once we see a -pre: I
didn't know there was such widespread interest.

The idea is simple: encourage people to use the right iterators.  So
for_each_cpu() does each possible cpu (normally the right thing), 
for_each_online_cpu() does online cpus (implying you're holding the
cpucontrol sem) and for_each_cpu_mask() does a generic iteration.

FYI, patches below against 2.6.0: there are some other changes here which
conflict in -mm.

As to appreciation: I'd rather have 12 crossing patches than silence and
apathy.  At least this way we get informed criticism 8)

Thanks,
Rusty.
-- 
   there are those who do and those who hang on and you don't see too
   many doers quoting their contemporaries.  -- Larry McVoy

Name: Make for_each_cpu() more friendly.
Author: Rusty Russell
Status: Booted on 2.6.0

D: The for_each_cpu() and for_each_online_cpu() iterators take a mask,
D: and noone uses them that way (except for arch/i386/mach-voyager, which
D: uses for_each_cpu(cpu_online_mask).  Make them more usable iterators,
D: by dropping the "mask" arg.
D: 
D: This requires that archs provide a cpu_possible_mask: most do,
D: but PPC64 doesn't, so it is broken by this patch.  The other archs
D: use a #define to define it in asm/smp.h, so the iterators are moved
D: from linux/cpumask.h to linux/smp.h, where that is asm/smp.h is included.
D: 
D: Most places doing loops over cpus testing for cpu_online() should use
D: for_each_cpu: it is synonymous at the moment, but with the CPU hotplug patch
D: the difference becomes important.
D: 
D: Followup patches will convert users.
D: 
D: Thanks!
D: Rusty.

diff -urpN --exclude TAGS -X /home/rusty/devel/kernel/kernel-patches/current-dontdiff --minimal .22261-linux-2.6.0-test9-bk4/arch/i386/mach-voyager/voyager_smp.c .22261-linux-2.6.0-test9-bk4.updated/arch/i386/mach-voyager/voyager_smp.c
--- .22261-linux-2.6.0-test9-bk4/arch/i386/mach-voyager/voyager_smp.c	2003-09-22 10:27:54.000000000 +1000
+++ .22261-linux-2.6.0-test9-bk4.updated/arch/i386/mach-voyager/voyager_smp.c	2003-10-31 16:26:20.000000000 +1100
@@ -130,7 +130,7 @@ send_QIC_CPI(__u32 cpuset, __u8 cpi)
 {
 	int cpu;
 
-	for_each_cpu(cpu, cpu_online_map) {
+	for_each_online_cpu(cpu) {
 		if(cpuset & (1<<cpu)) {
 #ifdef VOYAGER_DEBUG
 			if(!cpu_isset(cpu, cpu_online_map))
@@ -1465,7 +1465,7 @@ send_CPI(__u32 cpuset, __u8 cpi)
 	cpuset &= 0xff;		/* only first 8 CPUs vaild for VIC CPI */
 	if(cpuset == 0)
 		return;
-	for_each_cpu(cpu, cpu_online_map) {
+	for_each_online_cpu(cpu) {
 		if(cpuset & (1<<cpu))
 			set_bit(cpi, &vic_cpi_mailbox[cpu]);
 	}
@@ -1579,7 +1579,7 @@ enable_vic_irq(unsigned int irq)
 	VDEBUG(("VOYAGER: enable_vic_irq(%d) CPU%d affinity 0x%lx\n",
 		irq, cpu, cpu_irq_affinity[cpu]));
 	spin_lock_irqsave(&vic_irq_lock, flags);
-	for_each_cpu(real_cpu, cpu_online_map) {
+	for_each_online_cpu(real_cpu) {
 		if(!(voyager_extended_vic_processors & (1<<real_cpu)))
 			continue;
 		if(!(cpu_irq_affinity[real_cpu] & mask)) {
@@ -1720,7 +1720,7 @@ after_handle_vic_irq(unsigned int irq)
 			int i;
 			__u8 cpu = smp_processor_id();
 			__u8 real_cpu;
-			int mask;
+			int mask; /* Um... initialize me??? --RR */
 
 			printk("VOYAGER SMP: CPU%d lost interrupt %d\n",
 			       cpu, irq);
@@ -1809,7 +1809,7 @@ set_vic_irq_affinity(unsigned int irq, c
 		 * bus) */
 		return;
 
-	for_each_cpu(cpu, cpu_online_map) {
+	for_each_online_cpu(cpu) {
 		unsigned long cpu_mask = 1 << cpu;
 		
 		if(cpu_mask & real_mask) {
@@ -1875,7 +1875,7 @@ voyager_smp_dump()
 	int old_cpu = smp_processor_id(), cpu;
 
 	/* dump the interrupt masks of each processor */
-	for_each_cpu(cpu, cpu_online_map) {
+	for_each_online_cpu(cpu) {
 		__u16 imr, isr, irr;
 		unsigned long flags;
 
diff -urpN --exclude TAGS -X /home/rusty/devel/kernel/kernel-patches/current-dontdiff --minimal .22261-linux-2.6.0-test9-bk4/include/asm-alpha/smp.h .22261-linux-2.6.0-test9-bk4.updated/include/asm-alpha/smp.h
--- .22261-linux-2.6.0-test9-bk4/include/asm-alpha/smp.h	2003-09-29 10:25:56.000000000 +1000
+++ .22261-linux-2.6.0-test9-bk4.updated/include/asm-alpha/smp.h	2003-10-31 16:26:20.000000000 +1100
@@ -48,8 +48,8 @@ extern struct cpuinfo_alpha cpu_data[NR_
 extern cpumask_t cpu_present_mask;
 extern cpumask_t cpu_online_map;
 extern int smp_num_cpus;
+#define cpu_possible_map	cpu_present_map
 
-#define cpu_possible(cpu)	cpu_isset(cpu, cpu_present_mask)
 #define cpu_online(cpu)		cpu_isset(cpu, cpu_online_map)
 
 extern int smp_call_function_on_cpu(void (*func) (void *info), void *info,int retry, int wait, unsigned long cpu);
diff -urpN --exclude TAGS -X /home/rusty/devel/kernel/kernel-patches/current-dontdiff --minimal .22261-linux-2.6.0-test9-bk4/include/asm-i386/smp.h .22261-linux-2.6.0-test9-bk4.updated/include/asm-i386/smp.h
--- .22261-linux-2.6.0-test9-bk4/include/asm-i386/smp.h	2003-09-22 10:28:12.000000000 +1000
+++ .22261-linux-2.6.0-test9-bk4.updated/include/asm-i386/smp.h	2003-10-31 16:26:20.000000000 +1100
@@ -53,8 +53,7 @@ extern void zap_low_mappings (void);
 #define smp_processor_id() (current_thread_info()->cpu)
 
 extern cpumask_t cpu_callout_map;
-
-#define cpu_possible(cpu) cpu_isset(cpu, cpu_callout_map)
+#define cpu_possible_map cpu_callout_map
 
 /* We don't mark CPUs online until __cpu_up(), so we need another measure */
 static inline int num_booting_cpus(void)
diff -urpN --exclude TAGS -X /home/rusty/devel/kernel/kernel-patches/current-dontdiff --minimal .22261-linux-2.6.0-test9-bk4/include/asm-ia64/smp.h .22261-linux-2.6.0-test9-bk4.updated/include/asm-ia64/smp.h
--- .22261-linux-2.6.0-test9-bk4/include/asm-ia64/smp.h	2003-09-22 10:27:36.000000000 +1000
+++ .22261-linux-2.6.0-test9-bk4.updated/include/asm-ia64/smp.h	2003-10-31 16:26:20.000000000 +1100
@@ -48,7 +48,7 @@ extern volatile int ia64_cpu_to_sapicid[
 
 extern unsigned long ap_wakeup_vector;
 
-#define cpu_possible(cpu)	cpu_isset(cpu, phys_cpu_present_map)
+#define cpu_possible_map phys_cpu_present_map
 
 /*
  * Function to map hard smp processor id to logical id.  Slow, so don't use this in
diff -urpN --exclude TAGS -X /home/rusty/devel/kernel/kernel-patches/current-dontdiff --minimal .22261-linux-2.6.0-test9-bk4/include/asm-mips/smp.h .22261-linux-2.6.0-test9-bk4.updated/include/asm-mips/smp.h
--- .22261-linux-2.6.0-test9-bk4/include/asm-mips/smp.h	2003-09-22 10:27:36.000000000 +1000
+++ .22261-linux-2.6.0-test9-bk4.updated/include/asm-mips/smp.h	2003-10-31 16:26:20.000000000 +1100
@@ -48,8 +48,8 @@ extern struct call_data_struct *call_dat
 
 extern cpumask_t phys_cpu_present_map;
 extern cpumask_t cpu_online_map;
+#define cpu_possible_map	phys_cpu_present_map
 
-#define cpu_possible(cpu)	cpu_isset(cpu, phys_cpu_present_map)
 #define cpu_online(cpu)		cpu_isset(cpu, cpu_online_map)
 
 static inline unsigned int num_online_cpus(void)
diff -urpN --exclude TAGS -X /home/rusty/devel/kernel/kernel-patches/current-dontdiff --minimal .22261-linux-2.6.0-test9-bk4/include/asm-parisc/smp.h .22261-linux-2.6.0-test9-bk4.updated/include/asm-parisc/smp.h
--- .22261-linux-2.6.0-test9-bk4/include/asm-parisc/smp.h	2003-09-22 10:27:36.000000000 +1000
+++ .22261-linux-2.6.0-test9-bk4.updated/include/asm-parisc/smp.h	2003-10-31 16:26:20.000000000 +1100
@@ -54,7 +54,7 @@ extern unsigned long cpu_present_mask;
 #define smp_processor_id()	(current_thread_info()->cpu)
 #define cpu_online(cpu) cpu_isset(cpu, cpu_online_map)
 
-#define cpu_possible(cpu)       cpu_isset(cpu, cpu_present_mask)
+#define cpu_possible_map	cpu_present_map
 
 #endif /* CONFIG_SMP */
 
diff -urpN --exclude TAGS -X /home/rusty/devel/kernel/kernel-patches/current-dontdiff --minimal .22261-linux-2.6.0-test9-bk4/include/asm-ppc/smp.h .22261-linux-2.6.0-test9-bk4.updated/include/asm-ppc/smp.h
--- .22261-linux-2.6.0-test9-bk4/include/asm-ppc/smp.h	2003-09-22 10:27:36.000000000 +1000
+++ .22261-linux-2.6.0-test9-bk4.updated/include/asm-ppc/smp.h	2003-10-31 16:26:20.000000000 +1100
@@ -48,7 +48,6 @@ extern void smp_local_timer_interrupt(st
 #define smp_processor_id() (current_thread_info()->cpu)
 
 #define cpu_online(cpu) cpu_isset(cpu, cpu_online_map)
-#define cpu_possible(cpu) cpu_isset(cpu, cpu_possible_map)
 
 extern int __cpu_up(unsigned int cpu);
 
diff -urpN --exclude TAGS -X /home/rusty/devel/kernel/kernel-patches/current-dontdiff --minimal .22261-linux-2.6.0-test9-bk4/include/asm-s390/smp.h .22261-linux-2.6.0-test9-bk4.updated/include/asm-s390/smp.h
--- .22261-linux-2.6.0-test9-bk4/include/asm-s390/smp.h	2003-09-29 10:26:01.000000000 +1000
+++ .22261-linux-2.6.0-test9-bk4.updated/include/asm-s390/smp.h	2003-10-31 16:26:20.000000000 +1100
@@ -49,7 +49,6 @@ extern cpumask_t cpu_possible_map;
 #define smp_processor_id() (S390_lowcore.cpu_data.cpu_nr)
 
 #define cpu_online(cpu) cpu_isset(cpu, cpu_online_map)
-#define cpu_possible(cpu) cpu_isset(cpu, cpu_possible_map)
 
 extern __inline__ __u16 hard_smp_processor_id(void)
 {
diff -urpN --exclude TAGS -X /home/rusty/devel/kernel/kernel-patches/current-dontdiff --minimal .22261-linux-2.6.0-test9-bk4/include/asm-sh/smp.h .22261-linux-2.6.0-test9-bk4.updated/include/asm-sh/smp.h
--- .22261-linux-2.6.0-test9-bk4/include/asm-sh/smp.h	2003-09-22 10:23:00.000000000 +1000
+++ .22261-linux-2.6.0-test9-bk4.updated/include/asm-sh/smp.h	2003-10-31 16:26:20.000000000 +1100
@@ -16,7 +16,6 @@
 extern unsigned long cpu_online_map;
 
 #define cpu_online(cpu)		(cpu_online_map & (1 << (cpu)))
-#define cpu_possible(cpu)	(cpu_online(cpu))
 
 #define smp_processor_id()	(current_thread_info()->cpu)
 
diff -urpN --exclude TAGS -X /home/rusty/devel/kernel/kernel-patches/current-dontdiff --minimal .22261-linux-2.6.0-test9-bk4/include/asm-sparc64/smp.h .22261-linux-2.6.0-test9-bk4.updated/include/asm-sparc64/smp.h
--- .22261-linux-2.6.0-test9-bk4/include/asm-sparc64/smp.h	2003-09-22 10:27:37.000000000 +1000
+++ .22261-linux-2.6.0-test9-bk4.updated/include/asm-sparc64/smp.h	2003-10-31 16:26:20.000000000 +1100
@@ -33,7 +33,7 @@
 extern unsigned char boot_cpu_id;
 
 extern cpumask_t phys_cpu_present_map;
-#define cpu_possible(cpu)	cpu_isset(cpu, phys_cpu_present_map)
+#define cpu_possible_map phys_cpu_present_map
 
 #define cpu_online(cpu)		cpu_isset(cpu, cpu_online_map)
 
diff -urpN --exclude TAGS -X /home/rusty/devel/kernel/kernel-patches/current-dontdiff --minimal .22261-linux-2.6.0-test9-bk4/include/asm-um/smp.h .22261-linux-2.6.0-test9-bk4.updated/include/asm-um/smp.h
--- .22261-linux-2.6.0-test9-bk4/include/asm-um/smp.h	2003-09-22 10:27:37.000000000 +1000
+++ .22261-linux-2.6.0-test9-bk4.updated/include/asm-um/smp.h	2003-10-31 16:26:20.000000000 +1100
@@ -20,7 +20,7 @@ extern int hard_smp_processor_id(void);
 #define cpu_online(cpu) cpu_isset(cpu, cpu_online_map)
 
 extern int ncpus;
-#define cpu_possible(cpu) (cpu < ncpus)
+
 
 extern inline void smp_cpus_done(unsigned int maxcpus)
 {
diff -urpN --exclude TAGS -X /home/rusty/devel/kernel/kernel-patches/current-dontdiff --minimal .22261-linux-2.6.0-test9-bk4/include/asm-x86_64/smp.h .22261-linux-2.6.0-test9-bk4.updated/include/asm-x86_64/smp.h
--- .22261-linux-2.6.0-test9-bk4/include/asm-x86_64/smp.h	2003-10-31 10:27:44.000000000 +1100
+++ .22261-linux-2.6.0-test9-bk4.updated/include/asm-x86_64/smp.h	2003-10-31 16:26:20.000000000 +1100
@@ -57,8 +57,7 @@ void smp_stop_cpu(void);
  */
 
 extern cpumask_t cpu_callout_map;
-
-#define cpu_possible(cpu) cpu_isset(cpu, cpu_callout_map)
+#define cpu_possible_map cpu_callout_map
 #define cpu_online(cpu) cpu_isset(cpu, cpu_online_map)
 
 static inline int num_booting_cpus(void)
diff -urpN --exclude TAGS -X /home/rusty/devel/kernel/kernel-patches/current-dontdiff --minimal .22261-linux-2.6.0-test9-bk4/include/linux/cpumask.h .22261-linux-2.6.0-test9-bk4.updated/include/linux/cpumask.h
--- .22261-linux-2.6.0-test9-bk4/include/linux/cpumask.h	2003-09-22 10:27:37.000000000 +1000
+++ .22261-linux-2.6.0-test9-bk4.updated/include/linux/cpumask.h	2003-10-31 16:28:16.000000000 +1100
@@ -41,31 +41,27 @@ typedef unsigned long cpumask_t;
 #ifdef CONFIG_SMP
 
 extern cpumask_t cpu_online_map;
+extern cpumask_t cpu_possible_map;
 
 #define num_online_cpus()		cpus_weight(cpu_online_map)
 #define cpu_online(cpu)			cpu_isset(cpu, cpu_online_map)
+#define cpu_possible(cpu)		cpu_isset(cpu, cpu_possible_map)
+
+#define for_each_cpu_mask(cpu, mask)			\
+	for (cpu = first_cpu_const(mask);		\
+		cpu < NR_CPUS;				\
+		cpu = next_cpu_const(cpu, mask))
+
+#define for_each_cpu(cpu) for_each_cpu_mask(cpu, cpu_possible_map)
+#define for_each_online_cpu(cpu) for_each_cpu_mask(cpu, cpu_online_map)
 #else
 #define	cpu_online_map			cpumask_of_cpu(0)
 #define num_online_cpus()		1
 #define cpu_online(cpu)			({ BUG_ON((cpu) != 0); 1; })
-#endif
-
-static inline int next_online_cpu(int cpu, cpumask_t map)
-{
-	do
-		cpu = next_cpu_const(cpu, map);
-	while (cpu < NR_CPUS && !cpu_online(cpu));
-	return cpu;
-}
-
-#define for_each_cpu(cpu, map)						\
-	for (cpu = first_cpu_const(map);				\
-		cpu < NR_CPUS;						\
-		cpu = next_cpu_const(cpu,map))
+#define cpu_possible(cpu)		({ BUG_ON((cpu) != 0); 1; })
 
-#define for_each_online_cpu(cpu, map)					\
-	for (cpu = first_cpu_const(map);				\
-		cpu < NR_CPUS;						\
-		cpu = next_online_cpu(cpu,map))
+#define for_each_cpu(cpu) for (cpu = 0; cpu < 1; cpu++)
+#define for_each_online_cpu(cpu) for (cpu = 0; cpu < 1; cpu++)
+#endif
 
 #endif /* __LINUX_CPUMASK_H */
diff -urpN --exclude TAGS -X /home/rusty/devel/kernel/kernel-patches/current-dontdiff --minimal .22261-linux-2.6.0-test9-bk4/include/linux/smp.h .22261-linux-2.6.0-test9-bk4.updated/include/linux/smp.h
--- .22261-linux-2.6.0-test9-bk4/include/linux/smp.h	2003-09-22 10:27:37.000000000 +1000
+++ .22261-linux-2.6.0-test9-bk4.updated/include/linux/smp.h	2003-10-31 16:26:20.000000000 +1100
@@ -103,7 +103,6 @@ void smp_prepare_boot_cpu(void);
 #define on_each_cpu(func,info,retry,wait)	({ func(info); 0; })
 static inline void smp_send_reschedule(int cpu) { }
 #define num_booting_cpus()			1
-#define cpu_possible(cpu)			({ BUG_ON((cpu) != 0); 1; })
 #define smp_prepare_boot_cpu()			do {} while (0)
 
 #endif /* !SMP */


Name: Use for_each_cpu() where cpu_online() is incorrectly used.
Author: Rusty Russell
Status: Booted on 2.6.0
Depends: Hotcpu/cpu_iterator.patch.gz

D: Some places use cpu_online() where they should be using cpu_possible,
D: most commonly for tallying statistics.  This makes no difference without
D: hotplug CPU.
D: 
D: Use the for_each_cpu() macro in those places, providing good
D: examples (and making the external hotplug CPU patch smaller).

diff -urpN --exclude TAGS -X /home/rusty/devel/kernel/kernel-patches/current-dontdiff --minimal .908-linux-2.6.0-test7-bk3/fs/buffer.c .908-linux-2.6.0-test7-bk3.updated/fs/buffer.c
--- .908-linux-2.6.0-test7-bk3/fs/buffer.c	2003-10-09 18:02:57.000000000 +1000
+++ .908-linux-2.6.0-test7-bk3.updated/fs/buffer.c	2003-10-13 08:45:09.000000000 +1000
@@ -2944,10 +2944,8 @@ static void recalc_bh_state(void)
 	if (__get_cpu_var(bh_accounting).ratelimit++ < 4096)
 		return;
 	__get_cpu_var(bh_accounting).ratelimit = 0;
-	for (i = 0; i < NR_CPUS; i++) {
-		if (cpu_online(i))
-			tot += per_cpu(bh_accounting, i).nr;
-	}
+	for_each_cpu(i)
+		tot += per_cpu(bh_accounting, i).nr;
 	buffer_heads_over_limit = (tot > max_buffer_heads);
 }
 	
diff -urpN --exclude TAGS -X /home/rusty/devel/kernel/kernel-patches/current-dontdiff --minimal .908-linux-2.6.0-test7-bk3/fs/proc/proc_misc.c .908-linux-2.6.0-test7-bk3.updated/fs/proc/proc_misc.c
--- .908-linux-2.6.0-test7-bk3/fs/proc/proc_misc.c	2003-09-29 10:25:53.000000000 +1000
+++ .908-linux-2.6.0-test7-bk3.updated/fs/proc/proc_misc.c	2003-10-13 08:45:09.000000000 +1000
@@ -378,10 +378,9 @@ int show_stat(struct seq_file *p, void *
 	jif = ((u64)now.tv_sec * HZ) + (now.tv_usec/(1000000/HZ)) - jif;
 	do_div(jif, HZ);
 
-	for (i = 0; i < NR_CPUS; i++) {
+	for_each_cpu(i) {
 		int j;
 
-		if (!cpu_online(i)) continue;
 		user += kstat_cpu(i).cpustat.user;
 		nice += kstat_cpu(i).cpustat.nice;
 		system += kstat_cpu(i).cpustat.system;
@@ -401,8 +400,7 @@ int show_stat(struct seq_file *p, void *
 		jiffies_to_clock_t(iowait),
 		jiffies_to_clock_t(irq),
 		jiffies_to_clock_t(softirq));
-	for (i = 0; i < NR_CPUS; i++){
-		if (!cpu_online(i)) continue;
+	for_each_online_cpu(i) {
 		seq_printf(p, "cpu%d %u %u %u %u %u %u %u\n",
 			i,
 			jiffies_to_clock_t(kstat_cpu(i).cpustat.user),
diff -urpN --exclude TAGS -X /home/rusty/devel/kernel/kernel-patches/current-dontdiff --minimal .908-linux-2.6.0-test7-bk3/kernel/fork.c .908-linux-2.6.0-test7-bk3.updated/kernel/fork.c
--- .908-linux-2.6.0-test7-bk3/kernel/fork.c	2003-10-12 11:04:17.000000000 +1000
+++ .908-linux-2.6.0-test7-bk3.updated/kernel/fork.c	2003-10-13 08:45:09.000000000 +1000
@@ -60,10 +60,9 @@ int nr_processes(void)
 	int cpu;
 	int total = 0;
 
-	for (cpu = 0; cpu < NR_CPUS; cpu++) {
-		if (cpu_online(cpu))
-			total += per_cpu(process_counts, cpu);
-	}
+	for_each_cpu(cpu)
+		total += per_cpu(process_counts, cpu);
+
 	return total;
 }
 
diff -urpN --exclude TAGS -X /home/rusty/devel/kernel/kernel-patches/current-dontdiff --minimal .908-linux-2.6.0-test7-bk3/kernel/sched.c .908-linux-2.6.0-test7-bk3.updated/kernel/sched.c
--- .908-linux-2.6.0-test7-bk3/kernel/sched.c	2003-10-09 18:03:02.000000000 +1000
+++ .908-linux-2.6.0-test7-bk3.updated/kernel/sched.c	2003-10-13 08:45:09.000000000 +1000
@@ -829,11 +829,9 @@ unsigned long nr_uninterruptible(void)
 {
 	unsigned long i, sum = 0;
 
-	for (i = 0; i < NR_CPUS; i++) {
-		if (!cpu_online(i))
-			continue;
+	for_each_cpu(i)
 		sum += cpu_rq(i)->nr_uninterruptible;
-	}
+
 	return sum;
 }
 
@@ -841,11 +839,9 @@ unsigned long nr_context_switches(void)
 {
 	unsigned long i, sum = 0;
 
-	for (i = 0; i < NR_CPUS; i++) {
-		if (!cpu_online(i))
-			continue;
+	for_each_cpu(i)
 		sum += cpu_rq(i)->nr_switches;
-	}
+
 	return sum;
 }
 
@@ -853,11 +849,9 @@ unsigned long nr_iowait(void)
 {
 	unsigned long i, sum = 0;
 
-	for (i = 0; i < NR_CPUS; ++i) {
-		if (!cpu_online(i))
-			continue;
+	for_each_cpu(i)
 		sum += atomic_read(&cpu_rq(i)->nr_iowait);
-	}
+
 	return sum;
 }
 
diff -urpN --exclude TAGS -X /home/rusty/devel/kernel/kernel-patches/current-dontdiff --minimal .908-linux-2.6.0-test7-bk3/kernel/workqueue.c .908-linux-2.6.0-test7-bk3.updated/kernel/workqueue.c
--- .908-linux-2.6.0-test7-bk3/kernel/workqueue.c	2003-09-22 10:27:38.000000000 +1000
+++ .908-linux-2.6.0-test7-bk3.updated/kernel/workqueue.c	2003-10-13 08:45:09.000000000 +1000
@@ -366,9 +366,7 @@ int current_is_keventd(void)
 
 	BUG_ON(!keventd_wq);
 
-	for (cpu = 0; cpu < NR_CPUS; cpu++) {
-		if (!cpu_online(cpu))
-			continue;
+	for_each_cpu(cpu) {
 		cwq = keventd_wq->cpu_wq + cpu;
 		if (current == cwq->thread)
 			return 1;
diff -urpN --exclude TAGS -X /home/rusty/devel/kernel/kernel-patches/current-dontdiff --minimal .908-linux-2.6.0-test7-bk3/mm/page_alloc.c .908-linux-2.6.0-test7-bk3.updated/mm/page_alloc.c
--- .908-linux-2.6.0-test7-bk3/mm/page_alloc.c	2003-10-09 18:03:02.000000000 +1000
+++ .908-linux-2.6.0-test7-bk3.updated/mm/page_alloc.c	2003-10-13 08:49:27.000000000 +1000
@@ -867,14 +867,14 @@ void __get_page_state(struct page_state 
 	while (cpu < NR_CPUS) {
 		unsigned long *in, *out, off;
 
-		if (!cpu_online(cpu)) {
+		if (!cpu_possible(cpu)) {
 			cpu++;
 			continue;
 		}
 
 		in = (unsigned long *)&per_cpu(page_states, cpu);
 		cpu++;
-		if (cpu < NR_CPUS && cpu_online(cpu))
+		if (cpu < NR_CPUS && cpu_possible(cpu))
 			prefetch(&per_cpu(page_states, cpu));
 		out = (unsigned long *)ret;
 		for (off = 0; off < nr; off++)
diff -urpN --exclude TAGS -X /home/rusty/devel/kernel/kernel-patches/current-dontdiff --minimal .908-linux-2.6.0-test7-bk3/net/ipv4/route.c .908-linux-2.6.0-test7-bk3.updated/net/ipv4/route.c
--- .908-linux-2.6.0-test7-bk3/net/ipv4/route.c	2003-10-09 18:03:03.000000000 +1000
+++ .908-linux-2.6.0-test7-bk3.updated/net/ipv4/route.c	2003-10-13 08:45:09.000000000 +1000
@@ -2703,12 +2703,9 @@ static int ip_rt_acct_read(char *buffer,
 		memcpy(dst, src, length);
 
 		/* Add the other cpus in, one int at a time */
-		for (i = 1; i < NR_CPUS; i++) {
+		for_each_cpu(i) {
 			unsigned int j;
 
-			if (!cpu_online(i))
-				continue;
-
 			src = ((u32 *) IP_RT_ACCT_CPU(i)) + offset;
 
 			for (j = 0; j < length/4; j++)
diff -urpN --exclude TAGS -X /home/rusty/devel/kernel/kernel-patches/current-dontdiff --minimal .3441-2.6.0-test9-hotcpu-i386.pre/kernel/timer.c .3441-2.6.0-test9-hotcpu-i386/kernel/timer.c
--- .3441-2.6.0-test9-hotcpu-i386.pre/kernel/timer.c	2003-10-27 13:26:01.000000000 +1100
+++ .3441-2.6.0-test9-hotcpu-i386/kernel/timer.c	2003-10-27 13:26:02.000000000 +1100
@@ -332,10 +332,7 @@ int del_timer_sync(struct timer_list *ti
 del_again:
 	ret += del_timer(timer);
 
-	for (i = 0; i < NR_CPUS; i++) {
-		if (!cpu_online(i))
-			continue;
-
+	for_each_cpu(i) {
 		base = &per_cpu(tvec_bases, i);
 		if (base->running_timer == timer) {
 			while (base->running_timer == timer) {

Re: [PATCH] another minor bit of cpumask cleanup

[Paul Jackson]

Rusty writes:
> I have a patch which changes these iterators [for_each_cpu] anyway,

Cool.


> I'd rather have 12 crossing patches than silence

yup


> For the hotplug CPU stuff,

Makes sense to me that the hotplug CPU work would be in the drivers seat
on these cpu looping macros.  I like your patch.  Since your more
substantial patch negates my trivial patch to remove the old
for_each_online_cpu(), I'll forget about my patch.

Andrew:

    You lucked out in dropping my patch to remove for_each_online_cpu(). 
    Good.


Speaking of trivial patches, didn't you (Rusty) used to be the Trivial
Patch Monkey, and what has become of that esteemed role in 2.6 land?

I do have a more substantial patch that is yet widely published to
provide an alternative underlying implementation of the cpumask macros
with something that can be used for both cpu and node masks, that takes
one file to express instead of 5 or 6, and that has one base type
(struct of array of unsigned longs) rather than a choice of three or so
implementations.

For at least one architecture, sparc64 (IIRC), wli informs me that davem
is quite certain this alternative can't be used (resulting machine code
way too painful).  But I am hopeful that we can make it cleaner source
code and just as good machine code, at least for the architectures that
can use recent gcc optimizing.

The basic goal of this patch-to-be will be to provide a solid 'mask'
type that can be used within the kernel for various bit masks of known
(compiled in) lengths of one to quite a few words.  This work stems from
a suggestion by Christoph Hellwig that I implement numamask_t using an
ADT shared with cpumask_t.  I originally told Christoph I would have
that ADT patch up and published in a week.  That was two months ago ;)

Unfortunately, I've just started on vacation for the next month or so,
so won't likely get much further publishing this right now.  If someone
has possibly conflicting work, or would like to join in, or otherwise
raise issues, please speak up.

I have the patch in a healthy state and being used on work that I am
doing to support the 'cpuset' feature being developed by Simon and
Sylvain of Bull (France).

There are some performance tests that Ray Bryant has graciously
volunteered to attempt, which will be critical to my final
recommendation of this alternative for at least the ia64 arch, which
tests will not happen until Feb, when I can set Ray up to make the
comparisons of this new implementation with the current one.  We need to
know whether for the > 256 bit case, seemingly replacing a call by
reference (pointer to mask) with a call by value will actually matter to
performance of the code we care about.

> possible cpu ...  online cpus

I'm not quite sure of the meaning to you of these terms.
Is it that possible cpus are the union of online and offline cpus?


> noone uses them that way (except for arch/i386/mach-voyager, which
> D: uses for_each_cpu(cpu_online_mask)

What about the one remaining usage of for_each_cpu(), also in
voyager, but not using cpu_online_mask:

arch/i386/mach-voyager/voyager_smp.c:

	=============================================================
	#ifdef VOYAGER_DEBUG
                ...
                if((isr & (1<<irq) && !(status & IRQ_REPLAY)) == 0) {
                        ...
                        int mask;

                        printk("VOYAGER SMP: CPU%d lost interrupt %d\n",
                               cpu, irq);
                        for_each_cpu(real_cpu, mask) {
	=============================================================

You noted that 'mask' needed initializing in a comment in your patch,
but I don't see that you change the calling signature of for_each_cpu(),
not that it is clear to what it should be changed ;(.


> so the iterators are moved
> D: from linux/cpumask.h to linux/smp.h, where that is asm/smp.h is included.

This comment says the iterators are moved to smp.h, but the patch seems
to still show them in cpumask.h.  I suspect that I prefer them in smp.h
better.


> D: Followup patches will convert users.

Looks to me like this here patch is converting some users, such as
in fork.c and sched.c.  Is this the conversion you speak of, or is
there more to come in a followup?

Good work, Rusty.  Thanks.

-- 
                          I won't rest till it's the best ...
                          Programmer, Linux Scalability
                          Paul Jackson <pj@sgi.com> 1.650.933.1373

Re: [PATCH] another minor bit of cpumask cleanup

[Rusty Russell]

In message <20031223021039.5b99a04b.pj@sgi.com> you write:
> I like your patch.  Since your more substantial patch negates my
> trivial patch to remove the old for_each_online_cpu(), I'll forget
> about my patch.

OK, thanks for reviewing!

> Speaking of trivial patches, didn't you (Rusty) used to be the Trivial
> Patch Monkey, and what has become of that esteemed role in 2.6 land?

Yes, but while things were frozen I only checked that mailbox once a
week or less, since things slowed to a crawl.  Also, there are only so
many genuinely trivial patches which aren't stupid 8)

> I do have a more substantial patch that is yet widely published to
> provide an alternative underlying implementation of the cpumask macros
> with something that can be used for both cpu and node masks, that takes
> one file to express instead of 5 or 6, and that has one base type
> (struct of array of unsigned longs) rather than a choice of three or so
> implementations.

Hmm, sounds interesting.

> For at least one architecture, sparc64 (IIRC), wli informs me that davem
> is quite certain this alternative can't be used (resulting machine code
> way too painful).  But I am hopeful that we can make it cleaner source
> code and just as good machine code, at least for the architectures that
> can use recent gcc optimizing.

I think we'll only be able to tell with the patch in front of us.
Maybe Dave will be convinced: he's dogmatic, but rarely unreasonable.

> > possible cpu ...  online cpus
> 
> I'm not quite sure of the meaning to you of these terms.
> Is it that possible cpus are the union of online and offline cpus?

Yes.  cpu_online(x) <= cpu_possible(x).  For adding counters and the
like, cpu_possible() is the test you want (most common usage).  If
you're using cpu_online(x), you need to either hold the cpucontrol
lock, or register a callback for it changing, or both.

> > noone uses them that way (except for arch/i386/mach-voyager, which
> > D: uses for_each_cpu(cpu_online_mask)
> 
> What about the one remaining usage of for_each_cpu(), also in
> voyager, but not using cpu_online_mask:
> 
> arch/i386/mach-voyager/voyager_smp.c:
> 
> 	=============================================================
> 	#ifdef VOYAGER_DEBUG
>                 ...
>                 if((isr & (1<<irq) && !(status & IRQ_REPLAY)) == 0) {
>                         ...
>                         int mask;
> 
>                         printk("VOYAGER SMP: CPU%d lost interrupt %d\n",
>                                cpu, irq);
>                         for_each_cpu(real_cpu, mask) {
> 	=============================================================
> 
> You noted that 'mask' needed initializing in a comment in your patch,
> but I don't see that you change the calling signature of for_each_cpu(),
> not that it is clear to what it should be changed ;(.

I figured the code is broken as is: I've left it broken (with the
benefit that it no longer even compiles).  Someday someone will enable
VOYAGER_DEBUG and they'll fix it.

> > so the iterators are moved
> > D: from linux/cpumask.h to linux/smp.h, where that is asm/smp.h is included.
> 
> This comment says the iterators are moved to smp.h, but the patch seems
> to still show them in cpumask.h.  I suspect that I prefer them in smp.h
> better.

Good catch: that comment is wrong.  Moving broke too much stuff IIRC.
Someone can do a separate patch if they want.

> > D: Followup patches will convert users.
> 
> Looks to me like this here patch is converting some users, such as
> in fork.c and sched.c.  Is this the conversion you speak of, or is
> there more to come in a followup?

I did the users which are actually wrong, but didn't do the ones which
are simply inefficient (ie. for (i = 0; i < NR_CPUS; i++)).  The
freeze came down, and I decided to go for minimal impact.

In 2.7, my aim is to switch the rest of them, move more things to
per-cpu rather than [NR_CPUS] arrays, add the more efficient dynamic
per-cpu allocation, and spread the per-cpu religion by fire and the
sword.

But I've said too much already...
Rusty.
--
  Anyone who quotes me in their sig is an idiot. -- Rusty Russell.

Re: [PATCH] another minor bit of cpumask cleanup

[Paul Jackson]

> thanks for reviewing!

My pleasure.

> Maybe Dave will be convinced: he's dogmatic, but rarely unreasonable.

On the third hand, it would be unreasonable of me to expect Dave to
agree to shoot his products performance in the foot for the sake of my
"more refined" coding style sensibilities.

> I think we'll only be able to tell with the patch in front of us.

I could publish an early version of it now, but I am supposed to be
on vacation now with my family, so should avoid starting a discussion
that I shouldn't participate in.

> Someday someone will enable VOYAGER_DEBUG and they'll fix it.

yup

> In 2.7, my aim is to switch the rest of them, move more things to
> per-cpu rather than [NR_CPUS] arrays, add the more efficient dynamic
> per-cpu allocation, and spread the per-cpu religion by fire and the
> sword.

For folks doing really large cpu counts, like my employer, this might
become of interest sooner.  On the other hand, we do really large memory
as well, so this might not be especially critical to us.

If NR_CPUS arrays start to annoy us sooner, I'll know where to consult.

-- 
                          I won't rest till it's the best ...
                          Programmer, Linux Scalability
                          Paul Jackson <pj@sgi.com> 1.650.933.1373

Re: [PATCH] another minor bit of cpumask cleanup

[William Lee Irwin III]

At some point in the past, rusty wrote:
>> In 2.7, my aim is to switch the rest of them, move more things to
>> per-cpu rather than [NR_CPUS] arrays, add the more efficient dynamic
>> per-cpu allocation, and spread the per-cpu religion by fire and the
>> sword.

On Tue, Dec 23, 2003 at 07:18:35PM -0800, Paul Jackson wrote:
> For folks doing really large cpu counts, like my employer, this might
> become of interest sooner.  On the other hand, we do really large memory
> as well, so this might not be especially critical to us.
> If NR_CPUS arrays start to annoy us sooner, I'll know where to consult.

This is primarily for the purpose of data placement, i.e. for node-local
per-cpu elements. That said, to each his own.


-- wli