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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Wed, 26 Feb 2020 20:30:30 +0800
From:   Baoquan He <bhe@...hat.com>
To:     Michal Hocko <mhocko@...nel.org>
Cc:     David Hildenbrand <david@...hat.com>, linux-kernel@...r.kernel.org,
        linux-mm@...ck.org, akpm@...ux-foundation.org,
        richardw.yang@...ux.intel.com, osalvador@...e.de,
        dan.j.williams@...el.com, rppt@...ux.ibm.com, robin.murphy@....com
Subject: Re: [PATCH v2 0/7] mm/hotplug: Only use subsection map in VMEMMAP
 case

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
>