Re: [PATCH 06/19] x86, mm: setup page table in top-down

[Konrad Rzeszutek Wilk]

> Changelog is always problem now. Looks like everyone is complaining about that.
> 
> Actually I already tried my best on that, really don't know what to do next.

Read on. I've some advice.
> 
> >
> > In particular you should state in clear letters that alloc_low_page is
> > always going to return a page that is already mapped. You should write
> > it both in the commit message and in the code as a comment.
> > This is particularly important because it is going to become part of the
> > interface between the common x86 code and the other x86 subsystems like
> > Xen.
> 
> alloc_low_page() is used in arch/x86/mm/init*.c. How come it becomes
> interface to
> other subsystem?
> 
> I'm not sure if we really need that comment in code for that:
> ---
> the page that alloc_low_page return is directed mapped already, could
> use virtual address
> to access it.
> ---
> 
> >
> > Also, it wouldn't hurt to explain the overall design at the beginning of
> > the series: I shouldn't have to read your code to understand what you
> > are doing. I should read the description of the patch, understand what
> > you are doing, then go and check if the code actually does what you say
> > it does.
> 
> Actually I really don't like to read too long change log in commit.
> Changelog should be concise and precise.

They depend on the patch in question. If it is something simple
(fix compile warning) sure. But for something that is deep in the bowels
of complicated code - then it should have a good explanation that
follows roughly these steps:

 1). Introduce the problem. Explain what is wrong with the existing
     code.
 2). Explain how your patch fixes it.
 3). Explain (if there are any) alternative solutions.

Then re-read it and look at the verb tense. It should be the same tense -
so don't mix them.
--
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/