From mboxrd@z Thu Jan 1 00:00:00 1970
Return-Path: <dev-bounces@dpdk.org>
Received: from dpdk.org (dpdk.org [92.243.14.124])
by inbox.dpdk.org (Postfix) with ESMTP id 0BA6EA04C7;
Fri, 18 Sep 2020 06:41:14 +0200 (CEST)
Received: from [92.243.14.124] (localhost [127.0.0.1])
by dpdk.org (Postfix) with ESMTP id F18131D703;
Fri, 18 Sep 2020 06:41:12 +0200 (CEST)
Received: from mga01.intel.com (mga01.intel.com [192.55.52.88])
by dpdk.org (Postfix) with ESMTP id 2A4581D6D7
for <dev@dpdk.org>; Fri, 18 Sep 2020 06:41:10 +0200 (CEST)
IronPort-SDR: g5Cck7OKt4llcgCjn3iZ8h3bbT91hxxY+4DrwbFssAljjM7OpPmT8OyW/NTWTEU6TybhcMDdid
PgyzVdLDdX/g==
X-IronPort-AV: E=McAfee;i="6000,8403,9747"; a="177953084"
X-IronPort-AV: E=Sophos;i="5.77,273,1596524400"; d="scan'208";a="177953084"
X-Amp-Result: SKIPPED(no attachment in message)
X-Amp-File-Uploaded: False
Received: from fmsmga007.fm.intel.com ([10.253.24.52])
by fmsmga101.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384;
17 Sep 2020 21:41:09 -0700
IronPort-SDR: CnmA75GtSPsxiSjt7BeA+67lf0fKmSPsfZEfb91F+G06rVxz2lyLuhLdAhMdowY7ewBU6sRdyE
iztXRKAWUGmg==
X-ExtLoop1: 1
X-IronPort-AV: E=Sophos;i="5.77,273,1596524400"; d="scan'208";a="287833188"
Received: from fmsmsx602.amr.corp.intel.com ([10.18.126.82])
by fmsmga007.fm.intel.com with ESMTP; 17 Sep 2020 21:41:09 -0700
Received: from shsmsx601.ccr.corp.intel.com (10.109.6.141) by
fmsmsx602.amr.corp.intel.com (10.18.126.82) with Microsoft SMTP Server
(version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256) id
15.1.1713.5; Thu, 17 Sep 2020 21:41:08 -0700
Received: from shsmsx601.ccr.corp.intel.com (10.109.6.141) by
SHSMSX601.ccr.corp.intel.com (10.109.6.141) with Microsoft SMTP Server
(version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256) id
15.1.1713.5; Fri, 18 Sep 2020 12:41:06 +0800
Received: from shsmsx601.ccr.corp.intel.com ([10.109.6.141]) by
SHSMSX601.ccr.corp.intel.com ([10.109.6.141]) with mapi id 15.01.1713.004;
Fri, 18 Sep 2020 12:41:06 +0800
From: "Guo, Jia" <jia.guo@intel.com>
To: "Zhang, Qi Z" <qi.z.zhang@intel.com>, "Yang, Qiming"
<qiming.yang@intel.com>, "Xing, Beilei" <beilei.xing@intel.com>, "Wu,
Jingjing" <jingjing.wu@intel.com>, "Wang, Haiyue" <haiyue.wang@intel.com>
CC: "Zhao1, Wei" <wei.zhao1@intel.com>, "Richardson, Bruce"
<bruce.richardson@intel.com>, "dev@dpdk.org" <dev@dpdk.org>, "Zhang, Helin"
<helin.zhang@intel.com>, "mb@smartsharesystems.com"
<mb@smartsharesystems.com>, "Yigit, Ferruh" <ferruh.yigit@intel.com>,
"stephen@networkplumber.org" <stephen@networkplumber.org>, "barbette@kth.se"
<barbette@kth.se>, "Han, YingyaX" <yingyax.han@intel.com>
Thread-Topic: [PATCH v4 4/5] net/ice: fix vector rx burst for ice
Thread-Index: AQHWjMjH2B1LKjMdsECgm//ok5rFe6lsJJ+AgAGOBOD//4jXgIAAlBlg
Date: Fri, 18 Sep 2020 04:41:06 +0000
Message-ID: <a84c13d48b864b7fbac32f125918763b@intel.com>
References: <20200827075452.1751-1-jia.guo@intel.com>
<20200917075834.60034-1-jia.guo@intel.com>
<20200917075834.60034-5-jia.guo@intel.com>
<a4c65097e1ca42609c74b020d48a5d3f@intel.com>
<ced8ec343f8a4de9812d92c62298bb6a@intel.com>
<3600603ea748465c85e28f71befcfa9e@intel.com>
In-Reply-To: <3600603ea748465c85e28f71befcfa9e@intel.com>
Accept-Language: en-US
Content-Language: en-US
X-MS-Has-Attach:
X-MS-TNEF-Correlator:
dlp-product: dlpe-windows
dlp-reaction: no-action
dlp-version: 11.5.1.3
x-originating-ip: [10.239.127.36]
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0
Subject: Re: [dpdk-dev] [PATCH v4 4/5] net/ice: fix vector rx burst for ice
X-BeenThere: dev@dpdk.org
X-Mailman-Version: 2.1.15
Precedence: list
List-Id: DPDK patches and discussions <dev.dpdk.org>
List-Unsubscribe: <https://mails.dpdk.org/options/dev>,
<mailto:dev-request@dpdk.org?subject=unsubscribe>
List-Archive: <http://mails.dpdk.org/archives/dev/>
List-Post: <mailto:dev@dpdk.org>
List-Help: <mailto:dev-request@dpdk.org?subject=help>
List-Subscribe: <https://mails.dpdk.org/listinfo/dev>,
<mailto:dev-request@dpdk.org?subject=subscribe>
Errors-To: dev-bounces@dpdk.org
Sender: "dev" <dev-bounces@dpdk.org>
> -----Original Message-----
> From: Zhang, Qi Z <qi.z.zhang@intel.com>
> Sent: Friday, September 18, 2020 11:41 AM
> To: Guo, Jia <jia.guo@intel.com>; Yang, Qiming <qiming.yang@intel.com>;
> Xing, Beilei <beilei.xing@intel.com>; Wu, Jingjing <jingjing.wu@intel.com=
>;
> Wang, Haiyue <haiyue.wang@intel.com>
> Cc: Zhao1, Wei <wei.zhao1@intel.com>; Richardson, Bruce
> <bruce.richardson@intel.com>; dev@dpdk.org; Zhang, Helin
> <helin.zhang@intel.com>; mb@smartsharesystems.com; Yigit, Ferruh
> <ferruh.yigit@intel.com>; stephen@networkplumber.org; barbette@kth.se;
> Han, YingyaX <yingyax.han@intel.com>
> Subject: RE: [PATCH v4 4/5] net/ice: fix vector rx burst for ice
>=20
>=20
>=20
> > -----Original Message-----
> > From: Guo, Jia <jia.guo@intel.com>
> > Sent: Friday, September 18, 2020 11:20 AM
> > To: Zhang, Qi Z <qi.z.zhang@intel.com>; Yang, Qiming
> > <qiming.yang@intel.com>; Xing, Beilei <beilei.xing@intel.com>; Wu,
> > Jingjing <jingjing.wu@intel.com>; Wang, Haiyue <haiyue.wang@intel.com>
> > Cc: Zhao1, Wei <wei.zhao1@intel.com>; Richardson, Bruce
> > <bruce.richardson@intel.com>; dev@dpdk.org; Zhang, Helin
> > <helin.zhang@intel.com>; mb@smartsharesystems.com; Yigit, Ferruh
> > <ferruh.yigit@intel.com>; stephen@networkplumber.org;
> barbette@kth.se;
> > Han, YingyaX <yingyax.han@intel.com>
> > Subject: RE: [PATCH v4 4/5] net/ice: fix vector rx burst for ice
> >
> > Hi, qi
> >
> > > -----Original Message-----
> > > From: Zhang, Qi Z <qi.z.zhang@intel.com>
> > > Sent: Thursday, September 17, 2020 7:03 PM
> > > To: Guo, Jia <jia.guo@intel.com>; Yang, Qiming
> > > <qiming.yang@intel.com>; Xing, Beilei <beilei.xing@intel.com>; Wu,
> > > Jingjing <jingjing.wu@intel.com>; Wang, Haiyue
> > > <haiyue.wang@intel.com>
> > > Cc: Zhao1, Wei <wei.zhao1@intel.com>; Richardson, Bruce
> > > <bruce.richardson@intel.com>; dev@dpdk.org; Zhang, Helin
> > > <helin.zhang@intel.com>; mb@smartsharesystems.com; Yigit, Ferruh
> > > <ferruh.yigit@intel.com>; stephen@networkplumber.org;
> > > barbette@kth.se; Han, YingyaX <yingyax.han@intel.com>
> > > Subject: RE: [PATCH v4 4/5] net/ice: fix vector rx burst for ice
> > >
> > >
> > >
> > > > -----Original Message-----
> > > > From: Guo, Jia <jia.guo@intel.com>
> > > > Sent: Thursday, September 17, 2020 3:59 PM
> > > > To: Yang, Qiming <qiming.yang@intel.com>; Xing, Beilei
> > > > <beilei.xing@intel.com>; Zhang, Qi Z <qi.z.zhang@intel.com>; Wu,
> > > > Jingjing <jingjing.wu@intel.com>; Wang, Haiyue
> > > > <haiyue.wang@intel.com>
> > > > Cc: Zhao1, Wei <wei.zhao1@intel.com>; Richardson, Bruce
> > > > <bruce.richardson@intel.com>; dev@dpdk.org; Guo, Jia
> > > > <jia.guo@intel.com>; Zhang, Helin <helin.zhang@intel.com>;
> > > > mb@smartsharesystems.com; Yigit, Ferruh <ferruh.yigit@intel.com>;
> > > > stephen@networkplumber.org; barbette@kth.se; Han, YingyaX
> > > > <yingyax.han@intel.com>
> > > > Subject: [PATCH v4 4/5] net/ice: fix vector rx burst for ice
> > > >
> > > > The limitation of burst size in vector rx was removed, since it
> > > > should retrieve as much received packets as possible. And also the
> > > > scattered receive path should use a wrapper function to achieve
> > > > the goal of burst maximizing. And do some code cleaning for vector =
rx
> path.
> > > >
> > > > Bugzilla ID: 516
> > > > Fixes: c68a52b8b38c ("net/ice: support vector SSE in Rx")
> > > > Fixes: ae60d3c9b227 ("net/ice: support Rx AVX2 vector")
> > > >
> > > > Signed-off-by: Jeff Guo <jia.guo@intel.com>
> > > > Tested-by: Yingya Han <yingyax.han@intel.com>
> > > > ---
> > > > drivers/net/ice/ice_rxtx.h | 1 +
> > > > drivers/net/ice/ice_rxtx_vec_avx2.c | 23 ++++++------
> > > > drivers/net/ice/ice_rxtx_vec_sse.c | 56
> > > > +++++++++++++++++++----------
> > > > 3 files changed, 49 insertions(+), 31 deletions(-)
> > > >
> > > > diff --git a/drivers/net/ice/ice_rxtx.h
> > > > b/drivers/net/ice/ice_rxtx.h index 2fdcfb7d0..3ef5f300d 100644
> > > > --- a/drivers/net/ice/ice_rxtx.h
> > > > +++ b/drivers/net/ice/ice_rxtx.h
> > > > @@ -35,6 +35,7 @@
> > > > #define ICE_MAX_RX_BURST ICE_RXQ_REARM_THRESH
> > > > #define ICE_TX_MAX_FREE_BUF_SZ 64
> > > > #define ICE_DESCS_PER_LOOP 4
> > > > +#define ICE_DESCS_PER_LOOP_AVX 8
> > >
> > > No need to expose this if no external link, better to keep all avx
> > > stuff inside avx.c
> > >
> >
> > Ok, so define it in avx.c is the best choice if avx should not in rxtx.=
h.
> >
> > > >
> > > > #define ICE_FDIR_PKT_LEN 512
> > > >
> > > > diff --git a/drivers/net/ice/ice_rxtx_vec_avx2.c
> > > > b/drivers/net/ice/ice_rxtx_vec_avx2.c
> > > > index be50677c2..843e4f32a 100644
> > > > --- a/drivers/net/ice/ice_rxtx_vec_avx2.c
> > > > +++ b/drivers/net/ice/ice_rxtx_vec_avx2.c
> > > > @@ -29,7 +29,7 @@ ice_rxq_rearm(struct ice_rx_queue *rxq)
> > > > __m128i dma_addr0;
> > > >
> > > > dma_addr0 =3D _mm_setzero_si128();
> > > > - for (i =3D 0; i < ICE_DESCS_PER_LOOP; i++) {
> > > > + for (i =3D 0; i < ICE_DESCS_PER_LOOP_AVX; i++) {
> > > > rxep[i].mbuf =3D &rxq->fake_mbuf;
> > > > _mm_store_si128((__m128i *)&rxdp[i].read,
> > > > dma_addr0);
> > > > @@ -132,12 +132,17 @@ ice_rxq_rearm(struct ice_rx_queue *rxq)
> > > > ICE_PCI_REG_WRITE(rxq->qrx_tail, rx_id); }
> > > >
> > > > +/**
> > > > + * vPMD raw receive routine, only accept(nb_pkts >=3D
> > > > +ICE_DESCS_PER_LOOP_AVX)
> > > > + *
> > > > + * Notice:
> > > > + * - nb_pkts < ICE_DESCS_PER_LOOP_AVX, just return no packet
> > > > + * - floor align nb_pkts to a ICE_DESCS_PER_LOOP_AVX power-of-two
> > > > +*/
> > >
> > > The comment is misleading, it looks like we are going to floor align
> > > nb_pkts to 2^8, better to reword .
> > >
> >
> > It should be, agree.
> >
> > > > static inline uint16_t
> > > > _ice_recv_raw_pkts_vec_avx2(struct ice_rx_queue *rxq, struct
> > > > rte_mbuf **rx_pkts,
> > > > uint16_t nb_pkts, uint8_t *split_packet) { -#define
> > > > ICE_DESCS_PER_LOOP_AVX 8
> > > > -
> > > > const uint32_t *ptype_tbl =3D rxq->vsi->adapter->ptype_tbl;
> > > > const __m256i mbuf_init =3D _mm256_set_epi64x(0, 0,
> > > > 0, rxq->mbuf_initializer);
> > > > @@ -603,10 +608,6 @@ _ice_recv_raw_pkts_vec_avx2(struct
> > > ice_rx_queue
> > > > *rxq, struct rte_mbuf **rx_pkts,
> > > > return received;
> > > > }
> > > >
> > > > -/*
> > > > - * Notice:
> > > > - * - nb_pkts < ICE_DESCS_PER_LOOP, just return no packet
> > > > - */
> > > > uint16_t
> > > > ice_recv_pkts_vec_avx2(void *rx_queue, struct rte_mbuf **rx_pkts,
> > > > uint16_t nb_pkts)
> > > > @@ -616,8 +617,6 @@ ice_recv_pkts_vec_avx2(void *rx_queue, struct
> > > > rte_mbuf **rx_pkts,
> > > >
> > > > /**
> > > > * vPMD receive routine that reassembles single burst of 32
> > > > scattered packets
> > > > - * Notice:
> > > > - * - nb_pkts < ICE_DESCS_PER_LOOP, just return no packet
> > > > */
> > >
> > > Why we need to remove this? is it still true for this function?
> > >
> >
> > The reason is that this comment is in the calling function "
> > _ice_recv_raw_pkts_vec_avx2" which process the related thing, no need
> > to add it more and more in the caller function.
>=20
> I think you remove related comment from the calling function also :)
>=20
> Also I think better to keep this even it's a little bit duplicate, that h=
elp people
> to understand the internal logic
>=20
> >
> > > > static uint16_t
> > > > ice_recv_scattered_burst_vec_avx2(void *rx_queue, struct rte_mbuf
> > > > **rx_pkts, @@ -626,6 +625,9 @@
> > > ice_recv_scattered_burst_vec_avx2(void
> > > > *rx_queue, struct rte_mbuf **rx_pkts,
> > > > struct ice_rx_queue *rxq =3D rx_queue;
> > > > uint8_t split_flags[ICE_VPMD_RX_BURST] =3D {0};
> > > >
> > > > + /* split_flags only can support max of ICE_VPMD_RX_BURST */
> > > > + nb_pkts =3D RTE_MIN(nb_pkts, ICE_VPMD_RX_BURST);
> > >
> > > Is this necessary? the only consumer of this function is
> > > ice_recv_scattered_pkts_vec_avx2, I think nb_pkts <=3D
> > > ICE_VPMD_RX_BURST it already be guaranteed.
> >
> > The reason is that we remove "nb_pkts <=3D ICE_VPMD_RX_BURST" and in
> > this function split_flags have a limit for ICE_VPMD_RX_BURST, so a
> > checking is need in the function.
>=20
> Can't get this, could tell me is there any case that nb_pkts >
> ICE_VPMD_RX_BURST?
>=20
I know we just set the hard value here and only one case usage, but I think=
only the caller know what would be the input param, but the calling should=
not know the input param will be, even there is no any caller but the call=
ing still need to be complete. =20
>=20
> >
> > > > +
> > > > /* get some new buffers */
> > > > uint16_t nb_bufs =3D _ice_recv_raw_pkts_vec_avx2(rxq, rx_pkts,
> > > nb_pkts,
> > > > split_flags);
> > > > @@ -657,9 +659,6 @@ ice_recv_scattered_burst_vec_avx2(void
> > > *rx_queue,
> > > > struct rte_mbuf **rx_pkts,
> > > >
> > > > /**
> > > > * vPMD receive routine that reassembles scattered packets.
> > > > - * Main receive routine that can handle arbitrary burst sizes
> > > > - * Notice:
> > > > - * - nb_pkts < ICE_DESCS_PER_LOOP, just return no packet
> > > > */
> > >
> > > Why we need to remove this? isn't it the main routine that be able
> > > to handle arbitrary burst size?
> > >
> >
> > The question is why we need to said the arbitrary sizes if we process
> > and return what we could receive packet for maximum? It is not only
> > useless comment but also maybe bring some confuse I think.
>=20
> Yes arbitrary size description can be removed, as this is assumed to be t=
he
> default behavior.
> But the description for nb_pkts should still be kept.
>=20
> >
> > > Btw, I will suggest all AVX2 changes can be in a separate patch,
> > > because this looks like some code clean and fix.
> > > its not related with the main purpose of the patch set.
> >
> > I consider it and ask any objection before, so totally I am not
> > disagree on separate it, but I think if the purpose of the patch set
> > is to clean some misleading for vec(sse/avx) burst, it could still be
> > on a set even separate it to patch.
>=20
> I will not be insist on patch separate, but if you separate them, some of=
fixes
> can be merged early and no need to wait for those part need more review.
Ok, seems that there still something discuss on the code cleaning patch, le=
t me separate it for better review.