[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 9:07 AM
To: Cavitt, Jonathan <jonathan.cavitt at intel.com>; igt-dev at lists.freedesktop.org
Cc: 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>
Subject: Re: [PATCH i-g-t v3 1/2] lib/igt_kmemleak: library to interact with kmemleak
> On 27.01.2025 17:38, Cavitt, Jonathan wrote:
> > -----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?
> 
> I am honestly not sure. Unit testing uses files that are likely to exists only in memory
> thanks to tmpfs. So calls to sync are unlikely to make any practical difference. The downside
> seems to be that there are blocks of dead code that will never be tested because of those
> 'false' flags.
> 
> It does not seem to be a big deal for the code as is now, but does not feel right looking
> forward. I have enough for a new revision:
> - I made a royal mess today trying to catch Patchwork's attention: last patch of v3 still
>   says v2 resend.
> - I will use the proper Reviewed-by tag with name and email
> - I will add the comma back after "results-path"
> - I will change unit testing to use both true *and* false for sync
> 
> Let me know if you want any other change. I will give time for others to review as well before
> sending v4...

That's sufficient from my end, but if I catch or think of anything in the interim, I'll let you know.
Thank you for your efforts.
-Jonathan Cavitt

> 
> > -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
> >>
> >>
> 
>