Re: [PATCH] selftests: cachestat: add tests for mmap

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

On Mon, 30 Jun 2025 23:38:03 +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

Hi Suresh,

Thank you for writing this patch! I'll let Nhat or Johannes comment more on the
patch, but just had a few thoughts. Before going into the code, I wanted to
note that it would be helpful in the future to note where this patch comes
from. I saw there were a few iterations of this before, so it would help
reviewers track what changed between the versions and what the motivation
for new versions are. 

[...snip...]

> -		ksft_print_msg("Unable to create shmem file.\n");
> +		ksft_print_msg("Unable to create file.\n");
>  		ret = false;
>  		goto out;

Maybe we don't want to lose information about this -- it would be helpful
to see why the test failed. It doesn't seem like there are any other
indicators that would let users know if it was shmem or mmap that failed, so
users would basically be guessing as to which of these two test failed.
(And the same feedback aplies to the next two print statements)

>  	}
>  
>  	if (ftruncate(fd, filesize)) {
> -		ksft_print_msg("Unable to truncate shmem file.\n");
> +		ksft_print_msg("Unable to truncate file.\n");
>  		ret = false;
>  		goto close_fd;
>  	}
>  
>  	if (!write_exactly(fd, filesize)) {
> -		ksft_print_msg("Unable to write to shmem file.\n");
> +		ksft_print_msg("Unable to write to file.\n");
>  		ret = false;
>  		goto close_fd;
>  	}

I'm curious if we need this part down below. It seems like we already call
write_exactly above, which should fill the file descriptor with random
things up to filesize. Maybe it makes more sense to have these two options
(write_exactly vs. directly modifying the contents) in a switch statement?
It seems a bit redundant to do both for FILE_MMAP.

> +	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';

I'm also curious what the point of having the last character be different
here. It doesn't seem like there is any validation code to check the contents
of the file, so it seems a bit redundant to me as well. 

Have a great day!
Joshua

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