From: Thomas Gleixner <[email protected]>
The comment about the restriction of the number of waiters to wake for the
REQUEUE_PI case is confusing at best. Rewrite it.
Signed-off-by: Thomas Gleixner <[email protected]>
---
kernel/futex.c | 28 ++++++++++++++++++++--------
1 file changed, 20 insertions(+), 8 deletions(-)
---
--- a/kernel/futex.c
+++ b/kernel/futex.c
@@ -1960,15 +1960,27 @@ static int futex_requeue(u32 __user *uad
*/
if (refill_pi_state_cache())
return -ENOMEM;
+
/*
- * requeue_pi must wake as many tasks as it can, up to nr_wake
- * + nr_requeue, since it acquires the rt_mutex prior to
- * returning to userspace, so as to not leave the rt_mutex with
- * waiters and no owner. However, second and third wake-ups
- * cannot be predicted as they involve race conditions with the
- * first wake and a fault while looking up the pi_state. Both
- * pthread_cond_signal() and pthread_cond_broadcast() should
- * use nr_wake=1.
+ * futex_requeue() allows the caller to define the number
+ * of waiters to wake up via the @nr_wake argument. With
+ * REQUEUE_PI waking up more than one waiter is creating
+ * more problems than it solves. Waking up a waiter makes
+ * only sense if the PI futex @uaddr2 is uncontended as
+ * this allows the requeue code to acquire the futex
+ * @uaddr2 before waking the waiter. The waiter can then
+ * return to user space without further action. A secondary
+ * wakeup would just make the futex_wait_requeue_pi()
+ * handling more complex because that code would have to
+ * look up pi_state and do more or less all the handling
+ * which the requeue code has to do for the to be requeued
+ * waiters. So restrict the number of waiters to wake to
+ * one and only wake it up when the PI futex is
+ * uncontended. Otherwise requeue it and let the unlock of
+ * the PI futex handle the wakeup.
+ *
+ * All REQUEUE_PI users, e.g. pthread_cond_signal() and
+ * pthread_cond_broadcast() must use nr_wake=1.
*/
if (nr_wake != 1)
return -EINVAL;
On Thu, 05 Aug 2021, Thomas Gleixner wrote:
>From: Thomas Gleixner <[email protected]>
>
>The comment about the restriction of the number of waiters to wake for the
>REQUEUE_PI case is confusing at best. Rewrite it.
This certainly reads better.
>
>Signed-off-by: Thomas Gleixner <[email protected]>
>---
> kernel/futex.c | 28 ++++++++++++++++++++--------
> 1 file changed, 20 insertions(+), 8 deletions(-)
>---
>--- a/kernel/futex.c
>+++ b/kernel/futex.c
>@@ -1960,15 +1960,27 @@ static int futex_requeue(u32 __user *uad
> */
> if (refill_pi_state_cache())
> return -ENOMEM;
Perhaps this can be moved after the nr_wake check below? No sense
in calling refill_pi_state_cache() if the user is passing bogus
parameters.
> if (nr_wake != 1)
> return -EINVAL;
>
Thanks,
Davidlohr
On Sun, Aug 08 2021 at 11:43, Davidlohr Bueso wrote:
> On Thu, 05 Aug 2021, Thomas Gleixner wrote:
>
>>From: Thomas Gleixner <[email protected]>
>>
>>The comment about the restriction of the number of waiters to wake for the
>>REQUEUE_PI case is confusing at best. Rewrite it.
>
> This certainly reads better.
>
>>
>>Signed-off-by: Thomas Gleixner <[email protected]>
>>---
>> kernel/futex.c | 28 ++++++++++++++++++++--------
>> 1 file changed, 20 insertions(+), 8 deletions(-)
>>---
>>--- a/kernel/futex.c
>>+++ b/kernel/futex.c
>>@@ -1960,15 +1960,27 @@ static int futex_requeue(u32 __user *uad
>> */
>> if (refill_pi_state_cache())
>> return -ENOMEM;
>
> Perhaps this can be moved after the nr_wake check below? No sense
> in calling refill_pi_state_cache() if the user is passing bogus
> parameters.
Yes.