Messages in this thread | | | From | "Song Bao Hua (Barry Song)" <> | Subject | RE: [PATCH] mm/gup_benchmark: GUP_BENCHMARK depends on DEBUG_FS | Date | Sun, 8 Nov 2020 04:05:35 +0000 |
| |
> -----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
| |