From: Alistair Francis <[email protected]>
In preparation for a more complex futex() function let's convert the
current macro into two functions. We need two functions to avoid
compiler failures as the macro is overloaded.
This will allow us to include pre-processor conditionals in the futex
syscall functions.
Signed-off-by: Alistair Francis <[email protected]>
---
tools/perf/bench/futex.h | 37 +++++++++++++++++++++++--------------
1 file changed, 23 insertions(+), 14 deletions(-)
diff --git a/tools/perf/bench/futex.h b/tools/perf/bench/futex.h
index b3853aac3021c..f80a4759ee79b 100644
--- a/tools/perf/bench/futex.h
+++ b/tools/perf/bench/futex.h
@@ -28,7 +28,7 @@ struct bench_futex_parameters {
};
/**
- * futex() - SYS_futex syscall wrapper
+ * futex_syscall() - SYS_futex syscall wrapper
* @uaddr: address of first futex
* @op: futex op code
* @val: typically expected value of uaddr, but varies by op
@@ -38,17 +38,26 @@ struct bench_futex_parameters {
* @val3: varies by op
* @opflags: flags to be bitwise OR'd with op, such as FUTEX_PRIVATE_FLAG
*
- * futex() is used by all the following futex op wrappers. It can also be
+ * futex_syscall() is used by all the following futex op wrappers. It can also be
* used for misuse and abuse testing. Generally, the specific op wrappers
- * should be used instead. It is a macro instead of an static inline function as
- * some of the types over overloaded (timeout is used for nr_requeue for
- * example).
+ * should be used instead.
*
* These argument descriptions are the defaults for all
* like-named arguments in the following wrappers except where noted below.
*/
-#define futex(uaddr, op, val, timeout, uaddr2, val3, opflags) \
- syscall(SYS_futex, uaddr, op | opflags, val, timeout, uaddr2, val3)
+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);
+}
+
+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);
+}
/**
* futex_wait() - block on uaddr with optional timeout
@@ -57,7 +66,7 @@ struct bench_futex_parameters {
static inline int
futex_wait(u_int32_t *uaddr, u_int32_t val, struct timespec *timeout, int opflags)
{
- return futex(uaddr, FUTEX_WAIT, val, timeout, NULL, 0, opflags);
+ return futex_syscall(uaddr, FUTEX_WAIT, val, timeout, NULL, 0, opflags);
}
/**
@@ -67,7 +76,7 @@ futex_wait(u_int32_t *uaddr, u_int32_t val, struct timespec *timeout, int opflag
static inline int
futex_wake(u_int32_t *uaddr, int nr_wake, int opflags)
{
- return futex(uaddr, FUTEX_WAKE, nr_wake, NULL, NULL, 0, opflags);
+ return futex_syscall(uaddr, FUTEX_WAKE, nr_wake, NULL, NULL, 0, opflags);
}
/**
@@ -76,7 +85,7 @@ futex_wake(u_int32_t *uaddr, int nr_wake, int opflags)
static inline int
futex_lock_pi(u_int32_t *uaddr, struct timespec *timeout, int opflags)
{
- return futex(uaddr, FUTEX_LOCK_PI, 0, timeout, NULL, 0, opflags);
+ return futex_syscall(uaddr, FUTEX_LOCK_PI, 0, timeout, NULL, 0, opflags);
}
/**
@@ -85,7 +94,7 @@ futex_lock_pi(u_int32_t *uaddr, struct timespec *timeout, int opflags)
static inline int
futex_unlock_pi(u_int32_t *uaddr, int opflags)
{
- return futex(uaddr, FUTEX_UNLOCK_PI, 0, NULL, NULL, 0, opflags);
+ return futex_syscall(uaddr, FUTEX_UNLOCK_PI, 0, NULL, NULL, 0, opflags);
}
/**
@@ -97,7 +106,7 @@ static inline int
futex_cmp_requeue(u_int32_t *uaddr, u_int32_t val, u_int32_t *uaddr2, int nr_wake,
int nr_requeue, int opflags)
{
- return futex(uaddr, FUTEX_CMP_REQUEUE, nr_wake, nr_requeue, uaddr2,
+ return futex_syscall_nr_requeue(uaddr, FUTEX_CMP_REQUEUE, nr_wake, nr_requeue, uaddr2,
val, opflags);
}
@@ -113,7 +122,7 @@ static inline int
futex_wait_requeue_pi(u_int32_t *uaddr, u_int32_t val, u_int32_t *uaddr2,
struct timespec *timeout, int opflags)
{
- return futex(uaddr, FUTEX_WAIT_REQUEUE_PI, val, timeout, uaddr2, 0,
+ return futex_syscall(uaddr, FUTEX_WAIT_REQUEUE_PI, val, timeout, uaddr2, 0,
opflags);
}
@@ -130,7 +139,7 @@ static inline int
futex_cmp_requeue_pi(u_int32_t *uaddr, u_int32_t val, u_int32_t *uaddr2,
int nr_requeue, int opflags)
{
- return futex(uaddr, FUTEX_CMP_REQUEUE_PI, 1, nr_requeue, uaddr2,
+ return futex_syscall_nr_requeue(uaddr, FUTEX_CMP_REQUEUE_PI, 1, nr_requeue, uaddr2,
val, opflags);
}
--
2.31.1
From: Alistair Francis <[email protected]>
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 <[email protected]>
---
tools/perf/bench/futex.h | 42 +++++++++++++++++++++++++++++++++++++---
1 file changed, 39 insertions(+), 3 deletions(-)
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
}
/**
--
2.31.1
On Fri, Sep 17, 2021 at 8:10 AM Alistair Francis
<[email protected]> wrote:
>
> From: Alistair Francis <[email protected]>
>
> 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 <[email protected]>
Thanks for the follow-up!
Reviewed-by: Arnd Bergmann <[email protected]>
On Fri, 17 Sep 2021, Alistair Francis wrote:
>From: Alistair Francis <[email protected]>
>
>In preparation for a more complex futex() function let's convert the
>current macro into two functions. We need two functions to avoid
>compiler failures as the macro is overloaded.
>
>This will allow us to include pre-processor conditionals in the futex
>syscall functions.
>
>Signed-off-by: Alistair Francis <[email protected]>
Acked-by: Davidlohr Bueso <[email protected]>
On Fri, 17 Sep 2021, Alistair Francis wrote:
>From: Alistair Francis <[email protected]>
>
>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 <[email protected]>
Hi Alistair,
Às 03:10 de 17/09/21, Alistair Francis escreveu:
> From: Alistair Francis <[email protected]>
>
> 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
<[email protected]> 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
> <[email protected]> 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 <[email protected]> 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 <[email protected]> wrote:
> Às 05:08 de 21/09/21, Arnd Bergmann escreveu:
> > On Tue, Sep 21, 2021 at 12:47 AM André Almeida
> > <[email protected]> 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 6:08 PM Arnd Bergmann <[email protected]> wrote:
>
> On Tue, Sep 21, 2021 at 12:47 AM André Almeida
> <[email protected]> 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
On Tue, Sep 21, 2021 at 8:47 AM André Almeida <[email protected]> wrote:
>
> Hi Alistair,
>
> Às 03:10 de 17/09/21, Alistair Francis escreveu:
> > From: Alistair Francis <[email protected]>
> >
> > 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é
Às 01:34 de 24/09/21, Alistair Francis escreveu:
> On Tue, Sep 21, 2021 at 8:47 AM André Almeida <[email protected]> wrote:
>>
>> Hi Alistair,
>>
>> Às 03:10 de 17/09/21, Alistair Francis escreveu:
>>> From: Alistair Francis <[email protected]>
>>>
>>> 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é