Messages in this thread | | | From | Rusty Russell <> | Subject | Re: [PATCH] another minor bit of cpumask cleanup | Date | Wed, 24 Dec 2003 12:26:02 +1100 |
| |
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/
| |