netdev - Re: [PATCH net-next v6 2/7] net: Suspend softirq when prefer_busy

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20241105210338.5364375d@kernel.org>
Date: Tue, 5 Nov 2024 21:03:38 -0800
From: Jakub Kicinski <kuba@...nel.org>
To: Joe Damato <jdamato@...tly.com>
Cc: netdev@...r.kernel.org, corbet@....net, hdanton@...a.com,
 bagasdotme@...il.com, pabeni@...hat.com, namangulati@...gle.com,
 edumazet@...gle.com, amritha.nambiar@...el.com,
 sridhar.samudrala@...el.com, sdf@...ichev.me, peter@...eblog.net,
 m2shafiei@...terloo.ca, bjorn@...osinc.com, hch@...radead.org,
 willy@...radead.org, willemdebruijn.kernel@...il.com, skhawaja@...gle.com,
 Martin Karsten <mkarsten@...terloo.ca>, "David S. Miller"
 <davem@...emloft.net>, Simon Horman <horms@...nel.org>, David Ahern
 <dsahern@...nel.org>, Sebastian Andrzej Siewior <bigeasy@...utronix.de>,
 Lorenzo Bianconi <lorenzo@...nel.org>, Alexander Lobakin
 <aleksander.lobakin@...el.com>, linux-kernel@...r.kernel.org (open list)
Subject: Re: [PATCH net-next v6 2/7] net: Suspend softirq when
 prefer_busy_poll is set

On Mon,  4 Nov 2024 21:55:26 +0000 Joe Damato wrote:
> From: Martin Karsten <mkarsten@...terloo.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@...terloo.ca>
> Co-developed-by: Joe Damato <jdamato@...tly.com>
> Signed-off-by: Joe Damato <jdamato@...tly.com>
> Tested-by: Joe Damato <jdamato@...tly.com>
> Tested-by: Martin Karsten <mkarsten@...terloo.ca>
> Acked-by: Stanislav Fomichev <sdf@...ichev.me>
> Reviewed-by: Sridhar Samudrala <sridhar.samudrala@...el.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;
>  		}