RE: [PATCH] mm/gup_benchmark: GUP_BENCHMARK depends on DEBUG_FS

[Song Bao Hua (Barry Song)]

> -----Original Message-----
> From: John Hubbard [mailto:jhubbard@xxxxxxxxxx]
> Sent: Saturday, November 7, 2020 1:13 PM
> To: Song Bao Hua (Barry Song) <song.bao.hua@xxxxxxxxxxxxx>;
> akpm@xxxxxxxxxxxxxxxxxxxx; linux-mm@xxxxxxxxx;
> linux-kernel@xxxxxxxxxxxxxxx
> Cc: Linuxarm <linuxarm@xxxxxxxxxx>; Ralph Campbell
> <rcampbell@xxxxxxxxxx>; John Garry <john.garry@xxxxxxxxxx>
> Subject: Re: [PATCH] mm/gup_benchmark: GUP_BENCHMARK depends on
> DEBUG_FS
> 
> On 11/4/20 2:05 AM, Barry Song wrote:
> > Without DEBUG_FS, all the code in gup_benchmark becomes meaningless.
> > For sure kernel provides debugfs stub while DEBUG_FS is disabled, but
> > the point here is that GUP_BENCHMARK can do nothing without DEBUG_FS.
> >
> > Cc: John Hubbard <jhubbard@xxxxxxxxxx>
> > Cc: Ralph Campbell <rcampbell@xxxxxxxxxx>
> > Inspired-by: John Garry <john.garry@xxxxxxxxxx>
> > Signed-off-by: Barry Song <song.bao.hua@xxxxxxxxxxxxx>
> > ---
> >   * inspired by John's comment in this patch:
> >
> > https://lore.kernel.org/linux-iommu/184797b8-512e-e3da-fae7-25c7d66264
> > 8b@xxxxxxxxxx/
> >
> >   mm/Kconfig | 1 +
> >   1 file changed, 1 insertion(+)
> >
> > diff --git a/mm/Kconfig b/mm/Kconfig
> > index d42423f..91fa923 100644
> > --- a/mm/Kconfig
> > +++ b/mm/Kconfig
> > @@ -836,6 +836,7 @@ config PERCPU_STATS
> >
> >   config GUP_BENCHMARK
> >   	bool "Enable infrastructure for get_user_pages() and related calls
> benchmarking"
> > +	depends on DEBUG_FS
> 
> 
> I think "select DEBUG_FS" is better here. "depends on" has the obnoxious
> behavior of hiding the choice from you, if the dependencies aren't already met.
> Whereas what the developer *really* wants is a no-nonsense activation of the
> choice: "enable GUP_BENCHMARK and the debug fs that it requires".
> 

To some extent, I agree with you. But I still think here it is better to use "depends on".
According to
https://www.kernel.org/doc/Documentation/kbuild/kconfig-language.txt

	select should be used with care. select will force
	a symbol to a value without visiting the dependencies.
	By abusing select you are able to select a symbol FOO even
	if FOO depends on BAR that is not set.
	In general use select only for non-visible symbols
	(no prompts anywhere) and for symbols with no dependencies.
	That will limit the usefulness but on the other hand avoid
	the illegal configurations all over.

On the other hand, in kernel there are 78 "depends on DEBUG_FS" and
only 14 "select DEBUG_FS".

$ git grep "depends on" | grep DEBUG_FS | wc -l
78

$ git grep "select" | grep DEBUG_FS | wc -l
14

> So depends on really on is better for things that you just can't control, such as
> the cpu arch you're on, etc.
> 
> Also note that this will have some minor merge conflict with mmotm, Due to
> renaming to GUP_TEST. No big deal though.
> 
> 
> thanks,
> --
> John Hubbard
> NVIDIA

Thanks
Barry