Re: [PATCH] FUTEX : new PRIVATE futexes

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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 [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[Index of Archives]     [Kernel Newbies]     [Netfilter]     [Bugtraq]     [Photo]     [Stuff]     [Gimp]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Video 4 Linux]     [Linux for the blind]     [Linux Resources]
  Powered by Linux