Re: [PATCH v2 0/7] mm/hotplug: Only use subsection map in VMEMMAP case

Baoquan He
2020-02-26T12:30:45Z

On 02/26/20 at 10:14am, Michal Hocko wrote:
> On Wed 26-02-20 11:42:36, Baoquan He wrote:
> > On 02/25/20 at 11:02am, Michal Hocko wrote:
> > > On Tue 25-02-20 10:10:45, David Hildenbrand wrote:
> > > > >>> include/linux/mmzone.h | 2 +
> > > > >>> mm/sparse.c | 178 +++++++++++++++++++++++++++++------------
> > > > >>> 2 files changed, 127 insertions(+), 53 deletions(-)
> > > > >>
> > > > >> Why do we need to add so much code to remove a functionality from one
> > > > >> memory model?
> > > > >
> > > > > Hmm, Dan also asked this before.
> > > > >
> > > > > The adding mainly happens in patch 2, 3, 4, including the two newly
> > > > > added function defitions, the code comments above them, and those added
> > > > > dummy functions for !VMEMMAP.
> > > >
> > > > AFAIKS, it's mostly a bunch of newly added comments on top of functions.
> > > > E.g., the comment for fill_subsection_map() alone spans 12 LOC in total.
> > > > I do wonder if we have to be that verbose. We are barely that verbose on
> > > > MM code (and usually I don't see much benefit unless it's a function
> > > > with many users from many different places).
> > >
> > > I would tend to agree here. Not that I am against kernel doc
> > > documentation but these are internal functions and the comment doesn't
> > > really give any better insight IMHO. I would be much more inclined if
> > > this was the general pattern in the respective file but it just stands
> > > out.
> >
> > I saw there are internal functions which have code comments, e.g
> > shrink_slab() in mm/vmscan.c, not only this one place, there are several
> > places. I personally prefer to see code comment for function if
> > possible, this can save time, e.g people can skip the bitmap operation
> > when read code if not necessary. And here I mainly want to tell there
> > are different returned value to note different behaviour when call them.
>
> ... yet nobody really cares about different return code. It is an error
> that is propagated up the call chain and that's all.
>
> I also like when there is a kernel doc comment that helps to understand
> the intented usage, context the function can be called from, potential
> side effects, locking requirements and other details people need to know

Fair enough. As I have said, I didn't intend to stick to add kernel doc
comments for these two functions. Will remove them. Thanks for
reviewing.

> when calling functions. But have a look at
> /**
> * clear_subsection_map - Clear subsection map of one memory region
> *
> * @pfn - start pfn of the memory range
> * @nr_pages - number of pfns to add in the region
> *
> * This is only intended for hotplug, and clear the related subsection
> * map inside one section.
> *
> * Return:
> * * -EINVAL - Section already deactived.
> * * 0 - Subsection map is emptied.
> * * 1 - Subsection map is not empty.
> */
>
> the only useful information in here is that this is a hotplug stuff but
> I would be completely lost about the intention without already knowing
> what is this whole subsection about.
>
> --
> Michal Hocko
> SUSE Labs
>