| Message ID | 20210917061040.2270822-2-alistair.francis@opensource.wdc.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v3,1/2] perf benchmark: Call the futex syscall from a function | expand |
On Fri, Sep 17, 2021 at 8:10 AM Alistair Francis <alistair.francis@opensource.wdc.com> wrote: > > From: Alistair Francis <alistair.francis@wdc.com> > > Some 32-bit architectures (such are 32-bit RISC-V) only have a 64-bit > time_t and as such don't have the SYS_futex syscall. This patch will > allow us to use the SYS_futex_time64 syscall on those platforms. > > Signed-off-by: Alistair Francis <alistair.francis@wdc.com> Thanks for the follow-up! Reviewed-by: Arnd Bergmann <arnd@arndb.de>
On Fri, 17 Sep 2021, Alistair Francis wrote: >From: Alistair Francis <alistair.francis@wdc.com> > >Some 32-bit architectures (such are 32-bit RISC-V) only have a 64-bit >time_t and as such don't have the SYS_futex syscall. This patch will >allow us to use the SYS_futex_time64 syscall on those platforms. Not objecting, but this ifdefiry will hurt my eyes every time I have to look at this file :) Acked-by: Davidlohr Bueso <dbueso@suse.de>
Hi Alistair, Às 03:10 de 17/09/21, Alistair Francis escreveu: > From: Alistair Francis <alistair.francis@wdc.com> > > Some 32-bit architectures (such are 32-bit RISC-V) only have a 64-bit > time_t and as such don't have the SYS_futex syscall. This patch will > allow us to use the SYS_futex_time64 syscall on those platforms. > Thanks for your patch! However, I don't think that any futex operation at perf has timeout. Do you plan to implement a test that use it? Or the idea is to get this ready for it in case someone want to do so in the future? Also, I faced a similar problem with the new futex2 syscalls, that supports exclusively 64bit timespec. But I took a different approach: I called __NR_clock_gettime64 for 32bit architectures so it wouldn't require to convert the struct: #if defined(__i386__) || __TIMESIZE == 32 # define NR_gettime64 __NR_clock_gettime64 #else # define NR_gettime64 __NR_clock_gettime #endif struct timespec64 { long long tv_sec; /* seconds */ long long tv_nsec; /* nanoseconds */ }; int gettime64(clock_t clockid, struct timespec64 *tv) { return syscall(NR_gettime64, clockid, tv); } Then we can just use &timeout at __NR_futex_time64 for 32bit arch and at __NR_futex for 64bit arch. This might be a simpler solution to the problem that you are facing but I'm not entirely sure. Also, futex's selftests do use the timeout argument and I think that they also won't compile in 32-bit RISC-V, so maybe we can start from there so we can actually test the timeout argument and check if it's working. Thanks, André
On Tue, Sep 21, 2021 at 12:47 AM André Almeida <andrealmeid@collabora.com> wrote: > > #if defined(__i386__) || __TIMESIZE == 32 > # define NR_gettime64 __NR_clock_gettime64 > #else > # define NR_gettime64 __NR_clock_gettime > #endif > > struct timespec64 { > long long tv_sec; /* seconds */ > long long tv_nsec; /* nanoseconds */ > }; > > int gettime64(clock_t clockid, struct timespec64 *tv) > { > return syscall(NR_gettime64, clockid, tv); > } > > Then we can just use &timeout at __NR_futex_time64 for 32bit arch and at > __NR_futex for 64bit arch. This is still broken when you disable CONFIG_COMPAT_32BIT_TIME, which disables all system calls that take time32 arguments. > This might be a simpler solution to the problem that you are facing but > I'm not entirely sure. Also, futex's selftests do use the timeout > argument and I think that they also won't compile in 32-bit RISC-V, so > maybe we can start from there so we can actually test the timeout > argument and check if it's working. I would love to see the wrapper that Alistair wrote as part of some kernel uapi header provided to user space. futex is used by tons of applications, and we never had a library abstraction for it, so everyone has to do these by hand, and they all get them slightly wrong in different ways. We normally don't do this in kernel headers, but I think the benefits would be far greater compared to today's situation. Arnd
Às 05:08 de 21/09/21, Arnd Bergmann escreveu: > On Tue, Sep 21, 2021 at 12:47 AM André Almeida > <andrealmeid@collabora.com> wrote: >> >> #if defined(__i386__) || __TIMESIZE == 32 >> # define NR_gettime64 __NR_clock_gettime64 >> #else >> # define NR_gettime64 __NR_clock_gettime >> #endif >> >> struct timespec64 { >> long long tv_sec; /* seconds */ >> long long tv_nsec; /* nanoseconds */ >> }; >> >> int gettime64(clock_t clockid, struct timespec64 *tv) >> { >> return syscall(NR_gettime64, clockid, tv); >> } >> >> Then we can just use &timeout at __NR_futex_time64 for 32bit arch and at >> __NR_futex for 64bit arch. > > This is still broken when you disable CONFIG_COMPAT_32BIT_TIME, > which disables all system calls that take time32 arguments. > Oh, I think my point was confusing then. My suggestion was to use only the futex entry points that accepts time64, and to always use clock_gettime that uses time64, for all platforms. Then it will work if we disable CONFIG_COMPAT_32BIT_TIME. >> This might be a simpler solution to the problem that you are facing but >> I'm not entirely sure. Also, futex's selftests do use the timeout >> argument and I think that they also won't compile in 32-bit RISC-V, so >> maybe we can start from there so we can actually test the timeout >> argument and check if it's working. > > I would love to see the wrapper that Alistair wrote as part of some kernel > uapi header provided to user space. futex is used by tons of applications, > and we never had a library abstraction for it, so everyone has to do these > by hand, and they all get them slightly wrong in different ways. Why we don't have a futex() wrapper at glibc as we do have for others syscalls? > > We normally don't do this in kernel headers, but I think the benefits > would be far greater compared to today's situation. > > Arnd >
On Wed, Sep 22, 2021 at 1:06 AM André Almeida <andrealmeid@collabora.com> wrote: > Às 05:08 de 21/09/21, Arnd Bergmann escreveu: > > I would love to see the wrapper that Alistair wrote as part of some kernel > > uapi header provided to user space. futex is used by tons of applications, > > and we never had a library abstraction for it, so everyone has to do these > > by hand, and they all get them slightly wrong in different ways. > > Why we don't have a futex() wrapper at glibc as we do have for others > syscalls? I think mainly because there was no agreement on what the calling conventions should be: The raw syscall is awkward because of the argument overloading that cannot easily be expressed in standard C in a typesafe way. Having a per-operation interface would avoid that problem but requires specifying what that particular interface has to be, and there is no standard to fall back on for this syscall. Arnd
On Wed, Sep 22, 2021 at 1:06 AM André Almeida <andrealmeid@collabora.com> wrote: > Às 05:08 de 21/09/21, Arnd Bergmann escreveu: > > On Tue, Sep 21, 2021 at 12:47 AM André Almeida > > <andrealmeid@collabora.com> wrote: > >> > >> #if defined(__i386__) || __TIMESIZE == 32 > >> # define NR_gettime64 __NR_clock_gettime64 > >> #else > >> # define NR_gettime64 __NR_clock_gettime > >> #endif > >> > >> struct timespec64 { > >> long long tv_sec; /* seconds */ > >> long long tv_nsec; /* nanoseconds */ > >> }; > >> > >> int gettime64(clock_t clockid, struct timespec64 *tv) > >> { > >> return syscall(NR_gettime64, clockid, tv); > >> } > >> > >> Then we can just use &timeout at __NR_futex_time64 for 32bit arch and at > >> __NR_futex for 64bit arch. > > > > This is still broken when you disable CONFIG_COMPAT_32BIT_TIME, > > which disables all system calls that take time32 arguments. > > > > Oh, I think my point was confusing then. My suggestion was to use only > the futex entry points that accepts time64, and to always use > clock_gettime that uses time64, for all platforms. Then it will work if > we disable CONFIG_COMPAT_32BIT_TIME. Yes, that would be ok. It does require using at least linux-5.1, but we perf is already sort-of tied to the kernel version. Arnd
On Tue, Sep 21, 2021 at 8:47 AM André Almeida <andrealmeid@collabora.com> wrote: > > Hi Alistair, > > Às 03:10 de 17/09/21, Alistair Francis escreveu: > > From: Alistair Francis <alistair.francis@wdc.com> > > > > Some 32-bit architectures (such are 32-bit RISC-V) only have a 64-bit > > time_t and as such don't have the SYS_futex syscall. This patch will > > allow us to use the SYS_futex_time64 syscall on those platforms. > > > > Thanks for your patch! However, I don't think that any futex operation > at perf has timeout. Do you plan to implement a test that use it? Or the > idea is to get this ready for it in case someone want to do so in the > future? I don't have plans to implement any new tests (although I'm happy to add one if need be). My goal was just to get this to build for RISC-V 32-bit. The timeout was already exposed by the old futex macro, so I was just following that. > > > Also, I faced a similar problem with the new futex2 syscalls, that > supports exclusively 64bit timespec. But I took a different approach: I > called __NR_clock_gettime64 for 32bit architectures so it wouldn't > require to convert the struct: > > #if defined(__i386__) || __TIMESIZE == 32 > # define NR_gettime64 __NR_clock_gettime64 > #else > # define NR_gettime64 __NR_clock_gettime > #endif > > struct timespec64 { > long long tv_sec; /* seconds */ > long long tv_nsec; /* nanoseconds */ > }; > > int gettime64(clock_t clockid, struct timespec64 *tv) > { > return syscall(NR_gettime64, clockid, tv); > } > > Then we can just use &timeout at __NR_futex_time64 for 32bit arch and at > __NR_futex for 64bit arch. So the idea is to use 64-bit time_t everywhere and only work on 5.1+ kernels. If that's the favoured approach I can convert this series to your idea. Alistair > > This might be a simpler solution to the problem that you are facing but > I'm not entirely sure. Also, futex's selftests do use the timeout > argument and I think that they also won't compile in 32-bit RISC-V, so > maybe we can start from there so we can actually test the timeout > argument and check if it's working. > > Thanks, > André
On Tue, Sep 21, 2021 at 6:08 PM Arnd Bergmann <arnd@arndb.de> wrote: > > On Tue, Sep 21, 2021 at 12:47 AM André Almeida > <andrealmeid@collabora.com> wrote: > > > > #if defined(__i386__) || __TIMESIZE == 32 > > # define NR_gettime64 __NR_clock_gettime64 > > #else > > # define NR_gettime64 __NR_clock_gettime > > #endif > > > > struct timespec64 { > > long long tv_sec; /* seconds */ > > long long tv_nsec; /* nanoseconds */ > > }; > > > > int gettime64(clock_t clockid, struct timespec64 *tv) > > { > > return syscall(NR_gettime64, clockid, tv); > > } > > > > Then we can just use &timeout at __NR_futex_time64 for 32bit arch and at > > __NR_futex for 64bit arch. > > This is still broken when you disable CONFIG_COMPAT_32BIT_TIME, > which disables all system calls that take time32 arguments. > > > This might be a simpler solution to the problem that you are facing but > > I'm not entirely sure. Also, futex's selftests do use the timeout > > argument and I think that they also won't compile in 32-bit RISC-V, so > > maybe we can start from there so we can actually test the timeout > > argument and check if it's working. > > I would love to see the wrapper that Alistair wrote as part of some kernel > uapi header provided to user space. futex is used by tons of applications, > and we never had a library abstraction for it, so everyone has to do these > by hand, and they all get them slightly wrong in different ways. > > We normally don't do this in kernel headers, but I think the benefits > would be far greater compared to today's situation. I'm happy to prepare a patch, if others are on board with it being accepted. Alistair > > Arnd
Às 01:34 de 24/09/21, Alistair Francis escreveu: > On Tue, Sep 21, 2021 at 8:47 AM André Almeida <andrealmeid@collabora.com> wrote: >> >> Hi Alistair, >> >> Às 03:10 de 17/09/21, Alistair Francis escreveu: >>> From: Alistair Francis <alistair.francis@wdc.com> >>> >>> Some 32-bit architectures (such are 32-bit RISC-V) only have a 64-bit >>> time_t and as such don't have the SYS_futex syscall. This patch will >>> allow us to use the SYS_futex_time64 syscall on those platforms. >>> >> >> Thanks for your patch! However, I don't think that any futex operation >> at perf has timeout. Do you plan to implement a test that use it? Or the >> idea is to get this ready for it in case someone want to do so in the >> future? > > I don't have plans to implement any new tests (although I'm happy to > add one if need be). > > My goal was just to get this to build for RISC-V 32-bit. The timeout > was already exposed by the old futex macro, so I was just following > that. > I see, thanks for working on that. >> >> >> Also, I faced a similar problem with the new futex2 syscalls, that >> supports exclusively 64bit timespec. But I took a different approach: I >> called __NR_clock_gettime64 for 32bit architectures so it wouldn't >> require to convert the struct: >> >> #if defined(__i386__) || __TIMESIZE == 32 >> # define NR_gettime64 __NR_clock_gettime64 >> #else >> # define NR_gettime64 __NR_clock_gettime >> #endif >> >> struct timespec64 { >> long long tv_sec; /* seconds */ >> long long tv_nsec; /* nanoseconds */ >> }; >> >> int gettime64(clock_t clockid, struct timespec64 *tv) >> { >> return syscall(NR_gettime64, clockid, tv); >> } >> >> Then we can just use &timeout at __NR_futex_time64 for 32bit arch and at >> __NR_futex for 64bit arch. > > So the idea is to use 64-bit time_t everywhere and only work on 5.1+ kernels. > > If that's the favoured approach I can convert this series to your idea. > Yes, this is what I think it will be the best approach. I believe the code will be less complex, it's more future proof (it's ready for y2038) and when glibc supports time64, we can make this code even simpler using `-D__USE_TIME_BITS64` to compile it. Thanks again for working on that! > Alistair > >> >> This might be a simpler solution to the problem that you are facing but >> I'm not entirely sure. Also, futex's selftests do use the timeout >> argument and I think that they also won't compile in 32-bit RISC-V, so >> maybe we can start from there so we can actually test the timeout >> argument and check if it's working. >> >> Thanks, >> André
diff --git a/tools/perf/bench/futex.h b/tools/perf/bench/futex.h index f80a4759ee79b..e88b531bed855 100644 --- a/tools/perf/bench/futex.h +++ b/tools/perf/bench/futex.h @@ -12,6 +12,7 @@ #include <sys/syscall.h> #include <sys/types.h> #include <linux/futex.h> +#include <linux/time_types.h> struct bench_futex_parameters { bool silent; @@ -28,7 +29,7 @@ struct bench_futex_parameters { }; /** - * futex_syscall() - SYS_futex syscall wrapper + * futex_syscall() - __NR_futex syscall wrapper * @uaddr: address of first futex * @op: futex op code * @val: typically expected value of uaddr, but varies by op @@ -49,14 +50,49 @@ static inline int futex_syscall(u_int32_t *uaddr, int op, u_int32_t val, struct timespec *timeout, u_int32_t *uaddr2, int val3, int opflags) { - return syscall(SYS_futex, uaddr, op | opflags, val, ts32, uaddr2, val3); +#if defined(__NR_futex_time64) + if (sizeof(*timeout) != sizeof(struct __kernel_old_timespec)) { + int ret = syscall(__NR_futex_time64, uaddr, op | opflags, val, timeout, + uaddr2, val3); + if (ret == 0 || errno != ENOSYS) + return ret; + } +#endif + +#if defined(__NR_futex) + if (sizeof(*timeout) == sizeof(struct __kernel_old_timespec)) + return syscall(__NR_futex, uaddr, op | opflags, val, timeout, uaddr2, val3); + + if (timeout && timeout->tv_sec == (long)timeout->tv_sec) { + struct __kernel_old_timespec ts32; + + ts32.tv_sec = (__kernel_long_t) timeout->tv_sec; + ts32.tv_nsec = (__kernel_long_t) timeout->tv_nsec; + + return syscall(__NR_futex, uaddr, op | opflags, val, ts32, uaddr2, val3); + } else if (!timeout) { + return syscall(__NR_futex, uaddr, op | opflags, val, NULL, uaddr2, val3); + } +#endif + + errno = ENOSYS; + return -1; } static inline int futex_syscall_nr_requeue(u_int32_t *uaddr, int op, u_int32_t val, int nr_requeue, u_int32_t *uaddr2, int val3, int opflags) { - return syscall(SYS_futex, uaddr, op | opflags, val, nr_requeue, uaddr2, val3); +#if defined(__NR_futex_time64) + int ret = syscall(__NR_futex_time64, uaddr, op | opflags, val, nr_requeue, + uaddr2, val3); + if (ret == 0 || errno != ENOSYS) + return ret; +#endif + +#if defined(__NR_futex) + return syscall(__NR_futex, uaddr, op | opflags, val, nr_requeue, uaddr2, val3); +#endif } /**