| Message ID | 20230908123233.137134-24-janusz.krzysztofik@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Fix IGT Kunit implementation issues | expand |
On Fri, 8 Sep 2023 14:32:39 +0200 Janusz Krzysztofik <janusz.krzysztofik@linux.intel.com> wrote: > In a body of a subtest with dynamic sub-subtests, it is illegal to call > igt_fail() and its variants from outside of a dynamic sub-subtest body. > On the other hand, it is perfectly legal to call either igt_skip() and > friends or __igt_abort() or its variant from there. > > In the current implementation of igt_kunit(), there are several places > where igt_fail() is called despite being illegal. Moreover, it is called > with IGT_EXIT_ABORT as an argument with no good reason for using such > aggressive method that forces CI to trigger system reboot (in most cases > igt_runner can decide if abort is required). > > Follow igt_kselftests() pattern more closely, where similar setup and > cleanup operations are performed but their potential errors are processed > in a more friendly way. Move common cleanup and their corresponding setup > steps out of the subtest body. Place the latter as requirements in a > preceding igt_fixture section. Replace remaining illegal igt_fail() calls > with more friendly skips. Let igt_runner decide if abort is needed. > > Signed-off-by: Janusz Krzysztofik <janusz.krzysztofik@linux.intel.com> > --- > lib/igt_kmod.c | 75 +++++++++++++++----------------------------------- > 1 file changed, 22 insertions(+), 53 deletions(-) > > diff --git a/lib/igt_kmod.c b/lib/igt_kmod.c > index 1d1cd51170..78b8eb8f53 100644 > --- a/lib/igt_kmod.c > +++ b/lib/igt_kmod.c > @@ -754,59 +754,27 @@ void igt_kselftest_get_tests(struct kmod_module *kmod, > * > * Returns: IGT default codes > */ > -static void __igt_kunit(const char *module_name, const char *opts) > +static void __igt_kunit(struct igt_ktest *tst, const char *opts) > { > - struct igt_ktest tst; > struct kmod_module *kunit_kmod; > bool is_builtin; > int ret; > struct ktap_test_results *results; > struct ktap_test_results_element *temp; > - int skip = 0; > - bool fail = 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_ABORT); > - } > - > - 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_ABORT); > - } > > - if (tst.kmsg < 0) { > - igt_warn("Could not open /dev/kmsg\n"); > - fail = true; > - goto unload; > - } > + igt_skip_on_f(tst->kmsg < 0, "Could not open /dev/kmsg\n"); > > - if (lseek(tst.kmsg, 0, SEEK_END)) { > - igt_warn("Could not seek the end of /dev/kmsg\n"); > - fail = true; > - goto unload; > - } > - > - ret = kmod_module_new_from_name(kmod_ctx(), "kunit", &kunit_kmod); > - if (ret) { > - igt_warn("Unable to load KUnit\n"); > - skip = ret; > - goto unload; > - } > + igt_skip_on(lseek(tst->kmsg, 0, SEEK_END) < 0); > > + igt_skip_on(kmod_module_new_from_name(kmod_ctx(), "kunit", &kunit_kmod)); > is_builtin = kmod_module_get_initstate(kunit_kmod) == KMOD_MODULE_BUILTIN; > kmod_module_unref(kunit_kmod); > > - results = ktap_parser_start(tst.kmsg, is_builtin); > + results = ktap_parser_start(tst->kmsg, is_builtin); > > - ret = igt_kmod_load(module_name, opts); > - if (ret) { > - skip = ret; > - igt_warn("Unable to load %s module\n", module_name); > - ret = ktap_parser_stop(); > - goto unload; > + if (igt_debug_on(igt_kmod_load(tst->module_name, opts) < 0)) { > + igt_ignore_warn(ktap_parser_stop()); > + igt_skip("Unable to load %s module\n", tst->module_name); > } > > while (READ_ONCE(results->still_running) || READ_ONCE(results->head) != NULL) > @@ -825,24 +793,21 @@ static void __igt_kunit(const char *module_name, const char *opts) > } > } > > -unload: > - igt_ktest_end(&tst); > - > - igt_ktest_fini(&tst); > - > - igt_skip_on_f(skip, "Skipping test, as probing KUnit module failed\n"); > - > - if (fail) > - igt_fail(IGT_EXIT_ABORT); > - > ret = ktap_parser_stop(); > > - if (ret != 0) > - igt_fail(IGT_EXIT_ABORT); > + igt_skip_on_f(ret, "KTAP parser failed\n"); > } > > void igt_kunit(const char *module_name, const char *name, const char *opts) > { > + struct igt_ktest tst; > + > + if (igt_ktest_init(&tst, module_name) != 0) > + return; Shouldn't it be calling igt_skip() here too? > + > + igt_fixture > + igt_require(igt_ktest_begin(&tst) == 0); > + > /* > * We need to use igt_subtest here, as otherwise it may crash with: > * skipping is allowed only in fixtures, subtests or igt_simple_main > @@ -854,7 +819,11 @@ void igt_kunit(const char *module_name, const char *name, const char *opts) > name = module_name; > > igt_subtest_with_dynamic(name) > - __igt_kunit(module_name, opts); > + __igt_kunit(&tst, opts); > + > + igt_ktest_end(&tst); > + > + igt_ktest_fini(&tst); > } > > static int open_parameters(const char *module_name)
Hi Mauro, Thanks for review. On Monday, 11 September 2023 10:52:51 CEST Mauro Carvalho Chehab wrote: > On Fri, 8 Sep 2023 14:32:39 +0200 > Janusz Krzysztofik <janusz.krzysztofik@linux.intel.com> wrote: > > > In a body of a subtest with dynamic sub-subtests, it is illegal to call > > igt_fail() and its variants from outside of a dynamic sub-subtest body. > > On the other hand, it is perfectly legal to call either igt_skip() and > > friends or __igt_abort() or its variant from there. > > > > In the current implementation of igt_kunit(), there are several places > > where igt_fail() is called despite being illegal. Moreover, it is called > > with IGT_EXIT_ABORT as an argument with no good reason for using such > > aggressive method that forces CI to trigger system reboot (in most cases > > igt_runner can decide if abort is required). > > > > Follow igt_kselftests() pattern more closely, where similar setup and > > cleanup operations are performed but their potential errors are processed > > in a more friendly way. Move common cleanup and their corresponding setup > > steps out of the subtest body. Place the latter as requirements in a > > preceding igt_fixture section. Replace remaining illegal igt_fail() calls > > with more friendly skips. Let igt_runner decide if abort is needed. > > > > Signed-off-by: Janusz Krzysztofik <janusz.krzysztofik@linux.intel.com> > > --- > > lib/igt_kmod.c | 75 +++++++++++++++----------------------------------- > > 1 file changed, 22 insertions(+), 53 deletions(-) > > > > diff --git a/lib/igt_kmod.c b/lib/igt_kmod.c > > index 1d1cd51170..78b8eb8f53 100644 > > --- a/lib/igt_kmod.c > > +++ b/lib/igt_kmod.c ... > > @@ -825,24 +793,21 @@ static void __igt_kunit(const char *module_name, const char *opts) > > } > > } > > > > -unload: > > - igt_ktest_end(&tst); > > - > > - igt_ktest_fini(&tst); > > - > > - igt_skip_on_f(skip, "Skipping test, as probing KUnit module failed\n"); > > - > > - if (fail) > > - igt_fail(IGT_EXIT_ABORT); > > - > > ret = ktap_parser_stop(); > > > > - if (ret != 0) > > - igt_fail(IGT_EXIT_ABORT); > > + igt_skip_on_f(ret, "KTAP parser failed\n"); > > } > > > > void igt_kunit(const char *module_name, const char *name, const char *opts) > > { > > + struct igt_ktest tst; > > + > > + if (igt_ktest_init(&tst, module_name) != 0) > > + return; > > Shouldn't it be calling igt_skip() here too? Maybe yes. I've chosen to follow the algorithm used in igt_kselftest. There was an igt_skip() variant there initially but in 2017 that was converted to the current return only by Peter with IGT commit 9f92893b11e8 ("lib/igt_kmod: Don't call igt_assert or igt_require without a fixture"). However, justification for dropping igt_require() instead of calling it from an igt_fixture section may not apply to kunit modules: "If kmod_module_new_from_name fails, ... return normally from igt_kselftest, matching behaviour when the module loading is successful but it doesn't contain selftests." While i915 could be built with no selftests included, a kunit module without any tests doesn't make sense, then silent return may be not what we need. Thanks, Janusz > > > + > > + igt_fixture > > + igt_require(igt_ktest_begin(&tst) == 0); > > + > > /* > > * We need to use igt_subtest here, as otherwise it may crash with: > > * skipping is allowed only in fixtures, subtests or igt_simple_main > > @@ -854,7 +819,11 @@ void igt_kunit(const char *module_name, const char *name, const char *opts) > > name = module_name; > > > > igt_subtest_with_dynamic(name) > > - __igt_kunit(module_name, opts); > > + __igt_kunit(&tst, opts); > > + > > + igt_ktest_end(&tst); > > + > > + igt_ktest_fini(&tst); > > } > > > > static int open_parameters(const char *module_name) >
On Mon, 11 Sep 2023 11:28:32 +0200 Janusz Krzysztofik <janusz.krzysztofik@linux.intel.com> wrote: > Hi Mauro, > > Thanks for review. > > On Monday, 11 September 2023 10:52:51 CEST Mauro Carvalho Chehab wrote: > > On Fri, 8 Sep 2023 14:32:39 +0200 > > Janusz Krzysztofik <janusz.krzysztofik@linux.intel.com> wrote: > > > > > In a body of a subtest with dynamic sub-subtests, it is illegal to call > > > igt_fail() and its variants from outside of a dynamic sub-subtest body. > > > On the other hand, it is perfectly legal to call either igt_skip() and > > > friends or __igt_abort() or its variant from there. > > > > > > In the current implementation of igt_kunit(), there are several places > > > where igt_fail() is called despite being illegal. Moreover, it is called > > > with IGT_EXIT_ABORT as an argument with no good reason for using such > > > aggressive method that forces CI to trigger system reboot (in most cases > > > igt_runner can decide if abort is required). > > > > > > Follow igt_kselftests() pattern more closely, where similar setup and > > > cleanup operations are performed but their potential errors are processed > > > in a more friendly way. Move common cleanup and their corresponding setup > > > steps out of the subtest body. Place the latter as requirements in a > > > preceding igt_fixture section. Replace remaining illegal igt_fail() calls > > > with more friendly skips. Let igt_runner decide if abort is needed. > > > > > > Signed-off-by: Janusz Krzysztofik <janusz.krzysztofik@linux.intel.com> > > > --- > > > lib/igt_kmod.c | 75 +++++++++++++++----------------------------------- > > > 1 file changed, 22 insertions(+), 53 deletions(-) > > > > > > diff --git a/lib/igt_kmod.c b/lib/igt_kmod.c > > > index 1d1cd51170..78b8eb8f53 100644 > > > --- a/lib/igt_kmod.c > > > +++ b/lib/igt_kmod.c > ... > > > @@ -825,24 +793,21 @@ static void __igt_kunit(const char *module_name, const char *opts) > > > } > > > } > > > > > > -unload: > > > - igt_ktest_end(&tst); > > > - > > > - igt_ktest_fini(&tst); > > > - > > > - igt_skip_on_f(skip, "Skipping test, as probing KUnit module failed\n"); > > > - > > > - if (fail) > > > - igt_fail(IGT_EXIT_ABORT); > > > - > > > ret = ktap_parser_stop(); > > > > > > - if (ret != 0) > > > - igt_fail(IGT_EXIT_ABORT); > > > + igt_skip_on_f(ret, "KTAP parser failed\n"); > > > } > > > > > > void igt_kunit(const char *module_name, const char *name, const char *opts) > > > { > > > + struct igt_ktest tst; > > > + > > > + if (igt_ktest_init(&tst, module_name) != 0) > > > + return; > > > > Shouldn't it be calling igt_skip() here too? > > Maybe yes. I've chosen to follow the algorithm used in igt_kselftest. There > was an igt_skip() variant there initially but in 2017 that was converted to > the current return only by Peter with IGT commit 9f92893b11e8 ("lib/igt_kmod: > Don't call igt_assert or igt_require without a fixture"). However, > justification for dropping igt_require() instead of calling it from an > igt_fixture section may not apply to kunit modules: > > "If kmod_module_new_from_name fails, ... return normally from igt_kselftest, > matching behaviour when the module loading is successful but it doesn't > contain selftests." > > While i915 could be built with no selftests included, a kunit module without > any tests doesn't make sense, then silent return may be not what we need. Yeah, selftests are handled on a different way with regards to module probe, so I guess we need the igt_skip there if modprobe fails. Well, you can probably simulate it by renaming a Kunit module and see how IGT will handle that with the current code and with igt_skip(). (Btw, I intend to review the other patches on this series, but need some time to do tests, as some changes here are not trivial) Regards, Mauro
On Monday, 11 September 2023 13:57:29 CEST Mauro Carvalho Chehab wrote: > On Mon, 11 Sep 2023 11:28:32 +0200 > Janusz Krzysztofik <janusz.krzysztofik@linux.intel.com> wrote: > > > Hi Mauro, > > > > Thanks for review. > > > > On Monday, 11 September 2023 10:52:51 CEST Mauro Carvalho Chehab wrote: > > > On Fri, 8 Sep 2023 14:32:39 +0200 > > > Janusz Krzysztofik <janusz.krzysztofik@linux.intel.com> wrote: > > > > > > > In a body of a subtest with dynamic sub-subtests, it is illegal to call > > > > igt_fail() and its variants from outside of a dynamic sub-subtest body. > > > > On the other hand, it is perfectly legal to call either igt_skip() and > > > > friends or __igt_abort() or its variant from there. > > > > > > > > In the current implementation of igt_kunit(), there are several places > > > > where igt_fail() is called despite being illegal. Moreover, it is called > > > > with IGT_EXIT_ABORT as an argument with no good reason for using such > > > > aggressive method that forces CI to trigger system reboot (in most cases > > > > igt_runner can decide if abort is required). > > > > > > > > Follow igt_kselftests() pattern more closely, where similar setup and > > > > cleanup operations are performed but their potential errors are processed > > > > in a more friendly way. Move common cleanup and their corresponding setup > > > > steps out of the subtest body. Place the latter as requirements in a > > > > preceding igt_fixture section. Replace remaining illegal igt_fail() calls > > > > with more friendly skips. Let igt_runner decide if abort is needed. > > > > > > > > Signed-off-by: Janusz Krzysztofik <janusz.krzysztofik@linux.intel.com> > > > > --- > > > > lib/igt_kmod.c | 75 ++++++++++++++ +----------------------------------- > > > > 1 file changed, 22 insertions(+), 53 deletions(-) > > > > > > > > diff --git a/lib/igt_kmod.c b/lib/igt_kmod.c > > > > index 1d1cd51170..78b8eb8f53 100644 > > > > --- a/lib/igt_kmod.c > > > > +++ b/lib/igt_kmod.c > > ... > > > > @@ -825,24 +793,21 @@ static void __igt_kunit(const char *module_name, const char *opts) > > > > } > > > > } > > > > > > > > -unload: > > > > - igt_ktest_end(&tst); > > > > - > > > > - igt_ktest_fini(&tst); > > > > - > > > > - igt_skip_on_f(skip, "Skipping test, as probing KUnit module failed\n"); > > > > - > > > > - if (fail) > > > > - igt_fail(IGT_EXIT_ABORT); > > > > - > > > > ret = ktap_parser_stop(); > > > > > > > > - if (ret != 0) > > > > - igt_fail(IGT_EXIT_ABORT); > > > > + igt_skip_on_f(ret, "KTAP parser failed\n"); > > > > } > > > > > > > > void igt_kunit(const char *module_name, const char *name, const char *opts) > > > > { > > > > + struct igt_ktest tst; > > > > + > > > > + if (igt_ktest_init(&tst, module_name) != 0) > > > > + return; > > > > > > Shouldn't it be calling igt_skip() here too? > > > > Maybe yes. I've chosen to follow the algorithm used in igt_kselftest. There > > was an igt_skip() variant there initially but in 2017 that was converted to > > the current return only by Peter with IGT commit 9f92893b11e8 ("lib/ igt_kmod: > > Don't call igt_assert or igt_require without a fixture"). However, > > justification for dropping igt_require() instead of calling it from an > > igt_fixture section may not apply to kunit modules: > > > > "If kmod_module_new_from_name fails, ... return normally from igt_kselftest, > > matching behaviour when the module loading is successful but it doesn't > > contain selftests." > > > > While i915 could be built with no selftests included, a kunit module without > > any tests doesn't make sense, then silent return may be not what we need. > > Yeah, selftests are handled on a different way with regards to module > probe, so I guess we need the igt_skip there if modprobe fails. After having a closer look at it, I think that igt_ktest_init() has nothing to do with actual modprobe, and it can fail only on either no memory or if module_name == NULL. Anyway, I'll make the subtest skip if it fails. > Well, you can probably simulate it by renaming a Kunit module > and see how IGT will handle that with the current code and with > igt_skip(). Yes, I've tired, and my results have confirmed my conclusions from code review. But more important, I've found an issue in patch 15/17, "Parse KTAP report from the main process thread", that can cause first read() to wait infinitely, unless interrupted, if modprobe fails. I've already developed a working fix that interrupts that read() on modprobe failure, and I'll include it in next version of the series. Thanks, Janusz > > (Btw, I intend to review the other patches on this series, but need > some time to do tests, as some changes here are not trivial) > > Regards, > Mauro >
diff --git a/lib/igt_kmod.c b/lib/igt_kmod.c index 1d1cd51170..78b8eb8f53 100644 --- a/lib/igt_kmod.c +++ b/lib/igt_kmod.c @@ -754,59 +754,27 @@ void igt_kselftest_get_tests(struct kmod_module *kmod, * * Returns: IGT default codes */ -static void __igt_kunit(const char *module_name, const char *opts) +static void __igt_kunit(struct igt_ktest *tst, const char *opts) { - struct igt_ktest tst; struct kmod_module *kunit_kmod; bool is_builtin; int ret; struct ktap_test_results *results; struct ktap_test_results_element *temp; - int skip = 0; - bool fail = 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_ABORT); - } - - 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_ABORT); - } - if (tst.kmsg < 0) { - igt_warn("Could not open /dev/kmsg\n"); - fail = true; - goto unload; - } + igt_skip_on_f(tst->kmsg < 0, "Could not open /dev/kmsg\n"); - if (lseek(tst.kmsg, 0, SEEK_END)) { - igt_warn("Could not seek the end of /dev/kmsg\n"); - fail = true; - goto unload; - } - - ret = kmod_module_new_from_name(kmod_ctx(), "kunit", &kunit_kmod); - if (ret) { - igt_warn("Unable to load KUnit\n"); - skip = ret; - goto unload; - } + igt_skip_on(lseek(tst->kmsg, 0, SEEK_END) < 0); + igt_skip_on(kmod_module_new_from_name(kmod_ctx(), "kunit", &kunit_kmod)); is_builtin = kmod_module_get_initstate(kunit_kmod) == KMOD_MODULE_BUILTIN; kmod_module_unref(kunit_kmod); - results = ktap_parser_start(tst.kmsg, is_builtin); + results = ktap_parser_start(tst->kmsg, is_builtin); - ret = igt_kmod_load(module_name, opts); - if (ret) { - skip = ret; - igt_warn("Unable to load %s module\n", module_name); - ret = ktap_parser_stop(); - goto unload; + if (igt_debug_on(igt_kmod_load(tst->module_name, opts) < 0)) { + igt_ignore_warn(ktap_parser_stop()); + igt_skip("Unable to load %s module\n", tst->module_name); } while (READ_ONCE(results->still_running) || READ_ONCE(results->head) != NULL) @@ -825,24 +793,21 @@ static void __igt_kunit(const char *module_name, const char *opts) } } -unload: - igt_ktest_end(&tst); - - igt_ktest_fini(&tst); - - igt_skip_on_f(skip, "Skipping test, as probing KUnit module failed\n"); - - if (fail) - igt_fail(IGT_EXIT_ABORT); - ret = ktap_parser_stop(); - if (ret != 0) - igt_fail(IGT_EXIT_ABORT); + igt_skip_on_f(ret, "KTAP parser failed\n"); } void igt_kunit(const char *module_name, const char *name, const char *opts) { + struct igt_ktest tst; + + if (igt_ktest_init(&tst, module_name) != 0) + return; + + igt_fixture + igt_require(igt_ktest_begin(&tst) == 0); + /* * We need to use igt_subtest here, as otherwise it may crash with: * skipping is allowed only in fixtures, subtests or igt_simple_main @@ -854,7 +819,11 @@ void igt_kunit(const char *module_name, const char *name, const char *opts) name = module_name; igt_subtest_with_dynamic(name) - __igt_kunit(module_name, opts); + __igt_kunit(&tst, opts); + + igt_ktest_end(&tst); + + igt_ktest_fini(&tst); } static int open_parameters(const char *module_name)
In a body of a subtest with dynamic sub-subtests, it is illegal to call igt_fail() and its variants from outside of a dynamic sub-subtest body. On the other hand, it is perfectly legal to call either igt_skip() and friends or __igt_abort() or its variant from there. In the current implementation of igt_kunit(), there are several places where igt_fail() is called despite being illegal. Moreover, it is called with IGT_EXIT_ABORT as an argument with no good reason for using such aggressive method that forces CI to trigger system reboot (in most cases igt_runner can decide if abort is required). Follow igt_kselftests() pattern more closely, where similar setup and cleanup operations are performed but their potential errors are processed in a more friendly way. Move common cleanup and their corresponding setup steps out of the subtest body. Place the latter as requirements in a preceding igt_fixture section. Replace remaining illegal igt_fail() calls with more friendly skips. Let igt_runner decide if abort is needed. Signed-off-by: Janusz Krzysztofik <janusz.krzysztofik@linux.intel.com> --- lib/igt_kmod.c | 75 +++++++++++++++----------------------------------- 1 file changed, 22 insertions(+), 53 deletions(-)