Messages in this thread | | | Date | Thu, 3 Jun 2004 01:27:28 -0700 | From | Paul Jackson <> | Subject | Re: [PATCH] fix sys cpumap for > 352 NR_CPUS |
| |
> + 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
There are over 300 such constructs in the kernel. And it has the advantage of providing an accurate source line number and a specifiable string.
My preference when checking limits is to check for the exact limit. Adding fuzz only serves to disguise details. Whether coding correctly, or screwing up, best to do so with clarity and precision.
Given all that, how about:
#if ALIGN(NR_CPUS,32)*9/32 > PAGE_SIZE #error "Need 9 bytes space per 32 CPUs in PAGE_SIZE buffer" #endif
==
I also could be tempted to remove BUILD_BUG_ON() from kernel.h, and replace it with a comment:
/* BUILD_BUG_ON() obsolete - consider using #if ... #error ... #endif */
==
> + 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 ;).
==
> + 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?
==
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?
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. Or if you prefer to ask me, that's fine. Seems to me this should be two patches - the one discussed above to limit how many bytes cpumask_scnprintf can posit, and a second to nuke node_dev->cpumap. The second patch would depend on the first, for the trivial reason that they collide on some of the same code.
-- I won't rest till it's the best ... Programmer, Linux Scalability Paul Jackson <pj@sgi.com> 1.650.933.1373 - 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/
| |