DPDK patches and discussions
 help / color / mirror / Atom feed
From: David Marchand <david.marchand@redhat.com>
To: dev@dpdk.org
Cc: chas3@att.com, p.oltarzewski@gmail.com, stable@dpdk.org
Subject: [dpdk-dev] [PATCH 1/4] net/bonding: fix oob access in LACP mode when sending many packets
Date: Wed, 10 Apr 2019 14:53:46 +0200	[thread overview]
Message-ID: <1554900829-16180-2-git-send-email-david.marchand@redhat.com> (raw)
Message-ID: <20190410125346.t69OHh1RWIHnxU9sJc8QEZbxBukKNS-dmZAQf9r-a-k@z> (raw)
In-Reply-To: <1554900829-16180-1-git-send-email-david.marchand@redhat.com>

We'd better consolidate the fast queue and the normal tx burst functions
under a common inline wrapper for maintenance.

But looking closer at the bufs_slave_port_idxs[] mapping array in those
tx burst functions, its size is invalid since up to nb_bufs are handled
here.
A previous patch [1] fixed this issue for balance tx burst function
without mentionning it.

802.3ad and balance modes are functionally equivalent on transmit.
The only difference is on the slave id distribution.
Add an additional inline wrapper to consolidate even more and fix this
issue.

[1]: https://git.dpdk.org/dpdk/commit/?id=c5224f623431

Fixes: 09150784a776 ("net/bonding: burst mode hash calculation")
Cc: stable@dpdk.org

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

diff --git a/drivers/net/bonding/rte_eth_bond_pmd.c b/drivers/net/bonding/rte_eth_bond_pmd.c
index f30422a..c193d6d 100644
--- a/drivers/net/bonding/rte_eth_bond_pmd.c
+++ b/drivers/net/bonding/rte_eth_bond_pmd.c
@@ -2,6 +2,7 @@
  * Copyright(c) 2010-2017 Intel Corporation
  */
 #include <stdlib.h>
+#include <stdbool.h>
 #include <netinet/in.h>
 
 #include <rte_mbuf.h>
@@ -295,97 +296,6 @@
 }
 
 static uint16_t
-bond_ethdev_tx_burst_8023ad_fast_queue(void *queue, struct rte_mbuf **bufs,
-		uint16_t nb_bufs)
-{
-	struct bond_tx_queue *bd_tx_q = (struct bond_tx_queue *)queue;
-	struct bond_dev_private *internals = bd_tx_q->dev_private;
-
-	uint16_t slave_port_ids[RTE_MAX_ETHPORTS];
-	uint16_t slave_count;
-
-	uint16_t dist_slave_port_ids[RTE_MAX_ETHPORTS];
-	uint16_t dist_slave_count;
-
-	/* 2-D array to sort mbufs for transmission on each slave into */
-	struct rte_mbuf *slave_bufs[RTE_MAX_ETHPORTS][nb_bufs];
-	/* Number of mbufs for transmission on each slave */
-	uint16_t slave_nb_bufs[RTE_MAX_ETHPORTS] = { 0 };
-	/* Mapping array generated by hash function to map mbufs to slaves */
-	uint16_t bufs_slave_port_idxs[RTE_MAX_ETHPORTS] = { 0 };
-
-	uint16_t slave_tx_count;
-	uint16_t total_tx_count = 0, total_tx_fail_count = 0;
-
-	uint16_t i;
-
-	if (unlikely(nb_bufs == 0))
-		return 0;
-
-	/* Copy slave list to protect against slave up/down changes during tx
-	 * bursting */
-	slave_count = internals->active_slave_count;
-	if (unlikely(slave_count < 1))
-		return 0;
-
-	memcpy(slave_port_ids, internals->active_slaves,
-			sizeof(slave_port_ids[0]) * slave_count);
-
-
-	dist_slave_count = 0;
-	for (i = 0; i < slave_count; i++) {
-		struct port *port = &bond_mode_8023ad_ports[slave_port_ids[i]];
-
-		if (ACTOR_STATE(port, DISTRIBUTING))
-			dist_slave_port_ids[dist_slave_count++] =
-					slave_port_ids[i];
-	}
-
-	if (unlikely(dist_slave_count < 1))
-		return 0;
-
-	/*
-	 * Populate slaves mbuf with the packets which are to be sent on it
-	 * selecting output slave using hash based on xmit policy
-	 */
-	internals->burst_xmit_hash(bufs, nb_bufs, dist_slave_count,
-			bufs_slave_port_idxs);
-
-	for (i = 0; i < nb_bufs; i++) {
-		/* Populate slave mbuf arrays with mbufs for that slave. */
-		uint16_t slave_idx = bufs_slave_port_idxs[i];
-
-		slave_bufs[slave_idx][slave_nb_bufs[slave_idx]++] = bufs[i];
-	}
-
-
-	/* Send packet burst on each slave device */
-	for (i = 0; i < dist_slave_count; i++) {
-		if (slave_nb_bufs[i] == 0)
-			continue;
-
-		slave_tx_count = rte_eth_tx_burst(dist_slave_port_ids[i],
-				bd_tx_q->queue_id, slave_bufs[i],
-				slave_nb_bufs[i]);
-
-		total_tx_count += slave_tx_count;
-
-		/* If tx burst fails move packets to end of bufs */
-		if (unlikely(slave_tx_count < slave_nb_bufs[i])) {
-			int slave_tx_fail_count = slave_nb_bufs[i] -
-					slave_tx_count;
-			total_tx_fail_count += slave_tx_fail_count;
-			memcpy(&bufs[nb_bufs - total_tx_fail_count],
-			       &slave_bufs[i][slave_tx_count],
-			       slave_tx_fail_count * sizeof(bufs[0]));
-		}
-	}
-
-	return total_tx_count;
-}
-
-
-static uint16_t
 bond_ethdev_rx_burst_8023ad(void *queue, struct rte_mbuf **bufs,
 		uint16_t nb_pkts)
 {
@@ -1200,16 +1110,13 @@ struct bwg_slave {
 	return num_tx_total;
 }
 
-static uint16_t
-bond_ethdev_tx_burst_balance(void *queue, struct rte_mbuf **bufs,
-		uint16_t nb_bufs)
+static inline uint16_t
+tx_burst_balance(void *queue, struct rte_mbuf **bufs, uint16_t nb_bufs,
+		 uint16_t *slave_port_ids, uint16_t slave_count)
 {
 	struct bond_tx_queue *bd_tx_q = (struct bond_tx_queue *)queue;
 	struct bond_dev_private *internals = bd_tx_q->dev_private;
 
-	uint16_t slave_port_ids[RTE_MAX_ETHPORTS];
-	uint16_t slave_count;
-
 	/* Array to sort mbufs for transmission on each slave into */
 	struct rte_mbuf *slave_bufs[RTE_MAX_ETHPORTS][nb_bufs];
 	/* Number of mbufs for transmission on each slave */
@@ -1222,18 +1129,6 @@ struct bwg_slave {
 
 	uint16_t i;
 
-	if (unlikely(nb_bufs == 0))
-		return 0;
-
-	/* Copy slave list to protect against slave up/down changes during tx
-	 * bursting */
-	slave_count = internals->active_slave_count;
-	if (unlikely(slave_count < 1))
-		return 0;
-
-	memcpy(slave_port_ids, internals->active_slaves,
-			sizeof(slave_port_ids[0]) * slave_count);
-
 	/*
 	 * Populate slaves mbuf with the packets which are to be sent on it
 	 * selecting output slave using hash based on xmit policy
@@ -1274,7 +1169,7 @@ struct bwg_slave {
 }
 
 static uint16_t
-bond_ethdev_tx_burst_8023ad(void *queue, struct rte_mbuf **bufs,
+bond_ethdev_tx_burst_balance(void *queue, struct rte_mbuf **bufs,
 		uint16_t nb_bufs)
 {
 	struct bond_tx_queue *bd_tx_q = (struct bond_tx_queue *)queue;
@@ -1283,18 +1178,36 @@ struct bwg_slave {
 	uint16_t slave_port_ids[RTE_MAX_ETHPORTS];
 	uint16_t slave_count;
 
+	if (unlikely(nb_bufs == 0))
+		return 0;
+
+	/* Copy slave list to protect against slave up/down changes during tx
+	 * bursting
+	 */
+	slave_count = internals->active_slave_count;
+	if (unlikely(slave_count < 1))
+		return 0;
+
+	memcpy(slave_port_ids, internals->active_slaves,
+			sizeof(slave_port_ids[0]) * slave_count);
+	return tx_burst_balance(queue, bufs, nb_bufs, slave_port_ids,
+				slave_count);
+}
+
+static inline uint16_t
+tx_burst_8023ad(void *queue, struct rte_mbuf **bufs, uint16_t nb_bufs,
+		bool dedicated_txq)
+{
+	struct bond_tx_queue *bd_tx_q = (struct bond_tx_queue *)queue;
+	struct bond_dev_private *internals = bd_tx_q->dev_private;
+
+	uint16_t slave_port_ids[RTE_MAX_ETHPORTS];
+	uint16_t slave_count;
+
 	uint16_t dist_slave_port_ids[RTE_MAX_ETHPORTS];
 	uint16_t dist_slave_count;
 
-	/* 2-D array to sort mbufs for transmission on each slave into */
-	struct rte_mbuf *slave_bufs[RTE_MAX_ETHPORTS][nb_bufs];
-	/* Number of mbufs for transmission on each slave */
-	uint16_t slave_nb_bufs[RTE_MAX_ETHPORTS] = { 0 };
-	/* Mapping array generated by hash function to map mbufs to slaves */
-	uint16_t bufs_slave_port_idxs[RTE_MAX_ETHPORTS] = { 0 };
-
 	uint16_t slave_tx_count;
-	uint16_t total_tx_count = 0, total_tx_fail_count = 0;
 
 	uint16_t i;
 
@@ -1307,6 +1220,9 @@ struct bwg_slave {
 	memcpy(slave_port_ids, internals->active_slaves,
 			sizeof(slave_port_ids[0]) * slave_count);
 
+	if (dedicated_txq)
+		goto skip_tx_ring;
+
 	/* Check for LACP control packets and send if available */
 	for (i = 0; i < slave_count; i++) {
 		struct port *port = &bond_mode_8023ad_ports[slave_port_ids[i]];
@@ -1328,6 +1244,7 @@ struct bwg_slave {
 		}
 	}
 
+skip_tx_ring:
 	if (unlikely(nb_bufs == 0))
 		return 0;
 
@@ -1340,53 +1257,25 @@ struct bwg_slave {
 					slave_port_ids[i];
 	}
 
-	if (likely(dist_slave_count > 0)) {
-
-		/*
-		 * Populate slaves mbuf with the packets which are to be sent
-		 * on it, selecting output slave using hash based on xmit policy
-		 */
-		internals->burst_xmit_hash(bufs, nb_bufs, dist_slave_count,
-				bufs_slave_port_idxs);
-
-		for (i = 0; i < nb_bufs; i++) {
-			/*
-			 * Populate slave mbuf arrays with mbufs for that
-			 * slave
-			 */
-			uint16_t slave_idx = bufs_slave_port_idxs[i];
-
-			slave_bufs[slave_idx][slave_nb_bufs[slave_idx]++] =
-					bufs[i];
-		}
-
-
-		/* Send packet burst on each slave device */
-		for (i = 0; i < dist_slave_count; i++) {
-			if (slave_nb_bufs[i] == 0)
-				continue;
-
-			slave_tx_count = rte_eth_tx_burst(
-					dist_slave_port_ids[i],
-					bd_tx_q->queue_id, slave_bufs[i],
-					slave_nb_bufs[i]);
-
-			total_tx_count += slave_tx_count;
+	if (unlikely(dist_slave_count < 1))
+		return 0;
 
-			/* If tx burst fails move packets to end of bufs */
-			if (unlikely(slave_tx_count < slave_nb_bufs[i])) {
-				int slave_tx_fail_count = slave_nb_bufs[i] -
-						slave_tx_count;
-				total_tx_fail_count += slave_tx_fail_count;
+	return tx_burst_balance(queue, bufs, nb_bufs, dist_slave_port_ids,
+				dist_slave_count);
+}
 
-				memcpy(&bufs[nb_bufs - total_tx_fail_count],
-				       &slave_bufs[i][slave_tx_count],
-				       slave_tx_fail_count * sizeof(bufs[0]));
-			}
-		}
-	}
+static uint16_t
+bond_ethdev_tx_burst_8023ad(void *queue, struct rte_mbuf **bufs,
+		uint16_t nb_bufs)
+{
+	return tx_burst_8023ad(queue, bufs, nb_bufs, false);
+}
 
-	return total_tx_count;
+static uint16_t
+bond_ethdev_tx_burst_8023ad_fast_queue(void *queue, struct rte_mbuf **bufs,
+		uint16_t nb_bufs)
+{
+	return tx_burst_8023ad(queue, bufs, nb_bufs, true);
 }
 
 static uint16_t
-- 
1.8.3.1


  parent reply	other threads:[~2019-04-10 12:54 UTC|newest]

Thread overview: 25+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-04-10 12:53 [dpdk-dev] [PATCH 0/4] lacp rx/tx handlers fixes for bonding pmd David Marchand
2019-04-10 12:53 ` David Marchand
2019-04-10 12:53 ` David Marchand [this message]
2019-04-10 12:53   ` [dpdk-dev] [PATCH 1/4] net/bonding: fix oob access in LACP mode when sending many packets David Marchand
2019-04-10 12:53 ` [dpdk-dev] [PATCH 2/4] net/bonding: fix LACP fast queue Rx handler David Marchand
2019-04-10 12:53   ` David Marchand
2019-04-12 14:01   ` Chas Williams
2019-04-12 14:01     ` Chas Williams
2019-04-18  7:11     ` David Marchand
2019-04-18  7:11       ` David Marchand
2019-04-18 22:50       ` Chas Williams
2019-04-18 22:50         ` Chas Williams
2019-05-16  9:12         ` David Marchand
2019-05-16  9:12           ` David Marchand
2019-07-02 15:01           ` Ferruh Yigit
2019-08-14  1:43             ` Chas Williams
2019-08-19  9:41               ` David Marchand
2019-04-10 12:53 ` [dpdk-dev] [PATCH 3/4] net/bonding: fix unicast packets filtering when not in promisc David Marchand
2019-04-10 12:53   ` David Marchand
2019-04-10 12:53 ` [dpdk-dev] [PATCH 4/4] net/bonding: prefer allmulti to promisc for LACP David Marchand
2019-04-10 12:53   ` David Marchand
2019-06-27  8:08 ` [dpdk-dev] [PATCH 0/4] lacp rx/tx handlers fixes for bonding pmd Ferruh Yigit
2019-06-27 12:07   ` WILLIAMS, CHARLES J
2019-06-27 12:19   ` Chas Williams
2019-08-22 16:48 ` Yigit, Ferruh

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=1554900829-16180-2-git-send-email-david.marchand@redhat.com \
    --to=david.marchand@redhat.com \
    --cc=chas3@att.com \
    --cc=dev@dpdk.org \
    --cc=p.oltarzewski@gmail.com \
    --cc=stable@dpdk.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).