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

["Song Bao Hua (Barry Song)" <song.bao.hua@hisilicon.com>]

> -----Original Message-----
> From: John Hubbard [mailto:jhubbard@nvidia.com]
> Sent: Sunday, November 8, 2020 5:56 PM
> To: Randy Dunlap <rdunlap@infradead.org>; 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 8:11 PM, Randy Dunlap wrote:
> ...
> > Look at kconfig-language.rst instead.
> 
> aha, yes.
> 
> >
> > 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
> >
> 
> Sweet--I just applied that here, and it does exactly what I wanted: puts a nice
> clear
> message on the "make menuconfig" screen. No more hidden item. Brilliant!
> 
> Let's go with that, shall we?

Do you want this

(Code A)

diff --git a/mm/Kconfig b/mm/Kconfig
index 01b0ae0cd9d3..d80839d1fad8 100644
--- a/mm/Kconfig
+++ b/mm/Kconfig
@@ -853,6 +853,9 @@ config GUP_TEST

          See tools/testing/selftests/vm/gup_test.c

+comment "GUP_TEST needs to have DEBUG_FS enabled"
+       depends on !GUP_TEST && !DEBUG_FS
+
 config GUP_GET_PTE_LOW_HIGH
        bool

Or do you want this ?

(Code B)
diff --git a/mm/Kconfig b/mm/Kconfig
index 01b0ae0cd9d3..a7ff0d31afd5 100644
--- a/mm/Kconfig
+++ b/mm/Kconfig
@@ -836,6 +836,7 @@ config PERCPU_STATS

 config GUP_TEST
        bool "Enable infrastructure for get_user_pages()-related unit tests"
+       depends on DEBUG_FS
        help
          Provides /sys/kernel/debug/gup_test, which in turn provides a way
          to make ioctl calls that can launch kernel-based unit tests for
@@ -853,6 +854,9 @@ config GUP_TEST

          See tools/testing/selftests/vm/gup_test.c

+comment "GUP_TEST needs to have DEBUG_FS enabled"
+       depends on !GUP_TEST && !DEBUG_FS
+
 config GUP_GET_PTE_LOW_HIGH
        bool

To be honest, I am not a big fan of both of code A and B. I think "depends on" has
clearly said everything the redundant comment wants to say.

  │ Symbol: GUP_TEST [=]
  │ Type  : bool
  │ Defined at mm/Kconfig:837
  │   Prompt: Enable infrastructure for get_user_pages()-related unit tests
  │   Depends on: DEBUG_FS [=n]
  │   Location:
  │ (1) -> Memory Management option

Menuconfig shows GUP_TEST depends on DEBUG_FS and right now DEBUG_FS is
"n". so it is impossible to enable GUP_TEST.

"comment" is a good thing, but it is more likely to be used for a menu or a group
of configurations to extend a bundle of things.

On the other hand, If this particular case needs this comment, so do countless
other configurations in hundreds of Kconfig files.

My favorite order is
1. depends on
2. select
3. depends on + comment(code B)
4. comment only(code A)

I won't accept 4, but it seems we can't get agreement on 1 which is my favorite. 
So if you want either one of 2 and 3, I am happy to send v2 for that. So which one
is your favorite? 2 or 3?

> 
> thanks,
> --
> John Hubbard
> NVIDIA

Thanks
Barry