Hi,

-----Original Message-----
From: Bie, Tiwei
Sent: Saturday, January 6, 2018 2:01 AM
To: Wang, Xiao W <redacted>
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:
[...]

quoted

+/*
+ * 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.

quoted

+	struct virtio_hw *hw = dev->data->dev_private;
+	struct virtnet_tx *txvq = dev->data->tx_queues[0];
+	int ret;

[...]

quoted

diff --git a/drivers/net/virtio/virtio_ethdev.h

b/drivers/net/virtio/virtio_ethdev.h

quoted

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.

quoted

 #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,

quoted

 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.

quoted

+

[...]

quoted

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