Re: [PATCH 1/2] wait: add wq_has_multiple_sleepers helper

[Oleg Nesterov <oleg@redhat.com>]

On 07/11, Oleg Nesterov wrote:
>
> Jens,
>
> I managed to convince myself I understand why 2/2 needs this change...
> But rq_qos_wait() still looks suspicious to me. Why can't the main loop
> "break" right after io_schedule()? rq_qos_wake_function() either sets
> data->got_token = true or it doesn't wakeup the waiter sleeping in
> io_schedule()
>
> This means that data.got_token = F at the 2nd iteration is only possible
> after a spurious wakeup, right? But in this case we need to set state =
> TASK_UNINTERRUPTIBLE again to avoid busy-wait looping ?

Oh. I can be easily wrong, I never read this code before, but it seems to
me there is another unrelated race.

rq_qos_wait() can't rely on finish_wait() because it doesn't necessarily
take wq_head->lock.

rq_qos_wait() inside the main loop does

		if (!has_sleeper && acquire_inflight_cb(rqw, private_data)) {
			finish_wait(&rqw->wait, &data.wq);

			/*
			 * We raced with wbt_wake_function() getting a token,
			 * which means we now have two. Put our local token
			 * and wake anyone else potentially waiting for one.
			 */
			if (data.got_token)
				cleanup_cb(rqw, private_data);
			break;
		}

finish_wait() + "if (data.got_token)" can race with rq_qos_wake_function()
which does

	data->got_token = true;
	list_del_init(&curr->entry);

rq_qos_wait() can see these changes out-of-order: finish_wait() can see
list_empty_careful() == T and avoid wq_head->lock, and in this case the
code above can see data->got_token = false.

No?

and I don't really understand

	has_sleeper = false;

at the end of the main loop. I think it should do "has_sleeper = true",
we need to execute the code above only once, right after prepare_to_wait().
But this is harmless.

Oleg.