Re: [PATCH liburing 3/5] Add helpers to set and get eventfd notification status

[Jens Axboe <axboe@kernel.dk>]

On 5/20/20 9:11 AM, Stefano Garzarella wrote:
> On Wed, May 20, 2020 at 07:43:43AM -0600, Jens Axboe wrote:
>> On 5/20/20 7:12 AM, Stefano Garzarella wrote:
>>>>> The bigger question is probably how to handle kernels that don't
>>>>> have this feature. It'll succeed, but we'll still post events. Maybe
>>>>> the kernel side should have a feature flag that we can test?
>>>>
>>>> I thought about that, and initially I added a
>>>> IORING_FEAT_EVENTFD_DISABLE, but then I realized that we are adding
>>>> the CQ 'flags' field together with the eventfd disabling feature.
>>>>
>>>> So I supposed that if 'p->cq_off.flags' is not zero, than the kernel
>>>> supports CQ flags and also the IORING_CQ_EVENTFD_DISABLED bit.
>>>>
>>>> Do you think that's okay, or should we add IORING_FEAT_EVENTFD_DISABLE
>>>> (or something similar)?
>>>
>>> Hi Jens,
>>> I'm changing io_uring_cq_eventfd_enable() to io_uring_cq_eventfd_toggle().
>>
>> Sounds good.
>>
>>> Any advice on the error and eventual feature flag?
>>
>> I guess we can use cq_off.flags != 0 to tell if we have this feature or
>> not, even though it's a bit quirky. But at the same time, probably not
>> worth adding a specific feature flag for.
> 
> Agree.
> 
>>
>> For the error, -EOPNOTSUPP seems fine if we don't have the feature. Just
>> don't flag errors for enabling when already enabled, or vice versa. It's
> 
> Okay.
> 
>> inherently racy in that completions can come in while the app is calling
>> the helper, so we should make the interface relaxed.
> 
> Yes, do you think we should also provide an interface to do double
> check while re-enabling notifications?
> Or we can leave this to the application?
> 
> I mean something like this:
> 
>     bool io_uring_cq_eventfd_safe_enable(struct io_uring *ring)
>     {
>         /* enable notifications */
>         io_uring_cq_eventfd_toggle(ring, true);
> 
>         /* Do we have any more cqe in the ring? */
>         if (io_uring_cq_ready(ring)) {
>             io_uring_cq_eventfd_toggle(ring, false);
>             return false;
>         }
> 
>         return true;
>     }

Let's just leave it for now unless/until there's a clear need for
something like that.

-- 
Jens Axboe