Nikolay Aleksandrov
2022-Apr-12 18:27 UTC
[Bridge] [PATCH RFC net-next 01/13] net: bridge: add control of bum flooding to bridge itself
On 11/04/2022 16:38, Joachim Wiberg wrote:> The bridge itself is also a port, but unfortunately it does not (yet) > have a 'struct net_bridge_port'. However, in many cases we want to > treat it as a proper port so concessions have been made, e.g., NULL > port or host_joined attributes. > > This patch is an attempt to more of the same by adding support for > controlling flooding of unknown broadcast/unicast/multicast to the > bridge. Something we often also want to control in an offloaded > switching fabric. > > Signed-off-by: Joachim Wiberg <troglobit at gmail.com> > --- > net/bridge/br_device.c | 4 ++++ > net/bridge/br_input.c | 11 ++++++++--- > net/bridge/br_private.h | 3 +++ > 3 files changed, 15 insertions(+), 3 deletions(-) > > diff --git a/net/bridge/br_device.c b/net/bridge/br_device.c > index 8d6bab244c4a..0aa7d21ac82c 100644 > --- a/net/bridge/br_device.c > +++ b/net/bridge/br_device.c > @@ -526,6 +526,10 @@ void br_dev_setup(struct net_device *dev) > br->bridge_ageing_time = br->ageing_time = BR_DEFAULT_AGEING_TIME; > dev->max_mtu = ETH_MAX_MTU; > > + br_opt_toggle(br, BROPT_UNICAST_FLOOD, 1);This one must be false by default. It changes current default behaviour. Unknown unicast is not currently passed up to the bridge if the port is not in promisc mode, this will change it. You'll have to make it consistent with promisc (e.g. one way would be for promisc always to enable unicast flood and it won't be possible to be disabled while promisc).> + br_opt_toggle(br, BROPT_MCAST_FLOOD, 1); > + br_opt_toggle(br, BROPT_BCAST_FLOOD, 1);s/1/true/ for consistency> + > br_netfilter_rtable_init(br); > br_stp_timer_init(br); > br_multicast_init(br); > diff --git a/net/bridge/br_input.c b/net/bridge/br_input.c > index 196417859c4a..d439b876bdf5 100644 > --- a/net/bridge/br_input.c > +++ b/net/bridge/br_input.c > @@ -118,7 +118,8 @@ int br_handle_frame_finish(struct net *net, struct sock *sk, struct sk_buff *skb > /* by definition the broadcast is also a multicast address */ > if (is_broadcast_ether_addr(eth_hdr(skb)->h_dest)) { > pkt_type = BR_PKT_BROADCAST; > - local_rcv = true; > + if (br_opt_get(br, BROPT_BCAST_FLOOD)) > + local_rcv = true; > } else { > pkt_type = BR_PKT_MULTICAST; > if (br_multicast_rcv(&brmctx, &pmctx, vlan, skb, vid)) > @@ -161,12 +162,16 @@ int br_handle_frame_finish(struct net *net, struct sock *sk, struct sk_buff *skb > } > mcast_hit = true; > } else { > - local_rcv = true; > - br->dev->stats.multicast++; > + if (br_opt_get(br, BROPT_MCAST_FLOOD)) { > + local_rcv = true; > + br->dev->stats.multicast++; > + } > } > break; > case BR_PKT_UNICAST: > dst = br_fdb_find_rcu(br, eth_hdr(skb)->h_dest, vid); > + if (!dst && br_opt_get(br, BROPT_UNICAST_FLOOD)) > + local_rcv = true; > break;This adds new tests for all fast paths for host traffic, especially the port - port communication which is the most critical one. Please at least move the unicast test to the "else" block of "if (dst)" later. The other tests can be moved to host only code too, but would require bigger changes. Please try to keep the impact on the fast-path at minimum.> default: > break; > diff --git a/net/bridge/br_private.h b/net/bridge/br_private.h > index 18ccc3d5d296..683bd0ee4c64 100644 > --- a/net/bridge/br_private.h > +++ b/net/bridge/br_private.h > @@ -449,6 +449,9 @@ enum net_bridge_opts { > BROPT_VLAN_BRIDGE_BINDING, > BROPT_MCAST_VLAN_SNOOPING_ENABLED, > BROPT_MST_ENABLED, > + BROPT_UNICAST_FLOOD, > + BROPT_MCAST_FLOOD, > + BROPT_BCAST_FLOOD, > }; > > struct net_bridge {
Nikolay Aleksandrov
2022-Apr-12 20:29 UTC
[Bridge] [PATCH RFC net-next 01/13] net: bridge: add control of bum flooding to bridge itself
On 12/04/2022 21:27, Nikolay Aleksandrov wrote:> On 11/04/2022 16:38, Joachim Wiberg wrote: >> The bridge itself is also a port, but unfortunately it does not (yet) >> have a 'struct net_bridge_port'. However, in many cases we want to >> treat it as a proper port so concessions have been made, e.g., NULL >> port or host_joined attributes. >> >> This patch is an attempt to more of the same by adding support for >> controlling flooding of unknown broadcast/unicast/multicast to the >> bridge. Something we often also want to control in an offloaded >> switching fabric. >> >> Signed-off-by: Joachim Wiberg <troglobit at gmail.com> >> --- >> net/bridge/br_device.c | 4 ++++ >> net/bridge/br_input.c | 11 ++++++++--- >> net/bridge/br_private.h | 3 +++ >> 3 files changed, 15 insertions(+), 3 deletions(-) >> >> diff --git a/net/bridge/br_device.c b/net/bridge/br_device.c >> index 8d6bab244c4a..0aa7d21ac82c 100644 >> --- a/net/bridge/br_device.c >> +++ b/net/bridge/br_device.c >> @@ -526,6 +526,10 @@ void br_dev_setup(struct net_device *dev) >> br->bridge_ageing_time = br->ageing_time = BR_DEFAULT_AGEING_TIME; >> dev->max_mtu = ETH_MAX_MTU; >> >> + br_opt_toggle(br, BROPT_UNICAST_FLOOD, 1); > > This one must be false by default. It changes current default behaviour. > Unknown unicast is not currently passed up to the bridge if the port iss/port/bridge/ in promisc mode> not in promisc mode, this will change it. You'll have to make it consistent > with promisc (e.g. one way would be for promisc always to enable unicast flood > and it won't be possible to be disabled while promisc). > >> + br_opt_toggle(br, BROPT_MCAST_FLOOD, 1); >> + br_opt_toggle(br, BROPT_BCAST_FLOOD, 1); > > s/1/true/ for consistency > >> + >> br_netfilter_rtable_init(br); >> br_stp_timer_init(br); >> br_multicast_init(br); >> diff --git a/net/bridge/br_input.c b/net/bridge/br_input.c >> index 196417859c4a..d439b876bdf5 100644 >> --- a/net/bridge/br_input.c >> +++ b/net/bridge/br_input.c >> @@ -118,7 +118,8 @@ int br_handle_frame_finish(struct net *net, struct sock *sk, struct sk_buff *skb >> /* by definition the broadcast is also a multicast address */ >> if (is_broadcast_ether_addr(eth_hdr(skb)->h_dest)) { >> pkt_type = BR_PKT_BROADCAST; >> - local_rcv = true; >> + if (br_opt_get(br, BROPT_BCAST_FLOOD)) >> + local_rcv = true; >> } else { >> pkt_type = BR_PKT_MULTICAST; >> if (br_multicast_rcv(&brmctx, &pmctx, vlan, skb, vid)) >> @@ -161,12 +162,16 @@ int br_handle_frame_finish(struct net *net, struct sock *sk, struct sk_buff *skb >> } >> mcast_hit = true; >> } else { >> - local_rcv = true; >> - br->dev->stats.multicast++; >> + if (br_opt_get(br, BROPT_MCAST_FLOOD)) { >> + local_rcv = true; >> + br->dev->stats.multicast++; >> + } >> } >> break; >> case BR_PKT_UNICAST: >> dst = br_fdb_find_rcu(br, eth_hdr(skb)->h_dest, vid); >> + if (!dst && br_opt_get(br, BROPT_UNICAST_FLOOD)) >> + local_rcv = true; >> break; > > This adds new tests for all fast paths for host traffic, > especially the port - port communication which is the most critical one. > Please at least move the unicast test to the "else" block of "if (dst)" later. > > The other tests can be moved to host only code too, but would require bigger changes. > Please try to keep the impact on the fast-path at minimum. > >> default: >> break; >> diff --git a/net/bridge/br_private.h b/net/bridge/br_private.h >> index 18ccc3d5d296..683bd0ee4c64 100644 >> --- a/net/bridge/br_private.h >> +++ b/net/bridge/br_private.h >> @@ -449,6 +449,9 @@ enum net_bridge_opts { >> BROPT_VLAN_BRIDGE_BINDING, >> BROPT_MCAST_VLAN_SNOOPING_ENABLED, >> BROPT_MST_ENABLED, >> + BROPT_UNICAST_FLOOD, >> + BROPT_MCAST_FLOOD, >> + BROPT_BCAST_FLOOD, >> }; >> >> struct net_bridge { >
Joachim Wiberg
2022-Apr-13 09:51 UTC
[Bridge] [PATCH RFC net-next 01/13] net: bridge: add control of bum flooding to bridge itself
On Tue, Apr 12, 2022 at 21:27, Nikolay Aleksandrov <razor at blackwall.org> wrote:> On 11/04/2022 16:38, Joachim Wiberg wrote: >> @@ -526,6 +526,10 @@ void br_dev_setup(struct net_device *dev) >> br->bridge_ageing_time = br->ageing_time = BR_DEFAULT_AGEING_TIME; >> dev->max_mtu = ETH_MAX_MTU; >> + br_opt_toggle(br, BROPT_UNICAST_FLOOD, 1); > This one must be false by default. It changes current default behaviour. > Unknown unicast is not currently passed up to the bridge if the port is > not in promisc mode, this will change it. You'll have to make it consistent > with promisc (e.g. one way would be for promisc always to enable unicast flood > and it won't be possible to be disabled while promisc).Ouch, my bad! Will look into how to let this have as little impact as possible. I like your semantics there, promisc should always win.>> + br_opt_toggle(br, BROPT_MCAST_FLOOD, 1); >> + br_opt_toggle(br, BROPT_BCAST_FLOOD, 1); > > s/1/true/ for consistencyOf course, thanks!>> @@ -118,7 +118,8 @@ int br_handle_frame_finish(struct net *net, struct sock *sk, struct sk_buff *skb >> /* by definition the broadcast is also a multicast address */ >> if (is_broadcast_ether_addr(eth_hdr(skb)->h_dest)) { >> pkt_type = BR_PKT_BROADCAST; >> - local_rcv = true; >> + if (br_opt_get(br, BROPT_BCAST_FLOOD)) >> + local_rcv = true; >> } else { >> pkt_type = BR_PKT_MULTICAST; >> if (br_multicast_rcv(&brmctx, &pmctx, vlan, skb, vid)) >> @@ -161,12 +162,16 @@ int br_handle_frame_finish(struct net *net, struct sock *sk, struct sk_buff *skb >> } >> mcast_hit = true; >> } else { >> - local_rcv = true; >> - br->dev->stats.multicast++; >> + if (br_opt_get(br, BROPT_MCAST_FLOOD)) { >> + local_rcv = true; >> + br->dev->stats.multicast++; >> + } >> } >> break; >> case BR_PKT_UNICAST: >> dst = br_fdb_find_rcu(br, eth_hdr(skb)->h_dest, vid); >> + if (!dst && br_opt_get(br, BROPT_UNICAST_FLOOD)) >> + local_rcv = true; >> break; > > This adds new tests for all fast paths for host traffic, especially > the port - port communication which is the most critical one. Please > at least move the unicast test to the "else" block of "if (dst)" > later.OK, will fix!> The other tests can be moved to host only code too, but would require > bigger changes. Please try to keep the impact on the fast-path at > minimum.Interesting, you mean by speculatively setting local_rcv = true and postpone the decsion to br_pass_frame_up(), right? Yeah that would indeed be a bit more work.