Linux Ethernet Bridging - May 2022 - [Bridge] [PATCH V3 net-next 1/4] net: bridge: add fdb flag to extent locked port feature

On 24/05/2022 19:21, Hans Schultz wrote:
>>
>> Hi Hans,
>> So this approach has a fundamental problem, f->dst is changed
without any synchronization
>> you cannot rely on it and thus you cannot account for these entries
properly. We must be very
>> careful if we try to add any new synchronization not to affect
performance as well.
>> More below...
>>
>>> @@ -319,6 +326,9 @@ static void fdb_delete(struct net_bridge *br,
struct net_bridge_fdb_entry *f,
>>>  	if (test_bit(BR_FDB_STATIC, &f->flags))
>>>  		fdb_del_hw_addr(br, f->key.addr.addr);
>>>  
>>> +	if (test_bit(BR_FDB_ENTRY_LOCKED, &f->flags) &&
!test_bit(BR_FDB_OFFLOADED, &f->flags))
>>> +		atomic_dec(&f->dst->locked_entry_cnt);
>>
>> Sorry but you cannot do this for multiple reasons:
>>  - f->dst can be NULL
>>  - f->dst changes without any synchronization
>>  - there is no synchronization between fdb's flags and its ->dst
>>
>> Cheers,
>>  Nik
> 
> Hi Nik,
> 
> if a port is decoupled from the bridge, the locked entries would of
> course be invalid, so maybe if adding and removing a port is accounted
> for wrt locked entries and the count of locked entries, would that not
> work?
> 
> Best,
> Hans
Hi Hans,
Unfortunately you need the correct amount of locked entries per-port if you want
to limit their number per-port, instead of globally. So you need a consistent
fdb view with all its attributes when changing its dst in this case, which would
require new locking because you have multiple dependent struct fields and it
will
kill roaming/learning scalability. I don't think this use case is worth the
complexity it
will bring, so I'd suggest an alternative - you can monitor the number of
locked entries
per-port from a user-space agent and disable port learning or some similar
solution that
doesn't require any complex kernel changes. Is the limit a requirement to
add the feature?

I have an idea how to do it and to minimize the performance hit if it really is
needed
but it'll add a lot of complexity which I'd like to avoid if possible.

Cheers,
 Nik

On ons, maj 25, 2022 at 11:06, Nikolay Aleksandrov <razor at
blackwall.org> wrote:
> On 24/05/2022 19:21, Hans Schultz wrote:
>>>
>>> Hi Hans,
>>> So this approach has a fundamental problem, f->dst is changed
without any synchronization
>>> you cannot rely on it and thus you cannot account for these entries
properly. We must be very
>>> careful if we try to add any new synchronization not to affect
performance as well.
>>> More below...
>>>
>>>> @@ -319,6 +326,9 @@ static void fdb_delete(struct net_bridge
*br, struct net_bridge_fdb_entry *f,
>>>>  	if (test_bit(BR_FDB_STATIC, &f->flags))
>>>>  		fdb_del_hw_addr(br, f->key.addr.addr);
>>>>  
>>>> +	if (test_bit(BR_FDB_ENTRY_LOCKED, &f->flags)
&& !test_bit(BR_FDB_OFFLOADED, &f->flags))
>>>> +		atomic_dec(&f->dst->locked_entry_cnt);
>>>
>>> Sorry but you cannot do this for multiple reasons:
>>>  - f->dst can be NULL
>>>  - f->dst changes without any synchronization
>>>  - there is no synchronization between fdb's flags and its
->dst
>>>
>>> Cheers,
>>>  Nik
>> 
>> Hi Nik,
>> 
>> if a port is decoupled from the bridge, the locked entries would of
>> course be invalid, so maybe if adding and removing a port is accounted
>> for wrt locked entries and the count of locked entries, would that not
>> work?
>> 
>> Best,
>> Hans
>
> Hi Hans,
> Unfortunately you need the correct amount of locked entries per-port if you
want
> to limit their number per-port, instead of globally. So you need a
> consistent
Hi Nik,
the used dst is a port structure, so it is per-port and not globally.

Best,
Hans
> fdb view with all its attributes when changing its dst in this case, which
would
> require new locking because you have multiple dependent struct fields and
it will
> kill roaming/learning scalability. I don't think this use case is worth
the complexity it
> will bring, so I'd suggest an alternative - you can monitor the number
of locked entries
> per-port from a user-space agent and disable port learning or some similar
solution that
> doesn't require any complex kernel changes. Is the limit a requirement
to add the feature?
>
> I have an idea how to do it and to minimize the performance hit if it
really is needed
> but it'll add a lot of complexity which I'd like to avoid if
possible.
>
> Cheers,
>  Nik