[PATCH i-g-t v3 4/6] lib/kunit: Support writable filter* parameters of kunit module
Lucas De Marchi
lucas.demarchi at intel.com
Fri Feb 2 15:57:46 UTC 2024
On Fri, Feb 02, 2024 at 11:14:49AM +0100, Janusz Krzysztofik wrote:
>On Thursday, 1 February 2024 21:26:35 CET Lucas De Marchi wrote:
>> On Thu, Feb 01, 2024 at 07:59:20PM +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.
>> >
>> >v3: Don't read-compare-write, just write the values (Lucas),
>> > - skip calling igt_sysfs_set() when the string to be written to
>> > filter_action is empty (Lucas),
>> > - warn if we leave the filter_action set to "skip" while setting a non-
>> > default value of the filter parameter,
>> > - transform generic kunit_set_params() to kunit_set_filtering().
>> >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 | 91 +++++++++++++++++++++++++++++++++++---------------
>> > 1 file changed, 65 insertions(+), 26 deletions(-)
>> >
>> >diff --git a/lib/igt_kmod.c b/lib/igt_kmod.c
>> >index d2c28d0a64..83416d2aa4 100644
>> >--- a/lib/igt_kmod.c
>> >+++ b/lib/igt_kmod.c
>> >@@ -811,6 +811,49 @@ static int open_parameters(const char *module_name)
>> > return open(path, O_RDONLY);
>> > }
>> >
>> >+static bool kunit_set_filtering(const char *filter_glob, const char *filter,
>> >+ const char *filter_action)
>> >+{
>> >+ int params;
>> >+ bool ret;
>> >+
>> >+ params = open_parameters("kunit");
>> >+ ret = !igt_debug_on(params < 0);
>>
>> why proceed? just return early when it fails pre-conditions
>
>Right.
>
>> >+
>> >+ /*
>> >+ * Default values of the KUnit base module filtering parameters
>> >+ * are all NULLs. Reapplying those NULLs over sysfs once
>> >+ * overwritten with non-NULL strings seems not possible.
>> >+ * As a workaround, we use human-readable strings that exhibit
>> >+ * the same behaviour as if default NULLs were in place.
>> >+ */
>> >+ if (ret)
>> >+ ret = !igt_debug_on(!igt_sysfs_set(params, "filter_glob",
>> >+ filter_glob ?: "*"));
>> >+ if (ret)
>> >+ ret = !igt_debug_on(!igt_sysfs_set(params, "filter",
>> >+ filter ?: "module!=none"));
>> >+ /*
>> >+ * In case of filter_action parameter there is no obvious human-
>> >+ * readable string that behaves like default NULL. However, if NULL
>>
>> that is not true.. it's just igt_sysfs not doing the right thing to
>
>That depends on how we feel about empty strings, whether they are human-
>readable or not. For me they're not quite, e.g. when simply reading them back
>from sysfs it's not possible to distinguished them from potential white space,
>unless some graphical representation of non-printing characters is used.
>Maybe it would be more clear if kernel was using a convention to display empty
>strings as e.g. "(empty)", like it displays NULL as "(null)".
you can't print NULL. You can print empty like it's done now. That's not
going to change ever, as it would break perfectly usable and sane
userspace.
>
>> write an empty string, that does behave like NULL on the kernel side.
>
>Then maybe let's better send the empty string to igt_sysfs_set() instead of
>skipping that, and replace my "human-readable" with "non-NULL" in the
as long as the "human-readable" comment is removed, fine. Because empty
is empty, it's not a non-printing char. Let me try to make it clearer:
# echo -ne a > /sys/module/firmware_class/parameters/path
# hexdump -C /sys/module/firmware_class/parameters/path
00000000 61 0a |a.|
00000002
There are 2 chars: 'a' and '\n', with the latter always inserted by the
kernel.
# echo -ne "\0" > /sys/module/firmware_class/parameters/path
# hexdump -C /sys/module/firmware_class/parameters/path
00000000 0a |.|
00000001
there is 1 char, just the \n inserted by the kernel. Nobody would ever
ask for the kernel to start printing "(newline)" instead of \n.
Yes, the kernel side could behave differently depending if it's NULL
vs empty. kunit doesn't and shouldn't.
Lucas De Marchi
>comments. Thanks to "module!=none" we are still on the safe side, even before
>igt_sysfs_set() is fixed.
>
>> I'd drop these lines and just justify with the lines below why we are
>> not setting it here.
>>
>> with a small change below we can also move these to use igt_require()
>>
>> >+ * is requested for both filter and filter_action then that's not a
>> >+ * problem for us since even if filter_action was previously set to
>> >+ * "skip" we can leave it as is because our "module!=none" default
>> >+ * filter guarantees that no test cases are filtered out to be skipped.
>> >+ */
>> >+ if (ret)
>> >+ ret = !igt_warn_on_f(!filter_action && filter,
>> >+ "test error: filter_action=NULL requires filter=NULL\n");
>> >+ if (ret && filter_action)
>> >+ ret = !igt_debug_on(!igt_sysfs_set(params, "filter_action",
>> >+ filter_action));
>> >+
>> >+ if (params >= 0)
>> >+ close(params);
>> >+
>> >+ return ret;
>> >+}
>> >+
>> > struct modprobe_data {
>> > struct kmod_module *kmod;
>> > const char *opts;
>> >@@ -1116,24 +1159,26 @@ static void kunit_get_tests(struct igt_list_head *tests,
>> > /*
>> > * 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.
>> >+ * We could also try to use action=list kunit parameter to get the
>> >+ * listing, however, parsing a structured KTAP report -- something that
>> >+ * we already can do perfectly -- seems to be more safe than extracting
>> >+ * a free text list of unknown length from /dev/kmsg.
>> > */
>> >- 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_filtering(NULL, "module=none", "skip"))) {
>>
>> probably in the caller or very early:
>>
>> if (!igt_syfs_has_rw_attr())
>> return fallback_kunit_execution();
>>
>> I think it's too different this approach from the previous one, that
>> results in the long comments trying to explain each different
>> step.
>
>OK.
>
>>
>> Humn... I guess we can do that on top, so if this works, seems ok for
>> now.
>
>OK.
>
>Thanks,
>Janusz
>
>>
>> Lucas De Marchi
>>
>> >+ /*
>> >+ * TODO: drop the following workaround, required by LTS kernel
>> >+ * v6.1 not capable of listing test cases when built as as
>> >+ * module, when no longer needed.
>> >+ * If updating writable filter parameters of the kunit base
>> >+ * module with required values fails then assume 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 +1245,7 @@ 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));
>> >+ igt_require(kunit_set_filtering(NULL, NULL, NULL));
>> >
>> > igt_assert_eq(pthread_mutexattr_init(&attr), 0);
>> > igt_assert_eq(pthread_mutexattr_setrobust(&attr,
>> >@@ -1378,6 +1414,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