Re: [PATCH v2 4/4] io_uring: get next req on subm ref drop

[Jens Axboe]

On 3/3/20 3:46 AM, Pavel Begunkov wrote:
> On 3/3/2020 9:54 AM, Pavel Begunkov wrote:
>> On 03/03/2020 07:26, Jens Axboe wrote:
>>> On 3/2/20 1:45 PM, Pavel Begunkov wrote:
>>>> Get next request when dropping the submission reference. However, if
>>>> there is an asynchronous counterpart (i.e. read/write, timeout, etc),
>>>> that would be dangerous to do, so ignore them using new
>>>> REQ_F_DONT_STEAL_NEXT flag.
>>>
>>> Hmm, not so sure I like this one. It's not quite clear to me where we
>>> need REQ_F_DONT_STEAL_NEXT. If we have an async component, then we set
>>> REQ_F_DONT_STEAL_NEXT. So this is generally the case where our
>>> io_put_req() for submit is not the last drop. And for the other case,
>>> the put is generally in the caller anyway. So I don't really see what
>>> this extra flag buys us?
>>
>> Because io_put_work() holds a reference, no async handler can achive req->refs
>> == 0, so it won't return next upon dropping the submission ref (i.e. by
>> put_find_nxt()). And I want to have next before io_put_work(), to, instead of as
>> currently:
>>
>> run_work(work);
>> assign_cur_work(NULL); // spinlock + unlock worker->lock
>> new_work = put_work(work);
>> assign_cur_work(new_work); // the second time
>>
>> do:
>>
>> new_work = run_work(work);
>> assign_cur_work(new_work); // need new_work here
>> put_work(work);
>>
>>
>> The other way:
>>
>> io_wq_submit_work() // for all async handlers
>> {
>> 	...
>> 	// Drop submission reference.
>> 	// One extra ref will be put in io_put_work() right
>> 	// after return, and it'll be done in the same thread
>> 	if (atomic_dec_and_get(req) == 1)
>> 		steal_next(req);
>> }
>>
>> Maybe cleaner, but looks fragile as well. Would you prefer it?
> 
> Any chance you've measured your next-work fix? I wonder how much does it
> hurt performance, and whether we need a terse patch for 5.6.

Unless I'm missing something, the worker will pick up the next work
without sleeping, since the request will have finished. So it really
should not add any extra overhead, except you'll do an extra wqe lock
roundtrip.

But I'll run some testing to be totally sure.

-- 
Jens Axboe