Re: [PATCH] FUTEX : new PRIVATE futexes

[Nick Piggin]

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.
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/