Re: [PATCH] Make for_each_cpu_mask a bit smaller

[Alexander van Heukelum]

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-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/