From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from kanga.kvack.org (kanga.kvack.org [205.233.56.17]) by smtp.lore.kernel.org (Postfix) with ESMTP id AADBFC4332F for ; Mon, 21 Nov 2022 14:45:47 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 1A26C6B0071; Mon, 21 Nov 2022 09:45:47 -0500 (EST) Received: by kanga.kvack.org (Postfix, from userid 40) id 12D396B0073; Mon, 21 Nov 2022 09:45:47 -0500 (EST) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id EE7798E0001; Mon, 21 Nov 2022 09:45:46 -0500 (EST) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0016.hostedemail.com [216.40.44.16]) by kanga.kvack.org (Postfix) with ESMTP id DB02F6B0071 for ; Mon, 21 Nov 2022 09:45:46 -0500 (EST) Received: from smtpin04.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay10.hostedemail.com (Postfix) with ESMTP id 976CDC010E for ; Mon, 21 Nov 2022 14:45:46 +0000 (UTC) X-FDA: 80157723492.04.F008457 Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.129.124]) by imf04.hostedemail.com (Postfix) with ESMTP id 2FE6740008 for ; Mon, 21 Nov 2022 14:45:45 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1669041945; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: in-reply-to:in-reply-to:references:references; bh=grM+KS8TII8/rEOFyAQQEbedoSGBGFwtgLZp4306LrY=; b=BtCLunUq67dLMqr9cHItTqaziZZecm9l65fVKfOUQWpsgoaZvHeKzNnFfo+O1GlmLQkWnp QC3pt87rz16DAwj4FEFwn8piRLcyTwsxKTvTTZTlX2uoRp9bBl3r5MrYw7I3xxKlM7hU+C QryRrRuDjAUpDHsJcUvI1m2S6dr6Dqc= Received: from mail-qv1-f69.google.com (mail-qv1-f69.google.com [209.85.219.69]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_128_GCM_SHA256) id us-mta-447-wwEbbMprM36wDGHl60anEw-1; Mon, 21 Nov 2022 09:45:44 -0500 X-MC-Unique: wwEbbMprM36wDGHl60anEw-1 Received: by mail-qv1-f69.google.com with SMTP id ng1-20020a0562143bc100b004bb706b3a27so11673794qvb.20 for ; Mon, 21 Nov 2022 06:45:44 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=in-reply-to:content-disposition:mime-version:references:message-id :subject:cc:to:from:date:x-gm-message-state:from:to:cc:subject:date :message-id:reply-to; bh=grM+KS8TII8/rEOFyAQQEbedoSGBGFwtgLZp4306LrY=; b=ihp+QX6LxV09rxpfVG8xPvdjbLDz69IP/Hrg89qgpl5AbQg6P3ZGs9lwDzWQOTBMQr FFEissRZXNZim5d3CTCH4UPm+NkOW6o3G+uFJ128vaw8L9d/v1GR+zBrdiSQQ76s8wAm ppdKAkHVpMzCsGpV9NHkQRA+yMlOhJ/K23iyAqPCA/3v+oIaT/of8hGa10j5qE8kAwRd FXz93cxxdhkDf8XUTqM9BRxzkw7PMbQsw7zpNmRJuVYBxSMxtiL0hqu709Ie2ZiItOJd D16E7gbYu7p5zHUWeIJCyQGAv/TRAAGrOxYGdRMWJXyxEGYYRgVoDVtfY7BVL/NhcnRQ waoQ== X-Gm-Message-State: ANoB5pk6dyAHcf/qmQUnFgZOC4S+DGX0DYeuGTCElMouHD5Sg40bLf8O k2GR6HRXXgicHRYNJi7P1mqWqPpUzRUuxHEMg3byRZImkZG0e3lKnHzy2ciNT0p5Hx7A7qqyeWj 0ePzBKKRFVuk= X-Received: by 2002:ae9:ed58:0:b0:6ec:1753:e4e8 with SMTP id c85-20020ae9ed58000000b006ec1753e4e8mr3339315qkg.746.1669041943670; Mon, 21 Nov 2022 06:45:43 -0800 (PST) X-Google-Smtp-Source: AA0mqf4TR0H7Sh4y4BHAfcYvwEgHNQtqx70jm2P/DL0nqLeWXzsoVZHmml5ag3NYrHnoS2QuxnjFrg== X-Received: by 2002:ae9:ed58:0:b0:6ec:1753:e4e8 with SMTP id c85-20020ae9ed58000000b006ec1753e4e8mr3339283qkg.746.1669041943289; Mon, 21 Nov 2022 06:45:43 -0800 (PST) Received: from bfoster (c-24-61-119-116.hsd1.ma.comcast.net. [24.61.119.116]) by smtp.gmail.com with ESMTPSA id z4-20020ac86b84000000b003996aa171b9sm6613168qts.97.2022.11.21.06.45.42 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Mon, 21 Nov 2022 06:45:43 -0800 (PST) Date: Mon, 21 Nov 2022 09:45:49 -0500 From: Brian Foster To: Nhat Pham Cc: linux-mm@kvack.org, linux-kernel@vger.kernel.org, hannes@cmpxchg.org Subject: Re: [PATCH 3/4] cachestat: implement cachestat syscall Message-ID: References: <20221115182901.2755368-1-nphamcs@gmail.com> <20221115182901.2755368-4-nphamcs@gmail.com> MIME-Version: 1.0 In-Reply-To: <20221115182901.2755368-4-nphamcs@gmail.com> X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset=us-ascii Content-Disposition: inline ARC-Authentication-Results: i=1; imf04.hostedemail.com; dkim=pass header.d=redhat.com header.s=mimecast20190719 header.b=BtCLunUq; spf=pass (imf04.hostedemail.com: domain of bfoster@redhat.com designates 170.10.129.124 as permitted sender) smtp.mailfrom=bfoster@redhat.com; dmarc=pass (policy=none) header.from=redhat.com ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1669041946; a=rsa-sha256; cv=none; b=aAUG5wP5EYXewferGgK7YAvOKnxkk8RF1lZAYdx+pEg3rCNFq6LyT+LhOrtgINeDRxFw7d N5QLI1YQOtdyEqkfTF1v6BQVuDimxbv3QHoskLukRbSpOqtAg5ZG4E87CgXljHSHj3HDsE NGRFgUb1p6KU2OzZI0TEMsMZYgoYB+I= ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1669041946; h=from:from:sender:reply-to:subject:subject:date:date: message-id:message-id:to:to:cc:cc:mime-version:mime-version: content-type:content-type:content-transfer-encoding: in-reply-to:in-reply-to:references:references:dkim-signature; bh=grM+KS8TII8/rEOFyAQQEbedoSGBGFwtgLZp4306LrY=; b=hZUpOUT2sgQowkABNwsFdK2v67FL0+ifu8Wbpq531WjWODcMDIH16Tq6cp1xfYPZyUMzp3 xMgAjGg5eokZDP6JbQxa1bBqHt8iipjvTq31MMoVVdzG+rcd0WOALgMy1drBd40RCj7NK/ 5eN9orrfSKLs/SYsdKMPIW+s1r/ATWE= X-Stat-Signature: h8x1duyef1ygcbn4hxkz69omcmqwomxq X-Rspamd-Server: rspam08 X-Rspam-User: Authentication-Results: imf04.hostedemail.com; dkim=pass header.d=redhat.com header.s=mimecast20190719 header.b=BtCLunUq; spf=pass (imf04.hostedemail.com: domain of bfoster@redhat.com designates 170.10.129.124 as permitted sender) smtp.mailfrom=bfoster@redhat.com; dmarc=pass (policy=none) header.from=redhat.com X-Rspamd-Queue-Id: 2FE6740008 X-HE-Tag: 1669041945-184587 X-Bogosity: Ham, tests=bogofilter, spamicity=0.000000, version=1.2.4 Sender: owner-linux-mm@kvack.org Precedence: bulk X-Loop: owner-majordomo@kvack.org List-ID: On Tue, Nov 15, 2022 at 10:29:00AM -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 > > struct cachestat { > unsigned long nr_cache; > unsigned long nr_dirty; > unsigned long nr_writeback; > unsigned long nr_evicted; > unsigned long nr_recently_evicted; > }; > > int cachestat(unsigned int fd, off_t off, size_t len, > struct cachestat *cstat); > Do you have a strong use case for a user specified range vs. just checking the entire file? If not, have you considered whether it might be worth expanding statx() to include this data? That call is already designed to include "extended" file status and avoids the need for a new syscall. For example, the fields could be added individually with multiple flags, or the entire struct tied to a new STATX_CACHE flag or some such. That said, I'm not sure how sensitive folks might be to increasing the size of struct statx or populating it with cache data. Just a thought that it might be worth bringing up on linux-fsdevel if you haven't considered it already. A few random nits below.. > 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` argument must be a non-negative integers, If `off` + `len` > >= `off`, the queried range is [`off`, `off` + `len`]. Otherwise, we > will query in the range from `off` to the end of the file. > (off + len < off) is an error condition on some (most?) other syscalls. At least some calls (i.e. fadvise(), sync_file_range()) use len == 0 to explicitly specify "to EOF." > 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 `off` is negative. > > EBADF invalid file descriptor. > > Signed-off-by: Nhat Pham > --- > MAINTAINERS | 7 ++ > arch/x86/entry/syscalls/syscall_32.tbl | 1 + > arch/x86/entry/syscalls/syscall_64.tbl | 1 + > include/linux/syscalls.h | 2 + > include/uapi/asm-generic/unistd.h | 5 +- > include/uapi/linux/mman.h | 8 ++ > kernel/sys_ni.c | 1 + > mm/Makefile | 2 +- > mm/cachestat.c | 109 +++++++++++++++++++++++++ > 9 files changed, 134 insertions(+), 2 deletions(-) > create mode 100644 mm/cachestat.c > ... > diff --git a/mm/cachestat.c b/mm/cachestat.c > new file mode 100644 > index 000000000000..193151cb0767 > --- /dev/null > +++ b/mm/cachestat.c > @@ -0,0 +1,109 @@ > +// SPDX-License-Identifier: GPL-2.0-or-later > + > +/* > + * The cachestat() system call. > + */ > +#include > +#include > +#include > +#include > +#include > +#include > +#include > + > +#include "swap.h" > + > +/* > + * The cachestat(3) system call. > + * > + * cachestat() returns the page cache status 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. > + * > + * If `off` + `len` >= `off`, the queried range is [`off`, `off` + `len`]. > + * Otherwise, we will query in the range from `off` to the end of the file. > + * > + * 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_DEFINE4(cachestat, unsigned int, fd, off_t, off, size_t, len, > + struct cachestat __user *, cstat) > +{ > + struct fd f; > + struct cachestat cs; > + > + memset(&cs, 0, sizeof(struct cachestat)); Maybe move this after the next couple error checks to avoid unnecessary work? > + > + if (off < 0) > + return -EINVAL; > + > + if (!access_ok(cstat, sizeof(struct cachestat))) > + return -EFAULT; > + > + f = fdget(fd); Doing this: if (!f.file) return -EBADF; ... removes a level of indentation for the rest of the function, making it a bit easier to read (and removing the locals defined in the if branch, which I'm not sure is widely accepted style for the kernel). Brian > + if (f.file) { > + struct address_space *mapping = f.file->f_mapping; > + pgoff_t first_index = off >> PAGE_SHIFT; > + XA_STATE(xas, &mapping->i_pages, first_index); > + struct folio *folio; > + pgoff_t last_index = (off + len - 1) >> PAGE_SHIFT; > + > + if (last_index < first_index) > + last_index = ULONG_MAX; > + > + rcu_read_lock(); > + xas_for_each(&xas, folio, last_index) { > + if (xas_retry(&xas, folio) || !folio) > + continue; > + > + if (xa_is_value(folio)) { > + /* page is evicted */ > + void *shadow; > + bool workingset; /* not used */ > + > + cs.nr_evicted += 1; > + > + 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); > + } else { > + /* in page cache */ > + shadow = (void *) folio; > + } > + > + 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; > + } > + rcu_read_unlock(); > + fdput(f); > + > + if (copy_to_user(cstat, &cs, sizeof(struct cachestat))) > + return -EFAULT; > + > + return 0; > + } > + > + return -EBADF; > +} > -- > 2.30.2 > >