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

[Jesper Dangaard Brouer]

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@xxxxxxxxx>

Acked-by: Jesper Dangaard Brouer <hawk@xxxxxxxxxx>

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