From: Stefan Roesch <[email protected]>
To: Ammar Faizi <[email protected]>
Cc: Facebook Kernel Team <[email protected]>,
Jens Axboe <[email protected]>,
Olivier Langlois <[email protected]>,
netdev Mailing List <[email protected]>,
io-uring Mailing List <[email protected]>,
Jakub Kicinski <[email protected]>
Subject: Re: [PATCH v6 2/3] io_uring: add api to set / get napi configuration.
Date: Thu, 02 Feb 2023 10:47:17 -0800 [thread overview]
Message-ID: <[email protected]> (raw)
In-Reply-To: <[email protected]>
Ammar Faizi <[email protected]> writes:
> On Wed, Feb 01, 2023 at 02:22:53PM -0800, Stefan Roesch wrote:
>> +static int io_unregister_napi(struct io_ring_ctx *ctx, void __user *arg)
>> +{
>> +#ifdef CONFIG_NET_RX_BUSY_POLL
>> + const struct io_uring_napi curr = {
>> + .busy_poll_to = ctx->napi_busy_poll_to,
>> + };
>> +
>> + if (arg) {
>> + if (copy_to_user(arg, &curr, sizeof(curr)))
>> + return -EFAULT;
>> + }
>> +
>> + WRITE_ONCE(ctx->napi_busy_poll_to, 0);
>> + return 0;
>> +#else
>> + return -EINVAL;
>> +#endif
>> +}
>
> Just to follow the common pattern when a feature is not enabled the
> return value is -EOPNOTSUPP instead of -EINVAL. What do you think?
>
Sounds good, I'll return -EOPNOTSUPP when CONFIG_NET_RX_BUSY_POLL is not
enabled. I'll make the change for the register and the unregister
function.
>> + case IORING_UNREGISTER_NAPI:
>> + ret = -EINVAL;
>> + if (!arg)
>> + break;
>> + ret = io_unregister_napi(ctx, arg);
>> + break;
>
> This needs to be corrected. If the @arg var is NULL, you return -EINVAL.
> So io_unregister_napi() will always have @arg != NULL. This @arg check
> should go, allow the user to pass a NULL pointer to it.
>
> Our previous agreement on this API is to allow the user to pass a NULL
> pointer in case the user doesn't care about the old value.
>
> Also, having a liburing test case that verifies this behavior would be
> excellent.
>
I'll remove the check for the arg parameter. In addition I will output
the busy poll timeout and the prefer busy poll setting in the client
example program if one of the settings has been enabled.
next prev parent reply other threads:[~2023-02-02 18:49 UTC|newest]
Thread overview: 6+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-02-01 22:22 [PATCH v6 0/3] io_uring: add napi busy polling support Stefan Roesch
2023-02-01 22:22 ` [PATCH v6 1/3] " Stefan Roesch
2023-02-01 22:22 ` [PATCH v6 2/3] io_uring: add api to set / get napi configuration Stefan Roesch
2023-02-01 23:15 ` Ammar Faizi
2023-02-02 18:47 ` Stefan Roesch [this message]
2023-02-01 22:22 ` [PATCH v6 3/3] io_uring: add api to set napi prefer busy poll Stefan Roesch
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] \
[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