* [dpdk-dev] [PATCH v2 1/3] ethdev: new API to free consumed buffers in Tx ring
2017-01-11 20:03 [dpdk-dev] [PATCH v2 0/3] new API to free consumed buffers in Tx ring Billy McFall
@ 2017-01-11 20:03 ` Billy McFall
2017-01-12 1:41 ` Stephen Hemminger
2017-01-11 20:03 ` [dpdk-dev] [PATCH v2 2/3] net/e1000: e1000 igb support to free consumed buffers Billy McFall
2017-01-11 20:03 ` [dpdk-dev] [PATCH v2 3/3] net/vhost: vHost " Billy McFall
2 siblings, 1 reply; 7+ messages in thread
From: Billy McFall @ 2017-01-11 20:03 UTC (permalink / raw)
To: thomas.monjalon, wenzhuo.lu; +Cc: dev, 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 <bmcfall@redhat.com>
---
lib/librte_ether/rte_ethdev.h | 43 +++++++++++++++++++++++++++++++++++++++++++
1 file changed, 43 insertions(+)
diff --git a/lib/librte_ether/rte_ethdev.h b/lib/librte_ether/rte_ethdev.h
index 9678179..ce662b6 100644
--- a/lib/librte_ether/rte_ethdev.h
+++ b/lib/librte_ether/rte_ethdev.h
@@ -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
--
2.9.3
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [dpdk-dev] [PATCH v2 1/3] ethdev: new API to free consumed buffers in Tx ring
2017-01-11 20:03 ` [dpdk-dev] [PATCH v2 1/3] ethdev: " Billy McFall
@ 2017-01-12 1:41 ` Stephen Hemminger
0 siblings, 0 replies; 7+ messages in thread
From: Stephen Hemminger @ 2017-01-12 1:41 UTC (permalink / raw)
To: Billy McFall; +Cc: thomas.monjalon, wenzhuo.lu, dev
On Wed, 11 Jan 2017 15:03:21 -0500
Billy McFall <bmcfall@redhat.com> 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.
> + */
> +
> +static inline int
> +rte_eth_tx_done_cleanup(uint8_t port_id, uint16_t queue_id, uint32_t free_cnt)
Extra white space.
> +{
> + 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.
^ permalink raw reply [flat|nested] 7+ messages in thread
* [dpdk-dev] [PATCH v2 2/3] net/e1000: e1000 igb support to free consumed buffers
2017-01-11 20:03 [dpdk-dev] [PATCH v2 0/3] new API to free consumed buffers in Tx ring Billy McFall
2017-01-11 20:03 ` [dpdk-dev] [PATCH v2 1/3] ethdev: " Billy McFall
@ 2017-01-11 20:03 ` Billy McFall
2017-01-11 20:03 ` [dpdk-dev] [PATCH v2 3/3] net/vhost: vHost " Billy McFall
2 siblings, 0 replies; 7+ messages in thread
From: Billy McFall @ 2017-01-11 20:03 UTC (permalink / raw)
To: thomas.monjalon, wenzhuo.lu; +Cc: dev, 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.
Signed-off-by: Billy McFall <bmcfall@redhat.com>
---
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(+)
diff --git a/drivers/net/e1000/e1000_ethdev.h b/drivers/net/e1000/e1000_ethdev.h
index 6c25c8d..fce0278 100644
--- a/drivers/net/e1000/e1000_ethdev.h
+++ b/drivers/net/e1000/e1000_ethdev.h
@@ -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);
diff --git a/drivers/net/e1000/igb_ethdev.c b/drivers/net/e1000/igb_ethdev.c
index 2fddf0c..4010dc4 100644
--- a/drivers/net/e1000/igb_ethdev.c
+++ b/drivers/net/e1000/igb_ethdev.c
@@ -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,
diff --git a/drivers/net/e1000/igb_rxtx.c b/drivers/net/e1000/igb_rxtx.c
index dbd37ac..1d4fbcb 100644
--- a/drivers/net/e1000/igb_rxtx.c
+++ b/drivers/net/e1000/igb_rxtx.c
@@ -1227,6 +1227,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
+ * 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. */
+ 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
+ * 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
^ permalink raw reply [flat|nested] 7+ messages in thread* [dpdk-dev] [PATCH v2 3/3] net/vhost: vHost support to free consumed buffers
2017-01-11 20:03 [dpdk-dev] [PATCH v2 0/3] new API to free consumed buffers in Tx ring Billy McFall
2017-01-11 20:03 ` [dpdk-dev] [PATCH v2 1/3] ethdev: " Billy McFall
2017-01-11 20:03 ` [dpdk-dev] [PATCH v2 2/3] net/e1000: e1000 igb support to free consumed buffers Billy McFall
@ 2017-01-11 20:03 ` Billy McFall
2017-01-12 1:39 ` Stephen Hemminger
2 siblings, 1 reply; 7+ messages in thread
From: Billy McFall @ 2017-01-11 20:03 UTC (permalink / raw)
To: thomas.monjalon, wenzhuo.lu; +Cc: dev, 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.
Signed-off-by: Billy McFall <bmcfall@redhat.com>
---
drivers/net/vhost/rte_eth_vhost.c | 11 +++++++++++
1 file changed, 11 insertions(+)
diff --git a/drivers/net/vhost/rte_eth_vhost.c b/drivers/net/vhost/rte_eth_vhost.c
index 766d4ef..6493d56 100644
--- a/drivers/net/vhost/rte_eth_vhost.c
+++ b/drivers/net/vhost/rte_eth_vhost.c
@@ -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,
--
2.9.3
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [dpdk-dev] [PATCH v2 3/3] net/vhost: vHost support to free consumed buffers
2017-01-11 20:03 ` [dpdk-dev] [PATCH v2 3/3] net/vhost: vHost " Billy McFall
@ 2017-01-12 1:39 ` Stephen Hemminger
2017-01-20 16:00 ` Billy McFall
0 siblings, 1 reply; 7+ messages in thread
From: Stephen Hemminger @ 2017-01-12 1:39 UTC (permalink / raw)
To: Billy McFall; +Cc: thomas.monjalon, wenzhuo.lu, dev
On Wed, 11 Jan 2017 15:03:23 -0500
Billy McFall <bmcfall@redhat.com> 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.
>
> Signed-off-by: Billy McFall <bmcfall@redhat.com>
> ---
> drivers/net/vhost/rte_eth_vhost.c | 11 +++++++++++
> 1 file changed, 11 insertions(+)
>
> diff --git a/drivers/net/vhost/rte_eth_vhost.c b/drivers/net/vhost/rte_eth_vhost.c
> index 766d4ef..6493d56 100644
> --- a/drivers/net/vhost/rte_eth_vhost.c
> +++ b/drivers/net/vhost/rte_eth_vhost.c
> @@ -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,
Rather than change drivers, since this is not critical path, make
it optional to have tx_done_cleanup.
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [dpdk-dev] [PATCH v2 3/3] net/vhost: vHost support to free consumed buffers
2017-01-12 1:39 ` Stephen Hemminger
@ 2017-01-20 16:00 ` Billy McFall
0 siblings, 0 replies; 7+ messages in thread
From: Billy McFall @ 2017-01-20 16:00 UTC (permalink / raw)
To: Stephen Hemminger; +Cc: thomas.monjalon, wenzhuo.lu, dev
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
On Wed, Jan 11, 2017 at 8:39 PM, Stephen Hemminger <
stephen@networkplumber.org> wrote:
> On Wed, 11 Jan 2017 15:03:23 -0500
> Billy McFall <bmcfall@redhat.com> 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.
> >
> > Signed-off-by: Billy McFall <bmcfall@redhat.com>
> > ---
> > drivers/net/vhost/rte_eth_vhost.c | 11 +++++++++++
> > 1 file changed, 11 insertions(+)
> >
> > diff --git a/drivers/net/vhost/rte_eth_vhost.c
> b/drivers/net/vhost/rte_eth_vhost.c
> > index 766d4ef..6493d56 100644
> > --- a/drivers/net/vhost/rte_eth_vhost.c
> > +++ b/drivers/net/vhost/rte_eth_vhost.c
> > @@ -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,
>
> Rather than change drivers, since this is not critical path, make
> it optional to have tx_done_cleanup.
>
^ permalink raw reply [flat|nested] 7+ messages in thread