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;
}
Thanks,
Stefano
