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");
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");if (!write_exactly(fd, filesize)) {
ksft_print_msg("Unable to write to shmem file.\n");
ksft_print_msg("Unable to write to file.\n");
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)