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 >
