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

[Martin Karsten]

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@xxxxxxxxxxxx>
> >
> > 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