Feedback-ID: i56a14606:Fastmail
User-Agent: Cyrus-JMAP/3.9.0-alpha0-156-g081acc5ed5-fm-20230206.001-g081acc5e
Mime-Version: 1.0
Message-Id: <3f70f54e-206f-438e-9ef4-2cd514d3a690@app.fastmail.com>
In-Reply-To: <CAKEwX=MneVSm49FYxNAP+-uQijOZnNfWjuzF7FPub_rcDFY__A@mail.gmail.com>
References: <20230203190413.2559707-1-nphamcs@gmail.com>
 <20230203190413.2559707-3-nphamcs@gmail.com>
 <834254e6-a9c2-440c-9c67-fc5fa5ca43bc@app.fastmail.com>
 <CAKEwX=MneVSm49FYxNAP+-uQijOZnNfWjuzF7FPub_rcDFY__A@mail.gmail.com>
Date:   Mon, 13 Feb 2023 08:00:06 +0100
From:   "Arnd Bergmann" <arnd@arndb.de>
To:     "Nhat Pham" <nphamcs@gmail.com>
Cc:     "Andrew Morton" <akpm@linux-foundation.org>,
        "Johannes Weiner" <hannes@cmpxchg.org>,
        "Matthew Wilcox" <willy@infradead.org>, bfoster@redhat.com,
        kernel-team@meta.com, linux-api@vger.kernel.org,
        linux-kernel@vger.kernel.org, linux-mm@kvack.org
Subject: Re: [PATCH v9 2/3] cachestat: implement cachestat syscall
Content-Type: text/plain
Precedence: bulk

On Sun, Feb 12, 2023, at 07:58, Nhat Pham wrote:
> On Sun, Feb 5, 2023 at 4:56 PM Arnd Bergmann <arnd@arndb.de> wrote:
>> On Fri, Feb 3, 2023, at 20:04, Nhat Pham wrote:
>> 
>> > +SYSCALL_DEFINE5(cachestat, unsigned int, fd, loff_t, off, size_t, len,
>> > +             struct cachestat __user *, cstat, unsigned int, flags)
>> > +{
>> > +     return ksys_cachestat(fd, off, len, cstat, flags);
>> > +}
>> > +
>> > +#ifdef CONFIG_COMPAT
>> > +COMPAT_SYSCALL_DEFINE6(cachestat, unsigned int, fd, 
>> > compat_arg_u64_dual(off),
>> > +             size_t, len, struct cachestat __user *, cstat, unsigned int, flags)
>> > +{
>> > +     return ksys_cachestat(fd, compat_arg_u64_glue(off), len, cstat, 
>> > flags);
>> > +}
>> 
>> This still looks wrong to me, as this compat definition does not match
>> the native variant on architectures that require 64-bit arguments to
>> be passed in aligned register pairs, such as arm, mips or ppc, but
>> not x86, s390 or riscv.
>
> Oh I see - thanks for pointing that out! And the last bit means this
> is a non-issue for x86, s390 or riscv right? 

Right. It still requires the separate compat entry point that we
generally try to avoid for new syscalls, but it's probably still
better than passing the offset through a pointer.


> And iirc from the last thread, this is fixable via a simple reordering
> of the args in order to properly align the 64-bit arguments, for e.g:
>
> SYSCALL_DEFINE5(cachestat, loff_t, off, unsigned int, fd, size_t, len,
>               struct cachestat __user *, cstat, unsigned int, flags)
>
> ...
>
> COMPAT_SYSCALL_DEFINE6(cachestat, compat_arg_u64_dual(off), 
>              unsigned int, fd, size_t, len, struct cachestat __user *, cstat, 
>              unsigned int, flags)
>
>
> It looks a bit odd to me that fd is not the first argument, but perhaps this
> is an acceptable sacrifice to avoid unused arg and keep the flags...
>
> Let me know what you think about this!

Right, this should work. You can also move the offset to the third
or fifth argument in order to keep the fd one first.

I would actually like to change all the syscalls that have loff_t
arguments to have explicit '64-bit' and '32-bit' versions rather
than 'native' and 'compat', to make it more obvious what is going on
even on the architectures that have no 64-bit variant.
That is something to do later though, you should definitely keep
doing this the same way we do for all other syscalls.

    Arnd