| Message ID | 20190715201120.72749-4-josef@toxicpanda.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | rq-qos memory barrier shenanigans | expand |
On 07/15, Josef Bacik wrote: > > Oleg noticed that our checking of data.got_token is unsafe in the > cleanup case, and should really use a memory barrier. Use the > READ_ONCE/WRITE_ONCE helpers on got_token so we can be sure we're always > safe. READ/WRITE_ONCE can't help, both are compiler barriers. You need smp_wmb/rmb. Alternatively, > @@ -246,7 +246,7 @@ void rq_qos_wait(struct rq_wait *rqw, void *private_data, > prepare_to_wait_exclusive(&rqw->wait, &data.wq, TASK_UNINTERRUPTIBLE); > has_sleeper = !wq_has_single_sleeper(&rqw->wait); > do { > - if (data.got_token) > + if (READ_ONCE(data.got_token)) > break; > if (!has_sleeper && acquire_inflight_cb(rqw, private_data)) { > finish_wait(&rqw->wait, &data.wq); You can use remove_wait_queue() which takes rqw->wait->lock unconditonally, but then you will need to do __set_current_state(TASK_RUNNING) and use "return" instead of break. Oleg.
diff --git a/block/blk-rq-qos.c b/block/blk-rq-qos.c index 67a0a4c07060..f4aa7b818cf5 100644 --- a/block/blk-rq-qos.c +++ b/block/blk-rq-qos.c @@ -201,7 +201,7 @@ static int rq_qos_wake_function(struct wait_queue_entry *curr, if (!data->cb(data->rqw, data->private_data)) return -1; - data->got_token = true; + WRITE_ONCE(data->got_token, true); list_del_init(&curr->entry); wake_up_process(data->task); return 1; @@ -246,7 +246,7 @@ void rq_qos_wait(struct rq_wait *rqw, void *private_data, prepare_to_wait_exclusive(&rqw->wait, &data.wq, TASK_UNINTERRUPTIBLE); has_sleeper = !wq_has_single_sleeper(&rqw->wait); do { - if (data.got_token) + if (READ_ONCE(data.got_token)) break; if (!has_sleeper && acquire_inflight_cb(rqw, private_data)) { finish_wait(&rqw->wait, &data.wq); @@ -256,7 +256,7 @@ void rq_qos_wait(struct rq_wait *rqw, void *private_data, * which means we now have two. Put our local token * and wake anyone else potentially waiting for one. */ - if (data.got_token) + if (READ_ONCE(data.got_token)) cleanup_cb(rqw, private_data); break; }
Oleg noticed that our checking of data.got_token is unsafe in the cleanup case, and should really use a memory barrier. Use the READ_ONCE/WRITE_ONCE helpers on got_token so we can be sure we're always safe. Signed-off-by: Josef Bacik <josef@toxicpanda.com> --- block/blk-rq-qos.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)