Feedback-ID: i56a14606:Fastmail
User-Agent: Cyrus-JMAP/3.9.0-alpha0-85-gd6d859e0cf-fm-20230116.001-gd6d859e0
Mime-Version: 1.0
Message-Id: <12e9afc8-cec4-4fce-ad81-09790cbe3938@app.fastmail.com>
In-Reply-To: <Y9QqFJUFo1RAbIqP@cmpxchg.org>
References: <20230126175356.1582123-1-nphamcs@gmail.com>
 <20230126175356.1582123-3-nphamcs@gmail.com>
 <54c8ecbd-1d6e-40f1-af30-7efd04c63a7e@app.fastmail.com>
 <Y9QqFJUFo1RAbIqP@cmpxchg.org>
Date:   Fri, 27 Jan 2023 21:29:47 +0100
From:   "Arnd Bergmann" <arnd@arndb.de>
To:     "Johannes Weiner" <hannes@cmpxchg.org>
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
Content-Type: text/plain
Precedence: bulk

On Fri, Jan 27, 2023, at 20:46, Johannes Weiner wrote:
> On Fri, Jan 27, 2023 at 04:46:38PM +0100, Arnd Bergmann wrote:
>> 
>> - 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);

There is still a problem of incompatible calling conventions:
on architectures that require even/odd register pairs, this would
end up like

int cachestat(unsigned int, long unused, u32 off_low, u32 off_high,
              size_t len, struct cachestat *cstat);

A more portable way to do this would be to pass the offset by
reference, but that makes it a bit awkward in userspace.

Or the arguments could be rearranged to put the low/high argument
pair first/second, third/fourth or fifth/sixth argument, at least
on the kernel ABI to avoid having another situation like
sys_arm_fadvise64_64.

> and should we ever require extensions - new fields, flags - they would
> come through a new cachestat2().
>
> Would anybody have objections to that?

If there is room for another argument, I would keep the 'flags'
as a way for extending in a compatible way, that is pretty standard
now, just not flags plus version.

      Arnd