[PATCH 0/5] bonding fixes

[PATCH 0/5] bonding fixes

[David Marchand]

Here is a first series of fixes on the bonding driver I caught while
reviewing its code.

-- 
David Marchand

David Marchand (5):
  doc: fix incorrect bond examples
  net/bonding: fix incorrect bond port id types
  net/bonding: fix incorrect slave id types
  net/bonding: fix incorrect packet count type for lacp
  net/bonding: fix incorrect rxq/txq index types

 .../prog_guide/link_bonding_poll_mode_drv_lib.rst  |  8 ++--
 drivers/net/bonding/rte_eth_bond_8023ad.c          | 10 ++---
 drivers/net/bonding/rte_eth_bond_alb.c             |  4 +-
 drivers/net/bonding/rte_eth_bond_api.c             |  4 +-
 drivers/net/bonding/rte_eth_bond_pmd.c             | 43 ++++++++++++----------
 drivers/net/bonding/rte_eth_bond_private.h         |  8 ++--
 6 files changed, 41 insertions(+), 36 deletions(-)

-- 
1.8.3.1

[PATCH 1/5] doc: fix incorrect bond examples

[David Marchand]

Removed incorrect space character and fixed pci addresses.

Fixes: fc1f2750a3ec ("doc: programmers guide")
Cc: stable@dpdk.org

Signed-off-by: David Marchand <david.marchand@redhat.com>
---
 doc/guides/prog_guide/link_bonding_poll_mode_drv_lib.rst | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/doc/guides/prog_guide/link_bonding_poll_mode_drv_lib.rst b/doc/guides/prog_guide/link_bonding_poll_mode_drv_lib.rst
index 56abee5..2459fd2 100644
--- a/doc/guides/prog_guide/link_bonding_poll_mode_drv_lib.rst
+++ b/doc/guides/prog_guide/link_bonding_poll_mode_drv_lib.rst
@@ -477,22 +477,22 @@ Create a bonded device in round robin mode with two slaves specified by their PC
 
 .. code-block:: console
 
-    $RTE_TARGET/app/testpmd -l 0-3 -n 4 --vdev 'net_bonding0,mode=0, slave=0000:00a:00.01,slave=0000:004:00.00' -- --port-topology=chained
+    $RTE_TARGET/app/testpmd -l 0-3 -n 4 --vdev 'net_bonding0,mode=0,slave=0000:0a:00.01,slave=0000:04:00.00' -- --port-topology=chained
 
 Create a bonded device in round robin mode with two slaves specified by their PCI address and an overriding MAC address:
 
 .. code-block:: console
 
-    $RTE_TARGET/app/testpmd -l 0-3 -n 4 --vdev 'net_bonding0,mode=0, slave=0000:00a:00.01,slave=0000:004:00.00,mac=00:1e:67:1d:fd:1d' -- --port-topology=chained
+    $RTE_TARGET/app/testpmd -l 0-3 -n 4 --vdev 'net_bonding0,mode=0,slave=0000:0a:00.01,slave=0000:04:00.00,mac=00:1e:67:1d:fd:1d' -- --port-topology=chained
 
 Create a bonded device in active backup mode with two slaves specified, and a primary slave specified by their PCI addresses:
 
 .. code-block:: console
 
-    $RTE_TARGET/app/testpmd -l 0-3 -n 4 --vdev 'net_bonding0,mode=1, slave=0000:00a:00.01,slave=0000:004:00.00,primary=0000:00a:00.01' -- --port-topology=chained
+    $RTE_TARGET/app/testpmd -l 0-3 -n 4 --vdev 'net_bonding0,mode=1,slave=0000:0a:00.01,slave=0000:04:00.00,primary=0000:0a:00.01' -- --port-topology=chained
 
 Create a bonded device in balance mode with two slaves specified by their PCI addresses, and a transmission policy of layer 3 + 4 forwarding:
 
 .. code-block:: console
 
-    $RTE_TARGET/app/testpmd -l 0-3 -n 4 --vdev 'net_bonding0,mode=2, slave=0000:00a:00.01,slave=0000:004:00.00,xmit_policy=l34' -- --port-topology=chained
+    $RTE_TARGET/app/testpmd -l 0-3 -n 4 --vdev 'net_bonding0,mode=2,slave=0000:0a:00.01,slave=0000:04:00.00,xmit_policy=l34' -- --port-topology=chained
-- 
1.8.3.1

[PATCH 2/5] net/bonding: fix incorrect bond port id types

[David Marchand]

Following the port id conversion to 16bits, two references to bonding
port id have been missed.

Fixes: f8244c6399d9 ("ethdev: increase port id range")
Cc: stable@dpdk.org

Signed-off-by: David Marchand <david.marchand@redhat.com>
---
 drivers/net/bonding/rte_eth_bond_pmd.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/net/bonding/rte_eth_bond_pmd.c b/drivers/net/bonding/rte_eth_bond_pmd.c
index b0d191d..df0a3b4 100644
--- a/drivers/net/bonding/rte_eth_bond_pmd.c
+++ b/drivers/net/bonding/rte_eth_bond_pmd.c
@@ -2196,7 +2196,7 @@ struct bwg_slave {
 bond_ethdev_close(struct rte_eth_dev *dev)
 {
 	struct bond_dev_private *internals = dev->data->dev_private;
-	uint8_t bond_port_id = internals->port_id;
+	uint16_t bond_port_id = internals->port_id;
 	int skipped = 0;
 	struct rte_flow_error ferror;
 
@@ -2663,7 +2663,7 @@ struct bwg_slave {
 	if (type != RTE_ETH_EVENT_INTR_LSC || param == NULL)
 		return rc;
 
-	bonded_eth_dev = &rte_eth_devices[*(uint8_t *)param];
+	bonded_eth_dev = &rte_eth_devices[*(uint16_t *)param];
 
 	if (check_for_bonded_ethdev(bonded_eth_dev))
 		return rc;
-- 
1.8.3.1

[PATCH 3/5] net/bonding: fix incorrect slave id types

[David Marchand]

Caught by code review, with port id conversion to 16bits, the slave id
have been extended to 16bits as well (both slave index and count).

Fixes: f8244c6399d9 ("ethdev: increase port id range")
Cc: stable@dpdk.org

Signed-off-by: David Marchand <david.marchand@redhat.com>
---
 drivers/net/bonding/rte_eth_bond_8023ad.c  | 10 ++++-----
 drivers/net/bonding/rte_eth_bond_alb.c     |  4 ++--
 drivers/net/bonding/rte_eth_bond_api.c     |  4 ++--
 drivers/net/bonding/rte_eth_bond_pmd.c     | 36 +++++++++++++++++-------------
 drivers/net/bonding/rte_eth_bond_private.h |  8 +++----
 5 files changed, 33 insertions(+), 29 deletions(-)

diff --git a/drivers/net/bonding/rte_eth_bond_8023ad.c b/drivers/net/bonding/rte_eth_bond_8023ad.c
index dd847c6..ba44660 100644
--- a/drivers/net/bonding/rte_eth_bond_8023ad.c
+++ b/drivers/net/bonding/rte_eth_bond_8023ad.c
@@ -664,7 +664,7 @@
  * @param port_pos			Port to assign.
  */
 static void
-selection_logic(struct bond_dev_private *internals, uint8_t slave_id)
+selection_logic(struct bond_dev_private *internals, uint16_t slave_id)
 {
 	struct port *agg, *port;
 	uint16_t slaves_count, new_agg_id, i, j = 0;
@@ -781,7 +781,7 @@
 }
 
 static void
-rx_machine_update(struct bond_dev_private *internals, uint8_t slave_id,
+rx_machine_update(struct bond_dev_private *internals, uint16_t slave_id,
 		struct rte_mbuf *lacp_pkt) {
 	struct lacpdu_header *lacp;
 
@@ -805,8 +805,8 @@
 	struct rte_eth_link link_info;
 	struct ether_addr slave_addr;
 	struct rte_mbuf *lacp_pkt = NULL;
-
-	uint8_t i, slave_id;
+	uint16_t slave_id;
+	uint16_t i;
 
 
 	/* Update link status on each port */
@@ -1149,7 +1149,7 @@
 bond_mode_8023ad_enable(struct rte_eth_dev *bond_dev)
 {
 	struct bond_dev_private *internals = bond_dev->data->dev_private;
-	uint8_t i;
+	uint16_t i;
 
 	for (i = 0; i < internals->active_slave_count; i++)
 		bond_mode_8023ad_activate_slave(bond_dev,
diff --git a/drivers/net/bonding/rte_eth_bond_alb.c b/drivers/net/bonding/rte_eth_bond_alb.c
index c3891c7..d3e16d4 100644
--- a/drivers/net/bonding/rte_eth_bond_alb.c
+++ b/drivers/net/bonding/rte_eth_bond_alb.c
@@ -18,10 +18,10 @@
 	return hash;
 }
 
-static uint8_t
+static uint16_t
 calculate_slave(struct bond_dev_private *internals)
 {
-	uint8_t idx;
+	uint16_t idx;
 
 	idx = (internals->mode6.last_slave + 1) % internals->active_slave_count;
 	internals->mode6.last_slave = idx;
diff --git a/drivers/net/bonding/rte_eth_bond_api.c b/drivers/net/bonding/rte_eth_bond_api.c
index b55752e..a23988d 100644
--- a/drivers/net/bonding/rte_eth_bond_api.c
+++ b/drivers/net/bonding/rte_eth_bond_api.c
@@ -76,7 +76,7 @@
 activate_slave(struct rte_eth_dev *eth_dev, uint16_t port_id)
 {
 	struct bond_dev_private *internals = eth_dev->data->dev_private;
-	uint8_t active_count = internals->active_slave_count;
+	uint16_t active_count = internals->active_slave_count;
 
 	if (internals->mode == BONDING_MODE_8023AD)
 		bond_mode_8023ad_activate_slave(eth_dev, port_id);
@@ -796,7 +796,7 @@
 			uint16_t len)
 {
 	struct bond_dev_private *internals;
-	uint8_t i;
+	uint16_t i;
 
 	if (valid_bonded_port_id(bonded_port_id) != 0)
 		return -1;
diff --git a/drivers/net/bonding/rte_eth_bond_pmd.c b/drivers/net/bonding/rte_eth_bond_pmd.c
index df0a3b4..bbe2568 100644
--- a/drivers/net/bonding/rte_eth_bond_pmd.c
+++ b/drivers/net/bonding/rte_eth_bond_pmd.c
@@ -353,7 +353,7 @@
 
 	for (i = 0; i < nb_bufs; i++) {
 		/* Populate slave mbuf arrays with mbufs for that slave. */
-		uint8_t slave_idx = bufs_slave_port_idxs[i];
+		uint16_t slave_idx = bufs_slave_port_idxs[i];
 
 		slave_bufs[slave_idx][slave_nb_bufs[slave_idx]++] = bufs[i];
 	}
@@ -404,8 +404,9 @@
 
 	uint8_t collecting;  /* current slave collecting status */
 	const uint8_t promisc = internals->promiscuous_en;
-	uint8_t i, j, k;
 	uint8_t subtype;
+	uint8_t j, k;
+	uint16_t i;
 
 	/* Copy slave list to protect against slave up/down changes during tx
 	 * bursting */
@@ -774,7 +775,7 @@ struct client_stats_t {
 
 void
 burst_xmit_l2_hash(struct rte_mbuf **buf, uint16_t nb_pkts,
-		uint8_t slave_count, uint16_t *slaves)
+		uint16_t slave_count, uint16_t *slaves)
 {
 	struct ether_hdr *eth_hdr;
 	uint32_t hash;
@@ -791,7 +792,7 @@ struct client_stats_t {
 
 void
 burst_xmit_l23_hash(struct rte_mbuf **buf, uint16_t nb_pkts,
-		uint8_t slave_count, uint16_t *slaves)
+		uint16_t slave_count, uint16_t *slaves)
 {
 	uint16_t i;
 	struct ether_hdr *eth_hdr;
@@ -829,7 +830,7 @@ struct client_stats_t {
 
 void
 burst_xmit_l34_hash(struct rte_mbuf **buf, uint16_t nb_pkts,
-		uint8_t slave_count, uint16_t *slaves)
+		uint16_t slave_count, uint16_t *slaves)
 {
 	struct ether_hdr *eth_hdr;
 	uint16_t proto;
@@ -899,7 +900,7 @@ struct client_stats_t {
 struct bwg_slave {
 	uint64_t bwg_left_int;
 	uint64_t bwg_left_remainder;
-	uint8_t slave;
+	uint16_t slave;
 };
 
 void
@@ -952,11 +953,12 @@ struct bwg_slave {
 	struct bond_dev_private *internals = arg;
 	struct rte_eth_stats slave_stats;
 	struct bwg_slave bwg_array[RTE_MAX_ETHPORTS];
-	uint8_t slave_count;
+	uint16_t slave_count;
 	uint64_t tx_bytes;
 
 	uint8_t update_stats = 0;
-	uint8_t i, slave_id;
+	uint16_t slave_id;
+	uint16_t i;
 
 	internals->slave_update_idx++;
 
@@ -1243,7 +1245,7 @@ struct bwg_slave {
 
 	for (i = 0; i < nb_bufs; i++) {
 		/* Populate slave mbuf arrays with mbufs for that slave. */
-		uint8_t slave_idx = bufs_slave_port_idxs[i];
+		uint16_t slave_idx = bufs_slave_port_idxs[i];
 
 		slave_bufs[slave_idx][slave_nb_bufs[slave_idx]++] = bufs[i];
 	}
@@ -1354,7 +1356,7 @@ struct bwg_slave {
 			 * Populate slave mbuf arrays with mbufs for that
 			 * slave
 			 */
-			uint8_t slave_idx = bufs_slave_port_idxs[i];
+			uint16_t slave_idx = bufs_slave_port_idxs[i];
 
 			slave_bufs[slave_idx][slave_nb_bufs[slave_idx]++] =
 					bufs[i];
@@ -1396,8 +1398,9 @@ struct bwg_slave {
 	struct bond_dev_private *internals;
 	struct bond_tx_queue *bd_tx_q;
 
-	uint8_t tx_failed_flag = 0, num_of_slaves;
 	uint16_t slaves[RTE_MAX_ETHPORTS];
+	uint8_t tx_failed_flag = 0;
+	uint16_t num_of_slaves;
 
 	uint16_t max_nb_of_tx_pkts = 0;
 
@@ -1948,7 +1951,7 @@ struct bwg_slave {
 slave_remove(struct bond_dev_private *internals,
 		struct rte_eth_dev *slave_eth_dev)
 {
-	uint8_t i;
+	uint16_t i;
 
 	for (i = 0; i < internals->slave_count; i++)
 		if (internals->slaves[i].port_id ==
@@ -2147,7 +2150,7 @@ struct bwg_slave {
 bond_ethdev_stop(struct rte_eth_dev *eth_dev)
 {
 	struct bond_dev_private *internals = eth_dev->data->dev_private;
-	uint8_t i;
+	uint16_t i;
 
 	if (internals->mode == BONDING_MODE_8023AD) {
 		struct port *port;
@@ -2243,7 +2246,7 @@ struct bwg_slave {
 	 */
 	if (internals->slave_count > 0) {
 		struct rte_eth_dev_info slave_info;
-		uint8_t idx;
+		uint16_t idx;
 
 		for (idx = 0; idx < internals->slave_count; idx++) {
 			rte_eth_dev_info_get(internals->slaves[idx].port_id,
@@ -2656,9 +2659,10 @@ struct bwg_slave {
 	struct rte_eth_link link;
 	int rc = -1;
 
-	int i, valid_slave = 0;
-	uint8_t active_pos;
 	uint8_t lsc_flag = 0;
+	int valid_slave = 0;
+	uint16_t active_pos;
+	uint16_t i;
 
 	if (type != RTE_ETH_EVENT_INTR_LSC || param == NULL)
 		return rc;
diff --git a/drivers/net/bonding/rte_eth_bond_private.h b/drivers/net/bonding/rte_eth_bond_private.h
index 032ffed..8afef39 100644
--- a/drivers/net/bonding/rte_eth_bond_private.h
+++ b/drivers/net/bonding/rte_eth_bond_private.h
@@ -100,7 +100,7 @@ struct rte_flow {
 };
 
 typedef void (*burst_xmit_hash_t)(struct rte_mbuf **buf, uint16_t nb_pkts,
-		uint8_t slave_count, uint16_t *slaves);
+		uint16_t slave_count, uint16_t *slaves);
 
 /** Link Bonding PMD device private configuration Structure */
 struct bond_dev_private {
@@ -256,15 +256,15 @@ struct bond_dev_private {
 
 void
 burst_xmit_l2_hash(struct rte_mbuf **buf, uint16_t nb_pkts,
-		uint8_t slave_count, uint16_t *slaves);
+		uint16_t slave_count, uint16_t *slaves);
 
 void
 burst_xmit_l23_hash(struct rte_mbuf **buf, uint16_t nb_pkts,
-		uint8_t slave_count, uint16_t *slaves);
+		uint16_t slave_count, uint16_t *slaves);
 
 void
 burst_xmit_l34_hash(struct rte_mbuf **buf, uint16_t nb_pkts,
-		uint8_t slave_count, uint16_t *slaves);
+		uint16_t slave_count, uint16_t *slaves);
 
 
 void
-- 
1.8.3.1

[PATCH 4/5] net/bonding: fix incorrect packet count type for lacp

[David Marchand]

Caught by code review, those variables are supposed to be on 16bits to
avoid endless loops in the (unlikely?) case where the application asks
for receiving more than 256 packets and the accumulated num_rx_total
count reaches 256:

uint16_t num_rx_total = 0;
uint8_t j, k;

j = num_rx_total;
num_rx_total += rte_eth_rx_burst();

for (k = j; k < 2 && k < num_rx_total; k++)
	rte_prefetch0(rte_pktmbuf_mtod(bufs[k], void *));

while (j < num_rx_total) {
	j++;
}

Fixes: 46fb43683679 ("bond: add mode 4")
Cc: stable@dpdk.org

Signed-off-by: David Marchand <david.marchand@redhat.com>
---
 drivers/net/bonding/rte_eth_bond_pmd.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/net/bonding/rte_eth_bond_pmd.c b/drivers/net/bonding/rte_eth_bond_pmd.c
index bbe2568..ae66a70 100644
--- a/drivers/net/bonding/rte_eth_bond_pmd.c
+++ b/drivers/net/bonding/rte_eth_bond_pmd.c
@@ -405,8 +405,9 @@
 	uint8_t collecting;  /* current slave collecting status */
 	const uint8_t promisc = internals->promiscuous_en;
 	uint8_t subtype;
-	uint8_t j, k;
 	uint16_t i;
+	uint16_t j;
+	uint16_t k;
 
 	/* Copy slave list to protect against slave up/down changes during tx
 	 * bursting */
-- 
1.8.3.1

[PATCH 5/5] net/bonding: fix incorrect rxq/txq index types

[David Marchand]

Caught by code review, rxq and txq indexes are on 16bits.

Fixes: d1d1e664c6c4 ("bonding: free queue memory when closing")
Cc: stable@dpdk.org

Signed-off-by: David Marchand <david.marchand@redhat.com>
---
 drivers/net/bonding/rte_eth_bond_pmd.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/bonding/rte_eth_bond_pmd.c b/drivers/net/bonding/rte_eth_bond_pmd.c
index ae66a70..58b6e43 100644
--- a/drivers/net/bonding/rte_eth_bond_pmd.c
+++ b/drivers/net/bonding/rte_eth_bond_pmd.c
@@ -2128,7 +2128,7 @@ struct bwg_slave {
 static void
 bond_ethdev_free_queues(struct rte_eth_dev *dev)
 {
-	uint8_t i;
+	uint16_t i;
 
 	if (dev->data->rx_queues != NULL) {
 		for (i = 0; i < dev->data->nb_rx_queues; i++) {
-- 
1.8.3.1

Re: [PATCH 0/5] bonding fixes

[Ferruh Yigit]

On 3/20/2019 11:47 AM, David Marchand wrote:
> Here is a first series of fixes on the bonding driver I caught while
> reviewing its code.
> 

For series,
Reviewed-by: Ferruh Yigit <ferruh.yigit@intel.com>

Series applied to dpdk-next-net/master, thanks.

Re: [PATCH 0/5] bonding fixes

[David Marchand]

On Thu, Mar 21, 2019 at 9:12 PM Ferruh Yigit <ferruh.yigit@intel.com> wrote:

> On 3/20/2019 11:47 AM, David Marchand wrote:
> > Here is a first series of fixes on the bonding driver I caught while
> > reviewing its code.
> >
>
> For series,
> Reviewed-by: Ferruh Yigit <ferruh.yigit@intel.com>
>
> Series applied to dpdk-next-net/master, thanks.
>

Thanks Ferruh.
2 follow up patches from Zhaohui incoming (I was not quick enough).


-- 
David Marchand