Re: [PATCH] virtio: split virtio rx/tx queue

[Yuanhan Liu <yuanhan.liu@linux.intel.com>]

On Wed, May 04, 2016 at 08:50:27AM +0800, Huawei Xie wrote:
> -int virtio_dev_queue_setup(struct rte_eth_dev *dev,
> -			int queue_type,
> -			uint16_t queue_idx,
> +static int
> +virtio_dev_cq_queue_setup(struct rte_eth_dev *dev,

While it's good to split Rx/Tx specific stuff, but why are you trying to
remove a common queue_setup function that does common setups, such as vring
memory allocation.

This results to much duplicated code: following diff summary also shows
it clearly:

    7 files changed, 655 insertions(+), 422 deletions(-)

which makes it harder for maintaining.

> -}
> +	rxvq = (struct virtnet_rx *)RTE_PTR_ADD(vq,
> +			sizeof(*vq) + vq_size * sizeof(struct vq_desc_extra));
> +	rxvq->vq = vq;
> +	vq->sw_ring = sw_ring;

sw_ring is needed for rx queue only, why not moving it to rx queue struct?

>  static void
> -virtio_update_packet_stats(struct virtqueue *vq, struct rte_mbuf *mbuf)
> +virtio_update_rxq_stats(struct virtnet_rx *rxvq, struct rte_mbuf *mbuf)
>  {
>  	uint32_t s = mbuf->pkt_len;
>  	struct ether_addr *ea;
>  
>  	if (s == 64) {
> -		vq->size_bins[1]++;
> +		rxvq->stats.size_bins[1]++;
>  	} else if (s > 64 && s < 1024) {
>  		uint32_t bin;
>  
>  		/* count zeros, and offset into correct bin */
>  		bin = (sizeof(s) * 8) - __builtin_clz(s) - 5;
> -		vq->size_bins[bin]++;
> +		rxvq->stats.size_bins[bin]++;
>  	} else {
>  		if (s < 64)
> -			vq->size_bins[0]++;
> +			rxvq->stats.size_bins[0]++;
>  		else if (s < 1519)
> -			vq->size_bins[6]++;
> +			rxvq->stats.size_bins[6]++;
>  		else if (s >= 1519)
> -			vq->size_bins[7]++;
> +			rxvq->stats.size_bins[7]++;
>  	}
>  
>  	ea = rte_pktmbuf_mtod(mbuf, struct ether_addr *);
>  	if (is_multicast_ether_addr(ea)) {
>  		if (is_broadcast_ether_addr(ea))
> -			vq->broadcast++;
> +			rxvq->stats.broadcast++;
>  		else
> -			vq->multicast++;
> +			rxvq->stats.multicast++;
> +	}
> +}
> +
> +static void
> +virtio_update_txq_stats(struct virtnet_tx *txvq, struct rte_mbuf *mbuf)

Why not taking "struct virtnet_stats *stats" as the arg, so that we
don't have to implment two exactly same functions.


> diff --git a/drivers/net/virtio/virtio_rxtx.h b/drivers/net/virtio/virtio_rxtx.h
> index a76c3e5..ced55a3 100644
> --- a/drivers/net/virtio/virtio_rxtx.h
> +++ b/drivers/net/virtio/virtio_rxtx.h
> @@ -34,7 +34,59 @@
>  #define RTE_PMD_VIRTIO_RX_MAX_BURST 64
>  
>  #ifdef RTE_MACHINE_CPUFLAG_SSSE3
> -int virtio_rxq_vec_setup(struct virtqueue *rxq);
> +
> +struct virtnet_stats {

Another remind again: you should put following codes before the
"#ifdef".

	--yliu