Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <20221208202808.908690-1-nphamcs@gmail.com> <20221208202808.908690-4-nphamcs@gmail.com>
 <Y5dNPYVNyjHgGCD6@casper.infradead.org>
In-Reply-To: <Y5dNPYVNyjHgGCD6@casper.infradead.org>
From:   Nhat Pham <nphamcs@gmail.com>
Date:   Thu, 15 Dec 2022 09:14:15 -0800
Message-ID: <CAKEwX=Pb8npL9p9bkeEnudp1FsLJ_HydabNDg9J8evD+qV9coQ@mail.gmail.com>
Subject: Re: [PATCH v3 3/4] cachestat: implement cachestat syscall
To:     Matthew Wilcox <willy@infradead.org>
Cc:     akpm@linux-foundation.org, hannes@cmpxchg.org, linux-mm@kvack.org,
        linux-kernel@vger.kernel.org, bfoster@redhat.com,
        kernel-team@meta.com
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Mon, Dec 12, 2022 at 7:48 AM Matthew Wilcox <willy@infradead.org> wrote:
>
> On Thu, Dec 08, 2022 at 12:28:07PM -0800, Nhat Pham wrote:
> > +             struct address_space *mapping = f.file->f_mapping;
> > +             pgoff_t first_index = off >> PAGE_SHIFT;
> > +             pgoff_t last_index =
> > +                     len == 0 ? ULONG_MAX : (off + len - 1) >> PAGE_SHIFT;
> > +             XA_STATE(xas, &mapping->i_pages, first_index);
> > +             struct folio *folio;
> > +
> > +             rcu_read_lock();
> > +             xas_for_each(&xas, folio, last_index) {
> > +                     if (xas_retry(&xas, folio) || !folio)
> > +                             continue;
>
> !folio can't be true.  xas_for_each() terminates when folio is NULL.

Good to know - I'll remove this in the next version.

>
> > +                     if (xa_is_value(folio)) {
> > +                             /* page is evicted */
> > +                             void *shadow = (void *)folio;
> > +                             bool workingset; /* not used */
> > +
> > +                             cs.nr_evicted += 1;
> > +
> > +#ifdef CONFIG_SWAP /* implies CONFIG_MMU */
> > +                             if (shmem_mapping(mapping)) {
> > +                                     /* shmem file - in swap cache */
> > +                                     swp_entry_t swp = radix_to_swp_entry(folio);
> > +
> > +                                     shadow = get_shadow_from_swap_cache(swp);
> > +                             }
> > +#endif
> > +                             if (workingset_test_recent(shadow, true, &workingset))
> > +                                     cs.nr_recently_evicted += 1;
> > +
> > +                             continue;
> > +                     }
> > +
> > +                     /* page is in cache */
> > +                     cs.nr_cache += 1;
> > +
> > +                     if (folio_test_dirty(folio))
> > +                             cs.nr_dirty += 1;
> > +
> > +                     if (folio_test_writeback(folio))
> > +                             cs.nr_writeback += 1;
>
> A folio may represent more than one page.  That's the point of folios.
> So there should be something in here which does
>
>         unsigned long nr = folio_nr_pages();
>
> and then all these '1' should be 'nr'.

Yikes, that's a bug that I missed! Thanks for pointing it out.

> Except that you may need to adjust nr if 'first_index' > folio->index, or
> 'last_index' < folio->index + nr.

Sounds good!

>
> You should test this with XFS, AFS or erofs to be sure you're getting
> results that look right.
>