From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga14.intel.com (mga14.intel.com [192.55.52.115]) by dpdk.org (Postfix) with ESMTP id 782E05A8B for ; Mon, 30 May 2016 10:17:16 +0200 (CEST) Received: from orsmga003.jf.intel.com ([10.7.209.27]) by fmsmga103.fm.intel.com with ESMTP; 30 May 2016 01:17:15 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.26,388,1459839600"; d="scan'208";a="817584410" Received: from fmsmsx107.amr.corp.intel.com ([10.18.124.205]) by orsmga003.jf.intel.com with ESMTP; 30 May 2016 01:17:15 -0700 Received: from shsmsx151.ccr.corp.intel.com (10.239.6.50) by fmsmsx107.amr.corp.intel.com (10.18.124.205) with Microsoft SMTP Server (TLS) id 14.3.248.2; Mon, 30 May 2016 01:17:14 -0700 Received: from shsmsx101.ccr.corp.intel.com ([169.254.1.150]) by SHSMSX151.ccr.corp.intel.com ([169.254.3.193]) with mapi id 14.03.0248.002; Mon, 30 May 2016 16:17:03 +0800 From: "Xie, Huawei" To: Yuanhan Liu CC: "dev@dpdk.org" Thread-Topic: [dpdk-dev] [PATCH] virtio: split virtio rx/tx queue Thread-Index: AdG6HI+JBnSf06fSRheil2QVfceXwg== Date: Mon, 30 May 2016 08:17:02 +0000 Message-ID: References: <1462323027-91942-1-git-send-email-huawei.xie@intel.com> <1464097112-47411-1-git-send-email-huawei.xie@intel.com> <20160527090756.GC5641@yliu-dev.sh.intel.com> <20160530030316.GD5641@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: Mon, 30 May 2016 08:17:17 -0000 On 5/30/2016 11:00 AM, Yuanhan Liu wrote:=0A= > On Mon, May 30, 2016 at 02:40:00AM +0000, Xie, Huawei wrote:=0A= >> On 5/27/2016 5:06 PM, Yuanhan Liu wrote:=0A= >>> On Tue, May 24, 2016 at 09:38:32PM +0800, Huawei Xie wrote:=0A= >>>> vq->vq_ring_mem =3D mz->phys_addr;=0A= >>>> vq->vq_ring_virt_mem =3D mz->addr;=0A= >>>> - PMD_INIT_LOG(DEBUG, "vq->vq_ring_mem: 0x%"PRIx64, (uint64_t)mz-= >phys_addr);=0A= >>>> - PMD_INIT_LOG(DEBUG, "vq->vq_ring_virt_mem: 0x%"PRIx64, (uint64_t)(ui= ntptr_t)mz->addr);=0A= >>>> - vq->virtio_net_hdr_mz =3D NULL;=0A= >>>> - vq->virtio_net_hdr_mem =3D 0;=0A= >>>> + PMD_INIT_LOG(DEBUG, "vq->vq_ring_mem: 0x%"PRIx64,=0A= >>>> + (uint64_t)mz->phys_addr);=0A= >>>> + PMD_INIT_LOG(DEBUG, "vq->vq_ring_virt_mem: 0x%"PRIx64,=0A= >>>> + (uint64_t)(uintptr_t)mz->addr);=0A= >>>> +=0A= >>>> + hdr_mz =3D rte_memzone_reserve_aligned(vq_hdr_name, sz_hdr_mz, socke= t_id,=0A= >>>> + 0, RTE_CACHE_LINE_SIZE);=0A= >>> We don't need allocate hdr_mz for Rx queue, and in such case, sz_hdr_mz= =0A= >>> is 0. I'm wondering what hdr_mz would be then, NULL?=0A= >>>=0A= >>> Anyway, you should skip the hdr_mz allocation for Rx queue, and I also= =0A= >>> would suggest you to move the vq_hdr_name setup here.=0A= >> will check sz_hdr_mz before the zone allocation.=0A= >>=0A= >>=0A= >>>> + if (hdr_mz =3D=3D NULL) {=0A= >>>> + if (rte_errno =3D=3D EEXIST)=0A= >>>> + hdr_mz =3D rte_memzone_lookup(vq_hdr_name);=0A= >>>> + if (hdr_mz =3D=3D NULL) {=0A= >>>> + ret =3D -ENOMEM;=0A= >>>> + goto fail_q_alloc;=0A= >>>> + }=0A= >>>> + }=0A= >>>> =0A= >>> ...=0A= >>>> =0A= >>>> PMD_INIT_FUNC_TRACE();=0A= >>>> ret =3D virtio_dev_queue_setup(dev, VTNET_CQ, VTNET_SQ_CQ_QUEUE_IDX,= =0A= >>>> - vtpci_queue_idx, 0, socket_id, &vq);=0A= >>>> + vtpci_queue_idx, 0, socket_id, (void **)&cvq);=0A= >>> Unnecessary cast. Note that there are few others like that in this=0A= >>> patch.=0A= >> This cast is needed.=0A= > Oh, right, indeed. Sorry.=0A= >=0A= >>>> - PMD_RX_LOG(DEBUG, "dequeue:%d", num);=0A= >>>> - PMD_RX_LOG(DEBUG, "packet len:%d", len[0]);=0A= >>>> + PMD_RX_LOG(DEBUG, "dequeue:%d\n", num);=0A= >>>> + PMD_RX_LOG(DEBUG, "packet len:%d\n", len[0]);=0A= >>> We should not append "\n" for PMD_RX_LOG; this macro alreadys does it.= =0A= >> Weird. Will remove it. Thanks.=0A= >>=0A= >>> Another note is that you might want to run checkpatch; I saw quite many= =0A= >>> warnings.=0A= >> Had checked. The warnings are all due to 80 char limitation of=0A= >> virtio_rxq_stats_strings. Just 4 or 5 chars cross 80 line limit. I=0A= >> prefer to keep the fields aligned.=0A= > Agreed. However, I was talking about others warnings.=0A= =0A= ok, i see you are using checkpatch of newer Linux kernel.=0A= =0A= =0A= >=0A= > --yliu=0A= >=0A= > ---=0A= > CHECK:BRACES: braces {} should be used on all arms of this statement=0A= > #198: FILE: drivers/net/virtio/virtio_ethdev.c:343:=0A= =0A= would fix this.=0A= =0A= > + if (queue_type =3D=3D VTNET_RQ)=0A= > [...]=0A= > + else if (queue_type =3D=3D VTNET_TQ) {=0A= > [...]=0A= > + } else if (queue_type =3D=3D VTNET_CQ) {=0A= > [...]=0A= >=0A= > CHECK:CAMELCASE: Avoid CamelCase: =0A= > #280: FILE: drivers/net/virtio/virtio_ethdev.c:404:=0A= > + PMD_INIT_LOG(DEBUG, "vq->vq_ring_mem: 0x%"PRIx64,=0A= >=0A= > CHECK:CONCATENATED_STRING: Concatenated strings should use spaces=0A= > between elements=0A= > #280: FILE: drivers/net/virtio/virtio_ethdev.c:404:=0A= > + PMD_INIT_LOG(DEBUG, "vq->vq_ring_mem: 0x%"PRIx64,=0A= >=0A= > CHECK:CONCATENATED_STRING: Concatenated strings should use spaces=0A= > between elements=0A= > #282: FILE: drivers/net/virtio/virtio_ethdev.c:406:=0A= > + PMD_INIT_LOG(DEBUG, "vq->vq_ring_virt_mem: 0x%"PRIx64,=0A= >=0A= > WARNING:UNSPECIFIED_INT: Prefer 'unsigned int' to bare use of 'unsigned'= =0A= > #482: FILE: drivers/net/virtio/virtio_ethdev.c:753:=0A= > + unsigned nstats =3D dev->data->nb_tx_queues * VIRTIO_NB_TXQ_XSTAT= S=0A= > +=0A= >=0A= > WARNING:LONG_LINE: line over 80 characters=0A= > #551: FILE: drivers/net/virtio/virtio_ethdev.c:819:=0A= > + memset(txvq->stats.size_bins, 0,=0A= > sizeof(txvq->stats.size_bins[0]) * 8);=0A= >=0A= > WARNING:LONG_LINE: line over 80 characters=0A= > #571: FILE: drivers/net/virtio/virtio_ethdev.c:832:=0A= > + memset(rxvq->stats.size_bins, 0,=0A= > sizeof(rxvq->stats.size_bins[0]) * 8);=0A= =0A= would fix this.=0A= =0A= >=0A= > CHECK:MULTIPLE_ASSIGNMENTS: multiple assignments should be avoided=0A= > #1529: FILE: drivers/net/virtio/virtio_rxtx_simple.c:362:=0A= > + nb_commit =3D nb_pkts =3D RTE_MIN((vq->vq_free_cnt >> 1), nb_pkts= );=0A= >=0A= > CHECK:SPACING: No space is necessary after a cast=0A= > #1530: FILE: drivers/net/virtio/virtio_rxtx_simple.c:363:=0A= > + desc_idx =3D (uint16_t) (vq->vq_avail_idx & desc_idx_max);=0A= =0A= All other warnings are due to the newer checkpatch script, and not=0A= introduced by this patch, so wouldn't fix in this patch.=0A= But i think some are better programming practice, like 'Concatenated=0A= strings should use spaces between elements', so would post a patch to=0A= fix them in virtio driver (if possible, all other drivers).=0A= =0A= =0A=