[dpdk-dev] [PATCH v5 2/3] net/virtio: add packet injection method

[Wang, Xiao W]

Hi,

> -----Original Message-----
> From: Bie, Tiwei
> Sent: Saturday, January 6, 2018 2:01 AM
> To: Wang, Xiao W <xiao.w.wang at intel.com>
> Cc: dev at dpdk.org; yliu at fridaylinux.org; stephen at 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