Messages in this thread | | | Subject | Re: [RFC PATCH 01/13] futex2: Implement wait and wake functions | From | André Almeida <> | Date | Thu, 18 Feb 2021 17:09:02 -0300 |
| |
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é
| |