lkml.org 
[lkml]   [2021]   [Feb]   [18]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [RFC PATCH 01/13] futex2: Implement wait and wake functions
From
Date
Hi Peter,

Às 06:02 de 16/02/21, Peter Zijlstra escreveu:
> On Mon, Feb 15, 2021 at 12:23:52PM -0300, André Almeida wrote:
>> +static int __futex_wait(struct futexv_head *futexv, unsigned int nr_futexes,
>> + struct hrtimer_sleeper *timeout)
>> +{
>> + int ret;
>> +
>> + while (1) {
>> + int awakened = -1;
>> +
>
> Might be easier to understand if the set_current_state() is here,
> instead of squirreled away in futex_enqueue().
>

I placed set_current_state() inside futex_enqueue() because we need to
set RUNNING and then INTERRUPTIBLE again for the retry path.

>> + ret = futex_enqueue(futexv, nr_futexes, &awakened);
>> +
>> + if (ret) {
>> + if (awakened >= 0)
>> + return awakened;
>> + return ret;
>> + }
>> +
>> + /* Before sleeping, check if someone was woken */
>> + if (!futexv->hint && (!timeout || timeout->task))
>> + freezable_schedule();
>> +
>> + __set_current_state(TASK_RUNNING);
>
> This is typically after the loop.
>

Sorry, which loop?

>> +
>> + /*
>> + * One of those things triggered this wake:
>> + *
>> + * * We have been removed from the bucket. futex_wake() woke
>> + * us. We just need to dequeue and return 0 to userspace.
>> + *
>> + * However, if no futex was dequeued by a futex_wake():
>> + *
>> + * * If the there's a timeout and it has expired,
>> + * return -ETIMEDOUT.
>> + *
>> + * * If there is a signal pending, something wants to kill our
>> + * thread, return -ERESTARTSYS.
>> + *
>> + * * If there's no signal pending, it was a spurious wake
>> + * (scheduler gave us a change to do some work, even if we
>
> chance?

Indeed, fixed.

>
>> + * don't want to). We need to remove ourselves from the
>> + * bucket and add again, to prevent losing wakeups in the
>> + * meantime.
>> + */
>
> Anyway, doing a dequeue and enqueue for spurious wakes is a bit of an
> anti-pattern that can lead to starvation. I've not actually looked at
> much detail yet as this is my first read-through, but did figure I'd
> mention it.
>

So we could just leave everything enqueued for spurious wake? I was
expecting that we would need to do all the work again (including
rechecking *uaddr == val) to see if we didn't miss a futex_wake()
between the kernel thread waking (spuriously) and going to sleep again.

>> +
>> + ret = futex_dequeue_multiple(futexv, nr_futexes);
>> +
>> + /* Normal wake */
>> + if (ret >= 0)
>> + return ret;
>> +
>> + if (timeout && !timeout->task)
>> + return -ETIMEDOUT;
>> +
>> + if (signal_pending(current))
>> + return -ERESTARTSYS;
>> +
>> + /* Spurious wake, do everything again */
>> + }
>> +}

Thanks,
André

\
 
 \ /
  Last update: 2021-02-18 21:13    [W:1.231 / U:0.004 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site