| Message ID | 0b6c06487234b0fb52b7a2fbd2237af42f9d11a6.1639560869.git.geert+renesas@glider.be (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [-next] lib: TEST_REF_TRACKER should depend on REF_TRACKER instead of selecting it | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Not a local patch |
On Wed, Dec 15, 2021 at 1:36 AM Geert Uytterhoeven <geert+renesas@glider.be> 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. > > 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 >
Hi Eric, On Wed, Dec 15, 2021 at 10:51 AM Eric Dumazet <edumazet@google.com> wrote: > On Wed, Dec 15, 2021 at 1:36 AM Geert Uytterhoeven > <geert+renesas@glider.be> 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. 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. > > --- 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 <geert@linux-m68k.org> wrote: > > Hi Eric, > > On Wed, Dec 15, 2021 at 10:51 AM Eric Dumazet <edumazet@google.com> wrote: > > On Wed, Dec 15, 2021 at 1:36 AM Geert Uytterhoeven > > <geert+renesas@glider.be> 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. > > 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). > > > 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) > > > I would not have sent patches built around ref_tracker if I had no > > ways of testing the base infrastructure. > > > > --- 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 <edumazet@google.com> wrote: > On Wed, Dec 15, 2021 at 2:10 AM Geert Uytterhoeven <geert@linux-m68k.org> wrote: > > On Wed, Dec 15, 2021 at 10:51 AM Eric Dumazet <edumazet@google.com> wrote: > > > On Wed, Dec 15, 2021 at 1:36 AM Geert Uytterhoeven > > > <geert+renesas@glider.be> 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. > > > > 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). > > > > > 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? ;-) > > 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 <geert@linux-m68k.org> wrote: > > Hi Eric, > > On Wed, Dec 15, 2021 at 11:24 AM Eric Dumazet <edumazet@google.com> wrote: > > On Wed, Dec 15, 2021 at 2:10 AM Geert Uytterhoeven <geert@linux-m68k.org> wrote: > > > On Wed, Dec 15, 2021 at 10:51 AM Eric Dumazet <edumazet@google.com> wrote: > > > > On Wed, Dec 15, 2021 at 1:36 AM Geert Uytterhoeven > > > > <geert+renesas@glider.be> 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. > > > > > > 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). > > > > > > > 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? ;-) > > > > 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 <edumazet@google.com> 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.
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.
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(-)