[PATCH i-g-t v2 4/6] lib/kunit: Support writable filter* parameters of kunit module
Janusz Krzysztofik
janusz.krzysztofik at linux.intel.com
Thu Feb 1 16:13:14 UTC 2024
Hi Kamil,
On Thursday, 1 February 2024 16:07:41 CET Kamil Konieczny wrote:
> Hi Janusz,
> On 2024-01-31 at 19:03:51 +0100, Janusz Krzysztofik wrote:
> > Instead of wasting resources on reloading the base Kunit module each time
> > a different set of filter parameters is needed, try to write the required
> > values to sysfs representation of those parameters. If that fails (e.g.
> > on older LTS kernels with read-only filter parameters), fall back to
> > legacy "no list of test cases available" mode as we do on kernels with
> > those module parameters not supported at all.
> >
> > This modification will also serve as a workaround for the issue of
> > impossibility to unload the base Kunit module on Xe platforms, available
> > to IGT as soon as the module supports writable filter parameters.
> >
> > v2: Work around ineffective writes of empty strings to sysfs module
> > parameter files (Lucas) by using human readable non-empty strings that
> > give the same results as default NULLs,
> > - drop fallback to reload of base Kunit module method if assigning new
> > values to module parameters via sysfs fails (Lucas), instead use the
> > existing fallback to blind execution like if getting a list of test
> > cases was not supported at all,
> > - split move of open_parameters() helper up in the source file as well
> > as cleanup of base KUnit module unloading to separate patches (Kamil),
> > - address the issue of the second paragraph of commit description
> > suggesting two separate changes combined in one patch (Kamil) by
> > rewording the description.
> >
> > Signed-off-by: Janusz Krzysztofik <janusz.krzysztofik at linux.intel.com>
> > Cc: Lucas De Marchi <lucas.demarchi at intel.com>
> > Cc: Kamil Konieczny <kamil.konieczny at linux.intel.com>
> > ---
> > lib/igt_kmod.c | 114 ++++++++++++++++++++++++++++++++++++-------------
> > 1 file changed, 84 insertions(+), 30 deletions(-)
> >
> > diff --git a/lib/igt_kmod.c b/lib/igt_kmod.c
> > index d2c28d0a64..1d57ea708d 100644
> > --- a/lib/igt_kmod.c
> > +++ b/lib/igt_kmod.c
> > @@ -811,6 +811,56 @@ static int open_parameters(const char *module_name)
> > return open(path, O_RDONLY);
> > }
> >
> > +static const char *kunit_params[3] = {
> > + "filter_glob", "filter", "filter_action",
> > +};
> > +
> > +static bool kunit_set_params(const char **values)
> > +{
> > + /*
> > + * Reapplying default NULLs to module parameters via sysfs seems not
> > + * possible, and applying empty strings doesn't work as expected,
> > + * leaving current values untouched. As a workaround, use
> > + * human-readable non-empty strings that exhibit the same behaviour
> > + * as if default NULLs were in place. In case of filter_action
> > + * parameter there is no such obvious non-empty string, however,
> > + * that's not a problem for us since even if it was previously set
> > + * to "skip" and we leave it as is, our "module!=none" default filter
> > + * guarantees that no test cases are filtered out to be skipped.
> > + */
> > + const char *defaults[ARRAY_SIZE(kunit_params)] = {
> > + /* filter_glob, filter, filter_action */
> > + "*", "module!=none", "",
>
> If writing empty string poses a problem, what about writing
> one space? Could it work?
The approach suggested by Lucas seems the best to me: don't write to
filter_action at all if we are not able to get the result we want (i.e., its
default NULL value restored) AND we can get what we want with its current
value "skip" in place when combined with the proposed value of filter
parameter "module!=none".
>
> One more nit below.
>
> > + };
> > + int i, params;
> > + bool ret;
> > +
> > + params = open_parameters("kunit");
> > + ret = !igt_debug_on(params < 0);
> > +
> > + for (i = 0; ret && i < ARRAY_SIZE(kunit_params); i++) {
> > + char *param = igt_sysfs_get(params, kunit_params[i]);
> > +
> > + if (igt_debug_on_f(!param, "param: %s\n", kunit_params[i])) {
> > + ret = false;
> > + break;
> > + }
> > +
> > + ret = !strcmp(param, values[i] ?: defaults[i]);
> > + free(param);
> > + if (!ret)
> > + ret = !igt_debug_on(!igt_sysfs_set(params,
> > + kunit_params[i],
> > + values[i] ?:
> > + defaults[i]));
> > + }
> > +
> > + if (params >= 0)
> > + close(params);
> > +
> > + return ret;
> > +}
> > +
> > struct modprobe_data {
> > struct kmod_module *kmod;
> > const char *opts;
> > @@ -1100,6 +1150,18 @@ static void kunit_get_tests(struct igt_list_head *tests,
> > const char *filter,
> > const char *opts)
> > {
> > + /*
> > + * 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 try to use action=list kunit parameter to get the
> > + * listing, however, parsing a KTAP report -- something that we already
> > + * can do perfectly -- seems to be more safe than extracting a test case
> > + * list of unknown length from /dev/kmsg.
> > + */
> > + const char *values[ARRAY_SIZE(kunit_params)] = {
> > + /* filter_glob, filter, filter_action */
> > + NULL, "module=none", "skip",
> > + };
> > char *suite_name = NULL, *case_name = NULL;
> > struct igt_ktap_result *r, *rn;
> > struct igt_ktap_results *ktap;
> > @@ -1113,27 +1175,21 @@ static void kunit_get_tests(struct igt_list_head *tests,
> >
> > 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 already can do
> > - * perfectly -- seems to be more safe than extracting a test case list
> > - * of unknown length from /dev/kmsg.
> > - *
> > - * TODO: drop the following workaround, which is required by LTS kernel
> > - * v6.1 not capable of listing test cases when built as a module.
> > - * If loading the kunit module with required parameters fails then
> > - * assume that we are running on a kernel with missing test case listing
> > - * capabilities. Dont's skip but just return with empty list of test
> > - * cases, that should tell the caller to use a legacy method of
> > - * iterating over KTAP results collected from blind execution of all
> > - * Kunit test cases provided by a Kunit test module.
> > - */
> > - igt_ignore_warn(igt_kmod_unload("kunit", KMOD_REMOVE_FORCE));
> > - if (igt_debug_on(igt_kmod_load("kunit",
> > - "filter=module=none filter_action=skip")))
> > + if (igt_debug_on(!kunit_set_params(values))) {
> > + /*
> > + * TODO: drop the following workaround, required by LTS kernel
> > + * v6.1 not capable of listing test cases when built as as
>
> s/as as/as/
OK, thanks.
Janusz
>
> Regards,
> Kamil
>
> > + * module, when no longer needed.
> > + * If updating writable filter parameters of the kunit base
> > + * module with required values then assume that we are running
> > + * on a kernel with missing test case listing capabilities.
> > + * Don't skip but just return with empty list of test cases,
> > + * which should tell the caller to use a legacy method of
> > + * iterating over KTAP results collected from blind execution
> > + * of all Kunit test cases provided by a Kunit test module.
> > + */
> > return;
> > + }
> >
> > igt_skip_on(modprobe(tst->kmod, opts));
> >
> > @@ -1200,16 +1256,11 @@ static void __igt_kunit(struct igt_ktest *tst,
> > t->case_name) {
> >
> > if (!modprobe.thread) {
> > - /*
> > - * Since we have successfully loaded the kunit
> > - * base module with non-default parameters in
> > - * order to get a list of test cases, now we
> > - * have to unload it so it is then automatically
> > - * reloaded with default parameter values when
> > - * we load the test module again for execution.
> > - */
> > - igt_skip_on(igt_kmod_unload("kunit",
> > - KMOD_REMOVE_FORCE));
> > + const char *values[ARRAY_SIZE(kunit_params)] = {
> > + NULL, NULL, NULL,
> > + };
> > +
> > + igt_require(kunit_set_params(values));
> >
> > igt_assert_eq(pthread_mutexattr_init(&attr), 0);
> > igt_assert_eq(pthread_mutexattr_setrobust(&attr,
> > @@ -1378,6 +1429,9 @@ void igt_kunit(const char *module_name, const char *name, const char *opts)
> > igt_assert(igt_list_empty(&tests));
> > }
> >
> > + /* We need the base KUnit module loaded if not built-in */
> > + igt_ignore_warn(igt_kmod_load("kunit", NULL));
> > +
> > /*
> > * We need to use igt_subtest here, as otherwise it may crash with:
> > * skipping is allowed only in fixtures, subtests or igt_simple_main
>
More information about the igt-dev
mailing list