Re: [PATCH v1 02/14] futex: Extend the FUTEX2 flags

[Thomas Gleixner]

On Mon, Jul 31 2023 at 23:33, Peter Zijlstra wrote:
> On Mon, Jul 31, 2023 at 11:14:11PM +0200, Thomas Gleixner wrote:
>> --- 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 :/

I know, but that doesn't make these things more readable and neither an
argument against doing it for futex2 :)

> Also, sys_futex_{wake,wait}() have this thing as a syscall argument,
> surely you don't want to put this union there as well?

Why not? The anon union does not break the ABI unless I'm missing
something. Existing user space can still use 'flags' and people who care
about readability can use the bitfield, no?

Its inside struct futex_waitv and not an explicit syscall argument, right?

> 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.

I'm aware of that by now :)

Still that explicit bitfield does neither need comments nor does it
leave room for interpretation.

Thanks,

        tglx