Re: [PATCH] FUTEX : new PRIVATE futexes

[Nick Piggin <nickpiggin@yahoo.com.au>]

Eric Dumazet wrote:
> Nick Piggin a écrit :

>> Did the whole thing just turn out neater when you passed the rwsem?
>> We always know to use current->mm->mmap_sem, so it doesn't seem like
>> a boolean flag would hurt?
> 
> 
> That's a good question
> 
> current->mm->mmap_sem being calculated once is a win in itself, because 
> current access is not cheap.
> It also does the memory access to go through part of the chain in 
> advance, before its use. It does a prefetch() equivalent for free : If 
> current->mm is not in CPU cache, CPU wont stall because next 
> instructions dont depend on it.

Fair enough. Current access I think should be cheap though (it is
effectively a constant), but I guess it is still improvement.

>> Shouldn't that be sizeof(long) to handle 64 bit futexes? Or strictly, it
>> should depend on the size of the operation. Maybe the access_ok check
>> should go outside get_futex_key?
> 
> 
> If you check again, you'll see that address points to the start of the 
> PAGE, not the real u32/u64 futex address. This checks the PAGE. We can 
> use char, short, int, long, or char[PAGE_SIZE] as long as we know a 
> futex cannot span two pages.

Ah, that works.

>>>       */
>>>      key->shared.inode = vma->vm_file->f_path.dentry->d_inode;
>>> -    key->both.offset++; /* Bit 0 of offset indicates inode-based 
>>> key. */
>>> +    key->both.offset += FUT_OFF_INODE; /* inode-based key. */
>>>      if (likely(!(vma->vm_flags & VM_NONLINEAR))) {
>>>          key->shared.pgoff = (((address - vma->vm_start) >> PAGE_SHIFT)
>>>                       + vma->vm_pgoff);
>>
>>
>> I like |= for adding flags, it seems less ambiguous. But I guess that's
>> a matter of opinion. Hugh seems to like +=, and I can't argue with him
>> about style issues ;)
> 
> 
> 
> Previous code was doing offset++ wich means offset += 1;

But it doesn't mean you have to ;)

>>> @@ -1598,6 +1656,8 @@ static int futex_wait(unsigned long __us
>>>          restart->arg1 = val;
>>>          restart->arg2 = (unsigned long)abs_time;
>>>          restart->arg3 = (unsigned long)futex64;
>>> +        if (shared)
>>> +            restart->arg3 |= 2;
>>
>>
>> Could you make this into a proper flags argument and use #define 
>> CONSTANTs for it?
> 
> 
> Yes, but I'm not sure it will improve readability.

Well that bit of code alone is obviously unreadable.

restart->arg3 = 0;
if (futex64)
     restart->arg3 |= FUTEX_64;
if (shared)
     restart->arg3 |= FUTEX_SHARED;

Maybe a matter of taste.

> 
>>
>>> @@ -2377,23 +2455,24 @@ sys_futex64(u64 __user *uaddr, int op, u
>>>      struct timespec ts;
>>>      ktime_t t, *tp = NULL;
>>>      u64 val2 = 0;
>>> +    int opm = op & FUTEX_CMD_MASK;
>>
>>
>> What's opm stand for?
> 
> 
> I guess 'm' stands for 'mask' or 'masked' ?

Why not call it cmd? (ie. what it is, rather than what you have done
to derive it).

-- 
SUSE Labs, Novell Inc.