Date:   Fri, 27 Jan 2023 14:46:28 -0500
From:   Johannes Weiner <hannes@cmpxchg.org>
To:     Arnd Bergmann <arnd@arndb.de>
Cc:     Nhat Pham <nphamcs@gmail.com>,
        Andrew Morton <akpm@linux-foundation.org>, linux-mm@kvack.org,
        linux-kernel@vger.kernel.org, bfoster@redhat.com,
        Matthew Wilcox <willy@infradead.org>,
        linux-api@vger.kernel.org, kernel-team@meta.com
Subject: Re: [PATCH v8 2/3] cachestat: implement cachestat syscall
Message-ID: <Y9QqFJUFo1RAbIqP@cmpxchg.org>
References: <20230126175356.1582123-1-nphamcs@gmail.com>
 <20230126175356.1582123-3-nphamcs@gmail.com>
 <54c8ecbd-1d6e-40f1-af30-7efd04c63a7e@app.fastmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <54c8ecbd-1d6e-40f1-af30-7efd04c63a7e@app.fastmail.com>
Precedence: bulk

On Fri, Jan 27, 2023 at 04:46:38PM +0100, Arnd Bergmann wrote:
> On Thu, Jan 26, 2023, at 18:53, Nhat Pham wrote:
> >
> > 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,
> >           unsigned int cstat_version, struct cachestat *cstat,
> >           unsigned int flags);
> 
> Is this "off_t off" argument intentionally limited to the old
> 32-bit type on 32-bit architectures? Unfortunately I fear 
> there are no good options to pass an offset here:
> 
> - if you make it a 32-bit type, this breaks calling it from
>   normal userspace that defines off_t as a 64-bit type
> 
> - if you change it to a 64-bit loff_t, there are three
>   separate calling conventions for 64-bit, 32-bit with
>   aligned register pairs and other 32-bit, plus you
>   exceed the usual limit of six system call arguments

That's a good point, thanks for raising it, Arnd.

> A separate problem may be the cstat_version argument, usually
> we don't use interface versions but instead use a new
> system call number if something changes in an incompatible
> way.

I suppose from a userspace POV, a version argument vs calling a
separate syscall doesn't make much of a difference. So going with
loff_t and dropping cstat_version seems like a sensible way forward.

As an added bonus, versioning the syscall itself means the signature
can change in v2. This allows dropping the unused flags arg for now.

That would leave it at:

  int cachestat(unsigned int, loff_t, size_t len, struct cachestat *cstat);

and should we ever require extensions - new fields, flags - they would
come through a new cachestat2().

Would anybody have objections to that?