Running the test currently fails on non-SMP systems, despite being
enabled by default. This means that running the test with:
./tools/testing/kunit/kunit.py run --arch x86_64 hw_breakpoint
results in every hw_breakpoint test failing with:
# test_one_cpu: failed to initialize: -22
not ok 1 - test_one_cpu
Instead, use kunit_skip(), which will mark the test as skipped, and give
a more comprehensible message:
ok 1 - test_one_cpu # SKIP not enough cpus
This makes it more obvious that the test is not suited to the test
environment, and so wasn't run, rather than having run and failed.
Signed-off-by: David Gow <[email protected]>
---
kernel/events/hw_breakpoint_test.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/kernel/events/hw_breakpoint_test.c b/kernel/events/hw_breakpoint_test.c
index 5ced822df788..c57610f52bb4 100644
--- a/kernel/events/hw_breakpoint_test.c
+++ b/kernel/events/hw_breakpoint_test.c
@@ -295,11 +295,11 @@ static int test_init(struct kunit *test)
{
/* Most test cases want 2 distinct CPUs. */
if (num_online_cpus() < 2)
- return -EINVAL;
+ kunit_skip(test, "not enough cpus");
/* Want the system to not use breakpoints elsewhere. */
if (hw_breakpoint_is_used())
- return -EBUSY;
+ kunit_skip(test, "hw breakpoint already in use");
return 0;
}
--
2.38.0.135.g90850a2211-goog
On Wed, 26 Oct 2022 at 07:10, David Gow <[email protected]> wrote:
>
> Running the test currently fails on non-SMP systems, despite being
> enabled by default. This means that running the test with:
>
> ./tools/testing/kunit/kunit.py run --arch x86_64 hw_breakpoint
>
> results in every hw_breakpoint test failing with:
>
> # test_one_cpu: failed to initialize: -22
> not ok 1 - test_one_cpu
>
> Instead, use kunit_skip(), which will mark the test as skipped, and give
> a more comprehensible message:
>
> ok 1 - test_one_cpu # SKIP not enough cpus
>
> This makes it more obvious that the test is not suited to the test
> environment, and so wasn't run, rather than having run and failed.
>
> Signed-off-by: David Gow <[email protected]>
Acked-by: Marco Elver <[email protected]>
Although I still get confused by the fact that skipped tests say "ok"
and then need to double check the log that tests weren't skipped.
> ---
> kernel/events/hw_breakpoint_test.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/kernel/events/hw_breakpoint_test.c b/kernel/events/hw_breakpoint_test.c
> index 5ced822df788..c57610f52bb4 100644
> --- a/kernel/events/hw_breakpoint_test.c
> +++ b/kernel/events/hw_breakpoint_test.c
> @@ -295,11 +295,11 @@ static int test_init(struct kunit *test)
> {
> /* Most test cases want 2 distinct CPUs. */
> if (num_online_cpus() < 2)
> - return -EINVAL;
> + kunit_skip(test, "not enough cpus");
>
> /* Want the system to not use breakpoints elsewhere. */
> if (hw_breakpoint_is_used())
> - return -EBUSY;
> + kunit_skip(test, "hw breakpoint already in use");
>
> return 0;
> }
> --
> 2.38.0.135.g90850a2211-goog
>
On Wed, Oct 26, 2022 at 7:10 AM David Gow <[email protected]> wrote:
>
> Running the test currently fails on non-SMP systems, despite being
> enabled by default. This means that running the test with:
>
> ./tools/testing/kunit/kunit.py run --arch x86_64 hw_breakpoint
>
> results in every hw_breakpoint test failing with:
>
> # test_one_cpu: failed to initialize: -22
> not ok 1 - test_one_cpu
>
> Instead, use kunit_skip(), which will mark the test as skipped, and give
> a more comprehensible message:
>
> ok 1 - test_one_cpu # SKIP not enough cpus
>
> This makes it more obvious that the test is not suited to the test
> environment, and so wasn't run, rather than having run and failed.
>
> Signed-off-by: David Gow <[email protected]>
Reviewed-by: Daniel Latypov <[email protected]>
This patch makes this command pass for me.
$ ./tools/testing/kunit/kunit.py run --arch x86_64
Since this test gets picked up by default, having it pass for common
uses of kunit.py is a priority, IMO.
(Note: if I add --alltests as well, these were the only failures)
I agree with Marco that TAP/KTAP saying "ok" for skipped tests can be
confusing at first.
But a SKIP status feels more appropriate than FAIL, so I'd strongly
like for this change to go in.
> ---
> kernel/events/hw_breakpoint_test.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/kernel/events/hw_breakpoint_test.c b/kernel/events/hw_breakpoint_test.c
> index 5ced822df788..c57610f52bb4 100644
> --- a/kernel/events/hw_breakpoint_test.c
> +++ b/kernel/events/hw_breakpoint_test.c
> @@ -295,11 +295,11 @@ static int test_init(struct kunit *test)
> {
> /* Most test cases want 2 distinct CPUs. */
> if (num_online_cpus() < 2)
> - return -EINVAL;
> + kunit_skip(test, "not enough cpus");
The only minor nit I have is that I'd personally prefer something like
kunit_skip(test, "need >=2 cpus");
since that makes it clearer
a) that we must only have 1 CPU by default
b) roughly how one might address this.
Note: b) is a bit more complicated than I would like. The final
command is something like
$ ./tools/testing/kunit/kunit.py run --arch x86_64 --qemu_args='-smp
2' --kconfig_add='CONFIG_SMP=y'
But that's orthogonal to this patch.
Hi David, Daniel,
On Wed, 26 Oct 2022 at 20:31, Daniel Latypov <[email protected]> wrote:
[...]
> > - return -EINVAL;
> > + kunit_skip(test, "not enough cpus");
>
> The only minor nit I have is that I'd personally prefer something like
> kunit_skip(test, "need >=2 cpus");
> since that makes it clearer
> a) that we must only have 1 CPU by default
> b) roughly how one might address this.
>
> Note: b) is a bit more complicated than I would like. The final
> command is something like
> $ ./tools/testing/kunit/kunit.py run --arch x86_64 --qemu_args='-smp
> 2' --kconfig_add='CONFIG_SMP=y'
>
> But that's orthogonal to this patch.
Was there going to be a v2 to address (a), or is this patch ready to
be picked up?
I assume (unless I hear otherwise), this patch shall also go through -tip?
Thanks,
-- Marco
On Wed, Nov 02, 2022 at 11:22:43AM +0100, Marco Elver wrote:
> Was there going to be a v2 to address (a), or is this patch ready to
> be picked up?
>
> I assume (unless I hear otherwise), this patch shall also go through -tip?
Yes, I've got it queued, I just haven't gotten around to pushing it out
to -tip, hopefully later today.
On Wed, Nov 2, 2022 at 3:23 AM Marco Elver <[email protected]> wrote:
>
> Hi David, Daniel,
>
> On Wed, 26 Oct 2022 at 20:31, Daniel Latypov <[email protected]> wrote:
> [...]
> > > - return -EINVAL;
> > > + kunit_skip(test, "not enough cpus");
> >
> > The only minor nit I have is that I'd personally prefer something like
> > kunit_skip(test, "need >=2 cpus");
> > since that makes it clearer
> > a) that we must only have 1 CPU by default
> > b) roughly how one might address this.
> >
> > Note: b) is a bit more complicated than I would like. The final
> > command is something like
> > $ ./tools/testing/kunit/kunit.py run --arch x86_64 --qemu_args='-smp
> > 2' --kconfig_add='CONFIG_SMP=y'
> >
> > But that's orthogonal to this patch.
>
> Was there going to be a v2 to address (a), or is this patch ready to
> be picked up?
>
> I assume (unless I hear otherwise), this patch shall also go through -tip?
Just noting for the record:
I'm totally fine with this version going in, esp. if Peter is already
planning on picking it up.
This patch makes it so `kunit.py run --arch=x86_64` doesn't have test
failures, so I don't want it delayed due to just my small nit.
Daniel
The following commit has been merged into the perf/urgent branch of tip:
Commit-ID: 4b18cb3f74dcfc183c2434e17bfce09ce6302e37
Gitweb: https://git.kernel.org/tip/4b18cb3f74dcfc183c2434e17bfce09ce6302e37
Author: David Gow <[email protected]>
AuthorDate: Wed, 26 Oct 2022 22:10:40 +08:00
Committer: Peter Zijlstra <[email protected]>
CommitterDate: Wed, 02 Nov 2022 12:22:05 +01:00
perf/hw_breakpoint: test: Skip the test if dependencies unmet
Running the test currently fails on non-SMP systems, despite being
enabled by default. This means that running the test with:
./tools/testing/kunit/kunit.py run --arch x86_64 hw_breakpoint
results in every hw_breakpoint test failing with:
# test_one_cpu: failed to initialize: -22
not ok 1 - test_one_cpu
Instead, use kunit_skip(), which will mark the test as skipped, and give
a more comprehensible message:
ok 1 - test_one_cpu # SKIP not enough cpus
This makes it more obvious that the test is not suited to the test
environment, and so wasn't run, rather than having run and failed.
Signed-off-by: David Gow <[email protected]>
Signed-off-by: Peter Zijlstra (Intel) <[email protected]>
Reviewed-by: Daniel Latypov <[email protected]>
Acked-by: Marco Elver <[email protected]>
Link: https://lore.kernel.org/r/[email protected]
---
kernel/events/hw_breakpoint_test.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/kernel/events/hw_breakpoint_test.c b/kernel/events/hw_breakpoint_test.c
index 5ced822..c57610f 100644
--- a/kernel/events/hw_breakpoint_test.c
+++ b/kernel/events/hw_breakpoint_test.c
@@ -295,11 +295,11 @@ static int test_init(struct kunit *test)
{
/* Most test cases want 2 distinct CPUs. */
if (num_online_cpus() < 2)
- return -EINVAL;
+ kunit_skip(test, "not enough cpus");
/* Want the system to not use breakpoints elsewhere. */
if (hw_breakpoint_is_used())
- return -EBUSY;
+ kunit_skip(test, "hw breakpoint already in use");
return 0;
}