Hi Chas From: Chas Williams > On Mon, Aug 6, 2018 at 3:35 PM Matan Azrad <ma...@mellanox.com> > wrote: > > > > > > 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. > > > > I have recently run into this issue again with a device that doesn't support > promiscuous, but does let me subscribe to the appropriate multicast groups. > At this point, I am leaning toward adding another API call to the bonding API > so that the user can provide a callback to setup whatever they want on the > slaves. > The default setup routine would be enable promiscuous. > > Comments?
The bonding already allows to the users to do operations directly to the slaves(it exports the port ids - rte_eth_bond_slaves_get), so I don't understand why do you need a new API. The only change you need may be to add parameter to disable the promiscuous configuration in mode4.
