[PATCH i-g-t 2/2] runner/executor: Integrate igt_kmemleak scans
Cavitt, Jonathan
jonathan.cavitt at intel.com
Wed Jan 22 18:18:10 UTC 2025
-----Original Message-----
From: Peter Senna Tschudin <peter.senna at linux.intel.com>
Sent: Tuesday, January 21, 2025 3:30 AM
To: igt-dev at lists.freedesktop.org
Cc: Peter Senna Tschudin <peter.senna at linux.intel.com>; stylon.wang at amd.com; Rodrigo.Siqueira at amd.com; Gandi, Ramadevi <ramadevi.gandi at intel.com>; Knop, Ryszard <ryszard.knop at intel.com>; Lattannavar, Sameer <sameer.lattannavar at intel.com>; De Marchi, Lucas <lucas.demarchi at intel.com>; Saarinen, Jani <jani.saarinen at intel.com>; Piecielska, Katarzyna <katarzyna.piecielska at intel.com>; Roper, Matthew D <matthew.d.roper at intel.com>; gregory.f.germano at intel.com; Taylor, Clinton A <clinton.a.taylor at intel.com>; Vivekanandan, Balasubramani <balasubramani.vivekanandan at intel.com>; Yu, Jianshui <jianshui.yu at intel.com>; Cavitt, Jonathan <jonathan.cavitt at intel.com>
Subject: [PATCH i-g-t 2/2] runner/executor: Integrate igt_kmemleak scans
>
> Modifies igt_runner to include calls to igt_kmemleak(). Kmemleak
> scanning is disabled by default, so add command line options to
> igt_runner to enable kmemleak scanning: -k, -k<option>, --kmemleak,
> --kmemleak=<option>. The options are:
> - once: run one kmemleak scan after all tests from the test-list
> complete.
> - each: run one kmemleak scan after each test completes.
>
> If kmemleaks are found they will be saved to the igt_runner results
> directory in a file named kmemleak.txt.
>
> Updates serialize_settings() and read_settings_from_file() to save
> and restore igt_runner settings to and from disk. This is used when
> calling igt_runner with '--dry-run' and then by calling igt_resume
> instead of igt_runner.
>
> Updates unit testing for igt_runner to test that:
> - Kmemleak scans are disabled by default
> - Kmemleak scans be enabled by command line arguments
> - The choice about kmemleaks being enabled or not is saved to disk
> and restored from disk
>
> CC: stylon.wang at amd.com
> CC: Rodrigo.Siqueira at amd.com>
> CC: ramadevi.gandi at intel.com
> CC: ryszard.knop at intel.com
> CC: sameer.lattannavar at intel.com
> CC: lucas.demarchi at intel.com
> CC: jani.saarinen at intel.com
> CC: katarzyna.piecielska at intel.com
> CC: matthew.d.roper at intel.com
> CC: gregory.f.germano at intel.com
> CC: clinton.a.taylor at intel.com
> CC: balasubramani.vivekanandan at intel.com
> CC: jianshui.yu at intel.com
> CC: jonathan.cavitt at intel.com
> Signed-off-by: Peter Senna Tschudin <peter.senna at linux.intel.com>
I have some concerns regarding some seemingly old tags getting reintroduced,
as well as some refactoring that has occurred, but otherwise:
Reviewed-by: Jonathan Cavitt <jonathan.cavitt at intel.com>
> ---
> runner/executor.c | 23 +++++++++++++++++++++--
> runner/runner_tests.c | 18 ++++++++++++++++--
> runner/settings.c | 31 ++++++++++++++++++++++++++++++-
> runner/settings.h | 2 ++
> 4 files changed, 69 insertions(+), 5 deletions(-)
>
> diff --git a/runner/executor.c b/runner/executor.c
> index 999e7f719..11c97639e 100644
> --- a/runner/executor.c
> +++ b/runner/executor.c
> @@ -31,6 +31,7 @@
> #include "igt_aux.h"
> #include "igt_core.h"
> #include "igt_facts.h"
> +#include "igt_kmemleak.h"
> #include "igt_taints.h"
> #include "igt_vec.h"
> #include "executor.h"
> @@ -2370,6 +2371,13 @@ bool execute(struct execute_state *state,
> if (settings->facts)
> igt_facts_lists_init();
>
> + if (settings->kmemleak)
> + if (!igt_kmemleak_init(NULL, settings->sync)) {
> + errf("Failed to initialize kmemleak. Is kernel support enabled?\n");
> + errf("Disabling kmemleak on igt_runner and continuing...\n");
> + settings->kmemleak = settings->kmemleak_each = false;
> + }
> +
> if (state->next >= job_list->size) {
> outf("All tests already executed.\n");
> return true;
> @@ -2497,10 +2505,17 @@ bool execute(struct execute_state *state,
> bool already_written = false;
>
> /* Collect facts before running each test */
> - if (settings->facts) {
> + if (settings->facts)
> igt_facts(last_test);
> +
> + if (settings->kmemleak_each)
> + if (!igt_kmemleak(last_test, resdirfd,
> + settings->kmemleak_each))
> + errf("Failed to collect kmemleak logs after %s\n",
> + last_test);
> +
> + if (settings->facts || settings->kmemleak_each)
> last_test = entry_display_name(&job_list->entries[state->next]);
> - }
>
> if (should_die_because_signal(sigfd)) {
> status = false;
> @@ -2595,6 +2610,10 @@ bool execute(struct execute_state *state,
> if (settings->facts)
> igt_facts(last_test);
>
> + if (settings->kmemleak)
> + if (!igt_kmemleak(last_test, resdirfd, settings->kmemleak_each))
> + errf("Failed to collect kmemleak logs after the last test\n");
> +
> if ((timefd = openat(resdirfd, "endtime.txt", O_CREAT | O_WRONLY | O_EXCL, 0666)) >= 0) {
> dprintf(timefd, "%f\n", timeofday_double());
> close(timefd);
> diff --git a/runner/runner_tests.c b/runner/runner_tests.c
> index 8441763f2..a072a92c7 100644
> --- a/runner/runner_tests.c
> +++ b/runner/runner_tests.c
> @@ -191,6 +191,7 @@ static void assert_settings_equal(struct settings *one, struct settings *two)
> igt_assert_eq(one->dry_run, two->dry_run);
> igt_assert_eq(one->allow_non_root, two->allow_non_root);
> igt_assert_eq(one->facts, two->facts);
> + igt_assert_eq(one->kmemleak, two->kmemleak);
> igt_assert_eq(one->sync, two->sync);
> igt_assert_eq(one->log_level, two->log_level);
> igt_assert_eq(one->overwrite, two->overwrite);
> @@ -304,6 +305,7 @@ igt_main
> igt_assert(igt_list_empty(&settings->env_vars));
> igt_assert(!igt_vec_length(&settings->hook_strs));
> igt_assert(!settings->facts);
> + igt_assert(!settings->kmemleak);
> igt_assert(!settings->sync);
> igt_assert_eq(settings->log_level, LOG_LEVEL_NORMAL);
> igt_assert(!settings->overwrite);
> @@ -426,6 +428,7 @@ igt_main
> igt_assert_eq(settings->include_regexes.size, 0);
> igt_assert_eq(settings->exclude_regexes.size, 0);
> igt_assert(!settings->facts);
> + igt_assert(!settings->kmemleak);
> igt_assert(!settings->sync);
> igt_assert_eq(settings->log_level, LOG_LEVEL_NORMAL);
> igt_assert(!settings->overwrite);
> @@ -464,6 +467,7 @@ igt_main
> "-b", blacklist_name,
> "--blacklist", blacklist2_name,
> "-f",
> + "-k",
> "-s",
> "-l", "verbose",
> "--overwrite",
> @@ -523,6 +527,7 @@ igt_main
> igt_assert_eqstr(*((char **)igt_vec_elem(&settings->hook_strs, 1)), "echo world");
>
> igt_assert(settings->facts);
> + igt_assert(settings->kmemleak);
> igt_assert(settings->sync);
> igt_assert_eq(settings->log_level, LOG_LEVEL_VERBOSE);
> igt_assert(settings->overwrite);
> @@ -718,30 +723,39 @@ igt_main
> igt_subtest("parse-clears-old-data") {
> const char *argv[] = { "runner",
> "-n", "foo",
> + "--overwrite",
NIT:
What does overwrite do here? Is its addition related to integrating igt_kmemleak scans?
If we're adding it in just because it was missing, then that should probably be done
as a separate patch series.
Lower down, we add "foo", "test-root-dir", and "result-path" to the argv list, and I have
similar comments with respect to those as well. Same with the igt_asserts on
settings->overwrite.
> "--dry-run",
> "--allow-non-root",
> "test-root-dir",
> - "results-path",
> + "results-path"
NIT:
Removing the comma from the last element of the list is unnecessary, as many
of the lists in IGT leave the comma there.
There's definitely a discussion to be had as to whether or not this is proper
coding style, and if it's not, there's going to be a lot of refactoring work for us
in the future. But irrespective of what that discussion results in, refactoring
work like this is probably out of scope for this patch series. Especially since
"overwrite" isn't being appended to the end of the list (and maybe shouldn't
be added at all? See first NIT).
-Jonathan Cavitt
> };
>
> igt_assert(parse_options(ARRAY_SIZE(argv), (char**)argv, settings));
>
> igt_assert_eqstr(settings->name, "foo");
> + igt_assert(settings->overwrite);
> igt_assert(settings->dry_run);
> igt_assert(!settings->test_list);
> igt_assert(!settings->facts);
> + igt_assert(!settings->kmemleak);
> igt_assert(!settings->sync);
>
> argv[1] = "--test-list";
> + argv[2] = "foo"; /* Unchanged */
> argv[3] = "--facts";
> - argv[4] = "--sync";
> + argv[4] = "--kmemleak";
> + argv[5] = "--sync";
> + argv[6] = "test-root-dir"; /* Unchanged */
> + argv[7] = "results-path"; /* Unchanged */
>
> igt_assert(parse_options(ARRAY_SIZE(argv), (char**)argv, settings));
>
> igt_assert_eqstr(settings->name, "results-path");
> igt_assert(!settings->dry_run);
> + igt_assert(!settings->overwrite);
> igt_assert(strstr(settings->test_list, "foo") != NULL);
> igt_assert(settings->facts);
> + igt_assert(settings->kmemleak);
> igt_assert(settings->sync);
> }
>
> diff --git a/runner/settings.c b/runner/settings.c
> index 92fd42ea6..560bc2b5e 100644
> --- a/runner/settings.c
> +++ b/runner/settings.c
> @@ -41,6 +41,7 @@ enum {
> OPT_EXCLUDE = 'x',
> OPT_ENVIRONMENT = 'e',
> OPT_FACTS = 'f',
> + OPT_KMEMLEAK = 'k',
> OPT_SYNC = 's',
> OPT_LOG_LEVEL = 'l',
> OPT_OVERWRITE = 'o',
> @@ -232,6 +233,16 @@ static const char *usage_str =
> " not respond to ping.\n"
> " all - abort for all of the above.\n"
> " -f, --facts Enable facts tracking\n"
> + " -k, -k<option>, --kmemleak, --kmemleak=<option>\n"
> + " Enable kmemleak tracking. Each kmemleak scan\n"
> + " can take from 5 to 60 seconds, slowing down\n"
> + " the run considerably. The default is to scan\n"
> + " only once after the last test. It is also\n"
> + " possible to scan after each test. Possible\n"
> + " options:\n"
> + " once - The default is to run one kmemleak\n"
> + " scan after the last test\n"
> + " each - Run one kmemleak scan after each test\n"
> " -s, --sync Sync results to disk after every test\n"
> " -l {quiet,verbose,dummy}, --log-level {quiet,verbose,dummy}\n"
> " Set the logger verbosity level\n"
> @@ -668,6 +679,7 @@ bool parse_options(int argc, char **argv,
> {"abort-on-monitored-error", optional_argument, NULL, OPT_ABORT_ON_ERROR},
> {"disk-usage-limit", required_argument, NULL, OPT_DISK_USAGE_LIMIT},
> {"facts", no_argument, NULL, OPT_FACTS},
> + {"kmemleak", optional_argument, NULL, OPT_KMEMLEAK},
> {"sync", no_argument, NULL, OPT_SYNC},
> {"log-level", required_argument, NULL, OPT_LOG_LEVEL},
> {"test-list", required_argument, NULL, OPT_TEST_LIST},
> @@ -698,7 +710,7 @@ bool parse_options(int argc, char **argv,
> settings->dmesg_warn_level = -1;
> settings->prune_mode = -1;
>
> - while ((c = getopt_long(argc, argv, "hn:dt:x:e:fsl:omb:L",
> + while ((c = getopt_long(argc, argv, "hn:dt:x:e:fsl:omb:Lk::",
> long_options, NULL)) != -1) {
> switch (c) {
> case OPT_VERSION:
> @@ -742,6 +754,19 @@ bool parse_options(int argc, char **argv,
> case OPT_FACTS:
> settings->facts = true;
> break;
> + case OPT_KMEMLEAK:
> + settings->kmemleak = true;
> + if (optarg) {
> + if (strcmp(optarg, "once") == 0)
> + settings->kmemleak_each = false;
> + else if (strcmp(optarg, "each") == 0)
> + settings->kmemleak_each = true;
> + else {
> + usage(stderr, "Invalid kmemleak option");
> + goto error;
> + }
> + }
> + break;
> case OPT_SYNC:
> settings->sync = true;
> break;
> @@ -1105,6 +1130,8 @@ bool serialize_settings(struct settings *settings)
> SERIALIZE_LINE(f, settings, dry_run, "%d");
> SERIALIZE_LINE(f, settings, allow_non_root, "%d");
> SERIALIZE_LINE(f, settings, facts, "%d");
> + SERIALIZE_LINE(f, settings, kmemleak, "%d");
> + SERIALIZE_LINE(f, settings, kmemleak_each, "%d");
> SERIALIZE_LINE(f, settings, sync, "%d");
> SERIALIZE_LINE(f, settings, log_level, "%d");
> SERIALIZE_LINE(f, settings, overwrite, "%d");
> @@ -1176,6 +1203,8 @@ bool read_settings_from_file(struct settings *settings, FILE *f)
> PARSE_LINE(settings, name, val, dry_run, numval);
> PARSE_LINE(settings, name, val, allow_non_root, numval);
> PARSE_LINE(settings, name, val, facts, numval);
> + PARSE_LINE(settings, name, val, kmemleak, numval);
> + PARSE_LINE(settings, name, val, kmemleak_each, numval);
> PARSE_LINE(settings, name, val, sync, numval);
> PARSE_LINE(settings, name, val, log_level, numval);
> PARSE_LINE(settings, name, val, overwrite, numval);
> diff --git a/runner/settings.h b/runner/settings.h
> index f69f09778..ab00b3e32 100644
> --- a/runner/settings.h
> +++ b/runner/settings.h
> @@ -58,6 +58,8 @@ struct settings {
> struct igt_list_head env_vars;
> struct igt_vec hook_strs;
> bool facts;
> + bool kmemleak;
> + bool kmemleak_each;
> bool sync;
> int log_level;
> bool overwrite;
> --
> 2.34.1
>
>
More information about the igt-dev
mailing list