Re: [Intel-gfx] [PATCH i-g-t v2 05/17] lib/kunit: Fix illegal igt_fail() calls inside subtest body
On Monday, 11 September 2023 13:57:29 CEST Mauro Carvalho Chehab wrote: > On Mon, 11 Sep 2023 11:28:32 +0200 > Janusz Krzysztofik 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 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 > > > > --- > > > > 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(); > > > > - > > > > - igt_ktest_fini(); > > > > - > > > > - 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(, 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 >
Re: [Intel-gfx] [PATCH i-g-t v2 05/17] lib/kunit: Fix illegal igt_fail() calls inside subtest body
On Mon, 11 Sep 2023 11:28:32 +0200 Janusz Krzysztofik 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 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 > > > --- > > > 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(); > > > - > > > - igt_ktest_fini(); > > > - > > > - 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(, 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
Re: [Intel-gfx] [PATCH i-g-t v2 05/17] lib/kunit: Fix illegal igt_fail() calls inside subtest body
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 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 > > --- > > 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(); > > - > > - igt_ktest_fini(); > > - > > - 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(, 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() == 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(, opts); > > + > > + igt_ktest_end(); > > + > > + igt_ktest_fini(); > > } > > > > static int open_parameters(const char *module_name) >
Re: [Intel-gfx] [PATCH i-g-t v2 05/17] lib/kunit: Fix illegal igt_fail() calls inside subtest body
On Fri, 8 Sep 2023 14:32:39 +0200 Janusz Krzysztofik 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 > --- > 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(, module_name) != 0) { > - igt_warn("Unable to initialize ktest for %s\n", module_name); > - igt_fail(IGT_EXIT_ABORT); > - } > - > - if (igt_ktest_begin() != 0) { > - igt_warn("Unable to begin ktest for %s\n", module_name); > - igt_ktest_fini(); > - 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", _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", > _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(); > - > - igt_ktest_fini(); > - > - 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(, module_name) != 0) > + return; Shouldn't it be calling igt_skip() here too? > + > + igt_fixture > + igt_require(igt_ktest_begin() == 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 >