[PATCH -next] lib: TEST_REF_TRACKER should depend on REF_TRACKER instead of selecting it

TEST_REF_TRACKER selects REF_TRACKER, thus enabling an optional feature
the user may not want to have enabled.  Fix this by making the test
depend on REF_TRACKER instead.

Fixes: 914a7b5000d08f14 ("lib: add tests for reference tracker")
Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
---
 lib/Kconfig.debug | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug
index c77fe36bb3d89685..d5e4afee09d78a1e 100644
--- a/lib/Kconfig.debug
+++ b/lib/Kconfig.debug

@@ -2114,8 +2114,7 @@ config BACKTRACE_SELF_TEST
 
 config TEST_REF_TRACKER
 	tristate "Self test for reference tracker"
-	depends on DEBUG_KERNEL && STACKTRACE_SUPPORT
-	select REF_TRACKER
+	depends on DEBUG_KERNEL && STACKTRACE_SUPPORT && REF_TRACKER
 	help
 	  This option provides a kernel module performing tests
 	  using reference tracker infrastructure.

-- 
2.25.1

On Wed, Dec 15, 2021 at 1:36 AM Geert Uytterhoeven
[off-list ref] wrote:

TEST_REF_TRACKER selects REF_TRACKER, thus enabling an optional feature
the user may not want to have enabled.  Fix this by making the test
depend on REF_TRACKER instead.

I do not understand this.

How can I test this infra alone, without any ref_tracker being selected ?

I have in my configs

CONFIG_TEST_REF_TRACKER=m
# CONFIG_NET_DEV_REFCNT_TRACKER is not set
# CONFIG_NET_NS_REFCNT_TRACKER is not set

This should work.

I would not have sent patches built around ref_tracker if I had no
ways of testing the base infrastructure.

Hi Eric,

On Wed, Dec 15, 2021 at 10:51 AM Eric Dumazet [off-list ref] wrote:

On Wed, Dec 15, 2021 at 1:36 AM Geert Uytterhoeven
[off-list ref] wrote:

quoted

TEST_REF_TRACKER selects REF_TRACKER, thus enabling an optional feature
the user may not want to have enabled.  Fix this by making the test
depend on REF_TRACKER instead.

I do not understand this.

The issue is that merely enabling tests should not enable optional
features, to prevent unwanted features sneaking into a product.
If tests depend on features, all tests for features that are enabled can
still be enabled (e.g. made modular, so they can be loaded when needed).

How can I test this infra alone, without any ref_tracker being selected ?

I have in my configs

CONFIG_TEST_REF_TRACKER=m
# CONFIG_NET_DEV_REFCNT_TRACKER is not set
# CONFIG_NET_NS_REFCNT_TRACKER is not set

This should work.

So you want to test the reference tracker, without having any actual
users of the reference tracker enabled?

Perhaps REF_TRACKER should become visible, cfr. CRC32 and
the related CRC32_SELFTEST?

I would not have sent patches built around ref_tracker if I had no
ways of testing the base infrastructure.

quoted

--- a/lib/Kconfig.debug
+++ b/lib/Kconfig.debug

@@ -2114,8 +2114,7 @@ config BACKTRACE_SELF_TEST

 config TEST_REF_TRACKER
        tristate "Self test for reference tracker"
-       depends on DEBUG_KERNEL && STACKTRACE_SUPPORT
-       select REF_TRACKER
+       depends on DEBUG_KERNEL && STACKTRACE_SUPPORT && REF_TRACKER
        help
          This option provides a kernel module performing tests
          using reference tracker infrastructure.

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

On Wed, Dec 15, 2021 at 2:10 AM Geert Uytterhoeven [off-list ref] wrote:

Hi Eric,

On Wed, Dec 15, 2021 at 10:51 AM Eric Dumazet [off-list ref] wrote:

quoted

On Wed, Dec 15, 2021 at 1:36 AM Geert Uytterhoeven
[off-list ref] wrote:

quoted

TEST_REF_TRACKER selects REF_TRACKER, thus enabling an optional feature
the user may not want to have enabled.  Fix this by making the test
depend on REF_TRACKER instead.

I do not understand this.

The issue is that merely enabling tests should not enable optional
features, to prevent unwanted features sneaking into a product.

if you do not want the feature, just say no ?

# CONFIG_TEST_REF_TRACKER is not set
# CONFIG_NET_DEV_REFCNT_TRACKER is not set
# CONFIG_NET_NS_REFCNT_TRACKER is not set

If tests depend on features, all tests for features that are enabled can
still be enabled (e.g. made modular, so they can be loaded when needed).

quoted

How can I test this infra alone, without any ref_tracker being selected ?

I have in my configs

CONFIG_TEST_REF_TRACKER=m
# CONFIG_NET_DEV_REFCNT_TRACKER is not set
# CONFIG_NET_NS_REFCNT_TRACKER is not set

This should work.

So you want to test the reference tracker, without having any actual
users of the reference tracker enabled?

Yes, I fail to see the problem you have with this.

lib/ref_tracker.c is not adding intrusive features like KASAN.

Perhaps REF_TRACKER should become visible, cfr. CRC32 and
the related CRC32_SELFTEST?

I can not speak for CRC32.

My point is that I sent a series, I wanted this series to be bisectable.

When the test was added, I wanted to be able to use it right away.
(like compile it, and run it)

quoted

I would not have sent patches built around ref_tracker if I had no
ways of testing the base infrastructure.

quoted

quoted

--- a/lib/Kconfig.debug
+++ b/lib/Kconfig.debug

@@ -2114,8 +2114,7 @@ config BACKTRACE_SELF_TEST

 config TEST_REF_TRACKER
        tristate "Self test for reference tracker"
-       depends on DEBUG_KERNEL && STACKTRACE_SUPPORT
-       select REF_TRACKER
+       depends on DEBUG_KERNEL && STACKTRACE_SUPPORT && REF_TRACKER
        help
          This option provides a kernel module performing tests
          using reference tracker infrastructure.

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Hi Eric,

On Wed, Dec 15, 2021 at 11:24 AM Eric Dumazet [off-list ref] wrote:

On Wed, Dec 15, 2021 at 2:10 AM Geert Uytterhoeven [off-list ref] wrote:

quoted

On Wed, Dec 15, 2021 at 10:51 AM Eric Dumazet [off-list ref] wrote:

quoted

On Wed, Dec 15, 2021 at 1:36 AM Geert Uytterhoeven
[off-list ref] wrote:

quoted

TEST_REF_TRACKER selects REF_TRACKER, thus enabling an optional feature
the user may not want to have enabled.  Fix this by making the test
depend on REF_TRACKER instead.

I do not understand this.

The issue is that merely enabling tests should not enable optional
features, to prevent unwanted features sneaking into a product.

if you do not want the feature, just say no ?

# CONFIG_TEST_REF_TRACKER is not set
# CONFIG_NET_DEV_REFCNT_TRACKER is not set
# CONFIG_NET_NS_REFCNT_TRACKER is not set

quoted

If tests depend on features, all tests for features that are enabled can
still be enabled (e.g. made modular, so they can be loaded when needed).

quoted

How can I test this infra alone, without any ref_tracker being selected ?

I have in my configs

CONFIG_TEST_REF_TRACKER=m
# CONFIG_NET_DEV_REFCNT_TRACKER is not set
# CONFIG_NET_NS_REFCNT_TRACKER is not set

This should work.

So you want to test the reference tracker, without having any actual
users of the reference tracker enabled?

Yes, I fail to see the problem you have with this.

lib/ref_tracker.c is not adding intrusive features like KASAN.

How can I be sure of that? ;-)

quoted

Perhaps REF_TRACKER should become visible, cfr. CRC32 and
the related CRC32_SELFTEST?

I can not speak for CRC32.

My point is that I sent a series, I wanted this series to be bisectable.

When the test was added, I wanted to be able to use it right away.
(like compile it, and run it)

Then you indeed need a way to force-enable the feature. For other
library-like features, that is done by making the feature visible,
cfr. CRC32.

My point is that a user should be able to easily enable all available
tests for all features he has wilfully enabled in his kernel config,
without running into the risk of accidentally enabling more features.
Hence a test should depend on the feature under test, not blindly
enable the feature.

An example of this is commit 302fdadeafe4be53 ("ext: EXT4_KUNIT_TESTS
should depend on EXT4_FS instead of selecting it"). Before that commit,
enabling KUNIT_ALL_TESTS=m enabled EXT4_FS, even on diskless system.

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

On Wed, Dec 15, 2021 at 2:41 AM Geert Uytterhoeven [off-list ref] wrote:

Hi Eric,

On Wed, Dec 15, 2021 at 11:24 AM Eric Dumazet [off-list ref] wrote:

quoted

On Wed, Dec 15, 2021 at 2:10 AM Geert Uytterhoeven [off-list ref] wrote:

quoted

On Wed, Dec 15, 2021 at 10:51 AM Eric Dumazet [off-list ref] wrote:

quoted

On Wed, Dec 15, 2021 at 1:36 AM Geert Uytterhoeven
[off-list ref] wrote:

quoted

TEST_REF_TRACKER selects REF_TRACKER, thus enabling an optional feature
the user may not want to have enabled.  Fix this by making the test
depend on REF_TRACKER instead.

I do not understand this.

The issue is that merely enabling tests should not enable optional
features, to prevent unwanted features sneaking into a product.

if you do not want the feature, just say no ?

# CONFIG_TEST_REF_TRACKER is not set
# CONFIG_NET_DEV_REFCNT_TRACKER is not set
# CONFIG_NET_NS_REFCNT_TRACKER is not set

quoted

If tests depend on features, all tests for features that are enabled can
still be enabled (e.g. made modular, so they can be loaded when needed).

quoted

How can I test this infra alone, without any ref_tracker being selected ?

I have in my configs

CONFIG_TEST_REF_TRACKER=m
# CONFIG_NET_DEV_REFCNT_TRACKER is not set
# CONFIG_NET_NS_REFCNT_TRACKER is not set

This should work.

So you want to test the reference tracker, without having any actual
users of the reference tracker enabled?

Yes, I fail to see the problem you have with this.

lib/ref_tracker.c is not adding intrusive features like KASAN.

How can I be sure of that? ;-)

quoted

quoted

Perhaps REF_TRACKER should become visible, cfr. CRC32 and
the related CRC32_SELFTEST?

I can not speak for CRC32.

My point is that I sent a series, I wanted this series to be bisectable.

When the test was added, I wanted to be able to use it right away.
(like compile it, and run it)

Then you indeed need a way to force-enable the feature. For other
library-like features, that is done by making the feature visible,
cfr. CRC32.

My point is that a user should be able to easily enable all available
tests for all features he has wilfully enabled in his kernel config,
without running into the risk of accidentally enabling more features.
Hence a test should depend on the feature under test, not blindly
enable the feature.

So you say that STACKDEPOT should be user selectable,
even if no layer is using it ?

I based my work on STACKDEPOT, not on EXT4

On Wed, Dec 15, 2021 at 2:55 AM Eric Dumazet [off-list ref] wrote:

So you say that STACKDEPOT should be user selectable,
even if no layer is using it ?

I based my work on STACKDEPOT, not on EXT4

In any case, the patch you sent prevents me from testing the module alone.

So whatever you had in mind, you will have to send another patch.