[PATCH] kfence: test: use kunit_skip() to skip tests

[PATCH] kfence: test: use kunit_skip() to skip tests

[Marco Elver]

Use the new kunit_skip() to skip tests if requirements were not met. It
makes it easier to see in KUnit's summary if there were skipped tests.

Signed-off-by: Marco Elver <elver@google.com>
---
 mm/kfence/kfence_test.c | 14 ++++++++------
 1 file changed, 8 insertions(+), 6 deletions(-)

diff --git a/mm/kfence/kfence_test.c b/mm/kfence/kfence_test.c
index f1690cf54199..695030c1fff8 100644
--- a/mm/kfence/kfence_test.c
+++ b/mm/kfence/kfence_test.c
@@ -32,6 +32,11 @@
 #define arch_kfence_test_address(addr) (addr)
 #endif
 
+#define KFENCE_TEST_REQUIRES(test, cond) do {			\
+	if (!(cond))						\
+		kunit_skip((test), "Test requires: " #cond);	\
+} while (0)
+
 /* Report as observed from console. */
 static struct {
 	spinlock_t lock;
@@ -555,8 +560,7 @@ static void test_init_on_free(struct kunit *test)
 	};
 	int i;
 
-	if (!IS_ENABLED(CONFIG_INIT_ON_FREE_DEFAULT_ON))
-		return;
+	KFENCE_TEST_REQUIRES(test, IS_ENABLED(CONFIG_INIT_ON_FREE_DEFAULT_ON));
 	/* Assume it hasn't been disabled on command line. */
 
 	setup_test_cache(test, size, 0, NULL);
@@ -603,10 +607,8 @@ static void test_gfpzero(struct kunit *test)
 	char *buf1, *buf2;
 	int i;
 
-	if (CONFIG_KFENCE_SAMPLE_INTERVAL > 100) {
-		kunit_warn(test, "skipping ... would take too long\n");
-		return;
-	}
+	/* Skip if we think it'd take too long. */
+	KFENCE_TEST_REQUIRES(test, CONFIG_KFENCE_SAMPLE_INTERVAL <= 100);
 
 	setup_test_cache(test, size, 0, NULL);
 	buf1 = test_alloc(test, size, GFP_KERNEL, ALLOCATE_ANY);
-- 
2.33.0.464.g1972c5931b-goog

Re: [PATCH] kfence: test: use kunit_skip() to skip tests

[David Gow]

On Thu, Sep 23, 2021 at 2:26 AM Marco Elver <elver@google.com> wrote:
>
> Use the new kunit_skip() to skip tests if requirements were not met. It
> makes it easier to see in KUnit's summary if there were skipped tests.
>
> Signed-off-by: Marco Elver <elver@google.com>
> ---

Thanks: I'm glad these features are proving useful. I've tested these
under qemu, and it works pretty well.

Certainly from the KUnit point of view, this is:
Reviewed-by: David Gow <davidgow@google.com>

(A couple of unrelated complaints about the kfence tests are that
TRACEPOINTS isn't selected by default, and that the manual
registering/unregistering of the tracepoints does break some of the
kunit tooling when several tests are built-in. That's something that
exists independently of this patch, though, and possibly requires some
KUnit changes to be fixed cleanly (kfence isn't the only thing to do
this). So not something to hold up this patch.)

Cheers,
-- David

>  mm/kfence/kfence_test.c | 14 ++++++++------
>  1 file changed, 8 insertions(+), 6 deletions(-)
>
> diff --git a/mm/kfence/kfence_test.c b/mm/kfence/kfence_test.c
> index f1690cf54199..695030c1fff8 100644
> --- a/mm/kfence/kfence_test.c
> +++ b/mm/kfence/kfence_test.c
> @@ -32,6 +32,11 @@
>  #define arch_kfence_test_address(addr) (addr)
>  #endif
>
> +#define KFENCE_TEST_REQUIRES(test, cond) do {                  \
> +       if (!(cond))                                            \
> +               kunit_skip((test), "Test requires: " #cond);    \
> +} while (0)
> +
>  /* Report as observed from console. */
>  static struct {
>         spinlock_t lock;
> @@ -555,8 +560,7 @@ static void test_init_on_free(struct kunit *test)
>         };
>         int i;
>
> -       if (!IS_ENABLED(CONFIG_INIT_ON_FREE_DEFAULT_ON))
> -               return;
> +       KFENCE_TEST_REQUIRES(test, IS_ENABLED(CONFIG_INIT_ON_FREE_DEFAULT_ON));
>         /* Assume it hasn't been disabled on command line. */
>
>         setup_test_cache(test, size, 0, NULL);
> @@ -603,10 +607,8 @@ static void test_gfpzero(struct kunit *test)
>         char *buf1, *buf2;
>         int i;
>
> -       if (CONFIG_KFENCE_SAMPLE_INTERVAL > 100) {
> -               kunit_warn(test, "skipping ... would take too long\n");
> -               return;
> -       }
> +       /* Skip if we think it'd take too long. */
> +       KFENCE_TEST_REQUIRES(test, CONFIG_KFENCE_SAMPLE_INTERVAL <= 100);
>
>         setup_test_cache(test, size, 0, NULL);
>         buf1 = test_alloc(test, size, GFP_KERNEL, ALLOCATE_ANY);
> --
> 2.33.0.464.g1972c5931b-goog
>

Re: [PATCH] kfence: test: use kunit_skip() to skip tests

[Marco Elver]

On Thu, 23 Sept 2021 at 19:39, David Gow <davidgow@google.com> wrote:
> On Thu, Sep 23, 2021 at 2:26 AM Marco Elver <elver@google.com> wrote:
> >
> > Use the new kunit_skip() to skip tests if requirements were not met. It
> > makes it easier to see in KUnit's summary if there were skipped tests.
> >
> > Signed-off-by: Marco Elver <elver@google.com>
> > ---
>
> Thanks: I'm glad these features are proving useful. I've tested these
> under qemu, and it works pretty well.
>
> Certainly from the KUnit point of view, this is:
> Reviewed-by: David Gow <davidgow@google.com>

Thanks!

> (A couple of unrelated complaints about the kfence tests are that
> TRACEPOINTS isn't selected by default, and that the manual
> registering/unregistering of the tracepoints does break some of the
> kunit tooling when several tests are built-in. That's something that
> exists independently of this patch, though, and possibly requires some
> KUnit changes to be fixed cleanly (kfence isn't the only thing to do
> this). So not something to hold up this patch.)

I think there was a reason we wanted it to "depends on TRACEPOINTS".
If it were to select it, then if you do a CONFIG_KUNIT_ALL_TESTS=y,
and also have KFENCE on, you'll always select tracepoints. In certain
situations this may not be wanted. If we didn't have
CONFIG_KUNIT_ALL_TESTS, then certainly, auto-selecting TRACEPOINTS
would be ok.

If you can live with that, we can of course switch it to do "select
TRACEPOINTS".

On a whole I err on the side of fewer auto-selected Kconfig options.

Thanks,
-- Marco

Re: [PATCH] kfence: test: use kunit_skip() to skip tests

[David Gow]

On Fri, Sep 24, 2021 at 1:58 AM Marco Elver <elver@google.com> wrote:
>
> On Thu, 23 Sept 2021 at 19:39, David Gow <davidgow@google.com> wrote:
> > On Thu, Sep 23, 2021 at 2:26 AM Marco Elver <elver@google.com> wrote:
> > >
> > > Use the new kunit_skip() to skip tests if requirements were not met. It
> > > makes it easier to see in KUnit's summary if there were skipped tests.
> > >
> > > Signed-off-by: Marco Elver <elver@google.com>
> > > ---
> >
> > Thanks: I'm glad these features are proving useful. I've tested these
> > under qemu, and it works pretty well.
> >
> > Certainly from the KUnit point of view, this is:
> > Reviewed-by: David Gow <davidgow@google.com>
>
> Thanks!
>
> > (A couple of unrelated complaints about the kfence tests are that
> > TRACEPOINTS isn't selected by default, and that the manual
> > registering/unregistering of the tracepoints does break some of the
> > kunit tooling when several tests are built-in. That's something that
> > exists independently of this patch, though, and possibly requires some
> > KUnit changes to be fixed cleanly (kfence isn't the only thing to do
> > this). So not something to hold up this patch.)
>
> I think there was a reason we wanted it to "depends on TRACEPOINTS".
> If it were to select it, then if you do a CONFIG_KUNIT_ALL_TESTS=y,
> and also have KFENCE on, you'll always select tracepoints. In certain
> situations this may not be wanted. If we didn't have
> CONFIG_KUNIT_ALL_TESTS, then certainly, auto-selecting TRACEPOINTS
> would be ok.
>
> If you can live with that, we can of course switch it to do "select
> TRACEPOINTS".

That's probably more convenient for me, but I confess that my use case
is almost always wanting to run the KUnit tests, so I'm not unbiased.
:-)

>
> On a whole I err on the side of fewer auto-selected Kconfig options.

Yeah, it's perfectly sensible to do it either way. Maybe the right
option is to have a .kunitconfig file which has TRACEPOINTS enabled.

It's probably not worth doing if there's still issues with kunit_tool
parsing the results when the test is built-in, so this should probably
wait until KUnit has a way of running code on init/exit of suites as
well as individual tests within those suites. KFENCE is not the only
test suite which needs something like that (nor the only one which
does some module_init or late_initcall stuff which causes some
formatting issues with builtin tests).

Cheers,
-- David