Re: [PATCH v2 1/2] selftests: cachestat: add tests for mmap

[Joshua Hahn <joshua.hahnjy@gmail.com>]

On Mon,  7 Jul 2025 20:55:56 +0530 Suresh K C <suresh.k.chandrappa@gmail.com> wrote:

> From: Suresh K C <suresh.k.chandrappa@gmail.com>
> 
> Add a test case to verify cachestat behavior with memory-mapped files
> using mmap(). This ensures that pages accessed via mmap are correctly
> accounted for in the page cache.
> 
> Tested on x86_64 with default kernel config

Hey Suresh,

Thanks for the second version with the updates, sorry that I missed the
first time you sent this patch.

[...snip...]

>  	if (fd < 0) {
> -		ksft_print_msg("Unable to create shmem file.\n");
> +		ksft_print_msg("Unable to create file.\n");

NIT: I saw that you change this in the second part of the patch. However, why
not just include it in this patch? I feel that it would be good practice
to keep the kerenl in a "correct" state, even in between patches belonging
to the same series. If someone were to just apply this patch but not the
next (however unlikely that is), then they will not see the description of
what file type they failed to create. Just my 2c, no need to change this if
you don't think this is important.

[...snip...]

> +	if (type == FILE_MMAP){
> +		char *map = mmap(NULL, filesize, PROT_READ | PROT_WRITE, MAP_SHARED, fd, 0);
> +		if (map == MAP_FAILED) {
> +			ksft_print_msg("mmap failed.\n");
> +			ret = false;
> +			goto close_fd;
> +		}
> +		for (int i = 0; i < filesize; i++) {
> +			map[i] = 'A';
> +		}
> +		map[filesize - 1] = 'X';

NIT: Likewise, I don't know if there is a good reason to include this, only to
remove it in the second patch. Perhaps it would be best to just remove it
in this patch, so you don't have to delete it later?

Please let me know what you think. Have a great day!
Joshua

Sent using hkml (https://github.com/sjp38/hackermail)