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

From: Hugh Dickins
Date: Sun Sep 07 2003 - 07:28:11 EST


On Sun, 7 Sep 2003, 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? You are trying to avoid the lockup but
> the first one ought to be the most straightforward way to do it.

I think you're reading it as a "list_for_each(i, head1)" loop,
whereas it is and must be a "list_for_each_safe(i, next, head1)" loop.

So it won't (in general) terminate after this one requeueing (as
list_for_each would, finding i->next == head1): termination depends on
next (already set) and head1, so I repoint head1 to the first requeued.

So it should terminate after one pass down the list, when it reaches the
first requeued, and can then return the appropriate "ret" count to user.

You may perhaps know that the ret count is not important, but I don't
know that, so wanted to get it right. (At the time, I also wanted to
have the list sorted exactly as intended, but now I can't see that the
relative positions of different keys could matter at all.)

It may be bad practice to use a familiar macro like list_for_each_safe,
yet play with its controlling variables within the loop. I just felt
safer that way than expanding it, or adding extraneous variables.

Hugh

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