netdev - Re: [PATCH bpf-next] bpf: cpumap: Move xdp:xdp_cpumap

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: <e34bc47d-aada-4bf8-ac29-d3462f351f20@kernel.org>
Date: Tue, 10 Sep 2024 10:48:03 +0200
From: Jesper Dangaard Brouer <hawk@...nel.org>
To: Daniel Xu <dxu@...uu.xyz>, davem@...emloft.net, ast@...nel.org,
 kuba@...nel.org, daniel@...earbox.net, john.fastabend@...il.com,
 andrii@...nel.org
Cc: martin.lau@...ux.dev, eddyz87@...il.com, song@...nel.org,
 yonghong.song@...ux.dev, kpsingh@...nel.org, sdf@...ichev.me,
 haoluo@...gle.com, jolsa@...nel.org, netdev@...r.kernel.org,
 bpf@...r.kernel.org, linux-kernel@...r.kernel.org, lorenzo@...nel.org,
 aleksander.lobakin@...el.com, kernel-team@...a.com
Subject: Re: [PATCH bpf-next] bpf: cpumap: Move xdp:xdp_cpumap_kthread
 tracepoint before rcv



On 06/09/2024 03.22, Daniel Xu wrote:
> cpumap takes RX processing out of softirq and onto a separate kthread.
> Since the kthread needs to be scheduled in order to run (versus softirq
> which does not), we can theoretically experience extra latency if the
> system is under load and the scheduler is being unfair to us.
> 
> Moving the tracepoint to before passing the skb list up the stack allows
> users to more accurately measure enqueue/dequeue latency introduced by
> cpumap via xdp:xdp_cpumap_enqueue and xdp:xdp_cpumap_kthread tracepoints.
> 

It makes sense for me to move this :-)
It actually fits my use-case even better.

> f9419f7bd7a5 ("bpf: cpumap add tracepoints") which added the tracepoints
> states that the intent behind them was for general observability and for
> a feedback loop to see if the queues are being overwhelmed. This change
> does not mess with either of those use cases but rather adds a third
> one.

Yes, my use-case is to this as a feedback loop, to see when queue is
overwhelmed as you say.  I will soon be playing with this feature in
production environments, so I'm excited that it looks like you have
similar use-cases for this :-)

> 
> Signed-off-by: Daniel Xu <dxu@...uu.xyz>

Acked-by: Jesper Dangaard Brouer <hawk@...nel.org>

> ---
>   kernel/bpf/cpumap.c | 6 ++++--
>   1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/kernel/bpf/cpumap.c b/kernel/bpf/cpumap.c
> index fbdf5a1aabfe..a2f46785ac3b 100644
> --- a/kernel/bpf/cpumap.c
> +++ b/kernel/bpf/cpumap.c
> @@ -354,12 +354,14 @@ static int cpu_map_kthread_run(void *data)
>   
>   			list_add_tail(&skb->list, &list);
>   		}
> -		netif_receive_skb_list(&list);
>   
> -		/* Feedback loop via tracepoint */
> +		/* Feedback loop via tracepoint.
> +		 * NB: keep before recv to allow measuring enqueue/dequeue latency.
> +		 */
>   		trace_xdp_cpumap_kthread(rcpu->map_id, n, kmem_alloc_drops,
>   					 sched, &stats);
>   
> +		netif_receive_skb_list(&list);
>   		local_bh_enable(); /* resched point, may call do_softirq() */
>   	}
>   	__set_current_state(TASK_RUNNING);