Re: [PATCH v1 02/14] futex: Extend the FUTEX2 flags
From: Peter Zijlstra <peterz@infradead.org>
Date: 2023-07-31 21:34:58
Also in:
linux-arch, linux-mm, lkml
On Mon, Jul 31, 2023 at 11:14:11PM +0200, Thomas Gleixner wrote:
On Mon, Jul 31 2023 at 21:20, Peter Zijlstra wrote:quoted
-#define FUTEX2_MASK (FUTEX2_64 | FUTEX2_PRIVATE) +#define FUTEX2_MASK (FUTEX2_SIZE_MASK | FUTEX2_PRIVATE)Along with some comment which documents that the size "flags" constitute a number field and not flags in the sense of binary flags. And please name these size constants so it really becomes obvious: #define FUTEX2_SIZE_U32 2
So you want them named: #define FUTEX2_SIZE_U8 0x00 #define FUTEX2_SIZE_U16 0x01 #define FUTEX2_SIZE_U32 0x02 #define FUTEX2_SIZE_U64 0x03 #define FUTEX2_SIZE_MASK 0x03 Sure, can do.
quoted
/** * futex_parse_waitv - Parse a waitv array from userspace@@ -208,11 +208,11 @@ static int futex_parse_waitv(struct fute return -EINVAL; if (!IS_ENABLED(CONFIG_64BIT) || in_compat_syscall()) { - if ((aux.flags & FUTEX2_64) == FUTEX2_64) + if ((aux.flags & FUTEX2_SIZE_MASK) == FUTEX2_64) return -EINVAL; }That should be part of the actual 64bit futex enablement, no?
The 'unsigned long' thing is part of the syscalls, which is why I had it now.
quoted hunk ↗ jump to hunk
quoted
- if ((aux.flags & FUTEX2_64) != FUTEX2_32) + if ((aux.flags & FUTEX2_SIZE_MASK) != FUTEX2_32) return -EINVAL;In hindsight I think it was as mistake just to have this __u32 flags field in the new interface. Soemthing like the incomplete below might be retrofittable, no?--- a/include/uapi/linux/futex.h +++ b/include/uapi/linux/futex.h@@ -74,7 +74,12 @@ struct futex_waitv { __u64 val; __u64 uaddr; - __u32 flags; + union { + __u32 flags; + __u32 size : 2, + : 5, + private : 1; + }; __u32 __reserved; };
Durr, I'm not sure I remember if that does the right thing across
architectures -- might just work. But I'm fairly sure this isn't the
only case of a field in a flags thing in our APIs. Although obviously I
can't find another case in a hurry :/
Also, sys_futex_{wake,wait}() have this thing as a syscall argument,
surely you don't want to put this union there as well?
I'd much prefer to just keep the 'unsigned int flags' thing and perhaps
put a comment on-top of the '#define FUTEX2_*' thingies. Note that
having it a field instead of a bunch of flags makes sense, since you can
only have a single size, not a combination of sizes.