On 2024-05-06 21:41:38 [+0200], Toke Høiland-Jørgensen wrote:
> > On PREEMPT_RT the pointer to bpf_net_context is saved task's
> > task_struct. On non-PREEMPT_RT builds the pointer saved in a per-CPU
> > variable (which is always NODE-local memory). Using always the
> > bpf_net_context approach has the advantage that there is almost zero
> > differences between PREEMPT_RT and non-PREEMPT_RT builds.
>
> Did you ever manage to get any performance data to see if this has an
> impact?
Not really. I would expect far away memory is more expensive.
I have just a 10G setup and after disabling IOMMU I got the "expected"
packet rate. Since the CPU usage was not 100% I always got that packet
rate. Lowering the CPU clock speed resulted in three (I think) rate
ranges depending on the invocation and I didn't figure out why. Since it
is always a range, I didn't see here if my changes had any impact since
the numbers were roughly the same.
With enabled IOMMU, its overhead was major so again I didn't see any
impact of my changes.
> [...]
>
> > +static inline struct bpf_net_context *bpf_net_ctx_get(void)
> > +{
> > + struct bpf_net_context *bpf_net_ctx = this_cpu_read(bpf_net_context);
> > +
> > + WARN_ON_ONCE(!bpf_net_ctx);
>
> If we have this WARN...
>
> > +static inline struct bpf_redirect_info *bpf_net_ctx_get_ri(void)
> > +{
> > + struct bpf_net_context *bpf_net_ctx = bpf_net_ctx_get();
> > +
> > + if (!bpf_net_ctx)
> > + return NULL;
>
> ... do we really need all the NULL checks?
>
> (not just here, but in the code below as well).
>
> I'm a little concerned that we are introducing a bunch of new branches
> in the XDP hot path. Which is also why I'm asking for benchmarks :)
We could hide the WARN behind CONFIG_DEBUG_NET. The only purpose is to
see the backtrace where the context is missing. Having just an error
somewhere will make it difficult to track.
The NULL check is to avoid a crash if the context is missing. You could
argue that this should be noticed in development and never hit
production. If so, then we get the backtrace from NULL-pointer
dereference and don't need the checks and WARN.
> [...]
>
> > + /* ri->map is assigned in __bpf_xdp_redirect_map() from within a eBPF
> > + * program/ during NAPI callback. It is used during
> > + * xdp_do_generic_redirect_map()/ __xdp_do_redirect_frame() from the
> > + * redirect callback afterwards. ri->map is cleared after usage.
> > + * The path has no explicit RCU read section but the local_bh_disable()
> > + * is also a RCU read section which makes the complete softirq callback
> > + * RCU protected. This in turn makes ri->map RCU protocted and it is
>
> s/protocted/protected/
>
> > + * sufficient to wait a grace period to ensure that no "ri->map == map"
> > + * exist. dev_map_free() removes the map from the list and then
>
> s/exist/exists/
Thank you.
>
> -Toke
Sebastian
