public inbox for [email protected]
 help / color / mirror / Atom feed
From: Stefano Garzarella <[email protected]>
To: Jens Axboe <[email protected]>
Cc: [email protected]
Subject: Re: [PATCH liburing 3/5] Add helpers to set and get eventfd notification status
Date: Wed, 20 May 2020 17:11:43 +0200	[thread overview]
Message-ID: <20200520151143.22n3jkv2byfhioqj@steredhat> (raw)
In-Reply-To: <[email protected]>

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


  reply	other threads:[~2020-05-20 15:11 UTC|newest]

Thread overview: 12+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-05-15 16:43 [PATCH liburing 0/5] liburing: add helpers to enable/disable eventfd notifications Stefano Garzarella
2020-05-15 16:43 ` [PATCH liburing 1/5] Add CQ ring 'flags' field Stefano Garzarella
2020-05-15 16:43 ` [PATCH liburing 2/5] man/io_uring_setup.2: add 'flags' field in the struct io_cqring_offsets Stefano Garzarella
2020-05-15 16:43 ` [PATCH liburing 3/5] Add helpers to set and get eventfd notification status Stefano Garzarella
2020-05-15 16:53   ` Jens Axboe
2020-05-15 17:11     ` Stefano Garzarella
2020-05-20 13:12       ` Stefano Garzarella
2020-05-20 13:43         ` Jens Axboe
2020-05-20 15:11           ` Stefano Garzarella [this message]
2020-05-20 15:19             ` Jens Axboe
2020-05-15 16:43 ` [PATCH liburing 4/5] man/io_uring_register.2: add IORING_CQ_EVENTFD_DISABLED description Stefano Garzarella
2020-05-15 16:43 ` [PATCH liburing 5/5] Add test/eventfd-disable.c test case Stefano Garzarella

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20200520151143.22n3jkv2byfhioqj@steredhat \
    [email protected] \
    [email protected] \
    [email protected] \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox