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

[Matan Azrad <matan@mellanox.com>]

Hi Chas

From: Chas Williams
>On Mon, Aug 6, 2018 at 1:46 PM Matan Azrad <mailto:matan@mellanox.com> wrote:
>Hi Chas
>
>From: Chas Williams 
>>On Fri, Aug 3, 2018 at 1:47 AM Matan Azrad <mailto:mailto:matan@mellanox.com> wrote:
>>Hi Chas
>>
>> From: Chas Williams [mailto:mailto:mailto:mailto:3chas3@gmail.com] On Thu, Aug 2, 2018 at 1:33
>>> PM Matan Azrad <mailto:mailto: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.
>>
>>I think providing another option will be better as well.  However we disagree on the option.
>>If the PMD has no other way to subscribe the multicast group, it has to use promiscuous mode.
>
>>Yes, it is true but there are a lot of other and better options, promiscuous is greedy! Should be the last alternative to use. 
>
>Unfortunately, it's the only option implemented.

Yes, I know, I suggest to change it or at least not to make it worst.

>>Providing a list of options only makes life complicated for the developer and doesn't really
>>make any difference in the end results.
>
>>A big different, for example:
>>Let's say the bonding groups 2 devices that support rte_flow.
>>The user don't want neither promiscuous nor all multicast, he just want to get it's mac traffic + LACP MC group traffic,(a realistic use case)
>> if he has an option to tell to the bond PMD, please use rte_flow  to configure the specific LACP MC group it will be great.
>>Think how much work these applications should do in the current behavior.
>
>The bond PMD should already know how to do that itself.

The bond can do it with a lot of complexity, but again the user must know what the bond chose to be synchronized.
So, I think it's better that the user will define it because it is a traffic configuration (the same as promiscuous configuration - the user configures it)
>  Again, you are forcing more work on the user to ask them to select between the methods.

We can create a default option as now(promiscuous).

>>  For instance, if the least common denominator between the two PMDs is promiscuous mode,
>> you are going to be forced to run both in promiscuous mode
>>instead of selecting the best mode for each PMD.
>
>>In this case promiscuous is better,
>>Using a different configuration is worst and against the bonding PMD principle to get a consistent traffic from the slaves.
>>So, if one uses allmulti and one uses promiscuous the application may get an inconsistent traffic
>>and it may trigger a lot of problems and complications for some applications.
>
>Those applications should already have those problems.
>  I can make the counter
>argument that there are potentially applications relying on the broken behavior.

You right. So adding allmulticast will require changes in these applications. 

>We need to ignore those issues and fix this the "right" way.  The "right" way IMHO
>is the pass the least amount of traffic possible in each case.

Not in cost of an inconsistency, but looks like we are not agree here.