[PATCH i-g-t v2 4/6] lib/kunit: Support writable filter* parameters of kunit module
Lucas De Marchi
lucas.demarchi at intel.com
Thu Feb 1 01:01:17 UTC 2024
On Wed, Jan 31, 2024 at 07:03:51PM +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", "",
>+ };
>+ 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]);
why do we read-modify-write? I'd rather just write the values. An
optimization by checking previous value belongs in the kernel, when it
makes an actual difference. Something like this (but with error
handling):
int dirfd = open("/sys/modules/kunit/parameters", O_RDONLY);
igt_sysfs_set(dirfd, "filter_glob", ...);
igt_sysfs_set(dirfd, "filter", ...);
igt_sysfs_set(dirfd, "filter_action", ...);
close(dirfd);
an abstraction could better be something like:
igt_sysfs_set_all("/sys/modules/kunit/parameters",
attr1, val1,
attr2, val2,
attr3, val2,
NULL);
>+
>+ 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]));
filter_action is "". How does this fix the issue of writing a 0-length
string? If we try to go back and forth between "skip" and "", this will
fail. A workaround would be to make defaults above to be something else
like "run", but we should really fix this in igt. I'm submitting a
patch for that.
Lucas de Marchi
>+ }
>+
>+ 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
>+ * 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
>--
>2.43.0
>
More information about the Intel-xe
mailing list