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

[Peter Senna Tschudin <peter.senna@linux.intel.com>]

On 27.01.2025 17:38, Cavitt, Jonathan wrote:
> -----Original Message-----
> From: Peter Senna Tschudin <peter.senna@linux.intel.com> 
> Sent: Monday, January 27, 2025 7:28 AM
> To: igt-dev@lists.freedesktop.org
> Cc: Peter Senna Tschudin <peter.senna@linux.intel.com>; Rodrigo.Siqueira@amd.com; Gandi, Ramadevi <ramadevi.gandi@intel.com>; Knop, Ryszard <ryszard.knop@intel.com>; Lattannavar, Sameer <sameer.lattannavar@intel.com>; De Marchi, Lucas <lucas.demarchi@intel.com>; Saarinen, Jani <jani.saarinen@intel.com>; Piecielska, Katarzyna <katarzyna.piecielska@intel.com>; Roper, Matthew D <matthew.d.roper@intel.com>; Taylor, Clinton A <clinton.a.taylor@intel.com>; Yu, Jianshui <jianshui.yu@intel.com>; Vivekanandan, Balasubramani <balasubramani.vivekanandan@intel.com>; Cavitt, Jonathan <jonathan.cavitt@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@amd.com
>> CC: ramadevi.gandi@intel.com
>> CC: ryszard.knop@intel.com
>> CC: sameer.lattannavar@intel.com
>> CC: lucas.demarchi@intel.com
>> CC: jani.saarinen@intel.com
>> CC: katarzyna.piecielska@intel.com
>> CC: matthew.d.roper@intel.com
>> CC: clinton.a.taylor@intel.com
>> CC: jianshui.yu@intel.com
>> CC: balasubramani.vivekanandan@intel.com
>>
>> Reviewed-by: jonathan.cavitt@intel.com
> 
> My reviewed-by still stands, though I have a NIT/concern below:
> 
>> Signed-off-by: Peter Senna Tschudin <peter.senna@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...

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