On Jan 17, 2006, at 9:50 AM, Ulrich Drepper wrote:

> And another thing: semaphores are on their way out.  So, in
> futex_deregister and in futex_head, shouldn't you use mutexes?  I
> don't see that you realy need semaphores.
>
> In futex_register, you define mm and initialize it with current->mm.
> That's OK.  But why then are you using
>
> +       down_read(&current->mm->mmap_sem);
>
> just a few lines below?
>
> And finally (for now): in get_futex_key the VMA containing the futex
> is determined.  And yet, in futex_register you have an identical
> find_extend_vma call.  I don't know how expensive this function is.
> But I would assume that at least the error handling in futex_register
> can go away since the VMA cannot be torn down while mmap_sem is taken,
> right?  But perhaps this just points to more inconsistencies.  Why is
> the list/sem lookup in get_futex_key?  Only to share the code with
> futex_deregister.  But is that really worth it?  The majority of calls
> to get_futex_key come from all the other call sites so the code you
> added is only a cost without any gain.  Especially since you could in
> futex_register do the whole thing without any additional cost and
> because most of the new tests in get_futex_key are again tested in
> futex_register (to determined shared vs non-shared) and do not have to
> be tested in futex_deregister (we know the futex is in shared memory).
>
> I suggest that if find_extend_vma is sufficiently expensive, pass a
> pointer to a variable of that type to get_futex_key.  If it is cheap,
> don't do anything.  Pull the new code in get_futex_key into
> futex_register and futex_deregister, optimize out unnecessary code,
> and merge with the rest of the functions.  It'll be much less
> invasive.