On 02/08/2018 3:24 PM, Matan Azrad 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.nico...@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.nico...@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.nico...@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 guess so, but it was the configuration directly via the slave port bit
which had me concerned, I think this needs to be managed directly from
the bond port, ideally they
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.
Matan.
