| Message ID | 20201104100552.20156-1-song.bao.hua@hisilicon.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | mm/gup_benchmark: GUP_BENCHMARK depends on DEBUG_FS | expand |
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@nvidia.com> > Cc: Ralph Campbell <rcampbell@nvidia.com> > Inspired-by: John Garry <john.garry@huawei.com> > Signed-off-by: Barry Song <song.bao.hua@hisilicon.com> > --- > * inspired by John's comment in this patch: > https://lore.kernel.org/linux-iommu/184797b8-512e-e3da-fae7-25c7d662648b@huawei.com/ > > 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". 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,
> -----Original Message----- > From: John Hubbard [mailto:jhubbard@nvidia.com] > Sent: Saturday, November 7, 2020 1:13 PM > To: Song Bao Hua (Barry Song) <song.bao.hua@hisilicon.com>; > akpm@linux-foundation.org; linux-mm@kvack.org; > linux-kernel@vger.kernel.org > Cc: Linuxarm <linuxarm@huawei.com>; Ralph Campbell > <rcampbell@nvidia.com>; John Garry <john.garry@huawei.com> > 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@nvidia.com> > > Cc: Ralph Campbell <rcampbell@nvidia.com> > > Inspired-by: John Garry <john.garry@huawei.com> > > Signed-off-by: Barry Song <song.bao.hua@hisilicon.com> > > --- > > * inspired by John's comment in this patch: > > > > https://lore.kernel.org/linux-iommu/184797b8-512e-e3da-fae7-25c7d66264 > > 8b@huawei.com/ > > > > 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
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. thanks,
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. 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.
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,
On 11/7/20 4:03 PM, John Hubbard wrote: > 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 > OK, thanks, I see how you get that list now. JFTR, those are not 42 independent users of DEBUG_FS. There are lots of &&s and a few ||s in that list. xconfig shows this for DEBUG_FS: (13 selects for x86_64 allmodconfig) Selected by [y]: - PAGE_OWNER [=y] && DEBUG_KERNEL [=y] && STACKTRACE_SUPPORT [=y] - DEBUG_KMEMLEAK [=y] && DEBUG_KERNEL [=y] && HAVE_DEBUG_KMEMLEAK [=y] - BLK_DEV_IO_TRACE [=y] && TRACING_SUPPORT [=y] && FTRACE [=y] && SYSFS [=y] && BLOCK [=y] - FAIL_FUTEX [=y] && FAULT_INJECTION [=y] && FUTEX [=y] - KCOV [=y] && ARCH_HAS_KCOV [=y] && (CC_HAS_SANCOV_TRACE_PC [=y] || GCC_PLUGINS [=n]) Selected by [m]: - ZSMALLOC_STAT [=y] && ZSMALLOC [=m] - BCACHE_CLOSURES_DEBUG [=y] && MD [=y] && BCACHE [=m] - DVB_C8SECTPFE [=m] && MEDIA_SUPPORT [=m] && MEDIA_PLATFORM_SUPPORT [=y] && DVB_PLATFORM_DRIVERS [=y] && PINCTRL [=y] && DVB_CORE [=m] && I2C [=y] && (ARCH_STI || ARCH_MULTIPLATFORM || COMPILE_TEST [=y]) - DRM_I915_DEBUG [=y] && HAS_IOMEM [=y] && EXPERT [=y] && DRM_I915 [=m] - EDAC_DEBUG [=y] && EDAC [=m] - SUNRPC_DEBUG [=y] && NETWORK_FILESYSTEMS [=y] && SUNRPC [=m] && SYSCTL [=y] - PUNIT_ATOM_DEBUG [=m] && PCI [=y] - NOTIFIER_ERROR_INJECTION [=m] && DEBUG_KERNEL [=y] Some other $ARCH could be more... >> >> 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, thanks.
On 11/7/20 4:24 PM, Randy Dunlap wrote: > On 11/7/20 4:03 PM, John Hubbard wrote: >> 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: ... > OK, thanks, I see how you get that list now. > > JFTR, those are not 42 independent users of DEBUG_FS. There are lots of &&s > and a few ||s in that list. > You are right, and that means that I have to withdraw my earlier claim that "42 committers" made such a decision. > > xconfig shows this for DEBUG_FS: (13 selects for x86_64 allmodconfig) > ...maybe only 13 committers and dependency chain, then. :) thanks,
> -----Original Message----- > From: John Hubbard [mailto:jhubbard@nvidia.com] > Sent: Sunday, November 8, 2020 1:03 PM > To: Randy Dunlap <rdunlap@infradead.org>; Song Bao Hua (Barry Song) > <song.bao.hua@hisilicon.com>; akpm@linux-foundation.org; > linux-mm@kvack.org; linux-kernel@vger.kernel.org > Cc: Linuxarm <linuxarm@huawei.com>; Ralph Campbell > <rcampbell@nvidia.com>; John Garry <john.garry@huawei.com> > 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@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. :) 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
On 11/7/20 6:58 PM, Song Bao Hua (Barry Song) wrote: >> 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: >>>>>> From: John Hubbard [mailto:jhubbard@nvidia.com] >>>> ... >> 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. > Looking at the choices, from the user's (usually kernel developer's) experience: a) The user enables GUP_TEST, then boots up, runs, and is briefly surprised by a runtime failure. But it's a very quick diagnosis: "open: No such file or directory", when trying to make that ioctl call. The path indicates that it's a debug fs path, so the solution is pretty clear, at least for the main audience. b) The other choice: the user *never even sees* GUP_TEST as a choice. This especially bothers me because sometimes you find things by poking around in the menu, although of course "you should already know about it"...but there's a lot to "already know" in a large kernel. From a user experience, it's way better to simply see what you want, and select it in the menu. Or, at least get some prompt that you need to pre-select something else. > 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 Right, but first of course they must read every single line of the test code carefully. And while it is true the you often *do* end up reading most or all of the test code, there are situations in which you don't need to. We'd be taking away some of those situations. :) > they must manually enable it if they haven't. That's why I think this patch is > making things better. > ...which makes things a little bit worse. thanks,
On 11/7/20 7:14 PM, John Hubbard wrote: > On 11/7/20 6:58 PM, Song Bao Hua (Barry Song) wrote: >>> 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: >>>>>>> From: John Hubbard [mailto:jhubbard@nvidia.com] >>>>> ... >>> 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. >> > > Looking at the choices, from the user's (usually kernel developer's) experience: > > a) The user enables GUP_TEST, then boots up, runs, and is briefly surprised by a > runtime failure. But it's a very quick diagnosis: "open: No such file or directory", > when trying to make that ioctl call. The path indicates that it's a debug fs path, > so the solution is pretty clear, at least for the main audience. > > b) The other choice: the user *never even sees* GUP_TEST as a choice. This especially > bothers me because sometimes you find things by poking around in the menu, although > of course "you should already know about it"...but there's a lot to "already know" > in a large kernel. > > From a user experience, it's way better to simply see what you want, and select it > in the menu. Or, at least get some prompt that you need to pre-select something else. > ...and again, this is all fallout from Kconfig. I might be missing some advanced feature, because it seems surprising to only be allowed to choose between missing dependencies (which this patch would correct), or a poorer user experience (which I argue that this patch would also provide). Ideally, we'd just set up the dependencies, and then have some options for visibility, but I'm not aware of any way to do that...and after a quick peek at Documentation/kbuild/kconfig-macro-language.rst it looks pretty bare bones. thanks,
> -----Original Message----- > From: John Hubbard [mailto:jhubbard@nvidia.com] > Sent: Sunday, November 8, 2020 4:14 PM > To: Song Bao Hua (Barry Song) <song.bao.hua@hisilicon.com>; Randy Dunlap > <rdunlap@infradead.org>; akpm@linux-foundation.org; linux-mm@kvack.org; > linux-kernel@vger.kernel.org > Cc: Linuxarm <linuxarm@huawei.com>; Ralph Campbell > <rcampbell@nvidia.com>; John Garry <john.garry@huawei.com> > Subject: Re: [PATCH] mm/gup_benchmark: GUP_BENCHMARK depends on > DEBUG_FS > > On 11/7/20 6:58 PM, Song Bao Hua (Barry Song) wrote: > >> 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: > >>>>>> From: John Hubbard [mailto:jhubbard@nvidia.com] > >>>> ... > >> 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. > > > > Looking at the choices, from the user's (usually kernel developer's) experience: > > a) The user enables GUP_TEST, then boots up, runs, and is briefly surprised by a > runtime failure. But it's a very quick diagnosis: "open: No such file or > directory", > when trying to make that ioctl call. The path indicates that it's a debug fs path, > so the solution is pretty clear, at least for the main audience. users could have these two different stories: Story A: users want to use GUP_TEST, so they simply enable GUP_TEST, build a kernel and run the test. Then they get failed at runtime but the kernel build has no any issue. Then they have to read the code, and figure out DEBUG_FS is a must-have, then they enable DEBUG_FS afterwards. After that, they re-build kernel and re-test. Users might have wasted one hour on it. Story B: if we put "depends on", users want to use GUP_TEST, then they try to enable "GUP_TEST", but they couldn't enable it at all since DEBUG_FS is not enabled. And they use "/" to search GUP_TEST, menuconfig will show "GUP_TEST" depend on "DEBUG_FS", they will enable DEBUG_FS to get GUP_TEST enabled. For story B, users only spend one minute in menuconfig :-) > > b) The other choice: the user *never even sees* GUP_TEST as a choice. This > especially > bothers me because sometimes you find things by poking around in the menu, > although > of course "you should already know about it"...but there's a lot to "already > know" > in a large kernel. > > From a user experience, it's way better to simply see what you want, and > select it > in the menu. Or, at least get some prompt that you need to pre-select > something else. > If we type "/" to search GUP_TEST, menuconfig will show it depends on DEBUG_FS and show the status of DEBUG_FS Y or N. Wouldn't it has been a prompt? > > > 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 > > > Right, but first of course they must read every single line of the test code > carefully. And while it is true the you often *do* end up reading most or > all of the test code, there are situations in which you don't need to. We'd > be taking away some of those situations. :) > An careless engineer like me often ignore some dependency even after I have read code carefully. "depends on" will enforce me to resolve the dependency during build stage and save me much time :-) > > > they must manually enable it if they haven't. That's why I think this patch is > > making things better. > > > > ...which makes things a little bit worse. For this case, I am also happy with "select" as it also resolves the problem of story A. Just kconfig documentation says "select" should be used with care. > > > thanks, > -- > John Hubbard > NVIDIA Thanks Barry
On 11/7/20 7:22 PM, John Hubbard wrote: > On 11/7/20 7:14 PM, John Hubbard wrote: >> On 11/7/20 6:58 PM, Song Bao Hua (Barry Song) wrote: >>>> 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: >>>>>>>> From: John Hubbard [mailto:jhubbard@nvidia.com] >>>>>> ... >>>> 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. >>> >> >> Looking at the choices, from the user's (usually kernel developer's) experience: >> >> a) The user enables GUP_TEST, then boots up, runs, and is briefly surprised by a >> runtime failure. But it's a very quick diagnosis: "open: No such file or directory", >> when trying to make that ioctl call. The path indicates that it's a debug fs path, >> so the solution is pretty clear, at least for the main audience. >> >> b) The other choice: the user *never even sees* GUP_TEST as a choice. This especially >> bothers me because sometimes you find things by poking around in the menu, although >> of course "you should already know about it"...but there's a lot to "already know" >> in a large kernel. >> >> From a user experience, it's way better to simply see what you want, and select it >> in the menu. Or, at least get some prompt that you need to pre-select something else. >> > > ...and again, this is all fallout from Kconfig. I might be missing some advanced > feature, because it seems surprising to only be allowed to choose between > missing dependencies (which this patch would correct), or a poorer user experience > (which I argue that this patch would also provide). > > Ideally, we'd just set up the dependencies, and then have some options for > visibility, but I'm not aware of any way to do that...and after a quick peek > at Documentation/kbuild/kconfig-macro-language.rst it looks pretty bare bones. Look at kconfig-language.rst instead. One thing that could be done (and is done in a few places for other reasons) is to add a Kconfig comment if DEBUG_FS is not enabled: comment "GUP_TEST needs to have DEBUG_FS enabled" depends on !GUP_TEST && !DEBUG_FS e.g. drivers/hid/usbhid/Kconfig: comment "Input core support is needed for USB HID input layer or HIDBP support" depends on USB_HID && INPUT=n
> -----Original Message----- > From: Randy Dunlap [mailto:rdunlap@infradead.org] > Sent: Sunday, November 8, 2020 5:12 PM > To: John Hubbard <jhubbard@nvidia.com>; Song Bao Hua (Barry Song) > <song.bao.hua@hisilicon.com>; akpm@linux-foundation.org; > linux-mm@kvack.org; linux-kernel@vger.kernel.org > Cc: Linuxarm <linuxarm@huawei.com>; Ralph Campbell > <rcampbell@nvidia.com>; John Garry <john.garry@huawei.com> > Subject: Re: [PATCH] mm/gup_benchmark: GUP_BENCHMARK depends on > DEBUG_FS > > On 11/7/20 7:22 PM, John Hubbard wrote: > > On 11/7/20 7:14 PM, John Hubbard wrote: > >> On 11/7/20 6:58 PM, Song Bao Hua (Barry Song) wrote: > >>>> 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: > >>>>>>>> From: John Hubbard [mailto:jhubbard@nvidia.com] > >>>>>> ... > >>>> 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. > >>> > >> > >> Looking at the choices, from the user's (usually kernel developer's) > experience: > >> > >> a) The user enables GUP_TEST, then boots up, runs, and is briefly surprised > by a > >> runtime failure. But it's a very quick diagnosis: "open: No such file or > directory", > >> when trying to make that ioctl call. The path indicates that it's a debug fs > path, > >> so the solution is pretty clear, at least for the main audience. > >> > >> b) The other choice: the user *never even sees* GUP_TEST as a choice. This > especially > >> bothers me because sometimes you find things by poking around in the > menu, although > >> of course "you should already know about it"...but there's a lot to "already > know" > >> in a large kernel. > >> > >> From a user experience, it's way better to simply see what you want, and > select it > >> in the menu. Or, at least get some prompt that you need to pre-select > something else. > >> > > > > ...and again, this is all fallout from Kconfig. I might be missing some advanced > > feature, because it seems surprising to only be allowed to choose between > > missing dependencies (which this patch would correct), or a poorer user > experience > > (which I argue that this patch would also provide). > > > > Ideally, we'd just set up the dependencies, and then have some options for > > visibility, but I'm not aware of any way to do that...and after a quick peek > > at Documentation/kbuild/kconfig-macro-language.rst it looks pretty bare > bones. > > Look at kconfig-language.rst instead. > > One thing that could be done (and is done in a few places for other reasons) is > to add > a Kconfig comment if DEBUG_FS is not enabled: > > comment "GUP_TEST needs to have DEBUG_FS enabled" > depends on !GUP_TEST && !DEBUG_FS > > e.g. drivers/hid/usbhid/Kconfig: > > comment "Input core support is needed for USB HID input layer or HIDBP > support" > depends on USB_HID && INPUT=n This is interesting. Thanks for sharing this. I've never realized we can do this kind of comments. What I have been always doing is simply reading the status from menuconfig. For example, to use IIO_CONSUMERS_PER_TRIGGER, I search it in menuconfig by "/", I get: Symbol: IIO_CONSUMERS_PER_TRIGGER [=] │ Type : integer │ Defined at drivers/iio/Kconfig:41 │ Prompt: Maximum number of consumers per trigger │ Depends on: IIO [=n] && IIO_TRIGGER [=n] │ Location: │ -> Device Drivers │ (1) -> Industrial I/O support (IIO [=n]) │ -> Enable triggered sampling support (IIO_TRIGGER [=n]) │ The menuconfig tells me it depends on IIO and IIO_TRIGGER. But both of them are "n". so the next step, I am going to make IIO and IIO_TRIGGER "y". > > > -- > ~Randy Thanks Barry
On 11/7/20 8:11 PM, Randy Dunlap wrote: ... > Look at kconfig-language.rst instead. aha, yes. > > One thing that could be done (and is done in a few places for other reasons) is to add > a Kconfig comment if DEBUG_FS is not enabled: > > comment "GUP_TEST needs to have DEBUG_FS enabled" > depends on !GUP_TEST && !DEBUG_FS > Sweet--I just applied that here, and it does exactly what I wanted: puts a nice clear message on the "make menuconfig" screen. No more hidden item. Brilliant! Let's go with that, shall we? thanks,
> -----Original Message----- > From: John Hubbard [mailto:jhubbard@nvidia.com] > Sent: Sunday, November 8, 2020 5:56 PM > To: Randy Dunlap <rdunlap@infradead.org>; Song Bao Hua (Barry Song) > <song.bao.hua@hisilicon.com>; akpm@linux-foundation.org; > linux-mm@kvack.org; linux-kernel@vger.kernel.org > Cc: Linuxarm <linuxarm@huawei.com>; Ralph Campbell > <rcampbell@nvidia.com>; John Garry <john.garry@huawei.com> > Subject: Re: [PATCH] mm/gup_benchmark: GUP_BENCHMARK depends on > DEBUG_FS > > On 11/7/20 8:11 PM, Randy Dunlap wrote: > ... > > Look at kconfig-language.rst instead. > > aha, yes. > > > > > One thing that could be done (and is done in a few places for other reasons) > is to add > > a Kconfig comment if DEBUG_FS is not enabled: > > > > comment "GUP_TEST needs to have DEBUG_FS enabled" > > depends on !GUP_TEST && !DEBUG_FS > > > > Sweet--I just applied that here, and it does exactly what I wanted: puts a nice > clear > message on the "make menuconfig" screen. No more hidden item. Brilliant! > > Let's go with that, shall we? Do you want this (Code A) diff --git a/mm/Kconfig b/mm/Kconfig index 01b0ae0cd9d3..d80839d1fad8 100644 --- a/mm/Kconfig +++ b/mm/Kconfig @@ -853,6 +853,9 @@ config GUP_TEST See tools/testing/selftests/vm/gup_test.c +comment "GUP_TEST needs to have DEBUG_FS enabled" + depends on !GUP_TEST && !DEBUG_FS + config GUP_GET_PTE_LOW_HIGH bool Or do you want this ? (Code B) diff --git a/mm/Kconfig b/mm/Kconfig index 01b0ae0cd9d3..a7ff0d31afd5 100644 --- a/mm/Kconfig +++ b/mm/Kconfig @@ -836,6 +836,7 @@ config PERCPU_STATS config GUP_TEST bool "Enable infrastructure for get_user_pages()-related unit tests" + depends on DEBUG_FS help Provides /sys/kernel/debug/gup_test, which in turn provides a way to make ioctl calls that can launch kernel-based unit tests for @@ -853,6 +854,9 @@ config GUP_TEST See tools/testing/selftests/vm/gup_test.c +comment "GUP_TEST needs to have DEBUG_FS enabled" + depends on !GUP_TEST && !DEBUG_FS + config GUP_GET_PTE_LOW_HIGH bool To be honest, I am not a big fan of both of code A and B. I think "depends on" has clearly said everything the redundant comment wants to say. │ Symbol: GUP_TEST [=] │ Type : bool │ Defined at mm/Kconfig:837 │ Prompt: Enable infrastructure for get_user_pages()-related unit tests │ Depends on: DEBUG_FS [=n] │ Location: │ (1) -> Memory Management option Menuconfig shows GUP_TEST depends on DEBUG_FS and right now DEBUG_FS is "n". so it is impossible to enable GUP_TEST. "comment" is a good thing, but it is more likely to be used for a menu or a group of configurations to extend a bundle of things. On the other hand, If this particular case needs this comment, so do countless other configurations in hundreds of Kconfig files. My favorite order is 1. depends on 2. select 3. depends on + comment(code B) 4. comment only(code A) I won't accept 4, but it seems we can't get agreement on 1 which is my favorite. So if you want either one of 2 and 3, I am happy to send v2 for that. So which one is your favorite? 2 or 3? > > thanks, > -- > John Hubbard > NVIDIA Thanks Barry
On 11/7/20 11:35 PM, Song Bao Hua (Barry Song) wrote: > Or do you want this ? > > (Code B) > diff --git a/mm/Kconfig b/mm/Kconfig > index 01b0ae0cd9d3..a7ff0d31afd5 100644 > --- a/mm/Kconfig > +++ b/mm/Kconfig > @@ -836,6 +836,7 @@ config PERCPU_STATS > > config GUP_TEST > bool "Enable infrastructure for get_user_pages()-related unit tests" > + depends on DEBUG_FS > help > Provides /sys/kernel/debug/gup_test, which in turn provides a way > to make ioctl calls that can launch kernel-based unit tests for > @@ -853,6 +854,9 @@ config GUP_TEST > > See tools/testing/selftests/vm/gup_test.c > > +comment "GUP_TEST needs to have DEBUG_FS enabled" > + depends on !GUP_TEST && !DEBUG_FS > + > config GUP_GET_PTE_LOW_HIGH > bool The above, please. It's your original patch, plus a way to display something if the "depends on" is not met. > > To be honest, I am not a big fan of both of code A and B. I think "depends on" has > clearly said everything the redundant comment wants to say. It's not really just a "comment", in the sense of commenting the Kconfig sources. It actually has an effect, which is that it displays something in "make menuconfig". So it's not redundant, because it provides that output *instead* of hiding the option entirely, when !DEBUG_FS. Try it out, it's nice. > > │ Symbol: GUP_TEST [=] > │ Type : bool > │ Defined at mm/Kconfig:837 > │ Prompt: Enable infrastructure for get_user_pages()-related unit tests > │ Depends on: DEBUG_FS [=n] > │ Location: > │ (1) -> Memory Management option > > Menuconfig shows GUP_TEST depends on DEBUG_FS and right now DEBUG_FS is > "n". so it is impossible to enable GUP_TEST. > > "comment" is a good thing, but it is more likely to be used for a menu or a group > of configurations to extend a bundle of things. > > On the other hand, If this particular case needs this comment, so do countless > other configurations in hundreds of Kconfig files. Well, maybe, yes. I personally find it quite difficult, having options appear and disappear on me, in this system. If they all had this "comment" behavior by default, to show up as a placeholder, I think it would be a better user experience. thanks,
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 help Provides /sys/kernel/debug/gup_benchmark that helps with testing performance of get_user_pages() and related calls.
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@nvidia.com> Cc: Ralph Campbell <rcampbell@nvidia.com> Inspired-by: John Garry <john.garry@huawei.com> Signed-off-by: Barry Song <song.bao.hua@hisilicon.com> --- * inspired by John's comment in this patch: https://lore.kernel.org/linux-iommu/184797b8-512e-e3da-fae7-25c7d662648b@huawei.com/ mm/Kconfig | 1 + 1 file changed, 1 insertion(+)