Re: [PATCH] fix sys cpumap for > 352 NR_CPUS

[Rusty Russell <rusty@rustcorp.com.au>]

On Thu, 2004-06-03 at 18:27, Paul Jackson wrote:
> > +	BUILD_BUG_ON(NR_CPUS/4 > PAGE_SIZE/2);
> 
> Interesting.  This would be the only use in the entire kernel of
> BUILD_BUG_ON().
> 
> An alternative mechanism would be:
> 
>     #if(badthing) ... #error "darn" ... #endif

Whatever, I like BUILD_BUG_ON().

>     #if ALIGN(NR_CPUS,32)*9/32 > PAGE_SIZE
>     #error "Need 9 bytes space per 32 CPUs in PAGE_SIZE buffer"
>     #endif

Honestly, I dislike the static check altogether, since it's tied too
closely to the implementation of independent code.  Since glibc will
break on > 1024 cpus, I'm not too concerned about the 10000+ CPU limit.

> > +	len = cpumask_scnprintf(buf, -1UL, mask);
> 
> Why not instead:
> 
> > +	len = cpumask_scnprintf(buf, PAGE_SIZE-1, mask);
> 
> I see no sense in giving cpumask_scnprintf() license to write past the
> end of the buffer, independent of any build-time checks (the -1 is for
> the trailing newline).  And since the contract says "PAGE_SIZE", we
> should code exactly to that value "PAGE_SIZE", for clarity as to our
> understandings.  Once again - I hate fuzz ;).

Because now you have silently truncated, which is much worse than
blowing up loudly.  My version also works correctly when sysfs starts
handing out two pages rather than one because something else needed it.

If you want to do this, then please do:

	len = cpumask_scnprintf(buf, PAGE_SIZE, mask);
	if (len == PAGE_SIZE)
		return -ENOMEM;

> > +	BUG_ON(count > PAGE_SIZE);
> 
> Only 'BUG_ON' ??  We have in hand almost certain proof of just
> having scrogged kernel memory.  Time to panic, no?

That would be extremely unusual; we tend not to panic even when things
are bad.  In this case, things are bad because of a static configuration
error, so I find it hard to really care...

> ==
> 
> What are we going to do about the removal of the node_dev->cpumap field,
> and changing this node_read_cpumap() routine to display instead the
> value of node_to_cpumask(node_dev->sysdev.id)?
> 
> Should I do it, or you?  Should it presume your patch above, or collide
> with it, or replace and extend it?

I'll do it; seems like we need negotiation with Greg-KH, too.

> Since I am most impressed with your abilities, since you doubt my
> abilities, and since I'm a lazy s.o.b., you're welcome to it.

That's not true.  I have faith in your abilities; I question anyone's
ability to produce a perfectly balanced solution without any external
input.

Rusty.
-- 
Anyone who quotes me in their signature is an idiot -- Rusty Russell