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

From: Baoquan He <bhe@redhat.com>
To: Michal Hocko <mhocko@kernel.org>
Cc: David Hildenbrand <david@redhat.com>,
	linux-kernel@vger.kernel.org, linux-mm@kvack.org,
	akpm@linux-foundation.org, richardw.yang@linux.intel.com,
	osalvador@suse.de, dan.j.williams@intel.com, rppt@linux.ibm.com,
	robin.murphy@arm.com
Subject: Re: [PATCH v2 0/7] mm/hotplug: Only use subsection map in VMEMMAP case
Date: Wed, 26 Feb 2020 20:30:30 +0800	[thread overview]
Message-ID: <20200226123030.GG24216@MiWiFi-R3L-srv> (raw)
In-Reply-To: <20200226091421.GE3771@dhcp22.suse.cz>

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
> 