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

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

On 5/5/2016 11:46 AM, Yuanhan Liu wrote:
> On Thu, May 05, 2016 at 03:29:44AM +0000, Xie, Huawei wrote:
>> 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.
> Well, if you insist, I'm Okay. But please don't do it in the way this
> patch does, that introduces quite many duplicated codes.

Yuanhan, I have no insist.

Our target is 1) remove the queue type if else checking in the
virtio_dev_queue_setup 2) extract the common setup for vq and call them
in specific RX/TX/CQ setup.
For 2, which is really meaningful to me is the queue size retrieve,
queue allocation

What I mean is firstly we split the queue, without breaking the common
setup; then introduce RX/TX specific setup calling extracted common
setup, so we don't have a chance to introduce duplicated code.


> 	--yliu
>