Hi Janusz, On 2023-10-03 at 11:10:53 +0200, Janusz Krzysztofik wrote: > Recent improvements to the kernel kunit framework allow us to obtain a > list of test cases provided by a kunit test module without actually > running them. Use that feature to get a list of expected test cases > before we enter a loop around igt_dynamic(). Once done, enter the > igt_dynamic() section for each consecutive test case immediately, even > before first line of a related KTAP report appears, then look for a result > from that test case. That should make our IGT results output still better > synchronized with related kernel messages. > > The list of test cases provided by a kunit test module can be obtained by > loading the kunit base module with specific options, then loading the test > module. For that to be possible, take care of unloading the kunit base > module before each kunit subtest (I was wrong when in one of my previous > commit messages I suggested that on final unload of a kunit test module > the kunit base module is unloaded automatically as its dependency, > however, that didn't matter before, then no separate fix was required). > Since that module can then be left loaded with non-default options if an > error occurs, unload it explicitly before returning from igt_kunit(). > > There are two possible ways of getting a list of test cases: by loading > the base kunit module with action=list module option, or by filtering > out all test cases from being executed while asking for SKIP results from > those filtered out. Since the latter provides regular KTAP report that we > can alredy parse perfectly, use it instead of trying to identify an ----- ^^^^^^ s/alredy/already/
> unstructured list of test cases of unknown length submitted by the former. > > If an IGT test that calls igt_kunit() provides a subtest name then use > that name to filter out potential test cases that don't belong to the > named test suite from the list. > > To avoid loading any modules if no subtest is going to be executed (e.g., > if a nonexistent subtest has been requested), load the kunit modules in > list mode from inside the igt_subtest_with_dynamic() section. In order to > be free to skip the whole subtest on unmet requirements that need to be > verified after that list has been already populated, clean it up from a > follow up igt_fixture section. > > Since we start reading the list of test cases from /dev/kmsg only after > the kunit test module is loaded successfully in list only mode, don't > synchronize reads with potential modprobe breakage in that case, unlike we > still do later when parsing KTAP results in parallel to loading the test > module in normal (execute) mode. > > Since we neither fetch KTAP results before entering igt_dynamic section > nor even return an error from KTAP result fetch attempts immediately on > modprobe error or kernel taint, break the loop of dynamic sub-subtests > explicitly as soon as one of those conditions is detected. Could you split this patch into two, one with reading names and rest with modprobe error or kernel taint detection? > Also, don't > force IGT SKIP result from the subtest if KTAP parsing hasn't completed. > That's perfectly legitimate since we no longer iterate over KTAP results, > only over a list of test cases obtained in advance, then we stop parsing > KTAP report as soon as we get a result from the last test case from the > list. > > Signed-off-by: Janusz Krzysztofik <[email protected]> > --- > lib/igt_kmod.c | 217 +++++++++++++++++++++++++++++++++++-------------- > 1 file changed, 157 insertions(+), 60 deletions(-) > > diff --git a/lib/igt_kmod.c b/lib/igt_kmod.c > index 387efbb59f..4fba77ead4 100644 > --- a/lib/igt_kmod.c > +++ b/lib/igt_kmod.c > @@ -802,34 +802,36 @@ static int kunit_kmsg_result_get(struct igt_list_head > *results, > if (igt_debug_on(igt_kernel_tainted(&taints))) > return -ENOTRECOVERABLE; > > - err = igt_debug_on(sigaction(SIGCHLD, &sigchld, saved)); > - if (err == -1) > - return -errno; > - else if (unlikely(err)) > - return err; > - > - err = pthread_mutex_lock(&modprobe->lock); > - switch (err) { > - case EOWNERDEAD: > - /* leave the mutex unrecoverable */ > - igt_debug_on(pthread_mutex_unlock(&modprobe->lock)); > - __attribute__ ((fallthrough)); > - case ENOTRECOVERABLE: > - igt_debug_on(sigaction(SIGCHLD, saved, NULL)); > - if (igt_debug_on(modprobe->err)) > - return modprobe->err; > - break; > - case 0: > - break; > - default: > - igt_debug("pthread_mutex_lock() error: %d\n", err); > - igt_debug_on(sigaction(SIGCHLD, saved, NULL)); > - return -err; > + if (modprobe) { > + err = igt_debug_on(sigaction(SIGCHLD, &sigchld, saved)); > + if (err == -1) > + return -errno; > + else if (unlikely(err)) > + return err; > + > + err = pthread_mutex_lock(&modprobe->lock); > + switch (err) { > + case EOWNERDEAD: > + /* leave the mutex unrecoverable */ > + > igt_debug_on(pthread_mutex_unlock(&modprobe->lock)); > + __attribute__ ((fallthrough)); > + case ENOTRECOVERABLE: > + igt_debug_on(sigaction(SIGCHLD, saved, NULL)); > + if (igt_debug_on(modprobe->err)) > + return modprobe->err; > + break; > + case 0: > + break; > + default: > + igt_debug("pthread_mutex_lock() error: %d\n", > err); > + igt_debug_on(sigaction(SIGCHLD, saved, NULL)); > + return -err; > + } > } > > ret = read(fd, record, BUF_LEN); > > - if (!err) { > + if (modprobe && !err) { > igt_debug_on(pthread_mutex_unlock(&modprobe->lock)); > igt_debug_on(sigaction(SIGCHLD, saved, NULL)); > } > @@ -885,17 +887,15 @@ static void kunit_result_free(struct igt_ktap_result > **r, > *r = NULL; > } > > -static void > -__igt_kunit(struct igt_ktest *tst, const char *name, const char *opts) > +static void kunit_get_tests(struct igt_list_head *tests, > + struct igt_ktest *tst, > + const char *filter, > + const char *opts) > { > - struct modprobe_data modprobe = { pthread_self(), tst->kmod, opts, 0, }; > char *suite_name = NULL, *case_name = NULL; > struct igt_ktap_result *r, *rn; > struct igt_ktap_results *ktap; > - pthread_mutexattr_t attr; > - IGT_LIST_HEAD(results); > - unsigned long taints; > - int flags, ret; > + int flags, err; > > igt_skip_on_f(tst->kmsg < 0, "Could not open /dev/kmsg\n"); > > @@ -905,6 +905,63 @@ __igt_kunit(struct igt_ktest *tst, const char *name, > const char *opts) > > igt_skip_on(lseek(tst->kmsg, 0, SEEK_END) < 0); > > + /* > + * To get a list of test cases provided by a kunit test module, ask the > + * generic kunit module to respond with SKIP result for each test found. > + * We could also use action=list kunit parameter to get the listing, > + * however, parsing a KTAP report -- something that we alredy can do --------------------------------------------------------------^ Same here. Regards, Kamil > + * perfectly -- seems to be more safe than extracting a test case list > + * of unknown length from /dev/kmsg. > + */ > + igt_skip_on(igt_kmod_load("kunit", "filter=module=none > filter_action=skip")); > + igt_skip_on(modprobe(tst->kmod, opts)); > + > + ktap = igt_ktap_alloc(tests); > + igt_require(ktap); > + > + do { > + err = kunit_kmsg_result_get(tests, NULL, tst->kmsg, ktap); > + if (err && !igt_debug_on(err != -EINPROGRESS)) { > + r = igt_list_last_entry(tests, r, link); > + > + if (igt_debug_on(r->code != IGT_EXIT_SKIP)) > + err = r->code ?: -EPROTO; > + } > + > + } while (err == -EINPROGRESS); > + > + igt_ktap_free(ktap); > + > + if (err || filter) { > + igt_list_for_each_entry_safe(r, rn, tests, link) { > + if (err || strcmp(r->suite_name, filter)) > + kunit_result_free(&r, &case_name, &suite_name); > + } > + } > + > + igt_skip_on(kmod_module_remove_module(tst->kmod, KMOD_REMOVE_FORCE)); > + igt_skip_on(igt_kmod_unload("kunit", KMOD_REMOVE_FORCE)); > + > + igt_skip_on_f(err, > + "KTAP parser failed while getting a list of test > cases\n"); > +} > + > +static void __igt_kunit(struct igt_ktest *tst, > + const char *name, > + const char *opts, > + struct igt_list_head *tests) > +{ > + struct modprobe_data modprobe = { pthread_self(), tst->kmod, opts, 0, }; > + char *suite_name = NULL, *case_name = NULL; > + struct igt_ktap_result *t, *r = NULL, *rn; > + struct igt_ktap_results *ktap; > + pthread_mutexattr_t attr; > + IGT_LIST_HEAD(results); > + int ret = -EINPROGRESS; > + unsigned long taints; > + > + igt_skip_on(lseek(tst->kmsg, 0, SEEK_END) < 0); > + > igt_skip_on(pthread_mutexattr_init(&attr)); > igt_skip_on(pthread_mutexattr_setrobust(&attr, PTHREAD_MUTEX_ROBUST)); > igt_skip_on(pthread_mutex_init(&modprobe.lock, &attr)); > @@ -918,37 +975,50 @@ __igt_kunit(struct igt_ktest *tst, const char *name, > const char *opts) > igt_skip("Failed to create a modprobe thread\n"); > } > > - do { > - ret = kunit_kmsg_result_get(&results, &modprobe, > - tst->kmsg, ktap); > - if (igt_debug_on(ret && ret != -EINPROGRESS)) > - break; > - > - if (igt_debug_on(igt_list_empty(&results))) > - break; > - > - r = igt_list_first_entry(&results, r, link); > - > + igt_list_for_each_entry(t, tests, link) { > igt_dynamic_f("%s%s%s", > - strcmp(r->suite_name, name) ? r->suite_name : "", > - strcmp(r->suite_name, name) ? "-" : "", > - r->case_name) { > + strcmp(t->suite_name, name) ? t->suite_name : "", > + strcmp(t->suite_name, name) ? "-" : "", > + t->case_name) { > + if (!r) { > + if (igt_list_empty(&results)) { > + igt_assert_eq(ret, -EINPROGRESS); > + ret = kunit_kmsg_result_get(&results, > + &modprobe, > + tst->kmsg, > + ktap); > + igt_fail_on(igt_list_empty(&results)); > + } > > - if (r->code == IGT_EXIT_INVALID) { > - /* parametrized test case, get actual result */ > - kunit_result_free(&r, &suite_name, &case_name); > + r = igt_list_first_entry(&results, r, link); > + } > > - igt_assert(igt_list_empty(&results)); > + while (igt_debug_on_f(strcmp(r->suite_name, > t->suite_name), > + "suite_name expected: %s, got: > %s\n", > + t->suite_name, r->suite_name) || > + igt_debug_on_f(strcmp(r->case_name, > t->case_name), > + "case_name expected: %s, got: > %s\n", > + t->case_name, r->case_name) || > + r->code == IGT_EXIT_INVALID) { > > - ret = kunit_kmsg_result_get(&results, &modprobe, > - tst->kmsg, ktap); > - if (ret != -EINPROGRESS) > - igt_fail_on(ret); > + int code = r->code; > > - igt_fail_on(igt_list_empty(&results)); > + kunit_result_free(&r, &suite_name, &case_name); > + if (igt_list_empty(&results)) { > + igt_assert_eq(ret, -EINPROGRESS); > + ret = kunit_kmsg_result_get(&results, > + &modprobe, > + tst->kmsg, > + ktap); > + igt_fail_on(igt_list_empty(&results)); > + } > > r = igt_list_first_entry(&results, r, link); > > + if (code != IGT_EXIT_INVALID) > + continue; > + > + /* result from parametrized test case */ > igt_fail_on_f(strcmp(r->suite_name, suite_name), > "suite_name expected: %s, got: > %s\n", > suite_name, r->suite_name); > @@ -995,7 +1065,9 @@ __igt_kunit(struct igt_ktest *tst, const char *name, > const char *opts) > > kunit_result_free(&r, &suite_name, &case_name); > > - } while (ret == -EINPROGRESS); > + if (modprobe.err || igt_kernel_tainted(&taints)) > + break; > + } > > igt_list_for_each_entry_safe(r, rn, &results, link) > kunit_result_free(&r, &suite_name, &case_name); > @@ -1025,7 +1097,8 @@ __igt_kunit(struct igt_ktest *tst, const char *name, > const char *opts) > > igt_skip_on(modprobe.err); > igt_skip_on(igt_kernel_tainted(&taints)); > - igt_skip_on_f(ret, "KTAP parser failed\n"); > + if (ret != -EINPROGRESS) > + igt_skip_on_f(ret, "KTAP parser failed\n"); > } > > /** > @@ -1039,7 +1112,8 @@ __igt_kunit(struct igt_ktest *tst, const char *name, > const char *opts) > void igt_kunit(const char *module_name, const char *name, const char *opts) > { > struct igt_ktest tst = { .kmsg = -1, }; > - > + const char *filter = name; > + IGT_LIST_HEAD(tests); > > /* > * If the caller (an IGT test) provides no subtest name then we > @@ -1064,6 +1138,15 @@ void igt_kunit(const char *module_name, const char > *name, const char *opts) > > igt_skip_on(igt_ktest_init(&tst, module_name)); > igt_skip_on(igt_ktest_begin(&tst)); > + > + /* > + * Since we need to load kunit base module with specific > + * options in order to get a list of test cases, make > + * sure that the module is not loaded. However, since > + * unload may fail if kunit base module is not loaded, > + * ignore any failures, we'll fail later if still loaded. > + */ > + igt_ignore_warn(igt_kmod_unload("kunit", KMOD_REMOVE_FORCE)); > } > > /* > @@ -1073,11 +1156,25 @@ void igt_kunit(const char *module_name, const char > *name, const char *opts) > * proper namespace for dynamic subtests, with is required for CI > * and for documentation. > */ > - igt_subtest_with_dynamic(name) > - __igt_kunit(&tst, name, opts); > + igt_subtest_with_dynamic(name) { > + kunit_get_tests(&tests, &tst, filter, opts); > + igt_skip_on(igt_list_empty(&tests)); > + > + __igt_kunit(&tst, name, opts, &tests); > + } > + > + igt_fixture { > + char *suite_name = NULL, *case_name = NULL; > + struct igt_ktap_result *t, *tn; > + > + igt_list_for_each_entry_safe(t, tn, &tests, link) > + kunit_result_free(&t, &suite_name, &case_name); > + free(case_name); > + free(suite_name); > > - igt_fixture > igt_ktest_end(&tst); > + igt_debug_on(igt_kmod_unload("kunit", KMOD_REMOVE_FORCE)); > + } > > igt_ktest_fini(&tst); > } > -- > 2.42.0 >
