[PATCH i-g-t v3 1/2] lib/igt_kmemleak: library to interact with kmemleak

[Cavitt, Jonathan]

-----Original Message-----
From: Peter Senna Tschudin <peter.senna at linux.intel.com> 
Sent: Monday, January 27, 2025 7:28 AM
To: igt-dev at lists.freedesktop.org
Cc: Peter Senna Tschudin <peter.senna at linux.intel.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>; Taylor, Clinton A <clinton.a.taylor at intel.com>; Yu, Jianshui <jianshui.yu at intel.com>; Vivekanandan, Balasubramani <balasubramani.vivekanandan at intel.com>; Cavitt, Jonathan <jonathan.cavitt at intel.com>
Subject: [PATCH i-g-t v3 1/2] lib/igt_kmemleak: library to interact with kmemleak
> 
> Adds a simple library for interacting with kmemleak and add
> unit testing for the library. There are two modes intended to
> integrate with igt_runner:
> - once: collect kmemleaks after all test completed
> - each: collect kmemleaks after eachb test completes
> 
> To use the library include "igt_kmemleak.h", call
> igt_kmemleak_init(NULL) to check if kmemleak is enabled and finally
> call igt_kmemleak() to collect kmemleaks. igt_kmemleak() expect the
> following arguments:
>   - const char *last_test: Name of the last lest or NULL
>   - int resdirfd: file descriptor of the results directory
>   - bool kmemleak_each: Are we scanning once or scanning after
>     each test?
>   - bool sync: sync after each write?
> 
> 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: clinton.a.taylor at intel.com
> CC: jianshui.yu at intel.com
> CC: balasubramani.vivekanandan at intel.com
> 
> Reviewed-by: jonathan.cavitt at intel.com

My reviewed-by still stands, though I have a NIT/concern below:

> Signed-off-by: Peter Senna Tschudin <peter.senna at linux.intel.com>
> ---
>  lib/igt_kmemleak.c       | 274 +++++++++++++++++++++++++++++++++++++++
>  lib/igt_kmemleak.h       |  16 +++
>  lib/meson.build          |   1 +
>  lib/tests/igt_kmemleak.c | 267 ++++++++++++++++++++++++++++++++++++++
>  lib/tests/meson.build    |   1 +
>  5 files changed, 559 insertions(+)
>  create mode 100644 lib/igt_kmemleak.c
>  create mode 100644 lib/igt_kmemleak.h
>  create mode 100644 lib/tests/igt_kmemleak.c
> 

[...]

> +
> +	igt_subtest_group {
> +		igt_subtest("test_igt_kmemleak_once")
> +			igt_assert(igt_kmemleak(NULL, resdirfd, false, false));
> +
> +		igt_subtest("test_igt_kmemleak_each") {
> +			igt_assert(igt_kmemleak("test_name_1", resdirfd,
> +						true, false));
> +			igt_assert(igt_kmemleak("test_name_2", resdirfd,
> +						true, false));
> +			igt_assert(igt_kmemleak("test_name_3", resdirfd,
> +						true, false));

NIT:
IIRC, in the first revision of these tests, when igt_kmemleak_init was in charge of the sync
value, we were passing a value of "true" for sync.  Was that important to preserve, or is it
acceptable to pass false here as we do currently?
-Jonathan Cavitt

> +		}
> +		igt_fixture {
> +			close(resdirfd);
> +		}
> +	}
> +	igt_fixture
> +		unlinkat(resdirfd, KMEMLEAKRESFILENAME, 0);
> +}
> diff --git a/lib/tests/meson.build b/lib/tests/meson.build
> index 1ce19f63c..5c42408d5 100644
> --- a/lib/tests/meson.build
> +++ b/lib/tests/meson.build
> @@ -16,6 +16,7 @@ lib_tests = [
>          'igt_ktap_parser',
>  	'igt_list_only',
>  	'igt_invalid_subtest_name',
> +	'igt_kmemleak',
>  	'igt_nesting',
>  	'igt_no_exit',
>  	'igt_runnercomms_packets',
> -- 
> 2.34.1
> 
>