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.
> 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 ;)
Thanks,
Paolo
