Linux Ethernet Bridging - Jan 2020 - [Bridge] [RFC net-next v3 03/10] net: bridge: mrp: Add MRP interface used by netlink

On 25.01.2020 16:20, Andrew Lunn wrote:
>EXTERNAL EMAIL: Do not click links or open attachments unless you know the
content is safe
>
>On Sat, Jan 25, 2020 at 12:37:26PM +0100, Horatiu Vultur wrote:
>> The 01/24/2020 18:43, Andrew Lunn wrote:
>> > EXTERNAL EMAIL: Do not click links or open attachments unless you
know the content is safe
>> >
>> > > br_mrp_flush - will flush the FDB.
>> >
>> > How does this differ from a normal bridge flush? I assume there is
a
>> > way for user space to flush the bridge FDB.
>>
>> Hi,
>>
>> If I seen corectly the normal bridge flush will clear the entire FDB
for
>> all the ports of the bridge. In this case it is require to clear FDB
>> entries only for the ring ports.
>
>Maybe it would be better to extend the current bridge netlink call to
>be able to pass an optional interface to be flushed?  I'm not sure it
>is a good idea to have two APIs doing very similar things.
I agree.

And when looking at this again, I start to think that we should have
extended the existing netlink interface with new commands, instead of
adding a generic netlink.

/Allan

The 01/25/2020 20:16, Allan W. Nielsen wrote:
> On 25.01.2020 16:20, Andrew Lunn wrote:
> > EXTERNAL EMAIL: Do not click links or open attachments unless you know
the content is safe
> > 
> > On Sat, Jan 25, 2020 at 12:37:26PM +0100, Horatiu Vultur wrote:
> > > The 01/24/2020 18:43, Andrew Lunn wrote:
> > > > EXTERNAL EMAIL: Do not click links or open attachments
unless you know the content is safe
> > > >
> > > > > br_mrp_flush - will flush the FDB.
> > > >
> > > > How does this differ from a normal bridge flush? I assume
there is a
> > > > way for user space to flush the bridge FDB.
> > > 
> > > Hi,
> > > 
> > > If I seen corectly the normal bridge flush will clear the entire
FDB for
> > > all the ports of the bridge. In this case it is require to clear
FDB
> > > entries only for the ring ports.
> > 
> > Maybe it would be better to extend the current bridge netlink call to
> > be able to pass an optional interface to be flushed?  I'm not sure
it
> > is a good idea to have two APIs doing very similar things.
> I agree.
I would look over this.
> 
> And when looking at this again, I start to think that we should have
> extended the existing netlink interface with new commands, instead of
> adding a generic netlink.
We could do also that. The main reason why I have added a new generic
netlink was that I thought it would be clearer what commands are for MRP
configuration. But if you think that we should go forward by extending
existing netlink interface, that is perfectly fine for me.
> 
> /Allan
> 
-- 
/Horatiu

On 26/01/2020 15:28, Horatiu Vultur wrote:
> The 01/25/2020 20:16, Allan W. Nielsen wrote:
>> On 25.01.2020 16:20, Andrew Lunn wrote:
>>> EXTERNAL EMAIL: Do not click links or open attachments unless you
know the content is safe
>>>
>>> On Sat, Jan 25, 2020 at 12:37:26PM +0100, Horatiu Vultur wrote:
>>>> The 01/24/2020 18:43, Andrew Lunn wrote:
>>>>> EXTERNAL EMAIL: Do not click links or open attachments
unless you know the content is safe
>>>>>
>>>>>> br_mrp_flush - will flush the FDB.
>>>>>
>>>>> How does this differ from a normal bridge flush? I assume
there is a
>>>>> way for user space to flush the bridge FDB.
>>>>
>>>> Hi,
>>>>
>>>> If I seen corectly the normal bridge flush will clear the
entire FDB for
>>>> all the ports of the bridge. In this case it is require to
clear FDB
>>>> entries only for the ring ports.
>>>
>>> Maybe it would be better to extend the current bridge netlink call
to
>>> be able to pass an optional interface to be flushed?  I'm not
sure it
>>> is a good idea to have two APIs doing very similar things.
>> I agree.
> I would look over this.
> 
There's already a way to flush FDBs per-port - IFLA_BRPORT_FLUSH.
>>
>> And when looking at this again, I start to think that we should have
>> extended the existing netlink interface with new commands, instead of
>> adding a generic netlink.
> We could do also that. The main reason why I have added a new generic
> netlink was that I thought it would be clearer what commands are for MRP
> configuration. But if you think that we should go forward by extending
> existing netlink interface, that is perfectly fine for me.
> 
>>
>> /Allan
>>
> 
I don't mind extending the current netlink interface but the bridge already
has
a huge (the largest) set of options and each time we add a new option we have
to adjust RTNL_MAX_TYPE. If you do decide to go this way maybe look into nesting
all the MRP options under one master MRP element into the bridge options,
example:
[IFLA_BR_MRP]
  [IFLA_BR_MRP_X]
  [IFLA_BR_MRP_Y]
  ...

To avoid increasing the stack usage and bumping the max rtnl type too much.

Cheers,
 Nik