[igt-dev] [PATCH i-g-t 10/10] KUnit: replace abort with graceful skip
Janusz Krzysztofik
janusz.krzysztofik at linux.intel.com
Tue Jun 13 11:15:50 UTC 2023
On Tuesday, 13 June 2023 11:58:47 CEST Mauro Carvalho Chehab wrote:
> On Tue, 13 Jun 2023 11:11:00 +0200
> Janusz Krzysztofik <janusz.krzysztofik at linux.intel.com> wrote:
>
> > On Tuesday, 13 June 2023 10:41:03 CEST Mauro Carvalho Chehab wrote:
> > > On Tue, 13 Jun 2023 09:27:26 +0200
> > > Dominik Karol Piatkowski <dominik.karol.piatkowski at intel.com> wrote:
> > >
> > > > Sample drm_buddy output with missing requirements:
> > > > Starting subtest: drm_buddy_test
> > > > (drm_buddy:32218) igt_kmod-WARNING: Unable to load KUnit
> > > > Subtest drm_buddy_test: SKIP (0.001s)
> > > >
> > > > Signed-off-by: Dominik Karol Piątkowski
> > <dominik.karol.piatkowski at intel.com>
> > > > Cc: Janusz Krzysztofik <janusz.krzysztofik at linux.intel.com>
> > > > Cc: Mauro Carvalho Chehab <mauro.chehab at linux.intel.com>
> > > > ---
> > > > lib/igt_kmod.c | 20 +++++++++++++-------
> > > > 1 file changed, 13 insertions(+), 7 deletions(-)
> > > >
> > > > diff --git a/lib/igt_kmod.c b/lib/igt_kmod.c
> > > > index 1511bdef4..04220f404 100644
> > > > --- a/lib/igt_kmod.c
> > > > +++ b/lib/igt_kmod.c
> > > > @@ -763,27 +763,27 @@ static void __igt_kunit(const char *module_name,
> > const char *opts)
> > > > int ret;
> > > > struct ktap_test_results *results;
> > > > struct ktap_test_results_element *temp;
> > > > + bool skip = false;
> > > >
> > > > /* get normalized module name */
> > > > if (igt_ktest_init(&tst, module_name) != 0) {
> > > > - igt_warn("Unable to initialize ktest for %s\n",
> > module_name);
> > > > - igt_fail(IGT_EXIT_SKIP);
> > > > + igt_skip("Unable to initialize ktest for %s\n",
> > module_name);
> > > > }
> > > >
> > > > if (igt_ktest_begin(&tst) != 0) {
> > > > - igt_warn("Unable to begin ktest for %s\n", module_name);
> > > > -
> > > > igt_ktest_fini(&tst);
> > > > - igt_fail(IGT_EXIT_SKIP);
> > > > + igt_skip("Unable to begin ktest for %s\n", module_name);
> > > > }
> > > >
> > > > if (tst.kmsg < 0) {
> > > > igt_warn("Could not open /dev/kmsg\n");
> > > > + skip = true;
> > >
> > > Hmm.... not being able to read from /dev/kmsg doesn't seem a reason for
> > > skipping the test.
> > >
> > > IMO, the test should be skipped only if:
> > >
> > > 1. The Kernel is not compiled with KUnit support;
> > > 2. The KUnit test was not compiled.
> > >
> > > Errors for all other conditions should be considered as failures.
> > >
> > > Now, I'm not sure what would be the proper way to test for such
> > > skip condition. I suspect you could check the error code at
> > > igt_kmod_load(), calling igt_skip() if it returns -ENOENT,
> > > e. g. on this part of __igt_kunit():
> > >
> > > /* The KUnit module is required for running any KUnit tests */
> > > if (igt_kmod_load("kunit", NULL) != 0 ||
> > > kmod_module_new_from_name(kmod_ctx(), "kunit", &kunit_kmod) !=
> > 0) {
> > > igt_warn("Unable to load KUnit\n");
> > > igt_fail(IGT_EXIT_FAILURE);
> > > }
> > >
> > > is_builtin = kmod_module_get_initstate(kunit_kmod) ==
> > KMOD_MODULE_BUILTIN;
> > >
> > > results = ktap_parser_start(f, is_builtin);
> > >
> > > if (igt_kmod_load(module_name, opts) != 0) {
> > > igt_warn("Unable to load %s module\n", module_name);
> > > ret = ktap_parser_stop();
> > > igt_fail(IGT_EXIT_FAILURE);
> > > }
> > >
> > >
> > > This could be replaced by something similar to:
> > >
> > > err = igt_kmod_load("kunit", NULL);
> > > igt_skip_on(err == ENOENT, "KUnit not supported");
> > > igt_fail_on_f(err != 0, "Error %d when trying to load kunit", err);
> > > if (kmod_module_new_from_name(kmod_ctx(), "kunit", &kunit_kmod) !=
> > 0) {
> > > igt_warn("Unable to load KUnit\n");
> > > igt_fail(IGT_EXIT_FAILURE);
> > > }
> > >
> > >
> > > is_builtin = kmod_module_get_initstate(kunit_kmod) ==
> > KMOD_MODULE_BUILTIN;
> > >
> > > results = ktap_parser_start(f, is_builtin);
> > >
> > > err = igt_kmod_load(module_name, opts)
> > > igt_skip_on(err == ENOENT, "Module %s not found", module_name);
> > > if (err) {
> > > igt_warn("Error %d when trying to load %s module\n", err,
> > module_name);
> > > ret = ktap_parser_stop();
> > > igt_fail(IGT_EXIT_FAILURE);
> > > }
> > >
> > >
> > > In order to explicitly check for (1) and (2).
> >
> > Why do we reinvent the wheel instead of following the pattern from
> > igt_kselftest()?
>
> Nobody is talking about reinventing the wheel. Selftests and KUnit tests
> are executed on different ways.
>
> For selftests, the same driver that was loaded before needs to be unloaded
> and re-probed with a different argument.
>
> For KUnit tests, a different module will be loaded, if the Kernel was
> compiled with KUnit support for a given test suite.
>
> Looking at KUnit modprobe time, if a module doesn't load because the file
> doesn't exist (-ENOENT error), it means that:
>
> - for KUnit: the KUnit module was not compiled.
> - for selftests: this is really unlikely to happen, as all other
> previous IGT tests depend on having the driver module loaded.
> So, no need for any explicit check for this specific condition.
>
> > We used to follow it, before the whole igt_kunit() body was
> > put under igt_subtest_with_dynamic() (patch 6/8 of v5, then squashed with 5/5,
> > IIRC). That forced us to introduce a series of changes inside igt_kunit(),
> > which we now contest. Was that really necessary?
>
> This has nothing to do with using (or not) a dynamic IGT subtest.
It has. As soon as you enter an igt_subtest or igt_subtest_with_dynamic
section, your inform users (CI) that the subtest execution started. And, you
can't call return, only igt_success(), or igt_skip()/igt_fail() and friends,
form inside, while you can before.
The DRM selftests we are going to update are now using igt_kselftest(), which
calls return on some initial conditions not met, before the
igt_subtest_with_dynamic section is entered. CI expects that behavior, users
are used to it. We shouldn't change that only because we switch from an old
underlying kernel selftest format to a new shiny one, I believe, unless we are
able to prove that there was something wrong with that former approach, or we
can explain why it no longer fits into the kunit variant.
>
> It is related to how to report IGT errors while trying to run KUnit
> tests.
>
> Without this patch, every kind of errors will produce a failure;
Yes, that was wrong, and thank you Dominik for fixing this. But plaese also
note that the badness was mostly caused by ad-hoc corrections to igt_kunit()
body, forced by putting it as a whole insinde igt_subtest_with_dynamic()
section while it was not ready for that. IOW, my objections don't apply to
this patch specifically, but rather the the whole series that started from
igt_kunit() trying to provide the same used experience as igt_ksleftest(),
then that approach being destroyed with patches subsequently added to the
series.
> after this patch, they'll all produce a skip.
>
> IMO, what it is needed is to split KUnit errors on two different
> categories:
>
> 1. "real" errors while running KUnit (like troubles reading dmesg);
I agree that troubles reading dmesg are critical to kunit variants, while they
were negligible for old selftests, hence their handling specifically must now
be different, but for me such a difference is not a sufficient justification
for killing the whole igt_ksleftest() error handling pattern and inventing a
new one from scratch.
Thanks,
Janusz
> 2. the KUnit module to be modprobed doesn't exist.
>
> for (1), igt_fail() is the proper error handling.
>
> For (2), igt_skip() is the right error handling, as the Kernel under
> tests doesn't have what it is needed to run KUnit, as the module was
> not compiled.
>
> Regards,
> Mauro
>
>
>
More information about the igt-dev
mailing list