On Sun, 11 May 2008 16:01:12 -0600, "Matthew Wilcox" <matthew@xxxxxx>
said:
> On Sun, May 11, 2008 at 06:19:39PM +0200, Alexander van Heukelum wrote:
> >
> > On Sun, 11 May 2008 09:24:40 -0600, "Matthew Wilcox" <matthew@xxxxxx>
> > said:
> > > On Sun, May 11, 2008 at 03:50:39PM +0200, Alexander van Heukelum wrote:
> > > > #if NR_CPUS > 1
> > > > -#define for_each_cpu_mask(cpu, mask) \
> > > > - for ((cpu) = first_cpu(mask); \
> > > > - (cpu) < NR_CPUS; \
> > > > - (cpu) = next_cpu((cpu), (mask)))
> > > > +#define for_each_cpu_mask(cpu, mask) \
> > > > + for ((cpu) = 0; \
> > > > + (cpu) = find_next_cpu_mask((cpu), &(mask)), \
> > > > + (cpu) < NR_CPUS; (cpu)++)
> > >
> > > For anyone else having similar cognitive dissonance while reading this
> > > thinking "But won't the first call to find_next_cpu_mask return a number
> > > > 0", the answer is "no, find_next_bit returns the next set bit that's
> > > >= the number passed in, which is why we need both the cpu++ and
> > > find_next_cpu_mask".
> >
> > That's how it works, indeed.
> >
> > > > +int find_next_cpu_mask(int n, const cpumask_t *srcp)
> > > > +{
> > > > + return find_next_bit(srcp->bits, NR_CPUS, n);
> > > > +}
> > > > +EXPORT_SYMBOL(find_next_cpu_mask);
> > >
> > > Maybe a better name for this function would help. I can't think of a
> > > good one right now though.
> >
> > I can't think of a better name, and there is find_next_bit of which
> > find_next_cpu_mask is just a wrapper. I think the name is good enough.
>
> How about doing it this way?
>
> #define for_each_cpu_mask(cpu, mask) \
> for ((cpu) = -1; \
> (cpu) < NR_CPUS; \
> (cpu) = find_next_cpu_mask((cpu), &(mask)))
>
> int find_next_cpu_mask(int n, const cpumask_t *srcp)
> {
> return find_next_bit(srcp->bits, NR_CPUS, ++n);
> }
>
> That actually behaves the way I'd expect a function called
> 'find_next_cpu_mask' to work. It also abuses the 'for' condtion
> less and might take a little less text space.
But it does not work.
It introduces a stray cpu=-1 iteration if cpu happens to be
(replaced by) a signed variable.
It skips the entire loop if cpu happens to be unsigned.
I don't think that using 'for' in a less conventional way
is bad if it is hidden in a macro, as long as the name of
the macro makes the intention sufficiently clear.
I think of find_next_cpu_mask(cpu, mask) as: "find next
cpu-index in mask, starting at index cpu". And similar
with find_next_bit.
As for the text-space argument, I think you might be right.
Just not on i386/x86_64 where initialising a register to -1
can be done in three bytes, initialising to 0 in two bytes
and an increment in one byte :-).
Greetings,
Alexander
> --
> Intel are signing my paycheques ... these opinions are still mine
> "Bill, look, we understand that you're interested in selling us this
> operating system, but compare it to ours. We can't possibly take such
> a retrograde step."
--
Alexander van Heukelum
heukelum@xxxxxxxxxxx
--
http://www.fastmail.fm - mmm... Fastmail...
--
To unsubscribe from this list: send the line "unsubscribe linux-arch" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
