lkml.org 
[lkml]   [2003]   [Dec]   [23]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
SubjectRe: [PATCH] another minor bit of cpumask cleanup
Date
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.
-
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/

\
 
 \ /
  Last update: 2005-03-22 13:59    [W:0.076 / U:0.332 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site