public inbox for [email protected]
 help / color / mirror / Atom feed
From: Jens Axboe <[email protected]>
To: Stefano Garzarella <[email protected]>
Cc: [email protected], [email protected],
	[email protected],
	Alexander Viro <[email protected]>
Subject: Re: [PATCH 0/2] io_uring: add a CQ ring flag to enable/disable eventfd notification
Date: Fri, 15 May 2020 09:26:20 -0600	[thread overview]
Message-ID: <[email protected]> (raw)
In-Reply-To: <20200515152418.oi6btvogplmuezfn@steredhat>

On 5/15/20 9:24 AM, Stefano Garzarella wrote:
> On Fri, May 15, 2020 at 09:13:33AM -0600, Jens Axboe wrote:
>> On 5/15/20 8:34 AM, Stefano Garzarella wrote:
>>> On Fri, May 15, 2020 at 08:24:58AM -0600, Jens Axboe wrote:
>>>> On 5/15/20 4:54 AM, Stefano Garzarella wrote:
>>>>> The first patch adds the new 'cq_flags' field for the CQ ring. It
>>>>> should be written by the application and read by the kernel.
>>>>>
>>>>> The second patch adds a new IORING_CQ_NEED_WAKEUP flag that can be
>>>>> used by the application to enable/disable eventfd notifications.
>>>>>
>>>>> I'm not sure the name is the best one, an alternative could be
>>>>> IORING_CQ_NEED_EVENT.
>>>>>
>>>>> This feature can be useful if the application are using eventfd to be
>>>>> notified when requests are completed, but they don't want a notification
>>>>> for every request.
>>>>> Of course the application can already remove the eventfd from the event
>>>>> loop, but as soon as it adds the eventfd again, it will be notified,
>>>>> even if it has already handled all the completed requests.
>>>>>
>>>>> The most important use case is when the registered eventfd is used to
>>>>> notify a KVM guest through irqfd and we want a mechanism to
>>>>> enable/disable interrupts.
>>>>>
>>>>> I also extended liburing API and added a test case here:
>>>>> https://github.com/stefano-garzarella/liburing/tree/eventfd-disable
>>>>
>>>> Don't mind the feature, and I think the patches look fine. But the name
>>>> is really horrible, I'd have no idea what that flag does without looking
>>>> at the code or a man page. Why not call it IORING_CQ_EVENTFD_ENABLED or
>>>> something like that? Or maybe IORING_CQ_EVENTFD_DISABLED, and then you
>>>> don't have to muck with the default value either. The app would set the
>>>> flag to disable eventfd, temporarily, and clear it again when it wants
>>>> notifications again.
>>>
>>> You're clearly right! :-) The name was horrible.
>>
>> Sometimes you go down that path on naming and just can't think of
>> the right one. I think we've all been there.
> 
> :-)
> 
>>
>>> I agree that IORING_CQ_EVENTFD_DISABLED should be the best.
>>> I'll send a v2 changing the name and removing the default value.
>>
>> Great thanks, and please do queue a pull for the liburing side too.
> 
> For the liburing side do you prefer a PR on github or posting the
> patches on [email protected] with 'liburing' tag?

Either one is fine. I tend to prefer patches, but that's mostly because
various contributors on the liburing side don't have the same kind of
patch writing etiquette that we do on the kernel side. Hence they need
massaging in terms of commit messages, and patches are then easier. But
for you, just do what you prefer. I never use the github merge features,
always do it manually myself anyway.

-- 
Jens Axboe


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

Thread overview: 8+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-05-15 10:54 [PATCH 0/2] io_uring: add a CQ ring flag to enable/disable eventfd notification Stefano Garzarella
2020-05-15 10:54 ` [PATCH 1/2] io_uring: add 'cq_flags' field for the CQ ring Stefano Garzarella
2020-05-15 10:54 ` [PATCH 2/2] io_uring: add IORING_CQ_NEED_WAKEUP to the CQ ring flags Stefano Garzarella
2020-05-15 14:24 ` [PATCH 0/2] io_uring: add a CQ ring flag to enable/disable eventfd notification Jens Axboe
2020-05-15 14:34   ` Stefano Garzarella
2020-05-15 15:13     ` Jens Axboe
2020-05-15 15:24       ` Stefano Garzarella
2020-05-15 15:26         ` Jens Axboe [this message]

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 \
    [email protected] \
    [email protected] \
    [email protected] \
    [email protected] \
    [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