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 >
