Re: [PATCH 0/5][v3] rq-qos memory barrier shenanigans

[Oleg Nesterov <oleg@redhat.com>]

On 07/16, Josef Bacik wrote:
>
> - add a comment about why we don't need a mb for the first data.token check
>   which I'm sure Oleg will tell me is wrong and I'll have to send a v4

we don't need it because prepare_to_wait_exclusive() does set_current_state()
and this implies mb().

(in fact I think in this particular case it is not needed at all because
 rq_qos_wake_function() sets condition == T under wq_head->lock)

I see nothing wrong in this series. Feel free to add

Reviewed-by: Oleg Nesterov <oleg@redhat.com>

But why does it use wq_has_sleeper() ? I do not understand why do we need
mb() before waitqueue_active() in this particular case...


and just for record. iiuc acquire_inflight_cb() can't block, so it is not
clear to me why performance-wise this all is actually better than just

	void rq_qos_wait(struct rq_wait *rqw, void *private_data,
			 acquire_inflight_cb_t *acquire_inflight_cb)
	{
		struct rq_qos_wait_data data = {
			.wq = {
				.flags	= WQ_FLAG_EXCLUSIVE;
				.func	= rq_qos_wake_function,
				.entry	= LIST_HEAD_INIT(data.wq.entry),
			},
			.task = current,
			.rqw = rqw,
			.cb = acquire_inflight_cb,
			.private_data = private_data,
		};

		if (!wq_has_sleeper(&rqw->wait) && acquire_inflight_cb(rqw, private_data))
			return;

		spin_lock_irq(&rqw->wait.lock);
		if (list_empty(&wq_entry->entry) && acquire_inflight_cb(rqw, private_data))
			data.got_token = true;
		else
			__add_wait_queue_entry_tail(&rqw->wait, &data.wq);
		spin_unlock_irq(&rqw->wait.lock);

		for (;;) {
			set_current_state(TASK_UNINTERRUPTIBLE);
			if (data.got_token)
				break;
			io_schedule();
		}
		finish_wait(&rqw->wait, &data.wq);
	}

note also that the acquire_inflight_cb argument goes away.

Nevermind, feel free to ignore.

Oleg.