Hi Chas From: Chas Williams >On Mon, Aug 6, 2018 at 1:46 PM Matan Azrad <mailto:ma...@mellanox.com> wrote: >Hi Chas > >From: Chas Williams >>On Fri, Aug 3, 2018 at 1:47 AM Matan Azrad <mailto:mailto:ma...@mellanox.com> >>wrote: >>Hi Chas >> >> From: Chas Williams [mailto:mailto:mailto:mailto:3ch...@gmail.com] On Thu, >>Aug 2, 2018 at 1:33 >>> PM Matan Azrad <mailto:mailto:ma...@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.
