Re: [RFC PATCH net-next 2/2] net: napi: Skip poll when arming GRO timer in busy poll

Martin Karsten
2026-04-29T01:02:52Z

On 2026-04-28 20:37, Jakub Kicinski wrote:
> On Tue, 28 Apr 2026 17:51:31 +0000 Dragos Tatulea wrote:
>> From: Martin Karsten <mkarsten@uwaterloo.ca>
>>
>> As referenced in the previous patch, having the GRO timer scheduled
>> while poll is running can lead to issues.
>>
>> Skip the extra call to napi->poll() when the GRO timer is armed. This
>> removes the need for having a separate __busy_poll_stop routine and its
>> code is moved directly into the relevant places in busy_poll_stop.
>
> This needs to go in separately, the previous patch should be a Fix,
> this is net-next material.

Ok, thanks for the recommendation!

>> @@ -6918,13 +6898,28 @@ static void busy_poll_stop(struct napi_struct *napi, void *have_poll_lock,
>>
>> if (flags & NAPI_F_PREFER_BUSY_POLL) {
>> napi->defer_hard_irqs_count = napi_get_defer_hard_irqs(napi);
>> - if (napi->defer_hard_irqs_count) {
>> - /* Timer will be scheduled after napi poll to avoid
>> - * firing during a slow poll which could cause the
>> - * queue to get stuck with interrupts disabled and no
>> - * scheduled timer.
>> + timeout = napi_get_gro_flush_timeout(napi);
>> + if (napi->defer_hard_irqs_count && timeout) {
>> + unsigned long flags;
>
> Feels like either timeout should also be declared in closest scope or
> flags at function level

Good point, thanks. I also just realize there's two flags - I somehow
missed that before. Maybe the 2nd flags isn't necessary after all.

>> + /* Drop prefer-busy state as in napi_complete_done(). */
>
> sloppy comment

Ok, will improve.

>> + clear_bit(NAPI_STATE_PREFER_BUSY_POLL, &napi->state);
>> + netpoll_poll_unlock(have_poll_lock);
>> +
>> + /* Flush too old packets. If HZ < 1000, flush all
>> + * packets.
>> + */
>
> sloppy comment

This one was just copied over from __busy_poll_stop. TBH, I don't know
exactly what's happening in the 'gro_flush_normal' call.

>> + gro_flush_normal(&napi->gro, HZ >= 1000);
>> + local_irq_save(flags);
>> + hrtimer_start(&napi->timer, ns_to_ktime(timeout),
>> + HRTIMER_MODE_REL_PINNED);
>> + clear_bit(NAPI_STATE_SCHED, &napi->state);
>> + local_irq_restore(flags);
>> +
>> + /* Timer started, so need for another call to
>> + * napi->poll().
>> */
>> - timeout = napi_get_gro_flush_timeout(napi);
>> + goto out;
>
> I think an else branch would do here? Let's not abuse goto

Ok, will rewrite. Among other things, I thought a goto avoids
indentation of the unchanged code and thus makes the patch smaller, but
I have no strong preference.

Thanks,
Martin