[dpdk-dev,v6,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 threads 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 | 56 +++++++++++++++++++++++++++++++++
drivers/net/virtio/virtio_ethdev.h | 5 +++
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, 70 insertions(+), 2 deletions(-)
Comments
On Sun, Jan 07, 2018 at 04:05:12AM -0800, Xiao Wang wrote:
> + /*
> + * 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;
Why not turning the "started" to atomic type, so that you don't need
the lock?
--yliu
> + struct rte_mbuf **inject_pkts;
>
> struct virtqueue **vqs;
> };
> -----Original Message-----
> From: Yuanhan Liu [mailto:yliu@fridaylinux.org]
> Sent: Monday, January 8, 2018 9:04 PM
> To: Wang, Xiao W <xiao.w.wang@intel.com>
> Cc: Bie, Tiwei <tiwei.bie@intel.com>; dev@dpdk.org;
> stephen@networkplumber.org
> Subject: Re: [PATCH v6 2/3] net/virtio: add packet injection method
>
> On Sun, Jan 07, 2018 at 04:05:12AM -0800, Xiao Wang wrote:
> > + /*
> > + * 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;
>
> Why not turning the "started" to atomic type, so that you don't need
> the lock?
>
> --yliu
To avoid impacting datapath performance, this patch doesn't change "started" to atomic
type.
During the interrupt handler routine, there are a series of instructions between lock acquire and release. An atomic value is not suitable for this scenario.
BRs,
Xiao
> -----Original Message-----
> From: Wang, Xiao W
> Sent: Monday, January 8, 2018 11:12 PM
> To: Yuanhan Liu <yliu@fridaylinux.org>
> Cc: Bie, Tiwei <tiwei.bie@intel.com>; dev@dpdk.org;
> stephen@networkplumber.org
> Subject: RE: [PATCH v6 2/3] net/virtio: add packet injection method
>
>
>
> > -----Original Message-----
> > From: Yuanhan Liu [mailto:yliu@fridaylinux.org]
> > Sent: Monday, January 8, 2018 9:04 PM
> > To: Wang, Xiao W <xiao.w.wang@intel.com>
> > Cc: Bie, Tiwei <tiwei.bie@intel.com>; dev@dpdk.org;
> > stephen@networkplumber.org
> > Subject: Re: [PATCH v6 2/3] net/virtio: add packet injection method
> >
> > On Sun, Jan 07, 2018 at 04:05:12AM -0800, Xiao Wang wrote:
> > > + /*
> > > + * 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;
> >
> > Why not turning the "started" to atomic type, so that you don't need
> > the lock?
> >
> During the interrupt handler routine, there are a series of instructions
> between lock acquire and release. An atomic value is not suitable for this
> scenario.
>
The current comment may doesn't explain the state_lock correctly, this lock needs to be acquired in dev_pause and released in dev_resume, so it's not just used to protect the "started" value.
I would improve the comment as " App management thread and virtio interrupt handler thread both can change device state, ..."
Thanks for comments,
Xiao
v7:
- Improve comment for state_lock.
- Rename spinlock variable 'sl' to 'lock'.
v6:
- Use rte_pktmbuf_alloc() instead of rte_mbuf_raw_alloc().
- Remove the 'len' parameter in calling virtio_send_command().
- Remove extra space between typo and var.
- Improve comment and alignment.
- Remove the unnecessary header file.
- A better usage of 'unlikely' indication.
v5:
- Remove txvq parameter in virtio_inject_pkts.
- Zero hw->special_buf after using it.
- Return the retval of tx_pkt_burst().
- Allocate a mbuf pointer on stack directly.
v4:
- Move spinlock lock/unlock into dev_pause/resume.
- Separate out a patch for packet injection.
v3:
- Remove Tx function code duplication, use a special pointer for rarp
injection.
- Rename function generate_rarp to virtio_notify_peers, replace
'virtnet_' with 'virtio_'.
- Add comment for state_lock.
- Typo fix and comment improvement.
v2:
- Use spaces instead of tabs between the code and comments.
- Remove unnecessary parentheses.
- Use rte_pktmbuf_mtod directly to get eth_hdr addr.
- Fix virtio_dev_pause return value check.
Xiao Wang (3):
net/virtio: make control queue thread-safe
net/virtio: add packet injection method
net/virtio: support GUEST ANNOUNCE
drivers/net/virtio/virtio_ethdev.c | 158 +++++++++++++++++++++++++++++++-
drivers/net/virtio/virtio_ethdev.h | 6 ++
drivers/net/virtio/virtio_pci.h | 7 ++
drivers/net/virtio/virtio_rxtx.c | 3 +-
drivers/net/virtio/virtio_rxtx.h | 1 +
drivers/net/virtio/virtio_rxtx_simple.c | 2 +-
drivers/net/virtio/virtqueue.h | 11 +++
7 files changed, 183 insertions(+), 5 deletions(-)
@@ -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,57 @@ virtio_negotiate_features(struct virtio_hw *hw, uint64_t req_features)
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) {
+ /* Device is just stopped. */
+ rte_spinlock_unlock(&hw->state_lock);
+ return -1;
+ }
+ hw->started = 0;
+ /*
+ * Prevent the worker threads 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 the worker threads 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);
+}
+
+/*
+ * Should be called only after device is paused.
+ */
+int
+virtio_inject_pkts(struct rte_eth_dev *dev, struct rte_mbuf **tx_pkts,
+ int nb_pkts)
+{
+ struct virtio_hw *hw = dev->data->dev_private;
+ struct virtnet_tx *txvq = dev->data->tx_queues[0];
+ int ret;
+
+ hw->inject_pkts = tx_pkts;
+ ret = dev->tx_pkt_burst(txvq, tx_pkts, nb_pkts);
+ hw->inject_pkts = NULL;
+
+ return ret;
+}
+
/*
* Process Virtio Config changed interrupt and call the callback
* if link state changed.
@@ -1786,6 +1838,8 @@ virtio_dev_configure(struct rte_eth_dev *dev)
return -EBUSY;
}
+ rte_spinlock_init(&hw->state_lock);
+
hw->use_simple_rx = 1;
hw->use_simple_tx = 1;
@@ -1952,12 +2006,14 @@ virtio_dev_stop(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
@@ -121,4 +121,9 @@ int eth_virtio_dev_init(struct rte_eth_dev *eth_dev);
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 **tx_pkts,
+ int nb_pkts);
+
#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_pkts;
struct virtqueue **vqs;
};
@@ -1017,7 +1017,7 @@ virtio_xmit_pkts(void *tx_queue, struct rte_mbuf **tx_pkts, uint16_t nb_pkts)
uint16_t nb_used, nb_tx = 0;
int error;
- if (unlikely(hw->started == 0))
+ if (unlikely(hw->started == 0 && tx_pkts != hw->inject_pkts))
return nb_tx;
if (unlikely(nb_pkts < 1))
@@ -99,7 +99,7 @@ virtio_xmit_pkts_simple(void *tx_queue, struct rte_mbuf **tx_pkts,
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_pkts))
return nb_tx;
nb_used = VIRTQUEUE_NUSED(vq);