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

Xie, Huawei huawei.xie at intel.com
Mon Dec 21 13:25:50 CET 2015


On 12/19/2015 1:32 AM, Stephen Hemminger wrote:
> On Fri, 18 Dec 2015 10:44:02 +0000
> "Ananyev, Konstantin" <konstantin.ananyev at intel.com> 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 <huawei.xie at intel.com> 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 <gerald.rogers at intel.com>
>>>> Signed-off-by: Huawei Xie <huawei.xie at intel.com>
>>>> Acked-by: Konstantin Ananyev <konstantin.ananyev at intel.com>
>>>> ---
>>>>  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.



More information about the dev mailing list