On Mon, 7 Nov 2022 09:52:39 -0800 Stefan Roesch wrote:
> This adds the napi busy polling support in io_uring.c. It adds a new
> napi_list to the io_ring_ctx structure. This list contains the list of
> napi_id's that are currently enabled for busy polling. The list is
> synchronized by the new napi_lock spin lock. The current default napi
> busy polling time is stored in napi_busy_poll_to. If napi busy polling
> is not enabled, the value is 0.
>
> The busy poll timeout is also stored as part of the io_wait_queue. This
> is necessary as for sq polling the poll interval needs to be adjusted
> and the napi callback allows only to pass in one value.
>
> Testing has shown that the round-trip times are reduced to 38us from
> 55us by enabling napi busy polling with a busy poll timeout of 100us.
What's the test, exactly? What's the network latency? Did you busy poll
on both ends?
I reckon we should either find a real application or not include any
numbers. Most of the quoted win likely comes from skipping IRQ
coalescing. Which can just be set lowered if latency of 30usec is
a win in itself..
Would it be possible to try to integrate this with Jonathan's WIP
zero-copy work? I presume he has explicit NAPI/queue <> io_uring
instance mapping which is exactly the kind of use case we should
make a first-class citizen here.
> + spin_lock(&ctx->napi_lock);
> + list_for_each_entry(ne, &ctx->napi_list, list) {
> + if (ne->napi_id == napi_id) {
> + ne->timeout = jiffies + NAPI_TIMEOUT;
What's the NAPI_TIMEOUT thing? I don't see it mentioned in
the commit msg.
> + list_for_each_entry_safe(ne, n, napi_list, list) {
> + napi_busy_loop(ne->napi_id, NULL, NULL, true, BUSY_POLL_BUDGET);
You can't opt the user into prefer busy poll without the user asking
for it. Default to false and add an explicit knob like patch 2.
> timeout = ktime_add_ns(timespec64_to_ktime(ts), ktime_get_ns());
> }
> +#ifdef CONFIG_NET_RX_BUSY_POLL
> + else if (!list_empty(&local_napi_list)) {
> + iowq.busy_poll_to = READ_ONCE(ctx->napi_busy_poll_to);
> + }
> +#endif
You don't have to break the normal bracket placement for an ifdef:
if (something) {
boring_code();
#ifdef CONFIG_WANT_CHEESE
} else if (is_gouda) {
/* mmm */
nom_nom();
#endif
}
