Re: [PATCH v3 RESEND] io_uring: add timeout support for io_uring_enter()

[Jens Axboe <axboe@kernel.dk>]

On 11/4/20 1:16 PM, Pavel Begunkov wrote:
> On 04/11/2020 19:34, Jens Axboe wrote:
>> On 11/4/20 12:27 PM, Pavel Begunkov wrote:
>>> On 04/11/2020 18:32, Jens Axboe wrote:
>>>> On 11/4/20 10:50 AM, Jens Axboe wrote:
>>>>> +struct io_uring_getevents_arg {
>>>>> +	sigset_t *sigmask;
>>>>> +	struct __kernel_timespec *ts;
>>>>> +};
>>>>> +
>>>>
>>>> I missed that this is still not right, I did bring it up in your last
>>>> posting though - you can't have pointers as a user API, since the size
>>>> of the pointer will vary depending on whether this is a 32-bit or 64-bit
>>>> arch (or 32-bit app running on 64-bit kernel).
>>>
>>> Maybe it would be better 
>>>
>>> 1) to kill this extra indirection?
>>>
>>> struct io_uring_getevents_arg {
>>> -	sigset_t *sigmask;
>>> -	struct __kernel_timespec *ts;
>>> +	sigset_t sigmask;
>>> +	struct __kernel_timespec ts;
>>> };
>>>
>>> then,
>>>
>>> sigset_t *sig = (...)arg;
>>> __kernel_timespec* ts = (...)(arg + offset);
>>
>> But then it's kind of hard to know which, if any, of them are set... I
>> did think about this, and any solution seemed worse than just having the
>> extra indirection.
> 
> struct io_uring_getevents_arg {
> 	sigset_t sigmask;
> 	u32 mask;
> 	struct __kernel_timespec ts;
> };
> 
> if size > sizeof(sigmask), then use mask to determine that.
> Though, not sure how horrid the rest of the code would be.

I'm not saying it's not possible, just that I think the end result would
be worse in terms of both kernel code and how the user applications (or
liburing) would need to use it. I'd rather sacrifice an extra copy for
something that's straight forward (and logical) to use, rather than
needing weird setups or hoops to jump through. And this mask vs
sizeof(mask) thing seems pretty horrendeous to me :-)

>> Yeah, not doing the extra indirection would save a copy, but don't think
>> it's worth it for this path.
> 
> I much more don't like branching like IORING_ENTER_GETEVENTS_TIMEOUT,
> from conceptual point. I may try it out to see how it looks like while
> it's still for-next.

One thing I think we should change is the name,
IORING_ENTER_GETEVENTS_TIMEOUT will quickly be a bad name if we end up
adding just one more thing to the struct. Would be better to call it
IORING_ENTER_EXTRA_DATA or something, meaning that the sigmask pointer
is a pointer to the aux data instead of a sigmask. Better name
suggestions welcome...

-- 
Jens Axboe