From mboxrd@z Thu Jan 1 00:00:00 1970
Return-Path: <huawei.xie@intel.com>
Received: from mga14.intel.com (mga14.intel.com [192.55.52.115])
by dpdk.org (Postfix) with ESMTP id 782E05A8B
for <dev@dpdk.org>; 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" <huawei.xie@intel.com>
To: Yuanhan Liu <yuanhan.liu@linux.intel.com>
CC: "dev@dpdk.org" <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: <C37D651A908B024F974696C65296B57B4C7BDFEA@SHSMSX101.ccr.corp.intel.com>
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>
<C37D651A908B024F974696C65296B57B4C7BBCB6@SHSMSX101.ccr.corp.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 <dev.dpdk.org>
List-Unsubscribe: <http://dpdk.org/ml/options/dev>,
<mailto:dev-request@dpdk.org?subject=unsubscribe>
List-Archive: <http://dpdk.org/ml/archives/dev/>
List-Post: <mailto:dev@dpdk.org>
List-Help: <mailto:dev-request@dpdk.org?subject=help>
List-Subscribe: <http://dpdk.org/ml/listinfo/dev>,
<mailto:dev-request@dpdk.org?subject=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: <PRIx64>=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=