lkml.org 
[lkml]   [2022]   [Dec]   [22]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    From
    Date
    SubjectRe: [PATCH v4 3/4] cachestat: implement cachestat syscall
    On Tue, Dec 20, 2022 at 6:37 AM Brian Foster <bfoster@redhat.com> wrote:
    >
    > On Fri, Dec 16, 2022 at 11:21:48AM -0800, Nhat Pham wrote:
    > > Implement a new syscall that queries cache state of a file and
    > > summarizes the number of cached pages, number of dirty pages, number of
    > > pages marked for writeback, number of (recently) evicted pages, etc. in
    > > a given range.
    > >
    > > NAME
    > > cachestat - query the page cache status of a file.
    > >
    > > SYNOPSIS
    > > #include <sys/mman.h>
    > >
    > > struct cachestat {
    > > __u64 nr_cache;
    > > __u64 nr_dirty;
    > > __u64 nr_writeback;
    > > __u64 nr_evicted;
    > > __u64 nr_recently_evicted;
    > > };
    > >
    > > int cachestat(unsigned int fd, off_t off, size_t len,
    > > size_t cstat_size, struct cachestat *cstat,
    > > unsigned int flags);
    > >
    > > DESCRIPTION
    > > cachestat() queries the number of cached pages, number of dirty
    > > pages, number of pages marked for writeback, number of (recently)
    > > evicted pages, in the bytes range given by `off` and `len`.
    > >
    > > These values are returned in a cachestat struct, whose address is
    > > given by the `cstat` argument.
    > >
    > > The `off` and `len` arguments must be non-negative integers. If
    > > `len` > 0, the queried range is [`off`, `off` + `len`]. If `len` ==
    > > 0, we will query in the range from `off` to the end of the file.
    > >
    > > `cstat_size` allows users to obtain partial results. The syscall
    > > will copy the first `csstat_size` bytes to the specified userspace
    > > memory. `cstat_size` must be a non-negative value that is no larger
    > > than the current size of the cachestat struct.
    > >
    > > The `flags` argument is unused for now, but is included for future
    > > extensibility. User should pass 0 (i.e no flag specified).
    > >
    > > RETURN VALUE
    > > On success, cachestat returns 0. On error, -1 is returned, and errno
    > > is set to indicate the error.
    > >
    > > ERRORS
    > > EFAULT cstat points to an invalid address.
    > >
    > > EINVAL invalid `cstat_size` or `flags`
    > >
    > > EBADF invalid file descriptor.
    > >
    > > Signed-off-by: Nhat Pham <nphamcs@gmail.com>
    > > ---
    >
    > Hi Nhat,
    >
    > Thanks for the tweaks. FWIW, this by and large looks reasonable to me.

    Thanks for suggesting the tweaks! I tried refactoring it out and it
    did look cleaner to me (and one fewer file that I have to keep
    track of...)

    > Just a couple more random nitty things, if you happen to care about
    > them..
    >
    > > arch/alpha/kernel/syscalls/syscall.tbl | 1 +
    > > arch/arm/tools/syscall.tbl | 1 +
    > > arch/ia64/kernel/syscalls/syscall.tbl | 1 +
    > > arch/m68k/kernel/syscalls/syscall.tbl | 1 +
    > > arch/microblaze/kernel/syscalls/syscall.tbl | 1 +
    > > arch/parisc/kernel/syscalls/syscall.tbl | 1 +
    > > arch/powerpc/kernel/syscalls/syscall.tbl | 1 +
    > > arch/s390/kernel/syscalls/syscall.tbl | 1 +
    > > arch/sh/kernel/syscalls/syscall.tbl | 1 +
    > > arch/sparc/kernel/syscalls/syscall.tbl | 1 +
    > > arch/x86/entry/syscalls/syscall_32.tbl | 1 +
    > > arch/x86/entry/syscalls/syscall_64.tbl | 1 +
    > > arch/xtensa/kernel/syscalls/syscall.tbl | 1 +
    > > include/linux/fs.h | 3 +
    > > include/linux/syscalls.h | 3 +
    > > include/uapi/asm-generic/unistd.h | 5 +-
    > > include/uapi/linux/mman.h | 9 ++
    > > init/Kconfig | 10 ++
    > > kernel/sys_ni.c | 1 +
    > > mm/filemap.c | 137 ++++++++++++++++++++
    > > 20 files changed, 180 insertions(+), 1 deletion(-)
    > >
    > ...
    > > diff --git a/mm/filemap.c b/mm/filemap.c
    > > index 08341616ae7a..29ffb906caa4 100644
    > > --- a/mm/filemap.c
    > > +++ b/mm/filemap.c
    > ...
    > > @@ -3949,3 +3953,136 @@ bool filemap_release_folio(struct folio *folio, gfp_t gfp)
    > > return try_to_free_buffers(folio);
    > > }
    > > EXPORT_SYMBOL(filemap_release_folio);
    > > +
    > > +#ifdef CONFIG_CACHESTAT
    > > +/**
    > > + * filemap_cachestat() - compute the page cache statistics of a mapping
    > > + * @mapping: The mapping to compute the statistics for.
    > > + * @first_index: The starting page cache index.
    > > + * @last_index: The final page index (inclusive).
    > > + * @cs: the cachestat struct to write the result to.
    > > + *
    > > + * This will query the page cache statistics of a mapping in the
    > > + * page range of [first_index, last_index] (inclusive). THe statistics
    > > + * queried include: number of dirty pages, number of pages marked for
    > > + * writeback, and the number of (recently) evicted pages.
    > > + */
    > > +void filemap_cachestat(struct address_space *mapping, pgoff_t first_index,
    > > + pgoff_t last_index, struct cachestat *cs)
    > > +{
    >
    > Not sure why the internal helper needs to be wrapped in a config option?
    > Perhaps it makes more sense to define CONFIG_CACHESTAT_SYSCALL and just
    > wrap the syscall bits with that..?

    No particularly strong reasons - I was just bundling the two together
    because I was not entirely sure if it's interesting or has any use
    case outside of the syscall itself.

    Do you have something in mind?

    >
    > I would also think it might make things simpler to split out all the
    > syscall bits into a separate patch from filemap_cachestat().

    Same as above.

    >
    > > + XA_STATE(xas, &mapping->i_pages, first_index);
    > > + struct folio *folio;
    > > +
    > > + rcu_read_lock();
    > > + xas_for_each(&xas, folio, last_index) {
    > > + unsigned long nr_pages;
    > > + pgoff_t folio_first_index, folio_last_index;
    > > +
    > > + if (xas_retry(&xas, folio))
    > > + continue;
    > > +
    > > + nr_pages = folio_nr_pages(folio);
    > > + folio_first_index = folio_pgoff(folio);
    > > + folio_last_index = folio_first_index + nr_pages - 1;
    > > +
    > > + /* Folios might straddle the range boundaries, only count covered subpages */
    > > + if (folio_first_index < first_index)
    > > + nr_pages -= first_index - folio_first_index;
    > > +
    > > + if (folio_last_index > last_index)
    > > + nr_pages -= folio_last_index - last_index;
    > > +
    > > + if (xa_is_value(folio)) {
    > > + /* page is evicted */
    > > + void *shadow = (void *)folio;
    > > + bool workingset; /* not used */
    > > +
    > > + cs->nr_evicted += nr_pages;
    > > +
    > > +#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 += nr_pages;
    > > +
    > > + continue;
    > > + }
    > > +
    > > + /* page is in cache */
    > > + cs->nr_cache += nr_pages;
    > > +
    > > + if (folio_test_dirty(folio))
    > > + cs->nr_dirty += nr_pages;
    > > +
    > > + if (folio_test_writeback(folio))
    > > + cs->nr_writeback += nr_pages;
    >
    > I'm not sure this is an issue right now (or if it will ever be for your
    > use cases), but from a filesystem perspective it is possible to have
    > large or variable sized folios in cache. At the moment I think the fs
    > (or XFS+iomap at least) manages dirty/writeback state at folio
    > granularity, but that may change in the near future if/when iomap
    > sub-page dirty tracking comes along. I suspect that means it may become
    > possible to have a large folio of some N number of pages where only a
    > subset of those pages are actually in dirty/writeback state, and thus
    > introduces some inaccuracy here because this assumes that folio state
    > applies to folio_nr_pages() worth of pages. Just something to be aware
    > of..
    >
    > Brian

    Oof, I coded this with the mental framework of folio-as-a-unit, and
    assumed that the dirty/writeback state is managed at the granularity of folio.
    Thanks for bringing this up Brian! I'll have to watch out for this as iomap
    evolves (and the subpage tracking becomes a thing).

    >
    > > + }
    > > + rcu_read_unlock();
    > > +}
    > > +EXPORT_SYMBOL(filemap_cachestat);
    > > +
    > > +/*
    > > + * The cachestat(5) system call.
    > > + *
    > > + * cachestat() returns the page cache statistics of a file in the
    > > + * bytes specified by `off` and `len`: number of cached pages,
    > > + * number of dirty pages, number of pages marked for writeback,
    > > + * number of (recently) evicted pages.
    > > + *
    > > + * `off` and `len` must be non-negative integers. If `len` > 0,
    > > + * the queried range is [`off`, `off` + `len`]. If `len` == 0,
    > > + * we will query in the range from `off` to the end of the file.
    > > + *
    > > + * `cstat_size` allows users to obtain partial results. The syscall
    > > + * will copy the first `csstat_size` bytes to the specified userspace
    > > + * memory. It also makes the cachestat struct extensible - new fields
    > > + * can be added in the future without breaking existing usage.
    > > + * `cstat_size` must be a non-negative value that is no larger than
    > > + * the current size of the cachestat struct.
    > > + *
    > > + * The `flags` argument is unused for now, but is included for future
    > > + * extensibility. User should pass 0 (i.e no flag specified).
    > > + *
    > > + * Because the status of a page can change after cachestat() checks it
    > > + * but before it returns to the application, the returned values may
    > > + * contain stale information.
    > > + *
    > > + * return values:
    > > + * zero - success
    > > + * -EFAULT - cstat points to an illegal address
    > > + * -EINVAL - invalid arguments
    > > + * -EBADF - invalid file descriptor
    > > + */
    > > +SYSCALL_DEFINE6(cachestat, unsigned int, fd, off_t, off, size_t, len,
    > > + size_t, cstat_size, struct cachestat __user *, cstat,
    > > + unsigned int, flags)
    > > +{
    > > + struct fd f = fdget(fd);
    > > + struct address_space *mapping;
    > > + struct cachestat cs;
    > > + pgoff_t first_index = off >> PAGE_SHIFT;
    > > + pgoff_t last_index =
    > > + len == 0 ? ULONG_MAX : (off + len - 1) >> PAGE_SHIFT;
    > > +
    > > + if (off < 0 || cstat_size > sizeof(struct cachestat) || flags != 0)
    > > + return -EINVAL;
    > > +
    > > + if (!f.file)
    > > + return -EBADF;
    > > +
    > > + memset(&cs, 0, sizeof(struct cachestat));
    > > + mapping = f.file->f_mapping;
    > > + filemap_cachestat(mapping, first_index, last_index, &cs);
    > > + fdput(f);
    > > +
    > > + if (copy_to_user(cstat, &cs, cstat_size))
    > > + return -EFAULT;
    > > +
    > > + return 0;
    > > +}
    > > +#endif /* CONFIG_CACHESTAT */
    > > --
    > > 2.30.2
    > >
    >

    \
     
     \ /
      Last update: 2023-03-26 23:18    [W:5.043 / U:0.012 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site