[3/4] rq-qos: use READ_ONCE/WRITE_ONCE for got_token
diff mbox series

Message ID 20190715201120.72749-4-josef@toxicpanda.com
State New
Headers show
Series
  • rq-qos memory barrier shenanigans
Related show

Commit Message

Josef Bacik July 15, 2019, 8:11 p.m. UTC
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(-)

Comments

Oleg Nesterov July 16, 2019, 8:29 a.m. UTC | #1
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.

Patch
diff mbox series

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;
 		}