Messages in this thread | | | From | Nick Piggin <> | Subject | Re: [git pull] cpus4096 fixes | Date | Mon, 28 Jul 2008 16:58:48 +1000 |
| |
On Monday 28 July 2008 16:34, Rusty Russell wrote: > On Monday 28 July 2008 13:06:36 Andrew Morton wrote: > > On Mon, 28 Jul 2008 10:42:12 +1000 Rusty Russell <rusty@rustcorp.com.au> > > wrote: > > > The 4k CPU patches have been sliding in without review up until now. > > > > wot? > > This surprises you? I stumbled across the cpumask_of_cpu() bug because I > happened to want it for stop_machine and read the damned code. But it lead > me to the surrounding code, which is pretty questionable. An arch-specific > map, rather than depending on NR_CPUS? Adding set_cpus_allowed_ptr() > instead of changing set_cpus_allowed()? Macros which declare things and > may or may not do an allocation/free? Finally a patch so horrifically ugly > that it can't be ignored any more gets all the way to Linus. > > Overall, it seems like an attempt to sneak in gradual workarounds for > cpumasks on the stack, rather than a coherent plan. I understand the > temptation to avoid an "are we prepared to pay this price for large > NR_CPUS?" discussion, but we need it anyway. > > And that's what I call "review".
Well, I'm not talking about this case specifically, but lots of people I see these days tend to "review" code without doing a lot of high level critical thinking (is the overall design any good, could it be better or completely different, are the tradeoffs worthwhile etc).
Don't get me wrong, the lower level spot-the-bug and coding style etc reviews are indespensible too...
Unfortunately the development process isn't exactly conducive to that higher level kind of review... there are pros and cons to this, it's not all bad...
| |