From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga03.intel.com (mga03.intel.com [134.134.136.65]) by dpdk.org (Postfix) with ESMTP id 48A352B8B for ; Thu, 5 May 2016 01:59:43 +0200 (CEST) Received: from fmsmga004.fm.intel.com ([10.253.24.48]) by orsmga103.jf.intel.com with ESMTP; 04 May 2016 16:59:42 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.24,579,1455004800"; d="scan'208";a="97427015" Received: from yliu-dev.sh.intel.com (HELO yliu-dev) ([10.239.67.162]) by fmsmga004.fm.intel.com with ESMTP; 04 May 2016 16:59:42 -0700 Date: Wed, 4 May 2016 17:03:27 -0700 From: Yuanhan Liu To: Huawei Xie Cc: dev@dpdk.org Message-ID: <20160505000327.GT5641@yliu-dev.sh.intel.com> References: <1462323027-91942-1-git-send-email-huawei.xie@intel.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1462323027-91942-1-git-send-email-huawei.xie@intel.com> User-Agent: Mutt/1.5.23 (2014-03-12) Subject: Re: [dpdk-dev] [PATCH] virtio: split virtio rx/tx queue X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: patches and discussions about DPDK List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 04 May 2016 23:59:43 -0000 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: 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? > 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. > 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