Re: [PATCH v1 11/14] futex: Implement FUTEX2_NUMA

[Thomas Gleixner <tglx@linutronix.de>]

On Mon, Jul 31 2023 at 20:03, Peter Zijlstra wrote:
> On Mon, Jul 31, 2023 at 07:36:21PM +0200, Thomas Gleixner wrote:
>> Hmm. Shouldn't that have changed with the allowance of the 1 and 2 byte
>> futexes?
>
> That patches comes after this.. :-)

Futexes are really cursed :)

> But I do have an open question here; do we want FUTEX2_NUMA futexes
> aligned at futex_size or double that? That is, what do we want the
> alignment of:
>
> struct futex_numa_32 {
> 	u32 val;
> 	u32 node;
> };
>
> to be? Having that u64 aligned will guarantee these two values end up in
> the same page, having them u32 aligned (as per this patch) allows for
> them to be split.

Same page and same cacheline.

> The current paths don't care, we don't hold locks, but perhaps it makes
> sense to be conservative.

I think it makes sense.

>> >  	address -= key->both.offset;
>> >  
>> > -	if (unlikely(!access_ok(uaddr, sizeof(u32))))
>> > +	if (flags & FLAGS_NUMA)
>> > +		size *= 2;
>> > +
>> > +	if (unlikely(!access_ok(uaddr, size)))
>> >  		return -EFAULT;
>> >  
>> >  	if (unlikely(should_fail_futex(fshared)))
>> >  		return -EFAULT;
>> >  
>> > +	key->both.node = -1;
>> 
>> Please put this into an else path.
>
> Can do, but I figured the compiler could figure it out through dead
> store elimitation or somesuch pass.

Sure, but taste disagrees and it simply makes the code more obvious.

>> > +	if (flags & FLAGS_NUMA) {
>> > +		void __user *naddr = uaddr + size/2;
>> 
>> size / 2;
>> 
>> > +
>> > +		if (futex_get_value(&node, naddr, flags))
>> > +			return -EFAULT;
>> > +
>> > +		if (node == -1) {
>> > +			node = numa_node_id();
>> > +			if (futex_put_value(node, naddr, flags))
>> > +				return -EFAULT;
>> > +		}
>> > +
>> > +		if (node >= MAX_NUMNODES || !node_possible(node))
>> > +			return -EINVAL;
>> 
>> That's clearly an else path too. No point in checking whether
>> numa_node_id() is valid.
>
> No, this also checks if the value we read from userspace is valid.
>
> Only when the value we read from userspace is -1 do we set
> numa_node_id(), otherwise we take the value as read, which then must be
> a valid value.

Right, but:

	if (node == -1) {
		node = numa_node_id();
		if (futex_put_value(node, naddr, flags))
			return -EFAULT;
	} else if (node >= MAX_NUMNODES || !node_possible(node)) {
		return -EINVAL;
        }

makes it clear that the path where @node read from user space is != -1
needs to be validated, while your version checks the result of

      node = numa_node_id();

too, which does not make sense to me. Yes, it works, but ...

Thanks,

        tglx