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 <[email protected]> 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.
