Re: [PATCH v5 2/3] net/virtio: add packet injection method

From: Tiwei Bie <tiwei.bie@intel.com>
To: Xiao Wang <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
Date: Sat, 6 Jan 2018 02:00:55 +0800	[thread overview]
Message-ID: <20180105180055.iqqd4km2m33jbuwc@debian-xvivbkq.sh.intel.com> (raw)
In-Reply-To: <1515170817-136539-3-git-send-email-xiao.w.wang@intel.com>

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