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. :)
thanks,
--
John Hubbard
NVIDIA
