On 26/01/2022 14:50, Toke Høiland-Jørgensen wrote:

quoted

Nikolay Aleksandrov [off-list ref] writes:

quoted

On 26/01/2022 13:27, Lorenzo Bianconi wrote:

quoted

quoted

On 24/01/2022 19:20, Lorenzo Bianconi wrote:

quoted

Similar to bpf_xdp_ct_lookup routine, introduce
br_fdb_find_port_from_ifindex unstable helper in order to accelerate
linux bridge with XDP. br_fdb_find_port_from_ifindex will perform a
lookup in the associated bridge fdb table and it will return the
output ifindex if the destination address is associated to a bridge
port or -ENODEV for BOM traffic or if lookup fails.

Signed-off-by: Lorenzo Bianconi <lorenzo@kernel.org>
---
 net/bridge/br.c         | 21 +++++++++++++
 net/bridge/br_fdb.c     | 67 +++++++++++++++++++++++++++++++++++------
 net/bridge/br_private.h | 12 ++++++++
 3 files changed, 91 insertions(+), 9 deletions(-)

Hi Lorenzo,

Hi Nikolay,

thx for the review.

quoted

Please CC bridge maintainers for bridge-related patches, I've added Roopa and the
bridge mailing list as well. Aside from that, the change is certainly interesting, I've been
thinking about a similar helper for some time now, few comments below.

yes, sorry for that. I figured it out after sending the series out.

quoted

Have you thought about the egress path and if by the current bridge state the packet would
be allowed to egress through the found port from the lookup? I'd guess you have to keep updating
the active ports list based on netlink events, but there's a lot of egress bridge logic that
either have to be duplicated or somehow synced. Check should_deliver() (br_forward.c) and later
egress stages, but I see how this is a good first step and perhaps we can build upon it.
There are a few possible solutions, but I haven't tried anything yet, most obvious being
yet another helper. :)

ack, right but I am bit worried about adding too much logic and slow down xdp
performances. I guess we can investigate first the approach proposed by Alexei
and then revaluate. Agree?

Sure, that approach sounds very interesting, but my point was that
bypassing the ingress and egress logic defeats most of the bridge
features. You just get an fdb hash table which you can build today
with ebpf without any changes to the kernel. :) You have multiple
states, flags and options for each port and each vlan which can change
dynamically based on external events (e.g. STP, config changes etc)
and they can affect forwarding even if the fdbs remain in the table.

To me, leveraging all this is precisely the reason to have BPF helpers
instead of just replicating state in BPF maps: it's very easy to do that
and show a nice speedup, and then once you get all the corner cases
covered that the in-kernel code already deals with, you've chipped away
at that speedup and spent a lot of time essentially re-writing the
battle-tested code already in the kernel.

So I think figuring out how to do the state sync is the right thing to
do; a second helper would be fine for this, IMO, but I'm not really
familiar enough with the bridge code to really have a qualified opinion.

-Toke

Right, sounds good to me. IMO it should be required in order to get a meaningful bridge
speedup, otherwise the solution is incomplete and you just do simple lookups that ignore
all of the state that could impact the forwarding decision.

ack, I agree but I need to review it first since I am not so familiar
with that codebase :)
Doing so we can compare this solution with the one proposed by Alexei.

Regards,
Lorenzo

Cheers,
 Nik