[dpdk-dev,v4,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 | 44 +++++++++++++++++++++++++++++++++
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, 59 insertions(+), 2 deletions(-)
Comments
On Thu, Jan 04, 2018 at 07:59:37AM -0800, Xiao Wang wrote:
[...]
> +void
> +virtio_inject_pkts(struct rte_eth_dev *dev, struct virtnet_tx *txvq,
> + struct rte_mbuf **buf, int count)
> +{
> + struct virtio_hw *hw = dev->data->dev_private;
> +
> + hw->special_buf = buf;
I think maybe you can give it (hw->special_buf) a
better (more meaningful) name.
You don't need to put txvq in the param list based
on your current implementation. Otherwise, you need
to make special_buf be per-queue variable.
> + dev->tx_pkt_burst(txvq, buf, count);
> +}
> +
You need to zero the hw->special_buf after calling
tx_pkt_burst().
You should also return the retval of tx_pkt_burst()
to the caller.
Thanks,
Tiwei
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 | 155 +++++++++++++++++++++++++++++++-
drivers/net/virtio/virtio_ethdev.h | 7 ++
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, 181 insertions(+), 5 deletions(-)
On Thu, 4 Jan 2018 07:59:37 -0800
Xiao Wang <xiao.w.wang@intel.com> wrote:
> 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>
Why is this needed? It isn't obvious what the mechanism is trying to solve.
On Fri, Jan 05, 2018 at 12:27:37PM -0800, Stephen Hemminger wrote:
> On Thu, 4 Jan 2018 07:59:37 -0800
> Xiao Wang <xiao.w.wang@intel.com> wrote:
>
> > 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>
>
> Why is this needed? It isn't obvious what the mechanism is trying to solve.
Xiao needs to use such mechanism to send some special packets
(RARP packet) in the interrupt handler to implement the GUEST
ANNOUNCE feature.
To avoid the contention between user's Tx threads and the
interrupt thread. He needs to pause user's Tx threads (by
reusing the existing 'started' flag in `virtio_hw`) first,
and call tx_burst() to send the RARP packet.
He already provided the pause() and resume() functions, but
the implementation of sending the RARP packet (add a field
named as `rarp_buf` in `virtio_hw`, and check it in tx_burst()
functions) is too specific. So I just suggested him to give
rarp_buf a more generic name and provide a simple wrapper of
tx_burst for internal use:
http://dpdk.org/ml/archives/dev/2018-January/085213.html
Is it OK to you? Or do you have any other suggestions? Thanks!
PS. The latest version is v5, below is the link:
http://dpdk.org/ml/archives/dev/2018-January/085354.html
Best regards,
Tiwei Bie
@@ -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,45 @@ 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) {
+ 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;
+}
+
+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);
+}
+
+void
+virtio_inject_pkts(struct rte_eth_dev *dev, struct virtnet_tx *txvq,
+ struct rte_mbuf **buf, int count)
+{
+ struct virtio_hw *hw = dev->data->dev_private;
+
+ hw->special_buf = buf;
+ dev->tx_pkt_burst(txvq, buf, count);
+}
+
/*
* Process Virtio Config changed interrupt and call the callback
* if link state changed.
@@ -1786,6 +1826,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 +1994,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);
+void virtio_inject_pkts(struct rte_eth_dev *dev,
+ struct virtnet_tx *txvq, 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 **special_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->special_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->special_buf)
return nb_tx;
nb_used = VIRTQUEUE_NUSED(vq);