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?
