[dpdk-dev] [PATCH 4/4] net/bonding: prefer allmulti to promisc for LACP

[David Marchand <david.marchand@redhat.com>]

Rather than the global promiscuous mode on all slaves, let's try to use
allmulti.
To do this, we apply the same mechanism than for promiscuous and drop in
rx_burst.

When adding a slave, we first try to use allmulti, else we try
promiscuous.
Because of this, we must be able to handle allmulti on the bonding
interface, so the associated dev_ops is added with checks on which mode
has been applied on each slave.

Rather than add a new flag in the bond private structure, we can remove
promiscuous_en since ethdev already maintains this information.
When starting the bond device, there is no promisc/allmulti re-apply
as, again, ethdev does this itself.

On reception, we must inspect if the packets are multicast or unicast
and take a decision to drop based on which mode has been enabled on the
bonding interface.

Note: in the future, if we define an API so that we can add/remove mc
addresses to a port (rather than the current global set_mc_addr_list
API), we can refine this to only register the LACP multicast mac
address.

Signed-off-by: David Marchand <david.marchand@redhat.com>
---
 drivers/net/bonding/rte_eth_bond_8023ad.c         |  51 +++++++++-
 drivers/net/bonding/rte_eth_bond_8023ad_private.h |   7 ++
 drivers/net/bonding/rte_eth_bond_pmd.c            | 113 +++++++++++++++++-----
 drivers/net/bonding/rte_eth_bond_private.h        |   3 -
 4 files changed, 148 insertions(+), 26 deletions(-)

diff --git a/drivers/net/bonding/rte_eth_bond_8023ad.c b/drivers/net/bonding/rte_eth_bond_8023ad.c
index 5004898..adf6b7e 100644
--- a/drivers/net/bonding/rte_eth_bond_8023ad.c
+++ b/drivers/net/bonding/rte_eth_bond_8023ad.c
@@ -907,6 +907,49 @@
 			bond_mode_8023ad_periodic_cb, arg);
 }
 
+static int
+bond_mode_8023ad_register_lacp_mac(uint16_t slave_id)
+{
+	rte_eth_allmulticast_enable(slave_id);
+	if (rte_eth_allmulticast_get(slave_id)) {
+		RTE_BOND_LOG(DEBUG, "forced allmulti for port %u",
+			     slave_id);
+		bond_mode_8023ad_ports[slave_id].forced_rx_flags =
+				BOND_8023AD_FORCED_ALLMULTI;
+		return 0;
+	}
+
+	rte_eth_promiscuous_enable(slave_id);
+	if (rte_eth_promiscuous_get(slave_id)) {
+		RTE_BOND_LOG(DEBUG, "forced promiscuous for port %u",
+			     slave_id);
+		bond_mode_8023ad_ports[slave_id].forced_rx_flags =
+				BOND_8023AD_FORCED_PROMISC;
+		return 0;
+	}
+
+	return -1;
+}
+
+static void
+bond_mode_8023ad_unregister_lacp_mac(uint16_t slave_id)
+{
+	switch (bond_mode_8023ad_ports[slave_id].forced_rx_flags) {
+	case BOND_8023AD_FORCED_ALLMULTI:
+		RTE_BOND_LOG(DEBUG, "unset allmulti for port %u", slave_id);
+		rte_eth_allmulticast_disable(slave_id);
+		break;
+
+	case BOND_8023AD_FORCED_PROMISC:
+		RTE_BOND_LOG(DEBUG, "unset promisc for port %u", slave_id);
+		rte_eth_promiscuous_disable(slave_id);
+		break;
+
+	default:
+		break;
+	}
+}
+
 void
 bond_mode_8023ad_activate_slave(struct rte_eth_dev *bond_dev,
 				uint16_t slave_id)
@@ -948,7 +991,11 @@
 
 	/* use this port as agregator */
 	port->aggregator_port_id = slave_id;
-	rte_eth_promiscuous_enable(slave_id);
+
+	if (bond_mode_8023ad_register_lacp_mac(slave_id) < 0) {
+		RTE_BOND_LOG(WARNING, "slave %u is most likely broken and won't receive LACP packets",
+			     slave_id);
+	}
 
 	timer_cancel(&port->warning_timer);
 
@@ -1022,6 +1069,8 @@
 	old_partner_state = port->partner_state;
 	record_default(port);
 
+	bond_mode_8023ad_unregister_lacp_mac(slave_id);
+
 	/* If partner timeout state changes then disable timer */
 	if (!((old_partner_state ^ port->partner_state) &
 			STATE_LACP_SHORT_TIMEOUT))
diff --git a/drivers/net/bonding/rte_eth_bond_8023ad_private.h b/drivers/net/bonding/rte_eth_bond_8023ad_private.h
index f91902e..edda841 100644
--- a/drivers/net/bonding/rte_eth_bond_8023ad_private.h
+++ b/drivers/net/bonding/rte_eth_bond_8023ad_private.h
@@ -109,6 +109,13 @@ struct port {
 	uint16_t sm_flags;
 	enum rte_bond_8023ad_selection selected;
 
+	/** Indicates if either allmulti or promisc has been enforced on the
+	 * slave so that we can receive lacp packets
+	 */
+#define BOND_8023AD_FORCED_ALLMULTI (1 << 0)
+#define BOND_8023AD_FORCED_PROMISC (1 << 1)
+	uint8_t forced_rx_flags;
+
 	uint64_t current_while_timer;
 	uint64_t periodic_timer;
 	uint64_t wait_while_timer;
diff --git a/drivers/net/bonding/rte_eth_bond_pmd.c b/drivers/net/bonding/rte_eth_bond_pmd.c
index 9ef0717..07e19c4 100644
--- a/drivers/net/bonding/rte_eth_bond_pmd.c
+++ b/drivers/net/bonding/rte_eth_bond_pmd.c
@@ -274,7 +274,8 @@
 	uint16_t slave_count, idx;
 
 	uint8_t collecting;  /* current slave collecting status */
-	const uint8_t promisc = internals->promiscuous_en;
+	const uint8_t promisc = rte_eth_promiscuous_get(internals->port_id);
+	const uint8_t allmulti = rte_eth_allmulticast_get(internals->port_id);
 	uint8_t subtype;
 	uint16_t i;
 	uint16_t j;
@@ -314,8 +315,10 @@
 			/* Remove packet from array if:
 			 * - it is slow packet but no dedicated rxq is present,
 			 * - slave is not in collecting state,
-			 * - bonding interface is not in promiscuous mode and
-			 *   packet is not multicast and address does not match,
+			 * - bonding interface is not in promiscuous mode:
+			 *   - packet is unicast and address does not match,
+			 *   - packet is multicast and bonding interface
+			 *     is not in allmulti,
 			 */
 			if (unlikely(
 				(!dedicated_rxq &&
@@ -323,9 +326,11 @@
 						 bufs[j])) ||
 				!collecting ||
 				(!promisc &&
-				 !is_multicast_ether_addr(&hdr->d_addr) &&
-				 !is_same_ether_addr(bond_mac,
-						     &hdr->d_addr)))) {
+				 ((is_unicast_ether_addr(&hdr->d_addr) &&
+				   !is_same_ether_addr(bond_mac,
+						       &hdr->d_addr)) ||
+				  (!allmulti &&
+				   is_multicast_ether_addr(&hdr->d_addr)))))) {
 
 				if (hdr->ether_type == ether_type_slow_be) {
 					bond_mode_8023ad_handle_slow_pkt(
@@ -1924,10 +1929,6 @@ struct bwg_slave {
 		}
 	}
 
-	/* If bonded device is configure in promiscuous mode then re-apply config */
-	if (internals->promiscuous_en)
-		bond_ethdev_promiscuous_enable(eth_dev);
-
 	if (internals->mode == BONDING_MODE_8023AD) {
 		if (internals->mode4.dedicated_queues.enabled == 1) {
 			internals->mode4.dedicated_queues.rx_qid =
@@ -2445,18 +2446,17 @@ struct bwg_slave {
 	struct bond_dev_private *internals = eth_dev->data->dev_private;
 	int i;
 
-	internals->promiscuous_en = 1;
-
 	switch (internals->mode) {
 	/* Promiscuous mode is propagated to all slaves */
 	case BONDING_MODE_ROUND_ROBIN:
 	case BONDING_MODE_BALANCE:
 	case BONDING_MODE_BROADCAST:
-		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 */
 	case BONDING_MODE_8023AD:
+		for (i = 0; i < internals->slave_count; i++) {
+			uint16_t port_id = internals->slaves[i].port_id;
+
+			rte_eth_promiscuous_enable(port_id);
+		}
 		break;
 	/* Promiscuous mode is propagated only to primary slave */
 	case BONDING_MODE_ACTIVE_BACKUP:
@@ -2476,18 +2476,21 @@ struct bwg_slave {
 	struct bond_dev_private *internals = dev->data->dev_private;
 	int i;
 
-	internals->promiscuous_en = 0;
-
 	switch (internals->mode) {
 	/* Promiscuous mode is propagated to all slaves */
 	case BONDING_MODE_ROUND_ROBIN:
 	case BONDING_MODE_BALANCE:
 	case BONDING_MODE_BROADCAST:
-		for (i = 0; i < internals->slave_count; i++)
-			rte_eth_promiscuous_disable(internals->slaves[i].port_id);
-		break;
-	/* In mode4 promiscus mode is set managed when slave is added/removed */
 	case BONDING_MODE_8023AD:
+		for (i = 0; i < internals->slave_count; i++) {
+			uint16_t port_id = internals->slaves[i].port_id;
+
+			if (internals->mode == BONDING_MODE_8023AD &&
+			    bond_mode_8023ad_ports[port_id].forced_rx_flags ==
+					BOND_8023AD_FORCED_PROMISC)
+				continue;
+			rte_eth_promiscuous_disable(port_id);
+		}
 		break;
 	/* Promiscuous mode is propagated only to primary slave */
 	case BONDING_MODE_ACTIVE_BACKUP:
@@ -2502,6 +2505,70 @@ struct bwg_slave {
 }
 
 static void
+bond_ethdev_allmulticast_enable(struct rte_eth_dev *eth_dev)
+{
+	struct bond_dev_private *internals = eth_dev->data->dev_private;
+	int i;
+
+	switch (internals->mode) {
+	/* allmulti mode is propagated to all slaves */
+	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++) {
+			uint16_t port_id = internals->slaves[i].port_id;
+
+			rte_eth_allmulticast_enable(port_id);
+		}
+		break;
+	/* allmulti mode is propagated only to primary slave */
+	case BONDING_MODE_ACTIVE_BACKUP:
+	case BONDING_MODE_TLB:
+	case BONDING_MODE_ALB:
+	default:
+		/* Do not touch allmulti when there cannot be primary ports */
+		if (internals->slave_count == 0)
+			break;
+		rte_eth_allmulticast_enable(internals->current_primary_port);
+	}
+}
+
+static void
+bond_ethdev_allmulticast_disable(struct rte_eth_dev *eth_dev)
+{
+	struct bond_dev_private *internals = eth_dev->data->dev_private;
+	int i;
+
+	switch (internals->mode) {
+	/* allmulti mode is propagated to all slaves */
+	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++) {
+			uint16_t port_id = internals->slaves[i].port_id;
+
+			if (internals->mode == BONDING_MODE_8023AD &&
+			    bond_mode_8023ad_ports[port_id].forced_rx_flags ==
+					BOND_8023AD_FORCED_ALLMULTI)
+				continue;
+			rte_eth_allmulticast_disable(port_id);
+		}
+		break;
+	/* allmulti mode is propagated only to primary slave */
+	case BONDING_MODE_ACTIVE_BACKUP:
+	case BONDING_MODE_TLB:
+	case BONDING_MODE_ALB:
+	default:
+		/* Do not touch allmulti when there cannot be primary ports */
+		if (internals->slave_count == 0)
+			break;
+		rte_eth_allmulticast_disable(internals->current_primary_port);
+	}
+}
+
+static void
 bond_ethdev_delayed_lsc_propagation(void *arg)
 {
 	if (arg == NULL)
@@ -2893,6 +2960,8 @@ struct bwg_slave {
 	.stats_reset          = bond_ethdev_stats_reset,
 	.promiscuous_enable   = bond_ethdev_promiscuous_enable,
 	.promiscuous_disable  = bond_ethdev_promiscuous_disable,
+	.allmulticast_enable  = bond_ethdev_allmulticast_enable,
+	.allmulticast_disable = bond_ethdev_allmulticast_disable,
 	.reta_update          = bond_ethdev_rss_reta_update,
 	.reta_query           = bond_ethdev_rss_reta_query,
 	.rss_hash_update      = bond_ethdev_rss_hash_update,
diff --git a/drivers/net/bonding/rte_eth_bond_private.h b/drivers/net/bonding/rte_eth_bond_private.h
index 8afef39..4154e17 100644
--- a/drivers/net/bonding/rte_eth_bond_private.h
+++ b/drivers/net/bonding/rte_eth_bond_private.h
@@ -122,9 +122,6 @@ struct bond_dev_private {
 
 	uint8_t user_defined_mac;
 	/**< Flag for whether MAC address is user defined or not */
-	uint8_t promiscuous_en;
-	/**< Enabled/disable promiscuous mode on bonding device */
-
 
 	uint8_t link_status_polling_enabled;
 	uint32_t link_status_polling_interval_ms;
-- 
1.8.3.1