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

[Chas Williams <3chas3@gmail.com>]

On Thu, Aug 2, 2018 at 10:24 AM Matan Azrad <matan@mellanox.com> wrote:

> Hi
>
> From: Doherty, Declan
> > On 02/08/2018 7:35 AM, Matan Azrad wrote:
> > > Hi Chas, Radu
> > >
> > > From: Chas Williams
> > >> On Wed, Aug 1, 2018 at 9:48 AM Radu Nicolau <radu.nicolau@intel.com>
> > >> wrote:
> > >>
> > >>>
> > >>>
> > >>> On 8/1/2018 2:34 PM, Chas Williams wrote:
> > >>>
> > >>>
> > >>>
> > >>> On Wed, Aug 1, 2018 at 9:04 AM Radu Nicolau <radu.nicolau@intel.com>
> > >>> wrote:
> > >>>
> > >>>> Update the bonding promiscuous mode enable/disable functions as to
> > >>>> propagate the change to all slaves instead of doing nothing; this
> > >>>> seems to be the correct behaviour according to the standard, and
> > >>>> also implemented in the linux network stack.
> > >>>>
> > >>>> Signed-off-by: Radu Nicolau <radu.nicolau@intel.com>
> > >>>> ---
> > >>>>   drivers/net/bonding/rte_eth_bond_pmd.c | 8 ++------
> > >>>>   1 file changed, 2 insertions(+), 6 deletions(-)
> > >>>>
> > >>>> diff --git a/drivers/net/bonding/rte_eth_bond_pmd.c
> > >>>> b/drivers/net/bonding/rte_eth_bond_pmd.c
> > >>>> index ad6e33f..16105cb 100644
> > >>>> --- a/drivers/net/bonding/rte_eth_bond_pmd.c
> > >>>> +++ b/drivers/net/bonding/rte_eth_bond_pmd.c
> > >>>> @@ -2617,12 +2617,10 @@ bond_ethdev_promiscuous_enable(struct
> > >>>> rte_eth_dev
> > >>>> *eth_dev)
> > >>>>          case BONDING_MODE_ROUND_ROBIN:
> > >>>>          case BONDING_MODE_BALANCE:
> > >>>>          case BONDING_MODE_BROADCAST:
> > >>>> +       case BONDING_MODE_8023AD:
> > >>>>                  for (i = 0; i < internals->slave_count; i++)
> > >>>>
> > >>>> rte_eth_promiscuous_enable(internals->slaves[i].port_id);
> > >>>>                  break;
> > >>>> -       /* In mode4 promiscus mode is managed when slave is
> > >> added/removed
> > >>>> */
> > >>>>
> > >>>
> > >>> This comment is true (and it appears it is always on in 802.3ad
> mode):
> > >>>
> > >>>          /* use this port as agregator */
> > >>>          port->aggregator_port_id = slave_id;
> > >>>          rte_eth_promiscuous_enable(slave_id);
> > >>>
> > >>> If we are going to do this here, we should probably get rid of it in
> > >>> the other location so that future readers aren't confused about
> > >>> which is the one doing the work.
> > >>>
> > >>> Since some adapters don't have group multicast support, we might
> > >>> already be in promiscuous anyway.  Turning off promiscuous for the
> > >>> bonding master might turn it off in the slaves where an application
> > >>> has already enabled it.
> > >>>
> > >>>
> > >>> The idea was to preserve the current behavior except for the
> > >>> explicit promiscuous disable/enable APIs; an application may disable
> > >>> the promiscuous mode on the bonding port and then enable it back,
> > >>> expecting it to propagate to the slaves.
> > >>>
> > >>
> > >> Yes, but an application doing that will break 802.3ad because
> > >> promiscuous mode is used to receive the LAG PDUs which are on a
> multicast
> > group.
> > >> That's why this code doesn't let you disable promiscuous when you are
> > >> in 802.3ad mode.
> > >>
> > >> If you want to do this it needs to be more complicated.  In 802.3ad,
> > >> you should try to add the multicast group to the slave interface.  If
> > >> that fails, turn on promisc mode for the slave.  Make note of it.
> > >> Later if bonding wants to enabled/disable promisc mode for the
> > >> slaves, it needs to check if that slaves needs to remain in promisc to
> > continue to get the LAG PDUs.
> > >
> > > I agree with Chas that this commit will hurt current LACP logic, but
> maybe
> > this is the time to open discussion about it:
> > > The current bonding implementation is greedy while it setting
> > > promiscuous automatically for LACP, The user asks LACP and he gets
> > promiscuous by the way.
> > >
> > > So if the user don't want promiscuous he must to disable it directly
> via slaves
> > ports and to allow LACP using rte_flow\flow
> > director\set_mc_addr_list\allmulti...
> > >
> > > I think the best way is to let the user to enable LACP  as he wants,
> directly via
> > slaves or by the bond promiscuous_enable API.
> > > For sure, it must be documented well.
> > >
> > > Matan.
> > >
> >
> > I'm thinking that default behavior should be that promiscuous mode
> should be
> > disabled by default, and that the bond port should fail to start if any
> of the slave
> > ports can't support subscription to the LACP multicast group. At this
> point the
> > user can decided to enable promiscuous mode on the bond port (and
> therefore
> > on all the slaves) and then start the bond. If we have slaves with
> different
> > configurations for multicast subscriptions or promiscuous mode
> enablement,
> > then there is potentially the opportunity for inconsistency in traffic
> depending
> > on which slaves are active.
>
> > Personally I would prefer that all configuration if possible is
> propagated
> > through the bond port. So if a user wants to use a port which doesn't
> support
> > multicast subscription then all ports in the bond need to be in
> promiscuous
> > mode, and the user needs to explicitly enable it through the bond port,
> that way
> > at least we can guarantee consist traffic irrespective of which ports in
> the bond
> > are active at any one time.
>
> That's exactly what I said :)
>
> 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?
>

Not a good idea.  The slave should do what it needs to go to get subscribed
to the multicast group.  For the user to make this decision ahead of time,
he would have to know that all the PMDs support the same method.  Forcing
more and more work on the caller of the API is not a solution.


>
> Matan.
>
>
>