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

From: Andrew Morton
Date: Thu Apr 17 2008 - 16:56:19 EST


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/