Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <20211015005923.2659140-1-alistair.francis@opensource.wdc.com>
 <CAK8P3a3GVcjPp+ANKQvgMaDLqN4xAhNv+3jSO5gUMRszsARtFA@mail.gmail.com> <CAKmqyKNZm+w=WhUXPRLs-UWCwgBpZzt=4DMne-FbfBe=UjSrKw@mail.gmail.com>
In-Reply-To: <CAKmqyKNZm+w=WhUXPRLs-UWCwgBpZzt=4DMne-FbfBe=UjSrKw@mail.gmail.com>
From:   Arnd Bergmann <arnd@arndb.de>
Date:   Thu, 21 Oct 2021 08:13:03 +0200
Message-ID: <CAK8P3a1CxFfHze6id1sQbQXV-x8DXkEdfqh51MwabzwhKAoTdQ@mail.gmail.com>
Subject: Re: [PATCH] uapi: futex: Add a futex syscall
To:     Alistair Francis <alistair23@gmail.com>
Cc:     Arnd Bergmann <arnd@arndb.de>,
        Alistair Francis <alistair.francis@opensource.wdc.com>,
        Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
        Alistair Francis <alistair.francis@wdc.com>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Mon, Oct 18, 2021 at 5:41 AM Alistair Francis <alistair23@gmail.com> wrote:

(sorry I missed this part of your reply earlier, just saw the new version of the
patch and wondered what had happened to my feedback)

> On Fri, Oct 15, 2021 at 6:24 PM Arnd Bergmann <arnd@arndb.de> wrote:
> > On Fri, Oct 15, 2021 at 2:59 AM Alistair Francis
> >
> > I think it would be nicer to use a transparent union like
> >
> > typedef union __attribute__((__transparent_inion__)) {
> >         struct timespec *timeout;
> >         u_int32_t nr_requeue;
> > } __futex_arg4;
> >
> > which would let us provide a single function for both variants.
> > The main downside is that this relies on a GCC extension, but I
> > don't expect that to be a problem since any code using this is
> > already nonportable.
>
> I don't love this. Relying on a GCC extension doesn't seem like a great idea.

I see that even glibc uses __transparent_union__, though they have
a fallback for incompatible compilers in there, which we could
duplicate if necessary. I would expect that the kernel headers already
have a number of gcc extensions in them elsewhere.

> As well as that we need to split the types out to check the timeout
> value, and I'm not sure of how we could do that. We could have a
> switch case on the futex op, but that seems likely to get out of sync.

This is a good point. It would be ok if we could guarantee that
no new futex operations are added in the future and all new work
is on futex2(), but we probably don't want to make that a hard
requirement.

Another option would be to add the new wrappers into the
existing uapi/linux/futex.h header instead of a new header.
This is also not great, but it would have the advantage of
having the full list of operations in the same file.

        Arnd