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: Sunday, November 8, 2020 1:03 PM
> To: Randy Dunlap <rdunlap@xxxxxxxxxxxxx>; 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/7/20 2:20 PM, Randy Dunlap wrote:
> > On 11/7/20 11:16 AM, John Hubbard wrote:
> >> On 11/7/20 11:05 AM, Song Bao Hua (Barry Song) wrote:
> >>>> -----Original Message-----
> >>>> From: John Hubbard [mailto:jhubbard@xxxxxxxxxx]
> >> ...
> >>>>>     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".
> >>>
> >>
> >> You're not looking at the best statistics. Go look at what *already* selects
> >> DEBUG_FS, and you'll find about 50 items.
> >
> > Sorry, I'm not following you. I see the same 14 "select DEBUG_FS" as Barry.
> 
> I ran make menuconfig, and looked at it. Because I want to see the true end
> result,
> and I didn't trust my grep use, given that the system has interlocking
> dependencies,
> and I think one select could end up activating others (yes?).
> 
> And sure enough, there are 42 items listed, here they are, cleaned up so that
> there
> is one per line:
> 
> ZSMALLOC_STAT [=n]
> ZSMALLOC [=m]
> BCACHE_CLOSURES_DEBUG [=n]
> MD [=y]
> BCACHE [=n]
> DVB_C8SECTPFE [=n]
> MEDIA_SUPPORT [=m]
> MEDIA_PLATFORM_SUPPORT [=y]
> DVB_PLATFORM_DRIVERS [=n]
> PINCT
> DRM_I915_DEBUG [=n]
> HAS_IOMEM [=y]
> EXPERT [=y]
> DRM_I915 [=m]
> EDAC_DEBUG [=n]
> EDAC [=y]
> SUNRPC_DEBUG [=n]
> NETWORK_FILESYSTEMS [=y]
> SUNRPC [=m]
> SYSCTL [=y]
> PAGE_OWNER [=n]
> DEBUG_KERNEL [=y]
> STACKTRACE_SUPPORT [=y]
> DEBUG_KMEMLEAK [=n]
> DEBUG_KERNEL [=y]
> HAVE_DEBUG_KMEMLEAK [=y]
> BLK_DEV_IO_TRACE [=n]
> TRACING_SUPPORT [=y]
> FTRACE [=y]
> SYSFS [=y]
> BLOCK [=y]
> PUNIT_ATOM_DEBUG [=n]
> PCI [=y]
> NOTIFIER_ERROR_INJECTION [=n]
> DEBUG_KERNEL [=y]
> FAIL_FUTEX [=n]
> FAULT_INJECTION [=n]
> FUTEX [=y]
> KCOV [=n]
> ARCH_HAS_KCOV [=y]
> CC_HAS_SANCOV_TRACE_PC [=y]
> GCC_PLUGINS
> 
> 
> >
> > In general we don't want any one large "feature" (or subsystem) to be
> enabled
> > by one driver. If someone has gone to the trouble to disable DEBUG_FS (or
> whatever),
> > then a different Kconfig symbol shouldn't undo that.
> >
> 
> I agree with the "in general" point, yes. And my complaint is really 80% due to
> the
> very unhappy situation with Kconfig, where we seem to get a choice between
> *hiding*
> a feature, or forcing a dependency break. What we really want is a way to
> indicate
> a dependency that doesn't hide entire features, unless we want that. (Maybe I
> should
> attempt to get into the implementation, although I suspect it's harder than I
> realize.)
> 
> But the other 20% of my complaint is, given what we have, I think the
> appropriate
> adaptation for GUP_BENCHMARK's relationship to DEBUG_FS *in particular*, is:
> select.
> 
> And 42 other committers have chosen the same thing, for their relationship to
> DEBUG_FS. I'm in good company.
> 
> But if you really disagree, then I'd go with, just drop the patch entirely, because
> it doesn't really make things better as written...IMHO anyway. :)

Just imagine a case, we don't enable DEBUG_FS but we enable GUP_TEST, we will
get an image with totally useless code section since GUP_TEST depends on debugfs
entry to perform any useful functionality.

The difference between "depends on" and "select" for this case is like:
depends on: if we want to use GUP_TEST, we have to enable DEBUG_FS first;
select: if we enable GUP_TEST, Kconfig will enable DEBUG_FS automatically.

To me, I am 60% inclined to "depends on" as I think "DEBUG_FS" is more
of a pre-condition of GUP_TEST than an internal part of GUP_TEST. So people
should realize the pre-condition must be met before using GUP_TEST and
they must manually enable it if they haven't. That's why I think this patch is
making things better.

However, as I replied before, to some extent, I also agree with you. if most
people vote for "select" for this particular case, I'm also happy to use "select".

> 
> thanks,
> --
> John Hubbard
> NVIDIA

Thanks
Barry