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

[Martin Karsten <mkarsten@uwaterloo.ca>]

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