From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <huawei.xie@intel.com>
Received: from mga01.intel.com (mga01.intel.com [192.55.52.88])
 by dpdk.org (Postfix) with ESMTP id 339F5B62
 for <dev@dpdk.org>; Mon, 21 Dec 2015 13:25:55 +0100 (CET)
Received: from orsmga001.jf.intel.com ([10.7.209.18])
 by fmsmga101.fm.intel.com with ESMTP; 21 Dec 2015 04:25:54 -0800
X-ExtLoop1: 1
X-IronPort-AV: E=Sophos;i="5.20,459,1444719600"; d="scan'208";a="845732380"
Received: from fmsmsx105.amr.corp.intel.com ([10.18.124.203])
 by orsmga001.jf.intel.com with ESMTP; 21 Dec 2015 04:25:53 -0800
Received: from shsmsx151.ccr.corp.intel.com (10.239.6.50) by
 FMSMSX105.amr.corp.intel.com (10.18.124.203) with Microsoft SMTP Server (TLS)
 id 14.3.248.2; Mon, 21 Dec 2015 04:25:53 -0800
Received: from shsmsx101.ccr.corp.intel.com ([169.254.1.190]) by
 SHSMSX151.ccr.corp.intel.com ([169.254.3.92]) with mapi id 14.03.0248.002;
 Mon, 21 Dec 2015 20:25:51 +0800
From: "Xie, Huawei" <huawei.xie@intel.com>
To: Stephen Hemminger <stephen@networkplumber.org>, "Ananyev, Konstantin"
 <konstantin.ananyev@intel.com>
Thread-Topic: [dpdk-dev] [PATCH v2 1/2] mbuf: provide rte_pktmbuf_alloc_bulk
 API
Thread-Index: AdE76rnZ5k4AFZq3TfmHtb59BK82Zg==
Date: Mon, 21 Dec 2015 12:25:50 +0000
Message-ID: <C37D651A908B024F974696C65296B57B4C54D6BC@SHSMSX101.ccr.corp.intel.com>
References: <1450049754-33635-1-git-send-email-huawei.xie@intel.com>
 <1450055682-51953-1-git-send-email-huawei.xie@intel.com>
 <1450055682-51953-2-git-send-email-huawei.xie@intel.com>
 <20151217210114.534a7561@xeon-e3>
 <2601191342CEEE43887BDE71AB97725836AD5AEB@irsmsx105.ger.corp.intel.com>
 <20151218093206.35e2e3e4@xeon-e3>
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
Cc: "dev@dpdk.org" <dev@dpdk.org>
Subject: Re: [dpdk-dev] [PATCH v2 1/2] mbuf: provide rte_pktmbuf_alloc_bulk
 API
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, 21 Dec 2015 12:25:55 -0000

On 12/19/2015 1:32 AM, Stephen Hemminger wrote:=0A=
> On Fri, 18 Dec 2015 10:44:02 +0000=0A=
> "Ananyev, Konstantin" <konstantin.ananyev@intel.com> wrote:=0A=
>=0A=
>>=0A=
>>> -----Original Message-----=0A=
>>> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Stephen Hemminger=
=0A=
>>> Sent: Friday, December 18, 2015 5:01 AM=0A=
>>> To: Xie, Huawei=0A=
>>> Cc: dev@dpdk.org=0A=
>>> Subject: Re: [dpdk-dev] [PATCH v2 1/2] mbuf: provide rte_pktmbuf_alloc_=
bulk API=0A=
>>>=0A=
>>> On Mon, 14 Dec 2015 09:14:41 +0800=0A=
>>> Huawei Xie <huawei.xie@intel.com> wrote:=0A=
>>>=0A=
>>>> v2 changes:=0A=
>>>>  unroll the loop a bit to help the performance=0A=
>>>>=0A=
>>>> rte_pktmbuf_alloc_bulk allocates a bulk of packet mbufs.=0A=
>>>>=0A=
>>>> There is related thread about this bulk API.=0A=
>>>> http://dpdk.org/dev/patchwork/patch/4718/=0A=
>>>> Thanks to Konstantin's loop unrolling.=0A=
>>>>=0A=
>>>> Signed-off-by: Gerald Rogers <gerald.rogers@intel.com>=0A=
>>>> Signed-off-by: Huawei Xie <huawei.xie@intel.com>=0A=
>>>> Acked-by: Konstantin Ananyev <konstantin.ananyev@intel.com>=0A=
>>>> ---=0A=
>>>>  lib/librte_mbuf/rte_mbuf.h | 50 +++++++++++++++++++++++++++++++++++++=
+++++++++=0A=
>>>>  1 file changed, 50 insertions(+)=0A=
>>>>=0A=
>>>> diff --git a/lib/librte_mbuf/rte_mbuf.h b/lib/librte_mbuf/rte_mbuf.h=
=0A=
>>>> index f234ac9..4e209e0 100644=0A=
>>>> --- a/lib/librte_mbuf/rte_mbuf.h=0A=
>>>> +++ b/lib/librte_mbuf/rte_mbuf.h=0A=
>>>> @@ -1336,6 +1336,56 @@ static inline struct rte_mbuf *rte_pktmbuf_allo=
c(struct rte_mempool *mp)=0A=
>>>>  }=0A=
>>>>=0A=
>>>>  /**=0A=
>>>> + * Allocate a bulk of mbufs, initialize refcnt and reset the fields t=
o default=0A=
>>>> + * values.=0A=
>>>> + *=0A=
>>>> + *  @param pool=0A=
>>>> + *    The mempool from which mbufs are allocated.=0A=
>>>> + *  @param mbufs=0A=
>>>> + *    Array of pointers to mbufs=0A=
>>>> + *  @param count=0A=
>>>> + *    Array size=0A=
>>>> + *  @return=0A=
>>>> + *   - 0: Success=0A=
>>>> + */=0A=
>>>> +static inline int rte_pktmbuf_alloc_bulk(struct rte_mempool *pool,=0A=
>>>> +	 struct rte_mbuf **mbufs, unsigned count)=0A=
>>>> +{=0A=
>>>> +	unsigned idx =3D 0;=0A=
>>>> +	int rc;=0A=
>>>> +=0A=
>>>> +	rc =3D rte_mempool_get_bulk(pool, (void **)mbufs, count);=0A=
>>>> +	if (unlikely(rc))=0A=
>>>> +		return rc;=0A=
>>>> +=0A=
>>>> +	switch (count % 4) {=0A=
>>>> +	while (idx !=3D count) {=0A=
>>>> +		case 0:=0A=
>>>> +			RTE_MBUF_ASSERT(rte_mbuf_refcnt_read(mbufs[idx]) =3D=3D 0);=0A=
>>>> +			rte_mbuf_refcnt_set(mbufs[idx], 1);=0A=
>>>> +			rte_pktmbuf_reset(mbufs[idx]);=0A=
>>>> +			idx++;=0A=
>>>> +		case 3:=0A=
>>>> +			RTE_MBUF_ASSERT(rte_mbuf_refcnt_read(mbufs[idx]) =3D=3D 0);=0A=
>>>> +			rte_mbuf_refcnt_set(mbufs[idx], 1);=0A=
>>>> +			rte_pktmbuf_reset(mbufs[idx]);=0A=
>>>> +			idx++;=0A=
>>>> +		case 2:=0A=
>>>> +			RTE_MBUF_ASSERT(rte_mbuf_refcnt_read(mbufs[idx]) =3D=3D 0);=0A=
>>>> +			rte_mbuf_refcnt_set(mbufs[idx], 1);=0A=
>>>> +			rte_pktmbuf_reset(mbufs[idx]);=0A=
>>>> +			idx++;=0A=
>>>> +		case 1:=0A=
>>>> +			RTE_MBUF_ASSERT(rte_mbuf_refcnt_read(mbufs[idx]) =3D=3D 0);=0A=
>>>> +			rte_mbuf_refcnt_set(mbufs[idx], 1);=0A=
>>>> +			rte_pktmbuf_reset(mbufs[idx]);=0A=
>>>> +			idx++;=0A=
>>>> +	}=0A=
>>>> +	}=0A=
>>>> +	return 0;=0A=
>>>> +}=0A=
>>> This is weird. Why not just use Duff's device in a more normal manner.=
=0A=
>> But it is a sort of Duff's method.=0A=
>> Not sure what looks weird to you here?=0A=
>> while () {} instead of do {} while();?=0A=
>> Konstantin=0A=
>>=0A=
>>=0A=
>>=0A=
> It is unusual to have cases not associated with block of the switch.=0A=
> Unusual to me means, "not used commonly in most code".=0A=
>=0A=
> Since you are jumping into the loop, might make more sense as a do { } wh=
ile()=0A=
>=0A=
Stephen:=0A=
How about we move while a bit:=0A=
    switch(count % 4) {=0A=
    case 0: while (idx !=3D count) {=0A=
            ... reset ...=0A=
    case 3:=0A=
            ... reset ...=0A=
    case 2:=0A=
            ... reset ...=0A=
    case 1:=0A=
            ... reset ...=0A=
     }=0A=
     }=0A=
=0A=
With do {} while, we probably need one more extra check on if count is=0A=
zero. Duff's initial implementation assumes that count isn't zero. With=0A=
while loop, we save one line of code.=0A=
=0A=