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
