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 >
