DPDK patches and discussions
 help / color / mirror / Atom feed
* [dpdk-dev] [RFC PATCH 0/3] *** SUBJECT HERE ***
@ 2017-12-01 14:47 Konstantin Ananyev
  2017-12-01 14:48 ` [dpdk-dev] [RFC PATCH 1/3] ethdev: introduce eth_queue_local Konstantin Ananyev
                   ` (3 more replies)
  0 siblings, 4 replies; 6+ messages in thread
From: Konstantin Ananyev @ 2017-12-01 14:47 UTC (permalink / raw)
  To: dev, dev; +Cc: Konstantin Ananyev

The series introduces 2 main changes:

1.Introduce a separate data structure (rte_eth_queue_local)
to store local to given process (i.e. no-shareable) information
for each configured rx/tx queue.
Memory for that structure is allocated/freed dynamically during
rte_eth_dev_configure().
Reserve a space for queue specific (rx|tx)_pkt_burst(),
tx_pkt_prepare() function pointers inside that structure.
Move rx/tx callback related information inside that structure.
That introduces a change in current behavior: all callbacks for
un-configured queues will be automatically removed.
Also as size of struct rte_eth_dev changes that patch is an ABI breakage,
so deprecation notice for 18.05 is filled.
Further suggestions how to introduce the same functionality
without ABI breakage are welcome.

2. Make it safe to remove rx/tx callback at runtime.
Right now it is not possible for the application to figure out
when it is safe to free removed callback handle and
associated with it resources(unless the queue is stopped).
That's probably not a big problem if all callbacks are static
hange through whole application lifetime)
and/or application doesn't allocate any resources for the callback handler.
Though if callbacks have to be added/removed dynamically and
callback handler would require more resources to operate properly -
then it might become an issue.
So patch #2 fixes that problem - now as soon as
rte_eth_remove_(rx|tx)_callback() completes successfully, application
can safely free all associated with the removed callback resources.

Performance impact:
If application doesn't use RX/TX callbacks, then the tests I run didn't
reveal any performance degradation.
Though if application do use RX/TX callbacks - patch #2 does introduce
some slowdown.
 
To be more specific here, on BDW (E5-2699 v4) 2.2GHz, 4x10Gb (X520-4)
with http://dpdk.org/dev/patchwork/patch/31864/ patch installed I got:
1) testpmd ... --latencystats=1 - slowdown < 1%
2) examples//l3fwd ... --parse-ptype - - slowdown < 1%
3) examples/rxtx_callbacks - slowdown ~8%
All that in terms of packet throughput (Mpps).

Ability to safely remove callbacks at runtime implies
some sort of synchronization.
Even I tried to make it as light as possible,
probably some slowdown is unavoidable.
Of course instead of introducing these changes at rte_ethdev layer
similar technique could be applied on individual callback basis.
In that case it would be up to callback writer/installer to decide
does he/she need a removable at runtime callback or not.
Though in that case, each installed callback might introduce extra
synchronization overhead and slowdown.

Konstantin Ananyev (3):
  ethdev: introduce eth_queue_local
  ethdev: make it safe to remove rx/tx callback at runtime
  doc: ethdev ABI change deprecation notice

 doc/guides/rel_notes/deprecation.rst |   5 +
 lib/librte_ether/rte_ethdev.c        | 390 ++++++++++++++++++++++-------------
 lib/librte_ether/rte_ethdev.h        | 174 ++++++++++++----
 3 files changed, 387 insertions(+), 182 deletions(-)

-- 
2.13.5

^ permalink raw reply	[flat|nested] 6+ messages in thread

* [dpdk-dev] [RFC PATCH 1/3] ethdev: introduce eth_queue_local
  2017-12-01 14:47 [dpdk-dev] [RFC PATCH 0/3] *** SUBJECT HERE *** Konstantin Ananyev
@ 2017-12-01 14:48 ` Konstantin Ananyev
  2017-12-01 14:48 ` [dpdk-dev] [RFC PATCH 2/3] ethdev: make it safe to remove rx/tx callback at runtime Konstantin Ananyev
                   ` (2 subsequent siblings)
  3 siblings, 0 replies; 6+ messages in thread
From: Konstantin Ananyev @ 2017-12-01 14:48 UTC (permalink / raw)
  To: dev, dev; +Cc: Konstantin Ananyev

Introduce a separate data structure (rte_eth_queue_local)
to store local to given process (i.e. no-shareable) information
for each configured rx/tx queue.
Memory for that structure is allocated/freed dynamically during
rte_eth_dev_configure().
Put a placeholder pointers for queue specific (rx|tx)_pkt_burst(),
tx_pkt_prepare() functions inside that structure.
Move rx/tx callback related information inside that structure.
That introduces a change in current behavior: all callbacks for
un-configured queues will be automatically removed.
Let say: rte_eth_dev_configure(port, 0, 0, ...);
would wipe out all installed callbacks for that port.


Signed-off-by: Konstantin Ananyev <konstantin.ananyev@intel.com>
---
 lib/librte_ether/rte_ethdev.c | 228 +++++++++++++++++++++++++++++-------------
 lib/librte_ether/rte_ethdev.h |  85 +++++++++++-----
 2 files changed, 216 insertions(+), 97 deletions(-)

diff --git a/lib/librte_ether/rte_ethdev.c b/lib/librte_ether/rte_ethdev.c
index 318af2869..ff8571d60 100644
--- a/lib/librte_ether/rte_ethdev.c
+++ b/lib/librte_ether/rte_ethdev.c
@@ -241,6 +241,14 @@ rte_eth_dev_allocate(const char *name)
 	return eth_dev;
 }
 
+static struct rte_eth_queue_local *
+alloc_queue_local(uint16_t nb_queues)
+{
+	struct rte_eth_queue_local *ql;
+	ql = rte_zmalloc(NULL, sizeof(ql[0]) * nb_queues, RTE_CACHE_LINE_SIZE);
+	return ql;
+}
+
 /*
  * Attach to a port already registered by the primary process, which
  * makes sure that the same device would have the same port id both
@@ -269,6 +277,16 @@ rte_eth_dev_attach_secondary(const char *name)
 	eth_dev = eth_dev_get(i);
 	RTE_ASSERT(eth_dev->data->port_id == i);
 
+	/*
+	 * obviously it is quite error prone:
+	 * if the primary process will decide to (re)configure device later,
+	 * there is no way for secondary one to notice that and update
+	 * it's local data (queue_local, rx|tx_pkt_burst, etc.).
+	 * We probably need an eth_dev_configure_secondary() or so.
+	 */
+	eth_dev->rx_ql = alloc_queue_local(eth_dev->data->nb_rx_queues);
+	eth_dev->tx_ql = alloc_queue_local(eth_dev->data->nb_tx_queues);
+
 	return eth_dev;
 }
 
@@ -444,52 +462,92 @@ rte_eth_dev_detach(uint16_t port_id, char *name)
 	return ret;
 }
 
+/*
+ * Helper routine - removes all registered RX/TX queue callbacks
+ * from the FIFO list.
+ * It is a caller responsibility to make sure no actual RX/TX happens
+ * simultanesoulsy on that queue.
+ */
+static void
+free_rxtx_cbs(struct rte_eth_rxtx_callback *cbs)
+{
+	struct rte_eth_rxtx_callback *cb, *next;
+
+	for (cb = cbs; cb != NULL; cb = next) {
+		next = cb->next;
+		rte_free(cb);
+	}
+}
+
+static void
+reset_queue_local(struct rte_eth_queue_local *ql)
+{
+	free_rxtx_cbs(ql->cbs);
+	memset(ql, 0, sizeof(*ql));
+}
+
 static int
 rte_eth_dev_rx_queue_config(struct rte_eth_dev *dev, uint16_t nb_queues)
 {
-	uint16_t old_nb_queues = dev->data->nb_rx_queues;
+	uint32_t diff, i, old_nb_queues;
+	struct rte_eth_queue_local *rlq;
 	void **rxq;
-	unsigned i;
 
-	if (dev->data->rx_queues == NULL && nb_queues != 0) { /* first time configuration */
-		dev->data->rx_queues = rte_zmalloc("ethdev->rx_queues",
-				sizeof(dev->data->rx_queues[0]) * nb_queues,
-				RTE_CACHE_LINE_SIZE);
-		if (dev->data->rx_queues == NULL) {
-			dev->data->nb_rx_queues = 0;
-			return -(ENOMEM);
-		}
-	} else if (dev->data->rx_queues != NULL && nb_queues != 0) { /* re-configure */
-		RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->rx_queue_release, -ENOTSUP);
+	old_nb_queues = dev->data->nb_rx_queues;
+
+	/* same # of queues nothing need to be done */
+	if (nb_queues == old_nb_queues)
+		return 0;
+
+	rxq = dev->data->rx_queues;
+	rlq = dev->rx_ql;
 
-		rxq = dev->data->rx_queues;
+	/* reduce # of queues */
+	if (old_nb_queues > nb_queues) {
 
-		for (i = nb_queues; i < old_nb_queues; i++)
+		RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->rx_queue_release,
+			-ENOTSUP);
+
+		dev->data->nb_rx_queues = nb_queues;
+
+		/* free resources used by the queues we are going to remove */
+		for (i = nb_queues; i < old_nb_queues; i++) {
+			reset_queue_local(rlq + i);
 			(*dev->dev_ops->rx_queue_release)(rxq[i]);
-		rxq = rte_realloc(rxq, sizeof(rxq[0]) * nb_queues,
-				RTE_CACHE_LINE_SIZE);
-		if (rxq == NULL)
-			return -(ENOMEM);
-		if (nb_queues > old_nb_queues) {
-			uint16_t new_qs = nb_queues - old_nb_queues;
-
-			memset(rxq + old_nb_queues, 0,
-				sizeof(rxq[0]) * new_qs);
+			rxq[i] = 0;
 		}
 
-		dev->data->rx_queues = rxq;
+		if (nb_queues == 0) {
+			dev->data->rx_queues = NULL;
+			dev->rx_ql = NULL;
+			rte_free(rxq);
+			rte_free(rlq);
+		}
+
+		return 0;
+	}
 
-	} else if (dev->data->rx_queues != NULL && nb_queues == 0) {
-		RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->rx_queue_release, -ENOTSUP);
+	/* increase # of queues */
 
-		rxq = dev->data->rx_queues;
+	diff = nb_queues - old_nb_queues;
 
-		for (i = nb_queues; i < old_nb_queues; i++)
-			(*dev->dev_ops->rx_queue_release)(rxq[i]);
+	rxq = rte_realloc(rxq, sizeof(rxq[0]) * nb_queues,
+			RTE_CACHE_LINE_SIZE);
+	rlq = rte_realloc(rlq, sizeof(rlq[0]) * nb_queues,
+			RTE_CACHE_LINE_SIZE);
 
-		rte_free(dev->data->rx_queues);
-		dev->data->rx_queues = NULL;
+	if (rxq != NULL) {
+		memset(rxq + old_nb_queues, 0, sizeof(rxq[0]) * diff);
+		dev->data->rx_queues = rxq;
+	}
+	if (rlq != NULL) {
+		memset(rlq + old_nb_queues, 0, sizeof(rlq[0]) * diff);
+		dev->rx_ql = rlq;
 	}
+
+	if (rxq == NULL || rlq == NULL)
+		return -ENOMEM;
+
 	dev->data->nb_rx_queues = nb_queues;
 	return 0;
 }
@@ -601,49 +659,65 @@ rte_eth_dev_tx_queue_stop(uint16_t port_id, uint16_t tx_queue_id)
 static int
 rte_eth_dev_tx_queue_config(struct rte_eth_dev *dev, uint16_t nb_queues)
 {
-	uint16_t old_nb_queues = dev->data->nb_tx_queues;
+	uint32_t diff, i, old_nb_queues;
+	struct rte_eth_queue_local *tlq;
 	void **txq;
-	unsigned i;
 
-	if (dev->data->tx_queues == NULL && nb_queues != 0) { /* first time configuration */
-		dev->data->tx_queues = rte_zmalloc("ethdev->tx_queues",
-						   sizeof(dev->data->tx_queues[0]) * nb_queues,
-						   RTE_CACHE_LINE_SIZE);
-		if (dev->data->tx_queues == NULL) {
-			dev->data->nb_tx_queues = 0;
-			return -(ENOMEM);
-		}
-	} else if (dev->data->tx_queues != NULL && nb_queues != 0) { /* re-configure */
-		RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->tx_queue_release, -ENOTSUP);
+	old_nb_queues = dev->data->nb_tx_queues;
+
+	/* same # of queues nothing need to be done */
+	if (nb_queues == old_nb_queues)
+		return 0;
+
+	txq = dev->data->tx_queues;
+	tlq = dev->tx_ql;
+
+	/* reduce # of queues */
+	if (old_nb_queues > nb_queues) {
+
+		RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->tx_queue_release,
+			-ENOTSUP);
 
-		txq = dev->data->tx_queues;
+		dev->data->nb_tx_queues = nb_queues;
 
-		for (i = nb_queues; i < old_nb_queues; i++)
+		/* free resources used by the queues we are going to remove */
+		for (i = nb_queues; i < old_nb_queues; i++) {
+			reset_queue_local(tlq + i);
 			(*dev->dev_ops->tx_queue_release)(txq[i]);
-		txq = rte_realloc(txq, sizeof(txq[0]) * nb_queues,
-				  RTE_CACHE_LINE_SIZE);
-		if (txq == NULL)
-			return -ENOMEM;
-		if (nb_queues > old_nb_queues) {
-			uint16_t new_qs = nb_queues - old_nb_queues;
-
-			memset(txq + old_nb_queues, 0,
-			       sizeof(txq[0]) * new_qs);
+			txq[i] = 0;
 		}
 
-		dev->data->tx_queues = txq;
+		if (nb_queues == 0) {
+			dev->data->tx_queues = NULL;
+			dev->tx_ql = NULL;
+			rte_free(txq);
+			rte_free(tlq);
+		}
+
+		return 0;
+	}
 
-	} else if (dev->data->tx_queues != NULL && nb_queues == 0) {
-		RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->tx_queue_release, -ENOTSUP);
+	/* increase # of queues */
 
-		txq = dev->data->tx_queues;
+	diff = nb_queues - old_nb_queues;
 
-		for (i = nb_queues; i < old_nb_queues; i++)
-			(*dev->dev_ops->tx_queue_release)(txq[i]);
+	txq = rte_realloc(txq, sizeof(txq[0]) * nb_queues,
+			RTE_CACHE_LINE_SIZE);
+	tlq = rte_realloc(tlq, sizeof(tlq[0]) * nb_queues,
+			RTE_CACHE_LINE_SIZE);
 
-		rte_free(dev->data->tx_queues);
-		dev->data->tx_queues = NULL;
+	if (txq != NULL) {
+		memset(txq + old_nb_queues, 0, sizeof(txq[0]) * diff);
+		dev->data->tx_queues = txq;
+	}
+	if (tlq != NULL) {
+		memset(tlq + old_nb_queues, 0, sizeof(tlq[0]) * diff);
+		dev->tx_ql = tlq;
 	}
+
+	if (txq == NULL || tlq == NULL)
+		return -ENOMEM;
+
 	dev->data->nb_tx_queues = nb_queues;
 	return 0;
 }
@@ -1084,6 +1158,7 @@ void
 rte_eth_dev_close(uint16_t port_id)
 {
 	struct rte_eth_dev *dev;
+	uint32_t i;
 
 	RTE_ETH_VALID_PORTID_OR_RET(port_id);
 	dev = &rte_eth_devices[port_id];
@@ -1092,12 +1167,25 @@ rte_eth_dev_close(uint16_t port_id)
 	dev->data->dev_started = 0;
 	(*dev->dev_ops->dev_close)(dev);
 
+	/* free local resources for RX/TX queues */
+
+	for (i = 0; i != dev->data->nb_rx_queues; i++)
+		reset_queue_local(dev->rx_ql + i);
+
+	for (i = 0; i != dev->data->nb_tx_queues; i++)
+		reset_queue_local(dev->tx_ql + i);
+
 	dev->data->nb_rx_queues = 0;
 	rte_free(dev->data->rx_queues);
 	dev->data->rx_queues = NULL;
+	rte_free(dev->rx_ql);
+	dev->rx_ql = NULL;
+
 	dev->data->nb_tx_queues = 0;
 	rte_free(dev->data->tx_queues);
 	dev->data->tx_queues = NULL;
+	rte_free(dev->tx_ql);
+	dev->tx_ql = NULL;
 }
 
 int
@@ -3111,10 +3199,10 @@ rte_eth_add_rx_callback(uint16_t port_id, uint16_t queue_id,
 	rte_spinlock_lock(&rte_eth_rx_cb_lock);
 	/* Add the callbacks in fifo order. */
 	struct rte_eth_rxtx_callback *tail =
-		rte_eth_devices[port_id].post_rx_burst_cbs[queue_id];
+		rte_eth_devices[port_id].rx_ql[queue_id].cbs;
 
 	if (!tail) {
-		rte_eth_devices[port_id].post_rx_burst_cbs[queue_id] = cb;
+		rte_eth_devices[port_id].rx_ql[queue_id].cbs = cb;
 
 	} else {
 		while (tail->next)
@@ -3153,9 +3241,9 @@ rte_eth_add_first_rx_callback(uint16_t port_id, uint16_t queue_id,
 
 	rte_spinlock_lock(&rte_eth_rx_cb_lock);
 	/* Add the callbacks at fisrt position*/
-	cb->next = rte_eth_devices[port_id].post_rx_burst_cbs[queue_id];
+	cb->next = rte_eth_devices[port_id].rx_ql[queue_id].cbs;
 	rte_smp_wmb();
-	rte_eth_devices[port_id].post_rx_burst_cbs[queue_id] = cb;
+	rte_eth_devices[port_id].rx_ql[queue_id].cbs = cb;
 	rte_spinlock_unlock(&rte_eth_rx_cb_lock);
 
 	return cb;
@@ -3189,10 +3277,10 @@ rte_eth_add_tx_callback(uint16_t port_id, uint16_t queue_id,
 	rte_spinlock_lock(&rte_eth_tx_cb_lock);
 	/* Add the callbacks in fifo order. */
 	struct rte_eth_rxtx_callback *tail =
-		rte_eth_devices[port_id].pre_tx_burst_cbs[queue_id];
+		rte_eth_devices[port_id].tx_ql[queue_id].cbs;
 
 	if (!tail) {
-		rte_eth_devices[port_id].pre_tx_burst_cbs[queue_id] = cb;
+		rte_eth_devices[port_id].tx_ql[queue_id].cbs = cb;
 
 	} else {
 		while (tail->next)
@@ -3223,7 +3311,7 @@ rte_eth_remove_rx_callback(uint16_t port_id, uint16_t queue_id,
 	int ret = -EINVAL;
 
 	rte_spinlock_lock(&rte_eth_rx_cb_lock);
-	prev_cb = &dev->post_rx_burst_cbs[queue_id];
+	prev_cb = &dev->rx_ql[queue_id].cbs;
 	for (; *prev_cb != NULL; prev_cb = &cb->next) {
 		cb = *prev_cb;
 		if (cb == user_cb) {
@@ -3257,7 +3345,7 @@ rte_eth_remove_tx_callback(uint16_t port_id, uint16_t queue_id,
 	struct rte_eth_rxtx_callback **prev_cb;
 
 	rte_spinlock_lock(&rte_eth_tx_cb_lock);
-	prev_cb = &dev->pre_tx_burst_cbs[queue_id];
+	prev_cb = &dev->tx_ql[queue_id].cbs;
 	for (; *prev_cb != NULL; prev_cb = &cb->next) {
 		cb = *prev_cb;
 		if (cb == user_cb) {
diff --git a/lib/librte_ether/rte_ethdev.h b/lib/librte_ether/rte_ethdev.h
index 341c2d624..d62e1bcc3 100644
--- a/lib/librte_ether/rte_ethdev.h
+++ b/lib/librte_ether/rte_ethdev.h
@@ -1694,6 +1694,10 @@ typedef uint16_t (*rte_tx_callback_fn)(uint16_t port, uint16_t queue,
  * @internal
  * Structure used to hold information about the callbacks to be called for a
  * queue on RX and TX.
+ * On RX: user-supplied functions called from rte_eth_rx_burst to post-process
+ * received packets before passing them to the user
+ * On TX: user-supplied functions called from rte_eth_tx_burst to pre-process
+ * received packets before passing them to the driver for transmission.
  */
 struct rte_eth_rxtx_callback {
 	struct rte_eth_rxtx_callback *next;
@@ -1715,6 +1719,24 @@ enum rte_eth_dev_state {
 
 /**
  * @internal
+ * Structure to hold process non-shareable information
+ * about RX/TX queue of the ethernet device.
+ */
+struct rte_eth_queue_local {
+	/**
+	 * placeholder for queue specific rx/tx and tx_preapare
+	 * functions pointers
+	 */
+	eth_rx_burst_t rx_pkt_burst; /**< receive function pointer. */
+	eth_tx_burst_t tx_pkt_burst; /**< transmit function pointer. */
+	eth_tx_prep_t tx_pkt_prepare; /**< transmit prepare function pointer. */
+
+	struct rte_eth_rxtx_callback *cbs;
+	/**< list of user supplied callbacks */
+} __rte_cache_aligned;
+
+/**
+ * @internal
  * The generic data structure associated with each ethernet device.
  *
  * Pointers to burst-oriented packet receive and transmit functions are
@@ -1730,19 +1752,13 @@ struct rte_eth_dev {
 	struct rte_eth_dev_data *data;  /**< Pointer to device data */
 	const struct eth_dev_ops *dev_ops; /**< Functions exported by PMD */
 	struct rte_device *device; /**< Backing device */
+
+	struct rte_eth_queue_local *rx_ql;  /**< RX queues local data */
+	struct rte_eth_queue_local *tx_ql;  /**< TX queues local data */
+
 	struct rte_intr_handle *intr_handle; /**< Device interrupt handle */
 	/** User application callbacks for NIC interrupts */
 	struct rte_eth_dev_cb_list link_intr_cbs;
-	/**
-	 * User-supplied functions called from rx_burst to post-process
-	 * received packets before passing them to the user
-	 */
-	struct rte_eth_rxtx_callback *post_rx_burst_cbs[RTE_MAX_QUEUES_PER_PORT];
-	/**
-	 * User-supplied functions called from tx_burst to pre-process
-	 * received packets before passing them to the driver for transmission.
-	 */
-	struct rte_eth_rxtx_callback *pre_tx_burst_cbs[RTE_MAX_QUEUES_PER_PORT];
 	enum rte_eth_dev_state state; /**< Flag indicating the port state */
 	void *security_ctx; /**< Context for security ops */
 } __rte_cache_aligned;
@@ -2883,29 +2899,37 @@ static inline uint16_t
 rte_eth_rx_burst(uint16_t port_id, uint16_t queue_id,
 		 struct rte_mbuf **rx_pkts, const uint16_t nb_pkts)
 {
+	uint16_t nb_rx;
 	struct rte_eth_dev *dev = &rte_eth_devices[port_id];
 
 #ifdef RTE_LIBRTE_ETHDEV_DEBUG
 	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, 0);
 	RTE_FUNC_PTR_OR_ERR_RET(*dev->rx_pkt_burst, 0);
 
-	if (queue_id >= dev->data->nb_rx_queues) {
+	if (queue_id >= dev->data->nb_rx_queues || dev->rx_ql == NULL) {
 		RTE_PMD_DEBUG_TRACE("Invalid RX queue_id=%d\n", queue_id);
 		return 0;
 	}
 #endif
-	int16_t nb_rx = (*dev->rx_pkt_burst)(dev->data->rx_queues[queue_id],
+	nb_rx = (*dev->rx_pkt_burst)(dev->data->rx_queues[queue_id],
 			rx_pkts, nb_pkts);
 
 #ifdef RTE_ETHDEV_RXTX_CALLBACKS
-	struct rte_eth_rxtx_callback *cb = dev->post_rx_burst_cbs[queue_id];
+	{
+		struct rte_eth_queue_local *ql;
+		struct rte_eth_rxtx_callback *cb;
 
-	if (unlikely(cb != NULL)) {
-		do {
-			nb_rx = cb->fn.rx(port_id, queue_id, rx_pkts, nb_rx,
+		ql = dev->rx_ql + queue_id;
+		cb = ql->cbs;
+
+		if (unlikely(cb != NULL)) {
+			do {
+				nb_rx = cb->fn.rx(port_id, queue_id,
+						rx_pkts, nb_rx,
 						nb_pkts, cb->param);
-			cb = cb->next;
-		} while (cb != NULL);
+				cb = cb->next;
+			} while (cb != NULL);
+		}
 	}
 #endif
 
@@ -3151,21 +3175,28 @@ rte_eth_tx_burst(uint16_t port_id, uint16_t queue_id,
 	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, 0);
 	RTE_FUNC_PTR_OR_ERR_RET(*dev->tx_pkt_burst, 0);
 
-	if (queue_id >= dev->data->nb_tx_queues) {
+	if (queue_id >= dev->data->nb_tx_queues || dev->tx_ql == NULL) {
 		RTE_PMD_DEBUG_TRACE("Invalid TX queue_id=%d\n", queue_id);
 		return 0;
 	}
 #endif
 
 #ifdef RTE_ETHDEV_RXTX_CALLBACKS
-	struct rte_eth_rxtx_callback *cb = dev->pre_tx_burst_cbs[queue_id];
-
-	if (unlikely(cb != NULL)) {
-		do {
-			nb_pkts = cb->fn.tx(port_id, queue_id, tx_pkts, nb_pkts,
-					cb->param);
-			cb = cb->next;
-		} while (cb != NULL);
+	{
+		struct rte_eth_queue_local *ql;
+		struct rte_eth_rxtx_callback *cb;
+
+		ql = dev->tx_ql + queue_id;
+		cb = ql->cbs;
+
+		if (unlikely(cb != NULL)) {
+			do {
+				nb_pkts = cb->fn.tx(port_id, queue_id,
+						tx_pkts, nb_pkts,
+						cb->param);
+				cb = cb->next;
+			} while (cb != NULL);
+		}
 	}
 #endif
 
-- 
2.13.5

^ permalink raw reply	[flat|nested] 6+ messages in thread

* [dpdk-dev] [RFC PATCH 2/3] ethdev: make it safe to remove rx/tx callback at runtime
  2017-12-01 14:47 [dpdk-dev] [RFC PATCH 0/3] *** SUBJECT HERE *** Konstantin Ananyev
  2017-12-01 14:48 ` [dpdk-dev] [RFC PATCH 1/3] ethdev: introduce eth_queue_local Konstantin Ananyev
@ 2017-12-01 14:48 ` Konstantin Ananyev
  2017-12-01 14:48 ` [dpdk-dev] [RFC PATCH 3/3] doc: ethdev ABI change deprecation notice Konstantin Ananyev
  2017-12-01 14:56 ` [dpdk-dev] [RFC PATCH 0/3] *** SUBJECT HERE *** Ananyev, Konstantin
  3 siblings, 0 replies; 6+ messages in thread
From: Konstantin Ananyev @ 2017-12-01 14:48 UTC (permalink / raw)
  To: dev, dev; +Cc: Konstantin Ananyev

Right now it is not possible for the application to know when it is safe
to free removed callback handle and associated with it resources
(unless the queue is stopped).
That patch changes that behavior to:
- if the rte_eth_remove_(rx|tx)_callback() completes successfully,
then it will automatically free the callback handle and the user can safely
release any associated with the removed callback resources.

Signed-off-by: Konstantin Ananyev <konstantin.ananyev@intel.com>
---
 lib/librte_ether/rte_ethdev.c | 180 +++++++++++++++++++++++-------------------
 lib/librte_ether/rte_ethdev.h | 127 ++++++++++++++++++++++-------
 2 files changed, 194 insertions(+), 113 deletions(-)

diff --git a/lib/librte_ether/rte_ethdev.c b/lib/librte_ether/rte_ethdev.c
index ff8571d60..30b23b9b2 100644
--- a/lib/librte_ether/rte_ethdev.c
+++ b/lib/librte_ether/rte_ethdev.c
@@ -482,7 +482,7 @@ free_rxtx_cbs(struct rte_eth_rxtx_callback *cbs)
 static void
 reset_queue_local(struct rte_eth_queue_local *ql)
 {
-	free_rxtx_cbs(ql->cbs);
+	free_rxtx_cbs(ql->cbs.head);
 	memset(ql, 0, sizeof(*ql));
 }
 
@@ -3172,6 +3172,79 @@ rte_eth_dev_filter_ctrl(uint16_t port_id, enum rte_filter_type filter_type,
 	return (*dev->dev_ops->filter_ctrl)(dev, filter_type, filter_op, arg);
 }
 
+#ifdef RTE_ETHDEV_RXTX_CALLBACKS
+
+/*
+ * Helper routine - contains common code to add RX/TX queue callbacks
+ * to the list.
+ */
+static struct rte_eth_rxtx_callback *
+add_rxtx_callback(struct rte_eth_rxtx_cbs *cbs, int32_t first,
+	struct rte_eth_rxtx_callback *cb, rte_spinlock_t *lock)
+{
+	struct rte_eth_rxtx_callback *tail, **pt;
+
+	rte_spinlock_lock(lock);
+
+	/* Add callback to the head of the list. */
+	if (first != 0) {
+		cb->next = cbs->head;
+		rte_smp_wmb();
+		cbs->head = cb;
+
+	/* Add callback to the tail of the list. */
+	} else {
+		for (pt = &cbs->head; *pt != NULL; pt = &tail->next)
+			tail = *pt;
+
+		*pt = cb;
+	}
+
+	rte_spinlock_unlock(lock);
+	return cb;
+}
+
+/*
+ * Helper routine - contains common code to delete RX/TX queue callbacks
+ * from the FIFO list.
+ */
+static int
+del_rxtx_callback(struct rte_eth_rxtx_cbs *cbs,
+	struct rte_eth_rxtx_callback *user_cb, rte_spinlock_t *lock)
+{
+	int32_t ret;
+	struct rte_eth_rxtx_callback *cb, **prev_cb;
+
+	ret = -EINVAL;
+	rte_spinlock_lock(lock);
+
+	for (prev_cb = &cbs->head; *prev_cb != NULL; prev_cb = &cb->next) {
+
+		cb = *prev_cb;
+		if (cb == user_cb) {
+			/* Remove the user cb from the callback list. */
+			*prev_cb = cb->next;
+			ret = 0;
+			break;
+		}
+	}
+
+	rte_spinlock_unlock(lock);
+
+	/*
+	 * first make sure datapath doesn't use removed callback anymore,
+	 * then free the callback structure.
+	 */
+	if (ret == 0) {
+		__rte_eth_rxtx_cbs_wait(cbs);
+		rte_free(cb);
+	}
+
+	return ret;
+}
+
+#endif /* RTE_ETHDEV_RXTX_CALLBACKS */
+
 void *
 rte_eth_add_rx_callback(uint16_t port_id, uint16_t queue_id,
 		rte_rx_callback_fn fn, void *user_param)
@@ -3180,14 +3253,16 @@ rte_eth_add_rx_callback(uint16_t port_id, uint16_t queue_id,
 	rte_errno = ENOTSUP;
 	return NULL;
 #endif
+	struct rte_eth_rxtx_callback *cb;
+
 	/* check input parameters */
 	if (!rte_eth_dev_is_valid_port(port_id) || fn == NULL ||
 		    queue_id >= rte_eth_devices[port_id].data->nb_rx_queues) {
 		rte_errno = EINVAL;
 		return NULL;
 	}
-	struct rte_eth_rxtx_callback *cb = rte_zmalloc(NULL, sizeof(*cb), 0);
 
+	cb = rte_zmalloc(NULL, sizeof(*cb), 0);
 	if (cb == NULL) {
 		rte_errno = ENOMEM;
 		return NULL;
@@ -3196,22 +3271,8 @@ rte_eth_add_rx_callback(uint16_t port_id, uint16_t queue_id,
 	cb->fn.rx = fn;
 	cb->param = user_param;
 
-	rte_spinlock_lock(&rte_eth_rx_cb_lock);
-	/* Add the callbacks in fifo order. */
-	struct rte_eth_rxtx_callback *tail =
-		rte_eth_devices[port_id].rx_ql[queue_id].cbs;
-
-	if (!tail) {
-		rte_eth_devices[port_id].rx_ql[queue_id].cbs = cb;
-
-	} else {
-		while (tail->next)
-			tail = tail->next;
-		tail->next = cb;
-	}
-	rte_spinlock_unlock(&rte_eth_rx_cb_lock);
-
-	return cb;
+	return add_rxtx_callback(&rte_eth_devices[port_id].rx_ql[queue_id].cbs,
+		0, cb, &rte_eth_rx_cb_lock);
 }
 
 void *
@@ -3222,6 +3283,8 @@ rte_eth_add_first_rx_callback(uint16_t port_id, uint16_t queue_id,
 	rte_errno = ENOTSUP;
 	return NULL;
 #endif
+	struct rte_eth_rxtx_callback *cb;
+
 	/* check input parameters */
 	if (!rte_eth_dev_is_valid_port(port_id) || fn == NULL ||
 		queue_id >= rte_eth_devices[port_id].data->nb_rx_queues) {
@@ -3229,7 +3292,7 @@ rte_eth_add_first_rx_callback(uint16_t port_id, uint16_t queue_id,
 		return NULL;
 	}
 
-	struct rte_eth_rxtx_callback *cb = rte_zmalloc(NULL, sizeof(*cb), 0);
+	cb = rte_zmalloc(NULL, sizeof(*cb), 0);
 
 	if (cb == NULL) {
 		rte_errno = ENOMEM;
@@ -3239,14 +3302,8 @@ rte_eth_add_first_rx_callback(uint16_t port_id, uint16_t queue_id,
 	cb->fn.rx = fn;
 	cb->param = user_param;
 
-	rte_spinlock_lock(&rte_eth_rx_cb_lock);
-	/* Add the callbacks at fisrt position*/
-	cb->next = rte_eth_devices[port_id].rx_ql[queue_id].cbs;
-	rte_smp_wmb();
-	rte_eth_devices[port_id].rx_ql[queue_id].cbs = cb;
-	rte_spinlock_unlock(&rte_eth_rx_cb_lock);
-
-	return cb;
+	return add_rxtx_callback(&rte_eth_devices[port_id].rx_ql[queue_id].cbs,
+		1, cb, &rte_eth_rx_cb_lock);
 }
 
 void *
@@ -3257,6 +3314,8 @@ rte_eth_add_tx_callback(uint16_t port_id, uint16_t queue_id,
 	rte_errno = ENOTSUP;
 	return NULL;
 #endif
+	struct rte_eth_rxtx_callback *cb;
+
 	/* check input parameters */
 	if (!rte_eth_dev_is_valid_port(port_id) || fn == NULL ||
 		    queue_id >= rte_eth_devices[port_id].data->nb_tx_queues) {
@@ -3264,8 +3323,7 @@ rte_eth_add_tx_callback(uint16_t port_id, uint16_t queue_id,
 		return NULL;
 	}
 
-	struct rte_eth_rxtx_callback *cb = rte_zmalloc(NULL, sizeof(*cb), 0);
-
+	cb = rte_zmalloc(NULL, sizeof(*cb), 0);
 	if (cb == NULL) {
 		rte_errno = ENOMEM;
 		return NULL;
@@ -3274,22 +3332,8 @@ rte_eth_add_tx_callback(uint16_t port_id, uint16_t queue_id,
 	cb->fn.tx = fn;
 	cb->param = user_param;
 
-	rte_spinlock_lock(&rte_eth_tx_cb_lock);
-	/* Add the callbacks in fifo order. */
-	struct rte_eth_rxtx_callback *tail =
-		rte_eth_devices[port_id].tx_ql[queue_id].cbs;
-
-	if (!tail) {
-		rte_eth_devices[port_id].tx_ql[queue_id].cbs = cb;
-
-	} else {
-		while (tail->next)
-			tail = tail->next;
-		tail->next = cb;
-	}
-	rte_spinlock_unlock(&rte_eth_tx_cb_lock);
-
-	return cb;
+	return add_rxtx_callback(&rte_eth_devices[port_id].tx_ql[queue_id].cbs,
+		0, cb, &rte_eth_tx_cb_lock);
 }
 
 int
@@ -3299,31 +3343,16 @@ rte_eth_remove_rx_callback(uint16_t port_id, uint16_t queue_id,
 #ifndef RTE_ETHDEV_RXTX_CALLBACKS
 	return -ENOTSUP;
 #endif
+	struct rte_eth_dev *dev = &rte_eth_devices[port_id];
+
 	/* Check input parameters. */
 	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -EINVAL);
 	if (user_cb == NULL ||
 			queue_id >= rte_eth_devices[port_id].data->nb_rx_queues)
 		return -EINVAL;
 
-	struct rte_eth_dev *dev = &rte_eth_devices[port_id];
-	struct rte_eth_rxtx_callback *cb;
-	struct rte_eth_rxtx_callback **prev_cb;
-	int ret = -EINVAL;
-
-	rte_spinlock_lock(&rte_eth_rx_cb_lock);
-	prev_cb = &dev->rx_ql[queue_id].cbs;
-	for (; *prev_cb != NULL; prev_cb = &cb->next) {
-		cb = *prev_cb;
-		if (cb == user_cb) {
-			/* Remove the user cb from the callback list. */
-			*prev_cb = cb->next;
-			ret = 0;
-			break;
-		}
-	}
-	rte_spinlock_unlock(&rte_eth_rx_cb_lock);
-
-	return ret;
+	return del_rxtx_callback(&dev->rx_ql[queue_id].cbs, user_cb,
+		&rte_eth_rx_cb_lock);
 }
 
 int
@@ -3333,31 +3362,16 @@ rte_eth_remove_tx_callback(uint16_t port_id, uint16_t queue_id,
 #ifndef RTE_ETHDEV_RXTX_CALLBACKS
 	return -ENOTSUP;
 #endif
+	struct rte_eth_dev *dev = &rte_eth_devices[port_id];
+
 	/* Check input parameters. */
 	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -EINVAL);
 	if (user_cb == NULL ||
 			queue_id >= rte_eth_devices[port_id].data->nb_tx_queues)
 		return -EINVAL;
 
-	struct rte_eth_dev *dev = &rte_eth_devices[port_id];
-	int ret = -EINVAL;
-	struct rte_eth_rxtx_callback *cb;
-	struct rte_eth_rxtx_callback **prev_cb;
-
-	rte_spinlock_lock(&rte_eth_tx_cb_lock);
-	prev_cb = &dev->tx_ql[queue_id].cbs;
-	for (; *prev_cb != NULL; prev_cb = &cb->next) {
-		cb = *prev_cb;
-		if (cb == user_cb) {
-			/* Remove the user cb from the callback list. */
-			*prev_cb = cb->next;
-			ret = 0;
-			break;
-		}
-	}
-	rte_spinlock_unlock(&rte_eth_tx_cb_lock);
-
-	return ret;
+	return del_rxtx_callback(&dev->tx_ql[queue_id].cbs, user_cb,
+		&rte_eth_tx_cb_lock);
 }
 
 int
diff --git a/lib/librte_ether/rte_ethdev.h b/lib/librte_ether/rte_ethdev.h
index d62e1bcc3..2957f620e 100644
--- a/lib/librte_ether/rte_ethdev.h
+++ b/lib/librte_ether/rte_ethdev.h
@@ -1709,6 +1709,23 @@ struct rte_eth_rxtx_callback {
 };
 
 /**
+ * @internal
+ * Structure used to hold list of RX/TX callbacks, plus usage counter.
+ * Usage counter is incremented each time (rx|tx)_burst starts/stops
+ * using callback list.
+ */
+struct rte_eth_rxtx_cbs {
+	struct rte_eth_rxtx_callback *head; /**< head of callbacks list */
+	uint32_t use; /**< usage counter */
+};
+
+/*
+ * Odd number means that callback list is used by datapath (RX/TX)
+ * Even number means that callback list is not used by datapath (RX/TX)
+ */
+#define	RTE_ETH_RXTX_CBS_INUSE	1
+
+/**
  * A set of values to describe the possible states of an eth device.
  */
 enum rte_eth_dev_state {
@@ -1731,7 +1748,7 @@ struct rte_eth_queue_local {
 	eth_tx_burst_t tx_pkt_burst; /**< transmit function pointer. */
 	eth_tx_prep_t tx_pkt_prepare; /**< transmit prepare function pointer. */
 
-	struct rte_eth_rxtx_callback *cbs;
+	struct rte_eth_rxtx_cbs cbs;
 	/**< list of user supplied callbacks */
 } __rte_cache_aligned;
 
@@ -2814,6 +2831,60 @@ int rte_eth_dev_get_vlan_offload(uint16_t port_id);
 int rte_eth_dev_set_vlan_pvid(uint16_t port_id, uint16_t pvid, int on);
 
 /**
+ * @internal
+ * Marks given callback list as used by datapath (RX/TX).
+ * @param cbs
+ *  Pointer to the callback list structure.
+ */
+static __rte_always_inline void
+__rte_eth_rxtx_cbs_inuse(struct rte_eth_rxtx_cbs *cbs)
+{
+	cbs->use++;
+	/* make sure no store/load reordering could happen */
+	rte_smp_mb();
+}
+
+/**
+ * @internal
+ * Marks given callback list as not used by datapath (RX/TX).
+ * @param cbs
+ *  Pointer to the callback list structure.
+ */
+static __rte_always_inline void
+__rte_eth_rxtx_cbs_unuse(struct rte_eth_rxtx_cbs *cbs)
+{
+	/* make sure all previous loads are completed */
+	rte_smp_rmb();
+	cbs->use++;
+}
+
+/**
+ * @internal
+ * Waits till datapath (RX/TX) finished using given callback list.
+ * @param cbs
+ *  Pointer to the callback list structure.
+ */
+static inline void
+__rte_eth_rxtx_cbs_wait(const struct rte_eth_rxtx_cbs *cbs)
+{
+	uint32_t nuse, puse;
+
+	/* make sure all previous loads and stores are completed */
+	rte_smp_mb();
+
+	puse = cbs->use;
+
+	/* in use, busy wait till current RX/TX iteration is finished */
+	if ((puse & RTE_ETH_RXTX_CBS_INUSE) != 0) {
+		do {
+			rte_pause();
+			rte_compiler_barrier();
+			nuse = cbs->use;
+		} while (nuse == puse);
+	}
+}
+
+/**
  *
  * Retrieve a burst of input packets from a receive queue of an Ethernet
  * device. The retrieved packets are stored in *rte_mbuf* structures whose
@@ -2911,6 +2982,7 @@ rte_eth_rx_burst(uint16_t port_id, uint16_t queue_id,
 		return 0;
 	}
 #endif
+
 	nb_rx = (*dev->rx_pkt_burst)(dev->data->rx_queues[queue_id],
 			rx_pkts, nb_pkts);
 
@@ -2920,15 +2992,15 @@ rte_eth_rx_burst(uint16_t port_id, uint16_t queue_id,
 		struct rte_eth_rxtx_callback *cb;
 
 		ql = dev->rx_ql + queue_id;
-		cb = ql->cbs;
-
-		if (unlikely(cb != NULL)) {
-			do {
-				nb_rx = cb->fn.rx(port_id, queue_id,
-						rx_pkts, nb_rx,
-						nb_pkts, cb->param);
-				cb = cb->next;
-			} while (cb != NULL);
+		if (unlikely(ql->cbs.head != NULL)) {
+
+			__rte_eth_rxtx_cbs_inuse(&ql->cbs);
+
+			for (cb = ql->cbs.head; cb != NULL; cb = cb->next)
+				nb_rx = cb->fn.rx(port_id, queue_id, rx_pkts,
+					nb_rx, nb_pkts, cb->param);
+
+			__rte_eth_rxtx_cbs_unuse(&ql->cbs);
 		}
 	}
 #endif
@@ -3187,20 +3259,21 @@ rte_eth_tx_burst(uint16_t port_id, uint16_t queue_id,
 		struct rte_eth_rxtx_callback *cb;
 
 		ql = dev->tx_ql + queue_id;
-		cb = ql->cbs;
-
-		if (unlikely(cb != NULL)) {
-			do {
-				nb_pkts = cb->fn.tx(port_id, queue_id,
-						tx_pkts, nb_pkts,
-						cb->param);
-				cb = cb->next;
-			} while (cb != NULL);
+		if (unlikely(ql->cbs.head != NULL)) {
+
+			__rte_eth_rxtx_cbs_inuse(&ql->cbs);
+
+			for (cb = ql->cbs.head; cb != NULL; cb = cb->next)
+				nb_pkts = cb->fn.tx(port_id, queue_id, tx_pkts,
+					nb_pkts, cb->param);
+
+			__rte_eth_rxtx_cbs_unuse(&ql->cbs);
 		}
 	}
 #endif
 
-	return (*dev->tx_pkt_burst)(dev->data->tx_queues[queue_id], tx_pkts, nb_pkts);
+	return (*dev->tx_pkt_burst)(dev->data->tx_queues[queue_id], tx_pkts,
+		nb_pkts);
 }
 
 /**
@@ -4199,16 +4272,10 @@ void *rte_eth_add_tx_callback(uint16_t port_id, uint16_t queue_id,
  * This function is used to removed callbacks that were added to a NIC port
  * queue using rte_eth_add_rx_callback().
  *
- * Note: the callback is removed from the callback list but it isn't freed
- * since the it may still be in use. The memory for the callback can be
- * subsequently freed back by the application by calling rte_free():
- *
- * - Immediately - if the port is stopped, or the user knows that no
- *   callbacks are in flight e.g. if called from the thread doing RX/TX
- *   on that queue.
- *
- * - After a short delay - where the delay is sufficient to allow any
- *   in-flight callbacks to complete.
+ * Note: that after callback is removed from the callback list associated
+ * with it memory is freed, and user shouldn't refer it any more.
+ * After successfull completion of that function user can safely release
+ * any resources associated with that callback.
  *
  * @param port_id
  *   The port identifier of the Ethernet device.
-- 
2.13.5

^ permalink raw reply	[flat|nested] 6+ messages in thread

* [dpdk-dev] [RFC PATCH 3/3] doc: ethdev ABI change deprecation notice
  2017-12-01 14:47 [dpdk-dev] [RFC PATCH 0/3] *** SUBJECT HERE *** Konstantin Ananyev
  2017-12-01 14:48 ` [dpdk-dev] [RFC PATCH 1/3] ethdev: introduce eth_queue_local Konstantin Ananyev
  2017-12-01 14:48 ` [dpdk-dev] [RFC PATCH 2/3] ethdev: make it safe to remove rx/tx callback at runtime Konstantin Ananyev
@ 2017-12-01 14:48 ` Konstantin Ananyev
  2017-12-12 14:55   ` Kovacevic, Marko
  2017-12-01 14:56 ` [dpdk-dev] [RFC PATCH 0/3] *** SUBJECT HERE *** Ananyev, Konstantin
  3 siblings, 1 reply; 6+ messages in thread
From: Konstantin Ananyev @ 2017-12-01 14:48 UTC (permalink / raw)
  To: dev, dev; +Cc: Konstantin Ananyev

Signed-off-by: Konstantin Ananyev <konstantin.ananyev@intel.com>
---
 doc/guides/rel_notes/deprecation.rst | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/doc/guides/rel_notes/deprecation.rst b/doc/guides/rel_notes/deprecation.rst
index 13e85432f..038b55fd5 100644
--- a/doc/guides/rel_notes/deprecation.rst
+++ b/doc/guides/rel_notes/deprecation.rst
@@ -8,6 +8,11 @@ API and ABI deprecation notices are to be posted here.
 Deprecation Notices
 -------------------
 
+* ethdev: an ABI changes for ``rte_ethdev`` are planned in v18.05.
+  The size and layout of struct rte_eth_dev will change.
+  Mainly to accommodate queue specific RX/TX function pointers, plus
+  reorganize RX/TX callback related information.
+
 * eal: several API and ABI changes are planned for ``rte_devargs`` in v18.02.
   The format of device command line parameters will change. The bus will need
   to be explicitly stated in the device declaration. The enum ``rte_devtype``
-- 
2.13.5

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [dpdk-dev] [RFC PATCH 0/3] *** SUBJECT HERE ***
  2017-12-01 14:47 [dpdk-dev] [RFC PATCH 0/3] *** SUBJECT HERE *** Konstantin Ananyev
                   ` (2 preceding siblings ...)
  2017-12-01 14:48 ` [dpdk-dev] [RFC PATCH 3/3] doc: ethdev ABI change deprecation notice Konstantin Ananyev
@ 2017-12-01 14:56 ` Ananyev, Konstantin
  3 siblings, 0 replies; 6+ messages in thread
From: Ananyev, Konstantin @ 2017-12-01 14:56 UTC (permalink / raw)
  To: dev

Oops sorry, resending with proper subject.

> -----Original Message-----
> From: Ananyev, Konstantin
> Sent: Friday, December 1, 2017 2:48 PM
> To: dev@dpdk.org; dev@dpdk.org
> Cc: Ananyev, Konstantin <konstantin.ananyev@intel.com>
> Subject: [RFC PATCH 0/3] *** SUBJECT HERE ***
> 
> The series introduces 2 main changes:
> 
> 1.Introduce a separate data structure (rte_eth_queue_local)
> to store local to given process (i.e. no-shareable) information
> for each configured rx/tx queue.
> Memory for that structure is allocated/freed dynamically during
> rte_eth_dev_configure().
> Reserve a space for queue specific (rx|tx)_pkt_burst(),
> tx_pkt_prepare() function pointers inside that structure.
> Move rx/tx callback related information inside that structure.
> That introduces a change in current behavior: all callbacks for
> un-configured queues will be automatically removed.
> Also as size of struct rte_eth_dev changes that patch is an ABI breakage,
> so deprecation notice for 18.05 is filled.
> Further suggestions how to introduce the same functionality
> without ABI breakage are welcome.
> 
> 2. Make it safe to remove rx/tx callback at runtime.
> Right now it is not possible for the application to figure out
> when it is safe to free removed callback handle and
> associated with it resources(unless the queue is stopped).
> That's probably not a big problem if all callbacks are static
> hange through whole application lifetime)
> and/or application doesn't allocate any resources for the callback handler.
> Though if callbacks have to be added/removed dynamically and
> callback handler would require more resources to operate properly -
> then it might become an issue.
> So patch #2 fixes that problem - now as soon as
> rte_eth_remove_(rx|tx)_callback() completes successfully, application
> can safely free all associated with the removed callback resources.
> 
> Performance impact:
> If application doesn't use RX/TX callbacks, then the tests I run didn't
> reveal any performance degradation.
> Though if application do use RX/TX callbacks - patch #2 does introduce
> some slowdown.
> 
> To be more specific here, on BDW (E5-2699 v4) 2.2GHz, 4x10Gb (X520-4)
> with http://dpdk.org/dev/patchwork/patch/31864/ patch installed I got:
> 1) testpmd ... --latencystats=1 - slowdown < 1%
> 2) examples//l3fwd ... --parse-ptype - - slowdown < 1%
> 3) examples/rxtx_callbacks - slowdown ~8%
> All that in terms of packet throughput (Mpps).
> 
> Ability to safely remove callbacks at runtime implies
> some sort of synchronization.
> Even I tried to make it as light as possible,
> probably some slowdown is unavoidable.
> Of course instead of introducing these changes at rte_ethdev layer
> similar technique could be applied on individual callback basis.
> In that case it would be up to callback writer/installer to decide
> does he/she need a removable at runtime callback or not.
> Though in that case, each installed callback might introduce extra
> synchronization overhead and slowdown.
> 
> Konstantin Ananyev (3):
>   ethdev: introduce eth_queue_local
>   ethdev: make it safe to remove rx/tx callback at runtime
>   doc: ethdev ABI change deprecation notice
> 
>  doc/guides/rel_notes/deprecation.rst |   5 +
>  lib/librte_ether/rte_ethdev.c        | 390 ++++++++++++++++++++++-------------
>  lib/librte_ether/rte_ethdev.h        | 174 ++++++++++++----
>  3 files changed, 387 insertions(+), 182 deletions(-)
> 
> --
> 2.13.5

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [dpdk-dev] [RFC PATCH 3/3] doc: ethdev ABI change deprecation notice
  2017-12-01 14:48 ` [dpdk-dev] [RFC PATCH 3/3] doc: ethdev ABI change deprecation notice Konstantin Ananyev
@ 2017-12-12 14:55   ` Kovacevic, Marko
  0 siblings, 0 replies; 6+ messages in thread
From: Kovacevic, Marko @ 2017-12-12 14:55 UTC (permalink / raw)
  To: Ananyev, Konstantin, dev; +Cc: Ananyev, Konstantin



> -----Original Message-----
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Konstantin Ananyev
> Sent: Friday, December 1, 2017 2:48 PM
> To: dev@dpdk.org; dev@dpdk.org
> Cc: Ananyev, Konstantin <konstantin.ananyev@intel.com>
> Subject: [dpdk-dev] [RFC PATCH 3/3] doc: ethdev ABI change deprecation notice
> 
> Signed-off-by: Konstantin Ananyev <konstantin.ananyev@intel.com>
> ---
>  doc/guides/rel_notes/deprecation.rst | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/doc/guides/rel_notes/deprecation.rst
> b/doc/guides/rel_notes/deprecation.rst
> index 13e85432f..038b55fd5 100644
> --- a/doc/guides/rel_notes/deprecation.rst
> +++ b/doc/guides/rel_notes/deprecation.rst
> @@ -8,6 +8,11 @@ API and ABI deprecation notices are to be posted here.
>  Deprecation Notices
>  -------------------
> 
> +* ethdev: an ABI changes for ``rte_ethdev`` are planned in v18.05.
> +  The size and layout of struct rte_eth_dev will change.
> +  Mainly to accommodate queue specific RX/TX function pointers, plus
> +  reorganize RX/TX callback related information.
> +
>  * eal: several API and ABI changes are planned for ``rte_devargs`` in v18.02.
>    The format of device command line parameters will change. The bus will need
>    to be explicitly stated in the device declaration. The enum ``rte_devtype``
> --
> 2.13.5

Acked-by: Marko Kovacevic <marko.kovacevic@intel.com>

^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2017-12-12 14:56 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-12-01 14:47 [dpdk-dev] [RFC PATCH 0/3] *** SUBJECT HERE *** Konstantin Ananyev
2017-12-01 14:48 ` [dpdk-dev] [RFC PATCH 1/3] ethdev: introduce eth_queue_local Konstantin Ananyev
2017-12-01 14:48 ` [dpdk-dev] [RFC PATCH 2/3] ethdev: make it safe to remove rx/tx callback at runtime Konstantin Ananyev
2017-12-01 14:48 ` [dpdk-dev] [RFC PATCH 3/3] doc: ethdev ABI change deprecation notice Konstantin Ananyev
2017-12-12 14:55   ` Kovacevic, Marko
2017-12-01 14:56 ` [dpdk-dev] [RFC PATCH 0/3] *** SUBJECT HERE *** Ananyev, Konstantin

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).