Message-ID: <66d9ee60-fbe3-4444-b98d-887845d4c187@kernel.org>
Date: Tue, 13 Feb 2024 21:50:51 +0100
From: Jesper Dangaard Brouer <hawk@...nel.org>
To: Sebastian Andrzej Siewior <bigeasy@...utronix.de>, bpf@...r.kernel.org,
 netdev@...r.kernel.org
Cc: Björn Töpel <bjorn@...nel.org>,
 "David S. Miller" <davem@...emloft.net>, Alexei Starovoitov
 <ast@...nel.org>, Andrii Nakryiko <andrii@...nel.org>,
 Daniel Borkmann <daniel@...earbox.net>, Eric Dumazet <edumazet@...gle.com>,
 Hao Luo <haoluo@...gle.com>, Jakub Kicinski <kuba@...nel.org>,
 Jiri Olsa <jolsa@...nel.org>, John Fastabend <john.fastabend@...il.com>,
 Jonathan Lemon <jonathan.lemon@...il.com>, KP Singh <kpsingh@...nel.org>,
 Maciej Fijalkowski <maciej.fijalkowski@...el.com>,
 Magnus Karlsson <magnus.karlsson@...el.com>,
 Martin KaFai Lau <martin.lau@...ux.dev>, Paolo Abeni <pabeni@...hat.com>,
 Peter Zijlstra <peterz@...radead.org>, Song Liu <song@...nel.org>,
 Stanislav Fomichev <sdf@...gle.com>, Thomas Gleixner <tglx@...utronix.de>,
 Yonghong Song <yonghong.song@...ux.dev>
Subject: Re: [PATCH RFC net-next 1/2] net: Reference bpf_redirect_info via
 task_struct on PREEMPT_RT.



On 13/02/2024 15.58, Sebastian Andrzej Siewior wrote:
> The XDP redirect process is two staged:
> - bpf_prog_run_xdp() is invoked to run a eBPF program which inspects the
>    packet and makes decisions. While doing that, the per-CPU variable
>    bpf_redirect_info is used.
> 
> - Afterwards xdp_do_redirect() is invoked and accesses bpf_redirect_info
>    and it may also access other per-CPU variables like xskmap_flush_list.
> 
> At the very end of the NAPI callback, xdp_do_flush() is invoked which
> does not access bpf_redirect_info but will touch the individual per-CPU
> lists.
> 
> The per-CPU variables are only used in the NAPI callback hence disabling
> bottom halves is the only protection mechanism. Users from preemptible
> context (like cpu_map_kthread_run()) explicitly disable bottom halves
> for protections reasons.
> Without locking in local_bh_disable() on PREEMPT_RT this data structure
> requires explicit locking to avoid corruption if preemption occurs.
> 
> PREEMPT_RT has forced-threaded interrupts enabled and every
> NAPI-callback runs in a thread. If each thread has its own data
> structure then locking can be avoided and data corruption is also avoided.
> 
> Create a struct bpf_xdp_storage which contains struct bpf_redirect_info.
> Define the variable on stack, use xdp_storage_set() to set a pointer to
> it in task_struct of the current task. Use the __free() annotation to
> automatically reset the pointer once function returns. Use a pointer which can
> be used by the __free() annotation to avoid invoking the callback the pointer
> is NULL. This helps the compiler to optimize the code.
> The xdp_storage_set() can nest. For instance local_bh_enable() in
> bpf_test_run_xdp_live() may run NET_RX_SOFTIRQ/ net_rx_action() which
> also uses xdp_storage_set(). Therefore only the first invocations
> updates the per-task pointer.
> Use xdp_storage_get_ri() as a wrapper to retrieve the current struct
> bpf_redirect_info.
> 
> This is only done on PREEMPT_RT. The !PREEMPT_RT builds keep using the
> per-CPU variable instead. This should also work for !PREEMPT_RT but
> isn't needed.
>  > Signed-off-by: Sebastian Andrzej Siewior<bigeasy@...utronix.de>

I generally like the idea around bpf_xdp_storage.

I only skimmed the code, but noticed some extra if-statements (for
!NULL). I don't think they will make a difference, but I know Toke want
me to test it...

I'll hopefully have time to look at code closer tomorrow.

--Jesper

Powered by blists - more mailing lists