[dpdk-dev] [PATCH v2 1/2] mbuf: provide rte_pktmbuf_alloc_bulk API

2015-12-22 Thread Xie, Huawei 
On 12/22/2015 5:32 AM, Thomas Monjalon wrote:
> 2015-12-21 17:20, Wiles, Keith:
>> On 12/21/15, 9:21 AM, "Xie, Huawei"  wrote:
>>> On 12/19/2015 3:27 AM, Wiles, Keith wrote:
 On 12/18/15, 11:32 AM, "dev on behalf of Stephen Hemminger" >>> at dpdk.org on behalf of stephen at networkplumber.org> wrote:
> On Fri, 18 Dec 2015 10:44:02 +
> "Ananyev, Konstantin"  wrote:
>> From: dev [mailto:dev-bounces at dpdk.org] On Behalf Of Stephen Hemminger
>>> On Mon, 14 Dec 2015 09:14:41 +0800
>>> Huawei Xie  wrote:
 +  switch (count % 4) {
 +  while (idx != count) {
 +  case 0:
 +  
 RTE_MBUF_ASSERT(rte_mbuf_refcnt_read(mbufs[idx]) == 0);
 +  rte_mbuf_refcnt_set(mbufs[idx], 1);
 +  rte_pktmbuf_reset(mbufs[idx]);
 +  idx++;
 +  case 3:
 +  
 RTE_MBUF_ASSERT(rte_mbuf_refcnt_read(mbufs[idx]) == 0);
 +  rte_mbuf_refcnt_set(mbufs[idx], 1);
 +  rte_pktmbuf_reset(mbufs[idx]);
 +  idx++;
 +  case 2:
 +  
 RTE_MBUF_ASSERT(rte_mbuf_refcnt_read(mbufs[idx]) == 0);
 +  rte_mbuf_refcnt_set(mbufs[idx], 1);
 +  rte_pktmbuf_reset(mbufs[idx]);
 +  idx++;
 +  case 1:
 +  
 RTE_MBUF_ASSERT(rte_mbuf_refcnt_read(mbufs[idx]) == 0);
 +  rte_mbuf_refcnt_set(mbufs[idx], 1);
 +  rte_pktmbuf_reset(mbufs[idx]);
 +  idx++;
 +  }
 +  }
 +  return 0;
 +}
>>> This is weird. Why not just use Duff's device in a more normal manner.
>> But it is a sort of Duff's method.
>> Not sure what looks weird to you here?
>> while () {} instead of do {} while();?
>> Konstantin
>>
>>
>>
> It is unusual to have cases not associated with block of the switch.
> Unusual to me means, "not used commonly in most code".
>
> Since you are jumping into the loop, might make more sense as a do { } 
> while()
 I find this a very odd coding practice and I would suggest we not do this, 
 unless it gives us some great performance gain.

 Keith
>>> The loop unwinding could give performance gain. The only problem is the
>>> switch/loop combination makes people feel weird at the first glance but
>>> soon they will grasp this style. Since this is inherited from old famous
>>> duff's device, i prefer to keep this style which saves lines of code.
>> Please add a comment to the code to reflex where this style came from and 
>> why you are using it, would be very handy here.
> +1
> At least the words "loop" and "unwinding" may be helpful to some readers.
OK. Will add more context. Probably the wiki page for duff's device
should be updated on how to handle the case count is zero, using while()
or add one line to check.

> Thanks
>

[dpdk-dev] [PATCH v2 1/2] mbuf: provide rte_pktmbuf_alloc_bulk API

2015-12-21 Thread Don Provan 
>From: Xie, Huawei [mailto:huawei.xie at intel.com] 
>Sent: Monday, December 21, 2015 7:22 AM
>Subject: Re: [dpdk-dev] [PATCH v2 1/2] mbuf: provide rte_pktmbuf_alloc_bulk API
>
>The loop unwinding could give performance gain. The only problem is the 
>switch/loop
>combination makes people feel weird at the first glance but soon they will 
>grasp this style.
>Since this is inherited from old famous duff's device, i prefer to keep this 
>style which saves
>lines of code.

You don't really mean "lines of code", of course, since it increases the lines 
of code.
It reduces the number of branches.

Is Duff's Device used in other "bulk" routines? If not, what justifies making 
this a special case?

-don provan
dprovan at bivio.net

[dpdk-dev] [PATCH v2 1/2] mbuf: provide rte_pktmbuf_alloc_bulk API

2015-12-21 Thread Thomas Monjalon 
2015-12-21 17:20, Wiles, Keith:
> On 12/21/15, 9:21 AM, "Xie, Huawei"  wrote:
> >On 12/19/2015 3:27 AM, Wiles, Keith wrote:
> >> On 12/18/15, 11:32 AM, "dev on behalf of Stephen Hemminger"  >> at dpdk.org on behalf of stephen at networkplumber.org> wrote:
> >>> On Fri, 18 Dec 2015 10:44:02 +
> >>> "Ananyev, Konstantin"  wrote:
>  From: dev [mailto:dev-bounces at dpdk.org] On Behalf Of Stephen Hemminger
> > On Mon, 14 Dec 2015 09:14:41 +0800
> > Huawei Xie  wrote:
> >> +  switch (count % 4) {
> >> +  while (idx != count) {
> >> +  case 0:
> >> +  
> >> RTE_MBUF_ASSERT(rte_mbuf_refcnt_read(mbufs[idx]) == 0);
> >> +  rte_mbuf_refcnt_set(mbufs[idx], 1);
> >> +  rte_pktmbuf_reset(mbufs[idx]);
> >> +  idx++;
> >> +  case 3:
> >> +  
> >> RTE_MBUF_ASSERT(rte_mbuf_refcnt_read(mbufs[idx]) == 0);
> >> +  rte_mbuf_refcnt_set(mbufs[idx], 1);
> >> +  rte_pktmbuf_reset(mbufs[idx]);
> >> +  idx++;
> >> +  case 2:
> >> +  
> >> RTE_MBUF_ASSERT(rte_mbuf_refcnt_read(mbufs[idx]) == 0);
> >> +  rte_mbuf_refcnt_set(mbufs[idx], 1);
> >> +  rte_pktmbuf_reset(mbufs[idx]);
> >> +  idx++;
> >> +  case 1:
> >> +  
> >> RTE_MBUF_ASSERT(rte_mbuf_refcnt_read(mbufs[idx]) == 0);
> >> +  rte_mbuf_refcnt_set(mbufs[idx], 1);
> >> +  rte_pktmbuf_reset(mbufs[idx]);
> >> +  idx++;
> >> +  }
> >> +  }
> >> +  return 0;
> >> +}
> > This is weird. Why not just use Duff's device in a more normal manner.
>  But it is a sort of Duff's method.
>  Not sure what looks weird to you here?
>  while () {} instead of do {} while();?
>  Konstantin
> 
> 
> 
> >>> It is unusual to have cases not associated with block of the switch.
> >>> Unusual to me means, "not used commonly in most code".
> >>>
> >>> Since you are jumping into the loop, might make more sense as a do { } 
> >>> while()
> >> I find this a very odd coding practice and I would suggest we not do this, 
> >> unless it gives us some great performance gain.
> >>
> >> Keith
> >The loop unwinding could give performance gain. The only problem is the
> >switch/loop combination makes people feel weird at the first glance but
> >soon they will grasp this style. Since this is inherited from old famous
> >duff's device, i prefer to keep this style which saves lines of code.
> 
> Please add a comment to the code to reflex where this style came from and why 
> you are using it, would be very handy here.

+1
At least the words "loop" and "unwinding" may be helpful to some readers.
Thanks

[dpdk-dev] [PATCH v2 1/2] mbuf: provide rte_pktmbuf_alloc_bulk API

2015-12-21 Thread Wiles, Keith 
On 12/21/15, 9:21 AM, "Xie, Huawei"  wrote:

>On 12/19/2015 3:27 AM, Wiles, Keith wrote:
>> On 12/18/15, 11:32 AM, "dev on behalf of Stephen Hemminger" > dpdk.org on behalf of stephen at networkplumber.org> wrote:
>>
>>> On Fri, 18 Dec 2015 10:44:02 +
>>> "Ananyev, Konstantin"  wrote:
>>>
>>>>
>>>>> -Original Message-
>>>>> From: dev [mailto:dev-bounces at dpdk.org] On Behalf Of Stephen Hemminger
>>>>> Sent: Friday, December 18, 2015 5:01 AM
>>>>> To: Xie, Huawei
>>>>> Cc: dev at dpdk.org
>>>>> Subject: Re: [dpdk-dev] [PATCH v2 1/2] mbuf: provide 
>>>>> rte_pktmbuf_alloc_bulk API
>>>>>
>>>>> On Mon, 14 Dec 2015 09:14:41 +0800
>>>>> Huawei Xie  wrote:
>>>>>
>>>>>> v2 changes:
>>>>>>  unroll the loop a bit to help the performance
>>>>>>
>>>>>> rte_pktmbuf_alloc_bulk allocates a bulk of packet mbufs.
>>>>>>
>>>>>> There is related thread about this bulk API.
>>>>>> http://dpdk.org/dev/patchwork/patch/4718/
>>>>>> Thanks to Konstantin's loop unrolling.
>>>>>>
>>>>>> Signed-off-by: Gerald Rogers 
>>>>>> Signed-off-by: Huawei Xie 
>>>>>> Acked-by: Konstantin Ananyev 
>>>>>> ---
>>>>>>  lib/librte_mbuf/rte_mbuf.h | 50 
>>>>>> ++
>>>>>>  1 file changed, 50 insertions(+)
>>>>>>
>>>>>> diff --git a/lib/librte_mbuf/rte_mbuf.h b/lib/librte_mbuf/rte_mbuf.h
>>>>>> index f234ac9..4e209e0 100644
>>>>>> --- a/lib/librte_mbuf/rte_mbuf.h
>>>>>> +++ b/lib/librte_mbuf/rte_mbuf.h
>>>>>> @@ -1336,6 +1336,56 @@ static inline struct rte_mbuf 
>>>>>> *rte_pktmbuf_alloc(struct rte_mempool *mp)
>>>>>>  }
>>>>>>
>>>>>>  /**
>>>>>> + * Allocate a bulk of mbufs, initialize refcnt and reset the fields to 
>>>>>> default
>>>>>> + * values.
>>>>>> + *
>>>>>> + *  @param pool
>>>>>> + *The mempool from which mbufs are allocated.
>>>>>> + *  @param mbufs
>>>>>> + *Array of pointers to mbufs
>>>>>> + *  @param count
>>>>>> + *Array size
>>>>>> + *  @return
>>>>>> + *   - 0: Success
>>>>>> + */
>>>>>> +static inline int rte_pktmbuf_alloc_bulk(struct rte_mempool *pool,
>>>>>> + struct rte_mbuf **mbufs, unsigned count)
>>>>>> +{
>>>>>> +unsigned idx = 0;
>>>>>> +int rc;
>>>>>> +
>>>>>> +rc = rte_mempool_get_bulk(pool, (void **)mbufs, count);
>>>>>> +if (unlikely(rc))
>>>>>> +return rc;
>>>>>> +
>>>>>> +switch (count % 4) {
>>>>>> +while (idx != count) {
>>>>>> +case 0:
>>>>>> +
>>>>>> RTE_MBUF_ASSERT(rte_mbuf_refcnt_read(mbufs[idx]) == 0);
>>>>>> +rte_mbuf_refcnt_set(mbufs[idx], 1);
>>>>>> +rte_pktmbuf_reset(mbufs[idx]);
>>>>>> +idx++;
>>>>>> +case 3:
>>>>>> +
>>>>>> RTE_MBUF_ASSERT(rte_mbuf_refcnt_read(mbufs[idx]) == 0);
>>>>>> +rte_mbuf_refcnt_set(mbufs[idx], 1);
>>>>>> +rte_pktmbuf_reset(mbufs[idx]);
>>>>>> +idx++;
>>>>>> +case 2:
>>>>>> +
>>>>>> RTE_MBUF_ASSERT(rte_mbuf_refcnt_read(mbufs[idx]) == 0);
>>>>>> +rte_mbuf_refcnt_set(mbufs[idx], 1);
>>>>>> +rte_pktmbuf_reset(mbufs[idx]);
>>>>>> +idx++;
>>>>>> +case 1:
>>>>>> +
>>>>>> RTE_MBUF_ASSERT(rte_mbuf_refcnt_read(mbufs[idx]) == 0);
>>>>>> +rte_mbuf_refcnt_set(mbufs[idx], 1);
>>>>>> +rte_pktmbuf_reset(mbufs[idx]);
>>>>>> +idx++;
>>>>>> +}
>>>>>> +}
>>>>>> +return 0;
>>>>>> +}
>>>>> This is weird. Why not just use Duff's device in a more normal manner.
>>>> But it is a sort of Duff's method.
>>>> Not sure what looks weird to you here?
>>>> while () {} instead of do {} while();?
>>>> Konstantin
>>>>
>>>>
>>>>
>>> It is unusual to have cases not associated with block of the switch.
>>> Unusual to me means, "not used commonly in most code".
>>>
>>> Since you are jumping into the loop, might make more sense as a do { } 
>>> while()
>> I find this a very odd coding practice and I would suggest we not do this, 
>> unless it gives us some great performance gain.
>>
>> Keith
>The loop unwinding could give performance gain. The only problem is the
>switch/loop combination makes people feel weird at the first glance but
>soon they will grasp this style. Since this is inherited from old famous
>duff's device, i prefer to keep this style which saves lines of code.

Please add a comment to the code to reflex where this style came from and why 
you are using it, would be very handy here.

>>>
>>
>> Regards,
>> Keith
>>
>>
>>
>>
>
>


Regards,
Keith

[dpdk-dev] [PATCH v2 1/2] mbuf: provide rte_pktmbuf_alloc_bulk API

2015-12-21 Thread Xie, Huawei 
On 12/19/2015 3:27 AM, Wiles, Keith wrote:
> On 12/18/15, 11:32 AM, "dev on behalf of Stephen Hemminger"  dpdk.org on behalf of stephen at networkplumber.org> wrote:
>
>> On Fri, 18 Dec 2015 10:44:02 +
>> "Ananyev, Konstantin"  wrote:
>>
>>>
>>>> -Original Message-
>>>> From: dev [mailto:dev-bounces at dpdk.org] On Behalf Of Stephen Hemminger
>>>> Sent: Friday, December 18, 2015 5:01 AM
>>>> To: Xie, Huawei
>>>> Cc: dev at dpdk.org
>>>> Subject: Re: [dpdk-dev] [PATCH v2 1/2] mbuf: provide 
>>>> rte_pktmbuf_alloc_bulk API
>>>>
>>>> On Mon, 14 Dec 2015 09:14:41 +0800
>>>> Huawei Xie  wrote:
>>>>
>>>>> v2 changes:
>>>>>  unroll the loop a bit to help the performance
>>>>>
>>>>> rte_pktmbuf_alloc_bulk allocates a bulk of packet mbufs.
>>>>>
>>>>> There is related thread about this bulk API.
>>>>> http://dpdk.org/dev/patchwork/patch/4718/
>>>>> Thanks to Konstantin's loop unrolling.
>>>>>
>>>>> Signed-off-by: Gerald Rogers 
>>>>> Signed-off-by: Huawei Xie 
>>>>> Acked-by: Konstantin Ananyev 
>>>>> ---
>>>>>  lib/librte_mbuf/rte_mbuf.h | 50 
>>>>> ++
>>>>>  1 file changed, 50 insertions(+)
>>>>>
>>>>> diff --git a/lib/librte_mbuf/rte_mbuf.h b/lib/librte_mbuf/rte_mbuf.h
>>>>> index f234ac9..4e209e0 100644
>>>>> --- a/lib/librte_mbuf/rte_mbuf.h
>>>>> +++ b/lib/librte_mbuf/rte_mbuf.h
>>>>> @@ -1336,6 +1336,56 @@ static inline struct rte_mbuf 
>>>>> *rte_pktmbuf_alloc(struct rte_mempool *mp)
>>>>>  }
>>>>>
>>>>>  /**
>>>>> + * Allocate a bulk of mbufs, initialize refcnt and reset the fields to 
>>>>> default
>>>>> + * values.
>>>>> + *
>>>>> + *  @param pool
>>>>> + *The mempool from which mbufs are allocated.
>>>>> + *  @param mbufs
>>>>> + *Array of pointers to mbufs
>>>>> + *  @param count
>>>>> + *Array size
>>>>> + *  @return
>>>>> + *   - 0: Success
>>>>> + */
>>>>> +static inline int rte_pktmbuf_alloc_bulk(struct rte_mempool *pool,
>>>>> +  struct rte_mbuf **mbufs, unsigned count)
>>>>> +{
>>>>> + unsigned idx = 0;
>>>>> + int rc;
>>>>> +
>>>>> + rc = rte_mempool_get_bulk(pool, (void **)mbufs, count);
>>>>> + if (unlikely(rc))
>>>>> + return rc;
>>>>> +
>>>>> + switch (count % 4) {
>>>>> + while (idx != count) {
>>>>> + case 0:
>>>>> + RTE_MBUF_ASSERT(rte_mbuf_refcnt_read(mbufs[idx]) == 0);
>>>>> + rte_mbuf_refcnt_set(mbufs[idx], 1);
>>>>> + rte_pktmbuf_reset(mbufs[idx]);
>>>>> + idx++;
>>>>> + case 3:
>>>>> + RTE_MBUF_ASSERT(rte_mbuf_refcnt_read(mbufs[idx]) == 0);
>>>>> + rte_mbuf_refcnt_set(mbufs[idx], 1);
>>>>> + rte_pktmbuf_reset(mbufs[idx]);
>>>>> + idx++;
>>>>> + case 2:
>>>>> + RTE_MBUF_ASSERT(rte_mbuf_refcnt_read(mbufs[idx]) == 0);
>>>>> + rte_mbuf_refcnt_set(mbufs[idx], 1);
>>>>> + rte_pktmbuf_reset(mbufs[idx]);
>>>>> + idx++;
>>>>> + case 1:
>>>>> + RTE_MBUF_ASSERT(rte_mbuf_refcnt_read(mbufs[idx]) == 0);
>>>>> + rte_mbuf_refcnt_set(mbufs[idx], 1);
>>>>> + rte_pktmbuf_reset(mbufs[idx]);
>>>>> + idx++;
>>>>> + }
>>>>> + }
>>>>> + return 0;
>>>>> +}
>>>> This is weird. Why not just use Duff's device in a more normal manner.
>>> But it is a sort of Duff's method.
>>> Not sure what looks weird to you here?
>>> while () {} instead of do {} while();?
>>> Konstantin
>>>
>>>
>>>
>> It is unusual to have cases not associated with block of the switch.
>> Unusual to me means, "not used commonly in most code".
>>
>> Since you are jumping into the loop, might make more sense as a do { } 
>> while()
> I find this a very odd coding practice and I would suggest we not do this, 
> unless it gives us some great performance gain.
>
> Keith
The loop unwinding could give performance gain. The only problem is the
switch/loop combination makes people feel weird at the first glance but
soon they will grasp this style. Since this is inherited from old famous
duff's device, i prefer to keep this style which saves lines of code.
>>
>
> Regards,
> Keith
>
>
>
>

[dpdk-dev] [PATCH v2 1/2] mbuf: provide rte_pktmbuf_alloc_bulk API

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

With do {} while, we probably need one more extra check on if count is
zero. Duff's initial implementation assumes that count isn't zero. With
while loop, we save one line of code.

[dpdk-dev] [PATCH v2 1/2] mbuf: provide rte_pktmbuf_alloc_bulk API

2015-12-18 Thread Wiles, Keith 
On 12/18/15, 11:32 AM, "dev on behalf of Stephen Hemminger"  wrote:

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

I find this a very odd coding practice and I would suggest we not do this, 
unless it gives us some great performance gain.

Keith
>
>


Regards,
Keith

[dpdk-dev] [PATCH v2 1/2] mbuf: provide rte_pktmbuf_alloc_bulk API

2015-12-18 Thread Yuanhan Liu 
On Thu, Dec 17, 2015 at 09:01:14PM -0800, Stephen Hemminger wrote:
...
> > +
> > +   switch (count % 4) {
> > +   while (idx != count) {
> > +   case 0:
> > +   RTE_MBUF_ASSERT(rte_mbuf_refcnt_read(mbufs[idx]) == 0);
> > +   rte_mbuf_refcnt_set(mbufs[idx], 1);
> > +   rte_pktmbuf_reset(mbufs[idx]);
> > +   idx++;
> > +   case 3:
> > +   RTE_MBUF_ASSERT(rte_mbuf_refcnt_read(mbufs[idx]) == 0);
> > +   rte_mbuf_refcnt_set(mbufs[idx], 1);
> > +   rte_pktmbuf_reset(mbufs[idx]);
> > +   idx++;
> > +   case 2:
> > +   RTE_MBUF_ASSERT(rte_mbuf_refcnt_read(mbufs[idx]) == 0);
> > +   rte_mbuf_refcnt_set(mbufs[idx], 1);
> > +   rte_pktmbuf_reset(mbufs[idx]);
> > +   idx++;
> > +   case 1:
> > +   RTE_MBUF_ASSERT(rte_mbuf_refcnt_read(mbufs[idx]) == 0);
> > +   rte_mbuf_refcnt_set(mbufs[idx], 1);
> > +   rte_pktmbuf_reset(mbufs[idx]);
> > +   idx++;
> > +   }
> > +   }
> > +   return 0;
> > +}
> 
> This is weird. Why not just use Duff's device in a more normal manner.

Duff's device; interesting and good to know. Thanks.

--yliu

[dpdk-dev] [PATCH v2 1/2] mbuf: provide rte_pktmbuf_alloc_bulk API

2015-12-18 Thread Ananyev, Konstantin 


> -Original Message-
> From: dev [mailto:dev-bounces at dpdk.org] On Behalf Of Stephen Hemminger
> Sent: Friday, December 18, 2015 5:01 AM
> To: Xie, Huawei
> Cc: dev at dpdk.org
> Subject: Re: [dpdk-dev] [PATCH v2 1/2] mbuf: provide rte_pktmbuf_alloc_bulk 
> API
> 
> On Mon, 14 Dec 2015 09:14:41 +0800
> Huawei Xie  wrote:
> 
> > v2 changes:
> >  unroll the loop a bit to help the performance
> >
> > rte_pktmbuf_alloc_bulk allocates a bulk of packet mbufs.
> >
> > There is related thread about this bulk API.
> > http://dpdk.org/dev/patchwork/patch/4718/
> > Thanks to Konstantin's loop unrolling.
> >
> > Signed-off-by: Gerald Rogers 
> > Signed-off-by: Huawei Xie 
> > Acked-by: Konstantin Ananyev 
> > ---
> >  lib/librte_mbuf/rte_mbuf.h | 50 
> > ++
> >  1 file changed, 50 insertions(+)
> >
> > diff --git a/lib/librte_mbuf/rte_mbuf.h b/lib/librte_mbuf/rte_mbuf.h
> > index f234ac9..4e209e0 100644
> > --- a/lib/librte_mbuf/rte_mbuf.h
> > +++ b/lib/librte_mbuf/rte_mbuf.h
> > @@ -1336,6 +1336,56 @@ static inline struct rte_mbuf 
> > *rte_pktmbuf_alloc(struct rte_mempool *mp)
> >  }
> >
> >  /**
> > + * Allocate a bulk of mbufs, initialize refcnt and reset the fields to 
> > default
> > + * values.
> > + *
> > + *  @param pool
> > + *The mempool from which mbufs are allocated.
> > + *  @param mbufs
> > + *Array of pointers to mbufs
> > + *  @param count
> > + *Array size
> > + *  @return
> > + *   - 0: Success
> > + */
> > +static inline int rte_pktmbuf_alloc_bulk(struct rte_mempool *pool,
> > +struct rte_mbuf **mbufs, unsigned count)
> > +{
> > +   unsigned idx = 0;
> > +   int rc;
> > +
> > +   rc = rte_mempool_get_bulk(pool, (void **)mbufs, count);
> > +   if (unlikely(rc))
> > +   return rc;
> > +
> > +   switch (count % 4) {
> > +   while (idx != count) {
> > +   case 0:
> > +   RTE_MBUF_ASSERT(rte_mbuf_refcnt_read(mbufs[idx]) == 0);
> > +   rte_mbuf_refcnt_set(mbufs[idx], 1);
> > +   rte_pktmbuf_reset(mbufs[idx]);
> > +   idx++;
> > +   case 3:
> > +   RTE_MBUF_ASSERT(rte_mbuf_refcnt_read(mbufs[idx]) == 0);
> > +   rte_mbuf_refcnt_set(mbufs[idx], 1);
> > +   rte_pktmbuf_reset(mbufs[idx]);
> > +   idx++;
> > +   case 2:
> > +   RTE_MBUF_ASSERT(rte_mbuf_refcnt_read(mbufs[idx]) == 0);
> > +   rte_mbuf_refcnt_set(mbufs[idx], 1);
> > +   rte_pktmbuf_reset(mbufs[idx]);
> > +   idx++;
> > +   case 1:
> > +   RTE_MBUF_ASSERT(rte_mbuf_refcnt_read(mbufs[idx]) == 0);
> > +   rte_mbuf_refcnt_set(mbufs[idx], 1);
> > +   rte_pktmbuf_reset(mbufs[idx]);
> > +   idx++;
> > +   }
> > +   }
> > +   return 0;
> > +}
> 
> This is weird. Why not just use Duff's device in a more normal manner.

But it is a sort of Duff's method.
Not sure what looks weird to you here?
while () {} instead of do {} while();?
Konstantin

[dpdk-dev] [PATCH v2 1/2] mbuf: provide rte_pktmbuf_alloc_bulk API

2015-12-18 Thread Yuanhan Liu 
On Thu, Dec 17, 2015 at 03:42:19PM +, Ananyev, Konstantin wrote:
> 
> 
> > -Original Message-
> > From: dev [mailto:dev-bounces at dpdk.org] On Behalf Of Yuanhan Liu
> > Sent: Thursday, December 17, 2015 6:41 AM
> > To: Xie, Huawei
> > Cc: dev at dpdk.org
> > Subject: Re: [dpdk-dev] [PATCH v2 1/2] mbuf: provide rte_pktmbuf_alloc_bulk 
> > API
> > 
> > > +{
> > > + unsigned idx = 0;
> > > + int rc;
> > > +
> > > + rc = rte_mempool_get_bulk(pool, (void **)mbufs, count);
> > > + if (unlikely(rc))
> > > + return rc;
> > > +
> > > + switch (count % 4) {
> > > + while (idx != count) {
> > 
> > Well, that's an awkward trick, putting while between switch and case.
> > 
> > How about moving the whole switch block ahead, and use goto?
> > 
> > switch (count % 4) {
> > case 3:
> > goto __3;
> > break;
> > case 2:
> > goto __2;
> > break;
> > ...
> > 
> > }
> > 
> > It basically generates same instructions, yet it improves the
> > readability a bit.
> 
> I am personally not a big fun of gotos, unless it is totally unavoidable.
> I think switch/while construction is pretty obvious these days.

To me, it's not. (well, maybe I have been out for a while :(

> For me the original variant looks cleaner,

I agree with you on that. But it sacrifices code readability a bit.
If two pieces of code generates same instructions, but one is cleaner
(shorter), another one is more readable, I'd prefer the later.

> so my vote would be to stick with it.

Okay. And anyway, above is just a suggestion, and I'm open to other
suggestions.

--yliu

[dpdk-dev] [PATCH v2 1/2] mbuf: provide rte_pktmbuf_alloc_bulk API

2015-12-18 Thread Stephen Hemminger 
On Fri, 18 Dec 2015 10:44:02 +
"Ananyev, Konstantin"  wrote:

> 
> 
> > -Original Message-
> > From: dev [mailto:dev-bounces at dpdk.org] On Behalf Of Stephen Hemminger
> > Sent: Friday, December 18, 2015 5:01 AM
> > To: Xie, Huawei
> > Cc: dev at dpdk.org
> > Subject: Re: [dpdk-dev] [PATCH v2 1/2] mbuf: provide rte_pktmbuf_alloc_bulk 
> > API
> > 
> > On Mon, 14 Dec 2015 09:14:41 +0800
> > Huawei Xie  wrote:
> > 
> > > v2 changes:
> > >  unroll the loop a bit to help the performance
> > >
> > > rte_pktmbuf_alloc_bulk allocates a bulk of packet mbufs.
> > >
> > > There is related thread about this bulk API.
> > > http://dpdk.org/dev/patchwork/patch/4718/
> > > Thanks to Konstantin's loop unrolling.
> > >
> > > Signed-off-by: Gerald Rogers 
> > > Signed-off-by: Huawei Xie 
> > > Acked-by: Konstantin Ananyev 
> > > ---
> > >  lib/librte_mbuf/rte_mbuf.h | 50 
> > > ++
> > >  1 file changed, 50 insertions(+)
> > >
> > > diff --git a/lib/librte_mbuf/rte_mbuf.h b/lib/librte_mbuf/rte_mbuf.h
> > > index f234ac9..4e209e0 100644
> > > --- a/lib/librte_mbuf/rte_mbuf.h
> > > +++ b/lib/librte_mbuf/rte_mbuf.h
> > > @@ -1336,6 +1336,56 @@ static inline struct rte_mbuf 
> > > *rte_pktmbuf_alloc(struct rte_mempool *mp)
> > >  }
> > >
> > >  /**
> > > + * Allocate a bulk of mbufs, initialize refcnt and reset the fields to 
> > > default
> > > + * values.
> > > + *
> > > + *  @param pool
> > > + *The mempool from which mbufs are allocated.
> > > + *  @param mbufs
> > > + *Array of pointers to mbufs
> > > + *  @param count
> > > + *Array size
> > > + *  @return
> > > + *   - 0: Success
> > > + */
> > > +static inline int rte_pktmbuf_alloc_bulk(struct rte_mempool *pool,
> > > +  struct rte_mbuf **mbufs, unsigned count)
> > > +{
> > > + unsigned idx = 0;
> > > + int rc;
> > > +
> > > + rc = rte_mempool_get_bulk(pool, (void **)mbufs, count);
> > > + if (unlikely(rc))
> > > + return rc;
> > > +
> > > + switch (count % 4) {
> > > + while (idx != count) {
> > > + case 0:
> > > + RTE_MBUF_ASSERT(rte_mbuf_refcnt_read(mbufs[idx]) == 0);
> > > + rte_mbuf_refcnt_set(mbufs[idx], 1);
> > > + rte_pktmbuf_reset(mbufs[idx]);
> > > + idx++;
> > > + case 3:
> > > + RTE_MBUF_ASSERT(rte_mbuf_refcnt_read(mbufs[idx]) == 0);
> > > + rte_mbuf_refcnt_set(mbufs[idx], 1);
> > > + rte_pktmbuf_reset(mbufs[idx]);
> > > + idx++;
> > > + case 2:
> > > + RTE_MBUF_ASSERT(rte_mbuf_refcnt_read(mbufs[idx]) == 0);
> > > + rte_mbuf_refcnt_set(mbufs[idx], 1);
> > > + rte_pktmbuf_reset(mbufs[idx]);
> > > + idx++;
> > > + case 1:
> > > + RTE_MBUF_ASSERT(rte_mbuf_refcnt_read(mbufs[idx]) == 0);
> > > + rte_mbuf_refcnt_set(mbufs[idx], 1);
> > > + rte_pktmbuf_reset(mbufs[idx]);
> > > + idx++;
> > > + }
> > > + }
> > > + return 0;
> > > +}
> > 
> > This is weird. Why not just use Duff's device in a more normal manner.
> 
> But it is a sort of Duff's method.
> Not sure what looks weird to you here?
> while () {} instead of do {} while();?
> Konstantin
> 
> 
> 

It is unusual to have cases not associated with block of the switch.
Unusual to me means, "not used commonly in most code".

Since you are jumping into the loop, might make more sense as a do { } while()

[dpdk-dev] [PATCH v2 1/2] mbuf: provide rte_pktmbuf_alloc_bulk API

2015-12-18 Thread Xie, Huawei 
On 12/18/2015 1:03 PM, Stephen Hemminger wrote:
> On Mon, 14 Dec 2015 09:14:41 +0800
> Huawei Xie  wrote:
>
>> v2 changes:
>>  unroll the loop a bit to help the performance
>>
>> rte_pktmbuf_alloc_bulk allocates a bulk of packet mbufs.
>>
>> There is related thread about this bulk API.
>> http://dpdk.org/dev/patchwork/patch/4718/
>> Thanks to Konstantin's loop unrolling.
>>
>> Signed-off-by: Gerald Rogers 
>> Signed-off-by: Huawei Xie 
>> Acked-by: Konstantin Ananyev 
>> ---
>>  lib/librte_mbuf/rte_mbuf.h | 50 
>> ++
>>  1 file changed, 50 insertions(+)
>>
>> diff --git a/lib/librte_mbuf/rte_mbuf.h b/lib/librte_mbuf/rte_mbuf.h
>> index f234ac9..4e209e0 100644
>> --- a/lib/librte_mbuf/rte_mbuf.h
>> +++ b/lib/librte_mbuf/rte_mbuf.h
>> @@ -1336,6 +1336,56 @@ static inline struct rte_mbuf 
>> *rte_pktmbuf_alloc(struct rte_mempool *mp)
>>  }
>>  
>>  /**
>> + * Allocate a bulk of mbufs, initialize refcnt and reset the fields to 
>> default
>> + * values.
>> + *
>> + *  @param pool
>> + *The mempool from which mbufs are allocated.
>> + *  @param mbufs
>> + *Array of pointers to mbufs
>> + *  @param count
>> + *Array size
>> + *  @return
>> + *   - 0: Success
>> + */
>> +static inline int rte_pktmbuf_alloc_bulk(struct rte_mempool *pool,
>> + struct rte_mbuf **mbufs, unsigned count)
>> +{
>> +unsigned idx = 0;
>> +int rc;
>> +
>> +rc = rte_mempool_get_bulk(pool, (void **)mbufs, count);
>> +if (unlikely(rc))
>> +return rc;
>> +
>> +switch (count % 4) {
>> +while (idx != count) {
>> +case 0:
>> +RTE_MBUF_ASSERT(rte_mbuf_refcnt_read(mbufs[idx]) == 0);
>> +rte_mbuf_refcnt_set(mbufs[idx], 1);
>> +rte_pktmbuf_reset(mbufs[idx]);
>> +idx++;
>> +case 3:
>> +RTE_MBUF_ASSERT(rte_mbuf_refcnt_read(mbufs[idx]) == 0);
>> +rte_mbuf_refcnt_set(mbufs[idx], 1);
>> +rte_pktmbuf_reset(mbufs[idx]);
>> +idx++;
>> +case 2:
>> +RTE_MBUF_ASSERT(rte_mbuf_refcnt_read(mbufs[idx]) == 0);
>> +rte_mbuf_refcnt_set(mbufs[idx], 1);
>> +rte_pktmbuf_reset(mbufs[idx]);
>> +idx++;
>> +case 1:
>> +RTE_MBUF_ASSERT(rte_mbuf_refcnt_read(mbufs[idx]) == 0);
>> +rte_mbuf_refcnt_set(mbufs[idx], 1);
>> +rte_pktmbuf_reset(mbufs[idx]);
>> +idx++;
>> +}
>> +}
>> +return 0;
>> +}
> This is weird. Why not just use Duff's device in a more normal manner.
Hi Stephen: I just compared with duff's unrolled version. It is slightly
different, but where is weird?
>
>

[dpdk-dev] [PATCH v2 1/2] mbuf: provide rte_pktmbuf_alloc_bulk API

2015-12-17 Thread Stephen Hemminger 
On Mon, 14 Dec 2015 09:14:41 +0800
Huawei Xie  wrote:

> v2 changes:
>  unroll the loop a bit to help the performance
> 
> rte_pktmbuf_alloc_bulk allocates a bulk of packet mbufs.
> 
> There is related thread about this bulk API.
> http://dpdk.org/dev/patchwork/patch/4718/
> Thanks to Konstantin's loop unrolling.
> 
> Signed-off-by: Gerald Rogers 
> Signed-off-by: Huawei Xie 
> Acked-by: Konstantin Ananyev 
> ---
>  lib/librte_mbuf/rte_mbuf.h | 50 
> ++
>  1 file changed, 50 insertions(+)
> 
> diff --git a/lib/librte_mbuf/rte_mbuf.h b/lib/librte_mbuf/rte_mbuf.h
> index f234ac9..4e209e0 100644
> --- a/lib/librte_mbuf/rte_mbuf.h
> +++ b/lib/librte_mbuf/rte_mbuf.h
> @@ -1336,6 +1336,56 @@ static inline struct rte_mbuf 
> *rte_pktmbuf_alloc(struct rte_mempool *mp)
>  }
>  
>  /**
> + * Allocate a bulk of mbufs, initialize refcnt and reset the fields to 
> default
> + * values.
> + *
> + *  @param pool
> + *The mempool from which mbufs are allocated.
> + *  @param mbufs
> + *Array of pointers to mbufs
> + *  @param count
> + *Array size
> + *  @return
> + *   - 0: Success
> + */
> +static inline int rte_pktmbuf_alloc_bulk(struct rte_mempool *pool,
> +  struct rte_mbuf **mbufs, unsigned count)
> +{
> + unsigned idx = 0;
> + int rc;
> +
> + rc = rte_mempool_get_bulk(pool, (void **)mbufs, count);
> + if (unlikely(rc))
> + return rc;
> +
> + switch (count % 4) {
> + while (idx != count) {
> + case 0:
> + RTE_MBUF_ASSERT(rte_mbuf_refcnt_read(mbufs[idx]) == 0);
> + rte_mbuf_refcnt_set(mbufs[idx], 1);
> + rte_pktmbuf_reset(mbufs[idx]);
> + idx++;
> + case 3:
> + RTE_MBUF_ASSERT(rte_mbuf_refcnt_read(mbufs[idx]) == 0);
> + rte_mbuf_refcnt_set(mbufs[idx], 1);
> + rte_pktmbuf_reset(mbufs[idx]);
> + idx++;
> + case 2:
> + RTE_MBUF_ASSERT(rte_mbuf_refcnt_read(mbufs[idx]) == 0);
> + rte_mbuf_refcnt_set(mbufs[idx], 1);
> + rte_pktmbuf_reset(mbufs[idx]);
> + idx++;
> + case 1:
> + RTE_MBUF_ASSERT(rte_mbuf_refcnt_read(mbufs[idx]) == 0);
> + rte_mbuf_refcnt_set(mbufs[idx], 1);
> + rte_pktmbuf_reset(mbufs[idx]);
> + idx++;
> + }
> + }
> + return 0;
> +}

This is weird. Why not just use Duff's device in a more normal manner.

[dpdk-dev] [PATCH v2 1/2] mbuf: provide rte_pktmbuf_alloc_bulk API

2015-12-17 Thread Ananyev, Konstantin 


> -Original Message-
> From: dev [mailto:dev-bounces at dpdk.org] On Behalf Of Yuanhan Liu
> Sent: Thursday, December 17, 2015 6:41 AM
> To: Xie, Huawei
> Cc: dev at dpdk.org
> Subject: Re: [dpdk-dev] [PATCH v2 1/2] mbuf: provide rte_pktmbuf_alloc_bulk 
> API
> 
> On Mon, Dec 14, 2015 at 09:14:41AM +0800, Huawei Xie wrote:
> > v2 changes:
> >  unroll the loop a bit to help the performance
> >
> > rte_pktmbuf_alloc_bulk allocates a bulk of packet mbufs.
> >
> > There is related thread about this bulk API.
> > http://dpdk.org/dev/patchwork/patch/4718/
> > Thanks to Konstantin's loop unrolling.
> >
> > Signed-off-by: Gerald Rogers 
> > Signed-off-by: Huawei Xie 
> > Acked-by: Konstantin Ananyev 
> > ---
> >  lib/librte_mbuf/rte_mbuf.h | 50 
> > ++
> >  1 file changed, 50 insertions(+)
> >
> > diff --git a/lib/librte_mbuf/rte_mbuf.h b/lib/librte_mbuf/rte_mbuf.h
> > index f234ac9..4e209e0 100644
> > --- a/lib/librte_mbuf/rte_mbuf.h
> > +++ b/lib/librte_mbuf/rte_mbuf.h
> > @@ -1336,6 +1336,56 @@ static inline struct rte_mbuf 
> > *rte_pktmbuf_alloc(struct rte_mempool *mp)
> >  }
> >
> >  /**
> > + * Allocate a bulk of mbufs, initialize refcnt and reset the fields to 
> > default
> > + * values.
> > + *
> > + *  @param pool
> > + *The mempool from which mbufs are allocated.
> > + *  @param mbufs
> > + *Array of pointers to mbufs
> > + *  @param count
> > + *Array size
> > + *  @return
> > + *   - 0: Success
> > + */
> > +static inline int rte_pktmbuf_alloc_bulk(struct rte_mempool *pool,
> > +struct rte_mbuf **mbufs, unsigned count)
> 
> It violates the coding style a bit.
> 
> > +{
> > +   unsigned idx = 0;
> > +   int rc;
> > +
> > +   rc = rte_mempool_get_bulk(pool, (void **)mbufs, count);
> > +   if (unlikely(rc))
> > +   return rc;
> > +
> > +   switch (count % 4) {
> > +   while (idx != count) {
> 
> Well, that's an awkward trick, putting while between switch and case.
> 
> How about moving the whole switch block ahead, and use goto?
> 
>   switch (count % 4) {
>   case 3:
>   goto __3;
>   break;
>   case 2:
>   goto __2;
>   break;
>   ...
> 
>   }
> 
> It basically generates same instructions, yet it improves the
> readability a bit.

I am personally not a big fun of gotos, unless it is totally unavoidable.
I think switch/while construction is pretty obvious these days.
For me the original variant looks cleaner, so my vote would be to stick with it.
Konstantin

> 
>   --yliu
> 
> > +   case 0:
> > +   RTE_MBUF_ASSERT(rte_mbuf_refcnt_read(mbufs[idx]) == 0);
> > +   rte_mbuf_refcnt_set(mbufs[idx], 1);
> > +   rte_pktmbuf_reset(mbufs[idx]);
> > +   idx++;
> > +   case 3:
> > +   RTE_MBUF_ASSERT(rte_mbuf_refcnt_read(mbufs[idx]) == 0);
> > +   rte_mbuf_refcnt_set(mbufs[idx], 1);
> > +   rte_pktmbuf_reset(mbufs[idx]);
> > +   idx++;
> > +   case 2:
> > +   RTE_MBUF_ASSERT(rte_mbuf_refcnt_read(mbufs[idx]) == 0);
> > +   rte_mbuf_refcnt_set(mbufs[idx], 1);
> > +   rte_pktmbuf_reset(mbufs[idx]);
> > +   idx++;
> > +   case 1:
> > +   RTE_MBUF_ASSERT(rte_mbuf_refcnt_read(mbufs[idx]) == 0);
> > +   rte_mbuf_refcnt_set(mbufs[idx], 1);
> > +   rte_pktmbuf_reset(mbufs[idx]);
> > +   idx++;
> > +   }
> > +   }
> > +   return 0;
> > +}
> > +
> > +/**
> >   * Attach packet mbuf to another packet mbuf.
> >   *
> >   * After attachment we refer the mbuf we attached as 'indirect',
> > --
> > 1.8.1.4

[dpdk-dev] [PATCH v2 1/2] mbuf: provide rte_pktmbuf_alloc_bulk API

2015-12-17 Thread Yuanhan Liu 
On Mon, Dec 14, 2015 at 09:14:41AM +0800, Huawei Xie wrote:
> v2 changes:
>  unroll the loop a bit to help the performance
> 
> rte_pktmbuf_alloc_bulk allocates a bulk of packet mbufs.
> 
> There is related thread about this bulk API.
> http://dpdk.org/dev/patchwork/patch/4718/
> Thanks to Konstantin's loop unrolling.
> 
> Signed-off-by: Gerald Rogers 
> Signed-off-by: Huawei Xie 
> Acked-by: Konstantin Ananyev 
> ---
>  lib/librte_mbuf/rte_mbuf.h | 50 
> ++
>  1 file changed, 50 insertions(+)
> 
> diff --git a/lib/librte_mbuf/rte_mbuf.h b/lib/librte_mbuf/rte_mbuf.h
> index f234ac9..4e209e0 100644
> --- a/lib/librte_mbuf/rte_mbuf.h
> +++ b/lib/librte_mbuf/rte_mbuf.h
> @@ -1336,6 +1336,56 @@ static inline struct rte_mbuf 
> *rte_pktmbuf_alloc(struct rte_mempool *mp)
>  }
>  
>  /**
> + * Allocate a bulk of mbufs, initialize refcnt and reset the fields to 
> default
> + * values.
> + *
> + *  @param pool
> + *The mempool from which mbufs are allocated.
> + *  @param mbufs
> + *Array of pointers to mbufs
> + *  @param count
> + *Array size
> + *  @return
> + *   - 0: Success
> + */
> +static inline int rte_pktmbuf_alloc_bulk(struct rte_mempool *pool,
> +  struct rte_mbuf **mbufs, unsigned count)

It violates the coding style a bit.

> +{
> + unsigned idx = 0;
> + int rc;
> +
> + rc = rte_mempool_get_bulk(pool, (void **)mbufs, count);
> + if (unlikely(rc))
> + return rc;
> +
> + switch (count % 4) {
> + while (idx != count) {

Well, that's an awkward trick, putting while between switch and case.

How about moving the whole switch block ahead, and use goto?

switch (count % 4) {
case 3:
goto __3;
break;
case 2:
goto __2;
break;
...

}

It basically generates same instructions, yet it improves the
readability a bit.

--yliu

> + case 0:
> + RTE_MBUF_ASSERT(rte_mbuf_refcnt_read(mbufs[idx]) == 0);
> + rte_mbuf_refcnt_set(mbufs[idx], 1);
> + rte_pktmbuf_reset(mbufs[idx]);
> + idx++;
> + case 3:
> + RTE_MBUF_ASSERT(rte_mbuf_refcnt_read(mbufs[idx]) == 0);
> + rte_mbuf_refcnt_set(mbufs[idx], 1);
> + rte_pktmbuf_reset(mbufs[idx]);
> + idx++;
> + case 2:
> + RTE_MBUF_ASSERT(rte_mbuf_refcnt_read(mbufs[idx]) == 0);
> + rte_mbuf_refcnt_set(mbufs[idx], 1);
> + rte_pktmbuf_reset(mbufs[idx]);
> + idx++;
> + case 1:
> + RTE_MBUF_ASSERT(rte_mbuf_refcnt_read(mbufs[idx]) == 0);
> + rte_mbuf_refcnt_set(mbufs[idx], 1);
> + rte_pktmbuf_reset(mbufs[idx]);
> + idx++;
> + }
> + }
> + return 0;
> +}
> +
> +/**
>   * Attach packet mbuf to another packet mbuf.
>   *
>   * After attachment we refer the mbuf we attached as 'indirect',
> -- 
> 1.8.1.4

[dpdk-dev] [PATCH v2 1/2] mbuf: provide rte_pktmbuf_alloc_bulk API

2015-12-14 Thread Huawei Xie 
v2 changes:
 unroll the loop a bit to help the performance

rte_pktmbuf_alloc_bulk allocates a bulk of packet mbufs.

There is related thread about this bulk API.
http://dpdk.org/dev/patchwork/patch/4718/
Thanks to Konstantin's loop unrolling.

Signed-off-by: Gerald Rogers 
Signed-off-by: Huawei Xie 
Acked-by: Konstantin Ananyev 
---
 lib/librte_mbuf/rte_mbuf.h | 50 ++
 1 file changed, 50 insertions(+)

diff --git a/lib/librte_mbuf/rte_mbuf.h b/lib/librte_mbuf/rte_mbuf.h
index f234ac9..4e209e0 100644
--- a/lib/librte_mbuf/rte_mbuf.h
+++ b/lib/librte_mbuf/rte_mbuf.h
@@ -1336,6 +1336,56 @@ static inline struct rte_mbuf *rte_pktmbuf_alloc(struct 
rte_mempool *mp)
 }

 /**
+ * Allocate a bulk of mbufs, initialize refcnt and reset the fields to default
+ * values.
+ *
+ *  @param pool
+ *The mempool from which mbufs are allocated.
+ *  @param mbufs
+ *Array of pointers to mbufs
+ *  @param count
+ *Array size
+ *  @return
+ *   - 0: Success
+ */
+static inline int rte_pktmbuf_alloc_bulk(struct rte_mempool *pool,
+struct rte_mbuf **mbufs, unsigned count)
+{
+   unsigned idx = 0;
+   int rc;
+
+   rc = rte_mempool_get_bulk(pool, (void **)mbufs, count);
+   if (unlikely(rc))
+   return rc;
+
+   switch (count % 4) {
+   while (idx != count) {
+   case 0:
+   RTE_MBUF_ASSERT(rte_mbuf_refcnt_read(mbufs[idx]) == 0);
+   rte_mbuf_refcnt_set(mbufs[idx], 1);
+   rte_pktmbuf_reset(mbufs[idx]);
+   idx++;
+   case 3:
+   RTE_MBUF_ASSERT(rte_mbuf_refcnt_read(mbufs[idx]) == 0);
+   rte_mbuf_refcnt_set(mbufs[idx], 1);
+   rte_pktmbuf_reset(mbufs[idx]);
+   idx++;
+   case 2:
+   RTE_MBUF_ASSERT(rte_mbuf_refcnt_read(mbufs[idx]) == 0);
+   rte_mbuf_refcnt_set(mbufs[idx], 1);
+   rte_pktmbuf_reset(mbufs[idx]);
+   idx++;
+   case 1:
+   RTE_MBUF_ASSERT(rte_mbuf_refcnt_read(mbufs[idx]) == 0);
+   rte_mbuf_refcnt_set(mbufs[idx], 1);
+   rte_pktmbuf_reset(mbufs[idx]);
+   idx++;
+   }
+   }
+   return 0;
+}
+
+/**
  * Attach packet mbuf to another packet mbuf.
  *
  * After attachment we refer the mbuf we attached as 'indirect',
-- 
1.8.1.4