Re: [PATCH 2] Little fixes to previous futex patch

[Jamie Lokier]

Ingo Molnar wrote:
> btw., regarding this fix:
> 
>   ChangeSet@xxxxxxxxxx, 2003-09-06 12:28:20-07:00, hugh@xxxxxxxxxxx
>     [PATCH] Fix futex hashing bugs
> 
> why dont we do this:
> 
>                         } else {
>                                 /* Make sure to stop if key1 == key2 */
> 				if (head1 == head2)
> 					break;
>                                 list_add_tail(i, head2);
>                                 this->key = key2;
>                                 if (ret - nr_wake >= nr_requeue)
>                                         break;
>                         }
> 
> instead of the current:
> 
>                         } else {
>                                 list_add_tail(i, head2);
>                                 this->key = key2;
>                                 if (ret - nr_wake >= nr_requeue)
>                                         break;
>                                 /* Make sure to stop if key1 == key2 */
>                                 if (head1 == head2 && head1 != next)
>                                         head1 = i;
>                         }
> 
> what's the point in requeueing once, and then exiting the loop by changing
> the loop exit condition variable?

Hugh's patch is clever and subtle.  It doesn't exit the loop; the
loop continues from "next".

What it does is change the end condition so that the loop stops just
before the first requeued futex.  Let's call that one REQUEUED1.

If are other futexes to requeue, they after inserted after REQUEUED1
(because head2 wasn't changed), yet the end condition _isn't_ changed
when this happens, because now head1 != head2.

This causes the correct number of futexes to be requeued at the end of
the wait list.

> You are trying to avoid the lockup but the first one ought to be the
> most straightforward way to do it.

Hugh's patch returns the correct retval _and_ requeues the correct
number of waiters to the end of the queue.  And it does it without
fancy code.

Remember that the waiter order is visible to userspace - it's used by
"fair" operations, so it's appropriate that requeuing a futex to
itself moves nr_requeues waiters to the end of the queue, just like it
does when it requeues to a different futex.

If the code to handle that were complicated, I'd vote for dropping it.
But Hugh's patch does exactly the right thing in a simple way.  Lovely!

-- Jamie
-
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/