Re: [RFC PATCH 01/13] futex2: Implement wait and wake functions

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é