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 A388BDE0 for ; Thu, 5 May 2016 03:54:30 +0200 (CEST) Received: from orsmga003.jf.intel.com ([10.7.209.27]) by orsmga103.jf.intel.com with ESMTP; 04 May 2016 18:54:29 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.24,580,1455004800"; d="scan'208";a="799170866" Received: from fmsmsx103.amr.corp.intel.com ([10.18.124.201]) by orsmga003.jf.intel.com with ESMTP; 04 May 2016 18:54:29 -0700 Received: from fmsmsx115.amr.corp.intel.com (10.18.116.19) by FMSMSX103.amr.corp.intel.com (10.18.124.201) with Microsoft SMTP Server (TLS) id 14.3.248.2; Wed, 4 May 2016 18:54:28 -0700 Received: from shsmsx103.ccr.corp.intel.com (10.239.4.69) by fmsmsx115.amr.corp.intel.com (10.18.116.19) with Microsoft SMTP Server (TLS) id 14.3.248.2; Wed, 4 May 2016 18:54:28 -0700 Received: from shsmsx101.ccr.corp.intel.com ([169.254.1.148]) by SHSMSX103.ccr.corp.intel.com ([169.254.4.58]) with mapi id 14.03.0248.002; Thu, 5 May 2016 09:54:26 +0800 From: "Xie, Huawei" To: Yuanhan Liu CC: "dev@dpdk.org" Thread-Topic: [dpdk-dev] [PATCH] virtio: split virtio rx/tx queue Thread-Index: AdGmcQ0ysEggt5/7Q9GTakcbXYug1Q== Date: Thu, 5 May 2016 01:54:25 +0000 Message-ID: References: <1462323027-91942-1-git-send-email-huawei.xie@intel.com> <20160505000327.GT5641@yliu-dev.sh.intel.com> Accept-Language: en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-originating-ip: [10.239.127.40] Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 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: Thu, 05 May 2016 01:54:31 -0000 On 5/5/2016 7:59 AM, Yuanhan Liu wrote:=0A= > On Wed, May 04, 2016 at 08:50:27AM +0800, Huawei Xie wrote:=0A= >> -int virtio_dev_queue_setup(struct rte_eth_dev *dev,=0A= >> - int queue_type,=0A= >> - uint16_t queue_idx,=0A= >> +static int=0A= >> +virtio_dev_cq_queue_setup(struct rte_eth_dev *dev,=0A= > While it's good to split Rx/Tx specific stuff, but why are you trying to= =0A= > remove a common queue_setup function that does common setups, such as vri= ng=0A= > memory allocation.=0A= >=0A= > This results to much duplicated code: following diff summary also shows= =0A= > it clearly:=0A= =0A= The motivation to do this is we need separate RX/TX queue setup.=0A= The switch/case in the common queue setup looks bad.=0A= =0A= I am aware of the common operations, and i had planned to extract them,=0A= maybe i could do this in this patchset.=0A= =0A= =0A= >=0A= > 7 files changed, 655 insertions(+), 422 deletions(-)=0A= >=0A= > which makes it harder for maintaining.=0A= >=0A= >> -}=0A= >> + rxvq =3D (struct virtnet_rx *)RTE_PTR_ADD(vq,=0A= >> + sizeof(*vq) + vq_size * sizeof(struct vq_desc_extra));=0A= >> + rxvq->vq =3D vq;=0A= >> + vq->sw_ring =3D sw_ring;=0A= > sw_ring is needed for rx queue only, why not moving it to rx queue struct= ?=0A= =0A= Actually this is not about sw_ring.=0A= I had planned to use sw_ring for both RX/TX and remove the vq_desc_extra.= =0A= Two issues=0A= 1. RX uses both sw_ring and vq_desc_extra=0A= 2. ndescs in vq_desc_extra isn't really needed, we could simply=0A= calculate this when we walk through the desc chain, and in most cases,=0A= it is 1 or 2.=0A= =0A= As it is not related to this rework, will do this in a separate patch.=0A= =0A= >=0A= >> static void=0A= >> -virtio_update_packet_stats(struct virtqueue *vq, struct rte_mbuf *mbuf)= =0A= >> +virtio_update_rxq_stats(struct virtnet_rx *rxvq, struct rte_mbuf *mbuf)= =0A= >> {=0A= >> uint32_t s =3D mbuf->pkt_len;=0A= >> struct ether_addr *ea;=0A= >> =0A= >> if (s =3D=3D 64) {=0A= >> - vq->size_bins[1]++;=0A= >> + rxvq->stats.size_bins[1]++;=0A= >> } else if (s > 64 && s < 1024) {=0A= >> uint32_t bin;=0A= >> =0A= >> /* count zeros, and offset into correct bin */=0A= >> bin =3D (sizeof(s) * 8) - __builtin_clz(s) - 5;=0A= >> - vq->size_bins[bin]++;=0A= >> + rxvq->stats.size_bins[bin]++;=0A= >> } else {=0A= >> if (s < 64)=0A= >> - vq->size_bins[0]++;=0A= >> + rxvq->stats.size_bins[0]++;=0A= >> else if (s < 1519)=0A= >> - vq->size_bins[6]++;=0A= >> + rxvq->stats.size_bins[6]++;=0A= >> else if (s >=3D 1519)=0A= >> - vq->size_bins[7]++;=0A= >> + rxvq->stats.size_bins[7]++;=0A= >> }=0A= >> =0A= >> ea =3D rte_pktmbuf_mtod(mbuf, struct ether_addr *);=0A= >> if (is_multicast_ether_addr(ea)) {=0A= >> if (is_broadcast_ether_addr(ea))=0A= >> - vq->broadcast++;=0A= >> + rxvq->stats.broadcast++;=0A= >> else=0A= >> - vq->multicast++;=0A= >> + rxvq->stats.multicast++;=0A= >> + }=0A= >> +}=0A= >> +=0A= >> +static void=0A= >> +virtio_update_txq_stats(struct virtnet_tx *txvq, struct rte_mbuf *mbuf)= =0A= > Why not taking "struct virtnet_stats *stats" as the arg, so that we=0A= > don't have to implment two exactly same functions.=0A= =0A= ok to me.=0A= =0A= >=0A= >> diff --git a/drivers/net/virtio/virtio_rxtx.h b/drivers/net/virtio/virti= o_rxtx.h=0A= >> index a76c3e5..ced55a3 100644=0A= >> --- a/drivers/net/virtio/virtio_rxtx.h=0A= >> +++ b/drivers/net/virtio/virtio_rxtx.h=0A= >> @@ -34,7 +34,59 @@=0A= >> #define RTE_PMD_VIRTIO_RX_MAX_BURST 64=0A= >> =0A= >> #ifdef RTE_MACHINE_CPUFLAG_SSSE3=0A= >> -int virtio_rxq_vec_setup(struct virtqueue *rxq);=0A= >> +=0A= >> +struct virtnet_stats {=0A= > Another remind again: you should put following codes before the=0A= > "#ifdef".=0A= >=0A= > --yliu=0A= >=0A= =0A=