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: <20211014055527.1238645-1-alistair.francis@opensource.wdc.com> <20211014055527.1238645-2-alistair.francis@opensource.wdc.com>
In-Reply-To: <20211014055527.1238645-2-alistair.francis@opensource.wdc.com>
From:   Arnd Bergmann <arnd@arndb.de>
Date:   Thu, 14 Oct 2021 08:51:27 +0200
Message-ID: <CAK8P3a3jyJvghm0yZA=fjaZfdKamb=-wPcF2LTFaPEn-KGzhiw@mail.gmail.com>
Subject: Re: [PATCH 2/2] selftests: futex: Use a 64-bit time_t
To:     Alistair Francis <alistair.francis@opensource.wdc.com>
Cc:     linux-riscv <linux-riscv@lists.infradead.org>,
        linux-perf-users@vger.kernel.org,
        Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
        Alistair Francis <alistair23@gmail.com>,
        Namhyung Kim <namhyung@kernel.org>,
        Jiri Olsa <jolsa@redhat.com>,
        Alexander Shishkin <alexander.shishkin@linux.intel.com>,
        Mark Rutland <mark.rutland@arm.com>,
        Arnaldo Carvalho de Melo <acme@kernel.org>,
        Davidlohr Bueso <dave@stgolabs.net>,
        Darren Hart <dvhart@infradead.org>,
        Peter Zijlstra <peterz@infradead.org>,
        Ingo Molnar <mingo@redhat.com>,
        Thomas Gleixner <tglx@linutronix.de>,
        Atish Patra <atish.patra@wdc.com>,
        Arnd Bergmann <arnd@arndb.de>,
        Alistair Francis <alistair.francis@wdc.com>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Thu, Oct 14, 2021 at 7:55 AM Alistair Francis
<alistair.francis@opensource.wdc.com> wrote:
>
> From: Alistair Francis <alistair.francis@wdc.com>
>
> Convert the futex selftests to only use a 64-bit time_t. On 64-bit
> architectures this isn't a functional change. On 32-bit architectures
> we now only perform 64-bit time_t syscalls (__NR_futex_time64) and
> use a struct timespec64.
>
> This won't work on kernels before 5.1, but as perf is tied to the kernel
> that's ok.
>
> This allows the tests to run and pass on RISC-V 32-bit.
>
> Signed-off-by: Alistair Francis <alistair.francis@wdc.com>

This looks correct to me, two minor comments:

> +struct timespec64 {
> +       long long tv_sec;       /* seconds */
> +       long long tv_nsec;      /* nanoseconds */
> +};

This is a bit different from the normal timespec definition, which has to
use a '__kernel_long_t' tv_nsec for POSIX compliance. The difference
is harmless, because the bit layout has the lower 32 bits in the same
position, and the kernel zeroes the upper 32 bits on the syscall boundary.

I would just use

#define timespec64 __kernel_timespec

for the same effect, since that is what __kernel_timespec is meant for.

> +#if __BITS_PER_LONG == 64 || (defined(__x86_64__) && defined(__ILP32__))
> +# define futex(uaddr, op, val, timeout, uaddr2, val3, opflags) \
> +       syscall(__NR_futex, uaddr, op | opflags, val, timeout, uaddr2, val3)
> +#else
> +# define futex(uaddr, op, val, timeout, uaddr2, val3, opflags) \
> +       syscall(__NR_futex_time64, uaddr, op | opflags, val, timeout, uaddr2, val3)
> +#endif

The check for x32 user space looks correct here, but as I commented in
the other patch, I would keep it simple and use futex_time64() on all 32-bit
ABIs. There are approximately zero users of x32, and they don't benefit from
being able to run new perf binaries on pre-5.1 kernels when everyone else
can't.

        Arnd