Re: [PATCH] FUTEX : new PRIVATE futexes

Nick Piggin
2007-04-06T11:50:38Z

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.