RE: [PATCH] mm/gup_benchmark: GUP_BENCHMARK depends on DEBUG_FS

[Song Bao Hua (Barry Song)]

> -----Original Message-----
> From: John Hubbard [mailto:jhubbard@xxxxxxxxxx]
> Sent: Sunday, November 8, 2020 4:14 PM
> To: Song Bao Hua (Barry Song) <song.bao.hua@xxxxxxxxxxxxx>; Randy Dunlap
> <rdunlap@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 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@xxxxxxxxxx]
> >>>> ...
> >> 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