Re: [dpdk-dev] [PATCH] net/bonding: propagate promiscous mode in mode 4

[Matan Azrad <matan@mellanox.com>]

Hi Chas

 From: Chas Williams [mailto:3chas3@gmail.com] On Thu, Aug 2, 2018 at 1:33
> PM Matan Azrad <matan@mellanox.com> wrote:
> >
> > > I suggest to do it like next,
> > > To add one more parameter for LACP which means how to configure the
> > LACP MC group - lacp_mc_grp_conf:
> > > 1. rte_flow.
> > > 2. flow director.
> > > 3. add_mac.
> > > 3. set_mc_add_list
> > > 4. allmulti
> > > 5. promiscuous
> > > Maybe more... or less :)
> > >
> > > By this way the user decides how to do it, if it's fail for a slave,
> > > the salve
> > should be rejected.
> > > Conflict with another configuration(for example calling to
> > > promiscuous
> > disable while running LACP lacp_mc_grp_conf=5) should raise an error.
> > >
> > > What do you think?
> > >
> >
> > Supporting an LACP mc group specific configuration does make sense,
> > but I wonder if this could just be handled by default during slave add.
> >
> >
> > 1 and 2 are essentially the same hardware filtering offload mode, and
> > the other modes are irrelevant if this is enabled, it should not be
> > possible to add the slave if the bond is configured for this mode, or
> > possible to change the bond into this mode if an existing slave
> > doesn't support it.
> 
> >
> > 3 should be the default expected behavior, but
> > rte_eth_bond_slave_add() should fail if the slave being added doesn't
> > support either adding the MAC to the slave or adding the LACP MC address.
> >
> > Then the user could try either rte_eth_allmulticast_enable() on the
> > bond port and then try to add the slave again, which should fail if
> > existing slave didn't support allmulticast or the add slave would fail
> > again if the slave didn't support allmulticast  and finally just call
> > rte_eth_promiscuous_enable() on the bond and then try to re-add the
> > that slave.
> >
> > but maybe having a explicit configuration parameter would be better.
> 
> I don't sure you understand exactly what I’m suggesting here, again:
> I suggest to add a new parameter to the LACP mode called
> lacp_mc_grp_conf(or something else).
> So, when the user configures LACP (mode 4) it must to configure the
> lacp_mc_grp_conf parameter to one of the options I suggested.
> This parameter is not per slave means the bond PMD will use the selected
> option to configure the LACP MC group for all the slave ports.
> 
> If one of the slaves doesn't support the selected option it should be rejected.
> Conflicts should rais an error.
> 
> I agree here.  Yes, if a slave can't manage to subscribe to the multicast group,
> an error should be raised.  The only way for this to happen is that you don't
> have promisc support which is the ultimate fallback.

> The advantages are:
> The user knows which option is better to synchronize with his application.
> The user knows better than the bond PMD what is the slaves capabilities.
> All the slaves are configured by the same way - consistent traffic.
> 
> 
> It would be ideal if all the slaves would have the same features and
> capabilities.  There wasn't enforced before, so this would be a new restriction
> that would be less flexible than what we currently have.  That doesn't seem like
> an improvement.

> The bonding user probably doesn't care which mode is used.
> The bonding user just wants bonding to work.  He doesn't care about the details.   If I am writing
> an application with this proposed API, I need to make a list of adapters and
> what they support (and keep this up to date as DPDK evolves).  Ugh.

The applications commonly know what are the nics capabilities they work with.

I know at least an one big application which really suffering because the bond
configures promiscuous in mode 4 without the application asking (it's considered there as a bug in dpdk).
I think that providing  another option will be better.

So, providing to applications a list of options will ease the application life and may be big improvement
while not hurting the current behavior. 

Matan