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. The switch/case in the common queue setup looks bad. I am aware of the common operations, and i had planned to extract them, maybe i could do this in this patchset. > > 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. > >> static void >> -virtio_update_packet_stats(struct virtqueue *vq, struct rte_mbuf *mbuf) >> +virtio_update_rxq_stats(struct virtnet_rx *rxvq, struct rte_mbuf *mbuf) >> { >> uint32_t s = mbuf->pkt_len; >> struct ether_addr *ea; >> >> if (s == 64) { >> - vq->size_bins[1]++; >> + rxvq->stats.size_bins[1]++; >> } else if (s > 64 && s < 1024) { >> uint32_t bin; >> >> /* count zeros, and offset into correct bin */ >> bin = (sizeof(s) * 8) - __builtin_clz(s) - 5; >> - vq->size_bins[bin]++; >> + rxvq->stats.size_bins[bin]++; >> } else { >> if (s < 64) >> - vq->size_bins[0]++; >> + rxvq->stats.size_bins[0]++; >> else if (s < 1519) >> - vq->size_bins[6]++; >> + rxvq->stats.size_bins[6]++; >> else if (s >= 1519) >> - vq->size_bins[7]++; >> + rxvq->stats.size_bins[7]++; >> } >> >> ea = rte_pktmbuf_mtod(mbuf, struct ether_addr *); >> if (is_multicast_ether_addr(ea)) { >> if (is_broadcast_ether_addr(ea)) >> - vq->broadcast++; >> + rxvq->stats.broadcast++; >> else >> - vq->multicast++; >> + rxvq->stats.multicast++; >> + } >> +} >> + >> +static void >> +virtio_update_txq_stats(struct virtnet_tx *txvq, struct rte_mbuf *mbuf) > Why not taking "struct virtnet_stats *stats" as the arg, so that we > don't have to implment two exactly same functions. ok to me. > >> diff --git a/drivers/net/virtio/virtio_rxtx.h >> b/drivers/net/virtio/virtio_rxtx.h >> index a76c3e5..ced55a3 100644 >> --- a/drivers/net/virtio/virtio_rxtx.h >> +++ b/drivers/net/virtio/virtio_rxtx.h >> @@ -34,7 +34,59 @@ >> #define RTE_PMD_VIRTIO_RX_MAX_BURST 64 >> >> #ifdef RTE_MACHINE_CPUFLAG_SSSE3 >> -int virtio_rxq_vec_setup(struct virtqueue *rxq); >> + >> +struct virtnet_stats { > Another remind again: you should put following codes before the > "#ifdef". > > --yliu >