Re: [PATCH 2/2] io_uring: add timeout update

[Jens Axboe <axboe@kernel.dk>]

On 11/30/20 11:27 AM, Pavel Begunkov wrote:
> On 30/11/2020 18:15, Jens Axboe wrote:
>> On 11/29/20 10:12 AM, Pavel Begunkov wrote:
>>> +	tr->flags = READ_ONCE(sqe->timeout_flags);
>>> +	if (tr->flags) {
>>> +		if (!(tr->flags & IORING_TIMEOUT_UPDATE))
>>> +			return -EINVAL;
>>> +		if (tr->flags & ~(IORING_TIMEOUT_UPDATE|IORING_TIMEOUT_ABS))
>>> +			return -EINVAL;
>>
>> These flag comparisons are a bit obtuse - perhaps warrants a comment?
> 
> Ok, the one below should be more readable.
> 
> if (tr->flags & IORING_TIMEOUT_UPDATE) {
> 	if (flags & ~ALLOWED_UPDATE_FLAGS)
> 		return -EINVAL;
> 	...
> } else if (tr->flags) {
> 	/* timeout removal doesn't support flags */
> 	return -EINVAL;
> }

Yeah, that's actually readable.

>>> +		ret = __io_sq_thread_acquire_mm(req->ctx);
>>> +		if (ret)
>>> +			return ret;
>>
>> Why is this done manually?
> 
> mm is only needed in *prep(), so don't want IO_WQ_WORK_MM to put it
> into req->work since it also affects timeout remove reqs.

Don't think it's worth it, I'd just mark it needing it uncondtionally
instead. Reason being the obvious of not having stuff like this buried
deep in an op hander, but also that for non SQPOLL, we'll do these
inline always and it's a no-op. For SQPOLL we'd need to grab it, but
I'd rather pay that cost than have this in there.

>>> diff --git a/include/uapi/linux/io_uring.h b/include/uapi/linux/io_uring.h
>>> index 6bb8229de892..12a6443ea60d 100644
>>> --- a/include/uapi/linux/io_uring.h
>>> +++ b/include/uapi/linux/io_uring.h
>>> @@ -151,6 +151,7 @@ enum {
>>>   * sqe->timeout_flags
>>>   */
>>>  #define IORING_TIMEOUT_ABS	(1U << 0)
>>> +#define IORING_TIMEOUT_UPDATE	(1U << 31)
>>
>> Why bit 31?
> 
> Left bits for other potential timeout modes, don't know which though.
> Can return it to bit 1.

Let's just use 1, there's no clear separation in there anyway.

-- 
Jens Axboe