From mboxrd@z Thu Jan 1 00:00:00 1970
Return-Path: <huawei.xie@intel.com>
Received: from mga03.intel.com (mga03.intel.com [134.134.136.65])
by dpdk.org (Postfix) with ESMTP id A388BDE0
for <dev@dpdk.org>; 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" <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: AdGmcQ0ysEggt5/7Q9GTakcbXYug1Q==
Date: Thu, 5 May 2016 01:54:25 +0000
Message-ID: <C37D651A908B024F974696C65296B57B4C74C637@SHSMSX101.ccr.corp.intel.com>
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 <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: 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=