[PATCH] for_each_possible_cpu [1/19] defines for_each_possible_cpu

[PATCH] for_each_possible_cpu [1/19] defines for_each_possible_cpu

[KAMEZAWA Hiroyuki]

Now,
for_each_cpu() is for-loop cpu over cpu_possible_map.
for_each_online_cpu is for-loop cpu over cpu_online_map.
.....for_each_cpu() looks bad name.

This patch renames for_each_cpu() as for_each_possible_cpu().

I also wrote patches to replace all for_each_cpu with for_each_possible_cpu.
please confirm....

BTW, when HOTPLUC_CPU is not suppoted, using for_each_possible_cpu()
should be avoided, I think.

all patches are against 2.6.16-rc6-mm1.

Signed-Off-By: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com>

Index: linux-2.6.16-rc6-mm1/include/linux/cpumask.h
===================================================================
--- linux-2.6.16-rc6-mm1.orig/include/linux/cpumask.h
+++ linux-2.6.16-rc6-mm1/include/linux/cpumask.h
@@ -67,7 +67,7 @@
  *
  * int any_online_cpu(mask)		First online cpu in mask
  *
- * for_each_cpu(cpu)			for-loop cpu over cpu_possible_map
+ * for_each_possible_cpu(cpu)		for-loop cpu over cpu_possible_map
  * for_each_online_cpu(cpu)		for-loop cpu over cpu_online_map
  * for_each_present_cpu(cpu)		for-loop cpu over cpu_present_map
  *
@@ -407,7 +407,7 @@ extern cpumask_t cpu_present_map;
 	cpu;					\
 })
 
-#define for_each_cpu(cpu)	  for_each_cpu_mask((cpu), cpu_possible_map)
+#define for_each_possible_cpu(cpu)  for_each_cpu_mask((cpu), cpu_possible_map)
 #define for_each_online_cpu(cpu)  for_each_cpu_mask((cpu), cpu_online_map)
 #define for_each_present_cpu(cpu) for_each_cpu_mask((cpu), cpu_present_map)
 

Re: [PATCH] for_each_possible_cpu [1/19] defines for_each_possible_cpu

[Andrew Morton]

KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com> wrote:
>
> Now,
>  for_each_cpu() is for-loop cpu over cpu_possible_map.
>  for_each_online_cpu is for-loop cpu over cpu_online_map.
>  .....for_each_cpu() looks bad name.
> 
>  This patch renames for_each_cpu() as for_each_possible_cpu().
>

Sane.

>  I also wrote patches to replace all for_each_cpu with for_each_possible_cpu.
>  please confirm....
> 
>  BTW, when HOTPLUC_CPU is not suppoted, using for_each_possible_cpu()
>  should be avoided, I think.

Sometimes.  Sometimes it's valid though - allocating (small amounts of)
per-cpu storage, summing up per-cpu counters (poorly), etc.

>  -#define for_each_cpu(cpu)	  for_each_cpu_mask((cpu), cpu_possible_map)
>  +#define for_each_possible_cpu(cpu)  for_each_cpu_mask((cpu), cpu_possible_map)

Nope, I'll change this to

#define for_each_cpu(cpu)	  for_each_cpu_mask((cpu), cpu_possible_map)
#define for_each_possible_cpu(cpu)  for_each_cpu_mask((cpu), cpu_possible_map)

So both are valid.  That way

a) The kernel continues to compile at each step of the patch series
   (important!) and

b) We can remove for_each_cpu() later on, after all the various
   out-of-tree usages have been converted.

Re: [PATCH] for_each_possible_cpu [1/19] defines for_each_possible_cpu

[Nick Piggin]

KAMEZAWA Hiroyuki wrote:
> Now,
> for_each_cpu() is for-loop cpu over cpu_possible_map.
> for_each_online_cpu is for-loop cpu over cpu_online_map.
> .....for_each_cpu() looks bad name.
> 
> This patch renames for_each_cpu() as for_each_possible_cpu().
> 
> I also wrote patches to replace all for_each_cpu with for_each_possible_cpu.
> please confirm....
> 
> BTW, when HOTPLUC_CPU is not suppoted, using for_each_possible_cpu()
> should be avoided, I think.
> 
> all patches are against 2.6.16-rc6-mm1.
> 

for_each_cpu() effectively is for_each_possible_cpu() as far as
generic code is concerned. In other words, nobody would ever expect
for_each_cpu to return an _impossible_ CPU, thus you are just
adding a redundant element to the name.

The only places where things might care is arch bootup code, but
the cpu interface is such that the arch code is expected to _hide_
any weird details from these generic interfaces.

-- 
SUSE Labs, Novell Inc.
Send instant messages to your online friends http://au.messenger.yahoo.com 

Re: [PATCH] for_each_possible_cpu [1/19] defines for_each_possible_cpu

[Andrew Morton]

Nick Piggin <nickpiggin@yahoo.com.au> wrote:
>
> KAMEZAWA Hiroyuki wrote:
> > Now,
> > for_each_cpu() is for-loop cpu over cpu_possible_map.
> > for_each_online_cpu is for-loop cpu over cpu_online_map.
> > .....for_each_cpu() looks bad name.
> > 
> > This patch renames for_each_cpu() as for_each_possible_cpu().
> > 
> > I also wrote patches to replace all for_each_cpu with for_each_possible_cpu.
> > please confirm....
> > 
> > BTW, when HOTPLUC_CPU is not suppoted, using for_each_possible_cpu()
> > should be avoided, I think.
> > 
> > all patches are against 2.6.16-rc6-mm1.
> > 
> 
> for_each_cpu() effectively is for_each_possible_cpu() as far as
> generic code is concerned. In other words, nobody would ever expect
> for_each_cpu to return an _impossible_ CPU, thus you are just
> adding a redundant element to the name.

We've had various screwups and confusions with these things.  I think the
new naming is good - it makes developers _think_ before they use it. 
Instead of "I want to touch all the CPUs, gee that looks right" they'll
have to stop and decide whether they want to access the online, possible or
present ones and then they'll (hopefully) have a little think about what
happens when CPUs migrate between those states.

Re: [PATCH] for_each_possible_cpu [1/19] defines for_each_possible_cpu

[Nick Piggin]

Andrew Morton wrote:
> Nick Piggin <nickpiggin@yahoo.com.au> wrote:

>>for_each_cpu() effectively is for_each_possible_cpu() as far as
>>generic code is concerned. In other words, nobody would ever expect
>>for_each_cpu to return an _impossible_ CPU, thus you are just
>>adding a redundant element to the name.
> 
> 
> We've had various screwups and confusions with these things.  I think the
> new naming is good - it makes developers _think_ before they use it. 
> Instead of "I want to touch all the CPUs, gee that looks right" they'll
> have to stop and decide whether they want to access the online, possible or
> present ones and then they'll (hopefully) have a little think about what
> happens when CPUs migrate between those states.
> 
> 

I think screwups probably came from unclear documentation (which it was
until recently, and some implementations were plain wrong IIRC), and the
recentish introduction of cpu hotplug.

I don't see the point in this though. If people don't want to even think
about these issues, then this change isn't going to make them.

-- 
SUSE Labs, Novell Inc.
Send instant messages to your online friends http://au.messenger.yahoo.com 

Re: [PATCH] for_each_possible_cpu [1/19] defines for_each_possible_cpu

[KAMEZAWA Hiroyuki]

On Wed, 15 Mar 2006 19:40:06 -0800
Andrew Morton <akpm@osdl.org> wrote:
> Sometimes.  Sometimes it's valid though - allocating (small amounts of)
> per-cpu storage, summing up per-cpu counters (poorly), etc.
> 
> >  -#define for_each_cpu(cpu)	  for_each_cpu_mask((cpu), cpu_possible_map)
> >  +#define for_each_possible_cpu(cpu)  for_each_cpu_mask((cpu), cpu_possible_map)
> 
> Nope, I'll change this to
> 
> #define for_each_cpu(cpu)	  for_each_cpu_mask((cpu), cpu_possible_map)
> #define for_each_possible_cpu(cpu)  for_each_cpu_mask((cpu), cpu_possible_map)
> 

Okay..It looks I was too aggressive :(
-- Kame

Re: [PATCH] for_each_possible_cpu [1/19] defines for_each_possible_cpu

[KAMEZAWA Hiroyuki]

On Thu, 16 Mar 2006 14:43:38 +1100
Nick Piggin <nickpiggin@yahoo.com.au> wrote:

> The only places where things might care is arch bootup code, but
> the cpu interface is such that the arch code is expected to _hide_
> any weird details from these generic interfaces.
> 
Please see i386 patch. it contains BUG fix.
cpu_msrs[i].coutners are allocated by for_each_online_cpu().
and free it by for_each_possible_cpus() without no pointer check.

I think this kind of confusion will be seen again in future.
--Kame
--
 static void free_msrs(void)
 {
        int i;
-       for_each_cpu(i) {
-               kfree(cpu_msrs[i].counters);
+       for_each_possible_cpu(i) {
+               if (cpu_msrs[i].counters)
+                       kfree(cpu_msrs[i].counters);
                cpu_msrs[i].counters = NULL;
-               kfree(cpu_msrs[i].controls);
+               if (cpu_msrs[i].controls)
+                       kfree(cpu_msrs[i].controls);
                cpu_msrs[i].controls = NULL;
        }
 }
--

Re: [PATCH] for_each_possible_cpu [1/19] defines for_each_possible_cpu

[Nick Piggin]

KAMEZAWA Hiroyuki wrote:
> On Thu, 16 Mar 2006 14:43:38 +1100
> Nick Piggin <nickpiggin@yahoo.com.au> wrote:
> 
> 
>>The only places where things might care is arch bootup code, but
>>the cpu interface is such that the arch code is expected to _hide_
>>any weird details from these generic interfaces.
>>
> 
> Please see i386 patch. it contains BUG fix.
> cpu_msrs[i].coutners are allocated by for_each_online_cpu().
> and free it by for_each_possible_cpus() without no pointer check.
> 

Well that's another problem then, such a fix should not be sent in
this patchset, but as a separate patch.

> I think this kind of confusion will be seen again in future.

I'm sure that renaming for_each_cpu would not prevent that either.

-- 
SUSE Labs, Novell Inc.
Send instant messages to your online friends http://au.messenger.yahoo.com 

Re: [PATCH] for_each_possible_cpu [1/19] defines for_each_possible_cpu

[YOSHIFUJI Hideaki / 吉藤英明]

In article <20060316131743.d7b716e9.kamezawa.hiroyu@jp.fujitsu.com> (at Thu, 16 Mar 2006 13:17:43 +0900), KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com> says:


> cpu_msrs[i].coutners are allocated by for_each_online_cpu().
> and free it by for_each_possible_cpus() without no pointer check.

No...

> -               kfree(cpu_msrs[i].counters);
> +       for_each_possible_cpu(i) {
> +               if (cpu_msrs[i].counters)
> +                       kfree(cpu_msrs[i].counters);

kfree() checks its argument for you.

>                 cpu_msrs[i].counters = NULL;
> -               kfree(cpu_msrs[i].controls);
> +               if (cpu_msrs[i].controls)
> +                       kfree(cpu_msrs[i].controls);
>                 cpu_msrs[i].controls = NULL;

ditto.

--yoshfuji

Re: [PATCH] for_each_possible_cpu [1/19] defines for_each_possible_cpu

[KAMEZAWA Hiroyuki]

On Thu, 16 Mar 2006 15:24:25 +1100
Nick Piggin <nickpiggin@yahoo.com.au> wrote:

> KAMEZAWA Hiroyuki wrote:
> > On Thu, 16 Mar 2006 14:43:38 +1100
> > Nick Piggin <nickpiggin@yahoo.com.au> wrote:
> > 
> > 
> >>The only places where things might care is arch bootup code, but
> >>the cpu interface is such that the arch code is expected to _hide_
> >>any weird details from these generic interfaces.
> >>
> > 
> > Please see i386 patch. it contains BUG fix.
> > cpu_msrs[i].coutners are allocated by for_each_online_cpu().
> > and free it by for_each_possible_cpus() without no pointer check.
> > 
> 
> Well that's another problem then, such a fix should not be sent in
> this patchset, but as a separate patch.
> 
Sorry. and It wasn't real problem as Yoshifuji said.
I'll send fix patch later.

> > I think this kind of confusion will be seen again in future.
> 
> I'm sure that renaming for_each_cpu would not prevent that either.
> 

But maintainers can check easily whether online or possible should be,
when they received a patch which includes for_each_cpu().

-- Kame 

Re: [PATCH] for_each_possible_cpu [1/19] defines for_each_possible_cpu

[Nick Piggin]

KAMEZAWA Hiroyuki wrote:
> On Thu, 16 Mar 2006 15:24:25 +1100
> Nick Piggin <nickpiggin@yahoo.com.au> wrote:

>>I'm sure that renaming for_each_cpu would not prevent that either.
>>
> 
> 
> But maintainers can check easily whether online or possible should be,
> when they received a patch which includes for_each_cpu().
> 

The submitter of the patch should have already thought of that
regardless of the name of the function. Everybody should be on
the same page anyway because for_each_cpu has always meant for
each possible CPU.

-- 
SUSE Labs, Novell Inc.
Send instant messages to your online friends http://au.messenger.yahoo.com 

Re: [PATCH] for_each_possible_cpu [1/19] defines for_each_possible_cpu

[Andrew Morton]

Nick Piggin <nickpiggin@yahoo.com.au> wrote:
>
> for_each_cpu has always meant for
>  each possible CPU.

That was the most long-winded ack I've ever seen ;)

Re: [PATCH] for_each_possible_cpu [1/19] defines for_each_possible_cpu

[Nick Piggin]

Andrew Morton wrote:
> Nick Piggin <nickpiggin@yahoo.com.au> wrote:
> 
>>for_each_cpu has always meant for
>> each possible CPU.
> 
> 
> That was the most long-winded ack I've ever seen ;)
> 

Is not! :). This is the very reason why I think it is pointless churn.
There are plenty of functions (even static, confined to a single file,
or ones much more complex than this) which could unquestionably have
their names changed for the better.

This change doesn't even add anything except a redundant element so
it is even questionable that it makes the name better at all.

But I know you've got your heart set on them now so I won't continue
with the impossible task of talking you out of them ;) Just don't
expect that people will suddenly start getting hotplug-cpu races
right, overnight.

-- 
SUSE Labs, Novell Inc.
Send instant messages to your online friends http://au.messenger.yahoo.com