Messages in this thread | | | Subject | Re: [PATCH] mm/gup_benchmark: GUP_BENCHMARK depends on DEBUG_FS | From | John Hubbard <> | Date | Sat, 7 Nov 2020 16:03:10 -0800 |
| |
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@nvidia.com] >> ... >>>>> 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. :)
thanks, -- John Hubbard NVIDIA
| |