[bpf-next] bpf: cpumap: Move xdp:xdp_cpumap_kthread tracepoint before rcv

Commit Message

Daniel Xu Sept. 6, 2024, 1:22 a.m. UTC

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.

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.

Signed-off-by: Daniel Xu <[e-mail]>
---
 kernel/bpf/cpumap.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

Comments

Jesper Dangaard Brouer Sept. 10, 2024, 8:48 a.m. UTC | #1

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 <[e-mail]>

Acked-by: Jesper Dangaard Brouer <[e-mail]>

> ---
>   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);

[e-mail] Sept. 11, 2024, 2:50 p.m. UTC | #2

Hello:

This patch was applied to bpf/bpf-next.git (net)
by Daniel Borkmann <[e-mail]>:

On Thu,  5 Sep 2024 19:22:44 -0600 you 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.
> 
> [...]

Here is the summary with links:
  - [bpf-next] bpf: cpumap: Move xdp:xdp_cpumap_kthread tracepoint before rcv
    https://git.kernel.org/bpf/bpf-next/c/23dc9867329c

You are awesome, thank you!

Patch

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);