lkml.org 
[lkml]   [2020]   [Nov]   [7]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
SubjectRE: [PATCH] mm/gup_benchmark: GUP_BENCHMARK depends on DEBUG_FS
Date


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

\
 
 \ /
  Last update: 2020-11-08 05:35    [W:0.062 / U:2.088 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site