MIME-Version: 1.0
References: <20211126060024.3290177-1-alistair.francis@opensource.wdc.com> <20211126060024.3290177-3-alistair.francis@opensource.wdc.com>
In-Reply-To: <20211126060024.3290177-3-alistair.francis@opensource.wdc.com>
From:   Arnd Bergmann <arnd@arndb.de>
Date:   Fri, 26 Nov 2021 08:24:27 +0100
Message-ID: <CAK8P3a1sFSaMGhXkKJcLKgd68xSK3MMQuySXb62sWEacwzxt=g@mail.gmail.com>
Subject: Re: [PATCH v3 3/6] uapi: futex: Add a futex syscall
To:     Alistair Francis <alistair.francis@opensource.wdc.com>
Cc:     Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
        Thomas Gleixner <tglx@linutronix.de>,
        Arnd Bergmann <arnd@arndb.de>,
        Namhyung Kim <namhyung@kernel.org>,
        Peter Zijlstra <peterz@infradead.org>,
        Alistair Francis <alistair23@gmail.com>,
        Jiri Olsa <jolsa@redhat.com>,
        Davidlohr Bueso <dave@stgolabs.net>,
        Ingo Molnar <mingo@redhat.com>,
        Darren Hart <dvhart@infradead.org>,
        Arnaldo Carvalho de Melo <acme@kernel.org>,
        linux-perf-users@vger.kernel.org,
        Mark Rutland <mark.rutland@arm.com>,
        Alexander Shishkin <alexander.shishkin@linux.intel.com>,
        Alistair Francis <alistair.francis@wdc.com>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Fri, Nov 26, 2021 at 7:00 AM Alistair Francis
<alistair.francis@opensource.wdc.com> wrote:
>
> From: Alistair Francis <alistair.francis@wdc.com>
>
> This commit adds two futex syscall wrappers that are exposed to
> userspace.
>
> Neither the kernel or glibc currently expose a futex wrapper, so
> userspace is left performing raw syscalls. This has mostly been because
> the overloading of one of the arguments makes it impossible to provide a
> single type safe function.
>
> Until recently the single syscall has worked fine. With the introduction
> of a 64-bit time_t futex call on 32-bit architectures, this has become
> more complex. The logic of handling the two possible futex syscalls is
> complex and often implemented incorrectly.
>
> This patch adds two futux syscall functions that correctly handle the

s/futux/futex/

> time_t complexity for userspace.
>
> This idea is based on previous discussions:
> https://lore.kernel.org/lkml/CAK8P3a3x_EyCiPDpMK54y=Rtm-Wb08ym2TNiuAZgXhYrThcWTw@mail.gmail.com/
>
> Signed-off-by: Alistair Francis <alistair.francis@wdc.com>

The implementation looks correct to me, the calling conventions seem
"good enough"
to me, but I wouldn't object if someone else can suggest a better interface.

One minor detail:

> +/**
> + * futex_syscall_timeout() - __NR_futex/__NR_futex_time64 syscall wrapper
> + * @uaddr:  address of first futex
> + * @op:   futex op code
> + * @val:  typically expected value of uaddr, but varies by op
> + * @timeout:  an absolute struct timespec
> + * @uaddr2: address of second futex for some ops
> + * @val3: varies by op
> + */
> +static inline int
> +__kernel_futex_syscall_timeout(volatile uint32_t *uaddr, int op, uint32_t val,
> +                     struct timespec *timeout, volatile uint32_t *uaddr2, int val3)

The two function names do not match the respective documentation.

        Arnd