Re: [PATCH net-next v6 2/7] net: Suspend softirq when prefer_busy_poll is set

[Jakub Kicinski <kuba@kernel.org>]

On Mon,  4 Nov 2024 21:55:26 +0000 Joe Damato wrote:
> From: Martin Karsten <mkarsten@uwaterloo.ca>
> 
> When NAPI_F_PREFER_BUSY_POLL is set during busy_poll_stop and the
> irq_suspend_timeout is nonzero, this timeout is used to defer softirq
> scheduling, potentially longer than gro_flush_timeout. This can be used
> to effectively suspend softirq processing during the time it takes for
> an application to process data and return to the next busy-poll.
> 
> The call to napi->poll in busy_poll_stop might lead to an invocation of

The call to napi->poll when we're arming the timer is counter
productive, right? Maybe we can take this opportunity to add
the seemingly missing logic to skip over it?

> napi_complete_done, but the prefer-busy flag is still set at that time,
> so the same logic is used to defer softirq scheduling for
> irq_suspend_timeout.
> 
> Signed-off-by: Martin Karsten <mkarsten@uwaterloo.ca>
> Co-developed-by: Joe Damato <jdamato@fastly.com>
> Signed-off-by: Joe Damato <jdamato@fastly.com>
> Tested-by: Joe Damato <jdamato@fastly.com>
> Tested-by: Martin Karsten <mkarsten@uwaterloo.ca>
> Acked-by: Stanislav Fomichev <sdf@fomichev.me>
> Reviewed-by: Sridhar Samudrala <sridhar.samudrala@intel.com>
> ---
>  v3:
>    - Removed reference to non-existent sysfs parameter from commit
>      message. No functional/code changes.
> 
>  net/core/dev.c | 17 +++++++++++++----
>  1 file changed, 13 insertions(+), 4 deletions(-)
> 
> diff --git a/net/core/dev.c b/net/core/dev.c
> index 4d910872963f..51d88f758e2e 100644
> --- a/net/core/dev.c
> +++ b/net/core/dev.c
> @@ -6239,7 +6239,12 @@ bool napi_complete_done(struct napi_struct *n, int work_done)
>  			timeout = napi_get_gro_flush_timeout(n);
>  		n->defer_hard_irqs_count = napi_get_defer_hard_irqs(n);
>  	}
> -	if (n->defer_hard_irqs_count > 0) {
> +	if (napi_prefer_busy_poll(n)) {
> +		timeout = napi_get_irq_suspend_timeout(n);

Why look at the suspend timeout in napi_complete_done()?
We are unlikely to be exiting busy poll here.
Is it because we need more time than gro_flush_timeout
for the application to take over the polling?

> +		if (timeout)
> +			ret = false;
> +	}
> +	if (ret && n->defer_hard_irqs_count > 0) {
>  		n->defer_hard_irqs_count--;
>  		timeout = napi_get_gro_flush_timeout(n);
>  		if (timeout)
> @@ -6375,9 +6380,13 @@ static void busy_poll_stop(struct napi_struct *napi, void *have_poll_lock,
>  	bpf_net_ctx = bpf_net_ctx_set(&__bpf_net_ctx);
>  
>  	if (flags & NAPI_F_PREFER_BUSY_POLL) {
> -		napi->defer_hard_irqs_count = napi_get_defer_hard_irqs(napi);
> -		timeout = napi_get_gro_flush_timeout(napi);
> -		if (napi->defer_hard_irqs_count && timeout) {
> +		timeout = napi_get_irq_suspend_timeout(napi);

Even here I'm not sure if we need to trigger suspend.
I don't know the eventpoll code well but it seems like you suspend 
and resume based on events when exiting epoll. Why also here?

> +		if (!timeout) {
> +			napi->defer_hard_irqs_count = napi_get_defer_hard_irqs(napi);
> +			if (napi->defer_hard_irqs_count)
> +				timeout = napi_get_gro_flush_timeout(napi);
> +		}
> +		if (timeout) {
>  			hrtimer_start(&napi->timer, ns_to_ktime(timeout), HRTIMER_MODE_REL_PINNED);
>  			skip_schedule = true;
>  		}