lkml.org 
[lkml]   [2008]   [May]   [12]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
SubjectRe: [PATCH] Make for_each_cpu_mask a bit smaller
Date
On Sun, 11 May 2008 16:01:12 -0600, "Matthew Wilcox" <matthew@wil.cx>
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@wil.cx>
> > 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@fastmail.fm

--
http://www.fastmail.fm - mmm... Fastmail...



\
 
 \ /
  Last update: 2008-05-12 13:09    [W:1.004 / U:0.444 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site