[dpdk-dev,v5,2/3] net/virtio: add packet injection method
Checks
Commit Message
This patch adds dev_pause, dev_resume and inject_pkts api to allow
driver to pause the worker thread and inject special packets into
Tx queue. The next patch will be based on this.
Signed-off-by: Xiao Wang <xiao.w.wang@intel.com>
---
drivers/net/virtio/virtio_ethdev.c | 51 +++++++++++++++++++++++++++++++++
drivers/net/virtio/virtio_ethdev.h | 6 ++++
drivers/net/virtio/virtio_pci.h | 7 +++++
drivers/net/virtio/virtio_rxtx.c | 2 +-
drivers/net/virtio/virtio_rxtx_simple.c | 2 +-
5 files changed, 66 insertions(+), 2 deletions(-)
Comments
On Fri, Jan 05, 2018 at 08:46:56AM -0800, Xiao Wang wrote:
[...]
> +/*
> + * Recover hw state to let worker thread continue.
> + */
> +void
> +virtio_dev_resume(struct rte_eth_dev *dev)
> +{
> + struct virtio_hw *hw = dev->data->dev_private;
> +
> + hw->started = 1;
> + rte_spinlock_unlock(&hw->state_lock);
> +}
> +
> +int
> +virtio_inject_pkts(struct rte_eth_dev *dev, struct rte_mbuf **buf, int count)
> +{
It would be better to name `buf` as tx_pkts and name
`count` as nb_pkts.
It would be better to add some comments to highlight
that the device needs to be paused before calling this
function in driver.
> + struct virtio_hw *hw = dev->data->dev_private;
> + struct virtnet_tx *txvq = dev->data->tx_queues[0];
> + int ret;
[...]
> diff --git a/drivers/net/virtio/virtio_ethdev.h b/drivers/net/virtio/virtio_ethdev.h
> index 2039bc5..4a2a2f0 100644
> --- a/drivers/net/virtio/virtio_ethdev.h
> +++ b/drivers/net/virtio/virtio_ethdev.h
> @@ -37,6 +37,7 @@
> #include <stdint.h>
>
> #include "virtio_pci.h"
> +#include "virtio_rxtx.h"
It's not necessary to include this header file.
>
> #define SPEED_10 10
> #define SPEED_100 100
> @@ -121,4 +122,9 @@ uint16_t virtio_xmit_pkts_simple(void *tx_queue, struct rte_mbuf **tx_pkts,
>
> void virtio_interrupt_handler(void *param);
>
> +int virtio_dev_pause(struct rte_eth_dev *dev);
> +void virtio_dev_resume(struct rte_eth_dev *dev);
> +int virtio_inject_pkts(struct rte_eth_dev *dev, struct rte_mbuf **buf,
> + int count);
Ditto.
> +
[...]
> diff --git a/drivers/net/virtio/virtio_rxtx.c b/drivers/net/virtio/virtio_rxtx.c
> index 6a24fde..bbf5aaf 100644
> --- a/drivers/net/virtio/virtio_rxtx.c
> +++ b/drivers/net/virtio/virtio_rxtx.c
> @@ -1017,7 +1017,7 @@
> uint16_t nb_used, nb_tx = 0;
> int error;
>
> - if (unlikely(hw->started == 0))
> + if (unlikely(hw->started == 0) && tx_pkts != hw->inject_buf)
Why not just put all the condition checks in unlikely()?
If (hw->started == 0) is "unlikely", then
(hw->started == 0 && tx_pkts != hw->inject_buf) would
be more "unlikely".
> return nb_tx;
>
> if (unlikely(nb_pkts < 1))
> diff --git a/drivers/net/virtio/virtio_rxtx_simple.c b/drivers/net/virtio/virtio_rxtx_simple.c
> index b5bc1c4..d81d162 100644
> --- a/drivers/net/virtio/virtio_rxtx_simple.c
> +++ b/drivers/net/virtio/virtio_rxtx_simple.c
> @@ -99,7 +99,7 @@ int __attribute__((cold))
> uint16_t desc_idx_max = (vq->vq_nentries >> 1) - 1;
> uint16_t nb_tx = 0;
>
> - if (unlikely(hw->started == 0))
> + if (unlikely(hw->started == 0) && tx_pkts != hw->inject_buf)
Ditto.
Thanks,
Tiwei
Hi,
> -----Original Message-----
> From: Bie, Tiwei
> Sent: Saturday, January 6, 2018 2:01 AM
> To: Wang, Xiao W <xiao.w.wang@intel.com>
> Cc: dev@dpdk.org; yliu@fridaylinux.org; stephen@networkplumber.org
> Subject: Re: [PATCH v5 2/3] net/virtio: add packet injection method
>
> On Fri, Jan 05, 2018 at 08:46:56AM -0800, Xiao Wang wrote:
> [...]
> > +/*
> > + * Recover hw state to let worker thread continue.
> > + */
> > +void
> > +virtio_dev_resume(struct rte_eth_dev *dev)
> > +{
> > + struct virtio_hw *hw = dev->data->dev_private;
> > +
> > + hw->started = 1;
> > + rte_spinlock_unlock(&hw->state_lock);
> > +}
> > +
> > +int
> > +virtio_inject_pkts(struct rte_eth_dev *dev, struct rte_mbuf **buf, int count)
> > +{
>
> It would be better to name `buf` as tx_pkts and name
> `count` as nb_pkts.
>
> It would be better to add some comments to highlight
> that the device needs to be paused before calling this
> function in driver.
Yes, making it aligned with the existing virtio_xmit_pkts looks better.
A highlight comment is helpful. Will add it in v6.
>
> > + struct virtio_hw *hw = dev->data->dev_private;
> > + struct virtnet_tx *txvq = dev->data->tx_queues[0];
> > + int ret;
> [...]
> > diff --git a/drivers/net/virtio/virtio_ethdev.h
> b/drivers/net/virtio/virtio_ethdev.h
> > index 2039bc5..4a2a2f0 100644
> > --- a/drivers/net/virtio/virtio_ethdev.h
> > +++ b/drivers/net/virtio/virtio_ethdev.h
> > @@ -37,6 +37,7 @@
> > #include <stdint.h>
> >
> > #include "virtio_pci.h"
> > +#include "virtio_rxtx.h"
>
> It's not necessary to include this header file.
Yes, it should be removed since I have removed the txvq parameter in virtio_inject_pkts.
>
> >
> > #define SPEED_10 10
> > #define SPEED_100 100
> > @@ -121,4 +122,9 @@ uint16_t virtio_xmit_pkts_simple(void *tx_queue,
> struct rte_mbuf **tx_pkts,
> >
> > void virtio_interrupt_handler(void *param);
> >
> > +int virtio_dev_pause(struct rte_eth_dev *dev);
> > +void virtio_dev_resume(struct rte_eth_dev *dev);
> > +int virtio_inject_pkts(struct rte_eth_dev *dev, struct rte_mbuf **buf,
> > + int count);
>
> Ditto.
>
> > +
> [...]
> > diff --git a/drivers/net/virtio/virtio_rxtx.c b/drivers/net/virtio/virtio_rxtx.c
> > index 6a24fde..bbf5aaf 100644
> > --- a/drivers/net/virtio/virtio_rxtx.c
> > +++ b/drivers/net/virtio/virtio_rxtx.c
> > @@ -1017,7 +1017,7 @@
> > uint16_t nb_used, nb_tx = 0;
> > int error;
> >
> > - if (unlikely(hw->started == 0))
> > + if (unlikely(hw->started == 0) && tx_pkts != hw->inject_buf)
>
> Why not just put all the condition checks in unlikely()?
>
> If (hw->started == 0) is "unlikely", then
> (hw->started == 0 && tx_pkts != hw->inject_buf) would
> be more "unlikely".
Your way could ensure that datapath perf is not affected.
Will change it in v6.
Thanks a lot,
Xiao
@@ -55,6 +55,7 @@
#include <rte_memory.h>
#include <rte_eal.h>
#include <rte_dev.h>
+#include <rte_cycles.h>
#include "virtio_ethdev.h"
#include "virtio_pci.h"
@@ -1249,6 +1250,52 @@ static int virtio_dev_xstats_get_names(struct rte_eth_dev *dev,
return 0;
}
+int
+virtio_dev_pause(struct rte_eth_dev *dev)
+{
+ struct virtio_hw *hw = dev->data->dev_private;
+
+ rte_spinlock_lock(&hw->state_lock);
+
+ if (hw->started == 0) {
+ /* In case that the device is just stopped. */
+ rte_spinlock_unlock(&hw->state_lock);
+ return -1;
+ }
+ hw->started = 0;
+ /*
+ * Prevent the worker thread from touching queues to avoid contention,
+ * 1 ms should be enough for the ongoing Tx function to finish.
+ */
+ rte_delay_ms(1);
+ return 0;
+}
+
+/*
+ * Recover hw state to let worker thread continue.
+ */
+void
+virtio_dev_resume(struct rte_eth_dev *dev)
+{
+ struct virtio_hw *hw = dev->data->dev_private;
+
+ hw->started = 1;
+ rte_spinlock_unlock(&hw->state_lock);
+}
+
+int
+virtio_inject_pkts(struct rte_eth_dev *dev, struct rte_mbuf **buf, int count)
+{
+ struct virtio_hw *hw = dev->data->dev_private;
+ struct virtnet_tx *txvq = dev->data->tx_queues[0];
+ int ret;
+
+ hw->inject_buf = buf;
+ ret = dev->tx_pkt_burst(txvq, buf, count);
+ hw->inject_buf = NULL;
+ return ret;
+}
+
/*
* Process Virtio Config changed interrupt and call the callback
* if link state changed.
@@ -1786,6 +1833,8 @@ static int eth_virtio_pci_remove(struct rte_pci_device *pci_dev)
return -EBUSY;
}
+ rte_spinlock_init(&hw->state_lock);
+
hw->use_simple_rx = 1;
hw->use_simple_tx = 1;
@@ -1952,12 +2001,14 @@ static void virtio_dev_free_mbufs(struct rte_eth_dev *dev)
PMD_INIT_LOG(DEBUG, "stop");
+ rte_spinlock_lock(&hw->state_lock);
if (intr_conf->lsc || intr_conf->rxq)
virtio_intr_disable(dev);
hw->started = 0;
memset(&link, 0, sizeof(link));
virtio_dev_atomic_write_link_status(dev, &link);
+ rte_spinlock_unlock(&hw->state_lock);
}
static int
@@ -37,6 +37,7 @@
#include <stdint.h>
#include "virtio_pci.h"
+#include "virtio_rxtx.h"
#define SPEED_10 10
#define SPEED_100 100
@@ -121,4 +122,9 @@ uint16_t virtio_xmit_pkts_simple(void *tx_queue, struct rte_mbuf **tx_pkts,
void virtio_interrupt_handler(void *param);
+int virtio_dev_pause(struct rte_eth_dev *dev);
+void virtio_dev_resume(struct rte_eth_dev *dev);
+int virtio_inject_pkts(struct rte_eth_dev *dev, struct rte_mbuf **buf,
+ int count);
+
#endif /* _VIRTIO_ETHDEV_H_ */
@@ -270,6 +270,13 @@ struct virtio_hw {
struct virtio_pci_common_cfg *common_cfg;
struct virtio_net_config *dev_cfg;
void *virtio_user_dev;
+ /*
+ * App management thread and virtio interrupt handler thread
+ * both can change the 'started' flag, this lock is meant to
+ * avoid such a contention.
+ */
+ rte_spinlock_t state_lock;
+ struct rte_mbuf **inject_buf;
struct virtqueue **vqs;
};
@@ -1017,7 +1017,7 @@
uint16_t nb_used, nb_tx = 0;
int error;
- if (unlikely(hw->started == 0))
+ if (unlikely(hw->started == 0) && tx_pkts != hw->inject_buf)
return nb_tx;
if (unlikely(nb_pkts < 1))
@@ -99,7 +99,7 @@ int __attribute__((cold))
uint16_t desc_idx_max = (vq->vq_nentries >> 1) - 1;
uint16_t nb_tx = 0;
- if (unlikely(hw->started == 0))
+ if (unlikely(hw->started == 0) && tx_pkts != hw->inject_buf)
return nb_tx;
nb_used = VIRTQUEUE_NUSED(vq);