Re: [PATCH 01/18] io_uring: remove the need for relying on an io-wq fallback worker

[Jens Axboe <axboe@kernel.dk>]

On 2/19/21 1:25 PM, Eric W. Biederman wrote:
>> @@ -2313,11 +2316,14 @@ static int io_req_task_work_add(struct io_kiocb *req)
>>  static void io_req_task_work_add_fallback(struct io_kiocb *req,
>>  					  task_work_func_t cb)
>>  {
>> -	struct task_struct *tsk = io_wq_get_task(req->ctx->io_wq);
>> +	struct io_ring_ctx *ctx = req->ctx;
>> +	struct callback_head *head;
>>  
>>  	init_task_work(&req->task_work, cb);
>> -	task_work_add(tsk, &req->task_work, TWA_NONE);
>> -	wake_up_process(tsk);
>> +	do {
>> +		head = ctx->exit_task_work;
>                        ^^^^^^^^^^^^^^^^^^^^
> This feels like this should be READ_ONCE to prevent tearing reads.
> 
> You use READ_ONCE on this same variable below which really suggests
> this should be a READ_ONCE.

It should, added.

>> +		req->task_work.next = head;
>> +	} while (cmpxchg(&ctx->exit_task_work, head, &req->task_work) != head);
>>  }
>>  
>>  static void __io_req_task_cancel(struct io_kiocb *req, int error)
>> @@ -9258,6 +9264,30 @@ void __io_uring_task_cancel(void)
>>  	io_uring_remove_task_files(tctx);
>>  }
>>  
>> +static void io_run_ctx_fallback(struct io_ring_ctx *ctx)
>> +{
>> +	struct callback_head *work, *head, *next;
>> +
>> +	do {
>> +		do {
>> +			head = NULL;
>> +			work = READ_ONCE(ctx->exit_task_work);
>> +			if (!work)
>> +				break;
>> +		} while (cmpxchg(&ctx->exit_task_work, work, head) != work);
>> +
>> +		if (!work)
>> +			break;
> 
> Why the double break on "!work"?  It seems like either the first should
> be goto out, or only the second should be here.

Yes good point, the first one should go away. I've made that change,
thanks.

-- 
Jens Axboe