Skip to contentSkip to conversation

new API to free consumed buffers in Tx ring

PATCH Superseded Billy McFall 2 participants 7 messages Jan 11, 2017 – Jan 20, 2017 Network drivers

v2 of 8 Superseded Latest is v8 →

lore
Share & tools
lore
#0 lore raw

v2 cover new API to free consumed buffers in Tx ring

Based on a request from Damjan Marion and seconded by Keith Wiles, see dpdk-dev mailing list from 11/21/2016, add a new API to free consumed buffers on TX ring. This addresses two scenarios:
1) Flooding a packet and want to reuse existing mbuf to avoid a packet copy. Increment the reference count of the packet and poll new API until reference count is decremented.
2) Application runs out of mbufs, or resets and is preparing for additional run, call API to free consumed packets so processing can continue.

API will return the number of packets freed (0-n) or error code if feature not supported (-ENOTSUP) or input invalid (-ENODEV).

API for e1000 igb driver and vHost driver have been implemented. Other drivers can be implemented over time. Some drivers implement a Tx done flush routine that should be reused where possible. e1000 igb driver and vHost driver do not have such functions.

---
v2:
* Removed rte_eth_tx_buffer_flush() call and associated parameters from new rte_eth_tx_done_cleanup() API.

* Remaining open issue is whether this should be a new API or overload existing rte_eth_tx_buffer() API with input parameter nb_pkts set to
0. My concern is that overloading existing API will not provided enough feedback to application. Application needs to know if feature is supported and driver is attempting to free mbufs or not.

* If new API is supported, second open issue is if parameter free_cnt should be included. It was in the original feature request.

Billy McFall (3):
ethdev: new API to free consumed buffers in Tx ring net/e1000: e1000 igb support to free consumed buffers net/vhost: vHost support to free consumed buffers

 drivers/net/e1000/e1000_ethdev.h  |   2 +
 drivers/net/e1000/igb_ethdev.c    |   1 +
 drivers/net/e1000/igb_rxtx.c      | 126 ++++++++++++++++++++++++++++++++++++++
 drivers/net/vhost/rte_eth_vhost.c |  11 ++++
 lib/librte_ether/rte_ethdev.h     |  43 +++++++++++++
 5 files changed, 183 insertions(+)
Signature · 1 line
2.9.3
#1 lore raw

PATCH v2 1/3 ethdev: new API to free consumed buffers in Tx ring

Add a new API to force free consumed buffers on Tx ring. API will return the number of packets freed (0-n) or error code if feature not supported (-ENOTSUP) or input invalid (-ENODEV).

---
 lib/librte_ether/rte_ethdev.h | 43 +++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 43 insertions(+)
lib/librte_ether/rte_ethdev.h +43 -0
index 9678179..ce662b6 100644
@@ -1150,6 +1150,9 @@ typedef uint32_t (*eth_rx_queue_count_t)(struct rte_eth_dev *dev,
 typedef int (*eth_rx_descriptor_done_t)(void *rxq, uint16_t offset);
 /**< @internal Check DD bit of specific RX descriptor */
 
+typedef int (*eth_tx_done_cleanup_t)(void *txq, uint32_t free_cnt);
+/**< @internal Force mbufs to be from TX ring. */
+
 typedef void (*eth_rxq_info_get_t)(struct rte_eth_dev *dev,
 	uint16_t rx_queue_id, struct rte_eth_rxq_info *qinfo);
 
@@ -1467,6 +1470,7 @@ struct eth_dev_ops {
 	eth_rx_disable_intr_t      rx_queue_intr_disable;
 	eth_tx_queue_setup_t       tx_queue_setup;/**< Set up device TX queue.*/
 	eth_queue_release_t        tx_queue_release;/**< Release TX queue.*/
+	eth_tx_done_cleanup_t      tx_done_cleanup;/**< Free tx ring mbufs */
 	eth_dev_led_on_t           dev_led_on;    /**< Turn on LED. */
 	eth_dev_led_off_t          dev_led_off;   /**< Turn off LED. */
 	flow_ctrl_get_t            flow_ctrl_get; /**< Get flow control. */
@@ -2943,6 +2947,45 @@ rte_eth_tx_buffer(uint8_t port_id, uint16_t queue_id,
 }
 
 /**
+ * Request the driver to free mbufs currently cached by the driver. The
+ * driver will only free the mbuf if it is no longer in use. It is the
+ * application's responsibity to ensure rte_eth_tx_buffer_flush(..) is
+ * called if needed.
+ *
+ * @param port_id
+ *   The port identifier of the Ethernet device.
+ * @param queue_id
+ *   The index of the transmit queue through which output packets must be
+ *   sent.
+ *   The value must be in the range [0, nb_tx_queue - 1] previously supplied
+ *   to rte_eth_dev_configure().
+ * @param free_cnt
+ *   Maximum number of packets to free. Use 0 to indicate all possible packets
+ *   should be freed. Note that a packet may be using multiple mbufs.
+ * @return
+ *   Failure: < 0
+ *     -ENODEV: Invalid interface
+ *     -ENOTSUP: Driver does not support function
+ *   Success: >= 0
+ *     0-n: Number of packets freed. More packets may still remain in ring that
+ *     are in use.
+ */
+
+static inline int
+rte_eth_tx_done_cleanup(uint8_t port_id, uint16_t queue_id,  uint32_t free_cnt)
+{
+	struct rte_eth_dev *dev = &rte_eth_devices[port_id];
+
+	/* Validate Input Data. Bail if not valid or not supported. */
+	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
+	RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->tx_done_cleanup, -ENOTSUP);
+
+	/* Call driver to free pending mbufs. */
+	return (*dev->dev_ops->tx_done_cleanup)(dev->data->tx_queues[queue_id],
+			free_cnt);
+}
+
+/**
  * Configure a callback for buffered packets which cannot be sent
  *
  * Register a specific callback to be called when an attempt is made to send
Signature · 1 line
2.9.3
#2 lore raw

Re: [PATCH v2 1/3] ethdev: new API to free consumed buffers in Tx ring

Quoted 24 lines · On Wed, 11 Jan 2017 15:03:21 -0500 Billy McFall <[email protected]> wrote:

/**
+ * Request the driver to free mbufs currently cached by the driver. The
+ * driver will only free the mbuf if it is no longer in use. It is the
+ * application's responsibity to ensure rte_eth_tx_buffer_flush(..) is
+ * called if needed.
+ *
+ * @param port_id
+ * The port identifier of the Ethernet device.
+ * @param queue_id

+ *   The index of the transmit queue through which output packets must be
+ *   sent.
+ *   The value must be in the range [0, nb_tx_queue - 1] previously supplied
+ *   to rte_eth_dev_configure().

+ * @param free_cnt
+ * Maximum number of packets to free. Use 0 to indicate all possible packets
+ * should be freed. Note that a packet may be using multiple mbufs.
+ * @return
+ * Failure: < 0
+ * -ENODEV: Invalid interface
+ * -ENOTSUP: Driver does not support function
+ * Success: >= 0
+ * 0-n: Number of packets freed. More packets may still remain in ring that
+ * are in use.
+ */

lib/librte_ether/rte_ethdev.h rte_eth_tx_buffer(uint8_t port_id, uint16_t queue_id, · line 2975

Quoted 3 lines
lib/librte_ether/rte_ethdev.h +3 -0
+
+static inline int
+rte_eth_tx_done_cleanup(uint8_t port_id, uint16_t queue_id,  uint32_t free_cnt)

Extra white space.

lib/librte_ether/rte_ethdev.h rte_eth_tx_buffer(uint8_t port_id, uint16_t queue_id, · line 2987

Quoted 12 lines
lib/librte_ether/rte_ethdev.h +12 -0
+{
+	struct rte_eth_dev *dev = &rte_eth_devices[port_id];
+
+	/* Validate Input Data. Bail if not valid or not supported. */
+	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
+	RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->tx_done_cleanup, -ENOTSUP);
+
+	/* Call driver to free pending mbufs. */
+	return (*dev->dev_ops->tx_done_cleanup)(dev->data->tx_queues[queue_id],
+			free_cnt);
+}
+

It doesn't look like this is something that needs to be in critical path. Therefore it makes no sense to inline it.

#3 lore raw

PATCH v2 2/3 net/e1000: e1000 igb support to free consumed buffers

In reply to Billy McFall

Add support to the e1000 igb driver for the new API to force free consumed buffers on Tx ring. e1000 igb driver does not implement a tx_rs_thresh to free mbufs, it frees a slot in the ring as needed, so a new function needed to be written.

---
 drivers/net/e1000/e1000_ethdev.h |   2 +
 drivers/net/e1000/igb_ethdev.c   |   1 +
 drivers/net/e1000/igb_rxtx.c     | 126 +++++++++++++++++++++++++++++++++++++++
 3 files changed, 129 insertions(+)
drivers/net/e1000/e1000_ethdev.h +2 -0
index 6c25c8d..fce0278 100644
@@ -308,6 +308,8 @@ int eth_igb_tx_queue_setup(struct rte_eth_dev *dev, uint16_t tx_queue_id,
 		uint16_t nb_tx_desc, unsigned int socket_id,
 		const struct rte_eth_txconf *tx_conf);
 
+int eth_igb_tx_done_cleanup(void *txq, uint32_t free_cnt);
+
 int eth_igb_rx_init(struct rte_eth_dev *dev);
 
 void eth_igb_tx_init(struct rte_eth_dev *dev);
drivers/net/e1000/igb_ethdev.c +1 -0
index 2fddf0c..4010dc4 100644
@@ -402,6 +402,7 @@ static const struct eth_dev_ops eth_igb_ops = {
 	.rx_descriptor_done   = eth_igb_rx_descriptor_done,
 	.tx_queue_setup       = eth_igb_tx_queue_setup,
 	.tx_queue_release     = eth_igb_tx_queue_release,
+	.tx_done_cleanup      = eth_igb_tx_done_cleanup,
 	.dev_led_on           = eth_igb_led_on,
 	.dev_led_off          = eth_igb_led_off,
 	.flow_ctrl_get        = eth_igb_flow_ctrl_get,
drivers/net/e1000/igb_rxtx.c +126 -0

132 lines not on this page · raw message

Signature · 1 line
2.9.3
#4 lore raw

PATCH v2 3/3 net/vhost: vHost support to free consumed buffers

In reply to Billy McFall

Add support to the vHostdriver for the new API to force free consumed buffers on Tx ring. vHost does not cache the mbufs so there is no work to do.

---
 drivers/net/vhost/rte_eth_vhost.c | 11 +++++++++++
 1 file changed, 11 insertions(+)
drivers/net/vhost/rte_eth_vhost.c +11 -0
index 766d4ef..6493d56 100644
@@ -939,6 +939,16 @@ eth_queue_release(void *q)
 }
 
 static int
+eth_tx_done_cleanup(void *txq __rte_unused, uint32_t free_cnt __rte_unused)
+{
+	/*
+	 * vHost does not hang onto mbuf. eth_vhost_tx() copies packet data
+	 * and releases mbuf, so nothing to cleanup.
+	 */
+	return 0;
+}
+
+static int
 eth_link_update(struct rte_eth_dev *dev __rte_unused,
 		int wait_to_complete __rte_unused)
 {
@@ -979,6 +989,7 @@ static const struct eth_dev_ops ops = {
 	.tx_queue_setup = eth_tx_queue_setup,
 	.rx_queue_release = eth_queue_release,
 	.tx_queue_release = eth_queue_release,
+	.tx_done_cleanup = eth_tx_done_cleanup,
 	.link_update = eth_link_update,
 	.stats_get = eth_stats_get,
 	.stats_reset = eth_stats_reset,
Signature · 1 line
2.9.3
#5 lore raw

Re: [PATCH v2 3/3] net/vhost: vHost support to free consumed buffers

Quoted 31 lines · On Wed, 11 Jan 2017 15:03:23 -0500 Billy McFall <[email protected]> wrote:

Add support to the vHostdriver for the new API to force free consumed buffers on Tx ring. vHost does not cache the mbufs so there is no work to do.

---
 drivers/net/vhost/rte_eth_vhost.c | 11 +++++++++++
 1 file changed, 11 insertions(+)
drivers/net/vhost/rte_eth_vhost.c +10 -0
index 766d4ef..6493d56 100644
@@ -939,6 +939,16 @@ eth_queue_release(void *q)
 }
 
 static int
+eth_tx_done_cleanup(void *txq __rte_unused, uint32_t free_cnt __rte_unused)
+{
+	/*
+	 * vHost does not hang onto mbuf. eth_vhost_tx() copies packet data
+	 * and releases mbuf, so nothing to cleanup.
+	 */
+	return 0;
+}
+
+static int
 eth_link_update(struct rte_eth_dev *dev __rte_unused,
 		int wait_to_complete __rte_unused)
 {

drivers/net/vhost/rte_eth_vhost.c static const struct eth_dev_ops ops = { · line 995

Quoted 7 lines
drivers/net/vhost/rte_eth_vhost.c +1 -0
@@ -979,6 +989,7 @@ static const struct eth_dev_ops ops = {
 	.tx_queue_setup = eth_tx_queue_setup,
 	.rx_queue_release = eth_queue_release,
 	.tx_queue_release = eth_queue_release,
+	.tx_done_cleanup = eth_tx_done_cleanup,
 	.link_update = eth_link_update,
 	.stats_get = eth_stats_get,
 	.stats_reset = eth_stats_reset,

Rather than change drivers, since this is not critical path, make it optional to have tx_done_cleanup.

#6 lore raw

Re: [PATCH v2 3/3] net/vhost: vHost support to free consumed buffers

rte_eth_tx_done_cleanup(..) checks to see if .tx_done_cleanup is set, so in that since it is optional. The reason for this function does no work and returns 0 is to indicate to the application calling it that the mbufs are free and the application can continue. The application needs to know if the driver is attempting to the free the mbufs (return value >= 0) or not implemented (return value < 0).

If return value is < 0, the application needs to take alternative action, like make a copy of the packet in the flooding case, because the mbuf is not going to be returned anytime soon. If return value is >= 0, keep polling until the desired mbuf is returned (reference count decremented).

Let me know if this doesn't answer the question or I am missing your point.

Thanks,
Billy McFall

Quoted 47 lines · Stephen Hemminger

Review

Quoted 10 lines · On Wed, 11 Jan 2017 15:03:23 -0500 Billy McFall <[email protected]> wrote:

Add support to the vHostdriver for the new API to force free consumed buffers on Tx ring. vHost does not cache the mbufs so there is no work to do.

---
 drivers/net/vhost/rte_eth_vhost.c | 11 +++++++++++
 1 file changed, 11 insertions(+)
(quoted diff) +0 -0

b/drivers/net/vhost/rte_eth_vhost.c

drivers/net/vhost/rte_eth_vhost.c eth_queue_release(void *q) · line 942

Quoted 8 lines

index 766d4ef..6493d56 100644

drivers/net/vhost/rte_eth_vhost.c +1 -0
@@ -939,6 +939,16 @@ eth_queue_release(void *q)
 }
 
 static int
+eth_tx_done_cleanup(void *txq __rte_unused, uint32_t free_cnt

__rte_unused)

Quoted 13 lines
drivers/net/vhost/rte_eth_vhost.c +9 -0
+{
+     /*
+      * vHost does not hang onto mbuf. eth_vhost_tx() copies packet data
+      * and releases mbuf, so nothing to cleanup.
+      */
+     return 0;
+}
+
+static int
 eth_link_update(struct rte_eth_dev *dev __rte_unused,
              int wait_to_complete __rte_unused)
 {

drivers/net/vhost/rte_eth_vhost.c static const struct eth_dev_ops ops = { · line 995

Quoted 7 lines
drivers/net/vhost/rte_eth_vhost.c +1 -0
@@ -979,6 +989,7 @@ static const struct eth_dev_ops ops = {
      .tx_queue_setup = eth_tx_queue_setup,
      .rx_queue_release = eth_queue_release,
      .tx_queue_release = eth_queue_release,
+     .tx_done_cleanup = eth_tx_done_cleanup,
      .link_update = eth_link_update,
      .stats_get = eth_stats_get,
      .stats_reset = eth_stats_reset,

Rather than change drivers, since this is not critical path, make it optional to have tx_done_cleanup.