[dpdk-dev] [PATCH v2 0/5] bonding: setup all queues of slave devices

[dpdk-dev] [PATCH v2 0/5] bonding: setup all queues of slave devices

[Jan Blunck]

Prior to 16.11 some drivers (e.g. virtio) still had problems if their
queues where setup repeatedly. The bonding driver was working around the
problem by reusing already setup queues. This series of patches changes the
way how queue setup is done to give control to the driver to properly release
already initialized queues before they are setup again. Therefore the driver
call sequence is as if the number of queues is temporarily reduced before the
queues are setup again.

Ilya Maximets (1):
  Revert "bonding: use existing enslaved device queues"

Jan Blunck (4):
  ethdev: Call rx/tx_queue_release before rx/tx_queue_setup
  ethdev: Free rx/tx_queues after releasing all queues
  ethdev: Add DPDK internal _rte_eth_dev_reset()
  net/bonding: Force reconfiguration of removed slave interfaces

 drivers/net/bonding/rte_eth_bond_pmd.c | 13 +++++------
 lib/librte_ether/rte_ethdev.c          | 40 ++++++++++++++++++++++++++++++++++
 lib/librte_ether/rte_ethdev.h          | 13 +++++++++++
 lib/librte_ether/rte_ether_version.map |  6 +++++
 4 files changed, 64 insertions(+), 8 deletions(-)

-- 
2.7.4

[dpdk-dev] [PATCH v2 1/5] ethdev: Call rx/tx_queue_release before rx/tx_queue_setup

[Jan Blunck]

If a queue has been setup before lets release it before we setup.
Otherwise we might leak resources.

Signed-off-by: Jan Blunck <jblunck@infradead.org>
---
 lib/librte_ether/rte_ethdev.c | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

diff --git a/lib/librte_ether/rte_ethdev.c b/lib/librte_ether/rte_ethdev.c
index fde8112..8c4b6cd 100644
--- a/lib/librte_ether/rte_ethdev.c
+++ b/lib/librte_ether/rte_ethdev.c
@@ -1010,6 +1010,7 @@ rte_eth_rx_queue_setup(uint8_t port_id, uint16_t rx_queue_id,
 	uint32_t mbp_buf_size;
 	struct rte_eth_dev *dev;
 	struct rte_eth_dev_info dev_info;
+	void **rxq;
 
 	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -EINVAL);
 
@@ -1068,6 +1069,14 @@ rte_eth_rx_queue_setup(uint8_t port_id, uint16_t rx_queue_id,
 		return -EINVAL;
 	}
 
+	rxq = dev->data->rx_queues;
+	if (rxq[rx_queue_id]) {
+		RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->rx_queue_release,
+					-ENOTSUP);
+		(*dev->dev_ops->rx_queue_release)(rxq[rx_queue_id]);
+		rxq[rx_queue_id] = NULL;
+	}
+
 	if (rx_conf == NULL)
 		rx_conf = &dev_info.default_rxconf;
 
@@ -1089,6 +1098,7 @@ rte_eth_tx_queue_setup(uint8_t port_id, uint16_t tx_queue_id,
 {
 	struct rte_eth_dev *dev;
 	struct rte_eth_dev_info dev_info;
+	void **txq;
 
 	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -EINVAL);
 
@@ -1121,6 +1131,14 @@ rte_eth_tx_queue_setup(uint8_t port_id, uint16_t tx_queue_id,
 		return -EINVAL;
 	}
 
+	txq = dev->data->tx_queues;
+	if (txq[tx_queue_id]) {
+		RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->tx_queue_release,
+					-ENOTSUP);
+		(*dev->dev_ops->tx_queue_release)(txq[tx_queue_id]);
+		txq[tx_queue_id] = NULL;
+	}
+
 	if (tx_conf == NULL)
 		tx_conf = &dev_info.default_txconf;
 
-- 
2.7.4

[dpdk-dev] [PATCH v2 2/5] ethdev: Free rx/tx_queues after releasing all queues

[Jan Blunck]

If all queues are released lets also free up the dev->data->rx/tx_queues
to be able to properly reinitialize.

Signed-off-by: Jan Blunck <jblunck@infradead.org>
---
 lib/librte_ether/rte_ethdev.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/lib/librte_ether/rte_ethdev.c b/lib/librte_ether/rte_ethdev.c
index 8c4b6cd..a3986ad 100644
--- a/lib/librte_ether/rte_ethdev.c
+++ b/lib/librte_ether/rte_ethdev.c
@@ -531,6 +531,9 @@ rte_eth_dev_rx_queue_config(struct rte_eth_dev *dev, uint16_t nb_queues)
 
 		for (i = nb_queues; i < old_nb_queues; i++)
 			(*dev->dev_ops->rx_queue_release)(rxq[i]);
+
+		rte_free(dev->data->rx_queues);
+		dev->data->rx_queues = NULL;
 	}
 	dev->data->nb_rx_queues = nb_queues;
 	return 0;
@@ -682,6 +685,9 @@ rte_eth_dev_tx_queue_config(struct rte_eth_dev *dev, uint16_t nb_queues)
 
 		for (i = nb_queues; i < old_nb_queues; i++)
 			(*dev->dev_ops->tx_queue_release)(txq[i]);
+
+		rte_free(dev->data->tx_queues);
+		dev->data->tx_queues = NULL;
 	}
 	dev->data->nb_tx_queues = nb_queues;
 	return 0;
-- 
2.7.4

[dpdk-dev] [PATCH v2 3/5] ethdev: Add DPDK internal _rte_eth_dev_reset()

[Jan Blunck]

This is a helper for DPDK internal users to force a reconfiguration of a
device.

Signed-off-by: Jan Blunck <jblunck@infradead.org>
---
 lib/librte_ether/rte_ethdev.c          | 16 ++++++++++++++++
 lib/librte_ether/rte_ethdev.h          | 13 +++++++++++++
 lib/librte_ether/rte_ether_version.map |  6 ++++++
 3 files changed, 35 insertions(+)

diff --git a/lib/librte_ether/rte_ethdev.c b/lib/librte_ether/rte_ethdev.c
index a3986ad..9e69ee5 100644
--- a/lib/librte_ether/rte_ethdev.c
+++ b/lib/librte_ether/rte_ethdev.c
@@ -858,6 +858,22 @@ rte_eth_dev_configure(uint8_t port_id, uint16_t nb_rx_q, uint16_t nb_tx_q,
 	return 0;
 }
 
+void
+_rte_eth_dev_reset(struct rte_eth_dev *dev)
+{
+	if (dev->data->dev_started) {
+		RTE_PMD_DEBUG_TRACE(
+			"port %d must be stopped to allow reset\n",
+			dev->data->port_id);
+		return;
+	}
+
+	rte_eth_dev_rx_queue_config(dev, 0);
+	rte_eth_dev_tx_queue_config(dev, 0);
+
+	memset(&dev->data->dev_conf, 0, sizeof(dev->data->dev_conf));
+}
+
 static void
 rte_eth_dev_config_restore(uint8_t port_id)
 {
diff --git a/lib/librte_ether/rte_ethdev.h b/lib/librte_ether/rte_ethdev.h
index 9678179..e0740db 100644
--- a/lib/librte_ether/rte_ethdev.h
+++ b/lib/librte_ether/rte_ethdev.h
@@ -1914,6 +1914,19 @@ int rte_eth_dev_configure(uint8_t port_id, uint16_t nb_rx_queue,
 		uint16_t nb_tx_queue, const struct rte_eth_conf *eth_conf);
 
 /**
+ * @internal Release a devices rx/tx queues and clear its configuration to
+ * force the user application to reconfigure it. It is for DPDK internal user
+ * only.
+ *
+ * @param dev
+ *  Pointer to struct rte_eth_dev.
+ *
+ * @return
+ *  void
+ */
+void _rte_eth_dev_reset(struct rte_eth_dev *dev);
+
+/**
  * Allocate and set up a receive queue for an Ethernet device.
  *
  * The function allocates a contiguous block of memory for *nb_rx_desc*
diff --git a/lib/librte_ether/rte_ether_version.map b/lib/librte_ether/rte_ether_version.map
index 72be66d..0c31c5d 100644
--- a/lib/librte_ether/rte_ether_version.map
+++ b/lib/librte_ether/rte_ether_version.map
@@ -147,3 +147,9 @@ DPDK_16.11 {
 	rte_eth_dev_pci_remove;
 
 } DPDK_16.07;
+
+DPDK_17.02 {
+	global:
+
+	_rte_eth_dev_reset;
+} DPDK_16.11;
-- 
2.7.4

[dpdk-dev] [PATCH v2 4/5] net/bonding: Force reconfiguration of removed slave interfaces

[Jan Blunck]

After a slave interface is removed from a bond group it still has the
configuration of the bond interface. Lets enforce that the slave interface
is reconfigured after removal by resetting it.

Signed-off-by: Jan Blunck <jblunck@infradead.org>
---
 drivers/net/bonding/rte_eth_bond_pmd.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/net/bonding/rte_eth_bond_pmd.c b/drivers/net/bonding/rte_eth_bond_pmd.c
index a80b6fa..e61afc9 100644
--- a/drivers/net/bonding/rte_eth_bond_pmd.c
+++ b/drivers/net/bonding/rte_eth_bond_pmd.c
@@ -1454,6 +1454,9 @@ slave_remove(struct bond_dev_private *internals,
 				(internals->slave_count - i - 1));
 
 	internals->slave_count--;
+
+	/* force reconfiguration of slave interfaces */
+	_rte_eth_dev_reset(slave_eth_dev);
 }
 
 static void
-- 
2.7.4

[dpdk-dev] [PATCH v2 5/5] Revert "bonding: use existing enslaved device queues"

[Jan Blunck]

From: Ilya Maximets <i.maximets@samsung.com>

This reverts commit 5b7bb2bda5519b7800f814df64d4e015282140e5.

It is necessary to reconfigure all queues every time because configuration
can be changed.

For example, if we're reconfiguring bonding device with new memory pool,
already configured queues will still use the old one. And if the old
mempool be freed, application likely will panic in attempt to use
freed mempool.

This happens when we use the bonding device with OVS 2.6 while MTU
reconfiguration:

PANIC in rte_mempool_get_ops():
assert "(ops_index >= 0) && (ops_index < RTE_MEMPOOL_MAX_OPS_IDX)" failed

Cc: <stable@dpdk.org>
Signed-off-by: Ilya Maximets <i.maximets@samsung.com>
Acked-by: Declan Doherty <declan.doherty@intel.com>
Acked-by: Declan Doherty <declan.doherty@intel.com>
Acked-by: Jan Blunck <jblunck@infradead.org>
---
 drivers/net/bonding/rte_eth_bond_pmd.c | 10 ++--------
 1 file changed, 2 insertions(+), 8 deletions(-)

diff --git a/drivers/net/bonding/rte_eth_bond_pmd.c b/drivers/net/bonding/rte_eth_bond_pmd.c
index e61afc9..b604642 100644
--- a/drivers/net/bonding/rte_eth_bond_pmd.c
+++ b/drivers/net/bonding/rte_eth_bond_pmd.c
@@ -1317,8 +1317,6 @@ slave_configure(struct rte_eth_dev *bonded_eth_dev,
 	struct bond_rx_queue *bd_rx_q;
 	struct bond_tx_queue *bd_tx_q;
 
-	uint16_t old_nb_tx_queues = slave_eth_dev->data->nb_tx_queues;
-	uint16_t old_nb_rx_queues = slave_eth_dev->data->nb_rx_queues;
 	int errval;
 	uint16_t q_id;
 
@@ -1362,9 +1360,7 @@ slave_configure(struct rte_eth_dev *bonded_eth_dev,
 	}
 
 	/* Setup Rx Queues */
-	/* Use existing queues, if any */
-	for (q_id = old_nb_rx_queues;
-	     q_id < bonded_eth_dev->data->nb_rx_queues; q_id++) {
+	for (q_id = 0; q_id < bonded_eth_dev->data->nb_rx_queues; q_id++) {
 		bd_rx_q = (struct bond_rx_queue *)bonded_eth_dev->data->rx_queues[q_id];
 
 		errval = rte_eth_rx_queue_setup(slave_eth_dev->data->port_id, q_id,
@@ -1380,9 +1376,7 @@ slave_configure(struct rte_eth_dev *bonded_eth_dev,
 	}
 
 	/* Setup Tx Queues */
-	/* Use existing queues, if any */
-	for (q_id = old_nb_tx_queues;
-	     q_id < bonded_eth_dev->data->nb_tx_queues; q_id++) {
+	for (q_id = 0; q_id < bonded_eth_dev->data->nb_tx_queues; q_id++) {
 		bd_tx_q = (struct bond_tx_queue *)bonded_eth_dev->data->tx_queues[q_id];
 
 		errval = rte_eth_tx_queue_setup(slave_eth_dev->data->port_id, q_id,
-- 
2.7.4

Re: [dpdk-dev] [PATCH v2 0/5] bonding: setup all queues of slave devices

[Thomas Monjalon]

2016-11-24 12:26, Jan Blunck:
> Prior to 16.11 some drivers (e.g. virtio) still had problems if their
> queues where setup repeatedly. The bonding driver was working around the
> problem by reusing already setup queues. This series of patches changes the
> way how queue setup is done to give control to the driver to properly release
> already initialized queues before they are setup again. Therefore the driver
> call sequence is as if the number of queues is temporarily reduced before the
> queues are setup again.
> 
> Ilya Maximets (1):
>   Revert "bonding: use existing enslaved device queues"
> 
> Jan Blunck (4):
>   ethdev: Call rx/tx_queue_release before rx/tx_queue_setup
>   ethdev: Free rx/tx_queues after releasing all queues
>   ethdev: Add DPDK internal _rte_eth_dev_reset()
>   net/bonding: Force reconfiguration of removed slave interfaces

Applied, thanks