> -----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