[igt-dev] [PATCH i-g-t 10/10] KUnit: replace abort with graceful skip
Mauro Carvalho Chehab
mauro.chehab at linux.intel.com
Tue Jun 13 13:34:00 UTC 2023
On Tue, 13 Jun 2023 13:15:50 +0200
Janusz Krzysztofik <janusz.krzysztofik at linux.intel.com> wrote:
> 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.
Well, igt selftest excecution also calls igt_subtest_with_dynamic().
However, on kunit, the tests start running during modprobe time, so
igt_subtest_with_dynamic() needs to be called before that. Maybe some
validations could happen earlier, but the most important one (checking
that the module exists) happens when the module is probed, which is the
same call that starts the test.
>
> 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.
No. Since 2022 - specifically, since those commits:
- 932da861956a ("drm: selftest: convert drm_buddy selftest to KUnit")
- fc8d29e298cf ("drm: selftest: convert drm_mm selftest to KUnit")
those tests don't work anymore, as upstream removed support for selftest,
in favor of KUnit. So, those tests currently do nothing.
We are changing because those tests are currently broken on IGT. That
is the reason why we need KUnit support for such tests.
> 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.
Yes, there's something wrong with the former approach: the tests are
currently broken, as upstream moved on.
>
> >
> > 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