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

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

On Thu, May 05, 2016 at 01:54:25AM +0000, Xie, Huawei wrote:
> On 5/5/2016 7:59 AM, Yuanhan Liu wrote:
> > 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:
> 
> The motivation to do this is we need separate RX/TX queue setup.

We actually have done that. If you look at current rx/tx/ctrl_queue_setup()
code, we invoked the common function; we also did some queue specific
settings. It has not been done in a very clean way though: there are quite
many "if .. else .." as you stated. And that's what you are going to resolve,
but IMO, you went far: you made __same__ code 3 copies, one for rx, tx and
ctrl queue, respectively.

> The switch/case in the common queue setup looks bad.

Assuming you are talking about the "if .. else .." ...

While I agree with you on that, introducing so many duplicated code is worse.

> I am aware of the common operations, and i had planned to extract them,
> maybe i could do this in this patchset.

If you meant to do in another patch on top of this patch, then it looks
like the wrong way to go: breaking something first and then fixing it
later does not sound a good practice to me.

> >
> >     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?
> 
> Actually this is not about sw_ring.
> I had planned to use sw_ring for both RX/TX and remove the vq_desc_extra.
> Two issues
> 1. RX uses both sw_ring and vq_desc_extra
> 2. ndescs in vq_desc_extra isn't really needed, we could simply
> calculate this when we walk through the desc chain, and in most cases,
> it is 1 or 2.
> 
> As it is not related to this rework, will do this in a separate patch.

Yes, it's not related to this patch, and this patch does rx/tx split
only. So, thinking that sw_ring is for rx only, you should move there.

It will not against with your plan; you can make corresponding change
there. But for this patch, let's do the split only.

BTW, I still would suggest you to build the patch on top of the cleanup
and memory leak fix patches from Jianfeng. Your patch won't apply on
top of current dpdk-next-virtio, and one way or another, you need do
a rebase.

Last, if I were you, I would split this patch in two: one to move
the queue specific settings to it's queue setup function, another
to split rx/tx fields. That would make it easier for review.

	--yliu