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

[Suresh Chandrappa <suresh.k.chandrappa@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 3028 bytes --]

Hi Joshua,

Thanks for the feedback! In the first patch, both shmem and mmap operations
are present, but I hadn’t introduced any logic to distinguish between them
yet. That distinction is added in the second patch through a new API.

> +	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 initially included the first version of the patch mainly to gather early
feedback and ensure all review comments were addressed. Based on the input,
I plan to consolidate the changes into a single patch that reflects the
final version.

Thanks,

Suresh K C

On Tue, Jul 8, 2025 at 2:56 AM Joshua Hahn <joshua.hahnjy@gmail.com> wrote:

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

[-- Attachment #2: Type: text/html, Size: 4734 bytes --]