new API to free consumed buffers in Tx ring

-----Original Message-----
From: Billy McFall [mailto:[email protected]]
Sent: Saturday, January 21, 2017 12:01 AM
To: [email protected]; Lu, Wenzhuo
Cc: [email protected]; Billy McFall
Subject: [PATCH v3 2/3] net/e1000: e1000 igb support to free consumed buffers

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.

Signed-off-by Billy McFall <[email protected]>

---
 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 81a6dbb..39b2f43 100644
@@ -315,6 +315,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); diff --git
a/drivers/net/e1000/igb_ethdev.c b/drivers/net/e1000/igb_ethdev.c index 8843dd1..12a1a30 100644

drivers/net/e1000/igb_ethdev.c +1 -0

@@ -408,6 +408,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 index renamed from drivers/net/e1000/igb_rxtx.c +0 -0

45f3f24..00cd2aa 100644

drivers/net/e1000/igb_rxtx.c +19 -0

@@ -1277,6 +1277,132 @@ eth_igb_tx_queue_release(void *txq)
 	igb_tx_queue_release(txq);
 }
 
+static int
+igb_tx_done_cleanup(struct igb_tx_queue *txq, uint32_t free_cnt) {
+	struct igb_tx_entry *sw_ring;
+	volatile union e1000_adv_tx_desc *txr;
+	uint16_t tx_first; /* First segment analyzed. */
+	uint16_t tx_id;    /* Current segment being processed. */
+	uint16_t tx_last;  /* Last segment in the current packet. */
+	uint16_t tx_next;  /* First segment of the next packet. */
+	int count;
+
+	if (txq != NULL) {
+		count = 0;
+		sw_ring = txq->sw_ring;
+		txr = txq->tx_ring;
+
+		/*
+		 * tx_tail is the last sent packet on the sw_ring. Goto the end
+		 * of that packet (the last segment in the packet chain) and
+		 * then the next segment will be the start of the oldest

segment

drivers/net/e1000/igb_rxtx.c +8 -0

+		 * in the sw_ring. This is the first packet that will be
+		 * attempted to be freed.
+		 */
+
+		/* Get last segment in most recently added packet. */
+		tx_first = sw_ring[txq->tx_tail].last_id;
+
+		/* Get the next segment, which is the oldest segment in ring.

*/

drivers/net/e1000/igb_rxtx.c +31 -0

+		tx_first = sw_ring[tx_first].next_id;
+
+		/* Set the current index to the first. */
+		tx_id = tx_first;
+
+		/*
+		 * Loop through each packet. For each packet, verify that an
+		 * mbuf exists and that the last segment is free. If so, free
+		 * it and move on.
+		 */
+		while (1) {
+			tx_last = sw_ring[tx_id].last_id;
+
+			if (sw_ring[tx_last].mbuf) {
+				if (txr[tx_last].wb.status &
+						E1000_TXD_STAT_DD) {
+					/*
+					 * Increment the number of packets
+					 * freed.
+					 */
+					count++;
+
+					/* Get the start of the next packet. */
+					tx_next = sw_ring[tx_last].next_id;
+
+					/*
+					 * Loop through all segments in a
+					 * packet.
+					 */
+					do {
+

	rte_pktmbuf_free_seg(sw_ring[tx_id].mbuf);

drivers/net/e1000/igb_rxtx.c +49 -0

+						sw_ring[tx_id].mbuf = NULL;
+						sw_ring[tx_id].last_id = tx_id;
+
+						/* Move to next segemnt. */
+						tx_id = sw_ring[tx_id].next_id;
+
+					} while (tx_id != tx_next);
+
+					if (unlikely(count == (int)free_cnt))
+						break;
+				} else
+					/*
+					 * mbuf still in use, nothing left to
+					 * free.
+					 */
+					break;
+			} else {
+				/*
+				 * There are multiple reasons to be here:
+				 * 1) All the packets on the ring have been
+				 *    freed - tx_id is equal to tx_first
+				 *    and some packets have been freed.
+				 *    - Done, exit
+				 * 2) Interfaces has not sent a rings worth of
+				 *    packets yet, so the segment after tail is
+				 *    still empty. Or a previous call to this
+				 *    function freed some of the segments but
+				 *    not all so there is a hole in the list.
+				 *    Hopefully this is a rare case.
+				 *    - Walk the list and find the next mbuf. If
+				 *      there isn't one, then done.
+				 */
+				if (likely((tx_id == tx_first) && (count != 0)))
+					break;
+
+				/*
+				 * Walk the list and find the next mbuf, if any.
+				 */
+				do {
+					/* Move to next segemnt. */
+					tx_id = sw_ring[tx_id].next_id;
+
+					if (sw_ring[tx_id].mbuf)
+						break;
+
+				} while (tx_id != tx_first);
+
+				/*
+				 * Determine why previous loop bailed. If

there

drivers/net/e1000/igb_rxtx.c +16 -1

+				 * is not an mbuf, done.
+				 */
+				if (sw_ring[tx_id].mbuf == NULL)
+					break;
+			}
+		}
+	} else
+		count = -ENODEV;
+
+	return count;
+}
+
+int
+eth_igb_tx_done_cleanup(void *txq, uint32_t free_cnt) {
+	return igb_tx_done_cleanup(txq, free_cnt); }
+
 static void
 igb_reset_tx_queue_stat(struct igb_tx_queue *txq)  {
--

2.9.3

Hi Billy,

Quoted 9 lines

-----Original Message-----
From: Billy McFall [mailto:[email protected]]
Sent: Saturday, January 21, 2017 12:01 AM
To: [email protected]; Lu, Wenzhuo
Cc: [email protected]; Billy McFall
Subject: [PATCH v3 2/3] net/e1000: e1000 igb support to free consumed buffers

Add support to the e1000 igb driver for the new API to force free

consumed

Quoted 1 line

buffers on Tx ring. e1000 igb driver does not implement a tx_rs_thresh

to free

Quoted 1 line

mbufs, it frees a slot in the ring as needed, so a new function needed

to be

drivers/net/e1000/igb_rxtx.c

Quoted 38 lines

written.

Signed-off-by Billy McFall <[email protected]>

---
 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 81a6dbb..39b2f43 100644
@@ -315,6 +315,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); diff --git
a/drivers/net/e1000/igb_ethdev.c b/drivers/net/e1000/igb_ethdev.c index 8843dd1..12a1a30 100644

drivers/net/e1000/igb_ethdev.c +1 -0

@@ -408,6 +408,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 +0 -0

index

Quoted 14 lines

45f3f24..00cd2aa 100644

drivers/net/e1000/igb_rxtx.c +7 -0

@@ -1277,6 +1277,132 @@ eth_igb_tx_queue_release(void *txq)
      igb_tx_queue_release(txq);
 }
 
+static int
+igb_tx_done_cleanup(struct igb_tx_queue *txq, uint32_t free_cnt) {
+     struct igb_tx_entry *sw_ring;
+     volatile union e1000_adv_tx_desc *txr;
+     uint16_t tx_first; /* First segment analyzed. */
+     uint16_t tx_id;    /* Current segment being processed. */
+     uint16_t tx_last;  /* Last segment in the current packet. */

drivers/net/e1000/igb_rxtx.c eth_igb_tx_queue_release(void *txq) · line 1296

Quoted 10 lines

drivers/net/e1000/igb_rxtx.c +10 -0

+     uint16_t tx_next;  /* First segment of the next packet. */
+     int count;
+
+     if (txq != NULL) {
+             count = 0;
+             sw_ring = txq->sw_ring;
+             txr = txq->tx_ring;
+
+             /*
+              * tx_tail is the last sent packet on the sw_ring. Goto

the end

drivers/net/e1000/igb_rxtx.c eth_igb_tx_queue_release(void *txq) · line 1297

Quoted 1 line

drivers/net/e1000/igb_rxtx.c +1 -0

+              * of that packet (the last segment in the packet chain)

and

drivers/net/e1000/igb_rxtx.c eth_igb_tx_queue_release(void *txq)

Quoted 10 lines

drivers/net/e1000/igb_rxtx.c +1 -0

+              * then the next segment will be the start of the oldest

segment

drivers/net/e1000/igb_rxtx.c +8 -0

+              * in the sw_ring. This is the first packet that will be
+              * attempted to be freed.
+              */
+
+             /* Get last segment in most recently added packet. */
+             tx_first = sw_ring[txq->tx_tail].last_id;
+
+             /* Get the next segment, which is the oldest segment in

ring.

drivers/net/e1000/igb_rxtx.c eth_igb_tx_queue_release(void *txq)

Quoted 8 lines

*/

drivers/net/e1000/igb_rxtx.c +7 -0

+             tx_first = sw_ring[tx_first].next_id;
+
+             /* Set the current index to the first. */
+             tx_id = tx_first;
+
+             /*
+              * Loop through each packet. For each packet, verify that

an

Quoted 1 line

+ * mbuf exists and that the last segment is free. If so,

free

Quoted 5 lines

drivers/net/e1000/igb_rxtx.c +5 -0

+              * it and move on.
+              */
+             while (1) {
+                     tx_last = sw_ring[tx_id].last_id;
+

drivers/net/e1000/igb_rxtx.c eth_igb_tx_queue_release(void *txq)

Quoted 10 lines

drivers/net/e1000/igb_rxtx.c +10 -0

+                     if (sw_ring[tx_last].mbuf) {
+                             if (txr[tx_last].wb.status &
+                                             E1000_TXD_STAT_DD) {
+                                     /*
+                                      * Increment the number of packets
+                                      * freed.
+                                      */
+                                     count++;
+
+                                     /* Get the start of the next

packet. */

drivers/net/e1000/igb_rxtx.c eth_igb_tx_queue_release(void *txq)

Quoted 11 lines

drivers/net/e1000/igb_rxtx.c +10 -0

+                                     tx_next = sw_ring[tx_last].next_id;
+
+                                     /*
+                                      * Loop through all segments in a
+                                      * packet.
+                                      */
+                                     do {
+
      rte_pktmbuf_free_seg(sw_ring[tx_id].mbuf);
+                                             sw_ring[tx_id].mbuf = NULL;
+                                             sw_ring[tx_id].last_id =

tx_id;

drivers/net/e1000/igb_rxtx.c eth_igb_tx_queue_release(void *txq)

Quoted 3 lines

drivers/net/e1000/igb_rxtx.c +3 -0

+
+                                             /* Move to next segemnt. */
+                                             tx_id =

sw_ring[tx_id].next_id;

drivers/net/e1000/igb_rxtx.c eth_igb_tx_queue_release(void *txq)

Quoted 4 lines

drivers/net/e1000/igb_rxtx.c +4 -0

+
+                                     } while (tx_id != tx_next);
+
+                                     if (unlikely(count ==

(int)free_cnt))

drivers/net/e1000/igb_rxtx.c eth_igb_tx_queue_release(void *txq)

Quoted 4 lines

drivers/net/e1000/igb_rxtx.c +4 -0

+                                             break;
+                             } else
+                                     /*
+                                      * mbuf still in use, nothing left

to

drivers/net/e1000/igb_rxtx.c eth_igb_tx_queue_release(void *txq)

Quoted 11 lines

drivers/net/e1000/igb_rxtx.c +11 -0

+                                      * free.
+                                      */
+                                     break;
+                     } else {
+                             /*
+                              * There are multiple reasons to be here:
+                              * 1) All the packets on the ring have been
+                              *    freed - tx_id is equal to tx_first
+                              *    and some packets have been freed.
+                              *    - Done, exit
+                              * 2) Interfaces has not sent a rings

worth of

Quoted 1 line

+ * packets yet, so the segment after

tail is

Quoted 1 line

+ * still empty. Or a previous call to

this

Quoted 1 line

+ * function freed some of the segments

but

Quoted 1 line

+ * not all so there is a hole in the

list.

drivers/net/e1000/igb_rxtx.c

Quoted 2 lines

drivers/net/e1000/igb_rxtx.c +2 -0

+                              *    Hopefully this is a rare case.
+                              *    - Walk the list and find the next

mbuf. If

drivers/net/e1000/igb_rxtx.c eth_igb_tx_queue_release(void *txq)

Quoted 3 lines

drivers/net/e1000/igb_rxtx.c +3 -0

+                              *      there isn't one, then done.
+                              */
+                             if (likely((tx_id == tx_first) && (count

!= 0)))

drivers/net/e1000/igb_rxtx.c eth_igb_tx_queue_release(void *txq)

Quoted 4 lines

drivers/net/e1000/igb_rxtx.c +4 -0

+                                     break;
+
+                             /*
+                              * Walk the list and find the next mbuf,

if any.

Quoted 32 lines

drivers/net/e1000/igb_rxtx.c +12 -0

+                              */
+                             do {
+                                     /* Move to next segemnt. */
+                                     tx_id = sw_ring[tx_id].next_id;
+
+                                     if (sw_ring[tx_id].mbuf)
+                                             break;
+
+                             } while (tx_id != tx_first);
+
+                             /*
+                              * Determine why previous loop bailed. If

there

drivers/net/e1000/igb_rxtx.c +16 -1

+                              * is not an mbuf, done.
+                              */
+                             if (sw_ring[tx_id].mbuf == NULL)
+                                     break;
+                     }
+             }
+     } else
+             count = -ENODEV;
+
+     return count;
+}
+
+int
+eth_igb_tx_done_cleanup(void *txq, uint32_t free_cnt) {
+     return igb_tx_done_cleanup(txq, free_cnt); }
+
 static void
 igb_reset_tx_queue_stat(struct igb_tx_queue *txq)  {
--

Quoted 1 line

2.9.3

I think the code is good. But I don't understand the whole idea. An API means APP need call it. When should APP call This API? Does APP need to check after sending every packet? Or just call it periodically?

-----Original Message-----
From: Billy McFall [mailto:[email protected]<mailto:[email protected]>]
Sent: Saturday, January 21, 2017 12:01 AM
To: [email protected]<mailto:[email protected]>; Lu, Wenzhuo
Cc: [email protected]<mailto:[email protected]>; Billy McFall
Subject: [PATCH v3 2/3] net/e1000: e1000 igb support to free consumed buffers

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.

Signed-off-by Billy McFall <[email protected]<mailto:[email protected]>>

---
 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 81a6dbb..39b2f43 100644
@@ -315,6 +315,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); diff --git
a/drivers/net/e1000/igb_ethdev.c b/drivers/net/e1000/igb_ethdev.c index 8843dd1..12a1a30 100644

drivers/net/e1000/igb_ethdev.c +1 -0

@@ -408,6 +408,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 index renamed from drivers/net/e1000/igb_rxtx.c +0 -0

45f3f24..00cd2aa 100644

drivers/net/e1000/igb_rxtx.c +19 -0

@@ -1277,6 +1277,132 @@ eth_igb_tx_queue_release(void *txq)
      igb_tx_queue_release(txq);
 }
 
+static int
+igb_tx_done_cleanup(struct igb_tx_queue *txq, uint32_t free_cnt) {
+     struct igb_tx_entry *sw_ring;
+     volatile union e1000_adv_tx_desc *txr;
+     uint16_t tx_first; /* First segment analyzed. */
+     uint16_t tx_id;    /* Current segment being processed. */
+     uint16_t tx_last;  /* Last segment in the current packet. */
+     uint16_t tx_next;  /* First segment of the next packet. */
+     int count;
+
+     if (txq != NULL) {
+             count = 0;
+             sw_ring = txq->sw_ring;
+             txr = txq->tx_ring;
+
+             /*
+              * tx_tail is the last sent packet on the sw_ring. Goto the end
+              * of that packet (the last segment in the packet chain) and
+              * then the next segment will be the start of the oldest

segment

drivers/net/e1000/igb_rxtx.c +8 -0

+              * in the sw_ring. This is the first packet that will be
+              * attempted to be freed.
+              */
+
+             /* Get last segment in most recently added packet. */
+             tx_first = sw_ring[txq->tx_tail].last_id;
+
+             /* Get the next segment, which is the oldest segment in ring.

*/

drivers/net/e1000/igb_rxtx.c +80 -0

+             tx_first = sw_ring[tx_first].next_id;
+
+             /* Set the current index to the first. */
+             tx_id = tx_first;
+
+             /*
+              * Loop through each packet. For each packet, verify that an
+              * mbuf exists and that the last segment is free. If so, free
+              * it and move on.
+              */
+             while (1) {
+                     tx_last = sw_ring[tx_id].last_id;
+
+                     if (sw_ring[tx_last].mbuf) {
+                             if (txr[tx_last].wb.status &
+                                             E1000_TXD_STAT_DD) {
+                                     /*
+                                      * Increment the number of packets
+                                      * freed.
+                                      */
+                                     count++;
+
+                                     /* Get the start of the next packet. */
+                                     tx_next = sw_ring[tx_last].next_id;
+
+                                     /*
+                                      * Loop through all segments in a
+                                      * packet.
+                                      */
+                                     do {
+
      rte_pktmbuf_free_seg(sw_ring[tx_id].mbuf);
+                                             sw_ring[tx_id].mbuf = NULL;
+                                             sw_ring[tx_id].last_id = tx_id;
+
+                                             /* Move to next segemnt. */
+                                             tx_id = sw_ring[tx_id].next_id;
+
+                                     } while (tx_id != tx_next);
+
+                                     if (unlikely(count == (int)free_cnt))
+                                             break;
+                             } else
+                                     /*
+                                      * mbuf still in use, nothing left to
+                                      * free.
+                                      */
+                                     break;
+                     } else {
+                             /*
+                              * There are multiple reasons to be here:
+                              * 1) All the packets on the ring have been
+                              *    freed - tx_id is equal to tx_first
+                              *    and some packets have been freed.
+                              *    - Done, exit
+                              * 2) Interfaces has not sent a rings worth of
+                              *    packets yet, so the segment after tail is
+                              *    still empty. Or a previous call to this
+                              *    function freed some of the segments but
+                              *    not all so there is a hole in the list.
+                              *    Hopefully this is a rare case.
+                              *    - Walk the list and find the next mbuf. If
+                              *      there isn't one, then done.
+                              */
+                             if (likely((tx_id == tx_first) && (count != 0)))
+                                     break;
+
+                             /*
+                              * Walk the list and find the next mbuf, if any.
+                              */
+                             do {
+                                     /* Move to next segemnt. */
+                                     tx_id = sw_ring[tx_id].next_id;
+
+                                     if (sw_ring[tx_id].mbuf)
+                                             break;
+
+                             } while (tx_id != tx_first);
+
+                             /*
+                              * Determine why previous loop bailed. If

there

drivers/net/e1000/igb_rxtx.c +16 -1

+                              * is not an mbuf, done.
+                              */
+                             if (sw_ring[tx_id].mbuf == NULL)
+                                     break;
+                     }
+             }
+     } else
+             count = -ENODEV;
+
+     return count;
+}
+
+int
+eth_igb_tx_done_cleanup(void *txq, uint32_t free_cnt) {
+     return igb_tx_done_cleanup(txq, free_cnt); }
+
 static void
 igb_reset_tx_queue_stat(struct igb_tx_queue *txq)  {
--

2.9.3

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.

2017-01-27 13:37, Billy McFall:

doc/guides/nics/features/default.ini FW version = · line 58

Quoted 7 lines

doc/guides/nics/features/default.ini +1 -0

@@ -55,6 +55,7 @@ FW version           =
 EEPROM dump          =
 Registers dump       =
 Multiprocess aware   =
+Free TX ring buffers =

I'm afraid this wording will be confusing, because every drivers free their buffers :)
What about "Free Tx mbuf on demand" ?

And please, move this line upper, just after "Rx interrupt".

Done

We also need to carefully review the doc you provided (thanks). First quick comment, please wrap lines shorter in the doc.

Done

About the function prototype, I've seen a double space :)

I think you could use rte_errno (while keeping negative return codes).

Quoted 2 lines · Thomas Monjalon

I think you could use rte_errno (while keeping negative return codes).

I can do that if you want, but if I understand your comment, it will make the implementation of the function not as clean. I cannot use the existing RTE_ETH_VALID_PORTID_OR_ERR_RET(..) and RTE_FUNC_PTR_OR_ERR_RET(..) MACROs because they are handling the return on error. Or am I missing something?

2017-03-07 09:29, Billy McFall:

Quoted 1 line

On Mon, Feb 27, 2017 at 8:48 AM, Thomas Monjalon <

[email protected]>

Quoted 6 lines

Quoted 2 lines · wrote:

I think you could use rte_errno (while keeping negative return codes).

I can do that if you want, but if I understand your comment, it will make the implementation of the function not as clean. I cannot use the

existing

Quoted 1 line

RTE_ETH_VALID_PORTID_OR_ERR_RET(..) and RTE_FUNC_PTR_OR_ERR_RET(..)

MACROs

Quoted 1 line

because they are handling the return on error. Or am I missing something?

Yes. Maybe we need new macros for basic error management with rte_errno.

Looking at the code. Do you think we need new MACROs or just set rte_errno

Quoted 9 lines · Thomas Monjalon

2017-03-07 09:29, Billy McFall:

Quoted 1 line

On Mon, Feb 27, 2017 at 8:48 AM, Thomas Monjalon <

[email protected]>

Quoted 6 lines

Quoted 2 lines · wrote:

I think you could use rte_errno (while keeping negative return codes).

I can do that if you want, but if I understand your comment, it will make the implementation of the function not as clean. I cannot use the

Quoted 8 lines

existing

Quoted 1 line

RTE_ETH_VALID_PORTID_OR_ERR_RET(..) and RTE_FUNC_PTR_OR_ERR_RET(..)

MACROs

Quoted 1 line

because they are handling the return on error. Or am I missing something?

Yes. Maybe we need new macros for basic error management with rte_errno.

Looking at the code. Do you think we need new MACROs or just set rte_errno

in the existing? What would be the down side to setting rte_errno for all the existing calls?

Looks like RTE_ETH_VALID_PORTID_OR_ERR_RET(..) is being called ~135 times. Most calls are with retval set to either -ENODEV or -EINVAL. A few instances of 0 and -1, but not many.

Looks like RTE_FUNC_PTR_OR_ERR_RET(..) is being called ~100 times. Most calls are with retval set to -ENOTSUP. A few instances of 0, but not many.

I was thinking:
/* Macros to check for valid port */
#define RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, retval) do { \
if (!rte_eth_dev_is_valid_port(port_id)) { \
RTE_PMD_DEBUG_TRACE("Invalid port_id=%d\n", port_id); \
+ if (retval < 0) \
+ rte_errno = -retval; \
return retval; \
} \
} while (0)

-----Original Message-----
From: dev [mailto:[email protected]] On Behalf Of Billy McFall
Sent: Tuesday, March 7, 2017 2:30 PM
To: Thomas Monjalon <[email protected]>
Cc: Lu, Wenzhuo <[email protected]>; [email protected]; [email protected]
Subject: Re: [dpdk-dev] [PATCH v5 1/3] ethdev: new API to free consumed buffers in Tx ring

Thomas,

Thanks for your comments. See inline.

Quoted 13 lines · Thomas Monjalon

2017-01-27 13:37, Billy McFall:

doc/guides/nics/features/default.ini FW version = · line 58

Quoted 7 lines

doc/guides/nics/features/default.ini +1 -0

@@ -55,6 +55,7 @@ FW version           =
 EEPROM dump          =
 Registers dump       =
 Multiprocess aware   =
+Free TX ring buffers =

I'm afraid this wording will be confusing, because every drivers free their buffers :) What about "Free Tx mbuf on demand" ?

I definitely like your wording of the feature better than mine. All the existing features were under 20 characters and I was trying to stay under that.

-----Original Message-----
From: dev [mailto:[email protected]] On Behalf Of Billy McFall
Sent: Friday, January 27, 2017 6:38 PM
To: [email protected]; Lu, Wenzhuo <[email protected]>; [email protected]
Cc: [email protected]; Billy McFall <[email protected]>
Subject: [dpdk-dev] [PATCH v5 1/3] ethdev: new API to free consumed buffers in Tx ring

+Driver Cache
+~~~~~~~~~~~~

+release the mbuf back to the mempool, or local cache, immediately after the packet has been transmitted.
+Instead, they leave the mbuf in their txRing and either perform a bulk

+release when the tx_rs_thresh has been crossed or free the mbuf when a

slot in the txRing is needed.

(quoted diff) +2 -0

+
+An application can request the driver to release used mbufs with the

``rte_eth_tx_done_cleanup()`` API.

(quoted diff) +3 -0

+This API requests the driver to release mbufs that are no longer in
+use, independent of whether or not the tx_rs_thresh has been crossed.
+There are two scenarios when an application may want the mbuf back

immediately:

(quoted diff) +2 -0

+
+* When a given packet needs to be sent to multiple destination interfaces

(either for Layer 2 flooding or Layer 3 multi-cast).
+ One option is to make a copy of the packet or a copy of the header portion that needs to manipulated.

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.

Signed-off-by Billy McFall <[email protected]>

---
 doc/guides/nics/features/e1000.ini |   1 +
 drivers/net/e1000/e1000_ethdev.h   |   2 +
 drivers/net/e1000/igb_ethdev.c     |   1 +
 drivers/net/e1000/igb_rxtx.c       | 126 +++++++++++++++++++++++++++++++++++++
 4 files changed, 130 insertions(+)

-----Original Message-----
From: Billy McFall [mailto:[email protected]]
Sent: Saturday, January 28, 2017 2:38 AM
To: [email protected]; Lu, Wenzhuo; [email protected]
Cc: [email protected]; Billy McFall
Subject: [PATCH v5 2/3] net/e1000: e1000 igb support to free consumed buffers

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.

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.

Signed-off-by Billy McFall <[email protected]>

2017-01-27 13:38, Billy McFall:

Quoted 5 lines

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.

Signed-off-by Billy McFall <[email protected]>

Yuanhan, Maxime, do you agree with this empty function?

Quoted 8 lines · Thomas Monjalon

2017-01-27 13:38, Billy McFall:

Quoted 5 lines

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.

Signed-off-by Billy McFall <[email protected]>

Yuanhan, Maxime, do you agree with this empty function?

For this particular (vhost) implementation, I think it's okay.

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.

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

Please double check my python coding in conf.py and make sure I implemented 'feature_str_len' properly.

Signed-off-by Billy McFall <[email protected]>

doc/guides/prog_guide/mempool_lib.rst +14 -0

index ffdc109..a117881 100644
@@ -132,6 +132,32 @@ These user-owned caches can be explicitly passed to ``rte_mempool_generic_put()`
 The ``rte_mempool_default_cache()`` call returns the default internal cache if any.
 In contrast to the default caches, user-owned caches can be used by non-EAL threads too.
 
+In addition to a core's local cache, many of the drivers don't release the mbuf back to the mempool, or local cache,
+immediately after the packet has been transmitted.
+Instead, they leave the mbuf in their Tx ring and either perform a bulk release when the ``tx_rs_thresh`` has been
+crossed or free the mbuf when a slot in the Tx ring is needed.
+
+An application can request the driver to release used mbufs with the ``rte_eth_tx_done_cleanup()`` API.
+This API requests the driver to release mbufs that are no longer in use, independent of whether or not the
+``tx_rs_thresh`` has been crossed.
+There are two scenarios when an application may want the mbuf released immediately:
+
+* When a given packet needs to be sent to multiple destination interfaces (either for Layer 2 flooding or Layer 3
+  multi-cast).
+  One option is to make a copy of the packet or a copy of the header portion that needs to be manipulated.
+  A second option is to transmit the packet and then poll the ``rte_eth_tx_done_cleanup()`` API until the reference

Hi Billy,

Quoted 8 lines · Billy McFall

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

Please double check my python coding in conf.py and make sure I implemented 'feature_str_len' properly.

Signed-off-by Billy McFall <[email protected]>

[...]

Review

Quoted 1 line

(quoted diff) +0 -0

b/doc/guides/prog_guide/mempool_lib.rst

doc/guides/prog_guide/mempool_lib.rst These user-owned caches can be explicitly passed to

Quoted 4 lines

index ffdc109..a117881 100644

doc/guides/prog_guide/mempool_lib.rst +0 -0

@@ -132,6 +132,32 @@ These user-owned caches can be explicitly passed to

``rte_mempool_generic_put()`

doc/guides/prog_guide/mempool_lib.rst These user-owned caches can be explicitly passed to · line 132

Quoted 1 line

doc/guides/prog_guide/mempool_lib.rst +0 -0

 The ``rte_mempool_default_cache()`` call returns the default internal

cache if any.

doc/guides/prog_guide/mempool_lib.rst These user-owned caches can be explicitly passed to · line 133

Quoted 1 line

doc/guides/prog_guide/mempool_lib.rst +0 -0

 In contrast to the default caches, user-owned caches can be used by

non-EAL threads too.

doc/guides/prog_guide/mempool_lib.rst These user-owned caches can be explicitly passed to · line 134

Quoted 2 lines

doc/guides/prog_guide/mempool_lib.rst +1 -0

+In addition to a core's local cache, many of the drivers don't release

the mbuf back to the mempool, or local cache,

doc/guides/prog_guide/mempool_lib.rst These user-owned caches can be explicitly passed to · line 136

Quoted 2 lines

doc/guides/prog_guide/mempool_lib.rst +2 -0

+immediately after the packet has been transmitted.
+Instead, they leave the mbuf in their Tx ring and either perform a bulk

release when the ``tx_rs_thresh`` has been

doc/guides/prog_guide/mempool_lib.rst These user-owned caches can be explicitly passed to ``rte_mempool_generic_put()`

Quoted 3 lines

doc/guides/prog_guide/mempool_lib.rst +3 -0

+crossed or free the mbuf when a slot in the Tx ring is needed.
+
+An application can request the driver to release used mbufs with the

``rte_eth_tx_done_cleanup()`` API.

doc/guides/prog_guide/mempool_lib.rst These user-owned caches can be explicitly passed to · line 140

Quoted 1 line

doc/guides/prog_guide/mempool_lib.rst +1 -0

+This API requests the driver to release mbufs that are no longer in

use, independent of whether or not the

doc/guides/prog_guide/mempool_lib.rst These user-owned caches can be explicitly passed to · line 142

Quoted 2 lines

doc/guides/prog_guide/mempool_lib.rst +2 -0

+``tx_rs_thresh`` has been crossed.
+There are two scenarios when an application may want the mbuf released

immediately:

doc/guides/prog_guide/mempool_lib.rst These user-owned caches can be explicitly passed to · line 144

Quoted 2 lines

doc/guides/prog_guide/mempool_lib.rst +2 -0

+
+* When a given packet needs to be sent to multiple destination

interfaces (either for Layer 2 flooding or Layer 3

doc/guides/prog_guide/mempool_lib.rst These user-owned caches can be explicitly passed to · line 146

Quoted 2 lines

doc/guides/prog_guide/mempool_lib.rst +2 -0

+  multi-cast).
+  One option is to make a copy of the packet or a copy of the header

portion that needs to be manipulated.

doc/guides/prog_guide/mempool_lib.rst These user-owned caches can be explicitly passed to · line 147

Quoted 1 line

doc/guides/prog_guide/mempool_lib.rst +1 -0

+  A second option is to transmit the packet and then poll the

``rte_eth_tx_done_cleanup()`` API until the reference

doc/guides/prog_guide/mempool_lib.rst These user-owned caches can be explicitly passed to · line 149

Quoted 2 lines

doc/guides/prog_guide/mempool_lib.rst +2 -0

+  count on the packet is decremented.
+  Then the same packet can be transmitted to the next destination

interface.

doc/guides/prog_guide/mempool_lib.rst These user-owned caches can be explicitly passed to · line 151

Quoted 2 lines

doc/guides/prog_guide/mempool_lib.rst +2 -0

+
+* If an application is designed to make multiple runs, like a packet

generator, and one run has completed.

doc/guides/prog_guide/mempool_lib.rst These user-owned caches can be explicitly passed to · line 153

Quoted 2 lines

doc/guides/prog_guide/mempool_lib.rst +2 -0

+  The application may want to reset to a clean state.
+  In this case, it may want to call the ``rte_eth_tx_done_cleanup()``

API to request each destination interface it has

doc/guides/prog_guide/mempool_lib.rst These user-owned caches can be explicitly passed to ``rte_mempool_generic_put()`

Quoted 3 lines

doc/guides/prog_guide/mempool_lib.rst +3 -0

+  been using to release all of its used mbufs.
+
+To determine if a driver supports this API, check for the *Free Tx mbuf

on demand* feature in the *Network Interface

doc/guides/prog_guide/mempool_lib.rst These user-owned caches can be explicitly passed to ``rte_mempool_generic_put()` · line 158

Quoted 1 line

doc/guides/prog_guide/mempool_lib.rst +1 -0

+Controller Drivers* document.

I'm not sure the mempool documentation is the proper place to document this API. I'll suggest to move it in ethdev documentation (poll_mode_drv.rst).

You can keep a small paragraph in mempool lib, but keep in mind that mempool is not necessarily a pool of mbuf (it can be any kind of object).

Not sure this API needs to be documented in multiple places. I'll move the

Thanks,
Olivier

Billy

lib/librte_ether/rte_ethdev.h +20 -0

@@ -3097,6 +3101,33 @@ 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

-----Original Message-----
From: Billy McFall [mailto:[email protected]]
Sent: Friday, March 10, 2017 4:51 AM
To: [email protected]; Lu, Wenzhuo; [email protected]
Cc: [email protected]; Billy McFall
Subject: [PATCH v6 2/3] net/e1000: e1000 igb support to free consumed buffers

Add support to the e1000 igb driver for the new API to force free consumed buffers on Tx ring. This API is independent of the tx_rs_thresh setting. With this API, buffers should be free even if tx_rs_thresh is not met.

e1000 igb driver does not implement a tx_rs_thresh to free mbufs, it frees a slot in the ring as needed. However, it could be implemented at some future date.

Signed-off-by Billy McFall <[email protected]>

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.

Signed-off-by Billy McFall <[email protected]>

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

Signed-off-by Billy McFall <[email protected]>

---
 doc/guides/conf.py                      |  7 +++++--
 doc/guides/nics/features/default.ini    |  4 +++-
 doc/guides/prog_guide/poll_mode_drv.rst | 28 ++++++++++++++++++++++++++++
 doc/guides/rel_notes/release_17_05.rst  |  7 ++++++-
 lib/librte_ether/rte_ethdev.c           | 14 ++++++++++++++
 lib/librte_ether/rte_ethdev.h           | 31 +++++++++++++++++++++++++++++++
 6 files changed, 87 insertions(+), 4 deletions(-)

doc/guides/prog_guide/poll_mode_drv.rst +9 -0

@@ -249,6 +249,34 @@ One descriptor in the TX ring is used as a sentinel to avoid a hardware race con
 
     When configuring for DCB operation, at port initialization, both the number of transmit queues and the number of receive queues must be set to 128.
 
+Free Tx mbuf on Demand
+~~~~~~~~~~~~~~~~~~~~~~
+
+Many of the drivers don't release the mbuf back to the mempool, or local cache, immediately after the packet has been
+transmitted.
+Instead, they leave the mbuf in their Tx ring and either perform a bulk release when the ``tx_rs_thresh`` has been
+crossed or free the mbuf when a slot in the Tx ring is needed.
+
+An application can request the driver to release used mbufs with the ``rte_eth_tx_done_cleanup()`` API.

Hi Billy,

Quoted 9 lines · Billy McFall

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

Signed-off-by Billy McFall <[email protected]>

---
 doc/guides/conf.py                      |  7 +++++--
 doc/guides/nics/features/default.ini    |  4 +++-
 doc/guides/prog_guide/poll_mode_drv.rst | 28

++++++++++++++++++++++++++++

Quoted 3 lines

 doc/guides/rel_notes/release_17_05.rst  |  7 ++++++-
 lib/librte_ether/rte_ethdev.c           | 14 ++++++++++++++
 lib/librte_ether/rte_ethdev.h           | 31

+++++++++++++++++++++++++++++++

Quoted 2 lines

 6 files changed, 87 insertions(+), 4 deletions(-)

[...]

doc/guides/prog_guide/poll_mode_drv.rst One descriptor in the TX ring is used as a sentinel

Quoted 3 lines

doc/guides/prog_guide/poll_mode_drv.rst +0 -0

@@ -249,6 +249,34 @@ One descriptor in the TX ring is used as a sentinel

to avoid a hardware race con

doc/guides/prog_guide/poll_mode_drv.rst One descriptor in the TX ring is used as a sentinel · line 249

Quoted 2 lines

doc/guides/prog_guide/poll_mode_drv.rst +0 -0

     When configuring for DCB operation, at port initialization, both

the number of transmit queues and the number of receive queues must be set to 128.

doc/guides/prog_guide/poll_mode_drv.rst One descriptor in the TX ring is used as a sentinel to avoid a hardware race con

Quoted 5 lines

doc/guides/prog_guide/poll_mode_drv.rst +4 -0

+Free Tx mbuf on Demand
+~~~~~~~~~~~~~~~~~~~~~~
+
+Many of the drivers don't release the mbuf back to the mempool, or

local cache, immediately after the packet has been

doc/guides/prog_guide/poll_mode_drv.rst One descriptor in the TX ring is used as a sentinel · line 255

Quoted 2 lines

doc/guides/prog_guide/poll_mode_drv.rst +2 -0

+transmitted.
+Instead, they leave the mbuf in their Tx ring and either perform a bulk

release when the ``tx_rs_thresh`` has been

doc/guides/prog_guide/poll_mode_drv.rst One descriptor in the TX ring is used as a sentinel to avoid a hardware race con

Quoted 3 lines

doc/guides/prog_guide/poll_mode_drv.rst +3 -0

+crossed or free the mbuf when a slot in the Tx ring is needed.
+
+An application can request the driver to release used mbufs with the

``rte_eth_tx_done_cleanup()`` API.

doc/guides/prog_guide/poll_mode_drv.rst One descriptor in the TX ring is used as a sentinel · line 259

Quoted 1 line

doc/guides/prog_guide/poll_mode_drv.rst +1 -0

+This API requests the driver to release mbufs that are no longer in

use, independent of whether or not the

doc/guides/prog_guide/poll_mode_drv.rst One descriptor in the TX ring is used as a sentinel · line 261

Quoted 2 lines

doc/guides/prog_guide/poll_mode_drv.rst +2 -0

+``tx_rs_thresh`` has been crossed.
+There are two scenarios when an application may want the mbuf released

immediately:

doc/guides/prog_guide/poll_mode_drv.rst One descriptor in the TX ring is used as a sentinel · line 263

Quoted 2 lines

doc/guides/prog_guide/poll_mode_drv.rst +2 -0

+
+* When a given packet needs to be sent to multiple destination

interfaces (either for Layer 2 flooding or Layer 3

doc/guides/prog_guide/poll_mode_drv.rst One descriptor in the TX ring is used as a sentinel · line 265

Quoted 2 lines

doc/guides/prog_guide/poll_mode_drv.rst +2 -0

+  multi-cast).
+  One option is to make a copy of the packet or a copy of the header

portion that needs to be manipulated.

doc/guides/prog_guide/poll_mode_drv.rst One descriptor in the TX ring is used as a sentinel · line 266

Quoted 1 line

doc/guides/prog_guide/poll_mode_drv.rst +1 -0

+  A second option is to transmit the packet and then poll the

``rte_eth_tx_done_cleanup()`` API until the reference

doc/guides/prog_guide/poll_mode_drv.rst One descriptor in the TX ring is used as a sentinel · line 268

Quoted 2 lines

doc/guides/prog_guide/poll_mode_drv.rst +2 -0

+  count on the packet is decremented.
+  Then the same packet can be transmitted to the next destination

interface.

By reading this paragraph, it's not so clear to me that the packet that will be transmitted on all interfaces will be different from one port to another.

Maybe it could be reworded to insist on that?

doc/guides/prog_guide/poll_mode_drv.rst One descriptor in the TX ring is used as a sentinel · line 270

Quoted 3 lines

doc/guides/prog_guide/poll_mode_drv.rst +2 -0

+
+* If an application is designed to make multiple runs, like a packet

generator, and one run has completed.

doc/guides/prog_guide/poll_mode_drv.rst One descriptor in the TX ring is used as a sentinel to avoid a hardware race con · line 273

Quoted 1 line

doc/guides/prog_guide/poll_mode_drv.rst +1 -0

+  The application may want to reset to a clean state.

I'd reword into:

Some applications are designed to make multiple runs, like a packet generator.
Between each run, the application may want to reset to a clean state.

What do you mean by "clean state"? All mbufs returned into the mempools? Why would a packet generator need that? For performance?

Reworded as you suggested, then attempted to explain a 'clean state'.

Also, do we want to ensure that all packets are actually transmitted?

Can we do that with this API or should we use another API like rte_eth_tx_descriptor_status() [1] ?

[1] http://dpdk.org/dev/patchwork/patch/21549/

I read through this patch. This API doesn't indicate if the packet was

Thanks,
Olivier

Thank you for your comments. See inline.

Quoted 59 lines · Olivier MATZ

Hi Billy,

Quoted 9 lines · Billy McFall

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

Signed-off-by Billy McFall <[email protected]>

---
 doc/guides/conf.py                      |  7 +++++--
 doc/guides/nics/features/default.ini    |  4 +++-
 doc/guides/prog_guide/poll_mode_drv.rst | 28  

++++++++++++++++++++++++++++

Quoted 3 lines

 doc/guides/rel_notes/release_17_05.rst  |  7 ++++++-
 lib/librte_ether/rte_ethdev.c           | 14 ++++++++++++++
 lib/librte_ether/rte_ethdev.h           | 31  

+++++++++++++++++++++++++++++++

Quoted 2 lines

 6 files changed, 87 insertions(+), 4 deletions(-)

[...]

doc/guides/prog_guide/poll_mode_drv.rst One descriptor in the TX ring is used as a sentinel

Quoted 3 lines

doc/guides/prog_guide/poll_mode_drv.rst +0 -0

@@ -249,6 +249,34 @@ One descriptor in the TX ring is used as a sentinel  

to avoid a hardware race con

doc/guides/prog_guide/poll_mode_drv.rst One descriptor in the TX ring is used as a sentinel · line 249

Quoted 2 lines

doc/guides/prog_guide/poll_mode_drv.rst +0 -0

     When configuring for DCB operation, at port initialization, both  

the number of transmit queues and the number of receive queues must be set to 128.

doc/guides/prog_guide/poll_mode_drv.rst One descriptor in the TX ring is used as a sentinel to avoid a hardware race con

Quoted 5 lines

doc/guides/prog_guide/poll_mode_drv.rst +4 -0

+Free Tx mbuf on Demand
+~~~~~~~~~~~~~~~~~~~~~~
+
+Many of the drivers don't release the mbuf back to the mempool, or  

local cache, immediately after the packet has been

doc/guides/prog_guide/poll_mode_drv.rst One descriptor in the TX ring is used as a sentinel · line 255

Quoted 2 lines

doc/guides/prog_guide/poll_mode_drv.rst +2 -0

+transmitted.
+Instead, they leave the mbuf in their Tx ring and either perform a bulk  

release when the ``tx_rs_thresh`` has been

doc/guides/prog_guide/poll_mode_drv.rst One descriptor in the TX ring is used as a sentinel to avoid a hardware race con

Quoted 3 lines

doc/guides/prog_guide/poll_mode_drv.rst +3 -0

+crossed or free the mbuf when a slot in the Tx ring is needed.
+
+An application can request the driver to release used mbufs with the  

``rte_eth_tx_done_cleanup()`` API.

doc/guides/prog_guide/poll_mode_drv.rst One descriptor in the TX ring is used as a sentinel · line 259

Quoted 1 line

doc/guides/prog_guide/poll_mode_drv.rst +1 -0

+This API requests the driver to release mbufs that are no longer in  

use, independent of whether or not the

doc/guides/prog_guide/poll_mode_drv.rst One descriptor in the TX ring is used as a sentinel · line 261

Quoted 2 lines

doc/guides/prog_guide/poll_mode_drv.rst +2 -0

+``tx_rs_thresh`` has been crossed.
+There are two scenarios when an application may want the mbuf released  

immediately:

doc/guides/prog_guide/poll_mode_drv.rst One descriptor in the TX ring is used as a sentinel · line 263

Quoted 2 lines

doc/guides/prog_guide/poll_mode_drv.rst +2 -0

+
+* When a given packet needs to be sent to multiple destination  

interfaces (either for Layer 2 flooding or Layer 3

doc/guides/prog_guide/poll_mode_drv.rst One descriptor in the TX ring is used as a sentinel · line 265

Quoted 2 lines

doc/guides/prog_guide/poll_mode_drv.rst +2 -0

+  multi-cast).
+  One option is to make a copy of the packet or a copy of the header  

portion that needs to be manipulated.

doc/guides/prog_guide/poll_mode_drv.rst One descriptor in the TX ring is used as a sentinel · line 266

Quoted 1 line

doc/guides/prog_guide/poll_mode_drv.rst +1 -0

+  A second option is to transmit the packet and then poll the  

``rte_eth_tx_done_cleanup()`` API until the reference

doc/guides/prog_guide/poll_mode_drv.rst One descriptor in the TX ring is used as a sentinel · line 268

Quoted 2 lines

doc/guides/prog_guide/poll_mode_drv.rst +2 -0

+  count on the packet is decremented.
+  Then the same packet can be transmitted to the next destination  

Quoted 9 lines

interface.

By reading this paragraph, it's not so clear to me that the packet that will be transmitted on all interfaces will be different from one port to another.

Maybe it could be reworded to insist on that?

What if I add the following sentence:

Then the same packet can be transmitted to the next destination interface.
+ The application is still responsible for managing any packet manipulations needed between the different destination
+ interfaces, but a packet copy can be avoided.

Quoted 7 lines

doc/guides/prog_guide/poll_mode_drv.rst One descriptor in the TX ring is used as a sentinel · line 270

Quoted 2 lines

doc/guides/prog_guide/poll_mode_drv.rst +2 -0

+
+* If an application is designed to make multiple runs, like a packet  

generator, and one run has completed.

doc/guides/prog_guide/poll_mode_drv.rst One descriptor in the TX ring is used as a sentinel to avoid a hardware race con · line 273

Quoted 1 line

doc/guides/prog_guide/poll_mode_drv.rst +1 -0

+  The application may want to reset to a clean state.  

I'd reword into:

Quoted 8 lines

Some applications are designed to make multiple runs, like a packet generator.
Between each run, the application may want to reset to a clean state.

What do you mean by "clean state"? All mbufs returned into the mempools? Why would a packet generator need that? For performance?

Reworded as you suggested, then attempted to explain a 'clean state'.

Also reworded the last sentence a little.

+ * Some applications are designed to make multiple runs, like a packet generator.
+ For performance reasons and consistency between runs, the application
may want to reset back to an initial state

(quoted diff) +2 -0

+   between each run, where all mbufs are returned to the mempool.
+   In this case, it can call the ``rte_eth_tx_done_cleanup()`` API for

each destination interface it has been using
+ to request it to release of all its used mbufs.

Quoted 2 lines

Also, do we want to ensure that all packets are actually transmitted?

Added an additional sentence to indicate that this API doesn't manage whether or not the packet has been transmitted.

Then the same packet can be transmitted to the next destination interface. The application is still responsible for managing any packet
manipulations needed between the different destination
interface, but a packet copy can be avoided.
+ This API is independent of whether the packet was transmitted or dropped, only that the mbuf is no longer in use by
+ the interface.

Quoted 6 lines

Can we do that with this API or should we use another API like rte_eth_tx_descriptor_status() [1] ?

[1] http://dpdk.org/dev/patchwork/patch/21549/

I read through this patch. This API doesn't indicate if the packet was

transmitted or dropped (I think that is what you were asking). This API could be used by the application to determine if the mbuf has been freed, as opposed to polling the rte_mbuf_refcnt_read() for a change in value. Did I miss your point?

Hi Billy,

Quoted 10 lines · Billy McFall

Thank you for your comments. See inline.

Quoted 5 lines · Olivier MATZ

Hi Billy,

Quoted 1 line · Billy McFall

Add a new API to force free consumed buffers on Tx ring. API will

return

Quoted 1 line

Quoted 1 line

Quoted 1 line

the number of packets freed (0-n) or error code if feature not

supported

Quoted 12 lines

Quoted 12 lines

Quoted 7 lines

(-ENOTSUP) or input invalid (-ENODEV).

Signed-off-by Billy McFall <[email protected]>

---
 doc/guides/conf.py                      |  7 +++++--
 doc/guides/nics/features/default.ini    |  4 +++-
 doc/guides/prog_guide/poll_mode_drv.rst | 28

++++++++++++++++++++++++++++

Quoted 3 lines

 doc/guides/rel_notes/release_17_05.rst  |  7 ++++++-
 lib/librte_ether/rte_ethdev.c           | 14 ++++++++++++++
 lib/librte_ether/rte_ethdev.h           | 31

+++++++++++++++++++++++++++++++

Quoted 8 lines

Quoted 8 lines

Quoted 2 lines

 6 files changed, 87 insertions(+), 4 deletions(-)

[...]

doc/guides/prog_guide/poll_mode_drv.rst One descriptor in the TX ring is used as a

Quoted 3 lines

doc/guides/prog_guide/poll_mode_drv.rst +0 -0

@@ -249,6 +249,34 @@ One descriptor in the TX ring is used as a

sentinel

Quoted 4 lines

Quoted 4 lines

to avoid a hardware race con

doc/guides/prog_guide/poll_mode_drv.rst One descriptor in the TX ring is used as a · line 249

Quoted 2 lines

doc/guides/prog_guide/poll_mode_drv.rst +0 -0

     When configuring for DCB operation, at port initialization, both

the number of transmit queues and the number of receive queues must be

set

Quoted 9 lines

Quoted 9 lines

to 128.

doc/guides/prog_guide/poll_mode_drv.rst One descriptor in the TX ring is used as a sentinel to avoid a hardware race con

Quoted 5 lines

doc/guides/prog_guide/poll_mode_drv.rst +4 -0

+Free Tx mbuf on Demand
+~~~~~~~~~~~~~~~~~~~~~~
+
+Many of the drivers don't release the mbuf back to the mempool, or

local cache, immediately after the packet has been

doc/guides/prog_guide/poll_mode_drv.rst One descriptor in the TX ring is used as a · line 255

Quoted 2 lines

doc/guides/prog_guide/poll_mode_drv.rst +2 -0

+transmitted.
+Instead, they leave the mbuf in their Tx ring and either perform a

bulk

Quoted 9 lines

Quoted 9 lines

release when the ``tx_rs_thresh`` has been

doc/guides/prog_guide/poll_mode_drv.rst One descriptor in the TX ring is used as a sentinel to avoid a hardware race con

Quoted 3 lines

doc/guides/prog_guide/poll_mode_drv.rst +3 -0

+crossed or free the mbuf when a slot in the Tx ring is needed.
+
+An application can request the driver to release used mbufs with the

``rte_eth_tx_done_cleanup()`` API.

doc/guides/prog_guide/poll_mode_drv.rst One descriptor in the TX ring is used as a · line 259

Quoted 1 line

doc/guides/prog_guide/poll_mode_drv.rst +1 -0

+This API requests the driver to release mbufs that are no longer in

use, independent of whether or not the

Quoted 2 lines

doc/guides/prog_guide/poll_mode_drv.rst +2 -0

+``tx_rs_thresh`` has been crossed.
+There are two scenarios when an application may want the mbuf

released

Quoted 17 lines

Quoted 17 lines

immediately:

doc/guides/prog_guide/poll_mode_drv.rst One descriptor in the TX ring is used as a · line 263

Quoted 2 lines

doc/guides/prog_guide/poll_mode_drv.rst +2 -0

+
+* When a given packet needs to be sent to multiple destination

interfaces (either for Layer 2 flooding or Layer 3

doc/guides/prog_guide/poll_mode_drv.rst One descriptor in the TX ring is used as a · line 265

Quoted 2 lines

doc/guides/prog_guide/poll_mode_drv.rst +2 -0

+  multi-cast).
+  One option is to make a copy of the packet or a copy of the header

portion that needs to be manipulated.

doc/guides/prog_guide/poll_mode_drv.rst One descriptor in the TX ring is used as a · line 266

Quoted 1 line

doc/guides/prog_guide/poll_mode_drv.rst +1 -0

+  A second option is to transmit the packet and then poll the

``rte_eth_tx_done_cleanup()`` API until the reference

doc/guides/prog_guide/poll_mode_drv.rst One descriptor in the TX ring is used as a · line 268

Quoted 2 lines

doc/guides/prog_guide/poll_mode_drv.rst +2 -0

+  count on the packet is decremented.
+  Then the same packet can be transmitted to the next destination

interface.

By reading this paragraph, it's not so clear to me that the packet that will be transmitted on all interfaces will be different from one port to another.

Quoted 6 lines

Quoted 3 lines

Maybe it could be reworded to insist on that?

What if I add the following sentence:

Then the same packet can be transmitted to the next destination

interface.

Quoted 3 lines

+ The application is still responsible for managing any packet manipulations needed between the different destination
+ interfaces, but a packet copy can be avoided.

looks good, thanks.

Quoted 5 lines

Quoted 5 lines

doc/guides/prog_guide/poll_mode_drv.rst One descriptor in the TX ring is used as a · line 270

Quoted 2 lines

doc/guides/prog_guide/poll_mode_drv.rst +2 -0

+
+* If an application is designed to make multiple runs, like a packet

generator, and one run has completed.

Quoted 1 line

doc/guides/prog_guide/poll_mode_drv.rst +1 -0

+  The application may want to reset to a clean state.

Quoted 7 lines

Quoted 7 lines

I'd reword into:

Some applications are designed to make multiple runs, like a packet generator.
Between each run, the application may want to reset to a clean state.

What do you mean by "clean state"? All mbufs returned into the

mempools?

Quoted 11 lines

Quoted 3 lines

Why would a packet generator need that? For performance?

Reworded as you suggested, then attempted to explain a 'clean state'.

Also reworded the last sentence a little.

+ * Some applications are designed to make multiple runs, like a packet generator.
+ For performance reasons and consistency between runs, the application
may want to reset back to an initial state

(quoted diff) +2 -0

+   between each run, where all mbufs are returned to the mempool.
+   In this case, it can call the ``rte_eth_tx_done_cleanup()`` API for

Quoted 2 lines

each destination interface it has been using
+ to request it to release of all its used mbufs.

ok, looks clearer to me, thanks

Quoted 7 lines

Quoted 2 lines

Also, do we want to ensure that all packets are actually transmitted?

Added an additional sentence to indicate that this API doesn't manage whether or not the packet has been transmitted.

Then the same packet can be transmitted to the next destination

interface.

Quoted 6 lines

The application is still responsible for managing any packet
manipulations needed between the different destination
interface, but a packet copy can be avoided.
+ This API is independent of whether the packet was transmitted or dropped, only that the mbuf is no longer in use by
+ the interface.

ok

Quoted 10 lines

Quoted 6 lines

Can we do that with this API or should we use another API like rte_eth_tx_descriptor_status() [1] ?

[1] http://dpdk.org/dev/patchwork/patch/21549/

I read through this patch. This API doesn't indicate if the packet was

transmitted or dropped (I think that is what you were asking). This API could be used by the application to determine if the mbuf has been freed, as opposed to polling the rte_mbuf_refcnt_read() for a change in value. Did I miss your point?

Maybe my question was not clear :)
Let me try to reword it.

For a traffic generator use-case, a dummy algorithm may be:

1/ send packets in a loop until a condition is met (ex: packet count reached)
2/ call rte_eth_tx_done_cleanup()
3/ read stats for report

I think there is something missing between 1/ and 2/, to ensure that all packets that were in the tx queue are processed (either transmitted or dropped). If that's not the case, both steps 2/ and 3/ will not behave as expected:
- all mbufs won't be returned to the pool
- statistics may be wrong

Maybe a simple wait() could do the job. Using a combination of rte_eth_tx_done_cleanup() + rte_eth_tx_descriptor_status()
is probably also a solution.

Do you confirm rte_eth_tx_done_cleanup() does not check that?

Confirm. rte_eth_tx_done_cleanup() does not check that. In the flooding

Olivier

On Fri, Mar 24, 2017 at 8:46 AM, Olivier Matz <[email protected]> wrote:

Quoted 23 lines

Quoted 5 lines

I read through this patch. This API doesn't indicate if the packet was transmitted or dropped (I think that is what you were asking). This API could be used by the application to determine if the mbuf has been freed, as opposed to polling the rte_mbuf_refcnt_read() for a change in value. Did I miss your point?

Maybe my question was not clear :)
Let me try to reword it.

For a traffic generator use-case, a dummy algorithm may be:

1/ send packets in a loop until a condition is met (ex: packet count reached)
2/ call rte_eth_tx_done_cleanup()
3/ read stats for report

I think there is something missing between 1/ and 2/, to ensure that all packets that were in the tx queue are processed (either transmitted or dropped). If that's not the case, both steps 2/ and 3/ will not behave as expected:
- all mbufs won't be returned to the pool
- statistics may be wrong

Quoted 7 lines

Maybe a simple wait() could do the job. Using a combination of rte_eth_tx_done_cleanup() + rte_eth_tx_descriptor_status()
is probably also a solution.

Do you confirm rte_eth_tx_done_cleanup() does not check that?

Confirm. rte_eth_tx_done_cleanup() does not check that. In the flooding case,
the applications is expected to poll rte_eth_tx_done_cleanup() until some condition
is met, like ref_count of given packet is decremented. So on the packetGen case, the
application would need to wait some time and/or call rte_eth_tx_descriptor_status()
as you suggested.

My original patch returned RTE_DONE (no more packets pending), RTE_PROCESSING (freed what I could but there are still packets in the queue) or -ERRNO for error. Then packets freed count was returned via a pointer in the param list.
That would have solved what you are asking, but that was shot down as being overkill.

Should I add another sentence to the packet generator bullet indicating that it is the
application's job to make sure no more packets are pending? Like:

In this case, it can call the ``rte_eth_tx_done_cleanup()`` API for each
destination interface it has been using
to request it to release of all its used mbufs.
+ It is the application's responsibility to ensure all packets have been processed by the destination interface.
+ Use rte_eth_tx_descriptor_status() to obtain the status of the transmit queue,

See request from 11/21/2016:
http://dpdk.org/ml/archives/dev/2016-November/050585.html

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.

---
v3:
* Removed extra white space in rte_ethdev.h.
* Removed inline of new API function in rte_ethdev.h.

---
v4:
* Added new API to documentation of per nic supported features.

---
v5:
* Added documentation to the Programmer's Guide.

---
v6:
* Rebase to master.
* Added Release Note.
* Addressed comments on documentation to the Programmer's Guide.
* Renamed feature string to "Free Tx mbuf on demand" and modified feature string processing code to extend the maximum string length from 20 characters to 25 characters.
* Reworded the commit log for PATCH 2/3 to attempt to clearify that the API is independent of tx_rs_thresh.
* Will address the rte_errno comment in a separate merge.

---
v7:
* New API was located between rte_eth_tx_buffer and rte_eth_tx_buffer_set_err_callback in the file. Moved this function below the tx_buffer functions.
* Added previously acked-by tags to merge comments.
* Moved documentation content from the mempool documentation to the ethdev documentation.

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

doc/guides/conf.py                      |   7 +-
doc/guides/nics/features/default.ini    |   4 +-
doc/guides/nics/features/e1000.ini      |   1 +
doc/guides/nics/features/vhost.ini      |   1 +
doc/guides/prog_guide/poll_mode_drv.rst |  28 +++++++
doc/guides/rel_notes/release_17_05.rst  |   7 +-
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.c           |  14 ++++
lib/librte_ether/rte_ethdev.h           |  31 ++++++++
12 files changed, 229 insertions(+), 4 deletions(-)

Signature · 1 line

2.9.3

See request from 11/21/2016:
http://dpdk.org/ml/archives/dev/2016-November/050585.html

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.

See request from 11/21/2016:
http://dpdk.org/ml/archives/dev/2016-November/050585.html

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.