Re: [dpdk-dev] [PATCH] virtio: split virtio rx/tx queue

["Xie, Huawei" <huawei.xie@intel.com>]

On 5/5/2016 11:03 AM, Yuanhan Liu wrote:
> 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.

To your later comment, we could split first, then do the queue setup rework.

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