linux-kernel - Re: [PATCH] mm/gup_benchmark: GUP_BENCHMARK depends on DEBUG

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <dd9e5f78-d627-89d6-2b9d-f2912213171f@nvidia.com>
Date:   Sat, 7 Nov 2020 19:14:10 -0800
From:   John Hubbard <jhubbard@...dia.com>
To:     "Song Bao Hua (Barry Song)" <song.bao.hua@...ilicon.com>,
        Randy Dunlap <rdunlap@...radead.org>,
        "akpm@...ux-foundation.org" <akpm@...ux-foundation.org>,
        "linux-mm@...ck.org" <linux-mm@...ck.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
CC:     Linuxarm <linuxarm@...wei.com>,
        Ralph Campbell <rcampbell@...dia.com>,
        John Garry <john.garry@...wei.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@...dia.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