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