Linux Ethernet Bridging - Apr 2022 - [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 consistency
Of 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.

On 13/04/2022 12:51, Joachim Wiberg wrote:
> 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 consistency
> 
> Of 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.
Yes, I was thinking maybe local_rcv can become an enum with an exact reason for
the
local_rcv, so if it's > 0 do the local_rcv and br_pass_frame_up() then
can make a proper decision without passing all of the vars. I haven't tried
it,
so not sure if it's feasible. :)