Messages in this thread | | | Subject | Re: [PATCH] mm/gup_benchmark: GUP_BENCHMARK depends on DEBUG_FS | From | John Hubbard <> | Date | Sat, 7 Nov 2020 19:14:10 -0800 |
| |
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, -- John Hubbard NVIDIA
| |