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

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

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.

	--yliu