[PATCH 0/7] net/bonding: fixes and LACP short timeout

[PATCH 0/7] net/bonding: fixes and LACP short timeout

[Robert Sanford]

This patchset makes the following changes to net/bonding:
- Clean up minor errors in spelling, whitespace, C++ wrappers, and
  comments.
- Replace directly overwriting of slave port's rte_eth_conf by copying
  it, but only updating it via rte_eth_dev_configure().
- Make minor changes to allocation of mbuf pool and rx/tx rings.
- Add support for enabling LACP short timeout, i.e., link partner can
  use fast periodic time interval between transmits.
- Add LACP short timeout to tests.
- Include bond_8023ad and bond_alb in doxygen.

Robert Sanford (7):
  net/bonding: fix typos and whitespace
  net/bonding: fix bonded dev configuring slave dev
  net/bonding: change mbuf pool and ring allocation
  net/bonding: support enabling LACP short timeout
  net/bonding: add LACP short timeout to tests
  net/bonding: add bond_8023ad and bond_alb to doc
  Remove self from Timers maintainers.

 MAINTAINERS                                   |  1 -
 app/test-pmd/cmdline.c                        | 81 ++++++++++++++++++++++-
 app/test/test_link_bonding_mode4.c            | 93 +++++++++++++++++++++++----
 doc/api/doxy-api-index.md                     |  2 +
 drivers/net/bonding/eth_bond_8023ad_private.h | 13 ++--
 drivers/net/bonding/rte_eth_bond_8023ad.c     | 64 ++++++++++++------
 drivers/net/bonding/rte_eth_bond_8023ad.h     | 18 ++++--
 drivers/net/bonding/rte_eth_bond_pmd.c        | 43 +++++++------
 8 files changed, 245 insertions(+), 70 deletions(-)

-- 
2.7.4

[PATCH 1/7] net/bonding: fix typos and whitespace

[Robert Sanford]

- Clean up minor typos in comments, strings, and private names.
- Fix whitespace in log messages and function formatting (open brace
  after a new line).
- Move closing C++ wrapper to the end of rte_eth_bond_8023ad.h.

Signed-off-by: Robert Sanford <rsanford@akamai.com>
---
 app/test-pmd/cmdline.c                        |  4 ++--
 app/test/test_link_bonding_mode4.c            | 28 +++++++++++++--------------
 drivers/net/bonding/eth_bond_8023ad_private.h | 10 +++++-----
 drivers/net/bonding/rte_eth_bond_8023ad.c     | 22 ++++++++++-----------
 drivers/net/bonding/rte_eth_bond_8023ad.h     | 15 +++++++-------
 drivers/net/bonding/rte_eth_bond_pmd.c        | 13 ++++++++-----
 6 files changed, 48 insertions(+), 44 deletions(-)

diff --git a/app/test-pmd/cmdline.c b/app/test-pmd/cmdline.c
index 6e10afe..9fd2c2a 100644
--- a/app/test-pmd/cmdline.c
+++ b/app/test-pmd/cmdline.c
@@ -630,8 +630,8 @@ static void cmd_help_long_parsed(void *parsed_result,
 			"set bonding mac_addr (port_id) (address)\n"
 			"	Set the MAC address of a bonded device.\n\n"
 
-			"set bonding mode IEEE802.3AD aggregator policy (port_id) (agg_name)"
-			"	Set Aggregation mode for IEEE802.3AD (mode 4)"
+			"set bonding mode IEEE802.3AD aggregator policy (port_id) (agg_name)\n"
+			"	Set Aggregation mode for IEEE802.3AD (mode 4)\n\n"
 
 			"set bonding balance_xmit_policy (port_id) (l2|l23|l34)\n"
 			"	Set the transmit balance policy for bonded device running in balance mode.\n\n"
diff --git a/app/test/test_link_bonding_mode4.c b/app/test/test_link_bonding_mode4.c
index 351129d..2be86d5 100644
--- a/app/test/test_link_bonding_mode4.c
+++ b/app/test/test_link_bonding_mode4.c
@@ -58,11 +58,11 @@ static const struct rte_ether_addr slave_mac_default = {
 	{ 0x00, 0xFF, 0x00, 0xFF, 0x00, 0x00 }
 };
 
-static const struct rte_ether_addr parnter_mac_default = {
+static const struct rte_ether_addr partner_mac_default = {
 	{ 0x22, 0xBB, 0xFF, 0xBB, 0x00, 0x00 }
 };
 
-static const struct rte_ether_addr parnter_system = {
+static const struct rte_ether_addr partner_system = {
 	{ 0x33, 0xFF, 0xBB, 0xFF, 0x00, 0x00 }
 };
 
@@ -76,7 +76,7 @@ struct slave_conf {
 	uint16_t port_id;
 	uint8_t bonded : 1;
 
-	uint8_t lacp_parnter_state;
+	uint8_t lacp_partner_state;
 };
 
 struct ether_vlan_hdr {
@@ -258,7 +258,7 @@ add_slave(struct slave_conf *slave, uint8_t start)
 	TEST_ASSERT_EQUAL(rte_is_same_ether_addr(&addr, &addr_check), 1,
 			"Slave MAC address is not as expected");
 
-	RTE_VERIFY(slave->lacp_parnter_state == 0);
+	RTE_VERIFY(slave->lacp_partner_state == 0);
 	return 0;
 }
 
@@ -288,7 +288,7 @@ remove_slave(struct slave_conf *slave)
 			test_params.bonded_port_id);
 
 	slave->bonded = 0;
-	slave->lacp_parnter_state = 0;
+	slave->lacp_partner_state = 0;
 	return 0;
 }
 
@@ -501,20 +501,20 @@ make_lacp_reply(struct slave_conf *slave, struct rte_mbuf *pkt)
 	slow_hdr = rte_pktmbuf_mtod(pkt, struct slow_protocol_frame *);
 
 	/* Change source address to partner address */
-	rte_ether_addr_copy(&parnter_mac_default, &slow_hdr->eth_hdr.src_addr);
+	rte_ether_addr_copy(&partner_mac_default, &slow_hdr->eth_hdr.src_addr);
 	slow_hdr->eth_hdr.src_addr.addr_bytes[RTE_ETHER_ADDR_LEN - 1] =
 		slave->port_id;
 
 	lacp = (struct lacpdu *) &slow_hdr->slow_protocol;
 	/* Save last received state */
-	slave->lacp_parnter_state = lacp->actor.state;
+	slave->lacp_partner_state = lacp->actor.state;
 	/* Change it into LACP replay by matching parameters. */
 	memcpy(&lacp->partner.port_params, &lacp->actor.port_params,
 		sizeof(struct port_params));
 
 	lacp->partner.state = lacp->actor.state;
 
-	rte_ether_addr_copy(&parnter_system, &lacp->actor.port_params.system);
+	rte_ether_addr_copy(&partner_system, &lacp->actor.port_params.system);
 	lacp->actor.state = STATE_LACP_ACTIVE |
 						STATE_SYNCHRONIZATION |
 						STATE_AGGREGATION |
@@ -580,7 +580,7 @@ bond_handshake_done(struct slave_conf *slave)
 	const uint8_t expected_state = STATE_LACP_ACTIVE | STATE_SYNCHRONIZATION |
 			STATE_AGGREGATION | STATE_COLLECTING | STATE_DISTRIBUTING;
 
-	return slave->lacp_parnter_state == expected_state;
+	return slave->lacp_partner_state == expected_state;
 }
 
 static unsigned
@@ -1165,7 +1165,7 @@ init_marker(struct rte_mbuf *pkt, struct slave_conf *slave)
 			&marker_hdr->eth_hdr.dst_addr);
 
 	/* Init source address */
-	rte_ether_addr_copy(&parnter_mac_default,
+	rte_ether_addr_copy(&partner_mac_default,
 			&marker_hdr->eth_hdr.src_addr);
 	marker_hdr->eth_hdr.src_addr.addr_bytes[RTE_ETHER_ADDR_LEN - 1] =
 		slave->port_id;
@@ -1353,7 +1353,7 @@ test_mode4_expired(void)
 	/* After test only expected slave should be in EXPIRED state */
 	FOR_EACH_SLAVE(i, slave) {
 		if (slave == exp_slave)
-			TEST_ASSERT(slave->lacp_parnter_state & STATE_EXPIRED,
+			TEST_ASSERT(slave->lacp_partner_state & STATE_EXPIRED,
 				"Slave %u should be in expired.", slave->port_id);
 		else
 			TEST_ASSERT_EQUAL(bond_handshake_done(slave), 1,
@@ -1392,7 +1392,7 @@ test_mode4_ext_ctrl(void)
 		},
 	};
 
-	rte_ether_addr_copy(&parnter_system, &src_mac);
+	rte_ether_addr_copy(&partner_system, &src_mac);
 	rte_ether_addr_copy(&slow_protocol_mac_addr, &dst_mac);
 
 	initialize_eth_header(&lacpdu.eth_hdr, &src_mac, &dst_mac,
@@ -1446,7 +1446,7 @@ test_mode4_ext_lacp(void)
 		},
 	};
 
-	rte_ether_addr_copy(&parnter_system, &src_mac);
+	rte_ether_addr_copy(&partner_system, &src_mac);
 	rte_ether_addr_copy(&slow_protocol_mac_addr, &dst_mac);
 
 	initialize_eth_header(&lacpdu.eth_hdr, &src_mac, &dst_mac,
@@ -1535,7 +1535,7 @@ check_environment(void)
 		if (port->bonded != 0)
 			env_state |= 0x04;
 
-		if (port->lacp_parnter_state != 0)
+		if (port->lacp_partner_state != 0)
 			env_state |= 0x08;
 
 		if (env_state != 0)
diff --git a/drivers/net/bonding/eth_bond_8023ad_private.h b/drivers/net/bonding/eth_bond_8023ad_private.h
index 9b5738a..e415f2f 100644
--- a/drivers/net/bonding/eth_bond_8023ad_private.h
+++ b/drivers/net/bonding/eth_bond_8023ad_private.h
@@ -20,7 +20,7 @@
 /** Maximum number of LACP packets from one slave queued in TX ring. */
 #define BOND_MODE_8023AX_SLAVE_TX_PKTS        1
 /**
- * Timeouts deffinitions (5.4.4 in 802.1AX documentation).
+ * Timeouts definitions (6.4.4 in 802.1AX documentation).
  */
 #define BOND_8023AD_FAST_PERIODIC_MS                900
 #define BOND_8023AD_SLOW_PERIODIC_MS              29000
@@ -34,7 +34,7 @@
 /**
  * Interval of showing warning message from state machines. All messages will
  * be held (and gathered together) to prevent flooding.
- * This is no parto of 802.1AX standard.
+ * This is not part of 802.1AX standard.
  */
 #define BOND_8023AD_WARNINGS_PERIOD_MS             1000
 
@@ -83,7 +83,7 @@
 #define PARTNER_STATE_SET(_p, _f) SET_FLAGS((_p)->partner_state, STATE_ ## _f)
 #define PARTNER_STATE_CLR(_p, _f) CLEAR_FLAGS((_p)->partner_state, STATE_ ## _f)
 
-/** Variables associated with each port (5.4.7 in 802.1AX documentation). */
+/** Variables associated with each port (6.4.7 in 802.1AX documentation). */
 struct port {
 	/**
 	 * The operational values of the Actor's state parameters. Bitmask
@@ -124,7 +124,7 @@ struct port {
 	uint64_t wait_while_timer;
 	uint64_t tx_machine_timer;
 	uint64_t tx_marker_timer;
-	/* Agregator parameters */
+	/* Aggregator parameters */
 	/** Used aggregator port ID */
 	uint16_t aggregator_port_id;
 
@@ -280,7 +280,7 @@ bond_mode_8023ad_activate_slave(struct rte_eth_dev *dev, uint16_t port_id);
 /**
  * @internal
  *
- * Denitializes and removes given slave from 802.1AX mode.
+ * Deinitializes and removes given slave from 802.1AX mode.
  *
  * @param dev       Bonded interface.
  * @param slave_num Position of slave in active_slaves array
diff --git a/drivers/net/bonding/rte_eth_bond_8023ad.c b/drivers/net/bonding/rte_eth_bond_8023ad.c
index ca50583..43231bc 100644
--- a/drivers/net/bonding/rte_eth_bond_8023ad.c
+++ b/drivers/net/bonding/rte_eth_bond_8023ad.c
@@ -207,15 +207,15 @@ show_warnings(uint16_t slave_id)
 	if (warnings & WRN_RX_QUEUE_FULL) {
 		RTE_BOND_LOG(DEBUG,
 			     "Slave %u: failed to enqueue LACP packet into RX ring.\n"
-			     "Receive and transmit functions must be invoked on bonded"
-			     "interface at least 10 times per second or LACP will notwork correctly",
+			     "Receive and transmit functions must be invoked on bonded\n"
+			     "interface at least 10 times per second or LACP will not work correctly",
 			     slave_id);
 	}
 
 	if (warnings & WRN_TX_QUEUE_FULL) {
 		RTE_BOND_LOG(DEBUG,
 			     "Slave %u: failed to enqueue LACP packet into TX ring.\n"
-			     "Receive and transmit functions must be invoked on bonded"
+			     "Receive and transmit functions must be invoked on bonded\n"
 			     "interface at least 10 times per second or LACP will not work correctly",
 			     slave_id);
 	}
@@ -250,7 +250,7 @@ record_default(struct port *port)
 
 /** Function handles rx state machine.
  *
- * This function implements Receive State Machine from point 5.4.12 in
+ * This function implements Receive State Machine from point 6.4.12 in
  * 802.1AX documentation. It should be called periodically.
  *
  * @param lacpdu		LACPDU received.
@@ -384,7 +384,7 @@ rx_machine(struct bond_dev_private *internals, uint16_t slave_id,
 /**
  * Function handles periodic tx state machine.
  *
- * Function implements Periodic Transmission state machine from point 5.4.13
+ * Function implements Periodic Transmission state machine from point 6.4.13
  * in 802.1AX documentation. It should be called periodically.
  *
  * @param port			Port to handle state machine.
@@ -446,7 +446,7 @@ periodic_machine(struct bond_dev_private *internals, uint16_t slave_id)
 /**
  * Function handles mux state machine.
  *
- * Function implements Mux Machine from point 5.4.15 in 802.1AX documentation.
+ * Function implements Mux Machine from point 6.4.15 in 802.1AX documentation.
  * It should be called periodically.
  *
  * @param port			Port to handle state machine.
@@ -549,7 +549,7 @@ mux_machine(struct bond_dev_private *internals, uint16_t slave_id)
 /**
  * Function handles transmit state machine.
  *
- * Function implements Transmit Machine from point 5.4.16 in 802.1AX
+ * Function implements Transmit Machine from point 6.4.16 in 802.1AX
  * documentation.
  *
  * @param port
@@ -1051,14 +1051,14 @@ bond_mode_8023ad_activate_slave(struct rte_eth_dev *bond_dev,
 	struct bond_tx_queue *bd_tx_q;
 	uint16_t q_id;
 
-	/* Given slave mus not be in active list */
+	/* Given slave must not be in active list. */
 	RTE_ASSERT(find_slave_by_id(internals->active_slaves,
 	internals->active_slave_count, slave_id) == internals->active_slave_count);
 	RTE_SET_USED(internals); /* used only for assert when enabled */
 
 	memcpy(&port->actor, &initial, sizeof(struct port_params));
-	/* Standard requires that port ID must be grater than 0.
-	 * Add 1 do get corresponding port_number */
+	/* Standard requires that port ID must be greater than 0.
+	 * Add 1 to get corresponding port_number. */
 	port->actor.port_number = rte_cpu_to_be_16(slave_id + 1);
 
 	memcpy(&port->partner, &initial, sizeof(struct port_params));
@@ -1069,7 +1069,7 @@ bond_mode_8023ad_activate_slave(struct rte_eth_dev *bond_dev,
 	port->partner_state = STATE_LACP_ACTIVE | STATE_AGGREGATION;
 	port->sm_flags = SM_FLAGS_BEGIN;
 
-	/* use this port as agregator */
+	/* Use this port as aggregator. */
 	port->aggregator_port_id = slave_id;
 
 	if (bond_mode_8023ad_register_lacp_mac(slave_id) < 0) {
diff --git a/drivers/net/bonding/rte_eth_bond_8023ad.h b/drivers/net/bonding/rte_eth_bond_8023ad.h
index 11a71a5..7e9a018 100644
--- a/drivers/net/bonding/rte_eth_bond_8023ad.h
+++ b/drivers/net/bonding/rte_eth_bond_8023ad.h
@@ -68,7 +68,7 @@ struct port_params {
 	struct rte_ether_addr system;
 	/**< System ID - Slave MAC address, same as bonding MAC address */
 	uint16_t key;
-	/**< Speed information (implementation dependednt) and duplex. */
+	/**< Speed information (implementation dependent) and duplex. */
 	uint16_t port_priority;
 	/**< Priority of this (unused in current implementation) */
 	uint16_t port_number;
@@ -83,7 +83,7 @@ struct lacpdu_actor_partner_params {
 	uint8_t reserved_3[3];
 } __rte_packed __rte_aligned(2);
 
-/** LACPDU structure (5.4.2 in 802.1AX documentation). */
+/** LACPDU structure (6.4.2 in 802.1AX documentation). */
 struct lacpdu {
 	uint8_t subtype;
 	uint8_t version_number;
@@ -153,7 +153,7 @@ struct rte_eth_bond_8023ad_slave_info {
 /**
  * @internal
  *
- * Function returns current configuration of 802.3AX mode.
+ * Function returns current configuration of 802.1AX mode.
  *
  * @param port_id   Bonding device id
  * @param conf		Pointer to timeout structure.
@@ -197,10 +197,6 @@ int
 rte_eth_bond_8023ad_slave_info(uint16_t port_id, uint16_t slave_id,
 		struct rte_eth_bond_8023ad_slave_info *conf);
 
-#ifdef __cplusplus
-}
-#endif
-
 /**
  * Configure a slave port to start collecting.
  *
@@ -331,4 +327,9 @@ rte_eth_bond_8023ad_agg_selection_get(uint16_t port_id);
 int
 rte_eth_bond_8023ad_agg_selection_set(uint16_t port_id,
 		enum rte_bond_8023ad_agg_selection agg_selection);
+
+#ifdef __cplusplus
+}
+#endif
+
 #endif /* RTE_ETH_BOND_8023AD_H_ */
diff --git a/drivers/net/bonding/rte_eth_bond_pmd.c b/drivers/net/bonding/rte_eth_bond_pmd.c
index 84f4900..f6003b0 100644
--- a/drivers/net/bonding/rte_eth_bond_pmd.c
+++ b/drivers/net/bonding/rte_eth_bond_pmd.c
@@ -158,7 +158,8 @@ const struct rte_flow_attr flow_attr_8023ad = {
 
 int
 bond_ethdev_8023ad_flow_verify(struct rte_eth_dev *bond_dev,
-		uint16_t slave_port) {
+		uint16_t slave_port)
+{
 	struct rte_eth_dev_info slave_info;
 	struct rte_flow_error error;
 	struct bond_dev_private *internals = bond_dev->data->dev_private;
@@ -207,7 +208,8 @@ bond_ethdev_8023ad_flow_verify(struct rte_eth_dev *bond_dev,
 }
 
 int
-bond_8023ad_slow_pkt_hw_filter_supported(uint16_t port_id) {
+bond_8023ad_slow_pkt_hw_filter_supported(uint16_t port_id)
+{
 	struct rte_eth_dev *bond_dev = &rte_eth_devices[port_id];
 	struct bond_dev_private *internals = bond_dev->data->dev_private;
 	struct rte_eth_dev_info bond_info;
@@ -240,8 +242,8 @@ bond_8023ad_slow_pkt_hw_filter_supported(uint16_t port_id) {
 }
 
 int
-bond_ethdev_8023ad_flow_set(struct rte_eth_dev *bond_dev, uint16_t slave_port) {
-
+bond_ethdev_8023ad_flow_set(struct rte_eth_dev *bond_dev, uint16_t slave_port)
+{
 	struct rte_flow_error error;
 	struct bond_dev_private *internals = bond_dev->data->dev_private;
 	struct rte_flow_action_queue lacp_queue_conf = {
@@ -809,7 +811,8 @@ struct bwg_slave {
 };
 
 void
-bond_tlb_activate_slave(struct bond_dev_private *internals) {
+bond_tlb_activate_slave(struct bond_dev_private *internals)
+{
 	int i;
 
 	for (i = 0; i < internals->active_slave_count; i++) {
-- 
2.7.4

[PATCH 2/7] net/bonding: fix bonded dev configuring slave dev

[Robert Sanford]

- Replace directly overwriting of slave port's private rte_eth_conf
  by copying it, and then updating it via rte_eth_dev_configure().

Signed-off-by: Robert Sanford <rsanford@akamai.com>
---
 drivers/net/bonding/rte_eth_bond_pmd.c | 30 ++++++++++++++++--------------
 1 file changed, 16 insertions(+), 14 deletions(-)

diff --git a/drivers/net/bonding/rte_eth_bond_pmd.c b/drivers/net/bonding/rte_eth_bond_pmd.c
index f6003b0..8cce0aa 100644
--- a/drivers/net/bonding/rte_eth_bond_pmd.c
+++ b/drivers/net/bonding/rte_eth_bond_pmd.c
@@ -1691,6 +1691,7 @@ slave_configure(struct rte_eth_dev *bonded_eth_dev,
 	struct rte_flow_error flow_error;
 
 	struct bond_dev_private *internals = bonded_eth_dev->data->dev_private;
+	struct rte_eth_conf dev_conf;
 
 	/* Stop slave */
 	errval = rte_eth_dev_stop(slave_eth_dev->data->port_id);
@@ -1698,34 +1699,36 @@ slave_configure(struct rte_eth_dev *bonded_eth_dev,
 		RTE_BOND_LOG(ERR, "rte_eth_dev_stop: port %u, err (%d)",
 			     slave_eth_dev->data->port_id, errval);
 
+	/* Start with a copy of slave's current rte_eth_conf. */
+	dev_conf = slave_eth_dev->data->dev_conf;
+	dev_conf.rx_adv_conf.rss_conf.rss_key = NULL;
+
 	/* Enable interrupts on slave device if supported */
-	if (slave_eth_dev->data->dev_flags & RTE_ETH_DEV_INTR_LSC)
-		slave_eth_dev->data->dev_conf.intr_conf.lsc = 1;
+	dev_conf.intr_conf.lsc = 
+		(slave_eth_dev->data->dev_flags & RTE_ETH_DEV_INTR_LSC) ? 1 : 0;
 
 	/* If RSS is enabled for bonding, try to enable it for slaves  */
 	if (bonded_eth_dev->data->dev_conf.rxmode.mq_mode & RTE_ETH_MQ_RX_RSS_FLAG) {
 		/* rss_key won't be empty if RSS is configured in bonded dev */
-		slave_eth_dev->data->dev_conf.rx_adv_conf.rss_conf.rss_key_len =
-					internals->rss_key_len;
-		slave_eth_dev->data->dev_conf.rx_adv_conf.rss_conf.rss_key =
-					internals->rss_key;
+		dev_conf.rx_adv_conf.rss_conf.rss_key_len =
+				internals->rss_key_len;
+		dev_conf.rx_adv_conf.rss_conf.rss_key = internals->rss_key;
 
-		slave_eth_dev->data->dev_conf.rx_adv_conf.rss_conf.rss_hf =
+		dev_conf.rx_adv_conf.rss_conf.rss_hf =
 				bonded_eth_dev->data->dev_conf.rx_adv_conf.rss_conf.rss_hf;
-		slave_eth_dev->data->dev_conf.rxmode.mq_mode =
+		dev_conf.rxmode.mq_mode =
 				bonded_eth_dev->data->dev_conf.rxmode.mq_mode;
 	}
 
 	if (bonded_eth_dev->data->dev_conf.rxmode.offloads &
 			RTE_ETH_RX_OFFLOAD_VLAN_FILTER)
-		slave_eth_dev->data->dev_conf.rxmode.offloads |=
+		dev_conf.rxmode.offloads |=
 				RTE_ETH_RX_OFFLOAD_VLAN_FILTER;
 	else
-		slave_eth_dev->data->dev_conf.rxmode.offloads &=
+		dev_conf.rxmode.offloads &=
 				~RTE_ETH_RX_OFFLOAD_VLAN_FILTER;
 
-	slave_eth_dev->data->dev_conf.rxmode.mtu =
-			bonded_eth_dev->data->dev_conf.rxmode.mtu;
+	dev_conf.rxmode.mtu = bonded_eth_dev->data->dev_conf.rxmode.mtu;
 
 	nb_rx_queues = bonded_eth_dev->data->nb_rx_queues;
 	nb_tx_queues = bonded_eth_dev->data->nb_tx_queues;
@@ -1747,8 +1750,7 @@ slave_configure(struct rte_eth_dev *bonded_eth_dev,
 
 	/* Configure device */
 	errval = rte_eth_dev_configure(slave_eth_dev->data->port_id,
-			nb_rx_queues, nb_tx_queues,
-			&(slave_eth_dev->data->dev_conf));
+			nb_rx_queues, nb_tx_queues, &dev_conf);
 	if (errval != 0) {
 		RTE_BOND_LOG(ERR, "Cannot configure slave device: port %u, err (%d)",
 				slave_eth_dev->data->port_id, errval);
-- 
2.7.4

[PATCH 3/7] net/bonding: change mbuf pool and ring allocation

[Robert Sanford]

- Turn off mbuf pool caching to avoid mbufs lingering in pool caches.
  At most, we transmit one LACPDU per second, per port.
- Fix calculation of ring sizes, taking into account that a ring of
  size N holds up to N-1 items.

Signed-off-by: Robert Sanford <rsanford@akamai.com>
---
 drivers/net/bonding/rte_eth_bond_8023ad.c | 14 ++++++++------
 1 file changed, 8 insertions(+), 6 deletions(-)

diff --git a/drivers/net/bonding/rte_eth_bond_8023ad.c b/drivers/net/bonding/rte_eth_bond_8023ad.c
index 43231bc..83d3938 100644
--- a/drivers/net/bonding/rte_eth_bond_8023ad.c
+++ b/drivers/net/bonding/rte_eth_bond_8023ad.c
@@ -1101,9 +1101,7 @@ bond_mode_8023ad_activate_slave(struct rte_eth_dev *bond_dev,
 	}
 
 	snprintf(mem_name, RTE_DIM(mem_name), "slave_port%u_pool", slave_id);
-	port->mbuf_pool = rte_pktmbuf_pool_create(mem_name, total_tx_desc,
-		RTE_MEMPOOL_CACHE_MAX_SIZE >= 32 ?
-			32 : RTE_MEMPOOL_CACHE_MAX_SIZE,
+	port->mbuf_pool = rte_pktmbuf_pool_create(mem_name, total_tx_desc, 0,
 		0, element_size, socket_id);
 
 	/* Any memory allocation failure in initialization is critical because
@@ -1113,19 +1111,23 @@ bond_mode_8023ad_activate_slave(struct rte_eth_dev *bond_dev,
 			slave_id, mem_name, rte_strerror(rte_errno));
 	}
 
+	/* Add one extra because ring reserves one. */
 	snprintf(mem_name, RTE_DIM(mem_name), "slave_%u_rx", slave_id);
 	port->rx_ring = rte_ring_create(mem_name,
-			rte_align32pow2(BOND_MODE_8023AX_SLAVE_RX_PKTS), socket_id, 0);
+			rte_align32pow2(BOND_MODE_8023AX_SLAVE_RX_PKTS + 1),
+			socket_id, 0);
 
 	if (port->rx_ring == NULL) {
 		rte_panic("Slave %u: Failed to create rx ring '%s': %s\n", slave_id,
 			mem_name, rte_strerror(rte_errno));
 	}
 
-	/* TX ring is at least one pkt longer to make room for marker packet. */
+	/* TX ring is at least one pkt longer to make room for marker packet.
+	 * Add one extra because ring reserves one. */
 	snprintf(mem_name, RTE_DIM(mem_name), "slave_%u_tx", slave_id);
 	port->tx_ring = rte_ring_create(mem_name,
-			rte_align32pow2(BOND_MODE_8023AX_SLAVE_TX_PKTS + 1), socket_id, 0);
+			rte_align32pow2(BOND_MODE_8023AX_SLAVE_TX_PKTS + 2),
+			socket_id, 0);
 
 	if (port->tx_ring == NULL) {
 		rte_panic("Slave %u: Failed to create tx ring '%s': %s\n", slave_id,
-- 
2.7.4

[PATCH 4/7] net/bonding: support enabling LACP short timeout

[Robert Sanford]

- Add support for enabling LACP short timeout, i.e., link partner can
  use fast periodic time interval between transmits.

Signed-off-by: Robert Sanford <rsanford@akamai.com>
---
 drivers/net/bonding/eth_bond_8023ad_private.h |  3 ++-
 drivers/net/bonding/rte_eth_bond_8023ad.c     | 28 +++++++++++++++++++++++----
 drivers/net/bonding/rte_eth_bond_8023ad.h     |  3 +++
 3 files changed, 29 insertions(+), 5 deletions(-)

diff --git a/drivers/net/bonding/eth_bond_8023ad_private.h b/drivers/net/bonding/eth_bond_8023ad_private.h
index e415f2f..e1a7207 100644
--- a/drivers/net/bonding/eth_bond_8023ad_private.h
+++ b/drivers/net/bonding/eth_bond_8023ad_private.h
@@ -159,7 +159,6 @@ struct mode8023ad_private {
 	uint64_t rx_marker_timeout;
 	uint64_t update_timeout_us;
 	rte_eth_bond_8023ad_ext_slowrx_fn slowrx_cb;
-	uint8_t external_sm;
 	struct rte_ether_addr mac_addr;
 
 	struct rte_eth_link slave_link;
@@ -178,6 +177,8 @@ struct mode8023ad_private {
 		uint16_t tx_qid;
 	} dedicated_queues;
 	enum rte_bond_8023ad_agg_selection agg_selection;
+	uint8_t short_timeout_enabled : 1;
+	uint8_t short_timeout_updated : 1;
 };
 
 /**
diff --git a/drivers/net/bonding/rte_eth_bond_8023ad.c b/drivers/net/bonding/rte_eth_bond_8023ad.c
index 83d3938..93fbf39 100644
--- a/drivers/net/bonding/rte_eth_bond_8023ad.c
+++ b/drivers/net/bonding/rte_eth_bond_8023ad.c
@@ -868,10 +868,10 @@ bond_mode_8023ad_periodic_cb(void *arg)
 	struct rte_eth_link link_info;
 	struct rte_ether_addr slave_addr;
 	struct rte_mbuf *lacp_pkt = NULL;
+	uint8_t short_timeout_updated = internals->mode4.short_timeout_updated;
 	uint16_t slave_id;
 	uint16_t i;
 
-
 	/* Update link status on each port */
 	for (i = 0; i < internals->active_slave_count; i++) {
 		uint16_t key;
@@ -916,6 +916,13 @@ bond_mode_8023ad_periodic_cb(void *arg)
 		slave_id = internals->active_slaves[i];
 		port = &bond_mode_8023ad_ports[slave_id];
 
+		if (short_timeout_updated) {
+			if (internals->mode4.short_timeout_enabled)
+				ACTOR_STATE_SET(port, LACP_SHORT_TIMEOUT);
+			else
+				ACTOR_STATE_CLR(port, LACP_SHORT_TIMEOUT);
+		}
+
 		if ((port->actor.key &
 				rte_cpu_to_be_16(BOND_LINK_FULL_DUPLEX_KEY)) == 0) {
 
@@ -960,6 +967,9 @@ bond_mode_8023ad_periodic_cb(void *arg)
 		show_warnings(slave_id);
 	}
 
+	if (short_timeout_updated)
+		internals->mode4.short_timeout_updated = 0;
+
 	rte_eal_alarm_set(internals->mode4.update_timeout_us,
 			bond_mode_8023ad_periodic_cb, arg);
 }
@@ -1054,7 +1064,6 @@ bond_mode_8023ad_activate_slave(struct rte_eth_dev *bond_dev,
 	/* Given slave must not be in active list. */
 	RTE_ASSERT(find_slave_by_id(internals->active_slaves,
 	internals->active_slave_count, slave_id) == internals->active_slave_count);
-	RTE_SET_USED(internals); /* used only for assert when enabled */
 
 	memcpy(&port->actor, &initial, sizeof(struct port_params));
 	/* Standard requires that port ID must be greater than 0.
@@ -1065,7 +1074,9 @@ bond_mode_8023ad_activate_slave(struct rte_eth_dev *bond_dev,
 	memcpy(&port->partner_admin, &initial, sizeof(struct port_params));
 
 	/* default states */
-	port->actor_state = STATE_AGGREGATION | STATE_LACP_ACTIVE | STATE_DEFAULTED;
+	port->actor_state = STATE_AGGREGATION | STATE_LACP_ACTIVE |
+		STATE_DEFAULTED | (internals->mode4.short_timeout_enabled ?
+		STATE_LACP_SHORT_TIMEOUT : 0);
 	port->partner_state = STATE_LACP_ACTIVE | STATE_AGGREGATION;
 	port->sm_flags = SM_FLAGS_BEGIN;
 
@@ -1213,6 +1224,7 @@ bond_mode_8023ad_conf_get(struct rte_eth_dev *dev,
 	struct mode8023ad_private *mode4 = &internals->mode4;
 	uint64_t ms_ticks = rte_get_tsc_hz() / 1000;
 
+	memset(conf, 0, sizeof(*conf));
 	conf->fast_periodic_ms = mode4->fast_periodic_timeout / ms_ticks;
 	conf->slow_periodic_ms = mode4->slow_periodic_timeout / ms_ticks;
 	conf->short_timeout_ms = mode4->short_timeout / ms_ticks;
@@ -1223,6 +1235,7 @@ bond_mode_8023ad_conf_get(struct rte_eth_dev *dev,
 	conf->rx_marker_period_ms = mode4->rx_marker_timeout / ms_ticks;
 	conf->slowrx_cb = mode4->slowrx_cb;
 	conf->agg_selection = mode4->agg_selection;
+	conf->lacp_timeout_control = mode4->short_timeout_enabled;
 }
 
 static void
@@ -1238,6 +1251,7 @@ bond_mode_8023ad_conf_get_default(struct rte_eth_bond_8023ad_conf *conf)
 	conf->update_timeout_ms = BOND_MODE_8023AX_UPDATE_TIMEOUT_MS;
 	conf->slowrx_cb = NULL;
 	conf->agg_selection = AGG_STABLE;
+	conf->lacp_timeout_control = 0;
 }
 
 static void
@@ -1278,6 +1292,11 @@ bond_mode_8023ad_setup(struct rte_eth_dev *dev,
 	mode4->slowrx_cb = conf->slowrx_cb;
 	mode4->agg_selection = AGG_STABLE;
 
+	if (mode4->short_timeout_enabled != conf->lacp_timeout_control) {
+		mode4->short_timeout_enabled = conf->lacp_timeout_control;
+		mode4->short_timeout_updated = 1;
+	}
+
 	if (dev->data->dev_started)
 		bond_mode_8023ad_start(dev);
 }
@@ -1482,7 +1501,8 @@ bond_8023ad_setup_validate(uint16_t port_id,
 				conf->aggregate_wait_timeout_ms == 0 ||
 				conf->tx_period_ms == 0 ||
 				conf->rx_marker_period_ms == 0 ||
-				conf->update_timeout_ms == 0) {
+				conf->update_timeout_ms == 0 ||
+				conf->lacp_timeout_control > 1) {
 			RTE_BOND_LOG(ERR, "given mode 4 configuration is invalid");
 			return -EINVAL;
 		}
diff --git a/drivers/net/bonding/rte_eth_bond_8023ad.h b/drivers/net/bonding/rte_eth_bond_8023ad.h
index 7e9a018..87f6b2f 100644
--- a/drivers/net/bonding/rte_eth_bond_8023ad.h
+++ b/drivers/net/bonding/rte_eth_bond_8023ad.h
@@ -139,6 +139,9 @@ struct rte_eth_bond_8023ad_conf {
 	uint32_t update_timeout_ms;
 	rte_eth_bond_8023ad_ext_slowrx_fn slowrx_cb;
 	enum rte_bond_8023ad_agg_selection agg_selection;
+	uint8_t lacp_timeout_control;
+	/**< LACPDU.Actor_State.LACP_Timeout flag: 0=Long 1=Short. */
+	uint8_t reserved_8s[3];
 };
 
 struct rte_eth_bond_8023ad_slave_info {
-- 
2.7.4

[PATCH 5/7] net/bonding: add LACP short timeout to tests

[Robert Sanford]

- Add "set bonding lacp timeout_ctrl <port_id> on|off" to testpmd.
- Add "test_mode4_lacp_timeout_control" to app/test.

Signed-off-by: Robert Sanford <rsanford@akamai.com>
---
 app/test-pmd/cmdline.c             | 77 ++++++++++++++++++++++++++++++++++++++
 app/test/test_link_bonding_mode4.c | 65 ++++++++++++++++++++++++++++++++
 2 files changed, 142 insertions(+)

diff --git a/app/test-pmd/cmdline.c b/app/test-pmd/cmdline.c
index 9fd2c2a..b0c2fb4 100644
--- a/app/test-pmd/cmdline.c
+++ b/app/test-pmd/cmdline.c
@@ -633,6 +633,9 @@ static void cmd_help_long_parsed(void *parsed_result,
 			"set bonding mode IEEE802.3AD aggregator policy (port_id) (agg_name)\n"
 			"	Set Aggregation mode for IEEE802.3AD (mode 4)\n\n"
 
+			"set bonding lacp timeout_ctrl (port_id) (on|off)\n"
+				"Configure LACP partner to use fast|slow periodic tx interval.\n\n"
+
 			"set bonding balance_xmit_policy (port_id) (l2|l23|l34)\n"
 			"	Set the transmit balance policy for bonded device running in balance mode.\n\n"
 
@@ -6192,6 +6195,7 @@ static void lacp_conf_show(struct rte_eth_bond_8023ad_conf *conf)
 		printf("\taggregation mode: invalid\n");
 		break;
 	}
+	printf("\tlacp timeout control: %u\n", conf->lacp_timeout_control);
 
 	printf("\n");
 }
@@ -6863,6 +6867,78 @@ cmdline_parse_inst_t cmd_set_bonding_agg_mode_policy = {
 };
 
 
+/* *** SET LACP TIMEOUT CONTROL ON BONDED DEVICE *** */
+struct cmd_set_lacp_timeout_control_result {
+	cmdline_fixed_string_t set;
+	cmdline_fixed_string_t bonding;
+	cmdline_fixed_string_t lacp;
+	cmdline_fixed_string_t timeout_ctrl;
+	uint16_t port_id;
+	cmdline_fixed_string_t on_off;
+};
+
+static void
+cmd_set_lacp_timeout_control_parsed(void *parsed_result,
+		__rte_unused struct cmdline *cl,
+		__rte_unused void *data)
+{
+	struct cmd_set_lacp_timeout_control_result *res = parsed_result;
+	struct rte_eth_bond_8023ad_conf port_conf;
+	uint8_t on_off = 0;
+	int ret;
+
+	if (!strcmp(res->on_off, "on"))
+		on_off = 1;
+
+	ret = rte_eth_bond_8023ad_conf_get(res->port_id, &port_conf);
+	if (ret != 0) {
+		fprintf(stderr, "\tGet bonded device %u lacp conf failed\n",
+			res->port_id);
+		return;
+	}
+
+	port_conf.lacp_timeout_control = on_off;
+	ret = rte_eth_bond_8023ad_setup(res->port_id, &port_conf);
+	if (ret != 0)
+		fprintf(stderr, "\tSetup bonded device %u lacp conf failed\n",
+			res->port_id);
+}
+
+cmdline_parse_token_string_t cmd_set_lacp_timeout_control_set =
+	TOKEN_STRING_INITIALIZER(struct cmd_set_lacp_timeout_control_result,
+				set, "set");
+cmdline_parse_token_string_t cmd_set_lacp_timeout_control_bonding =
+	TOKEN_STRING_INITIALIZER(struct cmd_set_lacp_timeout_control_result,
+				bonding, "bonding");
+cmdline_parse_token_string_t cmd_set_lacp_timeout_control_lacp =
+	TOKEN_STRING_INITIALIZER(struct cmd_set_lacp_timeout_control_result,
+				lacp, "lacp");
+cmdline_parse_token_string_t cmd_set_lacp_timeout_control_timeout_ctrl =
+	TOKEN_STRING_INITIALIZER(struct cmd_set_lacp_timeout_control_result,
+				timeout_ctrl, "timeout_ctrl");
+cmdline_parse_token_num_t cmd_set_lacp_timeout_control_port_id =
+	TOKEN_NUM_INITIALIZER(struct cmd_set_lacp_timeout_control_result,
+				port_id, RTE_UINT16);
+cmdline_parse_token_string_t cmd_set_lacp_timeout_control_on_off =
+	TOKEN_STRING_INITIALIZER(struct cmd_set_lacp_timeout_control_result,
+				on_off, "on#off");
+
+cmdline_parse_inst_t cmd_set_lacp_timeout_control = {
+	.f = cmd_set_lacp_timeout_control_parsed,
+	.data = (void *) 0,
+	.help_str = "set bonding lacp timeout_ctrl <port_id> on|off: "
+		"Configure partner to use fast|slow periodic tx interval",
+	.tokens = {
+		(void *)&cmd_set_lacp_timeout_control_set,
+		(void *)&cmd_set_lacp_timeout_control_bonding,
+		(void *)&cmd_set_lacp_timeout_control_lacp,
+		(void *)&cmd_set_lacp_timeout_control_timeout_ctrl,
+		(void *)&cmd_set_lacp_timeout_control_port_id,
+		(void *)&cmd_set_lacp_timeout_control_on_off,
+		NULL
+	}
+};
+
 #endif /* RTE_NET_BOND */
 
 /* *** SET FORWARDING MODE *** */
@@ -17728,6 +17804,7 @@ cmdline_parse_ctx_t main_ctx[] = {
 	(cmdline_parse_inst_t *) &cmd_set_bond_mon_period,
 	(cmdline_parse_inst_t *) &cmd_set_lacp_dedicated_queues,
 	(cmdline_parse_inst_t *) &cmd_set_bonding_agg_mode_policy,
+	(cmdline_parse_inst_t *) &cmd_set_lacp_timeout_control,
 #endif
 	(cmdline_parse_inst_t *)&cmd_vlan_offload,
 	(cmdline_parse_inst_t *)&cmd_vlan_tpid,
diff --git a/app/test/test_link_bonding_mode4.c b/app/test/test_link_bonding_mode4.c
index 2be86d5..68f77ec 100644
--- a/app/test/test_link_bonding_mode4.c
+++ b/app/test/test_link_bonding_mode4.c
@@ -735,6 +735,63 @@ test_mode4_agg_mode_selection(void)
 }
 
 static int
+test_mode4_lacp_timeout_control(void)
+{
+	int retval;
+	int iterations;
+	size_t i;
+	struct slave_conf *slave;
+	uint16_t port_id = test_params.bonded_port_id;
+	struct rte_eth_bond_8023ad_conf conf;
+	struct rte_eth_bond_8023ad_slave_info info;
+	uint8_t on_off = 0;
+	uint8_t lacp_timeout_flag = 0;
+
+	retval = initialize_bonded_device_with_slaves(TEST_LACP_SLAVE_COUT, 0);
+	TEST_ASSERT_SUCCESS(retval, "Failed to initialize bonded device");
+
+	/* Iteration 0: Verify that LACP timeout control is off by default.
+	 * Iteration 1: Verify that we can set LACP timeout control.
+	 * Iteration 2: Verify that we can reset LACP timeout control.
+	 */
+	for (iterations = 0; iterations < 3; iterations++) {
+		/* Verify that bond conf has expected timeout control value.*/
+		retval = rte_eth_bond_8023ad_conf_get(port_id, &conf);
+		TEST_ASSERT_SUCCESS(retval, "Failed to get LACP conf");
+		TEST_ASSERT_EQUAL(conf.lacp_timeout_control, on_off,
+			"Wrong LACP timeout control value");
+
+		/* State machine must run to propagate new timeout control
+		 * value to slaves (iterations 1 and 2). */
+		retval = bond_handshake();
+		TEST_ASSERT_SUCCESS(retval, "Bond handshake failed");
+
+		/* Verify that slaves' actor states have expected value.*/
+		FOR_EACH_PORT(i, slave) {
+			retval = rte_eth_bond_8023ad_slave_info(port_id,
+				slave->port_id, &info);
+			TEST_ASSERT_SUCCESS(retval,
+				"Failed to get LACP slave info");
+			TEST_ASSERT_EQUAL((info.actor_state &
+				STATE_LACP_SHORT_TIMEOUT), lacp_timeout_flag,
+				" Wrong LACP slave info timeout flag");
+		}
+
+		/* Toggle timeout control. */
+		on_off ^= 1;
+		lacp_timeout_flag ^= STATE_LACP_SHORT_TIMEOUT;
+		conf.lacp_timeout_control = on_off;
+		retval = rte_eth_bond_8023ad_setup(port_id, &conf);
+		TEST_ASSERT_SUCCESS(retval, "Failed to setup LACP conf");
+	}
+
+	retval = remove_slaves_and_stop_bonded_device();
+	TEST_ASSERT_SUCCESS(retval, "Test cleanup failed.");
+
+	return TEST_SUCCESS;
+}
+
+static int
 generate_packets(struct rte_ether_addr *src_mac,
 	struct rte_ether_addr *dst_mac, uint16_t count, struct rte_mbuf **buf)
 {
@@ -1649,6 +1706,12 @@ test_mode4_ext_lacp_wrapper(void)
 	return test_mode4_executor(&test_mode4_ext_lacp);
 }
 
+static int
+test_mode4_lacp_timeout_control_wrapper(void)
+{
+	return test_mode4_executor(&test_mode4_lacp_timeout_control);
+}
+
 static struct unit_test_suite link_bonding_mode4_test_suite  = {
 	.suite_name = "Link Bonding mode 4 Unit Test Suite",
 	.setup = test_setup,
@@ -1665,6 +1728,8 @@ static struct unit_test_suite link_bonding_mode4_test_suite  = {
 				test_mode4_ext_ctrl_wrapper),
 		TEST_CASE_NAMED("test_mode4_ext_lacp",
 				test_mode4_ext_lacp_wrapper),
+		TEST_CASE_NAMED("test_mode4_lacp_timeout_control",
+				test_mode4_lacp_timeout_control_wrapper),
 
 		TEST_CASES_END() /**< NULL terminate unit test array */
 	}
-- 
2.7.4

[PATCH 6/7] net/bonding: add bond_8023ad and bond_alb to doc

[Robert Sanford]

- Add bond_8023ad and bond_alb to API documentation.

Signed-off-by: Robert Sanford <rsanford@akamai.com>
---
 doc/api/doxy-api-index.md | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/doc/api/doxy-api-index.md b/doc/api/doxy-api-index.md
index 4245b96..830235c 100644
--- a/doc/api/doxy-api-index.md
+++ b/doc/api/doxy-api-index.md
@@ -39,6 +39,8 @@ The public API headers are grouped by topics:
 - **device specific**:
   [softnic]            (@ref rte_eth_softnic.h),
   [bond]               (@ref rte_eth_bond.h),
+  [bond_8023ad]        (@ref rte_eth_bond_8023ad.h),
+  [bond_alb]           (@ref rte_eth_bond_alb.h),
   [vhost]              (@ref rte_vhost.h),
   [vdpa]               (@ref rte_vdpa.h),
   [KNI]                (@ref rte_kni.h),
-- 
2.7.4

[PATCH 7/7] Remove self from Timers maintainers.

[Robert Sanford]

Signed-off-by: Robert Sanford <rsanford@akamai.com>
---
 MAINTAINERS | 1 -
 1 file changed, 1 deletion(-)

diff --git a/MAINTAINERS b/MAINTAINERS
index 18d9eda..32663b0 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -1613,7 +1613,6 @@ F: examples/vm_power_manager/
 F: doc/guides/sample_app_ug/vm_power_management.rst
 
 Timers
-M: Robert Sanford <rsanford@akamai.com>
 M: Erik Gabriel Carrillo <erik.g.carrillo@intel.com>
 F: lib/timer/
 F: doc/guides/prog_guide/timer_lib.rst
-- 
2.7.4

Re: [PATCH 3/7] net/bonding: change mbuf pool and ring allocation

[Min Hu (Connor)]

Hi, Robert,

在 2021/12/16 2:19, Robert Sanford 写道:
> - Turn off mbuf pool caching to avoid mbufs lingering in pool caches.
>    At most, we transmit one LACPDU per second, per port.
Could you be more detailed, why does mbuf pool caching is not needed?

> - Fix calculation of ring sizes, taking into account that a ring of
>    size N holds up to N-1 items.
Same to that, why should resvere another items ?
> 
By the way, I found the comment for BOND_MODE_8023AX_SLAVE_RX_PKTS is
is wrong, could you fix it in this patch?
> Signed-off-by: Robert Sanford <rsanford@akamai.com>
> ---
>   drivers/net/bonding/rte_eth_bond_8023ad.c | 14 ++++++++------
>   1 file changed, 8 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/net/bonding/rte_eth_bond_8023ad.c b/drivers/net/bonding/rte_eth_bond_8023ad.c
> index 43231bc..83d3938 100644
> --- a/drivers/net/bonding/rte_eth_bond_8023ad.c
> +++ b/drivers/net/bonding/rte_eth_bond_8023ad.c
> @@ -1101,9 +1101,7 @@ bond_mode_8023ad_activate_slave(struct rte_eth_dev *bond_dev,
>   	}
>   
>   	snprintf(mem_name, RTE_DIM(mem_name), "slave_port%u_pool", slave_id);
> -	port->mbuf_pool = rte_pktmbuf_pool_create(mem_name, total_tx_desc,
> -		RTE_MEMPOOL_CACHE_MAX_SIZE >= 32 ?
> -			32 : RTE_MEMPOOL_CACHE_MAX_SIZE,
> +	port->mbuf_pool = rte_pktmbuf_pool_create(mem_name, total_tx_desc, 0,
>   		0, element_size, socket_id);
>   
>   	/* Any memory allocation failure in initialization is critical because
> @@ -1113,19 +1111,23 @@ bond_mode_8023ad_activate_slave(struct rte_eth_dev *bond_dev,
>   			slave_id, mem_name, rte_strerror(rte_errno));
>   	}
>   
> +	/* Add one extra because ring reserves one. */
>   	snprintf(mem_name, RTE_DIM(mem_name), "slave_%u_rx", slave_id);
>   	port->rx_ring = rte_ring_create(mem_name,
> -			rte_align32pow2(BOND_MODE_8023AX_SLAVE_RX_PKTS), socket_id, 0);
> +			rte_align32pow2(BOND_MODE_8023AX_SLAVE_RX_PKTS + 1),
> +			socket_id, 0);
>   
>   	if (port->rx_ring == NULL) {
>   		rte_panic("Slave %u: Failed to create rx ring '%s': %s\n", slave_id,
>   			mem_name, rte_strerror(rte_errno));
>   	}
>   
> -	/* TX ring is at least one pkt longer to make room for marker packet. */
> +	/* TX ring is at least one pkt longer to make room for marker packet.
> +	 * Add one extra because ring reserves one. */
>   	snprintf(mem_name, RTE_DIM(mem_name), "slave_%u_tx", slave_id);
>   	port->tx_ring = rte_ring_create(mem_name,
> -			rte_align32pow2(BOND_MODE_8023AX_SLAVE_TX_PKTS + 1), socket_id, 0);
> +			rte_align32pow2(BOND_MODE_8023AX_SLAVE_TX_PKTS + 2),
> +			socket_id, 0);
>   
>   	if (port->tx_ring == NULL) {
>   		rte_panic("Slave %u: Failed to create tx ring '%s': %s\n", slave_id,
> 

Re: [PATCH 3/7] net/bonding: change mbuf pool and ring allocation

[Sanford, Robert]

Hello Connor,

Thank you for the questions and comments. I will repeat the questions, followed by my answers.

Q: Could you be more detailed, why is mbuf pool caching not needed?

A: The short answer: under certain conditions, we can run out of
buffers from that small, LACPDU-mempool. We actually saw this occur
in production, on mostly-idle links.

For a long explanation, let's assume the following:
1. 1 tx-queue per bond and underlying ethdev ports.
2. 256 tx-descriptors (per ethdev port).
3. 257 mbufs in each port's LACPDU-pool, as computed by
bond_mode_8023ad_activate_slave(), and cache-size 32.
4. The "app" xmits zero packets to this bond for a long time.
5. In EAL intr thread context, LACP tx_machine() allocates 1 mbuf
(LACPDU) per second from the pool, and puts it into LACP tx-ring.
6. Every second, another thread, let's call it the tx-core, calls
tx-burst (with zero packets to xmit), finds 1 mbuf on LACP tx-ring,
and underlying ethdev PMD puts mbuf data into a tx-desc.
7. PMD tx-burst configured not to clean up used tx-descs until
there are almost none free, e.g., less than pool's cache-size *
CACHE_FLUSH_THRESH_MULTIPLIER (1.5).
8. When cleaning up tx-descs, we may leave up to 47 mbufs in the
tx-core's LACPDU-pool cache (not accessible from intr thread).

When the number of used tx-descs (0..255) + number of mbufs in the
cache (0..47) reaches 257, then allocation fails.

If I understand the LACP tx-burst code correctly, it would be
worse if nb_tx_queues > 1, because (assuming multiple tx-cores)
any queue/lcore could xmit an LACPDU. Thus, up to nb_tx_queues *
47 mbufs could be cached, and not accessible from tx_machine().

You would not see this problem if the app xmits other (non-LACP)
mbufs on a regular basis, to expedite the clean-up of tx-descs
including LACPDU mbufs (unless nb_tx_queues tx-core caches
could hold all LACPDU mbufs).

If we make mempool's cache size 0, then allocation will not fail.

A mempool cache for LACPDUs does not offer much additional speed:
during alloc, the intr thread does not have default mempool caches
(AFAIK); and the average time between frees is either 1 second (LACP
short timeouts) or 10 seconds (long timeouts), i.e., infrequent.

--------

Q: Why reserve one additional slot in the rx and tx rings?

A: rte_ring_create() requires the ring size N, to be a power of 2,
but it can only store N-1 items. Thus, if we want to store X items,
we need to ask for (at least) X+1. Original code fails when the real
desired size is a power of 2, because in such a case, align32pow2
does not round up.

For example, say we want a ring to hold 4:

    rte_ring_create(... rte_align32pow2(4) ...)

rte_align32pow2(4) returns 4, and we end up with a ring that only
stores 3 items.

    rte_ring_create(... rte_align32pow2(4+1) ...)

rte_align32pow2(5) returns 8, and we end up with a ring that
stores up to 7 items, more than we need, but acceptable.

--------

Q: I found the comment for BOND_MODE_8023AX_SLAVE_RX_PKTS is
wrong, could you fix it in this patch?

A: Yes, I will fix it in the next version of the patch.

--
Regards,
Robert Sanford


On 12/16/21, 4:01 AM, "Min Hu (Connor)" <humin29@huawei.com> wrote:

    Hi, Robert,

    在 2021/12/16 2:19, Robert Sanford 写道:
    > - Turn off mbuf pool caching to avoid mbufs lingering in pool caches.
    >    At most, we transmit one LACPDU per second, per port.
    Could you be more detailed, why does mbuf pool caching is not needed?

    > - Fix calculation of ring sizes, taking into account that a ring of
    >    size N holds up to N-1 items.
    Same to that, why should resvere another items ?
    > 
    By the way, I found the comment for BOND_MODE_8023AX_SLAVE_RX_PKTS is
    is wrong, could you fix it in this patch?
    > Signed-off-by: Robert Sanford <rsanford@akamai.com>
    > ---
    >   drivers/net/bonding/rte_eth_bond_8023ad.c | 14 ++++++++------
    >   1 file changed, 8 insertions(+), 6 deletions(-)
    > 
    > diff --git a/drivers/net/bonding/rte_eth_bond_8023ad.c b/drivers/net/bonding/rte_eth_bond_8023ad.c
    > index 43231bc..83d3938 100644
    > --- a/drivers/net/bonding/rte_eth_bond_8023ad.c
    > +++ b/drivers/net/bonding/rte_eth_bond_8023ad.c
    > @@ -1101,9 +1101,7 @@ bond_mode_8023ad_activate_slave(struct rte_eth_dev *bond_dev,
    >   	}
    >   
    >   	snprintf(mem_name, RTE_DIM(mem_name), "slave_port%u_pool", slave_id);
    > -	port->mbuf_pool = rte_pktmbuf_pool_create(mem_name, total_tx_desc,
    > -		RTE_MEMPOOL_CACHE_MAX_SIZE >= 32 ?
    > -			32 : RTE_MEMPOOL_CACHE_MAX_SIZE,
    > +	port->mbuf_pool = rte_pktmbuf_pool_create(mem_name, total_tx_desc, 0,
    >   		0, element_size, socket_id);
    >   
    >   	/* Any memory allocation failure in initialization is critical because
    > @@ -1113,19 +1111,23 @@ bond_mode_8023ad_activate_slave(struct rte_eth_dev *bond_dev,
    >   			slave_id, mem_name, rte_strerror(rte_errno));
    >   	}
    >   
    > +	/* Add one extra because ring reserves one. */
    >   	snprintf(mem_name, RTE_DIM(mem_name), "slave_%u_rx", slave_id);
    >   	port->rx_ring = rte_ring_create(mem_name,
    > -			rte_align32pow2(BOND_MODE_8023AX_SLAVE_RX_PKTS), socket_id, 0);
    > +			rte_align32pow2(BOND_MODE_8023AX_SLAVE_RX_PKTS + 1),
    > +			socket_id, 0);
    >   
    >   	if (port->rx_ring == NULL) {
    >   		rte_panic("Slave %u: Failed to create rx ring '%s': %s\n", slave_id,
    >   			mem_name, rte_strerror(rte_errno));
    >   	}
    >   
    > -	/* TX ring is at least one pkt longer to make room for marker packet. */
    > +	/* TX ring is at least one pkt longer to make room for marker packet.
    > +	 * Add one extra because ring reserves one. */
    >   	snprintf(mem_name, RTE_DIM(mem_name), "slave_%u_tx", slave_id);
    >   	port->tx_ring = rte_ring_create(mem_name,
    > -			rte_align32pow2(BOND_MODE_8023AX_SLAVE_TX_PKTS + 1), socket_id, 0);
    > +			rte_align32pow2(BOND_MODE_8023AX_SLAVE_TX_PKTS + 2),
    > +			socket_id, 0);
    >   
    >   	if (port->tx_ring == NULL) {
    >   		rte_panic("Slave %u: Failed to create tx ring '%s': %s\n", slave_id,
    > 

Re: [PATCH 3/7] net/bonding: change mbuf pool and ring allocation

[Min Hu (Connor)]

Hi, Sanford,
	Thanks for your detailed description, some questions as follows.

在 2021/12/18 3:49, Sanford, Robert 写道:
> Hello Connor,
> 
> Thank you for the questions and comments. I will repeat the questions, followed by my answers.
> 
> Q: Could you be more detailed, why is mbuf pool caching not needed?
> 
> A: The short answer: under certain conditions, we can run out of
> buffers from that small, LACPDU-mempool. We actually saw this occur
> in production, on mostly-idle links.
> 
> For a long explanation, let's assume the following:
> 1. 1 tx-queue per bond and underlying ethdev ports.
> 2. 256 tx-descriptors (per ethdev port).
> 3. 257 mbufs in each port's LACPDU-pool, as computed by
> bond_mode_8023ad_activate_slave(), and cache-size 32.
> 4. The "app" xmits zero packets to this bond for a long time.
> 5. In EAL intr thread context, LACP tx_machine() allocates 1 mbuf
> (LACPDU) per second from the pool, and puts it into LACP tx-ring.
> 6. Every second, another thread, let's call it the tx-core, calls
> tx-burst (with zero packets to xmit), finds 1 mbuf on LACP tx-ring,
> and underlying ethdev PMD puts mbuf data into a tx-desc.
> 7. PMD tx-burst configured not to clean up used tx-descs until
> there are almost none free, e.g., less than pool's cache-size *
> CACHE_FLUSH_THRESH_MULTIPLIER (1.5).
> 8. When cleaning up tx-descs, we may leave up to 47 mbufs in the
> tx-core's LACPDU-pool cache (not accessible from intr thread).
> 
> When the number of used tx-descs (0..255) + number of mbufs in the
> cache (0..47) reaches 257, then allocation fails.
> 
> If I understand the LACP tx-burst code correctly, it would be
> worse if nb_tx_queues > 1, because (assuming multiple tx-cores)
> any queue/lcore could xmit an LACPDU. Thus, up to nb_tx_queues *
> 47 mbufs could be cached, and not accessible from tx_machine().
> 
> You would not see this problem if the app xmits other (non-LACP)
> mbufs on a regular basis, to expedite the clean-up of tx-descs
> including LACPDU mbufs (unless nb_tx_queues tx-core caches
> could hold all LACPDU mbufs).
> 
I think, we could not see this problem only because the mempool can
offer much more mbufs than cache size on no-LACP circumstance.

> If we make mempool's cache size 0, then allocation will not fail.
How about enlarge the size of mempool, i.e., up to 4096 ? I think
it can also avoid this bug.
> 
> A mempool cache for LACPDUs does not offer much additional speed:
> during alloc, the intr thread does not have default mempool caches
Why? as I know, all the core has its own default mempool caches ?
> (AFAIK); and the average time between frees is either 1 second (LACP
> short timeouts) or 10 seconds (long timeouts), i.e., infrequent.
> 
> --------
> 
> Q: Why reserve one additional slot in the rx and tx rings?
> 
> A: rte_ring_create() requires the ring size N, to be a power of 2,
> but it can only store N-1 items. Thus, if we want to store X items,
Hi, Robert, could you describe it for me?
I cannot understand why it
"only store N -1 items". I check the source code, It writes:
"The real usable ring size is *count-1* instead of *count* to
differentiate a free ring from an empty ring."
But I still can not get what it wrote.

> we need to ask for (at least) X+1. Original code fails when the real
> desired size is a power of 2, because in such a case, align32pow2
> does not round up.
> 
> For example, say we want a ring to hold 4:
> 
>      rte_ring_create(... rte_align32pow2(4) ...)
> 
> rte_align32pow2(4) returns 4, and we end up with a ring that only
> stores 3 items.
> 
>      rte_ring_create(... rte_align32pow2(4+1) ...)
> 
> rte_align32pow2(5) returns 8, and we end up with a ring that
> stores up to 7 items, more than we need, but acceptable.
To fix the bug, how about just setting the flags "RING_F_EXACT_SZ"

> 
> --------
> 
> Q: I found the comment for BOND_MODE_8023AX_SLAVE_RX_PKTS is
> wrong, could you fix it in this patch?
> 
> A: Yes, I will fix it in the next version of the patch.
Thanks.
> 
> --
> Regards,
> Robert Sanford
> 
> 
> On 12/16/21, 4:01 AM, "Min Hu (Connor)" <humin29@huawei.com> wrote:
> 
>      Hi, Robert,
> 
>      在 2021/12/16 2:19, Robert Sanford 写道:
>      > - Turn off mbuf pool caching to avoid mbufs lingering in pool caches.
>      >    At most, we transmit one LACPDU per second, per port.
>      Could you be more detailed, why does mbuf pool caching is not needed?
> 
>      > - Fix calculation of ring sizes, taking into account that a ring of
>      >    size N holds up to N-1 items.
>      Same to that, why should resvere another items ?
>      >
>      By the way, I found the comment for BOND_MODE_8023AX_SLAVE_RX_PKTS is
>      is wrong, could you fix it in this patch?
>      > Signed-off-by: Robert Sanford <rsanford@akamai.com>
>      > ---
>      >   drivers/net/bonding/rte_eth_bond_8023ad.c | 14 ++++++++------
>      >   1 file changed, 8 insertions(+), 6 deletions(-)
>      >
>      > diff --git a/drivers/net/bonding/rte_eth_bond_8023ad.c b/drivers/net/bonding/rte_eth_bond_8023ad.c
>      > index 43231bc..83d3938 100644
>      > --- a/drivers/net/bonding/rte_eth_bond_8023ad.c
>      > +++ b/drivers/net/bonding/rte_eth_bond_8023ad.c
>      > @@ -1101,9 +1101,7 @@ bond_mode_8023ad_activate_slave(struct rte_eth_dev *bond_dev,
>      >   	}
>      >
>      >   	snprintf(mem_name, RTE_DIM(mem_name), "slave_port%u_pool", slave_id);
>      > -	port->mbuf_pool = rte_pktmbuf_pool_create(mem_name, total_tx_desc,
>      > -		RTE_MEMPOOL_CACHE_MAX_SIZE >= 32 ?
>      > -			32 : RTE_MEMPOOL_CACHE_MAX_SIZE,
>      > +	port->mbuf_pool = rte_pktmbuf_pool_create(mem_name, total_tx_desc, 0,
>      >   		0, element_size, socket_id);
>      >
>      >   	/* Any memory allocation failure in initialization is critical because
>      > @@ -1113,19 +1111,23 @@ bond_mode_8023ad_activate_slave(struct rte_eth_dev *bond_dev,
>      >   			slave_id, mem_name, rte_strerror(rte_errno));
>      >   	}
>      >
>      > +	/* Add one extra because ring reserves one. */
>      >   	snprintf(mem_name, RTE_DIM(mem_name), "slave_%u_rx", slave_id);
>      >   	port->rx_ring = rte_ring_create(mem_name,
>      > -			rte_align32pow2(BOND_MODE_8023AX_SLAVE_RX_PKTS), socket_id, 0);
>      > +			rte_align32pow2(BOND_MODE_8023AX_SLAVE_RX_PKTS + 1),
>      > +			socket_id, 0);
>      >
>      >   	if (port->rx_ring == NULL) {
>      >   		rte_panic("Slave %u: Failed to create rx ring '%s': %s\n", slave_id,
>      >   			mem_name, rte_strerror(rte_errno));
>      >   	}
>      >
>      > -	/* TX ring is at least one pkt longer to make room for marker packet. */
>      > +	/* TX ring is at least one pkt longer to make room for marker packet.
>      > +	 * Add one extra because ring reserves one. */
>      >   	snprintf(mem_name, RTE_DIM(mem_name), "slave_%u_tx", slave_id);
>      >   	port->tx_ring = rte_ring_create(mem_name,
>      > -			rte_align32pow2(BOND_MODE_8023AX_SLAVE_TX_PKTS + 1), socket_id, 0);
>      > +			rte_align32pow2(BOND_MODE_8023AX_SLAVE_TX_PKTS + 2),
>      > +			socket_id, 0);
>      >
>      >   	if (port->tx_ring == NULL) {
>      >   		rte_panic("Slave %u: Failed to create tx ring '%s': %s\n", slave_id,
>      >
> 

Re: [PATCH 3/7] net/bonding: change mbuf pool and ring allocation

[Sanford, Robert]

Hello Connor,

Please see responses inline.

On 12/17/21, 10:44 PM, "Min Hu (Connor)" <humin29@huawei.com> wrote:

> > When the number of used tx-descs (0..255) + number of mbufs in the
> > cache (0..47) reaches 257, then allocation fails.
> > 
> > If I understand the LACP tx-burst code correctly, it would be
> > worse if nb_tx_queues > 1, because (assuming multiple tx-cores)
> > any queue/lcore could xmit an LACPDU. Thus, up to nb_tx_queues *
> > 47 mbufs could be cached, and not accessible from tx_machine().
> > 
> > You would not see this problem if the app xmits other (non-LACP)
> > mbufs on a regular basis, to expedite the clean-up of tx-descs
> > including LACPDU mbufs (unless nb_tx_queues tx-core caches
> > could hold all LACPDU mbufs).
> > 
> I think, we could not see this problem only because the mempool can
> offer much more mbufs than cache size on no-LACP circumstance.
>
> > If we make mempool's cache size 0, then allocation will not fail.
> How about enlarge the size of mempool, i.e., up to 4096 ? I think
> it can also avoid this bug.
> > 
> > A mempool cache for LACPDUs does not offer much additional speed:
> > during alloc, the intr thread does not have default mempool caches
> Why? as I know, all the core has its own default mempool caches ?

These are private mbuf pools that we use *only* for LACPDUs, *one*
mbuf per second, at most. (When LACP link peer selects long timeouts,
we get/put one mbuf every 30 seconds.)

There is *NO* benefit for the consumer thread (interrupt thread
executing tx_machine()) to have caches on per-slave LACPDU pools.
The interrupt thread is a control thread, i.e., a non-EAL thread.
Its lcore_id is LCORE_ID_ANY, so it has no "default cache" in any
mempool.

There is little or no benefit for active data-plane threads to have
caches on per-slave LACPDU pools, because on each pool, the producer
thread puts back, at most, one mbuf per second. There is not much
contention with the consumer (interrupt thread).

I contend that caches are not necessary for these private LACPDU
mbuf pools, but just waste RAM and CPU-cache. If we still insist
on creating them *with* caches, then we should add at least
(cache-size x 1.5 x nb-tx-queues) mbufs per pool.

 
> > Q: Why reserve one additional slot in the rx and tx rings?
> > 
> > A: rte_ring_create() requires the ring size N, to be a power of 2,
> > but it can only store N-1 items. Thus, if we want to store X items,
> Hi, Robert, could you describe it for me?
> I cannot understand why it
> "only store N -1 items". I check the source code, It writes:
> "The real usable ring size is *count-1* instead of *count* to
> differentiate a free ring from an empty ring."
> But I still can not get what it wrote.

I believe there is a mistake in the ring comments (in 3 places).
It would be better if they replace "free" with "full":
"... to differentiate a *full* ring from an empty ring."


> > we need to ask for (at least) X+1. Original code fails when the real
> > desired size is a power of 2, because in such a case, align32pow2
> > does not round up.
> > 
> > For example, say we want a ring to hold 4:
> > 
> >      rte_ring_create(... rte_align32pow2(4) ...)
> > 
> > rte_align32pow2(4) returns 4, and we end up with a ring that only
> > stores 3 items.
> > 
> >      rte_ring_create(... rte_align32pow2(4+1) ...)
> > 
> > rte_align32pow2(5) returns 8, and we end up with a ring that
> > stores up to 7 items, more than we need, but acceptable.
> To fix the bug, how about just setting the flags "RING_F_EXACT_SZ"

Yes, this is a good idea. I will look for examples or test code that
use this flag.

 --
Regards,
Robert Sanford

Re: [PATCH 3/7] net/bonding: change mbuf pool and ring allocation

[Min Hu (Connor)]

Hi, Sanford,

在 2021/12/21 0:47, Sanford, Robert 写道:
> Hello Connor,
> 
> Please see responses inline.
> 
> On 12/17/21, 10:44 PM, "Min Hu (Connor)" <humin29@huawei.com> wrote:
> 
>>> When the number of used tx-descs (0..255) + number of mbufs in the
>>> cache (0..47) reaches 257, then allocation fails.
>>>
>>> If I understand the LACP tx-burst code correctly, it would be
>>> worse if nb_tx_queues > 1, because (assuming multiple tx-cores)
>>> any queue/lcore could xmit an LACPDU. Thus, up to nb_tx_queues *
>>> 47 mbufs could be cached, and not accessible from tx_machine().
>>>
>>> You would not see this problem if the app xmits other (non-LACP)
>>> mbufs on a regular basis, to expedite the clean-up of tx-descs
>>> including LACPDU mbufs (unless nb_tx_queues tx-core caches
>>> could hold all LACPDU mbufs).
>>>
>> I think, we could not see this problem only because the mempool can
>> offer much more mbufs than cache size on no-LACP circumstance.
>>
>>> If we make mempool's cache size 0, then allocation will not fail.
>> How about enlarge the size of mempool, i.e., up to 4096 ? I think
>> it can also avoid this bug.
>>>
>>> A mempool cache for LACPDUs does not offer much additional speed:
>>> during alloc, the intr thread does not have default mempool caches
>> Why? as I know, all the core has its own default mempool caches ?
> 
> These are private mbuf pools that we use *only* for LACPDUs, *one*
> mbuf per second, at most. (When LACP link peer selects long timeouts,
> we get/put one mbuf every 30 seconds.)
> 
> There is *NO* benefit for the consumer thread (interrupt thread
> executing tx_machine()) to have caches on per-slave LACPDU pools.
> The interrupt thread is a control thread, i.e., a non-EAL thread.
> Its lcore_id is LCORE_ID_ANY, so it has no "default cache" in any
> mempool.
Well, sorry, I forgot that interrupt thread is non-EAL thread.
> 
> There is little or no benefit for active data-plane threads to have
> caches on per-slave LACPDU pools, because on each pool, the producer
> thread puts back, at most, one mbuf per second. There is not much
> contention with the consumer (interrupt thread).
> 
> I contend that caches are not necessary for these private LACPDU
I agree with you.
> mbuf pools, but just waste RAM and CPU-cache. If we still insist
> on creating them *with* caches, then we should add at least
> (cache-size x 1.5 x nb-tx-queues) mbufs per pool.
> 
>   
>>> Q: Why reserve one additional slot in the rx and tx rings?
>>>
>>> A: rte_ring_create() requires the ring size N, to be a power of 2,
>>> but it can only store N-1 items. Thus, if we want to store X items,
>> Hi, Robert, could you describe it for me?
>> I cannot understand why it
>> "only store N -1 items". I check the source code, It writes:
>> "The real usable ring size is *count-1* instead of *count* to
>> differentiate a free ring from an empty ring."
>> But I still can not get what it wrote.
> 
> I believe there is a mistake in the ring comments (in 3 places).
> It would be better if they replace "free" with "full":
> "... to differentiate a *full* ring from an empty ring."
> 
Well, I still can not understand it. I think the ring size is N, it
should store N items, why "N - 1" items.?
Hope for your description, thanks.

> 
>>> we need to ask for (at least) X+1. Original code fails when the real
>>> desired size is a power of 2, because in such a case, align32pow2
>>> does not round up.
>>>
>>> For example, say we want a ring to hold 4:
>>>
>>>       rte_ring_create(... rte_align32pow2(4) ...)
>>>
>>> rte_align32pow2(4) returns 4, and we end up with a ring that only
>>> stores 3 items.
>>>
>>>       rte_ring_create(... rte_align32pow2(4+1) ...)
>>>
>>> rte_align32pow2(5) returns 8, and we end up with a ring that
>>> stores up to 7 items, more than we need, but acceptable.
>> To fix the bug, how about just setting the flags "RING_F_EXACT_SZ"
> 
> Yes, this is a good idea. I will look for examples or test code that
> use this flag.
Yes, if fixed, ILGM.
> 
>   --
> Regards,
> Robert Sanford
> 
> 

Re: [PATCH 3/7] net/bonding: change mbuf pool and ring allocation

[Sanford, Robert]

Hi Connor,

On 12/20/21, 9:03 PM, "Min Hu (Connor)" <humin29@huawei.com> wrote:

> Hi, Sanford,

> > There is *NO* benefit for the consumer thread (interrupt thread
> > executing tx_machine()) to have caches on per-slave LACPDU pools.
> > The interrupt thread is a control thread, i.e., a non-EAL thread.
> > Its lcore_id is LCORE_ID_ANY, so it has no "default cache" in any
> > mempool.
> Well, sorry, I forgot that interrupt thread is non-EAL thread.

No problem. (I added a temporary rte_log statement in tx_machine
to make sure lcore_id == LCORE_ID_ANY.)

> > There is little or no benefit for active data-plane threads to have
> > caches on per-slave LACPDU pools, because on each pool, the producer
> > thread puts back, at most, one mbuf per second. There is not much
> > contention with the consumer (interrupt thread).
> > 
> > I contend that caches are not necessary for these private LACPDU
> I agree with you.

Thanks.

> > I believe there is a mistake in the ring comments (in 3 places).
> > It would be better if they replace "free" with "full":
> > "... to differentiate a *full* ring from an empty ring."
> > 
> Well, I still can not understand it. I think the ring size is N, it
> should store N items, why "N - 1" items.?
> Hope for your description, thanks.

Here is an excellent article that describes ring buffers, empty vs full, N-1, etc.
https://embedjournal.com/implementing-circular-buffer-embedded-c/#the-full-vs-empty-problem


> >> To fix the bug, how about just setting the flags "RING_F_EXACT_SZ"
> > 
> > Yes, this is a good idea. I will look for examples or test code that
> > use this flag.
> Yes, if fixed, ILGM.

I will use RING_F_EXACT_SZ flag in the next version of the patchset. I did not know about that flag.
	rte_ring_create(... N_PKTS ... RING_F_EXACT_SZ)
... is equivalent to, and looks cleaner than ...
	rte_ring_create(... rte_align32pow2(N_PKTS + 1) ... 0)

I plan to create a separate patchset to update the comments in rte_ring.h,
re RING_F_EXACT_SZ and "free" vs "full".

--
Regards,
Robert Sanford

[PATCH v2 0/8] net/bonding: fixes and LACP short timeout

[Robert Sanford]

This patchset makes the following changes to net/bonding:
- Clean up minor errors in spelling, whitespace, C++ wrappers, and
  comments.
- Replace directly overwriting of slave port's rte_eth_conf by copying
  it, but only updating it via rte_eth_dev_configure().
- Make minor changes to allocation of mbuf pool and rx/tx rings.
- Add support for enabling LACP short timeout, i.e., link partner can
  use fast periodic time interval between transmits.
- Include bond_8023ad and bond_alb in doxygen.
- Remove self from Timers maintainers.
- Add API stubs to net/ring PMD.
- Add LACP short timeout to tests.

V2 changes:
- Additional typo and whitespace corrections.
- Minor changes to LACP private rings creation.
- Add net/ring API stubs patch.
- Insert extra "bond_handshake" to LACP short timeout autotest.

Robert Sanford (8):
  net/bonding: fix typos and whitespace
  net/bonding: fix bonded dev configuring slave dev
  net/bonding: change mbuf pool and ring creation
  net/bonding: support enabling LACP short timeout
  net/bonding: add bond_8023ad and bond_alb to doc
  Remove self from Timers maintainers.
  net/ring: add promiscuous and allmulticast API stubs
  net/bonding: add LACP short timeout to tests

 MAINTAINERS                                   |  1 -
 app/test-pmd/cmdline.c                        | 81 +++++++++++++++++++++-
 app/test/test_link_bonding_mode4.c            | 98 ++++++++++++++++++++++-----
 doc/api/doxy-api-index.md                     |  2 +
 drivers/net/bonding/eth_bond_8023ad_private.h | 15 ++--
 drivers/net/bonding/rte_eth_bond_8023ad.c     | 58 ++++++++++------
 drivers/net/bonding/rte_eth_bond_8023ad.h     | 18 +++--
 drivers/net/bonding/rte_eth_bond_pmd.c        | 43 ++++++------
 drivers/net/ring/rte_eth_ring.c               | 28 ++++++++
 9 files changed, 272 insertions(+), 72 deletions(-)

-- 
2.7.4

[PATCH v2 1/8] net/bonding: fix typos and whitespace

[Robert Sanford]

- Clean up minor typos in comments, strings, and private names.
- Fix whitespace in log messages and function formatting
  (new line before open brace).
- Move closing C++ wrapper to the end of rte_eth_bond_8023ad.h.

Signed-off-by: Robert Sanford <rsanford@akamai.com>
---
 app/test-pmd/cmdline.c                        |  4 ++--
 app/test/test_link_bonding_mode4.c            | 28 +++++++++++++--------------
 drivers/net/bonding/eth_bond_8023ad_private.h | 12 ++++++------
 drivers/net/bonding/rte_eth_bond_8023ad.c     | 22 ++++++++++-----------
 drivers/net/bonding/rte_eth_bond_8023ad.h     | 15 +++++++-------
 drivers/net/bonding/rte_eth_bond_pmd.c        | 13 ++++++++-----
 6 files changed, 49 insertions(+), 45 deletions(-)

diff --git a/app/test-pmd/cmdline.c b/app/test-pmd/cmdline.c
index 6e10afe..9fd2c2a 100644
--- a/app/test-pmd/cmdline.c
+++ b/app/test-pmd/cmdline.c
@@ -630,8 +630,8 @@ static void cmd_help_long_parsed(void *parsed_result,
 			"set bonding mac_addr (port_id) (address)\n"
 			"	Set the MAC address of a bonded device.\n\n"
 
-			"set bonding mode IEEE802.3AD aggregator policy (port_id) (agg_name)"
-			"	Set Aggregation mode for IEEE802.3AD (mode 4)"
+			"set bonding mode IEEE802.3AD aggregator policy (port_id) (agg_name)\n"
+			"	Set Aggregation mode for IEEE802.3AD (mode 4)\n\n"
 
 			"set bonding balance_xmit_policy (port_id) (l2|l23|l34)\n"
 			"	Set the transmit balance policy for bonded device running in balance mode.\n\n"
diff --git a/app/test/test_link_bonding_mode4.c b/app/test/test_link_bonding_mode4.c
index 351129d..2be86d5 100644
--- a/app/test/test_link_bonding_mode4.c
+++ b/app/test/test_link_bonding_mode4.c
@@ -58,11 +58,11 @@ static const struct rte_ether_addr slave_mac_default = {
 	{ 0x00, 0xFF, 0x00, 0xFF, 0x00, 0x00 }
 };
 
-static const struct rte_ether_addr parnter_mac_default = {
+static const struct rte_ether_addr partner_mac_default = {
 	{ 0x22, 0xBB, 0xFF, 0xBB, 0x00, 0x00 }
 };
 
-static const struct rte_ether_addr parnter_system = {
+static const struct rte_ether_addr partner_system = {
 	{ 0x33, 0xFF, 0xBB, 0xFF, 0x00, 0x00 }
 };
 
@@ -76,7 +76,7 @@ struct slave_conf {
 	uint16_t port_id;
 	uint8_t bonded : 1;
 
-	uint8_t lacp_parnter_state;
+	uint8_t lacp_partner_state;
 };
 
 struct ether_vlan_hdr {
@@ -258,7 +258,7 @@ add_slave(struct slave_conf *slave, uint8_t start)
 	TEST_ASSERT_EQUAL(rte_is_same_ether_addr(&addr, &addr_check), 1,
 			"Slave MAC address is not as expected");
 
-	RTE_VERIFY(slave->lacp_parnter_state == 0);
+	RTE_VERIFY(slave->lacp_partner_state == 0);
 	return 0;
 }
 
@@ -288,7 +288,7 @@ remove_slave(struct slave_conf *slave)
 			test_params.bonded_port_id);
 
 	slave->bonded = 0;
-	slave->lacp_parnter_state = 0;
+	slave->lacp_partner_state = 0;
 	return 0;
 }
 
@@ -501,20 +501,20 @@ make_lacp_reply(struct slave_conf *slave, struct rte_mbuf *pkt)
 	slow_hdr = rte_pktmbuf_mtod(pkt, struct slow_protocol_frame *);
 
 	/* Change source address to partner address */
-	rte_ether_addr_copy(&parnter_mac_default, &slow_hdr->eth_hdr.src_addr);
+	rte_ether_addr_copy(&partner_mac_default, &slow_hdr->eth_hdr.src_addr);
 	slow_hdr->eth_hdr.src_addr.addr_bytes[RTE_ETHER_ADDR_LEN - 1] =
 		slave->port_id;
 
 	lacp = (struct lacpdu *) &slow_hdr->slow_protocol;
 	/* Save last received state */
-	slave->lacp_parnter_state = lacp->actor.state;
+	slave->lacp_partner_state = lacp->actor.state;
 	/* Change it into LACP replay by matching parameters. */
 	memcpy(&lacp->partner.port_params, &lacp->actor.port_params,
 		sizeof(struct port_params));
 
 	lacp->partner.state = lacp->actor.state;
 
-	rte_ether_addr_copy(&parnter_system, &lacp->actor.port_params.system);
+	rte_ether_addr_copy(&partner_system, &lacp->actor.port_params.system);
 	lacp->actor.state = STATE_LACP_ACTIVE |
 						STATE_SYNCHRONIZATION |
 						STATE_AGGREGATION |
@@ -580,7 +580,7 @@ bond_handshake_done(struct slave_conf *slave)
 	const uint8_t expected_state = STATE_LACP_ACTIVE | STATE_SYNCHRONIZATION |
 			STATE_AGGREGATION | STATE_COLLECTING | STATE_DISTRIBUTING;
 
-	return slave->lacp_parnter_state == expected_state;
+	return slave->lacp_partner_state == expected_state;
 }
 
 static unsigned
@@ -1165,7 +1165,7 @@ init_marker(struct rte_mbuf *pkt, struct slave_conf *slave)
 			&marker_hdr->eth_hdr.dst_addr);
 
 	/* Init source address */
-	rte_ether_addr_copy(&parnter_mac_default,
+	rte_ether_addr_copy(&partner_mac_default,
 			&marker_hdr->eth_hdr.src_addr);
 	marker_hdr->eth_hdr.src_addr.addr_bytes[RTE_ETHER_ADDR_LEN - 1] =
 		slave->port_id;
@@ -1353,7 +1353,7 @@ test_mode4_expired(void)
 	/* After test only expected slave should be in EXPIRED state */
 	FOR_EACH_SLAVE(i, slave) {
 		if (slave == exp_slave)
-			TEST_ASSERT(slave->lacp_parnter_state & STATE_EXPIRED,
+			TEST_ASSERT(slave->lacp_partner_state & STATE_EXPIRED,
 				"Slave %u should be in expired.", slave->port_id);
 		else
 			TEST_ASSERT_EQUAL(bond_handshake_done(slave), 1,
@@ -1392,7 +1392,7 @@ test_mode4_ext_ctrl(void)
 		},
 	};
 
-	rte_ether_addr_copy(&parnter_system, &src_mac);
+	rte_ether_addr_copy(&partner_system, &src_mac);
 	rte_ether_addr_copy(&slow_protocol_mac_addr, &dst_mac);
 
 	initialize_eth_header(&lacpdu.eth_hdr, &src_mac, &dst_mac,
@@ -1446,7 +1446,7 @@ test_mode4_ext_lacp(void)
 		},
 	};
 
-	rte_ether_addr_copy(&parnter_system, &src_mac);
+	rte_ether_addr_copy(&partner_system, &src_mac);
 	rte_ether_addr_copy(&slow_protocol_mac_addr, &dst_mac);
 
 	initialize_eth_header(&lacpdu.eth_hdr, &src_mac, &dst_mac,
@@ -1535,7 +1535,7 @@ check_environment(void)
 		if (port->bonded != 0)
 			env_state |= 0x04;
 
-		if (port->lacp_parnter_state != 0)
+		if (port->lacp_partner_state != 0)
 			env_state |= 0x08;
 
 		if (env_state != 0)
diff --git a/drivers/net/bonding/eth_bond_8023ad_private.h b/drivers/net/bonding/eth_bond_8023ad_private.h
index 9b5738a..60db31e 100644
--- a/drivers/net/bonding/eth_bond_8023ad_private.h
+++ b/drivers/net/bonding/eth_bond_8023ad_private.h
@@ -15,12 +15,12 @@
 #include "rte_eth_bond_8023ad.h"
 
 #define BOND_MODE_8023AX_UPDATE_TIMEOUT_MS  100
-/** Maximum number of packets to one slave queued in TX ring. */
+/** Maximum number of packets to one slave queued in RX ring. */
 #define BOND_MODE_8023AX_SLAVE_RX_PKTS        3
 /** Maximum number of LACP packets from one slave queued in TX ring. */
 #define BOND_MODE_8023AX_SLAVE_TX_PKTS        1
 /**
- * Timeouts deffinitions (5.4.4 in 802.1AX documentation).
+ * Timeouts definitions (6.4.4 in 802.1AX documentation).
  */
 #define BOND_8023AD_FAST_PERIODIC_MS                900
 #define BOND_8023AD_SLOW_PERIODIC_MS              29000
@@ -34,7 +34,7 @@
 /**
  * Interval of showing warning message from state machines. All messages will
  * be held (and gathered together) to prevent flooding.
- * This is no parto of 802.1AX standard.
+ * This is not part of 802.1AX standard.
  */
 #define BOND_8023AD_WARNINGS_PERIOD_MS             1000
 
@@ -83,7 +83,7 @@
 #define PARTNER_STATE_SET(_p, _f) SET_FLAGS((_p)->partner_state, STATE_ ## _f)
 #define PARTNER_STATE_CLR(_p, _f) CLEAR_FLAGS((_p)->partner_state, STATE_ ## _f)
 
-/** Variables associated with each port (5.4.7 in 802.1AX documentation). */
+/** Variables associated with each port (6.4.7 in 802.1AX documentation). */
 struct port {
 	/**
 	 * The operational values of the Actor's state parameters. Bitmask
@@ -124,7 +124,7 @@ struct port {
 	uint64_t wait_while_timer;
 	uint64_t tx_machine_timer;
 	uint64_t tx_marker_timer;
-	/* Agregator parameters */
+	/* Aggregator parameters */
 	/** Used aggregator port ID */
 	uint16_t aggregator_port_id;
 
@@ -280,7 +280,7 @@ bond_mode_8023ad_activate_slave(struct rte_eth_dev *dev, uint16_t port_id);
 /**
  * @internal
  *
- * Denitializes and removes given slave from 802.1AX mode.
+ * Deinitializes and removes given slave from 802.1AX mode.
  *
  * @param dev       Bonded interface.
  * @param slave_num Position of slave in active_slaves array
diff --git a/drivers/net/bonding/rte_eth_bond_8023ad.c b/drivers/net/bonding/rte_eth_bond_8023ad.c
index ca50583..43231bc 100644
--- a/drivers/net/bonding/rte_eth_bond_8023ad.c
+++ b/drivers/net/bonding/rte_eth_bond_8023ad.c
@@ -207,15 +207,15 @@ show_warnings(uint16_t slave_id)
 	if (warnings & WRN_RX_QUEUE_FULL) {
 		RTE_BOND_LOG(DEBUG,
 			     "Slave %u: failed to enqueue LACP packet into RX ring.\n"
-			     "Receive and transmit functions must be invoked on bonded"
-			     "interface at least 10 times per second or LACP will notwork correctly",
+			     "Receive and transmit functions must be invoked on bonded\n"
+			     "interface at least 10 times per second or LACP will not work correctly",
 			     slave_id);
 	}
 
 	if (warnings & WRN_TX_QUEUE_FULL) {
 		RTE_BOND_LOG(DEBUG,
 			     "Slave %u: failed to enqueue LACP packet into TX ring.\n"
-			     "Receive and transmit functions must be invoked on bonded"
+			     "Receive and transmit functions must be invoked on bonded\n"
 			     "interface at least 10 times per second or LACP will not work correctly",
 			     slave_id);
 	}
@@ -250,7 +250,7 @@ record_default(struct port *port)
 
 /** Function handles rx state machine.
  *
- * This function implements Receive State Machine from point 5.4.12 in
+ * This function implements Receive State Machine from point 6.4.12 in
  * 802.1AX documentation. It should be called periodically.
  *
  * @param lacpdu		LACPDU received.
@@ -384,7 +384,7 @@ rx_machine(struct bond_dev_private *internals, uint16_t slave_id,
 /**
  * Function handles periodic tx state machine.
  *
- * Function implements Periodic Transmission state machine from point 5.4.13
+ * Function implements Periodic Transmission state machine from point 6.4.13
  * in 802.1AX documentation. It should be called periodically.
  *
  * @param port			Port to handle state machine.
@@ -446,7 +446,7 @@ periodic_machine(struct bond_dev_private *internals, uint16_t slave_id)
 /**
  * Function handles mux state machine.
  *
- * Function implements Mux Machine from point 5.4.15 in 802.1AX documentation.
+ * Function implements Mux Machine from point 6.4.15 in 802.1AX documentation.
  * It should be called periodically.
  *
  * @param port			Port to handle state machine.
@@ -549,7 +549,7 @@ mux_machine(struct bond_dev_private *internals, uint16_t slave_id)
 /**
  * Function handles transmit state machine.
  *
- * Function implements Transmit Machine from point 5.4.16 in 802.1AX
+ * Function implements Transmit Machine from point 6.4.16 in 802.1AX
  * documentation.
  *
  * @param port
@@ -1051,14 +1051,14 @@ bond_mode_8023ad_activate_slave(struct rte_eth_dev *bond_dev,
 	struct bond_tx_queue *bd_tx_q;
 	uint16_t q_id;
 
-	/* Given slave mus not be in active list */
+	/* Given slave must not be in active list. */
 	RTE_ASSERT(find_slave_by_id(internals->active_slaves,
 	internals->active_slave_count, slave_id) == internals->active_slave_count);
 	RTE_SET_USED(internals); /* used only for assert when enabled */
 
 	memcpy(&port->actor, &initial, sizeof(struct port_params));
-	/* Standard requires that port ID must be grater than 0.
-	 * Add 1 do get corresponding port_number */
+	/* Standard requires that port ID must be greater than 0.
+	 * Add 1 to get corresponding port_number. */
 	port->actor.port_number = rte_cpu_to_be_16(slave_id + 1);
 
 	memcpy(&port->partner, &initial, sizeof(struct port_params));
@@ -1069,7 +1069,7 @@ bond_mode_8023ad_activate_slave(struct rte_eth_dev *bond_dev,
 	port->partner_state = STATE_LACP_ACTIVE | STATE_AGGREGATION;
 	port->sm_flags = SM_FLAGS_BEGIN;
 
-	/* use this port as agregator */
+	/* Use this port as aggregator. */
 	port->aggregator_port_id = slave_id;
 
 	if (bond_mode_8023ad_register_lacp_mac(slave_id) < 0) {
diff --git a/drivers/net/bonding/rte_eth_bond_8023ad.h b/drivers/net/bonding/rte_eth_bond_8023ad.h
index 11a71a5..7e9a018 100644
--- a/drivers/net/bonding/rte_eth_bond_8023ad.h
+++ b/drivers/net/bonding/rte_eth_bond_8023ad.h
@@ -68,7 +68,7 @@ struct port_params {
 	struct rte_ether_addr system;
 	/**< System ID - Slave MAC address, same as bonding MAC address */
 	uint16_t key;
-	/**< Speed information (implementation dependednt) and duplex. */
+	/**< Speed information (implementation dependent) and duplex. */
 	uint16_t port_priority;
 	/**< Priority of this (unused in current implementation) */
 	uint16_t port_number;
@@ -83,7 +83,7 @@ struct lacpdu_actor_partner_params {
 	uint8_t reserved_3[3];
 } __rte_packed __rte_aligned(2);
 
-/** LACPDU structure (5.4.2 in 802.1AX documentation). */
+/** LACPDU structure (6.4.2 in 802.1AX documentation). */
 struct lacpdu {
 	uint8_t subtype;
 	uint8_t version_number;
@@ -153,7 +153,7 @@ struct rte_eth_bond_8023ad_slave_info {
 /**
  * @internal
  *
- * Function returns current configuration of 802.3AX mode.
+ * Function returns current configuration of 802.1AX mode.
  *
  * @param port_id   Bonding device id
  * @param conf		Pointer to timeout structure.
@@ -197,10 +197,6 @@ int
 rte_eth_bond_8023ad_slave_info(uint16_t port_id, uint16_t slave_id,
 		struct rte_eth_bond_8023ad_slave_info *conf);
 
-#ifdef __cplusplus
-}
-#endif
-
 /**
  * Configure a slave port to start collecting.
  *
@@ -331,4 +327,9 @@ rte_eth_bond_8023ad_agg_selection_get(uint16_t port_id);
 int
 rte_eth_bond_8023ad_agg_selection_set(uint16_t port_id,
 		enum rte_bond_8023ad_agg_selection agg_selection);
+
+#ifdef __cplusplus
+}
+#endif
+
 #endif /* RTE_ETH_BOND_8023AD_H_ */
diff --git a/drivers/net/bonding/rte_eth_bond_pmd.c b/drivers/net/bonding/rte_eth_bond_pmd.c
index 84f4900..f6003b0 100644
--- a/drivers/net/bonding/rte_eth_bond_pmd.c
+++ b/drivers/net/bonding/rte_eth_bond_pmd.c
@@ -158,7 +158,8 @@ const struct rte_flow_attr flow_attr_8023ad = {
 
 int
 bond_ethdev_8023ad_flow_verify(struct rte_eth_dev *bond_dev,
-		uint16_t slave_port) {
+		uint16_t slave_port)
+{
 	struct rte_eth_dev_info slave_info;
 	struct rte_flow_error error;
 	struct bond_dev_private *internals = bond_dev->data->dev_private;
@@ -207,7 +208,8 @@ bond_ethdev_8023ad_flow_verify(struct rte_eth_dev *bond_dev,
 }
 
 int
-bond_8023ad_slow_pkt_hw_filter_supported(uint16_t port_id) {
+bond_8023ad_slow_pkt_hw_filter_supported(uint16_t port_id)
+{
 	struct rte_eth_dev *bond_dev = &rte_eth_devices[port_id];
 	struct bond_dev_private *internals = bond_dev->data->dev_private;
 	struct rte_eth_dev_info bond_info;
@@ -240,8 +242,8 @@ bond_8023ad_slow_pkt_hw_filter_supported(uint16_t port_id) {
 }
 
 int
-bond_ethdev_8023ad_flow_set(struct rte_eth_dev *bond_dev, uint16_t slave_port) {
-
+bond_ethdev_8023ad_flow_set(struct rte_eth_dev *bond_dev, uint16_t slave_port)
+{
 	struct rte_flow_error error;
 	struct bond_dev_private *internals = bond_dev->data->dev_private;
 	struct rte_flow_action_queue lacp_queue_conf = {
@@ -809,7 +811,8 @@ struct bwg_slave {
 };
 
 void
-bond_tlb_activate_slave(struct bond_dev_private *internals) {
+bond_tlb_activate_slave(struct bond_dev_private *internals)
+{
 	int i;
 
 	for (i = 0; i < internals->active_slave_count; i++) {
-- 
2.7.4

[PATCH v2 2/8] net/bonding: fix bonded dev configuring slave dev

[Robert Sanford]

- Replace directly overwriting of slave port's private rte_eth_conf
  by copying it, and then updating it via rte_eth_dev_configure().

Signed-off-by: Robert Sanford <rsanford@akamai.com>
---
 drivers/net/bonding/rte_eth_bond_pmd.c | 30 ++++++++++++++++--------------
 1 file changed, 16 insertions(+), 14 deletions(-)

diff --git a/drivers/net/bonding/rte_eth_bond_pmd.c b/drivers/net/bonding/rte_eth_bond_pmd.c
index f6003b0..b9e7439 100644
--- a/drivers/net/bonding/rte_eth_bond_pmd.c
+++ b/drivers/net/bonding/rte_eth_bond_pmd.c
@@ -1691,6 +1691,7 @@ slave_configure(struct rte_eth_dev *bonded_eth_dev,
 	struct rte_flow_error flow_error;
 
 	struct bond_dev_private *internals = bonded_eth_dev->data->dev_private;
+	struct rte_eth_conf dev_conf;
 
 	/* Stop slave */
 	errval = rte_eth_dev_stop(slave_eth_dev->data->port_id);
@@ -1698,34 +1699,36 @@ slave_configure(struct rte_eth_dev *bonded_eth_dev,
 		RTE_BOND_LOG(ERR, "rte_eth_dev_stop: port %u, err (%d)",
 			     slave_eth_dev->data->port_id, errval);
 
+	/* Start with a copy of slave's current rte_eth_conf. */
+	dev_conf = slave_eth_dev->data->dev_conf;
+	dev_conf.rx_adv_conf.rss_conf.rss_key = NULL;
+
 	/* Enable interrupts on slave device if supported */
-	if (slave_eth_dev->data->dev_flags & RTE_ETH_DEV_INTR_LSC)
-		slave_eth_dev->data->dev_conf.intr_conf.lsc = 1;
+	dev_conf.intr_conf.lsc =
+		(slave_eth_dev->data->dev_flags & RTE_ETH_DEV_INTR_LSC) ? 1 : 0;
 
 	/* If RSS is enabled for bonding, try to enable it for slaves  */
 	if (bonded_eth_dev->data->dev_conf.rxmode.mq_mode & RTE_ETH_MQ_RX_RSS_FLAG) {
 		/* rss_key won't be empty if RSS is configured in bonded dev */
-		slave_eth_dev->data->dev_conf.rx_adv_conf.rss_conf.rss_key_len =
-					internals->rss_key_len;
-		slave_eth_dev->data->dev_conf.rx_adv_conf.rss_conf.rss_key =
-					internals->rss_key;
+		dev_conf.rx_adv_conf.rss_conf.rss_key_len =
+				internals->rss_key_len;
+		dev_conf.rx_adv_conf.rss_conf.rss_key = internals->rss_key;
 
-		slave_eth_dev->data->dev_conf.rx_adv_conf.rss_conf.rss_hf =
+		dev_conf.rx_adv_conf.rss_conf.rss_hf =
 				bonded_eth_dev->data->dev_conf.rx_adv_conf.rss_conf.rss_hf;
-		slave_eth_dev->data->dev_conf.rxmode.mq_mode =
+		dev_conf.rxmode.mq_mode =
 				bonded_eth_dev->data->dev_conf.rxmode.mq_mode;
 	}
 
 	if (bonded_eth_dev->data->dev_conf.rxmode.offloads &
 			RTE_ETH_RX_OFFLOAD_VLAN_FILTER)
-		slave_eth_dev->data->dev_conf.rxmode.offloads |=
+		dev_conf.rxmode.offloads |=
 				RTE_ETH_RX_OFFLOAD_VLAN_FILTER;
 	else
-		slave_eth_dev->data->dev_conf.rxmode.offloads &=
+		dev_conf.rxmode.offloads &=
 				~RTE_ETH_RX_OFFLOAD_VLAN_FILTER;
 
-	slave_eth_dev->data->dev_conf.rxmode.mtu =
-			bonded_eth_dev->data->dev_conf.rxmode.mtu;
+	dev_conf.rxmode.mtu = bonded_eth_dev->data->dev_conf.rxmode.mtu;
 
 	nb_rx_queues = bonded_eth_dev->data->nb_rx_queues;
 	nb_tx_queues = bonded_eth_dev->data->nb_tx_queues;
@@ -1747,8 +1750,7 @@ slave_configure(struct rte_eth_dev *bonded_eth_dev,
 
 	/* Configure device */
 	errval = rte_eth_dev_configure(slave_eth_dev->data->port_id,
-			nb_rx_queues, nb_tx_queues,
-			&(slave_eth_dev->data->dev_conf));
+			nb_rx_queues, nb_tx_queues, &dev_conf);
 	if (errval != 0) {
 		RTE_BOND_LOG(ERR, "Cannot configure slave device: port %u, err (%d)",
 				slave_eth_dev->data->port_id, errval);
-- 
2.7.4

[PATCH v2 3/8] net/bonding: change mbuf pool and ring creation

[Robert Sanford]

- Turn off mbuf pool caching to avoid mbufs lingering in pool caches.
  At most, we transmit one LACPDU per second, per port.
  LACP tx_machine() performs the "get", and runs in the context of the
  interrupt thread (no default cache).
  PMD typically "puts" no more than one LACPDU per second, on average.
- Create rings with RING_F_EXACT_SZ flag, so that they are the desired
  size, and not one less than requested.

Signed-off-by: Robert Sanford <rsanford@akamai.com>
---
 drivers/net/bonding/rte_eth_bond_8023ad.c | 8 +++-----
 1 file changed, 3 insertions(+), 5 deletions(-)

diff --git a/drivers/net/bonding/rte_eth_bond_8023ad.c b/drivers/net/bonding/rte_eth_bond_8023ad.c
index 43231bc..9ed2a46 100644
--- a/drivers/net/bonding/rte_eth_bond_8023ad.c
+++ b/drivers/net/bonding/rte_eth_bond_8023ad.c
@@ -1101,9 +1101,7 @@ bond_mode_8023ad_activate_slave(struct rte_eth_dev *bond_dev,
 	}
 
 	snprintf(mem_name, RTE_DIM(mem_name), "slave_port%u_pool", slave_id);
-	port->mbuf_pool = rte_pktmbuf_pool_create(mem_name, total_tx_desc,
-		RTE_MEMPOOL_CACHE_MAX_SIZE >= 32 ?
-			32 : RTE_MEMPOOL_CACHE_MAX_SIZE,
+	port->mbuf_pool = rte_pktmbuf_pool_create(mem_name, total_tx_desc, 0,
 		0, element_size, socket_id);
 
 	/* Any memory allocation failure in initialization is critical because
@@ -1115,7 +1113,7 @@ bond_mode_8023ad_activate_slave(struct rte_eth_dev *bond_dev,
 
 	snprintf(mem_name, RTE_DIM(mem_name), "slave_%u_rx", slave_id);
 	port->rx_ring = rte_ring_create(mem_name,
-			rte_align32pow2(BOND_MODE_8023AX_SLAVE_RX_PKTS), socket_id, 0);
+		BOND_MODE_8023AX_SLAVE_RX_PKTS, socket_id, RING_F_EXACT_SZ);
 
 	if (port->rx_ring == NULL) {
 		rte_panic("Slave %u: Failed to create rx ring '%s': %s\n", slave_id,
@@ -1125,7 +1123,7 @@ bond_mode_8023ad_activate_slave(struct rte_eth_dev *bond_dev,
 	/* TX ring is at least one pkt longer to make room for marker packet. */
 	snprintf(mem_name, RTE_DIM(mem_name), "slave_%u_tx", slave_id);
 	port->tx_ring = rte_ring_create(mem_name,
-			rte_align32pow2(BOND_MODE_8023AX_SLAVE_TX_PKTS + 1), socket_id, 0);
+		BOND_MODE_8023AX_SLAVE_TX_PKTS + 1, socket_id, RING_F_EXACT_SZ);
 
 	if (port->tx_ring == NULL) {
 		rte_panic("Slave %u: Failed to create tx ring '%s': %s\n", slave_id,
-- 
2.7.4

[PATCH v2 4/8] net/bonding: support enabling LACP short timeout

[Robert Sanford]

- Add support for enabling LACP short timeout, i.e., link partner can
  use fast periodic time interval between transmits.

Signed-off-by: Robert Sanford <rsanford@akamai.com>
---
 drivers/net/bonding/eth_bond_8023ad_private.h |  3 ++-
 drivers/net/bonding/rte_eth_bond_8023ad.c     | 28 +++++++++++++++++++++++----
 drivers/net/bonding/rte_eth_bond_8023ad.h     |  3 +++
 3 files changed, 29 insertions(+), 5 deletions(-)

diff --git a/drivers/net/bonding/eth_bond_8023ad_private.h b/drivers/net/bonding/eth_bond_8023ad_private.h
index 60db31e..bfde03c 100644
--- a/drivers/net/bonding/eth_bond_8023ad_private.h
+++ b/drivers/net/bonding/eth_bond_8023ad_private.h
@@ -159,7 +159,6 @@ struct mode8023ad_private {
 	uint64_t rx_marker_timeout;
 	uint64_t update_timeout_us;
 	rte_eth_bond_8023ad_ext_slowrx_fn slowrx_cb;
-	uint8_t external_sm;
 	struct rte_ether_addr mac_addr;
 
 	struct rte_eth_link slave_link;
@@ -178,6 +177,8 @@ struct mode8023ad_private {
 		uint16_t tx_qid;
 	} dedicated_queues;
 	enum rte_bond_8023ad_agg_selection agg_selection;
+	uint8_t short_timeout_enabled : 1;
+	uint8_t short_timeout_updated : 1;
 };
 
 /**
diff --git a/drivers/net/bonding/rte_eth_bond_8023ad.c b/drivers/net/bonding/rte_eth_bond_8023ad.c
index 9ed2a46..5c175e7 100644
--- a/drivers/net/bonding/rte_eth_bond_8023ad.c
+++ b/drivers/net/bonding/rte_eth_bond_8023ad.c
@@ -868,10 +868,10 @@ bond_mode_8023ad_periodic_cb(void *arg)
 	struct rte_eth_link link_info;
 	struct rte_ether_addr slave_addr;
 	struct rte_mbuf *lacp_pkt = NULL;
+	uint8_t short_timeout_updated = internals->mode4.short_timeout_updated;
 	uint16_t slave_id;
 	uint16_t i;
 
-
 	/* Update link status on each port */
 	for (i = 0; i < internals->active_slave_count; i++) {
 		uint16_t key;
@@ -916,6 +916,13 @@ bond_mode_8023ad_periodic_cb(void *arg)
 		slave_id = internals->active_slaves[i];
 		port = &bond_mode_8023ad_ports[slave_id];
 
+		if (short_timeout_updated) {
+			if (internals->mode4.short_timeout_enabled)
+				ACTOR_STATE_SET(port, LACP_SHORT_TIMEOUT);
+			else
+				ACTOR_STATE_CLR(port, LACP_SHORT_TIMEOUT);
+		}
+
 		if ((port->actor.key &
 				rte_cpu_to_be_16(BOND_LINK_FULL_DUPLEX_KEY)) == 0) {
 
@@ -960,6 +967,9 @@ bond_mode_8023ad_periodic_cb(void *arg)
 		show_warnings(slave_id);
 	}
 
+	if (short_timeout_updated)
+		internals->mode4.short_timeout_updated = 0;
+
 	rte_eal_alarm_set(internals->mode4.update_timeout_us,
 			bond_mode_8023ad_periodic_cb, arg);
 }
@@ -1054,7 +1064,6 @@ bond_mode_8023ad_activate_slave(struct rte_eth_dev *bond_dev,
 	/* Given slave must not be in active list. */
 	RTE_ASSERT(find_slave_by_id(internals->active_slaves,
 	internals->active_slave_count, slave_id) == internals->active_slave_count);
-	RTE_SET_USED(internals); /* used only for assert when enabled */
 
 	memcpy(&port->actor, &initial, sizeof(struct port_params));
 	/* Standard requires that port ID must be greater than 0.
@@ -1065,7 +1074,9 @@ bond_mode_8023ad_activate_slave(struct rte_eth_dev *bond_dev,
 	memcpy(&port->partner_admin, &initial, sizeof(struct port_params));
 
 	/* default states */
-	port->actor_state = STATE_AGGREGATION | STATE_LACP_ACTIVE | STATE_DEFAULTED;
+	port->actor_state = STATE_AGGREGATION | STATE_LACP_ACTIVE |
+		STATE_DEFAULTED | (internals->mode4.short_timeout_enabled ?
+		STATE_LACP_SHORT_TIMEOUT : 0);
 	port->partner_state = STATE_LACP_ACTIVE | STATE_AGGREGATION;
 	port->sm_flags = SM_FLAGS_BEGIN;
 
@@ -1209,6 +1220,7 @@ bond_mode_8023ad_conf_get(struct rte_eth_dev *dev,
 	struct mode8023ad_private *mode4 = &internals->mode4;
 	uint64_t ms_ticks = rte_get_tsc_hz() / 1000;
 
+	memset(conf, 0, sizeof(*conf));
 	conf->fast_periodic_ms = mode4->fast_periodic_timeout / ms_ticks;
 	conf->slow_periodic_ms = mode4->slow_periodic_timeout / ms_ticks;
 	conf->short_timeout_ms = mode4->short_timeout / ms_ticks;
@@ -1219,6 +1231,7 @@ bond_mode_8023ad_conf_get(struct rte_eth_dev *dev,
 	conf->rx_marker_period_ms = mode4->rx_marker_timeout / ms_ticks;
 	conf->slowrx_cb = mode4->slowrx_cb;
 	conf->agg_selection = mode4->agg_selection;
+	conf->lacp_timeout_control = mode4->short_timeout_enabled;
 }
 
 static void
@@ -1234,6 +1247,7 @@ bond_mode_8023ad_conf_get_default(struct rte_eth_bond_8023ad_conf *conf)
 	conf->update_timeout_ms = BOND_MODE_8023AX_UPDATE_TIMEOUT_MS;
 	conf->slowrx_cb = NULL;
 	conf->agg_selection = AGG_STABLE;
+	conf->lacp_timeout_control = 0;
 }
 
 static void
@@ -1274,6 +1288,11 @@ bond_mode_8023ad_setup(struct rte_eth_dev *dev,
 	mode4->slowrx_cb = conf->slowrx_cb;
 	mode4->agg_selection = AGG_STABLE;
 
+	if (mode4->short_timeout_enabled != conf->lacp_timeout_control) {
+		mode4->short_timeout_enabled = conf->lacp_timeout_control;
+		mode4->short_timeout_updated = 1;
+	}
+
 	if (dev->data->dev_started)
 		bond_mode_8023ad_start(dev);
 }
@@ -1478,7 +1497,8 @@ bond_8023ad_setup_validate(uint16_t port_id,
 				conf->aggregate_wait_timeout_ms == 0 ||
 				conf->tx_period_ms == 0 ||
 				conf->rx_marker_period_ms == 0 ||
-				conf->update_timeout_ms == 0) {
+				conf->update_timeout_ms == 0 ||
+				conf->lacp_timeout_control > 1) {
 			RTE_BOND_LOG(ERR, "given mode 4 configuration is invalid");
 			return -EINVAL;
 		}
diff --git a/drivers/net/bonding/rte_eth_bond_8023ad.h b/drivers/net/bonding/rte_eth_bond_8023ad.h
index 7e9a018..87f6b2f 100644
--- a/drivers/net/bonding/rte_eth_bond_8023ad.h
+++ b/drivers/net/bonding/rte_eth_bond_8023ad.h
@@ -139,6 +139,9 @@ struct rte_eth_bond_8023ad_conf {
 	uint32_t update_timeout_ms;
 	rte_eth_bond_8023ad_ext_slowrx_fn slowrx_cb;
 	enum rte_bond_8023ad_agg_selection agg_selection;
+	uint8_t lacp_timeout_control;
+	/**< LACPDU.Actor_State.LACP_Timeout flag: 0=Long 1=Short. */
+	uint8_t reserved_8s[3];
 };
 
 struct rte_eth_bond_8023ad_slave_info {
-- 
2.7.4

[PATCH v2 5/8] net/bonding: add bond_8023ad and bond_alb to doc

[Robert Sanford]

- Add bond_8023ad and bond_alb to API documentation.

Signed-off-by: Robert Sanford <rsanford@akamai.com>
---
 doc/api/doxy-api-index.md | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/doc/api/doxy-api-index.md b/doc/api/doxy-api-index.md
index 4245b96..830235c 100644
--- a/doc/api/doxy-api-index.md
+++ b/doc/api/doxy-api-index.md
@@ -39,6 +39,8 @@ The public API headers are grouped by topics:
 - **device specific**:
   [softnic]            (@ref rte_eth_softnic.h),
   [bond]               (@ref rte_eth_bond.h),
+  [bond_8023ad]        (@ref rte_eth_bond_8023ad.h),
+  [bond_alb]           (@ref rte_eth_bond_alb.h),
   [vhost]              (@ref rte_vhost.h),
   [vdpa]               (@ref rte_vdpa.h),
   [KNI]                (@ref rte_kni.h),
-- 
2.7.4

[PATCH v2 6/8] remove self from timers maintainers

[Robert Sanford]

Remove self from Timers maintainers.

Signed-off-by: Robert Sanford <rsanford@akamai.com>
---
 MAINTAINERS | 1 -
 1 file changed, 1 deletion(-)

diff --git a/MAINTAINERS b/MAINTAINERS
index 18d9eda..32663b0 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -1613,7 +1613,6 @@ F: examples/vm_power_manager/
 F: doc/guides/sample_app_ug/vm_power_management.rst
 
 Timers
-M: Robert Sanford <rsanford@akamai.com>
 M: Erik Gabriel Carrillo <erik.g.carrillo@intel.com>
 F: lib/timer/
 F: doc/guides/prog_guide/timer_lib.rst
-- 
2.7.4

[PATCH v2 7/8] net/ring: add promisc and all-MC stubs

[Robert Sanford]

Add promiscuous_enable, promiscuous_disable, allmulticast_enable,
and allmulticast_disable API stubs.
This helps clean up errors in dpdk-test link_bonding_mode4_autotest.

Signed-off-by: Robert Sanford <rsanford@akamai.com>
---
 drivers/net/ring/rte_eth_ring.c | 28 ++++++++++++++++++++++++++++
 1 file changed, 28 insertions(+)

diff --git a/drivers/net/ring/rte_eth_ring.c b/drivers/net/ring/rte_eth_ring.c
index db10f03..cfb81da 100644
--- a/drivers/net/ring/rte_eth_ring.c
+++ b/drivers/net/ring/rte_eth_ring.c
@@ -226,6 +226,30 @@ eth_mac_addr_add(struct rte_eth_dev *dev __rte_unused,
 }
 
 static int
+eth_promiscuous_enable(struct rte_eth_dev *dev __rte_unused)
+{
+	return 0;
+}
+
+static int
+eth_promiscuous_disable(struct rte_eth_dev *dev __rte_unused)
+{
+	return 0;
+}
+
+static int
+eth_allmulticast_enable(struct rte_eth_dev *dev __rte_unused)
+{
+	return 0;
+}
+
+static int
+eth_allmulticast_disable(struct rte_eth_dev *dev __rte_unused)
+{
+	return 0;
+}
+
+static int
 eth_link_update(struct rte_eth_dev *dev __rte_unused,
 		int wait_to_complete __rte_unused) { return 0; }
 
@@ -275,6 +299,10 @@ static const struct eth_dev_ops ops = {
 	.stats_reset = eth_stats_reset,
 	.mac_addr_remove = eth_mac_addr_remove,
 	.mac_addr_add = eth_mac_addr_add,
+	.promiscuous_enable = eth_promiscuous_enable,
+	.promiscuous_disable = eth_promiscuous_disable,
+	.allmulticast_enable = eth_allmulticast_enable,
+	.allmulticast_disable = eth_allmulticast_disable,
 };
 
 static int
-- 
2.7.4

[PATCH v2 8/8] net/bonding: add LACP short timeout tests

[Robert Sanford]

- Add "set bonding lacp timeout_ctrl <port_id> on|off" to testpmd.
- Add "test_mode4_lacp_timeout_control" to dpdk-test.
- Remove call to rte_eth_dev_mac_addr_remove from add_slave,
  as it always fails and prints an error.

Signed-off-by: Robert Sanford <rsanford@akamai.com>
---
 app/test-pmd/cmdline.c             | 77 ++++++++++++++++++++++++++++++++++++++
 app/test/test_link_bonding_mode4.c | 70 +++++++++++++++++++++++++++++++++-
 2 files changed, 145 insertions(+), 2 deletions(-)

diff --git a/app/test-pmd/cmdline.c b/app/test-pmd/cmdline.c
index 9fd2c2a..b0c2fb4 100644
--- a/app/test-pmd/cmdline.c
+++ b/app/test-pmd/cmdline.c
@@ -633,6 +633,9 @@ static void cmd_help_long_parsed(void *parsed_result,
 			"set bonding mode IEEE802.3AD aggregator policy (port_id) (agg_name)\n"
 			"	Set Aggregation mode for IEEE802.3AD (mode 4)\n\n"
 
+			"set bonding lacp timeout_ctrl (port_id) (on|off)\n"
+				"Configure LACP partner to use fast|slow periodic tx interval.\n\n"
+
 			"set bonding balance_xmit_policy (port_id) (l2|l23|l34)\n"
 			"	Set the transmit balance policy for bonded device running in balance mode.\n\n"
 
@@ -6192,6 +6195,7 @@ static void lacp_conf_show(struct rte_eth_bond_8023ad_conf *conf)
 		printf("\taggregation mode: invalid\n");
 		break;
 	}
+	printf("\tlacp timeout control: %u\n", conf->lacp_timeout_control);
 
 	printf("\n");
 }
@@ -6863,6 +6867,78 @@ cmdline_parse_inst_t cmd_set_bonding_agg_mode_policy = {
 };
 
 
+/* *** SET LACP TIMEOUT CONTROL ON BONDED DEVICE *** */
+struct cmd_set_lacp_timeout_control_result {
+	cmdline_fixed_string_t set;
+	cmdline_fixed_string_t bonding;
+	cmdline_fixed_string_t lacp;
+	cmdline_fixed_string_t timeout_ctrl;
+	uint16_t port_id;
+	cmdline_fixed_string_t on_off;
+};
+
+static void
+cmd_set_lacp_timeout_control_parsed(void *parsed_result,
+		__rte_unused struct cmdline *cl,
+		__rte_unused void *data)
+{
+	struct cmd_set_lacp_timeout_control_result *res = parsed_result;
+	struct rte_eth_bond_8023ad_conf port_conf;
+	uint8_t on_off = 0;
+	int ret;
+
+	if (!strcmp(res->on_off, "on"))
+		on_off = 1;
+
+	ret = rte_eth_bond_8023ad_conf_get(res->port_id, &port_conf);
+	if (ret != 0) {
+		fprintf(stderr, "\tGet bonded device %u lacp conf failed\n",
+			res->port_id);
+		return;
+	}
+
+	port_conf.lacp_timeout_control = on_off;
+	ret = rte_eth_bond_8023ad_setup(res->port_id, &port_conf);
+	if (ret != 0)
+		fprintf(stderr, "\tSetup bonded device %u lacp conf failed\n",
+			res->port_id);
+}
+
+cmdline_parse_token_string_t cmd_set_lacp_timeout_control_set =
+	TOKEN_STRING_INITIALIZER(struct cmd_set_lacp_timeout_control_result,
+				set, "set");
+cmdline_parse_token_string_t cmd_set_lacp_timeout_control_bonding =
+	TOKEN_STRING_INITIALIZER(struct cmd_set_lacp_timeout_control_result,
+				bonding, "bonding");
+cmdline_parse_token_string_t cmd_set_lacp_timeout_control_lacp =
+	TOKEN_STRING_INITIALIZER(struct cmd_set_lacp_timeout_control_result,
+				lacp, "lacp");
+cmdline_parse_token_string_t cmd_set_lacp_timeout_control_timeout_ctrl =
+	TOKEN_STRING_INITIALIZER(struct cmd_set_lacp_timeout_control_result,
+				timeout_ctrl, "timeout_ctrl");
+cmdline_parse_token_num_t cmd_set_lacp_timeout_control_port_id =
+	TOKEN_NUM_INITIALIZER(struct cmd_set_lacp_timeout_control_result,
+				port_id, RTE_UINT16);
+cmdline_parse_token_string_t cmd_set_lacp_timeout_control_on_off =
+	TOKEN_STRING_INITIALIZER(struct cmd_set_lacp_timeout_control_result,
+				on_off, "on#off");
+
+cmdline_parse_inst_t cmd_set_lacp_timeout_control = {
+	.f = cmd_set_lacp_timeout_control_parsed,
+	.data = (void *) 0,
+	.help_str = "set bonding lacp timeout_ctrl <port_id> on|off: "
+		"Configure partner to use fast|slow periodic tx interval",
+	.tokens = {
+		(void *)&cmd_set_lacp_timeout_control_set,
+		(void *)&cmd_set_lacp_timeout_control_bonding,
+		(void *)&cmd_set_lacp_timeout_control_lacp,
+		(void *)&cmd_set_lacp_timeout_control_timeout_ctrl,
+		(void *)&cmd_set_lacp_timeout_control_port_id,
+		(void *)&cmd_set_lacp_timeout_control_on_off,
+		NULL
+	}
+};
+
 #endif /* RTE_NET_BOND */
 
 /* *** SET FORWARDING MODE *** */
@@ -17728,6 +17804,7 @@ cmdline_parse_ctx_t main_ctx[] = {
 	(cmdline_parse_inst_t *) &cmd_set_bond_mon_period,
 	(cmdline_parse_inst_t *) &cmd_set_lacp_dedicated_queues,
 	(cmdline_parse_inst_t *) &cmd_set_bonding_agg_mode_policy,
+	(cmdline_parse_inst_t *) &cmd_set_lacp_timeout_control,
 #endif
 	(cmdline_parse_inst_t *)&cmd_vlan_offload,
 	(cmdline_parse_inst_t *)&cmd_vlan_tpid,
diff --git a/app/test/test_link_bonding_mode4.c b/app/test/test_link_bonding_mode4.c
index 2be86d5..bc73183 100644
--- a/app/test/test_link_bonding_mode4.c
+++ b/app/test/test_link_bonding_mode4.c
@@ -235,8 +235,6 @@ add_slave(struct slave_conf *slave, uint8_t start)
 	rte_ether_addr_copy(&slave_mac_default, &addr);
 	addr.addr_bytes[RTE_ETHER_ADDR_LEN - 1] = slave->port_id;
 
-	rte_eth_dev_mac_addr_remove(slave->port_id, &addr);
-
 	TEST_ASSERT_SUCCESS(rte_eth_dev_mac_addr_add(slave->port_id, &addr, 0),
 		"Failed to set slave MAC address");
 
@@ -735,6 +733,66 @@ test_mode4_agg_mode_selection(void)
 }
 
 static int
+test_mode4_lacp_timeout_control(void)
+{
+	int retval;
+	int iterations;
+	size_t i;
+	struct slave_conf *slave;
+	uint16_t port_id = test_params.bonded_port_id;
+	struct rte_eth_bond_8023ad_conf conf;
+	struct rte_eth_bond_8023ad_slave_info info;
+	uint8_t on_off = 0;
+	uint8_t lacp_timeout_flag = 0;
+
+	retval = initialize_bonded_device_with_slaves(TEST_LACP_SLAVE_COUT, 0);
+	TEST_ASSERT_SUCCESS(retval, "Failed to initialize bonded device");
+
+	/* Iteration 0: Verify that LACP timeout control is off by default.
+	 * Iteration 1: Verify that we can set LACP timeout control.
+	 * Iteration 2: Verify that we can reset LACP timeout control.
+	 */
+	for (iterations = 0; iterations < 3; iterations++) {
+		/* Verify that bond conf has expected timeout control value.*/
+		retval = rte_eth_bond_8023ad_conf_get(port_id, &conf);
+		TEST_ASSERT_SUCCESS(retval, "Failed to get LACP conf");
+		TEST_ASSERT_EQUAL(conf.lacp_timeout_control, on_off,
+			"Wrong LACP timeout control value");
+
+		/* State machine must run to propagate new timeout control
+		 * value to slaves (iterations 1 and 2).
+		 */
+		retval = bond_handshake();
+		TEST_ASSERT_SUCCESS(retval, "Bond handshake failed");
+		retval = bond_handshake();
+		TEST_ASSERT_SUCCESS(retval, "Bond handshake failed");
+
+		/* Verify that slaves' actor states have expected value.*/
+		FOR_EACH_PORT(i, slave) {
+			retval = rte_eth_bond_8023ad_slave_info(port_id,
+				slave->port_id, &info);
+			TEST_ASSERT_SUCCESS(retval,
+				"Failed to get LACP slave info");
+			TEST_ASSERT_EQUAL((info.actor_state &
+				STATE_LACP_SHORT_TIMEOUT), lacp_timeout_flag,
+				" Wrong LACP slave info timeout flag");
+		}
+
+		/* Toggle timeout control. */
+		on_off ^= 1;
+		lacp_timeout_flag ^= STATE_LACP_SHORT_TIMEOUT;
+		conf.lacp_timeout_control = on_off;
+		retval = rte_eth_bond_8023ad_setup(port_id, &conf);
+		TEST_ASSERT_SUCCESS(retval, "Failed to setup LACP conf");
+	}
+
+	retval = remove_slaves_and_stop_bonded_device();
+	TEST_ASSERT_SUCCESS(retval, "Test cleanup failed.");
+
+	return TEST_SUCCESS;
+}
+
+static int
 generate_packets(struct rte_ether_addr *src_mac,
 	struct rte_ether_addr *dst_mac, uint16_t count, struct rte_mbuf **buf)
 {
@@ -1649,6 +1707,12 @@ test_mode4_ext_lacp_wrapper(void)
 	return test_mode4_executor(&test_mode4_ext_lacp);
 }
 
+static int
+test_mode4_lacp_timeout_control_wrapper(void)
+{
+	return test_mode4_executor(&test_mode4_lacp_timeout_control);
+}
+
 static struct unit_test_suite link_bonding_mode4_test_suite  = {
 	.suite_name = "Link Bonding mode 4 Unit Test Suite",
 	.setup = test_setup,
@@ -1665,6 +1729,8 @@ static struct unit_test_suite link_bonding_mode4_test_suite  = {
 				test_mode4_ext_ctrl_wrapper),
 		TEST_CASE_NAMED("test_mode4_ext_lacp",
 				test_mode4_ext_lacp_wrapper),
+		TEST_CASE_NAMED("test_mode4_lacp_timeout_control",
+				test_mode4_lacp_timeout_control_wrapper),
 
 		TEST_CASES_END() /**< NULL terminate unit test array */
 	}
-- 
2.7.4

Re: [PATCH 3/7] net/bonding: change mbuf pool and ring allocation

[Min Hu (Connor)]

在 2021/12/21 23:31, Sanford, Robert 写道:
> Hi Connor,
> 
> On 12/20/21, 9:03 PM, "Min Hu (Connor)" <humin29@huawei.com> wrote:
> 
>> Hi, Sanford,
> 
>>> There is *NO* benefit for the consumer thread (interrupt thread
>>> executing tx_machine()) to have caches on per-slave LACPDU pools.
>>> The interrupt thread is a control thread, i.e., a non-EAL thread.
>>> Its lcore_id is LCORE_ID_ANY, so it has no "default cache" in any
>>> mempool.
>> Well, sorry, I forgot that interrupt thread is non-EAL thread.
> 
> No problem. (I added a temporary rte_log statement in tx_machine
> to make sure lcore_id == LCORE_ID_ANY.)
> 
>>> There is little or no benefit for active data-plane threads to have
>>> caches on per-slave LACPDU pools, because on each pool, the producer
>>> thread puts back, at most, one mbuf per second. There is not much
>>> contention with the consumer (interrupt thread).
>>>
>>> I contend that caches are not necessary for these private LACPDU
>> I agree with you.
> 
> Thanks.
> 
>>> I believe there is a mistake in the ring comments (in 3 places).
>>> It would be better if they replace "free" with "full":
>>> "... to differentiate a *full* ring from an empty ring."
>>>
>> Well, I still can not understand it. I think the ring size is N, it
>> should store N items, why "N - 1" items.?
>> Hope for your description, thanks.
> 
> Here is an excellent article that describes ring buffers, empty vs full, N-1, etc.
> https://embedjournal.com/implementing-circular-buffer-embedded-c/#the-full-vs-empty-problem
> 
Thanks Sanford, I see. It is characteristics of ring queues which is
different with common queue, like buffers.
> 
>>>> To fix the bug, how about just setting the flags "RING_F_EXACT_SZ"
>>>
>>> Yes, this is a good idea. I will look for examples or test code that
>>> use this flag.
>> Yes, if fixed, ILGM.
> 
> I will use RING_F_EXACT_SZ flag in the next version of the patchset. I did not know about that flag.
> 	rte_ring_create(... N_PKTS ... RING_F_EXACT_SZ)
> ... is equivalent to, and looks cleaner than ...
> 	rte_ring_create(... rte_align32pow2(N_PKTS + 1) ... 0)
> 
> I plan to create a separate patchset to update the comments in rte_ring.h,
> re RING_F_EXACT_SZ and "free" vs "full".
> 
> --
> Regards,
> Robert Sanford
> 
> 

Re: [PATCH v2 0/8] net/bonding: fixes and LACP short timeout

[Min Hu (Connor)]

Acked-by: Min Hu (Connor) <humin29@huawei.com>

在 2021/12/22 3:57, Robert Sanford 写道:
> This patchset makes the following changes to net/bonding:
> - Clean up minor errors in spelling, whitespace, C++ wrappers, and
>    comments.
> - Replace directly overwriting of slave port's rte_eth_conf by copying
>    it, but only updating it via rte_eth_dev_configure().
> - Make minor changes to allocation of mbuf pool and rx/tx rings.
> - Add support for enabling LACP short timeout, i.e., link partner can
>    use fast periodic time interval between transmits.
> - Include bond_8023ad and bond_alb in doxygen.
> - Remove self from Timers maintainers.
> - Add API stubs to net/ring PMD.
> - Add LACP short timeout to tests.
> 
> V2 changes:
> - Additional typo and whitespace corrections.
> - Minor changes to LACP private rings creation.
> - Add net/ring API stubs patch.
> - Insert extra "bond_handshake" to LACP short timeout autotest.
> 
> Robert Sanford (8):
>    net/bonding: fix typos and whitespace
>    net/bonding: fix bonded dev configuring slave dev
>    net/bonding: change mbuf pool and ring creation
>    net/bonding: support enabling LACP short timeout
>    net/bonding: add bond_8023ad and bond_alb to doc
>    Remove self from Timers maintainers.
>    net/ring: add promiscuous and allmulticast API stubs
>    net/bonding: add LACP short timeout to tests
> 
>   MAINTAINERS                                   |  1 -
>   app/test-pmd/cmdline.c                        | 81 +++++++++++++++++++++-
>   app/test/test_link_bonding_mode4.c            | 98 ++++++++++++++++++++++-----
>   doc/api/doxy-api-index.md                     |  2 +
>   drivers/net/bonding/eth_bond_8023ad_private.h | 15 ++--
>   drivers/net/bonding/rte_eth_bond_8023ad.c     | 58 ++++++++++------
>   drivers/net/bonding/rte_eth_bond_8023ad.h     | 18 +++--
>   drivers/net/bonding/rte_eth_bond_pmd.c        | 43 ++++++------
>   drivers/net/ring/rte_eth_ring.c               | 28 ++++++++
>   9 files changed, 272 insertions(+), 72 deletions(-)
> 

Re: [PATCH v2 0/8] net/bonding: fixes and LACP short timeout

[Kevin Traynor]

On 21/12/2021 19:57, Robert Sanford wrote:
> This patchset makes the following changes to net/bonding:
> - Clean up minor errors in spelling, whitespace, C++ wrappers, and
>    comments.
> - Replace directly overwriting of slave port's rte_eth_conf by copying
>    it, but only updating it via rte_eth_dev_configure().
> - Make minor changes to allocation of mbuf pool and rx/tx rings.
> - Add support for enabling LACP short timeout, i.e., link partner can
>    use fast periodic time interval between transmits.
> - Include bond_8023ad and bond_alb in doxygen.
> - Remove self from Timers maintainers.
> - Add API stubs to net/ring PMD.
> - Add LACP short timeout to tests.
> 
> V2 changes:
> - Additional typo and whitespace corrections.
> - Minor changes to LACP private rings creation.
> - Add net/ring API stubs patch.
> - Insert extra "bond_handshake" to LACP short timeout autotest.
> 
> Robert Sanford (8):
>    net/bonding: fix typos and whitespace
>    net/bonding: fix bonded dev configuring slave dev
>    net/bonding: change mbuf pool and ring creation
>    net/bonding: support enabling LACP short timeout
>    net/bonding: add bond_8023ad and bond_alb to doc
>    Remove self from Timers maintainers.
>    net/ring: add promiscuous and allmulticast API stubs
>    net/bonding: add LACP short timeout to tests
> 

You might want to consider patches that are fixes for backport to stable 
branches. Patches 1,2,3,5 look to be the ones most suited.

>   MAINTAINERS                                   |  1 -
>   app/test-pmd/cmdline.c                        | 81 +++++++++++++++++++++-
>   app/test/test_link_bonding_mode4.c            | 98 ++++++++++++++++++++++-----
>   doc/api/doxy-api-index.md                     |  2 +
>   drivers/net/bonding/eth_bond_8023ad_private.h | 15 ++--
>   drivers/net/bonding/rte_eth_bond_8023ad.c     | 58 ++++++++++------
>   drivers/net/bonding/rte_eth_bond_8023ad.h     | 18 +++--
>   drivers/net/bonding/rte_eth_bond_pmd.c        | 43 ++++++------
>   drivers/net/ring/rte_eth_ring.c               | 28 ++++++++
>   9 files changed, 272 insertions(+), 72 deletions(-)
> 

Re: [PATCH v2 7/8] net/ring: add promisc and all-MC stubs

[Ferruh Yigit]

On 12/21/2021 7:57 PM, Robert Sanford wrote:
> Add promiscuous_enable, promiscuous_disable, allmulticast_enable,
> and allmulticast_disable API stubs.
> This helps clean up errors in dpdk-test link_bonding_mode4_autotest.
> 
> Signed-off-by: Robert Sanford <rsanford@akamai.com>
> ---
>   drivers/net/ring/rte_eth_ring.c | 28 ++++++++++++++++++++++++++++
>   1 file changed, 28 insertions(+)
> 
> diff --git a/drivers/net/ring/rte_eth_ring.c b/drivers/net/ring/rte_eth_ring.c
> index db10f03..cfb81da 100644
> --- a/drivers/net/ring/rte_eth_ring.c
> +++ b/drivers/net/ring/rte_eth_ring.c
> @@ -226,6 +226,30 @@ eth_mac_addr_add(struct rte_eth_dev *dev __rte_unused,
>   }
>   
>   static int
> +eth_promiscuous_enable(struct rte_eth_dev *dev __rte_unused)
> +{
> +	return 0;
> +}
> +
> +static int
> +eth_promiscuous_disable(struct rte_eth_dev *dev __rte_unused)
> +{
> +	return 0;
> +}
> +
> +static int
> +eth_allmulticast_enable(struct rte_eth_dev *dev __rte_unused)
> +{
> +	return 0;
> +}
> +
> +static int
> +eth_allmulticast_disable(struct rte_eth_dev *dev __rte_unused)
> +{
> +	return 0;
> +}
> +
> +static int
>   eth_link_update(struct rte_eth_dev *dev __rte_unused,
>   		int wait_to_complete __rte_unused) { return 0; }
>   
> @@ -275,6 +299,10 @@ static const struct eth_dev_ops ops = {
>   	.stats_reset = eth_stats_reset,
>   	.mac_addr_remove = eth_mac_addr_remove,
>   	.mac_addr_add = eth_mac_addr_add,
> +	.promiscuous_enable = eth_promiscuous_enable,
> +	.promiscuous_disable = eth_promiscuous_disable,
> +	.allmulticast_enable = eth_allmulticast_enable,
> +	.allmulticast_disable = eth_allmulticast_disable,

not sure about adding dummy dev_ops to the driver for the unit test,
what about updating 'link_bonding_mode4_autotest' accordingly?

Bruce (net/ring maintainer), what do you think about dummy dev_ops?

Re: [PATCH v2 4/8] net/bonding: support enabling LACP short timeout

[Ferruh Yigit]

On 12/21/2021 7:57 PM, Robert Sanford wrote:
> - Add support for enabling LACP short timeout, i.e., link partner can
>    use fast periodic time interval between transmits.
> 
> Signed-off-by: Robert Sanford <rsanford@akamai.com>
> ---
>   drivers/net/bonding/eth_bond_8023ad_private.h |  3 ++-
>   drivers/net/bonding/rte_eth_bond_8023ad.c     | 28 +++++++++++++++++++++++----
>   drivers/net/bonding/rte_eth_bond_8023ad.h     |  3 +++
>   3 files changed, 29 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/net/bonding/eth_bond_8023ad_private.h b/drivers/net/bonding/eth_bond_8023ad_private.h
> index 60db31e..bfde03c 100644
> --- a/drivers/net/bonding/eth_bond_8023ad_private.h
> +++ b/drivers/net/bonding/eth_bond_8023ad_private.h
> @@ -159,7 +159,6 @@ struct mode8023ad_private {
>   	uint64_t rx_marker_timeout;
>   	uint64_t update_timeout_us;
>   	rte_eth_bond_8023ad_ext_slowrx_fn slowrx_cb;
> -	uint8_t external_sm;
>   	struct rte_ether_addr mac_addr;
>   
>   	struct rte_eth_link slave_link;
> @@ -178,6 +177,8 @@ struct mode8023ad_private {
>   		uint16_t tx_qid;
>   	} dedicated_queues;
>   	enum rte_bond_8023ad_agg_selection agg_selection;
> +	uint8_t short_timeout_enabled : 1;
> +	uint8_t short_timeout_updated : 1;
>   };
>   
>   /**
> diff --git a/drivers/net/bonding/rte_eth_bond_8023ad.c b/drivers/net/bonding/rte_eth_bond_8023ad.c
> index 9ed2a46..5c175e7 100644
> --- a/drivers/net/bonding/rte_eth_bond_8023ad.c
> +++ b/drivers/net/bonding/rte_eth_bond_8023ad.c
> @@ -868,10 +868,10 @@ bond_mode_8023ad_periodic_cb(void *arg)
>   	struct rte_eth_link link_info;
>   	struct rte_ether_addr slave_addr;
>   	struct rte_mbuf *lacp_pkt = NULL;
> +	uint8_t short_timeout_updated = internals->mode4.short_timeout_updated;
>   	uint16_t slave_id;
>   	uint16_t i;
>   
> -
>   	/* Update link status on each port */
>   	for (i = 0; i < internals->active_slave_count; i++) {
>   		uint16_t key;
> @@ -916,6 +916,13 @@ bond_mode_8023ad_periodic_cb(void *arg)
>   		slave_id = internals->active_slaves[i];
>   		port = &bond_mode_8023ad_ports[slave_id];
>   
> +		if (short_timeout_updated) {
> +			if (internals->mode4.short_timeout_enabled)
> +				ACTOR_STATE_SET(port, LACP_SHORT_TIMEOUT);
> +			else
> +				ACTOR_STATE_CLR(port, LACP_SHORT_TIMEOUT);
> +		}
> +
>   		if ((port->actor.key &
>   				rte_cpu_to_be_16(BOND_LINK_FULL_DUPLEX_KEY)) == 0) {
>   
> @@ -960,6 +967,9 @@ bond_mode_8023ad_periodic_cb(void *arg)
>   		show_warnings(slave_id);
>   	}
>   
> +	if (short_timeout_updated)
> +		internals->mode4.short_timeout_updated = 0;
> +
>   	rte_eal_alarm_set(internals->mode4.update_timeout_us,
>   			bond_mode_8023ad_periodic_cb, arg);
>   }
> @@ -1054,7 +1064,6 @@ bond_mode_8023ad_activate_slave(struct rte_eth_dev *bond_dev,
>   	/* Given slave must not be in active list. */
>   	RTE_ASSERT(find_slave_by_id(internals->active_slaves,
>   	internals->active_slave_count, slave_id) == internals->active_slave_count);
> -	RTE_SET_USED(internals); /* used only for assert when enabled */
>   
>   	memcpy(&port->actor, &initial, sizeof(struct port_params));
>   	/* Standard requires that port ID must be greater than 0.
> @@ -1065,7 +1074,9 @@ bond_mode_8023ad_activate_slave(struct rte_eth_dev *bond_dev,
>   	memcpy(&port->partner_admin, &initial, sizeof(struct port_params));
>   
>   	/* default states */
> -	port->actor_state = STATE_AGGREGATION | STATE_LACP_ACTIVE | STATE_DEFAULTED;
> +	port->actor_state = STATE_AGGREGATION | STATE_LACP_ACTIVE |
> +		STATE_DEFAULTED | (internals->mode4.short_timeout_enabled ?
> +		STATE_LACP_SHORT_TIMEOUT : 0);
>   	port->partner_state = STATE_LACP_ACTIVE | STATE_AGGREGATION;
>   	port->sm_flags = SM_FLAGS_BEGIN;
>   
> @@ -1209,6 +1220,7 @@ bond_mode_8023ad_conf_get(struct rte_eth_dev *dev,
>   	struct mode8023ad_private *mode4 = &internals->mode4;
>   	uint64_t ms_ticks = rte_get_tsc_hz() / 1000;
>   
> +	memset(conf, 0, sizeof(*conf));
>   	conf->fast_periodic_ms = mode4->fast_periodic_timeout / ms_ticks;
>   	conf->slow_periodic_ms = mode4->slow_periodic_timeout / ms_ticks;
>   	conf->short_timeout_ms = mode4->short_timeout / ms_ticks;
> @@ -1219,6 +1231,7 @@ bond_mode_8023ad_conf_get(struct rte_eth_dev *dev,
>   	conf->rx_marker_period_ms = mode4->rx_marker_timeout / ms_ticks;
>   	conf->slowrx_cb = mode4->slowrx_cb;
>   	conf->agg_selection = mode4->agg_selection;
> +	conf->lacp_timeout_control = mode4->short_timeout_enabled;
>   }
>   
>   static void
> @@ -1234,6 +1247,7 @@ bond_mode_8023ad_conf_get_default(struct rte_eth_bond_8023ad_conf *conf)
>   	conf->update_timeout_ms = BOND_MODE_8023AX_UPDATE_TIMEOUT_MS;
>   	conf->slowrx_cb = NULL;
>   	conf->agg_selection = AGG_STABLE;
> +	conf->lacp_timeout_control = 0;
>   }
>   
>   static void
> @@ -1274,6 +1288,11 @@ bond_mode_8023ad_setup(struct rte_eth_dev *dev,
>   	mode4->slowrx_cb = conf->slowrx_cb;
>   	mode4->agg_selection = AGG_STABLE;
>   
> +	if (mode4->short_timeout_enabled != conf->lacp_timeout_control) {
> +		mode4->short_timeout_enabled = conf->lacp_timeout_control;
> +		mode4->short_timeout_updated = 1;
> +	}
> +
>   	if (dev->data->dev_started)
>   		bond_mode_8023ad_start(dev);
>   }
> @@ -1478,7 +1497,8 @@ bond_8023ad_setup_validate(uint16_t port_id,
>   				conf->aggregate_wait_timeout_ms == 0 ||
>   				conf->tx_period_ms == 0 ||
>   				conf->rx_marker_period_ms == 0 ||
> -				conf->update_timeout_ms == 0) {
> +				conf->update_timeout_ms == 0 ||
> +				conf->lacp_timeout_control > 1) {
>   			RTE_BOND_LOG(ERR, "given mode 4 configuration is invalid");
>   			return -EINVAL;
>   		}
> diff --git a/drivers/net/bonding/rte_eth_bond_8023ad.h b/drivers/net/bonding/rte_eth_bond_8023ad.h
> index 7e9a018..87f6b2f 100644
> --- a/drivers/net/bonding/rte_eth_bond_8023ad.h
> +++ b/drivers/net/bonding/rte_eth_bond_8023ad.h
> @@ -139,6 +139,9 @@ struct rte_eth_bond_8023ad_conf {
>   	uint32_t update_timeout_ms;
>   	rte_eth_bond_8023ad_ext_slowrx_fn slowrx_cb;
>   	enum rte_bond_8023ad_agg_selection agg_selection;
> +	uint8_t lacp_timeout_control;
> +	/**< LACPDU.Actor_State.LACP_Timeout flag: 0=Long 1=Short. */
> +	uint8_t reserved_8s[3];
>   };
>   
>   struct rte_eth_bond_8023ad_slave_info {

The changes gives ABI warning [1], it increases size of struct that is
parameter to the public API.

So old applications will send a smaller struct, but new DPDK library
will check beyond the size of the struct, most probably some unrelated
memory in the stack, this looks an ABI break to me.
@Ray can you please check if I am missing anything?



[1]
   [C] 'function int rte_eth_bond_8023ad_conf_get(uint16_t, rte_eth_bond_8023ad_conf*)' at rte_eth_bond_8023ad.c:1423:1 has some indirect sub-type changes:
     parameter 2 of type 'rte_eth_bond_8023ad_conf*' has sub-type changes:
       in pointed to type 'struct rte_eth_bond_8023ad_conf' at rte_eth_bond_8023ad.h:131:1:
Error: ABI issue reported for 'abidiff --suppr devtools/libabigail.abignore --no-added-syms --headers-dir1 reference/usr/local/include --headers-dir2 install/usr/local/include reference/dump/librte_net_bond.dump install/dump/librte_net_bond.dump'
ABIDIFF_ABI_CHANGE, this change requires a review (abidiff flagged this as a potential issue).
         type size hasn't changed
         2 data member insertions:
           'uint8_t rte_eth_bond_8023ad_conf::lacp_timeout_control', at offset 352 (in bits) at rte_eth_bond_8023ad.h:142:1
           'uint8_t rte_eth_bond_8023ad_conf::reserved_8s[3]', at offset 360 (in bits) at rte_eth_bond_8023ad.h:144:1

Re: [PATCH v2 5/8] net/bonding: add bond_8023ad and bond_alb to doc

[Ferruh Yigit]

On 12/21/2021 7:57 PM, Robert Sanford wrote:
> - Add bond_8023ad and bond_alb to API documentation.
> 
> Signed-off-by: Robert Sanford <rsanford@akamai.com>
> ---
>   doc/api/doxy-api-index.md | 2 ++
>   1 file changed, 2 insertions(+)
> 
> diff --git a/doc/api/doxy-api-index.md b/doc/api/doxy-api-index.md
> index 4245b96..830235c 100644
> --- a/doc/api/doxy-api-index.md
> +++ b/doc/api/doxy-api-index.md
> @@ -39,6 +39,8 @@ The public API headers are grouped by topics:
>   - **device specific**:
>     [softnic]            (@ref rte_eth_softnic.h),
>     [bond]               (@ref rte_eth_bond.h),
> +  [bond_8023ad]        (@ref rte_eth_bond_8023ad.h),

ack

> +  [bond_alb]           (@ref rte_eth_bond_alb.h),

'rte_eth_bond_alb.h' is not public header, it is not installed,
also functions in this header is not exported, so I think it
should not be part of API documentation.

>     [vhost]              (@ref rte_vhost.h),
>     [vdpa]               (@ref rte_vdpa.h),
>     [KNI]                (@ref rte_kni.h),

Re: [PATCH v2 8/8] net/bonding: add LACP short timeout tests

[Ferruh Yigit]

On 12/21/2021 7:57 PM, Robert Sanford wrote:
> - Add "set bonding lacp timeout_ctrl <port_id> on|off" to testpmd.
> - Add "test_mode4_lacp_timeout_control" to dpdk-test.
> - Remove call to rte_eth_dev_mac_addr_remove from add_slave,
>    as it always fails and prints an error.
> 
> Signed-off-by: Robert Sanford<rsanford@akamai.com>
> ---
>   app/test-pmd/cmdline.c             | 77 ++++++++++++++++++++++++++++++++++++++
>   app/test/test_link_bonding_mode4.c | 70 +++++++++++++++++++++++++++++++++-
>   2 files changed, 145 insertions(+), 2 deletions(-)

This patch depends on path 4/8, which cause an ABI error, so can't proceed
with this patch before ABI issue is resolved.

Re: [PATCH v2 7/8] net/ring: add promisc and all-MC stubs

[Bruce Richardson]

On Fri, Feb 04, 2022 at 02:36:47PM +0000, Ferruh Yigit wrote:
> On 12/21/2021 7:57 PM, Robert Sanford wrote:
> > Add promiscuous_enable, promiscuous_disable, allmulticast_enable,
> > and allmulticast_disable API stubs.
> > This helps clean up errors in dpdk-test link_bonding_mode4_autotest.
> > 
> > Signed-off-by: Robert Sanford <rsanford@akamai.com>
> > ---
> >   drivers/net/ring/rte_eth_ring.c | 28 ++++++++++++++++++++++++++++
> >   1 file changed, 28 insertions(+)
> > 
> > diff --git a/drivers/net/ring/rte_eth_ring.c b/drivers/net/ring/rte_eth_ring.c
> > index db10f03..cfb81da 100644
> > --- a/drivers/net/ring/rte_eth_ring.c
> > +++ b/drivers/net/ring/rte_eth_ring.c
> > @@ -226,6 +226,30 @@ eth_mac_addr_add(struct rte_eth_dev *dev __rte_unused,
> >   }
> >   static int
> > +eth_promiscuous_enable(struct rte_eth_dev *dev __rte_unused)
> > +{
> > +	return 0;
> > +}
> > +
> > +static int
> > +eth_promiscuous_disable(struct rte_eth_dev *dev __rte_unused)
> > +{
> > +	return 0;
> > +}
> > +
> > +static int
> > +eth_allmulticast_enable(struct rte_eth_dev *dev __rte_unused)
> > +{
> > +	return 0;
> > +}
> > +
> > +static int
> > +eth_allmulticast_disable(struct rte_eth_dev *dev __rte_unused)
> > +{
> > +	return 0;
> > +}
> > +
> > +static int
> >   eth_link_update(struct rte_eth_dev *dev __rte_unused,
> >   		int wait_to_complete __rte_unused) { return 0; }
> > @@ -275,6 +299,10 @@ static const struct eth_dev_ops ops = {
> >   	.stats_reset = eth_stats_reset,
> >   	.mac_addr_remove = eth_mac_addr_remove,
> >   	.mac_addr_add = eth_mac_addr_add,
> > +	.promiscuous_enable = eth_promiscuous_enable,
> > +	.promiscuous_disable = eth_promiscuous_disable,
> > +	.allmulticast_enable = eth_allmulticast_enable,
> > +	.allmulticast_disable = eth_allmulticast_disable,
> 
> not sure about adding dummy dev_ops to the driver for the unit test,
> what about updating 'link_bonding_mode4_autotest' accordingly?
> 
> Bruce (net/ring maintainer), what do you think about dummy dev_ops?

For something like ring PMD, I don't see an issue with it, since they don't
really have any meaning for a ring PMD, they might as well just return
success rather than having application code have to handle errors from
them.


Acked-by: Bruce Richardson <bruce.richardson@intel.com>

Re: [PATCH v2 1/8] net/bonding: fix typos and whitespace

[Ferruh Yigit]

On 12/21/2021 7:57 PM, Robert Sanford wrote:
> - Clean up minor typos in comments, strings, and private names.
> - Fix whitespace in log messages and function formatting
>    (new line before open brace).
> - Move closing C++ wrapper to the end of rte_eth_bond_8023ad.h.
> 
> Signed-off-by: Robert Sanford <rsanford@akamai.com>
> ---
>   app/test-pmd/cmdline.c                        |  4 ++--
>   app/test/test_link_bonding_mode4.c            | 28 +++++++++++++--------------
>   drivers/net/bonding/eth_bond_8023ad_private.h | 12 ++++++------
>   drivers/net/bonding/rte_eth_bond_8023ad.c     | 22 ++++++++++-----------
>   drivers/net/bonding/rte_eth_bond_8023ad.h     | 15 +++++++-------
>   drivers/net/bonding/rte_eth_bond_pmd.c        | 13 ++++++++-----
>   6 files changed, 49 insertions(+), 45 deletions(-)
> 
> diff --git a/app/test-pmd/cmdline.c b/app/test-pmd/cmdline.c
> index 6e10afe..9fd2c2a 100644
> --- a/app/test-pmd/cmdline.c
> +++ b/app/test-pmd/cmdline.c
> @@ -630,8 +630,8 @@ static void cmd_help_long_parsed(void *parsed_result,
>   			"set bonding mac_addr (port_id) (address)\n"
>   			"	Set the MAC address of a bonded device.\n\n"
>   
> -			"set bonding mode IEEE802.3AD aggregator policy (port_id) (agg_name)"
> -			"	Set Aggregation mode for IEEE802.3AD (mode 4)"
> +			"set bonding mode IEEE802.3AD aggregator policy (port_id) (agg_name)\n"
> +			"	Set Aggregation mode for IEEE802.3AD (mode 4)\n\n"

ack

>   
>   			"set bonding balance_xmit_policy (port_id) (l2|l23|l34)\n"
>   			"	Set the transmit balance policy for bonded device running in balance mode.\n\n"
> diff --git a/app/test/test_link_bonding_mode4.c b/app/test/test_link_bonding_mode4.c
> index 351129d..2be86d5 100644
> --- a/app/test/test_link_bonding_mode4.c
> +++ b/app/test/test_link_bonding_mode4.c
> @@ -58,11 +58,11 @@ static const struct rte_ether_addr slave_mac_default = {
>   	{ 0x00, 0xFF, 0x00, 0xFF, 0x00, 0x00 }
>   };
>   
> -static const struct rte_ether_addr parnter_mac_default = {
> +static const struct rte_ether_addr partner_mac_default = {

ack

<...>

> diff --git a/drivers/net/bonding/eth_bond_8023ad_private.h b/drivers/net/bonding/eth_bond_8023ad_private.h
> index 9b5738a..60db31e 100644
> --- a/drivers/net/bonding/eth_bond_8023ad_private.h
> +++ b/drivers/net/bonding/eth_bond_8023ad_private.h
> @@ -15,12 +15,12 @@
>   #include "rte_eth_bond_8023ad.h"
>   
>   #define BOND_MODE_8023AX_UPDATE_TIMEOUT_MS  100
> -/** Maximum number of packets to one slave queued in TX ring. */
> +/** Maximum number of packets to one slave queued in RX ring. */
>   #define BOND_MODE_8023AX_SLAVE_RX_PKTS        3
>   /** Maximum number of LACP packets from one slave queued in TX ring. */
>   #define BOND_MODE_8023AX_SLAVE_TX_PKTS        1
>   /**
> - * Timeouts deffinitions (5.4.4 in 802.1AX documentation).
> + * Timeouts definitions (6.4.4 in 802.1AX documentation).

There are multiple updates in the document reference, section 5 -> 6,
since the old code is consistent about section 5, can it be because
of document version change?
If so should we document the document version to prevent same thing
happen again?
<...>

>   
> -#ifdef __cplusplus
> -}
> -#endif
> -
>   /**
>    * Configure a slave port to start collecting.
>    *
> @@ -331,4 +327,9 @@ rte_eth_bond_8023ad_agg_selection_get(uint16_t port_id);
>   int
>   rte_eth_bond_8023ad_agg_selection_set(uint16_t port_id,
>   		enum rte_bond_8023ad_agg_selection agg_selection);
> +
> +#ifdef __cplusplus
> +}
> +#endif
> +

This is not typo or whitespace, it seems we misplaced the cpp block, so this
may result issues for cpp applications using this header, I wonder if we should
have separate patch for fixes?

>   #endif /* RTE_ETH_BOND_8023AD_H_ */
> diff --git a/drivers/net/bonding/rte_eth_bond_pmd.c b/drivers/net/bonding/rte_eth_bond_pmd.c
> index 84f4900..f6003b0 100644
> --- a/drivers/net/bonding/rte_eth_bond_pmd.c
> +++ b/drivers/net/bonding/rte_eth_bond_pmd.c
> @@ -158,7 +158,8 @@ const struct rte_flow_attr flow_attr_8023ad = {
>   
>   int
>   bond_ethdev_8023ad_flow_verify(struct rte_eth_dev *bond_dev,
> -		uint16_t slave_port) {
> +		uint16_t slave_port)
> +{

OK to typo fixes, but not sure about the syntax fixes, they corrupt the git
history for little benefit, I think better to fix these when this code is
changed for some other functional reason.
What to you think to drop these changes?

And overall perhaps this change can be split into more patches, that also
helps backporting:
- spelling error, typo, whitespace fixes
- Document reference fix
- ifdef __cplusplus fix

Re: [PATCH v2 0/8] net/bonding: fixes and LACP short timeout

[Ferruh Yigit]

On 12/21/2021 7:57 PM, Robert Sanford wrote:
> This patchset makes the following changes to net/bonding:
> - Clean up minor errors in spelling, whitespace, C++ wrappers, and
>    comments.
> - Replace directly overwriting of slave port's rte_eth_conf by copying
>    it, but only updating it via rte_eth_dev_configure().
> - Make minor changes to allocation of mbuf pool and rx/tx rings.
> - Add support for enabling LACP short timeout, i.e., link partner can
>    use fast periodic time interval between transmits.
> - Include bond_8023ad and bond_alb in doxygen.
> - Remove self from Timers maintainers.
> - Add API stubs to net/ring PMD.
> - Add LACP short timeout to tests.
> 
> V2 changes:
> - Additional typo and whitespace corrections.
> - Minor changes to LACP private rings creation.
> - Add net/ring API stubs patch.
> - Insert extra "bond_handshake" to LACP short timeout autotest.
> 
> Robert Sanford (8):
>    net/bonding: fix typos and whitespace
>    net/bonding: fix bonded dev configuring slave dev
>    net/bonding: change mbuf pool and ring creation
>    net/bonding: support enabling LACP short timeout
>    net/bonding: add bond_8023ad and bond_alb to doc
>    Remove self from Timers maintainers.
>    net/ring: add promiscuous and allmulticast API stubs
>    net/bonding: add LACP short timeout to tests
> 

Hi Robert,

There are some unrelated (and independent) patches in the set,
can you please make new version as multiple sets to help to manage them?

- 4/8 & 8/8 can be a separate set, since they have ABI concern they can
   be managed separately

- 6/8 can be separate

- rest can be various bonding fix set

Thanks,
ferruh

Re: [PATCH v2 7/8] net/ring: add promisc and all-MC stubs

[Ferruh Yigit]

On 2/4/2022 2:49 PM, Bruce Richardson wrote:
> On Fri, Feb 04, 2022 at 02:36:47PM +0000, Ferruh Yigit wrote:
>> On 12/21/2021 7:57 PM, Robert Sanford wrote:
>>> Add promiscuous_enable, promiscuous_disable, allmulticast_enable,
>>> and allmulticast_disable API stubs.
>>> This helps clean up errors in dpdk-test link_bonding_mode4_autotest.
>>>
>>> Signed-off-by: Robert Sanford <rsanford@akamai.com>
>>> ---
>>>    drivers/net/ring/rte_eth_ring.c | 28 ++++++++++++++++++++++++++++
>>>    1 file changed, 28 insertions(+)
>>>
>>> diff --git a/drivers/net/ring/rte_eth_ring.c b/drivers/net/ring/rte_eth_ring.c
>>> index db10f03..cfb81da 100644
>>> --- a/drivers/net/ring/rte_eth_ring.c
>>> +++ b/drivers/net/ring/rte_eth_ring.c
>>> @@ -226,6 +226,30 @@ eth_mac_addr_add(struct rte_eth_dev *dev __rte_unused,
>>>    }
>>>    static int
>>> +eth_promiscuous_enable(struct rte_eth_dev *dev __rte_unused)
>>> +{
>>> +	return 0;
>>> +}
>>> +
>>> +static int
>>> +eth_promiscuous_disable(struct rte_eth_dev *dev __rte_unused)
>>> +{
>>> +	return 0;
>>> +}
>>> +
>>> +static int
>>> +eth_allmulticast_enable(struct rte_eth_dev *dev __rte_unused)
>>> +{
>>> +	return 0;
>>> +}
>>> +
>>> +static int
>>> +eth_allmulticast_disable(struct rte_eth_dev *dev __rte_unused)
>>> +{
>>> +	return 0;
>>> +}
>>> +
>>> +static int
>>>    eth_link_update(struct rte_eth_dev *dev __rte_unused,
>>>    		int wait_to_complete __rte_unused) { return 0; }
>>> @@ -275,6 +299,10 @@ static const struct eth_dev_ops ops = {
>>>    	.stats_reset = eth_stats_reset,
>>>    	.mac_addr_remove = eth_mac_addr_remove,
>>>    	.mac_addr_add = eth_mac_addr_add,
>>> +	.promiscuous_enable = eth_promiscuous_enable,
>>> +	.promiscuous_disable = eth_promiscuous_disable,
>>> +	.allmulticast_enable = eth_allmulticast_enable,
>>> +	.allmulticast_disable = eth_allmulticast_disable,
>>
>> not sure about adding dummy dev_ops to the driver for the unit test,
>> what about updating 'link_bonding_mode4_autotest' accordingly?
>>
>> Bruce (net/ring maintainer), what do you think about dummy dev_ops?
> 
> For something like ring PMD, I don't see an issue with it, since they don't
> really have any meaning for a ring PMD, they might as well just return
> success rather than having application code have to handle errors from
> them.
> 
> 
> Acked-by: Bruce Richardson <bruce.richardson@intel.com>

Applied to dpdk-next-net/main, thanks.

Re: [PATCH v2 6/8] remove self from timers maintainers

[Thomas Monjalon]

21/12/2021 20:57, Robert Sanford:
> Remove self from Timers maintainers.
> 
> Signed-off-by: Robert Sanford <rsanford@akamai.com>

Applied