Re: [v2.6.26] what's brewing in x86.git for v2.6.26

[Andrew Morton]

On Thu, 17 Apr 2008 22:39:55 +0200
"Vegard Nossum" <vegard.nossum@xxxxxxxxx> wrote:

> Hi Andrew,
> 
> Thank you very much for the review of this patch. Those are hard to
> come by, and I've posted kmemcheck to LKML already 3 or 4 times, with
> relatively sparse response. I mean, the fact that they were ALL
> whitespace damaged, but discovered by nobody, quite plainly tells me
> that nobody actually tried to apply it (except perhaps Daniel Walker,
> but we never realized it was whitespace damage causing the problems).
> The patches that Ingo took into x86 were probably sent as an
> attachment...

hm.  Our review problem is well-understood.

> to Ingo before sending this to the list, so he should know. But it
> should not have been part of this patch, I agree.
> 
> >  >  early_param("kmemcheck", param_kmemcheck);
> >
> >  kmemcheck= is documented in at least three places, which is nice, but it
> >  isn't mentioned in the place where we document kernel-parameters:
> >  Documentation/kernel-parameters.txt.  A brief section there which directs
> >  the user to the extended docs would be fine.
> >
> >  early_param() is unusual - we normally use __setup().  I assume there's a
> >  reason for using early_param(), but that reason cannot be discerned from
> >  reading the code.  A /*comment*/ is the way to fix that.
> 
> The reason is that we need to set this before kmalloc() is ever
> called. A comment will come.
> 
> But it seems that __setup() is what is really missing a comment. I
> don't know what it is or how it works, and the comments around the
> definition are not very helpful. Maybe somebody could enlighten me?

It makes my head spin too.  Reading through the first bit of
init/main.c:start_kernel() is your best hope, sorry.

> >  > +static pte_t *
> >  > +address_get_pte(unsigned int address)
> >
> >  This is not the preferred way of laying out function declarations but I've
> >  basically given up on this one.
> >
> >         (void *)address
> >
> >  is more common, but I'm close to giving up on that too.
> >
> >  >  static int
> >  > -show_addr(uint32_t addr)
> >  > +show_addr(uint32_t address)
> >
> >  u32 is preferred, but ditto.
> 
> This will be turned into unsigned long with 64-bit support. (Hopefully
> we can get that working too.)
> 
> Changing these to match the rest of the kernel is no problem for me.
> It is not the way I would write it, but Pekka and Ingo has already
> forced me to write if () instead of if(), so there should be no reason
> to stop here! :-)

These things are OK as-is, I think.  It'd be somewhat less nice in
situations where newly-added code was inconsistent with surrounding
existing code but it's still hardly a huge problem.

> >  Perhaps we should get all this code onto the list(s) for re-review.  It's
> >  been a while..
> 
> I'm not sure it would make much of a difference, except perhaps for
> you, if you want to review it all. (My latest post to LKML had 0
> replies in total. Well, except private e-mail exchange with Ingo and
> Pekka; they should know the code already. Once again, thanks to them
> for helping me.) Do you still want me to post it again?

mm...  I wouldn't mind taking a closer look at it all.  That documentation
file makes it _much_ easier to review the code, and the review becomes more
effective than it would otherwise be.

> 
> PS: And it's not that I do that much testing/reviewing myself. But I
> do think I have the excuse of being a newbie at this :-)

You're in good company ;)
--
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/