On 10/22/24 08:47, Paolo Abeni wrote:
Hi,
On 10/21/24 19:16, Pavel Begunkov wrote:
On 10/21/24 15:25, Paolo Abeni wrote:
On 10/16/24 20:52, David Wei wrote:
[...]
+ napi = napi_by_id(napi_id);
+ if (!napi)
+ return;
+
+ if (!IS_ENABLED(CONFIG_PREEMPT_RT))
+ preempt_disable();
+
+ for (;;) {
+ local_bh_disable();
+
+ if (napi_state_start_busy_polling(napi, 0)) {
+ have_poll_lock = netpoll_poll_lock(napi);
+ cb(cb_arg);
+ local_bh_enable();
+ busy_poll_stop(napi, have_poll_lock, 0, 1);
+ break;
+ }
+
+ local_bh_enable();
+ if (unlikely(need_resched()))
+ break;
+ cpu_relax();
Don't you need a 'loop_end' condition here?
As you mentioned in 14/15, it can indeed spin for long and is bound only
by need_resched(). Do you think it's reasonable to wait for it without a
time limit with NAPI_STATE_PREFER_BUSY_POLL? softirq should yield napi
after it exhausts the budget, it should limit it well enough, what do
you think?
The only ugly part is that I don't want it to mess with the
NAPI_F_PREFER_BUSY_POLL in busy_poll_stop()
busy_poll_stop() {
...
clear_bit(NAPI_STATE_IN_BUSY_POLL, &napi->state);
if (flags & NAPI_F_PREFER_BUSY_POLL) {
napi->defer_hard_irqs_count = READ_ONCE(napi->dev->napi_defer_hard_irqs);
timeout = READ_ONCE(napi->dev->gro_flush_timeout);
if (napi->defer_hard_irqs_count && timeout) {
hrtimer_start(&napi->timer, ns_to_ktime(timeout), HRTIMER_MODE_REL_PINNED);
skip_schedule = true;
}
}
}
Why do you want to avoid such branch? It will do any action only when
the user-space explicitly want to leverage the hrtimer to check for
incoming packets. In such case, I think the kernel should try to respect
the user configuration.
It should be fine to pass the flag here, it just doesn't feel right.
napi_execute() is not interested in polling, but IIRC this chunk delays
the moment when softirq kicks in when there are no napi pollers. I.e.
IMO ideally it shouldn't affect napi polling timings...
Is it fine to set PREFER_BUSY_POLL but do the stop call without? E.g.
set_bit(NAPI_STATE_PREFER_BUSY_POLL, &napi->state);
...
busy_poll_stop(napi, flags=0);
My preference is for using NAPI_STATE_PREFER_BUSY_POLL consistently. It
should ensure a reasonably low latency for napi_execute() and consistent
infra behavior. Unless I'm missing some dangerous side effect ;)
... but let's just set it then. It only affects the zerocopy private
queue.
--
Pavel Begunkov
