[dpdk-dev] [PATCH 1/2] Move the error check inside __mempool_check_cookies()

[dpdk-dev] [PATCH 1/2] Move the error check inside __mempool_check_cookies()

[Keith Wiles]

Three places check for the return value from __mempool_get_bulk to be zero
and then call the debug routine __mempool_check_cookies(). The test is
not required if moved into the debug routine. Minor cleanup and mostly
does not effect performance, unless the is not removed by the compiler
in the case where teh debug routine is not defined.

Signed-off-by: Keith Wiles <keith.wiles@windriver.com>
---
 lib/librte_mempool/rte_mempool.h | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/lib/librte_mempool/rte_mempool.h b/lib/librte_mempool/rte_mempool.h
index 597cf4f..154fdd4 100644
--- a/lib/librte_mempool/rte_mempool.h
+++ b/lib/librte_mempool/rte_mempool.h
@@ -325,6 +325,9 @@ static inline void __mempool_check_cookies(const struct rte_mempool *mp,
 	void *obj;
 	void **obj_table;
 
+	if ( n < 0 )
+		return;
+
 	/* Force to drop the "const" attribute. This is done only when
 	 * DEBUG is enabled */
 	tmp = (void *) obj_table_const;
@@ -1029,8 +1032,7 @@ rte_mempool_mc_get_bulk(struct rte_mempool *mp, void **obj_table, unsigned n)
 {
 	int ret;
 	ret = __mempool_get_bulk(mp, obj_table, n, 1);
-	if (ret == 0)
-		__mempool_check_cookies(mp, obj_table, n, 1);
+	__mempool_check_cookies(mp, obj_table, n, 1);
 	return ret;
 }
 
@@ -1058,8 +1060,7 @@ rte_mempool_sc_get_bulk(struct rte_mempool *mp, void **obj_table, unsigned n)
 {
 	int ret;
 	ret = __mempool_get_bulk(mp, obj_table, n, 0);
-	if (ret == 0)
-		__mempool_check_cookies(mp, obj_table, n, 1);
+	__mempool_check_cookies(mp, obj_table, n, 1);
 	return ret;
 }
 
@@ -1091,8 +1092,7 @@ rte_mempool_get_bulk(struct rte_mempool *mp, void **obj_table, unsigned n)
 	int ret;
 	ret = __mempool_get_bulk(mp, obj_table, n,
 				 !(mp->flags & MEMPOOL_F_SC_GET));
-	if (ret == 0)
-		__mempool_check_cookies(mp, obj_table, n, 1);
+	__mempool_check_cookies(mp, obj_table, n, 1);
 	return ret;
 }
 
-- 
2.1.0

[dpdk-dev] [PATCH 2/2] Adding the routines rte_pktmbuf_alloc_bulk() and rte_pktmbuf_free_bulk()

[Keith Wiles]

Minor helper routines to mirror the mempool routines and remove the code
from applications. The ixgbe_rxtx_vec.c routine could be changed to use
the ret_pktmbuf_alloc_bulk() routine inplace of rte_mempool_get_bulk().

Signed-off-by: Keith Wiles <keith.wiles@windriver.com>
---
 lib/librte_mbuf/rte_mbuf.h | 77 ++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 77 insertions(+)

diff --git a/lib/librte_mbuf/rte_mbuf.h b/lib/librte_mbuf/rte_mbuf.h
index 1c6e115..f298621 100644
--- a/lib/librte_mbuf/rte_mbuf.h
+++ b/lib/librte_mbuf/rte_mbuf.h
@@ -546,6 +546,41 @@ static inline void rte_pktmbuf_reset(struct rte_mbuf *m)
 }
 
 /**
+ * @internal Allocate a list of mbufs from mempool *mp*.
+ * The use of that function is reserved for RTE internal needs.
+ * Please use rte_pktmbuf_alloc_bulk().
+ *
+ * @param mp
+ *   The mempool from which mbuf is allocated.
+ * @param m_list
+ *   The array to place the allocated rte_mbufs pointers.
+ * @param cnt
+ *   The number of mbufs to allocate
+ * @return
+ *   - 0 if the number of mbufs allocated was ok
+ *   - <0 is an ERROR.
+ */
+static inline int __rte_mbuf_raw_alloc_bulk(struct rte_mempool *mp, struct rte_mbuf *m_list[], int cnt)
+{
+	struct rte_mbuf *m;
+	int		ret;
+
+	ret = rte_mempool_get_bulk(mp, (void **)m_list, cnt);
+	if ( ret == 0 ) {
+		int		i;
+		for(i = 0; i < cnt; i++) {
+			m = *m_list++;
+#ifdef RTE_MBUF_REFCNT
+			rte_mbuf_refcnt_set(m, 1);
+#endif /* RTE_MBUF_REFCNT */
+			rte_pktmbuf_reset(m);
+		}
+		ret = cnt;
+	}
+	return ret;
+}
+
+/**
  * Allocate a new mbuf from a mempool.
  *
  * This new mbuf contains one segment, which has a length of 0. The pointer
@@ -671,6 +706,32 @@ __rte_pktmbuf_prefree_seg(struct rte_mbuf *m)
 }
 
 /**
+ * Allocate a list of mbufs from a mempool into a mbufs array.
+ *
+ * This mbuf list contains one segment per mbuf, which has a length of 0. The pointer
+ * to data is initialized to have some bytes of headroom in the buffer
+ * (if buffer size allows).
+ *
+ * The routine is just a simple wrapper routine to reduce code in the application and
+ * provide a cleaner API for multiple mbuf requests.
+ *
+ * @param mp
+ *   The mempool from which the mbuf is allocated.
+ * @param m_list
+ *   An array of mbuf pointers, cnt must be less then or equal to the size of the list.
+ * @param cnt
+ *   Number of slots in the m_list array to fill.
+ * @return
+ *   - The number of valid mbufs pointers in the m_list array.
+ *   - Zero if the request cnt could not be allocated.
+ */
+static inline int __attribute__((always_inline))
+rte_pktmbuf_alloc_bulk(struct rte_mempool *mp, struct rte_mbuf *m_list[], int16_t cnt)
+{
+	return __rte_mbuf_raw_alloc_bulk(mp, m_list, cnt);
+}
+
+/**
  * Free a segment of a packet mbuf into its original mempool.
  *
  * Free an mbuf, without parsing other segments in case of chained
@@ -708,6 +769,22 @@ static inline void rte_pktmbuf_free(struct rte_mbuf *m)
 	}
 }
 
+/**
+ * Free a list of packet mbufs back into its original mempool.
+ *
+ * Free a list of mbufs by calling rte_pktmbuf_free() in a loop as a wrapper function.
+ *
+ * @param m_list
+ *   An array of rte_mbuf pointers to be freed.
+ * @param npkts
+ *   Number of packets to free in list.
+ */
+static inline void rte_pktmbuf_free_bulk(struct rte_mbuf *m_list[], int16_t npkts)
+{
+	while(npkts--)
+		rte_pktmbuf_free(*m_list++);
+}
+
 #ifdef RTE_MBUF_REFCNT
 
 /**
-- 
2.1.0

Re: [dpdk-dev] [PATCH 2/2] Adding the routines rte_pktmbuf_alloc_bulk() and rte_pktmbuf_free_bulk()

[Richardson, Bruce]

> -----Original Message-----
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Keith Wiles
> Sent: Sunday, October 05, 2014 12:10 AM
> To: dev@dpdk.org
> Subject: [dpdk-dev] [PATCH 2/2] Adding the routines rte_pktmbuf_alloc_bulk()
> and rte_pktmbuf_free_bulk()
> 
> Minor helper routines to mirror the mempool routines and remove the code
> from applications. The ixgbe_rxtx_vec.c routine could be changed to use
> the ret_pktmbuf_alloc_bulk() routine inplace of rte_mempool_get_bulk().
> 

I believe such a change would cause a performance regression, as the extra init code in the alloc_bulk() function would take additional cycles and is not needed. The vector routines use the mempool function directly, so that there is no overhead of mbuf initialization, as the vector routines use their additional "knowledge" of what the mbufs will be used for to init them in a faster manner than can be done inside the mbuf library.

/Bruce

> Signed-off-by: Keith Wiles <keith.wiles@windriver.com>
> ---
>  lib/librte_mbuf/rte_mbuf.h | 77
> ++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 77 insertions(+)
> 
> diff --git a/lib/librte_mbuf/rte_mbuf.h b/lib/librte_mbuf/rte_mbuf.h
> index 1c6e115..f298621 100644
> --- a/lib/librte_mbuf/rte_mbuf.h
> +++ b/lib/librte_mbuf/rte_mbuf.h
> @@ -546,6 +546,41 @@ static inline void rte_pktmbuf_reset(struct rte_mbuf
> *m)
>  }
> 
>  /**
> + * @internal Allocate a list of mbufs from mempool *mp*.
> + * The use of that function is reserved for RTE internal needs.
> + * Please use rte_pktmbuf_alloc_bulk().
> + *
> + * @param mp
> + *   The mempool from which mbuf is allocated.
> + * @param m_list
> + *   The array to place the allocated rte_mbufs pointers.
> + * @param cnt
> + *   The number of mbufs to allocate
> + * @return
> + *   - 0 if the number of mbufs allocated was ok
> + *   - <0 is an ERROR.
> + */
> +static inline int __rte_mbuf_raw_alloc_bulk(struct rte_mempool *mp, struct
> rte_mbuf *m_list[], int cnt)
> +{
> +	struct rte_mbuf *m;
> +	int		ret;
> +
> +	ret = rte_mempool_get_bulk(mp, (void **)m_list, cnt);
> +	if ( ret == 0 ) {
> +		int		i;
> +		for(i = 0; i < cnt; i++) {
> +			m = *m_list++;
> +#ifdef RTE_MBUF_REFCNT
> +			rte_mbuf_refcnt_set(m, 1);
> +#endif /* RTE_MBUF_REFCNT */
> +			rte_pktmbuf_reset(m);
> +		}
> +		ret = cnt;
> +	}
> +	return ret;
> +}
> +
> +/**
>   * Allocate a new mbuf from a mempool.
>   *
>   * This new mbuf contains one segment, which has a length of 0. The pointer
> @@ -671,6 +706,32 @@ __rte_pktmbuf_prefree_seg(struct rte_mbuf *m)
>  }
> 
>  /**
> + * Allocate a list of mbufs from a mempool into a mbufs array.
> + *
> + * This mbuf list contains one segment per mbuf, which has a length of 0. The
> pointer
> + * to data is initialized to have some bytes of headroom in the buffer
> + * (if buffer size allows).
> + *
> + * The routine is just a simple wrapper routine to reduce code in the application
> and
> + * provide a cleaner API for multiple mbuf requests.
> + *
> + * @param mp
> + *   The mempool from which the mbuf is allocated.
> + * @param m_list
> + *   An array of mbuf pointers, cnt must be less then or equal to the size of the
> list.
> + * @param cnt
> + *   Number of slots in the m_list array to fill.
> + * @return
> + *   - The number of valid mbufs pointers in the m_list array.
> + *   - Zero if the request cnt could not be allocated.
> + */
> +static inline int __attribute__((always_inline))
> +rte_pktmbuf_alloc_bulk(struct rte_mempool *mp, struct rte_mbuf *m_list[],
> int16_t cnt)
> +{
> +	return __rte_mbuf_raw_alloc_bulk(mp, m_list, cnt);
> +}
> +
> +/**
>   * Free a segment of a packet mbuf into its original mempool.
>   *
>   * Free an mbuf, without parsing other segments in case of chained
> @@ -708,6 +769,22 @@ static inline void rte_pktmbuf_free(struct rte_mbuf
> *m)
>  	}
>  }
> 
> +/**
> + * Free a list of packet mbufs back into its original mempool.
> + *
> + * Free a list of mbufs by calling rte_pktmbuf_free() in a loop as a wrapper
> function.
> + *
> + * @param m_list
> + *   An array of rte_mbuf pointers to be freed.
> + * @param npkts
> + *   Number of packets to free in list.
> + */
> +static inline void rte_pktmbuf_free_bulk(struct rte_mbuf *m_list[], int16_t
> npkts)
> +{
> +	while(npkts--)
> +		rte_pktmbuf_free(*m_list++);
> +}
> +
>  #ifdef RTE_MBUF_REFCNT
> 
>  /**
> --
> 2.1.0

Re: [dpdk-dev] [PATCH 2/2] Adding the routines rte_pktmbuf_alloc_bulk() and rte_pktmbuf_free_bulk()

[Wiles, Roger Keith]

Hi Bruce,

Do I need to reject the for the new routines or just make sure the vector driver does not get updated to use those routines?

Thanks
++Keith

On Oct 6, 2014, at 3:56 AM, Richardson, Bruce <bruce.richardson@intel.com> wrote:

> 
> 
>> -----Original Message-----
>> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Keith Wiles
>> Sent: Sunday, October 05, 2014 12:10 AM
>> To: dev@dpdk.org
>> Subject: [dpdk-dev] [PATCH 2/2] Adding the routines rte_pktmbuf_alloc_bulk()
>> and rte_pktmbuf_free_bulk()
>> 
>> Minor helper routines to mirror the mempool routines and remove the code
>> from applications. The ixgbe_rxtx_vec.c routine could be changed to use
>> the ret_pktmbuf_alloc_bulk() routine inplace of rte_mempool_get_bulk().
>> 
> 
> I believe such a change would cause a performance regression, as the extra init code in the alloc_bulk() function would take additional cycles and is not needed. The vector routines use the mempool function directly, so that there is no overhead of mbuf initialization, as the vector routines use their additional "knowledge" of what the mbufs will be used for to init them in a faster manner than can be done inside the mbuf library.
> 
> /Bruce
> 
>> Signed-off-by: Keith Wiles <keith.wiles@windriver.com>
>> ---
>> lib/librte_mbuf/rte_mbuf.h | 77
>> ++++++++++++++++++++++++++++++++++++++++++++++
>> 1 file changed, 77 insertions(+)
>> 
>> diff --git a/lib/librte_mbuf/rte_mbuf.h b/lib/librte_mbuf/rte_mbuf.h
>> index 1c6e115..f298621 100644
>> --- a/lib/librte_mbuf/rte_mbuf.h
>> +++ b/lib/librte_mbuf/rte_mbuf.h
>> @@ -546,6 +546,41 @@ static inline void rte_pktmbuf_reset(struct rte_mbuf
>> *m)
>> }
>> 
>> /**
>> + * @internal Allocate a list of mbufs from mempool *mp*.
>> + * The use of that function is reserved for RTE internal needs.
>> + * Please use rte_pktmbuf_alloc_bulk().
>> + *
>> + * @param mp
>> + *   The mempool from which mbuf is allocated.
>> + * @param m_list
>> + *   The array to place the allocated rte_mbufs pointers.
>> + * @param cnt
>> + *   The number of mbufs to allocate
>> + * @return
>> + *   - 0 if the number of mbufs allocated was ok
>> + *   - <0 is an ERROR.
>> + */
>> +static inline int __rte_mbuf_raw_alloc_bulk(struct rte_mempool *mp, struct
>> rte_mbuf *m_list[], int cnt)
>> +{
>> +     struct rte_mbuf *m;
>> +     int             ret;
>> +
>> +     ret = rte_mempool_get_bulk(mp, (void **)m_list, cnt);
>> +     if ( ret == 0 ) {
>> +             int             i;
>> +             for(i = 0; i < cnt; i++) {
>> +                     m = *m_list++;
>> +#ifdef RTE_MBUF_REFCNT
>> +                     rte_mbuf_refcnt_set(m, 1);
>> +#endif /* RTE_MBUF_REFCNT */
>> +                     rte_pktmbuf_reset(m);
>> +             }
>> +             ret = cnt;
>> +     }
>> +     return ret;
>> +}
>> +
>> +/**
>>  * Allocate a new mbuf from a mempool.
>>  *
>>  * This new mbuf contains one segment, which has a length of 0. The pointer
>> @@ -671,6 +706,32 @@ __rte_pktmbuf_prefree_seg(struct rte_mbuf *m)
>> }
>> 
>> /**
>> + * Allocate a list of mbufs from a mempool into a mbufs array.
>> + *
>> + * This mbuf list contains one segment per mbuf, which has a length of 0. The
>> pointer
>> + * to data is initialized to have some bytes of headroom in the buffer
>> + * (if buffer size allows).
>> + *
>> + * The routine is just a simple wrapper routine to reduce code in the application
>> and
>> + * provide a cleaner API for multiple mbuf requests.
>> + *
>> + * @param mp
>> + *   The mempool from which the mbuf is allocated.
>> + * @param m_list
>> + *   An array of mbuf pointers, cnt must be less then or equal to the size of the
>> list.
>> + * @param cnt
>> + *   Number of slots in the m_list array to fill.
>> + * @return
>> + *   - The number of valid mbufs pointers in the m_list array.
>> + *   - Zero if the request cnt could not be allocated.
>> + */
>> +static inline int __attribute__((always_inline))
>> +rte_pktmbuf_alloc_bulk(struct rte_mempool *mp, struct rte_mbuf *m_list[],
>> int16_t cnt)
>> +{
>> +     return __rte_mbuf_raw_alloc_bulk(mp, m_list, cnt);
>> +}
>> +
>> +/**
>>  * Free a segment of a packet mbuf into its original mempool.
>>  *
>>  * Free an mbuf, without parsing other segments in case of chained
>> @@ -708,6 +769,22 @@ static inline void rte_pktmbuf_free(struct rte_mbuf
>> *m)
>>      }
>> }
>> 
>> +/**
>> + * Free a list of packet mbufs back into its original mempool.
>> + *
>> + * Free a list of mbufs by calling rte_pktmbuf_free() in a loop as a wrapper
>> function.
>> + *
>> + * @param m_list
>> + *   An array of rte_mbuf pointers to be freed.
>> + * @param npkts
>> + *   Number of packets to free in list.
>> + */
>> +static inline void rte_pktmbuf_free_bulk(struct rte_mbuf *m_list[], int16_t
>> npkts)
>> +{
>> +     while(npkts--)
>> +             rte_pktmbuf_free(*m_list++);
>> +}
>> +
>> #ifdef RTE_MBUF_REFCNT
>> 
>> /**
>> --
>> 2.1.0
> 

Keith Wiles, Principal Technologist with CTO office, Wind River mobile 972-213-5533

Re: [dpdk-dev] [PATCH 2/2] Adding the routines rte_pktmbuf_alloc_bulk() and rte_pktmbuf_free_bulk()

[Bruce Richardson]

On Mon, Oct 06, 2014 at 03:50:38PM +0100, Wiles, Roger Keith wrote:
> Hi Bruce,
> 
> Do I need to reject the for the new routines or just make sure the vector driver does not get updated to use those routines?
> 

The new routines are probably useful in the general case. I see no issue 
with having them in the code, so long as the vector driver is not modified 
to use them.

/Bruce

> Thanks
> ++Keith
> 
> On Oct 6, 2014, at 3:56 AM, Richardson, Bruce <bruce.richardson@intel.com> wrote:
> 
> >
> >
> >> -----Original Message-----
> >> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Keith Wiles
> >> Sent: Sunday, October 05, 2014 12:10 AM
> >> To: dev@dpdk.org
> >> Subject: [dpdk-dev] [PATCH 2/2] Adding the routines rte_pktmbuf_alloc_bulk()
> >> and rte_pktmbuf_free_bulk()
> >>
> >> Minor helper routines to mirror the mempool routines and remove the code
> >> from applications. The ixgbe_rxtx_vec.c routine could be changed to use
> >> the ret_pktmbuf_alloc_bulk() routine inplace of rte_mempool_get_bulk().
> >>
> >
> > I believe such a change would cause a performance regression, as the extra init code in the alloc_bulk() function would take additional cycles and is not needed. The vector routines use the mempool function directly, so that there is no overhead of mbuf initialization, as the vector routines use their additional "knowledge" of what the mbufs will be used for to init them in a faster manner than can be done inside the mbuf library.
> >
> > /Bruce
> >
> >> Signed-off-by: Keith Wiles <keith.wiles@windriver.com>
> >> ---
> >> lib/librte_mbuf/rte_mbuf.h | 77
> >> ++++++++++++++++++++++++++++++++++++++++++++++
> >> 1 file changed, 77 insertions(+)
> >>
> >> diff --git a/lib/librte_mbuf/rte_mbuf.h b/lib/librte_mbuf/rte_mbuf.h
> >> index 1c6e115..f298621 100644
> >> --- a/lib/librte_mbuf/rte_mbuf.h
> >> +++ b/lib/librte_mbuf/rte_mbuf.h
> >> @@ -546,6 +546,41 @@ static inline void rte_pktmbuf_reset(struct rte_mbuf
> >> *m)
> >> }
> >>
> >> /**
> >> + * @internal Allocate a list of mbufs from mempool *mp*.
> >> + * The use of that function is reserved for RTE internal needs.
> >> + * Please use rte_pktmbuf_alloc_bulk().
> >> + *
> >> + * @param mp
> >> + *   The mempool from which mbuf is allocated.
> >> + * @param m_list
> >> + *   The array to place the allocated rte_mbufs pointers.
> >> + * @param cnt
> >> + *   The number of mbufs to allocate
> >> + * @return
> >> + *   - 0 if the number of mbufs allocated was ok
> >> + *   - <0 is an ERROR.
> >> + */
> >> +static inline int __rte_mbuf_raw_alloc_bulk(struct rte_mempool *mp, struct
> >> rte_mbuf *m_list[], int cnt)
> >> +{
> >> +     struct rte_mbuf *m;
> >> +     int             ret;
> >> +
> >> +     ret = rte_mempool_get_bulk(mp, (void **)m_list, cnt);
> >> +     if ( ret == 0 ) {
> >> +             int             i;
> >> +             for(i = 0; i < cnt; i++) {
> >> +                     m = *m_list++;
> >> +#ifdef RTE_MBUF_REFCNT
> >> +                     rte_mbuf_refcnt_set(m, 1);
> >> +#endif /* RTE_MBUF_REFCNT */
> >> +                     rte_pktmbuf_reset(m);
> >> +             }
> >> +             ret = cnt;
> >> +     }
> >> +     return ret;
> >> +}
> >> +
> >> +/**
> >>  * Allocate a new mbuf from a mempool.
> >>  *
> >>  * This new mbuf contains one segment, which has a length of 0. The pointer
> >> @@ -671,6 +706,32 @@ __rte_pktmbuf_prefree_seg(struct rte_mbuf *m)
> >> }
> >>
> >> /**
> >> + * Allocate a list of mbufs from a mempool into a mbufs array.
> >> + *
> >> + * This mbuf list contains one segment per mbuf, which has a length of 0. The
> >> pointer
> >> + * to data is initialized to have some bytes of headroom in the buffer
> >> + * (if buffer size allows).
> >> + *
> >> + * The routine is just a simple wrapper routine to reduce code in the application
> >> and
> >> + * provide a cleaner API for multiple mbuf requests.
> >> + *
> >> + * @param mp
> >> + *   The mempool from which the mbuf is allocated.
> >> + * @param m_list
> >> + *   An array of mbuf pointers, cnt must be less then or equal to the size of the
> >> list.
> >> + * @param cnt
> >> + *   Number of slots in the m_list array to fill.
> >> + * @return
> >> + *   - The number of valid mbufs pointers in the m_list array.
> >> + *   - Zero if the request cnt could not be allocated.
> >> + */
> >> +static inline int __attribute__((always_inline))
> >> +rte_pktmbuf_alloc_bulk(struct rte_mempool *mp, struct rte_mbuf *m_list[],
> >> int16_t cnt)
> >> +{
> >> +     return __rte_mbuf_raw_alloc_bulk(mp, m_list, cnt);
> >> +}
> >> +
> >> +/**
> >>  * Free a segment of a packet mbuf into its original mempool.
> >>  *
> >>  * Free an mbuf, without parsing other segments in case of chained
> >> @@ -708,6 +769,22 @@ static inline void rte_pktmbuf_free(struct rte_mbuf
> >> *m)
> >>      }
> >> }
> >>
> >> +/**
> >> + * Free a list of packet mbufs back into its original mempool.
> >> + *
> >> + * Free a list of mbufs by calling rte_pktmbuf_free() in a loop as a wrapper
> >> function.
> >> + *
> >> + * @param m_list
> >> + *   An array of rte_mbuf pointers to be freed.
> >> + * @param npkts
> >> + *   Number of packets to free in list.
> >> + */
> >> +static inline void rte_pktmbuf_free_bulk(struct rte_mbuf *m_list[], int16_t
> >> npkts)
> >> +{
> >> +     while(npkts--)
> >> +             rte_pktmbuf_free(*m_list++);
> >> +}
> >> +
> >> #ifdef RTE_MBUF_REFCNT
> >>
> >> /**
> >> --
> >> 2.1.0
> >
> 
> Keith Wiles, Principal Technologist with CTO office, Wind River mobile 972-213-5533
> 

Re: [dpdk-dev] [PATCH 2/2] Adding the routines rte_pktmbuf_alloc_bulk() and rte_pktmbuf_free_bulk()

[Ananyev, Konstantin]

> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Bruce Richardson
> Sent: Monday, October 06, 2014 3:54 PM
> To: Wiles, Roger Keith (Wind River)
> Cc: dev@dpdk.org
> Subject: Re: [dpdk-dev] [PATCH 2/2] Adding the routines rte_pktmbuf_alloc_bulk() and rte_pktmbuf_free_bulk()
> 
> On Mon, Oct 06, 2014 at 03:50:38PM +0100, Wiles, Roger Keith wrote:
> > Hi Bruce,
> >
> > Do I need to reject the for the new routines or just make sure the vector driver does not get updated to use those routines?
> >
> 
> The new routines are probably useful in the general case. I see no issue
> with having them in the code, so long as the vector driver is not modified
> to use them.

I 'd say the same thing for non-vector RX/TX PMD code-paths too.

BTW, are the new functions comments valid?

+ * @return
+ *   - 0 if the number of mbufs allocated was ok
+ *   - <0 is an ERROR.
+ */
+static inline int __rte_mbuf_raw_alloc_bulk(

Though, as I can see __rte_mbuf_raw_alloc_bulk() returns either:
- number of  allocated mbuf (cnt)
- negative error code

And:
+ * @return
+ *   - The number of valid mbufs pointers in the m_list array.
+ *   - Zero if the request cnt could not be allocated.
+ */
+static inline int __attribute__((always_inline))
+rte_pktmbuf_alloc_bulk(struct rte_mempool *mp, struct rte_mbuf *m_list[], int16_t cnt)
+{
+     return __rte_mbuf_raw_alloc_bulk(mp, m_list, cnt);
+}

Shouldn't be "less than zero if the request cnt could not be allocated."?

BTW, is there any point to have __rte_mbuf_raw_alloc_bulk() at all?
After all, as you are calling rte_pktmbuf_reset() inside it, it doesn't look __raw__ any more.
Might be just put its content into rte_pktmbuf_alloc_bulk() and get rid of it. 

Also wonder, what is the advantage of having multiple counters inside the same loop?
i.e:
+             for(i = 0; i < cnt; i++) {
+                     m = *m_list++;

Why not just:

for(i = 0; i < cnt; i++) {
    m = &m_list[i];

Same for free:
+     while(npkts--)
+             rte_pktmbuf_free(*m_list++);

While not just:
for (i = 0; i < npkts; i++)
      rte_pktmbuf_free(&m_list[i]);

Konstantin

> 
> /Bruce
> 
> > Thanks
> > ++Keith
> >
> > On Oct 6, 2014, at 3:56 AM, Richardson, Bruce <bruce.richardson@intel.com> wrote:
> >
> > >
> > >
> > >> -----Original Message-----
> > >> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Keith Wiles
> > >> Sent: Sunday, October 05, 2014 12:10 AM
> > >> To: dev@dpdk.org
> > >> Subject: [dpdk-dev] [PATCH 2/2] Adding the routines rte_pktmbuf_alloc_bulk()
> > >> and rte_pktmbuf_free_bulk()
> > >>
> > >> Minor helper routines to mirror the mempool routines and remove the code
> > >> from applications. The ixgbe_rxtx_vec.c routine could be changed to use
> > >> the ret_pktmbuf_alloc_bulk() routine inplace of rte_mempool_get_bulk().
> > >>
> > >
> > > I believe such a change would cause a performance regression, as the extra init code in the alloc_bulk() function would take
> additional cycles and is not needed. The vector routines use the mempool function directly, so that there is no overhead of mbuf
> initialization, as the vector routines use their additional "knowledge" of what the mbufs will be used for to init them in a faster manner
> than can be done inside the mbuf library.
> > >
> > > /Bruce
> > >
> > >> Signed-off-by: Keith Wiles <keith.wiles@windriver.com>
> > >> ---
> > >> lib/librte_mbuf/rte_mbuf.h | 77
> > >> ++++++++++++++++++++++++++++++++++++++++++++++
> > >> 1 file changed, 77 insertions(+)
> > >>
> > >> diff --git a/lib/librte_mbuf/rte_mbuf.h b/lib/librte_mbuf/rte_mbuf.h
> > >> index 1c6e115..f298621 100644
> > >> --- a/lib/librte_mbuf/rte_mbuf.h
> > >> +++ b/lib/librte_mbuf/rte_mbuf.h
> > >> @@ -546,6 +546,41 @@ static inline void rte_pktmbuf_reset(struct rte_mbuf
> > >> *m)
> > >> }
> > >>
> > >> /**
> > >> + * @internal Allocate a list of mbufs from mempool *mp*.
> > >> + * The use of that function is reserved for RTE internal needs.
> > >> + * Please use rte_pktmbuf_alloc_bulk().
> > >> + *
> > >> + * @param mp
> > >> + *   The mempool from which mbuf is allocated.
> > >> + * @param m_list
> > >> + *   The array to place the allocated rte_mbufs pointers.
> > >> + * @param cnt
> > >> + *   The number of mbufs to allocate
> > >> + * @return
> > >> + *   - 0 if the number of mbufs allocated was ok
> > >> + *   - <0 is an ERROR.
> > >> + */
> > >> +static inline int __rte_mbuf_raw_alloc_bulk(struct rte_mempool *mp, struct
> > >> rte_mbuf *m_list[], int cnt)
> > >> +{
> > >> +     struct rte_mbuf *m;
> > >> +     int             ret;
> > >> +
> > >> +     ret = rte_mempool_get_bulk(mp, (void **)m_list, cnt);
> > >> +     if ( ret == 0 ) {
> > >> +             int             i;
> > >> +             for(i = 0; i < cnt; i++) {
> > >> +                     m = *m_list++;
> > >> +#ifdef RTE_MBUF_REFCNT
> > >> +                     rte_mbuf_refcnt_set(m, 1);
> > >> +#endif /* RTE_MBUF_REFCNT */
> > >> +                     rte_pktmbuf_reset(m);
> > >> +             }
> > >> +             ret = cnt;
> > >> +     }
> > >> +     return ret;
> > >> +}
> > >> +
> > >> +/**
> > >>  * Allocate a new mbuf from a mempool.
> > >>  *
> > >>  * This new mbuf contains one segment, which has a length of 0. The pointer
> > >> @@ -671,6 +706,32 @@ __rte_pktmbuf_prefree_seg(struct rte_mbuf *m)
> > >> }
> > >>
> > >> /**
> > >> + * Allocate a list of mbufs from a mempool into a mbufs array.
> > >> + *
> > >> + * This mbuf list contains one segment per mbuf, which has a length of 0. The
> > >> pointer
> > >> + * to data is initialized to have some bytes of headroom in the buffer
> > >> + * (if buffer size allows).
> > >> + *
> > >> + * The routine is just a simple wrapper routine to reduce code in the application
> > >> and
> > >> + * provide a cleaner API for multiple mbuf requests.
> > >> + *
> > >> + * @param mp
> > >> + *   The mempool from which the mbuf is allocated.
> > >> + * @param m_list
> > >> + *   An array of mbuf pointers, cnt must be less then or equal to the size of the
> > >> list.
> > >> + * @param cnt
> > >> + *   Number of slots in the m_list array to fill.
> > >> + * @return
> > >> + *   - The number of valid mbufs pointers in the m_list array.
> > >> + *   - Zero if the request cnt could not be allocated.
> > >> + */
> > >> +static inline int __attribute__((always_inline))
> > >> +rte_pktmbuf_alloc_bulk(struct rte_mempool *mp, struct rte_mbuf *m_list[],
> > >> int16_t cnt)
> > >> +{
> > >> +     return __rte_mbuf_raw_alloc_bulk(mp, m_list, cnt);
> > >> +}
> > >> +
> > >> +/**
> > >>  * Free a segment of a packet mbuf into its original mempool.
> > >>  *
> > >>  * Free an mbuf, without parsing other segments in case of chained
> > >> @@ -708,6 +769,22 @@ static inline void rte_pktmbuf_free(struct rte_mbuf
> > >> *m)
> > >>      }
> > >> }
> > >>
> > >> +/**
> > >> + * Free a list of packet mbufs back into its original mempool.
> > >> + *
> > >> + * Free a list of mbufs by calling rte_pktmbuf_free() in a loop as a wrapper
> > >> function.
> > >> + *
> > >> + * @param m_list
> > >> + *   An array of rte_mbuf pointers to be freed.
> > >> + * @param npkts
> > >> + *   Number of packets to free in list.
> > >> + */
> > >> +static inline void rte_pktmbuf_free_bulk(struct rte_mbuf *m_list[], int16_t
> > >> npkts)
> > >> +{
> > >> +     while(npkts--)
> > >> +             rte_pktmbuf_free(*m_list++);
> > >> +}
> > >> +
> > >> #ifdef RTE_MBUF_REFCNT
> > >>
> > >> /**
> > >> --
> > >> 2.1.0
> > >
> >
> > Keith Wiles, Principal Technologist with CTO office, Wind River mobile 972-213-5533
> >

Re: [dpdk-dev] [PATCH 2/2] Adding the routines rte_pktmbuf_alloc_bulk() and rte_pktmbuf_free_bulk()

[Wiles, Roger Keith]

On Oct 6, 2014, at 10:54 AM, Ananyev, Konstantin <konstantin.ananyev@intel.com> wrote:

>> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Bruce Richardson
>> Sent: Monday, October 06, 2014 3:54 PM
>> To: Wiles, Roger Keith (Wind River)
>> Cc: dev@dpdk.org
>> Subject: Re: [dpdk-dev] [PATCH 2/2] Adding the routines rte_pktmbuf_alloc_bulk() and rte_pktmbuf_free_bulk()
>> 
>> On Mon, Oct 06, 2014 at 03:50:38PM +0100, Wiles, Roger Keith wrote:
>>> Hi Bruce,
>>> 
>>> Do I need to reject the for the new routines or just make sure the vector driver does not get updated to use those routines?
>>> 
>> 
>> The new routines are probably useful in the general case. I see no issue
>> with having them in the code, so long as the vector driver is not modified
>> to use them.
> 
> I 'd say the same thing for non-vector RX/TX PMD code-paths too.
> 
> BTW, are the new functions comments valid?
> 
> + * @return
> + *   - 0 if the number of mbufs allocated was ok
> + *   - <0 is an ERROR.
> + */
> +static inline int __rte_mbuf_raw_alloc_bulk(
> 
> Though, as I can see __rte_mbuf_raw_alloc_bulk() returns either:
> - number of  allocated mbuf (cnt)
> - negative error code

Let me fix up the comments.
> 
> And:
> + * @return
> + *   - The number of valid mbufs pointers in the m_list array.
> + *   - Zero if the request cnt could not be allocated.
> + */
> +static inline int __attribute__((always_inline))
> +rte_pktmbuf_alloc_bulk(struct rte_mempool *mp, struct rte_mbuf *m_list[], int16_t cnt)
> +{
> +     return __rte_mbuf_raw_alloc_bulk(mp, m_list, cnt);
> +}
> 
> Shouldn't be "less than zero if the request cnt could not be allocated."?
> 
> BTW, is there any point to have __rte_mbuf_raw_alloc_bulk() at all?
> After all, as you are calling rte_pktmbuf_reset() inside it, it doesn't look __raw__ any more.
> Might be just put its content into rte_pktmbuf_alloc_bulk() and get rid of it.
> 
I was just following the non-bulk routine style __rte_mbuf_raw_alloc(), but I can pull that into a single routine.

> Also wonder, what is the advantage of having multiple counters inside the same loop?
> i.e:
> +             for(i = 0; i < cnt; i++) {
> +                     m = *m_list++;
> 
> Why not just:
> 
> for(i = 0; i < cnt; i++) {
>    m = &m_list[i];
> 
> Same for free:
> +     while(npkts--)
> +             rte_pktmbuf_free(*m_list++);
> 
> While not just:
> for (i = 0; i < npkts; i++)
>      rte_pktmbuf_free(&m_list[i]);

Maybe I have it wrong or the compilers are doing the right thing now, but at one point the &m_list[i] would cause the compiler to generate a shift or multiple of ‘i’ and then add it to the base of m_list. If that is not the case anymore then I can update the code as you suggested. Using the *m_list++ just adds the size of a pointer to a register and continues.
> 
> Konstantin
> 
>> 
>> /Bruce
>> 
>>> Thanks
>>> ++Keith
>>> 
>>> On Oct 6, 2014, at 3:56 AM, Richardson, Bruce <bruce.richardson@intel.com> wrote:
>>> 
>>>> 
>>>> 
>>>>> -----Original Message-----
>>>>> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Keith Wiles
>>>>> Sent: Sunday, October 05, 2014 12:10 AM
>>>>> To: dev@dpdk.org
>>>>> Subject: [dpdk-dev] [PATCH 2/2] Adding the routines rte_pktmbuf_alloc_bulk()
>>>>> and rte_pktmbuf_free_bulk()
>>>>> 
>>>>> Minor helper routines to mirror the mempool routines and remove the code
>>>>> from applications. The ixgbe_rxtx_vec.c routine could be changed to use
>>>>> the ret_pktmbuf_alloc_bulk() routine inplace of rte_mempool_get_bulk().
>>>>> 
>>>> 
>>>> I believe such a change would cause a performance regression, as the extra init code in the alloc_bulk() function would take
>> additional cycles and is not needed. The vector routines use the mempool function directly, so that there is no overhead of mbuf
>> initialization, as the vector routines use their additional "knowledge" of what the mbufs will be used for to init them in a faster manner
>> than can be done inside the mbuf library.
>>>> 
>>>> /Bruce
>>>> 
>>>>> Signed-off-by: Keith Wiles <keith.wiles@windriver.com>
>>>>> ---
>>>>> lib/librte_mbuf/rte_mbuf.h | 77
>>>>> ++++++++++++++++++++++++++++++++++++++++++++++
>>>>> 1 file changed, 77 insertions(+)
>>>>> 
>>>>> diff --git a/lib/librte_mbuf/rte_mbuf.h b/lib/librte_mbuf/rte_mbuf.h
>>>>> index 1c6e115..f298621 100644
>>>>> --- a/lib/librte_mbuf/rte_mbuf.h
>>>>> +++ b/lib/librte_mbuf/rte_mbuf.h
>>>>> @@ -546,6 +546,41 @@ static inline void rte_pktmbuf_reset(struct rte_mbuf
>>>>> *m)
>>>>> }
>>>>> 
>>>>> /**
>>>>> + * @internal Allocate a list of mbufs from mempool *mp*.
>>>>> + * The use of that function is reserved for RTE internal needs.
>>>>> + * Please use rte_pktmbuf_alloc_bulk().
>>>>> + *
>>>>> + * @param mp
>>>>> + *   The mempool from which mbuf is allocated.
>>>>> + * @param m_list
>>>>> + *   The array to place the allocated rte_mbufs pointers.
>>>>> + * @param cnt
>>>>> + *   The number of mbufs to allocate
>>>>> + * @return
>>>>> + *   - 0 if the number of mbufs allocated was ok
>>>>> + *   - <0 is an ERROR.
>>>>> + */
>>>>> +static inline int __rte_mbuf_raw_alloc_bulk(struct rte_mempool *mp, struct
>>>>> rte_mbuf *m_list[], int cnt)
>>>>> +{
>>>>> +     struct rte_mbuf *m;
>>>>> +     int             ret;
>>>>> +
>>>>> +     ret = rte_mempool_get_bulk(mp, (void **)m_list, cnt);
>>>>> +     if ( ret == 0 ) {
>>>>> +             int             i;
>>>>> +             for(i = 0; i < cnt; i++) {
>>>>> +                     m = *m_list++;
>>>>> +#ifdef RTE_MBUF_REFCNT
>>>>> +                     rte_mbuf_refcnt_set(m, 1);
>>>>> +#endif /* RTE_MBUF_REFCNT */
>>>>> +                     rte_pktmbuf_reset(m);
>>>>> +             }
>>>>> +             ret = cnt;
>>>>> +     }
>>>>> +     return ret;
>>>>> +}
>>>>> +
>>>>> +/**
>>>>> * Allocate a new mbuf from a mempool.
>>>>> *
>>>>> * This new mbuf contains one segment, which has a length of 0. The pointer
>>>>> @@ -671,6 +706,32 @@ __rte_pktmbuf_prefree_seg(struct rte_mbuf *m)
>>>>> }
>>>>> 
>>>>> /**
>>>>> + * Allocate a list of mbufs from a mempool into a mbufs array.
>>>>> + *
>>>>> + * This mbuf list contains one segment per mbuf, which has a length of 0. The
>>>>> pointer
>>>>> + * to data is initialized to have some bytes of headroom in the buffer
>>>>> + * (if buffer size allows).
>>>>> + *
>>>>> + * The routine is just a simple wrapper routine to reduce code in the application
>>>>> and
>>>>> + * provide a cleaner API for multiple mbuf requests.
>>>>> + *
>>>>> + * @param mp
>>>>> + *   The mempool from which the mbuf is allocated.
>>>>> + * @param m_list
>>>>> + *   An array of mbuf pointers, cnt must be less then or equal to the size of the
>>>>> list.
>>>>> + * @param cnt
>>>>> + *   Number of slots in the m_list array to fill.
>>>>> + * @return
>>>>> + *   - The number of valid mbufs pointers in the m_list array.
>>>>> + *   - Zero if the request cnt could not be allocated.
>>>>> + */
>>>>> +static inline int __attribute__((always_inline))
>>>>> +rte_pktmbuf_alloc_bulk(struct rte_mempool *mp, struct rte_mbuf *m_list[],
>>>>> int16_t cnt)
>>>>> +{
>>>>> +     return __rte_mbuf_raw_alloc_bulk(mp, m_list, cnt);
>>>>> +}
>>>>> +
>>>>> +/**
>>>>> * Free a segment of a packet mbuf into its original mempool.
>>>>> *
>>>>> * Free an mbuf, without parsing other segments in case of chained
>>>>> @@ -708,6 +769,22 @@ static inline void rte_pktmbuf_free(struct rte_mbuf
>>>>> *m)
>>>>>     }
>>>>> }
>>>>> 
>>>>> +/**
>>>>> + * Free a list of packet mbufs back into its original mempool.
>>>>> + *
>>>>> + * Free a list of mbufs by calling rte_pktmbuf_free() in a loop as a wrapper
>>>>> function.
>>>>> + *
>>>>> + * @param m_list
>>>>> + *   An array of rte_mbuf pointers to be freed.
>>>>> + * @param npkts
>>>>> + *   Number of packets to free in list.
>>>>> + */
>>>>> +static inline void rte_pktmbuf_free_bulk(struct rte_mbuf *m_list[], int16_t
>>>>> npkts)
>>>>> +{
>>>>> +     while(npkts--)
>>>>> +             rte_pktmbuf_free(*m_list++);
>>>>> +}
>>>>> +
>>>>> #ifdef RTE_MBUF_REFCNT
>>>>> 
>>>>> /**
>>>>> --
>>>>> 2.1.0
>>>> 
>>> 
>>> Keith Wiles, Principal Technologist with CTO office, Wind River mobile 972-213-5533

Keith Wiles, Principal Technologist with CTO office, Wind River mobile 972-213-5533

Re: [dpdk-dev] [PATCH 2/2] Adding the routines rte_pktmbuf_alloc_bulk() and rte_pktmbuf_free_bulk()

[Wiles, Roger Keith]

On Oct 6, 2014, at 11:13 AM, Wiles, Roger Keith <keith.wiles@windriver.com> wrote:

>
> On Oct 6, 2014, at 10:54 AM, Ananyev, Konstantin <konstantin.ananyev@intel.com> wrote:
>
>>> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Bruce Richardson
>>> Sent: Monday, October 06, 2014 3:54 PM
>>> To: Wiles, Roger Keith (Wind River)
>>> Cc: dev@dpdk.org
>>> Subject: Re: [dpdk-dev] [PATCH 2/2] Adding the routines rte_pktmbuf_alloc_bulk() and rte_pktmbuf_free_bulk()
>>>
>>> On Mon, Oct 06, 2014 at 03:50:38PM +0100, Wiles, Roger Keith wrote:
>>>> Hi Bruce,
>>>>
>>>> Do I need to reject the for the new routines or just make sure the vector driver does not get updated to use those routines?
>>>>
>>>
>>> The new routines are probably useful in the general case. I see no issue
>>> with having them in the code, so long as the vector driver is not modified
>>> to use them.
>>
>> I 'd say the same thing for non-vector RX/TX PMD code-paths too.
>>
>> BTW, are the new functions comments valid?
>>
>> + * @return
>> + *   - 0 if the number of mbufs allocated was ok
>> + *   - <0 is an ERROR.
>> + */
>> +static inline int __rte_mbuf_raw_alloc_bulk(
>>
>> Though, as I can see __rte_mbuf_raw_alloc_bulk() returns either:
>> - number of  allocated mbuf (cnt)
>> - negative error code
>
> Let me fix up the comments.
>>
>> And:
>> + * @return
>> + *   - The number of valid mbufs pointers in the m_list array.
>> + *   - Zero if the request cnt could not be allocated.
>> + */
>> +static inline int __attribute__((always_inline))
>> +rte_pktmbuf_alloc_bulk(struct rte_mempool *mp, struct rte_mbuf *m_list[], int16_t cnt)
>> +{
>> +     return __rte_mbuf_raw_alloc_bulk(mp, m_list, cnt);
>> +}
>>
>> Shouldn't be "less than zero if the request cnt could not be allocated."?
>>
>> BTW, is there any point to have __rte_mbuf_raw_alloc_bulk() at all?
>> After all, as you are calling rte_pktmbuf_reset() inside it, it doesn't look __raw__ any more.
>> Might be just put its content into rte_pktmbuf_alloc_bulk() and get rid of it.
>>
> I was just following the non-bulk routine style __rte_mbuf_raw_alloc(), but I can pull that into a single routine.
>
>> Also wonder, what is the advantage of having multiple counters inside the same loop?
>> i.e:
>> +             for(i = 0; i < cnt; i++) {
>> +                     m = *m_list++;
>>
>> Why not just:
>>
>> for(i = 0; i < cnt; i++) {
>>   m = &m_list[i];
>>
>> Same for free:
>> +     while(npkts--)
>> +             rte_pktmbuf_free(*m_list++);
>>
>> While not just:
>> for (i = 0; i < npkts; i++)
>>     rte_pktmbuf_free(&m_list[i]);
>
> Maybe I have it wrong or the compilers are doing the right thing now, but at one point the &m_list[i] would cause the compiler to generate a shift or multiple of ‘i’ and then add it to the base of m_list. If that is not the case anymore then I can update the code as you suggested. Using the *m_list++ just adds the size of a pointer to a register and continues.

I compared the clang assembler (.s file) output from an example test code I wrote to see if we have any differences in the code using the two styles and I found no difference and the code looked the same. I am not a Intel assembler expert and I would suggest someone else determine if it generates different code. I tried to compare the GCC outputs and it did look the same to me.

I have attached the code and output, please let me know if I did something wrong, but as it stands using the original style is what I want to go with.

>>
>> Konstantin
>>
>>>
>>> /Bruce
>>>
>>>> Thanks
>>>> ++Keith
>>>>
>>>> On Oct 6, 2014, at 3:56 AM, Richardson, Bruce <bruce.richardson@intel.com> wrote:
>>>>
>>>>>
>>>>>
>>>>>> -----Original Message-----
>>>>>> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Keith Wiles
>>>>>> Sent: Sunday, October 05, 2014 12:10 AM
>>>>>> To: dev@dpdk.org
>>>>>> Subject: [dpdk-dev] [PATCH 2/2] Adding the routines rte_pktmbuf_alloc_bulk()
>>>>>> and rte_pktmbuf_free_bulk()
>>>>>>
>>>>>> Minor helper routines to mirror the mempool routines and remove the code
>>>>>> from applications. The ixgbe_rxtx_vec.c routine could be changed to use
>>>>>> the ret_pktmbuf_alloc_bulk() routine inplace of rte_mempool_get_bulk().
>>>>>>
>>>>>
>>>>> I believe such a change would cause a performance regression, as the extra init code in the alloc_bulk() function would take
>>> additional cycles and is not needed. The vector routines use the mempool function directly, so that there is no overhead of mbuf
>>> initialization, as the vector routines use their additional "knowledge" of what the mbufs will be used for to init them in a faster manner
>>> than can be done inside the mbuf library.
>>>>>
>>>>> /Bruce
>>>>>
>>>>>> Signed-off-by: Keith Wiles <keith.wiles@windriver.com>
>>>>>> ---
>>>>>> lib/librte_mbuf/rte_mbuf.h | 77
>>>>>> ++++++++++++++++++++++++++++++++++++++++++++++
>>>>>> 1 file changed, 77 insertions(+)
>>>>>>
>>>>>> diff --git a/lib/librte_mbuf/rte_mbuf.h b/lib/librte_mbuf/rte_mbuf.h
>>>>>> index 1c6e115..f298621 100644
>>>>>> --- a/lib/librte_mbuf/rte_mbuf.h
>>>>>> +++ b/lib/librte_mbuf/rte_mbuf.h
>>>>>> @@ -546,6 +546,41 @@ static inline void rte_pktmbuf_reset(struct rte_mbuf
>>>>>> *m)
>>>>>> }
>>>>>>
>>>>>> /**
>>>>>> + * @internal Allocate a list of mbufs from mempool *mp*.
>>>>>> + * The use of that function is reserved for RTE internal needs.
>>>>>> + * Please use rte_pktmbuf_alloc_bulk().
>>>>>> + *
>>>>>> + * @param mp
>>>>>> + *   The mempool from which mbuf is allocated.
>>>>>> + * @param m_list
>>>>>> + *   The array to place the allocated rte_mbufs pointers.
>>>>>> + * @param cnt
>>>>>> + *   The number of mbufs to allocate
>>>>>> + * @return
>>>>>> + *   - 0 if the number of mbufs allocated was ok
>>>>>> + *   - <0 is an ERROR.
>>>>>> + */
>>>>>> +static inline int __rte_mbuf_raw_alloc_bulk(struct rte_mempool *mp, struct
>>>>>> rte_mbuf *m_list[], int cnt)
>>>>>> +{
>>>>>> +     struct rte_mbuf *m;
>>>>>> +     int             ret;
>>>>>> +
>>>>>> +     ret = rte_mempool_get_bulk(mp, (void **)m_list, cnt);
>>>>>> +     if ( ret == 0 ) {
>>>>>> +             int             i;
>>>>>> +             for(i = 0; i < cnt; i++) {
>>>>>> +                     m = *m_list++;
>>>>>> +#ifdef RTE_MBUF_REFCNT
>>>>>> +                     rte_mbuf_refcnt_set(m, 1);
>>>>>> +#endif /* RTE_MBUF_REFCNT */
>>>>>> +                     rte_pktmbuf_reset(m);
>>>>>> +             }
>>>>>> +             ret = cnt;
>>>>>> +     }
>>>>>> +     return ret;
>>>>>> +}
>>>>>> +
>>>>>> +/**
>>>>>> * Allocate a new mbuf from a mempool.
>>>>>> *
>>>>>> * This new mbuf contains one segment, which has a length of 0. The pointer
>>>>>> @@ -671,6 +706,32 @@ __rte_pktmbuf_prefree_seg(struct rte_mbuf *m)
>>>>>> }
>>>>>>
>>>>>> /**
>>>>>> + * Allocate a list of mbufs from a mempool into a mbufs array.
>>>>>> + *
>>>>>> + * This mbuf list contains one segment per mbuf, which has a length of 0. The
>>>>>> pointer
>>>>>> + * to data is initialized to have some bytes of headroom in the buffer
>>>>>> + * (if buffer size allows).
>>>>>> + *
>>>>>> + * The routine is just a simple wrapper routine to reduce code in the application
>>>>>> and
>>>>>> + * provide a cleaner API for multiple mbuf requests.
>>>>>> + *
>>>>>> + * @param mp
>>>>>> + *   The mempool from which the mbuf is allocated.
>>>>>> + * @param m_list
>>>>>> + *   An array of mbuf pointers, cnt must be less then or equal to the size of the
>>>>>> list.
>>>>>> + * @param cnt
>>>>>> + *   Number of slots in the m_list array to fill.
>>>>>> + * @return
>>>>>> + *   - The number of valid mbufs pointers in the m_list array.
>>>>>> + *   - Zero if the request cnt could not be allocated.
>>>>>> + */
>>>>>> +static inline int __attribute__((always_inline))
>>>>>> +rte_pktmbuf_alloc_bulk(struct rte_mempool *mp, struct rte_mbuf *m_list[],
>>>>>> int16_t cnt)
>>>>>> +{
>>>>>> +     return __rte_mbuf_raw_alloc_bulk(mp, m_list, cnt);
>>>>>> +}
>>>>>> +
>>>>>> +/**
>>>>>> * Free a segment of a packet mbuf into its original mempool.
>>>>>> *
>>>>>> * Free an mbuf, without parsing other segments in case of chained
>>>>>> @@ -708,6 +769,22 @@ static inline void rte_pktmbuf_free(struct rte_mbuf
>>>>>> *m)
>>>>>>    }
>>>>>> }
>>>>>>
>>>>>> +/**
>>>>>> + * Free a list of packet mbufs back into its original mempool.
>>>>>> + *
>>>>>> + * Free a list of mbufs by calling rte_pktmbuf_free() in a loop as a wrapper
>>>>>> function.
>>>>>> + *
>>>>>> + * @param m_list
>>>>>> + *   An array of rte_mbuf pointers to be freed.
>>>>>> + * @param npkts
>>>>>> + *   Number of packets to free in list.
>>>>>> + */
>>>>>> +static inline void rte_pktmbuf_free_bulk(struct rte_mbuf *m_list[], int16_t
>>>>>> npkts)
>>>>>> +{
>>>>>> +     while(npkts--)
>>>>>> +             rte_pktmbuf_free(*m_list++);
>>>>>> +}
>>>>>> +
>>>>>> #ifdef RTE_MBUF_REFCNT
>>>>>>
>>>>>> /**
>>>>>> --
>>>>>> 2.1.0
>>>>>
>>>>
>>>> Keith Wiles, Principal Technologist with CTO office, Wind River mobile 972-213-5533
>
> Keith Wiles, Principal Technologist with CTO office, Wind River mobile 972-213-5533

Keith Wiles, Principal Technologist with CTO office, Wind River mobile 972-213-5533

Re: [dpdk-dev] [PATCH 2/2] Adding the routines rte_pktmbuf_alloc_bulk() and rte_pktmbuf_free_bulk()

[Wiles, Roger Keith]

Attaching to the list does not work. If you want the code let me know it is only about 5K in size.

On Oct 6, 2014, at 2:45 PM, Wiles, Roger Keith <keith.wiles@windriver.com> wrote:

> 
> On Oct 6, 2014, at 11:13 AM, Wiles, Roger Keith <keith.wiles@windriver.com> wrote:
> 
>> 
>> On Oct 6, 2014, at 10:54 AM, Ananyev, Konstantin <konstantin.ananyev@intel.com> wrote:
>> 
>>>> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Bruce Richardson
>>>> Sent: Monday, October 06, 2014 3:54 PM
>>>> To: Wiles, Roger Keith (Wind River)
>>>> Cc: dev@dpdk.org
>>>> Subject: Re: [dpdk-dev] [PATCH 2/2] Adding the routines rte_pktmbuf_alloc_bulk() and rte_pktmbuf_free_bulk()
>>>> 
>>>> On Mon, Oct 06, 2014 at 03:50:38PM +0100, Wiles, Roger Keith wrote:
>>>>> Hi Bruce,
>>>>> 
>>>>> Do I need to reject the for the new routines or just make sure the vector driver does not get updated to use those routines?
>>>>> 
>>>> 
>>>> The new routines are probably useful in the general case. I see no issue
>>>> with having them in the code, so long as the vector driver is not modified
>>>> to use them.
>>> 
>>> I 'd say the same thing for non-vector RX/TX PMD code-paths too.
>>> 
>>> BTW, are the new functions comments valid?
>>> 
>>> + * @return
>>> + *   - 0 if the number of mbufs allocated was ok
>>> + *   - <0 is an ERROR.
>>> + */
>>> +static inline int __rte_mbuf_raw_alloc_bulk(
>>> 
>>> Though, as I can see __rte_mbuf_raw_alloc_bulk() returns either:
>>> - number of  allocated mbuf (cnt)
>>> - negative error code
>> 
>> Let me fix up the comments.
>>> 
>>> And:
>>> + * @return
>>> + *   - The number of valid mbufs pointers in the m_list array.
>>> + *   - Zero if the request cnt could not be allocated.
>>> + */
>>> +static inline int __attribute__((always_inline))
>>> +rte_pktmbuf_alloc_bulk(struct rte_mempool *mp, struct rte_mbuf *m_list[], int16_t cnt)
>>> +{
>>> +     return __rte_mbuf_raw_alloc_bulk(mp, m_list, cnt);
>>> +}
>>> 
>>> Shouldn't be "less than zero if the request cnt could not be allocated."?
>>> 
>>> BTW, is there any point to have __rte_mbuf_raw_alloc_bulk() at all?
>>> After all, as you are calling rte_pktmbuf_reset() inside it, it doesn't look __raw__ any more.
>>> Might be just put its content into rte_pktmbuf_alloc_bulk() and get rid of it.
>>> 
>> I was just following the non-bulk routine style __rte_mbuf_raw_alloc(), but I can pull that into a single routine.
>> 
>>> Also wonder, what is the advantage of having multiple counters inside the same loop?
>>> i.e:
>>> +             for(i = 0; i < cnt; i++) {
>>> +                     m = *m_list++;
>>> 
>>> Why not just:
>>> 
>>> for(i = 0; i < cnt; i++) {
>>>  m = &m_list[i];
>>> 
>>> Same for free:
>>> +     while(npkts--)
>>> +             rte_pktmbuf_free(*m_list++);
>>> 
>>> While not just:
>>> for (i = 0; i < npkts; i++)
>>>    rte_pktmbuf_free(&m_list[i]);
>> 
>> Maybe I have it wrong or the compilers are doing the right thing now, but at one point the &m_list[i] would cause the compiler to generate a shift or multiple of ‘i’ and then add it to the base of m_list. If that is not the case anymore then I can update the code as you suggested. Using the *m_list++ just adds the size of a pointer to a register and continues.
> 
> I compared the clang assembler (.s file) output from an example test code I wrote to see if we have any differences in the code using the two styles and I found no difference and the code looked the same. I am not a Intel assembler expert and I would suggest someone else determine if it generates different code. I tried to compare the GCC outputs and it did look the same to me.
> 
> I have attached the code and output, please let me know if I did something wrong, but as it stands using the original style is what I want to go with.
> 
>>> 
>>> Konstantin
>>> 
>>>> 
>>>> /Bruce
>>>> 
>>>>> Thanks
>>>>> ++Keith
>>>>> 
>>>>> On Oct 6, 2014, at 3:56 AM, Richardson, Bruce <bruce.richardson@intel.com> wrote:
>>>>> 
>>>>>> 
>>>>>> 
>>>>>>> -----Original Message-----
>>>>>>> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Keith Wiles
>>>>>>> Sent: Sunday, October 05, 2014 12:10 AM
>>>>>>> To: dev@dpdk.org
>>>>>>> Subject: [dpdk-dev] [PATCH 2/2] Adding the routines rte_pktmbuf_alloc_bulk()
>>>>>>> and rte_pktmbuf_free_bulk()
>>>>>>> 
>>>>>>> Minor helper routines to mirror the mempool routines and remove the code
>>>>>>> from applications. The ixgbe_rxtx_vec.c routine could be changed to use
>>>>>>> the ret_pktmbuf_alloc_bulk() routine inplace of rte_mempool_get_bulk().
>>>>>>> 
>>>>>> 
>>>>>> I believe such a change would cause a performance regression, as the extra init code in the alloc_bulk() function would take
>>>> additional cycles and is not needed. The vector routines use the mempool function directly, so that there is no overhead of mbuf
>>>> initialization, as the vector routines use their additional "knowledge" of what the mbufs will be used for to init them in a faster manner
>>>> than can be done inside the mbuf library.
>>>>>> 
>>>>>> /Bruce
>>>>>> 
>>>>>>> Signed-off-by: Keith Wiles <keith.wiles@windriver.com>
>>>>>>> ---
>>>>>>> lib/librte_mbuf/rte_mbuf.h | 77
>>>>>>> ++++++++++++++++++++++++++++++++++++++++++++++
>>>>>>> 1 file changed, 77 insertions(+)
>>>>>>> 
>>>>>>> diff --git a/lib/librte_mbuf/rte_mbuf.h b/lib/librte_mbuf/rte_mbuf.h
>>>>>>> index 1c6e115..f298621 100644
>>>>>>> --- a/lib/librte_mbuf/rte_mbuf.h
>>>>>>> +++ b/lib/librte_mbuf/rte_mbuf.h
>>>>>>> @@ -546,6 +546,41 @@ static inline void rte_pktmbuf_reset(struct rte_mbuf
>>>>>>> *m)
>>>>>>> }
>>>>>>> 
>>>>>>> /**
>>>>>>> + * @internal Allocate a list of mbufs from mempool *mp*.
>>>>>>> + * The use of that function is reserved for RTE internal needs.
>>>>>>> + * Please use rte_pktmbuf_alloc_bulk().
>>>>>>> + *
>>>>>>> + * @param mp
>>>>>>> + *   The mempool from which mbuf is allocated.
>>>>>>> + * @param m_list
>>>>>>> + *   The array to place the allocated rte_mbufs pointers.
>>>>>>> + * @param cnt
>>>>>>> + *   The number of mbufs to allocate
>>>>>>> + * @return
>>>>>>> + *   - 0 if the number of mbufs allocated was ok
>>>>>>> + *   - <0 is an ERROR.
>>>>>>> + */
>>>>>>> +static inline int __rte_mbuf_raw_alloc_bulk(struct rte_mempool *mp, struct
>>>>>>> rte_mbuf *m_list[], int cnt)
>>>>>>> +{
>>>>>>> +     struct rte_mbuf *m;
>>>>>>> +     int             ret;
>>>>>>> +
>>>>>>> +     ret = rte_mempool_get_bulk(mp, (void **)m_list, cnt);
>>>>>>> +     if ( ret == 0 ) {
>>>>>>> +             int             i;
>>>>>>> +             for(i = 0; i < cnt; i++) {
>>>>>>> +                     m = *m_list++;
>>>>>>> +#ifdef RTE_MBUF_REFCNT
>>>>>>> +                     rte_mbuf_refcnt_set(m, 1);
>>>>>>> +#endif /* RTE_MBUF_REFCNT */
>>>>>>> +                     rte_pktmbuf_reset(m);
>>>>>>> +             }
>>>>>>> +             ret = cnt;
>>>>>>> +     }
>>>>>>> +     return ret;
>>>>>>> +}
>>>>>>> +
>>>>>>> +/**
>>>>>>> * Allocate a new mbuf from a mempool.
>>>>>>> *
>>>>>>> * This new mbuf contains one segment, which has a length of 0. The pointer
>>>>>>> @@ -671,6 +706,32 @@ __rte_pktmbuf_prefree_seg(struct rte_mbuf *m)
>>>>>>> }
>>>>>>> 
>>>>>>> /**
>>>>>>> + * Allocate a list of mbufs from a mempool into a mbufs array.
>>>>>>> + *
>>>>>>> + * This mbuf list contains one segment per mbuf, which has a length of 0. The
>>>>>>> pointer
>>>>>>> + * to data is initialized to have some bytes of headroom in the buffer
>>>>>>> + * (if buffer size allows).
>>>>>>> + *
>>>>>>> + * The routine is just a simple wrapper routine to reduce code in the application
>>>>>>> and
>>>>>>> + * provide a cleaner API for multiple mbuf requests.
>>>>>>> + *
>>>>>>> + * @param mp
>>>>>>> + *   The mempool from which the mbuf is allocated.
>>>>>>> + * @param m_list
>>>>>>> + *   An array of mbuf pointers, cnt must be less then or equal to the size of the
>>>>>>> list.
>>>>>>> + * @param cnt
>>>>>>> + *   Number of slots in the m_list array to fill.
>>>>>>> + * @return
>>>>>>> + *   - The number of valid mbufs pointers in the m_list array.
>>>>>>> + *   - Zero if the request cnt could not be allocated.
>>>>>>> + */
>>>>>>> +static inline int __attribute__((always_inline))
>>>>>>> +rte_pktmbuf_alloc_bulk(struct rte_mempool *mp, struct rte_mbuf *m_list[],
>>>>>>> int16_t cnt)
>>>>>>> +{
>>>>>>> +     return __rte_mbuf_raw_alloc_bulk(mp, m_list, cnt);
>>>>>>> +}
>>>>>>> +
>>>>>>> +/**
>>>>>>> * Free a segment of a packet mbuf into its original mempool.
>>>>>>> *
>>>>>>> * Free an mbuf, without parsing other segments in case of chained
>>>>>>> @@ -708,6 +769,22 @@ static inline void rte_pktmbuf_free(struct rte_mbuf
>>>>>>> *m)
>>>>>>>   }
>>>>>>> }
>>>>>>> 
>>>>>>> +/**
>>>>>>> + * Free a list of packet mbufs back into its original mempool.
>>>>>>> + *
>>>>>>> + * Free a list of mbufs by calling rte_pktmbuf_free() in a loop as a wrapper
>>>>>>> function.
>>>>>>> + *
>>>>>>> + * @param m_list
>>>>>>> + *   An array of rte_mbuf pointers to be freed.
>>>>>>> + * @param npkts
>>>>>>> + *   Number of packets to free in list.
>>>>>>> + */
>>>>>>> +static inline void rte_pktmbuf_free_bulk(struct rte_mbuf *m_list[], int16_t
>>>>>>> npkts)
>>>>>>> +{
>>>>>>> +     while(npkts--)
>>>>>>> +             rte_pktmbuf_free(*m_list++);
>>>>>>> +}
>>>>>>> +
>>>>>>> #ifdef RTE_MBUF_REFCNT
>>>>>>> 
>>>>>>> /**
>>>>>>> --
>>>>>>> 2.1.0
>>>>>> 
>>>>> 
>>>>> Keith Wiles, Principal Technologist with CTO office, Wind River mobile 972-213-5533
>> 
>> Keith Wiles, Principal Technologist with CTO office, Wind River mobile 972-213-5533
> 
> Keith Wiles, Principal Technologist with CTO office, Wind River mobile 972-213-5533

Keith Wiles, Principal Technologist with CTO office, Wind River mobile 972-213-5533

Re: [dpdk-dev] [PATCH 2/2] Adding the routines rte_pktmbuf_alloc_bulk() and rte_pktmbuf_free_bulk()

[Ananyev, Konstantin]

> -----Original Message-----
> From: Wiles, Roger Keith [mailto:keith.wiles@windriver.com]
> Sent: Monday, October 06, 2014 9:08 PM
> To: Ananyev, Konstantin
> Cc: dev@dpdk.org
> Subject: Re: [dpdk-dev] [PATCH 2/2] Adding the routines rte_pktmbuf_alloc_bulk() and rte_pktmbuf_free_bulk()
> 
> Attaching to the list does not work. If you want the code let me know it is only about 5K in size.
> 
> On Oct 6, 2014, at 2:45 PM, Wiles, Roger Keith <keith.wiles@windriver.com> wrote:
> 
> >
> > On Oct 6, 2014, at 11:13 AM, Wiles, Roger Keith <keith.wiles@windriver.com> wrote:
> >
> >>
> >> On Oct 6, 2014, at 10:54 AM, Ananyev, Konstantin <konstantin.ananyev@intel.com> wrote:
> >>
> >>>> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Bruce Richardson
> >>>> Sent: Monday, October 06, 2014 3:54 PM
> >>>> To: Wiles, Roger Keith (Wind River)
> >>>> Cc: dev@dpdk.org
> >>>> Subject: Re: [dpdk-dev] [PATCH 2/2] Adding the routines rte_pktmbuf_alloc_bulk() and rte_pktmbuf_free_bulk()
> >>>>
> >>>> On Mon, Oct 06, 2014 at 03:50:38PM +0100, Wiles, Roger Keith wrote:
> >>>>> Hi Bruce,
> >>>>>
> >>>>> Do I need to reject the for the new routines or just make sure the vector driver does not get updated to use those routines?
> >>>>>
> >>>>
> >>>> The new routines are probably useful in the general case. I see no issue
> >>>> with having them in the code, so long as the vector driver is not modified
> >>>> to use them.
> >>>
> >>> I 'd say the same thing for non-vector RX/TX PMD code-paths too.
> >>>
> >>> BTW, are the new functions comments valid?
> >>>
> >>> + * @return
> >>> + *   - 0 if the number of mbufs allocated was ok
> >>> + *   - <0 is an ERROR.
> >>> + */
> >>> +static inline int __rte_mbuf_raw_alloc_bulk(
> >>>
> >>> Though, as I can see __rte_mbuf_raw_alloc_bulk() returns either:
> >>> - number of  allocated mbuf (cnt)
> >>> - negative error code
> >>
> >> Let me fix up the comments.
> >>>
> >>> And:
> >>> + * @return
> >>> + *   - The number of valid mbufs pointers in the m_list array.
> >>> + *   - Zero if the request cnt could not be allocated.
> >>> + */
> >>> +static inline int __attribute__((always_inline))
> >>> +rte_pktmbuf_alloc_bulk(struct rte_mempool *mp, struct rte_mbuf *m_list[], int16_t cnt)
> >>> +{
> >>> +     return __rte_mbuf_raw_alloc_bulk(mp, m_list, cnt);
> >>> +}
> >>>
> >>> Shouldn't be "less than zero if the request cnt could not be allocated."?
> >>>
> >>> BTW, is there any point to have __rte_mbuf_raw_alloc_bulk() at all?
> >>> After all, as you are calling rte_pktmbuf_reset() inside it, it doesn't look __raw__ any more.
> >>> Might be just put its content into rte_pktmbuf_alloc_bulk() and get rid of it.
> >>>
> >> I was just following the non-bulk routine style __rte_mbuf_raw_alloc(), but I can pull that into a single routine.
> >>
> >>> Also wonder, what is the advantage of having multiple counters inside the same loop?
> >>> i.e:
> >>> +             for(i = 0; i < cnt; i++) {
> >>> +                     m = *m_list++;
> >>>
> >>> Why not just:
> >>>
> >>> for(i = 0; i < cnt; i++) {
> >>>  m = &m_list[i];
> >>>
> >>> Same for free:
> >>> +     while(npkts--)
> >>> +             rte_pktmbuf_free(*m_list++);
> >>>
> >>> While not just:
> >>> for (i = 0; i < npkts; i++)
> >>>    rte_pktmbuf_free(&m_list[i]);
> >>
> >> Maybe I have it wrong or the compilers are doing the right thing now, but at one point the &m_list[i] would cause the compiler to
> generate a shift or multiple of 'i' and then add it to the base of m_list. If that is not the case anymore then I can update the code as
> you suggested. Using the *m_list++ just adds the size of a pointer to a register and continues.
> >
> > I compared the clang assembler (.s file) output from an example test code I wrote to see if we have any differences in the code
> using the two styles and I found no difference and the code looked the same. I am not a Intel assembler expert and I would suggest
> someone else determine if it generates different code. I tried to compare the GCC outputs and it did look the same to me.

That's was my question:
Modern compilers are able to generate a good code for a simple loop as above.
So what's the point to use 2 iterators inside the loop, when just one is enough?
Nothing wrong technically, but makes code a bit harder to follow.
Plus, in general, it is a good practise to minimise number of iterators inside the loop, when possible.

Konstantin

> >
> > I have attached the code and output, please let me know if I did something wrong, but as it stands using the original style is what I
> want to go with.
> >
> >>>
> >>> Konstantin
> >>>
> >>>>
> >>>> /Bruce
> >>>>
> >>>>> Thanks
> >>>>> ++Keith
> >>>>>
> >>>>> On Oct 6, 2014, at 3:56 AM, Richardson, Bruce <bruce.richardson@intel.com> wrote:
> >>>>>
> >>>>>>
> >>>>>>
> >>>>>>> -----Original Message-----
> >>>>>>> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Keith Wiles
> >>>>>>> Sent: Sunday, October 05, 2014 12:10 AM
> >>>>>>> To: dev@dpdk.org
> >>>>>>> Subject: [dpdk-dev] [PATCH 2/2] Adding the routines rte_pktmbuf_alloc_bulk()
> >>>>>>> and rte_pktmbuf_free_bulk()
> >>>>>>>
> >>>>>>> Minor helper routines to mirror the mempool routines and remove the code
> >>>>>>> from applications. The ixgbe_rxtx_vec.c routine could be changed to use
> >>>>>>> the ret_pktmbuf_alloc_bulk() routine inplace of rte_mempool_get_bulk().
> >>>>>>>
> >>>>>>
> >>>>>> I believe such a change would cause a performance regression, as the extra init code in the alloc_bulk() function would take
> >>>> additional cycles and is not needed. The vector routines use the mempool function directly, so that there is no overhead of
> mbuf
> >>>> initialization, as the vector routines use their additional "knowledge" of what the mbufs will be used for to init them in a faster
> manner
> >>>> than can be done inside the mbuf library.
> >>>>>>
> >>>>>> /Bruce
> >>>>>>
> >>>>>>> Signed-off-by: Keith Wiles <keith.wiles@windriver.com>
> >>>>>>> ---
> >>>>>>> lib/librte_mbuf/rte_mbuf.h | 77
> >>>>>>> ++++++++++++++++++++++++++++++++++++++++++++++
> >>>>>>> 1 file changed, 77 insertions(+)
> >>>>>>>
> >>>>>>> diff --git a/lib/librte_mbuf/rte_mbuf.h b/lib/librte_mbuf/rte_mbuf.h
> >>>>>>> index 1c6e115..f298621 100644
> >>>>>>> --- a/lib/librte_mbuf/rte_mbuf.h
> >>>>>>> +++ b/lib/librte_mbuf/rte_mbuf.h
> >>>>>>> @@ -546,6 +546,41 @@ static inline void rte_pktmbuf_reset(struct rte_mbuf
> >>>>>>> *m)
> >>>>>>> }
> >>>>>>>
> >>>>>>> /**
> >>>>>>> + * @internal Allocate a list of mbufs from mempool *mp*.
> >>>>>>> + * The use of that function is reserved for RTE internal needs.
> >>>>>>> + * Please use rte_pktmbuf_alloc_bulk().
> >>>>>>> + *
> >>>>>>> + * @param mp
> >>>>>>> + *   The mempool from which mbuf is allocated.
> >>>>>>> + * @param m_list
> >>>>>>> + *   The array to place the allocated rte_mbufs pointers.
> >>>>>>> + * @param cnt
> >>>>>>> + *   The number of mbufs to allocate
> >>>>>>> + * @return
> >>>>>>> + *   - 0 if the number of mbufs allocated was ok
> >>>>>>> + *   - <0 is an ERROR.
> >>>>>>> + */
> >>>>>>> +static inline int __rte_mbuf_raw_alloc_bulk(struct rte_mempool *mp, struct
> >>>>>>> rte_mbuf *m_list[], int cnt)
> >>>>>>> +{
> >>>>>>> +     struct rte_mbuf *m;
> >>>>>>> +     int             ret;
> >>>>>>> +
> >>>>>>> +     ret = rte_mempool_get_bulk(mp, (void **)m_list, cnt);
> >>>>>>> +     if ( ret == 0 ) {
> >>>>>>> +             int             i;
> >>>>>>> +             for(i = 0; i < cnt; i++) {
> >>>>>>> +                     m = *m_list++;
> >>>>>>> +#ifdef RTE_MBUF_REFCNT
> >>>>>>> +                     rte_mbuf_refcnt_set(m, 1);
> >>>>>>> +#endif /* RTE_MBUF_REFCNT */
> >>>>>>> +                     rte_pktmbuf_reset(m);
> >>>>>>> +             }
> >>>>>>> +             ret = cnt;
> >>>>>>> +     }
> >>>>>>> +     return ret;
> >>>>>>> +}
> >>>>>>> +
> >>>>>>> +/**
> >>>>>>> * Allocate a new mbuf from a mempool.
> >>>>>>> *
> >>>>>>> * This new mbuf contains one segment, which has a length of 0. The pointer
> >>>>>>> @@ -671,6 +706,32 @@ __rte_pktmbuf_prefree_seg(struct rte_mbuf *m)
> >>>>>>> }
> >>>>>>>
> >>>>>>> /**
> >>>>>>> + * Allocate a list of mbufs from a mempool into a mbufs array.
> >>>>>>> + *
> >>>>>>> + * This mbuf list contains one segment per mbuf, which has a length of 0. The
> >>>>>>> pointer
> >>>>>>> + * to data is initialized to have some bytes of headroom in the buffer
> >>>>>>> + * (if buffer size allows).
> >>>>>>> + *
> >>>>>>> + * The routine is just a simple wrapper routine to reduce code in the application
> >>>>>>> and
> >>>>>>> + * provide a cleaner API for multiple mbuf requests.
> >>>>>>> + *
> >>>>>>> + * @param mp
> >>>>>>> + *   The mempool from which the mbuf is allocated.
> >>>>>>> + * @param m_list
> >>>>>>> + *   An array of mbuf pointers, cnt must be less then or equal to the size of the
> >>>>>>> list.
> >>>>>>> + * @param cnt
> >>>>>>> + *   Number of slots in the m_list array to fill.
> >>>>>>> + * @return
> >>>>>>> + *   - The number of valid mbufs pointers in the m_list array.
> >>>>>>> + *   - Zero if the request cnt could not be allocated.
> >>>>>>> + */
> >>>>>>> +static inline int __attribute__((always_inline))
> >>>>>>> +rte_pktmbuf_alloc_bulk(struct rte_mempool *mp, struct rte_mbuf *m_list[],
> >>>>>>> int16_t cnt)
> >>>>>>> +{
> >>>>>>> +     return __rte_mbuf_raw_alloc_bulk(mp, m_list, cnt);
> >>>>>>> +}
> >>>>>>> +
> >>>>>>> +/**
> >>>>>>> * Free a segment of a packet mbuf into its original mempool.
> >>>>>>> *
> >>>>>>> * Free an mbuf, without parsing other segments in case of chained
> >>>>>>> @@ -708,6 +769,22 @@ static inline void rte_pktmbuf_free(struct rte_mbuf
> >>>>>>> *m)
> >>>>>>>   }
> >>>>>>> }
> >>>>>>>
> >>>>>>> +/**
> >>>>>>> + * Free a list of packet mbufs back into its original mempool.
> >>>>>>> + *
> >>>>>>> + * Free a list of mbufs by calling rte_pktmbuf_free() in a loop as a wrapper
> >>>>>>> function.
> >>>>>>> + *
> >>>>>>> + * @param m_list
> >>>>>>> + *   An array of rte_mbuf pointers to be freed.
> >>>>>>> + * @param npkts
> >>>>>>> + *   Number of packets to free in list.
> >>>>>>> + */
> >>>>>>> +static inline void rte_pktmbuf_free_bulk(struct rte_mbuf *m_list[], int16_t
> >>>>>>> npkts)
> >>>>>>> +{
> >>>>>>> +     while(npkts--)
> >>>>>>> +             rte_pktmbuf_free(*m_list++);
> >>>>>>> +}
> >>>>>>> +
> >>>>>>> #ifdef RTE_MBUF_REFCNT
> >>>>>>>
> >>>>>>> /**
> >>>>>>> --
> >>>>>>> 2.1.0
> >>>>>>
> >>>>>
> >>>>> Keith Wiles, Principal Technologist with CTO office, Wind River mobile 972-213-5533
> >>
> >> Keith Wiles, Principal Technologist with CTO office, Wind River mobile 972-213-5533
> >
> > Keith Wiles, Principal Technologist with CTO office, Wind River mobile 972-213-5533
> 
> Keith Wiles, Principal Technologist with CTO office, Wind River mobile 972-213-5533
> 

Re: [dpdk-dev] [PATCH 2/2] Adding the routines rte_pktmbuf_alloc_bulk() and rte_pktmbuf_free_bulk()

[Wiles, Roger Keith]

On Oct 7, 2014, at 4:09 AM, Ananyev, Konstantin <konstantin.ananyev@intel.com> wrote:

> 
> 
>> -----Original Message-----
>> From: Wiles, Roger Keith [mailto:keith.wiles@windriver.com]
>> Sent: Monday, October 06, 2014 9:08 PM
>> To: Ananyev, Konstantin
>> Cc: dev@dpdk.org
>> Subject: Re: [dpdk-dev] [PATCH 2/2] Adding the routines rte_pktmbuf_alloc_bulk() and rte_pktmbuf_free_bulk()
>> 
>> Attaching to the list does not work. If you want the code let me know it is only about 5K in size.
>> 
>> On Oct 6, 2014, at 2:45 PM, Wiles, Roger Keith <keith.wiles@windriver.com> wrote:
>> 
>>> 
>>> On Oct 6, 2014, at 11:13 AM, Wiles, Roger Keith <keith.wiles@windriver.com> wrote:
>>> 
>>>> 
>>>> On Oct 6, 2014, at 10:54 AM, Ananyev, Konstantin <konstantin.ananyev@intel.com> wrote:
>>>> 
>>>>>> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Bruce Richardson
>>>>>> Sent: Monday, October 06, 2014 3:54 PM
>>>>>> To: Wiles, Roger Keith (Wind River)
>>>>>> Cc: dev@dpdk.org
>>>>>> Subject: Re: [dpdk-dev] [PATCH 2/2] Adding the routines rte_pktmbuf_alloc_bulk() and rte_pktmbuf_free_bulk()
>>>>>> 
>>>>>> On Mon, Oct 06, 2014 at 03:50:38PM +0100, Wiles, Roger Keith wrote:
>>>>>>> Hi Bruce,
>>>>>>> 
>>>>>>> Do I need to reject the for the new routines or just make sure the vector driver does not get updated to use those routines?
>>>>>>> 
>>>>>> 
>>>>>> The new routines are probably useful in the general case. I see no issue
>>>>>> with having them in the code, so long as the vector driver is not modified
>>>>>> to use them.
>>>>> 
>>>>> I 'd say the same thing for non-vector RX/TX PMD code-paths too.
>>>>> 
>>>>> BTW, are the new functions comments valid?
>>>>> 
>>>>> + * @return
>>>>> + *   - 0 if the number of mbufs allocated was ok
>>>>> + *   - <0 is an ERROR.
>>>>> + */
>>>>> +static inline int __rte_mbuf_raw_alloc_bulk(
>>>>> 
>>>>> Though, as I can see __rte_mbuf_raw_alloc_bulk() returns either:
>>>>> - number of  allocated mbuf (cnt)
>>>>> - negative error code
>>>> 
>>>> Let me fix up the comments.
>>>>> 
>>>>> And:
>>>>> + * @return
>>>>> + *   - The number of valid mbufs pointers in the m_list array.
>>>>> + *   - Zero if the request cnt could not be allocated.
>>>>> + */
>>>>> +static inline int __attribute__((always_inline))
>>>>> +rte_pktmbuf_alloc_bulk(struct rte_mempool *mp, struct rte_mbuf *m_list[], int16_t cnt)
>>>>> +{
>>>>> +     return __rte_mbuf_raw_alloc_bulk(mp, m_list, cnt);
>>>>> +}
>>>>> 
>>>>> Shouldn't be "less than zero if the request cnt could not be allocated."?
>>>>> 
>>>>> BTW, is there any point to have __rte_mbuf_raw_alloc_bulk() at all?
>>>>> After all, as you are calling rte_pktmbuf_reset() inside it, it doesn't look __raw__ any more.
>>>>> Might be just put its content into rte_pktmbuf_alloc_bulk() and get rid of it.
>>>>> 
>>>> I was just following the non-bulk routine style __rte_mbuf_raw_alloc(), but I can pull that into a single routine.
>>>> 
>>>>> Also wonder, what is the advantage of having multiple counters inside the same loop?
>>>>> i.e:
>>>>> +             for(i = 0; i < cnt; i++) {
>>>>> +                     m = *m_list++;
>>>>> 
>>>>> Why not just:
>>>>> 
>>>>> for(i = 0; i < cnt; i++) {
>>>>> m = &m_list[i];
>>>>> 
>>>>> Same for free:
>>>>> +     while(npkts--)
>>>>> +             rte_pktmbuf_free(*m_list++);
>>>>> 
>>>>> While not just:
>>>>> for (i = 0; i < npkts; i++)
>>>>>   rte_pktmbuf_free(&m_list[i]);
>>>> 
>>>> Maybe I have it wrong or the compilers are doing the right thing now, but at one point the &m_list[i] would cause the compiler to
>> generate a shift or multiple of 'i' and then add it to the base of m_list. If that is not the case anymore then I can update the code as
>> you suggested. Using the *m_list++ just adds the size of a pointer to a register and continues.
>>> 
>>> I compared the clang assembler (.s file) output from an example test code I wrote to see if we have any differences in the code
>> using the two styles and I found no difference and the code looked the same. I am not a Intel assembler expert and I would suggest
>> someone else determine if it generates different code. I tried to compare the GCC outputs and it did look the same to me.
> 
> That's was my question:
> Modern compilers are able to generate a good code for a simple loop as above.
> So what's the point to use 2 iterators inside the loop, when just one is enough?
> Nothing wrong technically, but makes code a bit harder to follow.
> Plus, in general, it is a good practise to minimise number of iterators inside the loop, when possible.
> 
> Konstantin

Hi Konstantin,

I really do not understand the concern if the code is the same, as it appears to me the current patch is very clean and simple. Maybe you have not seen the v2 patch and now v3 patch I sent this morning to fix Bruce’s comment suggestion.

For the case of the free routine your suggestion would require an extra counter/variable a bit more code a ‘for’ loop instead of a ‘while’ loop. 
+static inline void __attribute__((always_inline))
+rte_pktmbuf_free_bulk(struct rte_mbuf *m_list[], int16_t npkts)
+{
+	while(npkts--)
+		rte_pktmbuf_free(*m_list++);
+}

For the case of the alloc routine I did remove the rte_mbuf * m variable and now I believe it is very clean and changing it to use index variables is just a personal preference. I personal preference of this type is not useful IMO and does not cause any harm. Unless you can suggest a good technical reason to change I am going to leave the patch as is.

+static inline int __attribute__((always_inline))
+rte_pktmbuf_alloc_bulk(struct rte_mempool *mp, struct rte_mbuf *m_list[], int16_t cnt)
+{
+   int     ret;
+
+   ret = rte_mempool_get_bulk(mp, (void **)m_list, cnt);
+   if ( ret == 0 ) {
+       ret = cnt;
+       while(cnt--) {
+#ifdef RTE_MBUF_REFCNT
+           rte_mbuf_refcnt_set(*m_list, 1);
+#endif /* RTE_MBUF_REFCNT */
+           rte_pktmbuf_reset(*m_list++);
+       }
+   }
+   return ret;
+}

>>> 
>>> I have attached the code and output, please let me know if I did something wrong, but as it stands using the original style is what I
>> want to go with.
>>> 
>>>>> 
>>>>> Konstantin
>>>>> 
>>>>>> 
>>>>>> /Bruce
>>>>>> 
>>>>>>> Thanks
>>>>>>> ++Keith
>>>>>>> 
>>>>>>> On Oct 6, 2014, at 3:56 AM, Richardson, Bruce <bruce.richardson@intel.com> wrote:
>>>>>>> 
>>>>>>>> 
>>>>>>>> 
>>>>>>>>> -----Original Message-----
>>>>>>>>> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Keith Wiles
>>>>>>>>> Sent: Sunday, October 05, 2014 12:10 AM
>>>>>>>>> To: dev@dpdk.org
>>>>>>>>> Subject: [dpdk-dev] [PATCH 2/2] Adding the routines rte_pktmbuf_alloc_bulk()
>>>>>>>>> and rte_pktmbuf_free_bulk()
>>>>>>>>> 
>>>>>>>>> Minor helper routines to mirror the mempool routines and remove the code
>>>>>>>>> from applications. The ixgbe_rxtx_vec.c routine could be changed to use
>>>>>>>>> the ret_pktmbuf_alloc_bulk() routine inplace of rte_mempool_get_bulk().
>>>>>>>>> 
>>>>>>>> 
>>>>>>>> I believe such a change would cause a performance regression, as the extra init code in the alloc_bulk() function would take
>>>>>> additional cycles and is not needed. The vector routines use the mempool function directly, so that there is no overhead of
>> mbuf
>>>>>> initialization, as the vector routines use their additional "knowledge" of what the mbufs will be used for to init them in a faster
>> manner
>>>>>> than can be done inside the mbuf library.
>>>>>>>> 
>>>>>>>> /Bruce
>>>>>>>> 
>>>>>>>>> Signed-off-by: Keith Wiles <keith.wiles@windriver.com>
>>>>>>>>> ---
>>>>>>>>> lib/librte_mbuf/rte_mbuf.h | 77
>>>>>>>>> ++++++++++++++++++++++++++++++++++++++++++++++
>>>>>>>>> 1 file changed, 77 insertions(+)
>>>>>>>>> 
>>>>>>>>> diff --git a/lib/librte_mbuf/rte_mbuf.h b/lib/librte_mbuf/rte_mbuf.h
>>>>>>>>> index 1c6e115..f298621 100644
>>>>>>>>> --- a/lib/librte_mbuf/rte_mbuf.h
>>>>>>>>> +++ b/lib/librte_mbuf/rte_mbuf.h
>>>>>>>>> @@ -546,6 +546,41 @@ static inline void rte_pktmbuf_reset(struct rte_mbuf
>>>>>>>>> *m)
>>>>>>>>> }
>>>>>>>>> 
>>>>>>>>> /**
>>>>>>>>> + * @internal Allocate a list of mbufs from mempool *mp*.
>>>>>>>>> + * The use of that function is reserved for RTE internal needs.
>>>>>>>>> + * Please use rte_pktmbuf_alloc_bulk().
>>>>>>>>> + *
>>>>>>>>> + * @param mp
>>>>>>>>> + *   The mempool from which mbuf is allocated.
>>>>>>>>> + * @param m_list
>>>>>>>>> + *   The array to place the allocated rte_mbufs pointers.
>>>>>>>>> + * @param cnt
>>>>>>>>> + *   The number of mbufs to allocate
>>>>>>>>> + * @return
>>>>>>>>> + *   - 0 if the number of mbufs allocated was ok
>>>>>>>>> + *   - <0 is an ERROR.
>>>>>>>>> + */
>>>>>>>>> +static inline int __rte_mbuf_raw_alloc_bulk(struct rte_mempool *mp, struct
>>>>>>>>> rte_mbuf *m_list[], int cnt)
>>>>>>>>> +{
>>>>>>>>> +     struct rte_mbuf *m;
>>>>>>>>> +     int             ret;
>>>>>>>>> +
>>>>>>>>> +     ret = rte_mempool_get_bulk(mp, (void **)m_list, cnt);
>>>>>>>>> +     if ( ret == 0 ) {
>>>>>>>>> +             int             i;
>>>>>>>>> +             for(i = 0; i < cnt; i++) {
>>>>>>>>> +                     m = *m_list++;
>>>>>>>>> +#ifdef RTE_MBUF_REFCNT
>>>>>>>>> +                     rte_mbuf_refcnt_set(m, 1);
>>>>>>>>> +#endif /* RTE_MBUF_REFCNT */
>>>>>>>>> +                     rte_pktmbuf_reset(m);
>>>>>>>>> +             }
>>>>>>>>> +             ret = cnt;
>>>>>>>>> +     }
>>>>>>>>> +     return ret;
>>>>>>>>> +}
>>>>>>>>> +
>>>>>>>>> +/**
>>>>>>>>> * Allocate a new mbuf from a mempool.
>>>>>>>>> *
>>>>>>>>> * This new mbuf contains one segment, which has a length of 0. The pointer
>>>>>>>>> @@ -671,6 +706,32 @@ __rte_pktmbuf_prefree_seg(struct rte_mbuf *m)
>>>>>>>>> }
>>>>>>>>> 
>>>>>>>>> /**
>>>>>>>>> + * Allocate a list of mbufs from a mempool into a mbufs array.
>>>>>>>>> + *
>>>>>>>>> + * This mbuf list contains one segment per mbuf, which has a length of 0. The
>>>>>>>>> pointer
>>>>>>>>> + * to data is initialized to have some bytes of headroom in the buffer
>>>>>>>>> + * (if buffer size allows).
>>>>>>>>> + *
>>>>>>>>> + * The routine is just a simple wrapper routine to reduce code in the application
>>>>>>>>> and
>>>>>>>>> + * provide a cleaner API for multiple mbuf requests.
>>>>>>>>> + *
>>>>>>>>> + * @param mp
>>>>>>>>> + *   The mempool from which the mbuf is allocated.
>>>>>>>>> + * @param m_list
>>>>>>>>> + *   An array of mbuf pointers, cnt must be less then or equal to the size of the
>>>>>>>>> list.
>>>>>>>>> + * @param cnt
>>>>>>>>> + *   Number of slots in the m_list array to fill.
>>>>>>>>> + * @return
>>>>>>>>> + *   - The number of valid mbufs pointers in the m_list array.
>>>>>>>>> + *   - Zero if the request cnt could not be allocated.
>>>>>>>>> + */
>>>>>>>>> +static inline int __attribute__((always_inline))
>>>>>>>>> +rte_pktmbuf_alloc_bulk(struct rte_mempool *mp, struct rte_mbuf *m_list[],
>>>>>>>>> int16_t cnt)
>>>>>>>>> +{
>>>>>>>>> +     return __rte_mbuf_raw_alloc_bulk(mp, m_list, cnt);
>>>>>>>>> +}
>>>>>>>>> +
>>>>>>>>> +/**
>>>>>>>>> * Free a segment of a packet mbuf into its original mempool.
>>>>>>>>> *
>>>>>>>>> * Free an mbuf, without parsing other segments in case of chained
>>>>>>>>> @@ -708,6 +769,22 @@ static inline void rte_pktmbuf_free(struct rte_mbuf
>>>>>>>>> *m)
>>>>>>>>>  }
>>>>>>>>> }
>>>>>>>>> 
>>>>>>>>> +/**
>>>>>>>>> + * Free a list of packet mbufs back into its original mempool.
>>>>>>>>> + *
>>>>>>>>> + * Free a list of mbufs by calling rte_pktmbuf_free() in a loop as a wrapper
>>>>>>>>> function.
>>>>>>>>> + *
>>>>>>>>> + * @param m_list
>>>>>>>>> + *   An array of rte_mbuf pointers to be freed.
>>>>>>>>> + * @param npkts
>>>>>>>>> + *   Number of packets to free in list.
>>>>>>>>> + */
>>>>>>>>> +static inline void rte_pktmbuf_free_bulk(struct rte_mbuf *m_list[], int16_t
>>>>>>>>> npkts)
>>>>>>>>> +{
>>>>>>>>> +     while(npkts--)
>>>>>>>>> +             rte_pktmbuf_free(*m_list++);
>>>>>>>>> +}
>>>>>>>>> +
>>>>>>>>> #ifdef RTE_MBUF_REFCNT
>>>>>>>>> 
>>>>>>>>> /**
>>>>>>>>> --
>>>>>>>>> 2.1.0
>>>>>>>> 
>>>>>>> 
>>>>>>> Keith Wiles, Principal Technologist with CTO office, Wind River mobile 972-213-5533
>>>> 
>>>> Keith Wiles, Principal Technologist with CTO office, Wind River mobile 972-213-5533
>>> 
>>> Keith Wiles, Principal Technologist with CTO office, Wind River mobile 972-213-5533
>> 
>> Keith Wiles, Principal Technologist with CTO office, Wind River mobile 972-213-5533

Keith Wiles, Principal Technologist with CTO office, Wind River mobile 972-213-5533

Re: [dpdk-dev] [PATCH 2/2] Adding the routines rte_pktmbuf_alloc_bulk() and rte_pktmbuf_free_bulk()

[Ananyev, Konstantin]

Hi Keith,

> -----Original Message-----
> From: Wiles, Roger Keith [mailto:keith.wiles@windriver.com]
> Sent: Tuesday, October 07, 2014 3:22 PM
> To: Ananyev, Konstantin
> Cc: dev@dpdk.org
> Subject: Re: [dpdk-dev] [PATCH 2/2] Adding the routines rte_pktmbuf_alloc_bulk() and rte_pktmbuf_free_bulk()
> 
> 
> On Oct 7, 2014, at 4:09 AM, Ananyev, Konstantin <konstantin.ananyev@intel.com> wrote:
> 
> >
> >
> >> -----Original Message-----
> >> From: Wiles, Roger Keith [mailto:keith.wiles@windriver.com]
> >> Sent: Monday, October 06, 2014 9:08 PM
> >> To: Ananyev, Konstantin
> >> Cc: dev@dpdk.org
> >> Subject: Re: [dpdk-dev] [PATCH 2/2] Adding the routines rte_pktmbuf_alloc_bulk() and rte_pktmbuf_free_bulk()
> >>
> >> Attaching to the list does not work. If you want the code let me know it is only about 5K in size.
> >>
> >> On Oct 6, 2014, at 2:45 PM, Wiles, Roger Keith <keith.wiles@windriver.com> wrote:
> >>
> >>>
> >>> On Oct 6, 2014, at 11:13 AM, Wiles, Roger Keith <keith.wiles@windriver.com> wrote:
> >>>
> >>>>
> >>>> On Oct 6, 2014, at 10:54 AM, Ananyev, Konstantin <konstantin.ananyev@intel.com> wrote:
> >>>>
> >>>>>> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Bruce Richardson
> >>>>>> Sent: Monday, October 06, 2014 3:54 PM
> >>>>>> To: Wiles, Roger Keith (Wind River)
> >>>>>> Cc: dev@dpdk.org
> >>>>>> Subject: Re: [dpdk-dev] [PATCH 2/2] Adding the routines rte_pktmbuf_alloc_bulk() and rte_pktmbuf_free_bulk()
> >>>>>>
> >>>>>> On Mon, Oct 06, 2014 at 03:50:38PM +0100, Wiles, Roger Keith wrote:
> >>>>>>> Hi Bruce,
> >>>>>>>
> >>>>>>> Do I need to reject the for the new routines or just make sure the vector driver does not get updated to use those routines?
> >>>>>>>
> >>>>>>
> >>>>>> The new routines are probably useful in the general case. I see no issue
> >>>>>> with having them in the code, so long as the vector driver is not modified
> >>>>>> to use them.
> >>>>>
> >>>>> I 'd say the same thing for non-vector RX/TX PMD code-paths too.
> >>>>>
> >>>>> BTW, are the new functions comments valid?
> >>>>>
> >>>>> + * @return
> >>>>> + *   - 0 if the number of mbufs allocated was ok
> >>>>> + *   - <0 is an ERROR.
> >>>>> + */
> >>>>> +static inline int __rte_mbuf_raw_alloc_bulk(
> >>>>>
> >>>>> Though, as I can see __rte_mbuf_raw_alloc_bulk() returns either:
> >>>>> - number of  allocated mbuf (cnt)
> >>>>> - negative error code
> >>>>
> >>>> Let me fix up the comments.
> >>>>>
> >>>>> And:
> >>>>> + * @return
> >>>>> + *   - The number of valid mbufs pointers in the m_list array.
> >>>>> + *   - Zero if the request cnt could not be allocated.
> >>>>> + */
> >>>>> +static inline int __attribute__((always_inline))
> >>>>> +rte_pktmbuf_alloc_bulk(struct rte_mempool *mp, struct rte_mbuf *m_list[], int16_t cnt)
> >>>>> +{
> >>>>> +     return __rte_mbuf_raw_alloc_bulk(mp, m_list, cnt);
> >>>>> +}
> >>>>>
> >>>>> Shouldn't be "less than zero if the request cnt could not be allocated."?
> >>>>>
> >>>>> BTW, is there any point to have __rte_mbuf_raw_alloc_bulk() at all?
> >>>>> After all, as you are calling rte_pktmbuf_reset() inside it, it doesn't look __raw__ any more.
> >>>>> Might be just put its content into rte_pktmbuf_alloc_bulk() and get rid of it.
> >>>>>
> >>>> I was just following the non-bulk routine style __rte_mbuf_raw_alloc(), but I can pull that into a single routine.
> >>>>
> >>>>> Also wonder, what is the advantage of having multiple counters inside the same loop?
> >>>>> i.e:
> >>>>> +             for(i = 0; i < cnt; i++) {
> >>>>> +                     m = *m_list++;
> >>>>>
> >>>>> Why not just:
> >>>>>
> >>>>> for(i = 0; i < cnt; i++) {
> >>>>> m = &m_list[i];
> >>>>>
> >>>>> Same for free:
> >>>>> +     while(npkts--)
> >>>>> +             rte_pktmbuf_free(*m_list++);
> >>>>>
> >>>>> While not just:
> >>>>> for (i = 0; i < npkts; i++)
> >>>>>   rte_pktmbuf_free(&m_list[i]);
> >>>>
> >>>> Maybe I have it wrong or the compilers are doing the right thing now, but at one point the &m_list[i] would cause the compiler
> to
> >> generate a shift or multiple of 'i' and then add it to the base of m_list. If that is not the case anymore then I can update the code as
> >> you suggested. Using the *m_list++ just adds the size of a pointer to a register and continues.
> >>>
> >>> I compared the clang assembler (.s file) output from an example test code I wrote to see if we have any differences in the code
> >> using the two styles and I found no difference and the code looked the same. I am not a Intel assembler expert and I would
> suggest
> >> someone else determine if it generates different code. I tried to compare the GCC outputs and it did look the same to me.
> >
> > That's was my question:
> > Modern compilers are able to generate a good code for a simple loop as above.
> > So what's the point to use 2 iterators inside the loop, when just one is enough?
> > Nothing wrong technically, but makes code a bit harder to follow.
> > Plus, in general, it is a good practise to minimise number of iterators inside the loop, when possible.
> >
> > Konstantin
> 
> Hi Konstantin,
> 
> I really do not understand the concern if the code is the same, as it appears to me the current patch is very clean and simple. Maybe
> you have not seen the v2 patch and now v3 patch I sent this morning to fix Bruce's comment suggestion.
> 
> For the case of the free routine your suggestion would require an extra counter/variable a bit more code a 'for' loop instead of a
> 'while' loop.

My point was that just one iterator for both loops is enough.
In general, it is a good practise to minimise number of iterators per loop if possible:
in some cases  compiler might get confused and wouldn't be able to eliminate redundant  iterators itself.  
Though yes - technically there is nothing wrong with your approach.
So if you prefer to keep it as it is - I wouldn't insist. 

Konstantin

> +static inline void __attribute__((always_inline))
> +rte_pktmbuf_free_bulk(struct rte_mbuf *m_list[], int16_t npkts)
> +{
> +	while(npkts--)
> +		rte_pktmbuf_free(*m_list++);
> +}
> 
> For the case of the alloc routine I did remove the rte_mbuf * m variable and now I believe it is very clean and changing it to use index
> variables is just a personal preference. I personal preference of this type is not useful IMO and does not cause any harm. Unless you
> can suggest a good technical reason to change I am going to leave the patch as is.
> 
> +static inline int __attribute__((always_inline))
> +rte_pktmbuf_alloc_bulk(struct rte_mempool *mp, struct rte_mbuf *m_list[], int16_t cnt)
> +{
> +   int     ret;
> +
> +   ret = rte_mempool_get_bulk(mp, (void **)m_list, cnt);
> +   if ( ret == 0 ) {
> +       ret = cnt;
> +       while(cnt--) {
> +#ifdef RTE_MBUF_REFCNT
> +           rte_mbuf_refcnt_set(*m_list, 1);
> +#endif /* RTE_MBUF_REFCNT */
> +           rte_pktmbuf_reset(*m_list++);
> +       }
> +   }
> +   return ret;
> +}
> 
> >>>
> >>> I have attached the code and output, please let me know if I did something wrong, but as it stands using the original style is what I
> >> want to go with.
> >>>
> >>>>>
> >>>>> Konstantin
> >>>>>
> >>>>>>
> >>>>>> /Bruce
> >>>>>>
> >>>>>>> Thanks
> >>>>>>> ++Keith
> >>>>>>>
> >>>>>>> On Oct 6, 2014, at 3:56 AM, Richardson, Bruce <bruce.richardson@intel.com> wrote:
> >>>>>>>
> >>>>>>>>
> >>>>>>>>
> >>>>>>>>> -----Original Message-----
> >>>>>>>>> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Keith Wiles
> >>>>>>>>> Sent: Sunday, October 05, 2014 12:10 AM
> >>>>>>>>> To: dev@dpdk.org
> >>>>>>>>> Subject: [dpdk-dev] [PATCH 2/2] Adding the routines rte_pktmbuf_alloc_bulk()
> >>>>>>>>> and rte_pktmbuf_free_bulk()
> >>>>>>>>>
> >>>>>>>>> Minor helper routines to mirror the mempool routines and remove the code
> >>>>>>>>> from applications. The ixgbe_rxtx_vec.c routine could be changed to use
> >>>>>>>>> the ret_pktmbuf_alloc_bulk() routine inplace of rte_mempool_get_bulk().
> >>>>>>>>>
> >>>>>>>>
> >>>>>>>> I believe such a change would cause a performance regression, as the extra init code in the alloc_bulk() function would
> take
> >>>>>> additional cycles and is not needed. The vector routines use the mempool function directly, so that there is no overhead of
> >> mbuf
> >>>>>> initialization, as the vector routines use their additional "knowledge" of what the mbufs will be used for to init them in a faster
> >> manner
> >>>>>> than can be done inside the mbuf library.
> >>>>>>>>
> >>>>>>>> /Bruce
> >>>>>>>>
> >>>>>>>>> Signed-off-by: Keith Wiles <keith.wiles@windriver.com>
> >>>>>>>>> ---
> >>>>>>>>> lib/librte_mbuf/rte_mbuf.h | 77
> >>>>>>>>> ++++++++++++++++++++++++++++++++++++++++++++++
> >>>>>>>>> 1 file changed, 77 insertions(+)
> >>>>>>>>>
> >>>>>>>>> diff --git a/lib/librte_mbuf/rte_mbuf.h b/lib/librte_mbuf/rte_mbuf.h
> >>>>>>>>> index 1c6e115..f298621 100644
> >>>>>>>>> --- a/lib/librte_mbuf/rte_mbuf.h
> >>>>>>>>> +++ b/lib/librte_mbuf/rte_mbuf.h
> >>>>>>>>> @@ -546,6 +546,41 @@ static inline void rte_pktmbuf_reset(struct rte_mbuf
> >>>>>>>>> *m)
> >>>>>>>>> }
> >>>>>>>>>
> >>>>>>>>> /**
> >>>>>>>>> + * @internal Allocate a list of mbufs from mempool *mp*.
> >>>>>>>>> + * The use of that function is reserved for RTE internal needs.
> >>>>>>>>> + * Please use rte_pktmbuf_alloc_bulk().
> >>>>>>>>> + *
> >>>>>>>>> + * @param mp
> >>>>>>>>> + *   The mempool from which mbuf is allocated.
> >>>>>>>>> + * @param m_list
> >>>>>>>>> + *   The array to place the allocated rte_mbufs pointers.
> >>>>>>>>> + * @param cnt
> >>>>>>>>> + *   The number of mbufs to allocate
> >>>>>>>>> + * @return
> >>>>>>>>> + *   - 0 if the number of mbufs allocated was ok
> >>>>>>>>> + *   - <0 is an ERROR.
> >>>>>>>>> + */
> >>>>>>>>> +static inline int __rte_mbuf_raw_alloc_bulk(struct rte_mempool *mp, struct
> >>>>>>>>> rte_mbuf *m_list[], int cnt)
> >>>>>>>>> +{
> >>>>>>>>> +     struct rte_mbuf *m;
> >>>>>>>>> +     int             ret;
> >>>>>>>>> +
> >>>>>>>>> +     ret = rte_mempool_get_bulk(mp, (void **)m_list, cnt);
> >>>>>>>>> +     if ( ret == 0 ) {
> >>>>>>>>> +             int             i;
> >>>>>>>>> +             for(i = 0; i < cnt; i++) {
> >>>>>>>>> +                     m = *m_list++;
> >>>>>>>>> +#ifdef RTE_MBUF_REFCNT
> >>>>>>>>> +                     rte_mbuf_refcnt_set(m, 1);
> >>>>>>>>> +#endif /* RTE_MBUF_REFCNT */
> >>>>>>>>> +                     rte_pktmbuf_reset(m);
> >>>>>>>>> +             }
> >>>>>>>>> +             ret = cnt;
> >>>>>>>>> +     }
> >>>>>>>>> +     return ret;
> >>>>>>>>> +}
> >>>>>>>>> +
> >>>>>>>>> +/**
> >>>>>>>>> * Allocate a new mbuf from a mempool.
> >>>>>>>>> *
> >>>>>>>>> * This new mbuf contains one segment, which has a length of 0. The pointer
> >>>>>>>>> @@ -671,6 +706,32 @@ __rte_pktmbuf_prefree_seg(struct rte_mbuf *m)
> >>>>>>>>> }
> >>>>>>>>>
> >>>>>>>>> /**
> >>>>>>>>> + * Allocate a list of mbufs from a mempool into a mbufs array.
> >>>>>>>>> + *
> >>>>>>>>> + * This mbuf list contains one segment per mbuf, which has a length of 0. The
> >>>>>>>>> pointer
> >>>>>>>>> + * to data is initialized to have some bytes of headroom in the buffer
> >>>>>>>>> + * (if buffer size allows).
> >>>>>>>>> + *
> >>>>>>>>> + * The routine is just a simple wrapper routine to reduce code in the application
> >>>>>>>>> and
> >>>>>>>>> + * provide a cleaner API for multiple mbuf requests.
> >>>>>>>>> + *
> >>>>>>>>> + * @param mp
> >>>>>>>>> + *   The mempool from which the mbuf is allocated.
> >>>>>>>>> + * @param m_list
> >>>>>>>>> + *   An array of mbuf pointers, cnt must be less then or equal to the size of the
> >>>>>>>>> list.
> >>>>>>>>> + * @param cnt
> >>>>>>>>> + *   Number of slots in the m_list array to fill.
> >>>>>>>>> + * @return
> >>>>>>>>> + *   - The number of valid mbufs pointers in the m_list array.
> >>>>>>>>> + *   - Zero if the request cnt could not be allocated.
> >>>>>>>>> + */
> >>>>>>>>> +static inline int __attribute__((always_inline))
> >>>>>>>>> +rte_pktmbuf_alloc_bulk(struct rte_mempool *mp, struct rte_mbuf *m_list[],
> >>>>>>>>> int16_t cnt)
> >>>>>>>>> +{
> >>>>>>>>> +     return __rte_mbuf_raw_alloc_bulk(mp, m_list, cnt);
> >>>>>>>>> +}
> >>>>>>>>> +
> >>>>>>>>> +/**
> >>>>>>>>> * Free a segment of a packet mbuf into its original mempool.
> >>>>>>>>> *
> >>>>>>>>> * Free an mbuf, without parsing other segments in case of chained
> >>>>>>>>> @@ -708,6 +769,22 @@ static inline void rte_pktmbuf_free(struct rte_mbuf
> >>>>>>>>> *m)
> >>>>>>>>>  }
> >>>>>>>>> }
> >>>>>>>>>
> >>>>>>>>> +/**
> >>>>>>>>> + * Free a list of packet mbufs back into its original mempool.
> >>>>>>>>> + *
> >>>>>>>>> + * Free a list of mbufs by calling rte_pktmbuf_free() in a loop as a wrapper
> >>>>>>>>> function.
> >>>>>>>>> + *
> >>>>>>>>> + * @param m_list
> >>>>>>>>> + *   An array of rte_mbuf pointers to be freed.
> >>>>>>>>> + * @param npkts
> >>>>>>>>> + *   Number of packets to free in list.
> >>>>>>>>> + */
> >>>>>>>>> +static inline void rte_pktmbuf_free_bulk(struct rte_mbuf *m_list[], int16_t
> >>>>>>>>> npkts)
> >>>>>>>>> +{
> >>>>>>>>> +     while(npkts--)
> >>>>>>>>> +             rte_pktmbuf_free(*m_list++);
> >>>>>>>>> +}
> >>>>>>>>> +
> >>>>>>>>> #ifdef RTE_MBUF_REFCNT
> >>>>>>>>>
> >>>>>>>>> /**
> >>>>>>>>> --
> >>>>>>>>> 2.1.0
> >>>>>>>>
> >>>>>>>
> >>>>>>> Keith Wiles, Principal Technologist with CTO office, Wind River mobile 972-213-5533
> >>>>
> >>>> Keith Wiles, Principal Technologist with CTO office, Wind River mobile 972-213-5533
> >>>
> >>> Keith Wiles, Principal Technologist with CTO office, Wind River mobile 972-213-5533
> >>
> >> Keith Wiles, Principal Technologist with CTO office, Wind River mobile 972-213-5533
> 
> Keith Wiles, Principal Technologist with CTO office, Wind River mobile 972-213-5533
> 

Re: [dpdk-dev] [PATCH 2/2] Adding the routines rte_pktmbuf_alloc_bulk() and rte_pktmbuf_free_bulk()

[Wiles, Roger Keith]

On Oct 7, 2014, at 10:42 AM, Ananyev, Konstantin <konstantin.ananyev@intel.com> wrote:

> Hi Keith,
> 
>> -----Original Message-----
>> From: Wiles, Roger Keith [mailto:keith.wiles@windriver.com]
>> Sent: Tuesday, October 07, 2014 3:22 PM
>> To: Ananyev, Konstantin
>> Cc: dev@dpdk.org
>> Subject: Re: [dpdk-dev] [PATCH 2/2] Adding the routines rte_pktmbuf_alloc_bulk() and rte_pktmbuf_free_bulk()
>> 
>> 
>> On Oct 7, 2014, at 4:09 AM, Ananyev, Konstantin <konstantin.ananyev@intel.com> wrote:
>> 
>>> 
>>> 
>>>> -----Original Message-----
>>>> From: Wiles, Roger Keith [mailto:keith.wiles@windriver.com]
>>>> Sent: Monday, October 06, 2014 9:08 PM
>>>> To: Ananyev, Konstantin
>>>> Cc: dev@dpdk.org
>>>> Subject: Re: [dpdk-dev] [PATCH 2/2] Adding the routines rte_pktmbuf_alloc_bulk() and rte_pktmbuf_free_bulk()
>>>> 
>>>> Attaching to the list does not work. If you want the code let me know it is only about 5K in size.
>>>> 
>>>> On Oct 6, 2014, at 2:45 PM, Wiles, Roger Keith <keith.wiles@windriver.com> wrote:
>>>> 
>>>>> 
>>>>> On Oct 6, 2014, at 11:13 AM, Wiles, Roger Keith <keith.wiles@windriver.com> wrote:
>>>>> 
>>>>>> 
>>>>>> On Oct 6, 2014, at 10:54 AM, Ananyev, Konstantin <konstantin.ananyev@intel.com> wrote:
>>>>>> 
>>>>>>>> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Bruce Richardson
>>>>>>>> Sent: Monday, October 06, 2014 3:54 PM
>>>>>>>> To: Wiles, Roger Keith (Wind River)
>>>>>>>> Cc: dev@dpdk.org
>>>>>>>> Subject: Re: [dpdk-dev] [PATCH 2/2] Adding the routines rte_pktmbuf_alloc_bulk() and rte_pktmbuf_free_bulk()
>>>>>>>> 
>>>>>>>> On Mon, Oct 06, 2014 at 03:50:38PM +0100, Wiles, Roger Keith wrote:
>>>>>>>>> Hi Bruce,
>>>>>>>>> 
>>>>>>>>> Do I need to reject the for the new routines or just make sure the vector driver does not get updated to use those routines?
>>>>>>>>> 
>>>>>>>> 
>>>>>>>> The new routines are probably useful in the general case. I see no issue
>>>>>>>> with having them in the code, so long as the vector driver is not modified
>>>>>>>> to use them.
>>>>>>> 
>>>>>>> I 'd say the same thing for non-vector RX/TX PMD code-paths too.
>>>>>>> 
>>>>>>> BTW, are the new functions comments valid?
>>>>>>> 
>>>>>>> + * @return
>>>>>>> + *   - 0 if the number of mbufs allocated was ok
>>>>>>> + *   - <0 is an ERROR.
>>>>>>> + */
>>>>>>> +static inline int __rte_mbuf_raw_alloc_bulk(
>>>>>>> 
>>>>>>> Though, as I can see __rte_mbuf_raw_alloc_bulk() returns either:
>>>>>>> - number of  allocated mbuf (cnt)
>>>>>>> - negative error code
>>>>>> 
>>>>>> Let me fix up the comments.
>>>>>>> 
>>>>>>> And:
>>>>>>> + * @return
>>>>>>> + *   - The number of valid mbufs pointers in the m_list array.
>>>>>>> + *   - Zero if the request cnt could not be allocated.
>>>>>>> + */
>>>>>>> +static inline int __attribute__((always_inline))
>>>>>>> +rte_pktmbuf_alloc_bulk(struct rte_mempool *mp, struct rte_mbuf *m_list[], int16_t cnt)
>>>>>>> +{
>>>>>>> +     return __rte_mbuf_raw_alloc_bulk(mp, m_list, cnt);
>>>>>>> +}
>>>>>>> 
>>>>>>> Shouldn't be "less than zero if the request cnt could not be allocated."?
>>>>>>> 
>>>>>>> BTW, is there any point to have __rte_mbuf_raw_alloc_bulk() at all?
>>>>>>> After all, as you are calling rte_pktmbuf_reset() inside it, it doesn't look __raw__ any more.
>>>>>>> Might be just put its content into rte_pktmbuf_alloc_bulk() and get rid of it.
>>>>>>> 
>>>>>> I was just following the non-bulk routine style __rte_mbuf_raw_alloc(), but I can pull that into a single routine.
>>>>>> 
>>>>>>> Also wonder, what is the advantage of having multiple counters inside the same loop?
>>>>>>> i.e:
>>>>>>> +             for(i = 0; i < cnt; i++) {
>>>>>>> +                     m = *m_list++;
>>>>>>> 
>>>>>>> Why not just:
>>>>>>> 
>>>>>>> for(i = 0; i < cnt; i++) {
>>>>>>> m = &m_list[i];
>>>>>>> 
>>>>>>> Same for free:
>>>>>>> +     while(npkts--)
>>>>>>> +             rte_pktmbuf_free(*m_list++);
>>>>>>> 
>>>>>>> While not just:
>>>>>>> for (i = 0; i < npkts; i++)
>>>>>>>  rte_pktmbuf_free(&m_list[i]);
>>>>>> 
>>>>>> Maybe I have it wrong or the compilers are doing the right thing now, but at one point the &m_list[i] would cause the compiler
>> to
>>>> generate a shift or multiple of 'i' and then add it to the base of m_list. If that is not the case anymore then I can update the code as
>>>> you suggested. Using the *m_list++ just adds the size of a pointer to a register and continues.
>>>>> 
>>>>> I compared the clang assembler (.s file) output from an example test code I wrote to see if we have any differences in the code
>>>> using the two styles and I found no difference and the code looked the same. I am not a Intel assembler expert and I would
>> suggest
>>>> someone else determine if it generates different code. I tried to compare the GCC outputs and it did look the same to me.
>>> 
>>> That's was my question:
>>> Modern compilers are able to generate a good code for a simple loop as above.
>>> So what's the point to use 2 iterators inside the loop, when just one is enough?
>>> Nothing wrong technically, but makes code a bit harder to follow.
>>> Plus, in general, it is a good practise to minimise number of iterators inside the loop, when possible.
>>> 
>>> Konstantin
>> 
>> Hi Konstantin,
>> 
>> I really do not understand the concern if the code is the same, as it appears to me the current patch is very clean and simple. Maybe
>> you have not seen the v2 patch and now v3 patch I sent this morning to fix Bruce's comment suggestion.
>> 
>> For the case of the free routine your suggestion would require an extra counter/variable a bit more code a 'for' loop instead of a
>> 'while' loop.
> 
> My point was that just one iterator for both loops is enough.
> In general, it is a good practise to minimise number of iterators per loop if possible:
> in some cases  compiler might get confused and wouldn't be able to eliminate redundant  iterators itself.

I learned a while back to not to be a compiler, but a programmer :-) Now a days the compilers handle the basic cases we have here and for the special cases we need to be aware of how the compiler generates code. I agree having less iterators per loop is cleaner, but in this case I do not think it matters.
> Though yes - technically there is nothing wrong with your approach.
> So if you prefer to keep it as it is - I wouldn't insist.
> 
> Konstantin
> 
>> +static inline void __attribute__((always_inline))
>> +rte_pktmbuf_free_bulk(struct rte_mbuf *m_list[], int16_t npkts)
>> +{
>> +     while(npkts--)
>> +             rte_pktmbuf_free(*m_list++);
>> +}
>> 
>> For the case of the alloc routine I did remove the rte_mbuf * m variable and now I believe it is very clean and changing it to use index
>> variables is just a personal preference. I personal preference of this type is not useful IMO and does not cause any harm. Unless you
>> can suggest a good technical reason to change I am going to leave the patch as is.
>> 
>> +static inline int __attribute__((always_inline))
>> +rte_pktmbuf_alloc_bulk(struct rte_mempool *mp, struct rte_mbuf *m_list[], int16_t cnt)
>> +{
>> +   int     ret;
>> +
>> +   ret = rte_mempool_get_bulk(mp, (void **)m_list, cnt);
>> +   if ( ret == 0 ) {
>> +       ret = cnt;
>> +       while(cnt--) {
>> +#ifdef RTE_MBUF_REFCNT
>> +           rte_mbuf_refcnt_set(*m_list, 1);
>> +#endif /* RTE_MBUF_REFCNT */
>> +           rte_pktmbuf_reset(*m_list++);
>> +       }
>> +   }
>> +   return ret;
>> +}
>> 
>>>>> 
>>>>> I have attached the code and output, please let me know if I did something wrong, but as it stands using the original style is what I
>>>> want to go with.
>>>>> 
>>>>>>> 
>>>>>>> Konstantin
>>>>>>> 
>>>>>>>> 
>>>>>>>> /Bruce
>>>>>>>> 
>>>>>>>>> Thanks
>>>>>>>>> ++Keith
>>>>>>>>> 
>>>>>>>>> On Oct 6, 2014, at 3:56 AM, Richardson, Bruce <bruce.richardson@intel.com> wrote:
>>>>>>>>> 
>>>>>>>>>> 
>>>>>>>>>> 
>>>>>>>>>>> -----Original Message-----
>>>>>>>>>>> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Keith Wiles
>>>>>>>>>>> Sent: Sunday, October 05, 2014 12:10 AM
>>>>>>>>>>> To: dev@dpdk.org
>>>>>>>>>>> Subject: [dpdk-dev] [PATCH 2/2] Adding the routines rte_pktmbuf_alloc_bulk()
>>>>>>>>>>> and rte_pktmbuf_free_bulk()
>>>>>>>>>>> 
>>>>>>>>>>> Minor helper routines to mirror the mempool routines and remove the code
>>>>>>>>>>> from applications. The ixgbe_rxtx_vec.c routine could be changed to use
>>>>>>>>>>> the ret_pktmbuf_alloc_bulk() routine inplace of rte_mempool_get_bulk().
>>>>>>>>>>> 
>>>>>>>>>> 
>>>>>>>>>> I believe such a change would cause a performance regression, as the extra init code in the alloc_bulk() function would
>> take
>>>>>>>> additional cycles and is not needed. The vector routines use the mempool function directly, so that there is no overhead of
>>>> mbuf
>>>>>>>> initialization, as the vector routines use their additional "knowledge" of what the mbufs will be used for to init them in a faster
>>>> manner
>>>>>>>> than can be done inside the mbuf library.
>>>>>>>>>> 
>>>>>>>>>> /Bruce
>>>>>>>>>> 
>>>>>>>>>>> Signed-off-by: Keith Wiles <keith.wiles@windriver.com>
>>>>>>>>>>> ---
>>>>>>>>>>> lib/librte_mbuf/rte_mbuf.h | 77
>>>>>>>>>>> ++++++++++++++++++++++++++++++++++++++++++++++
>>>>>>>>>>> 1 file changed, 77 insertions(+)
>>>>>>>>>>> 
>>>>>>>>>>> diff --git a/lib/librte_mbuf/rte_mbuf.h b/lib/librte_mbuf/rte_mbuf.h
>>>>>>>>>>> index 1c6e115..f298621 100644
>>>>>>>>>>> --- a/lib/librte_mbuf/rte_mbuf.h
>>>>>>>>>>> +++ b/lib/librte_mbuf/rte_mbuf.h
>>>>>>>>>>> @@ -546,6 +546,41 @@ static inline void rte_pktmbuf_reset(struct rte_mbuf
>>>>>>>>>>> *m)
>>>>>>>>>>> }
>>>>>>>>>>> 
>>>>>>>>>>> /**
>>>>>>>>>>> + * @internal Allocate a list of mbufs from mempool *mp*.
>>>>>>>>>>> + * The use of that function is reserved for RTE internal needs.
>>>>>>>>>>> + * Please use rte_pktmbuf_alloc_bulk().
>>>>>>>>>>> + *
>>>>>>>>>>> + * @param mp
>>>>>>>>>>> + *   The mempool from which mbuf is allocated.
>>>>>>>>>>> + * @param m_list
>>>>>>>>>>> + *   The array to place the allocated rte_mbufs pointers.
>>>>>>>>>>> + * @param cnt
>>>>>>>>>>> + *   The number of mbufs to allocate
>>>>>>>>>>> + * @return
>>>>>>>>>>> + *   - 0 if the number of mbufs allocated was ok
>>>>>>>>>>> + *   - <0 is an ERROR.
>>>>>>>>>>> + */
>>>>>>>>>>> +static inline int __rte_mbuf_raw_alloc_bulk(struct rte_mempool *mp, struct
>>>>>>>>>>> rte_mbuf *m_list[], int cnt)
>>>>>>>>>>> +{
>>>>>>>>>>> +     struct rte_mbuf *m;
>>>>>>>>>>> +     int             ret;
>>>>>>>>>>> +
>>>>>>>>>>> +     ret = rte_mempool_get_bulk(mp, (void **)m_list, cnt);
>>>>>>>>>>> +     if ( ret == 0 ) {
>>>>>>>>>>> +             int             i;
>>>>>>>>>>> +             for(i = 0; i < cnt; i++) {
>>>>>>>>>>> +                     m = *m_list++;
>>>>>>>>>>> +#ifdef RTE_MBUF_REFCNT
>>>>>>>>>>> +                     rte_mbuf_refcnt_set(m, 1);
>>>>>>>>>>> +#endif /* RTE_MBUF_REFCNT */
>>>>>>>>>>> +                     rte_pktmbuf_reset(m);
>>>>>>>>>>> +             }
>>>>>>>>>>> +             ret = cnt;
>>>>>>>>>>> +     }
>>>>>>>>>>> +     return ret;
>>>>>>>>>>> +}
>>>>>>>>>>> +
>>>>>>>>>>> +/**
>>>>>>>>>>> * Allocate a new mbuf from a mempool.
>>>>>>>>>>> *
>>>>>>>>>>> * This new mbuf contains one segment, which has a length of 0. The pointer
>>>>>>>>>>> @@ -671,6 +706,32 @@ __rte_pktmbuf_prefree_seg(struct rte_mbuf *m)
>>>>>>>>>>> }
>>>>>>>>>>> 
>>>>>>>>>>> /**
>>>>>>>>>>> + * Allocate a list of mbufs from a mempool into a mbufs array.
>>>>>>>>>>> + *
>>>>>>>>>>> + * This mbuf list contains one segment per mbuf, which has a length of 0. The
>>>>>>>>>>> pointer
>>>>>>>>>>> + * to data is initialized to have some bytes of headroom in the buffer
>>>>>>>>>>> + * (if buffer size allows).
>>>>>>>>>>> + *
>>>>>>>>>>> + * The routine is just a simple wrapper routine to reduce code in the application
>>>>>>>>>>> and
>>>>>>>>>>> + * provide a cleaner API for multiple mbuf requests.
>>>>>>>>>>> + *
>>>>>>>>>>> + * @param mp
>>>>>>>>>>> + *   The mempool from which the mbuf is allocated.
>>>>>>>>>>> + * @param m_list
>>>>>>>>>>> + *   An array of mbuf pointers, cnt must be less then or equal to the size of the
>>>>>>>>>>> list.
>>>>>>>>>>> + * @param cnt
>>>>>>>>>>> + *   Number of slots in the m_list array to fill.
>>>>>>>>>>> + * @return
>>>>>>>>>>> + *   - The number of valid mbufs pointers in the m_list array.
>>>>>>>>>>> + *   - Zero if the request cnt could not be allocated.
>>>>>>>>>>> + */
>>>>>>>>>>> +static inline int __attribute__((always_inline))
>>>>>>>>>>> +rte_pktmbuf_alloc_bulk(struct rte_mempool *mp, struct rte_mbuf *m_list[],
>>>>>>>>>>> int16_t cnt)
>>>>>>>>>>> +{
>>>>>>>>>>> +     return __rte_mbuf_raw_alloc_bulk(mp, m_list, cnt);
>>>>>>>>>>> +}
>>>>>>>>>>> +
>>>>>>>>>>> +/**
>>>>>>>>>>> * Free a segment of a packet mbuf into its original mempool.
>>>>>>>>>>> *
>>>>>>>>>>> * Free an mbuf, without parsing other segments in case of chained
>>>>>>>>>>> @@ -708,6 +769,22 @@ static inline void rte_pktmbuf_free(struct rte_mbuf
>>>>>>>>>>> *m)
>>>>>>>>>>> }
>>>>>>>>>>> }
>>>>>>>>>>> 
>>>>>>>>>>> +/**
>>>>>>>>>>> + * Free a list of packet mbufs back into its original mempool.
>>>>>>>>>>> + *
>>>>>>>>>>> + * Free a list of mbufs by calling rte_pktmbuf_free() in a loop as a wrapper
>>>>>>>>>>> function.
>>>>>>>>>>> + *
>>>>>>>>>>> + * @param m_list
>>>>>>>>>>> + *   An array of rte_mbuf pointers to be freed.
>>>>>>>>>>> + * @param npkts
>>>>>>>>>>> + *   Number of packets to free in list.
>>>>>>>>>>> + */
>>>>>>>>>>> +static inline void rte_pktmbuf_free_bulk(struct rte_mbuf *m_list[], int16_t
>>>>>>>>>>> npkts)
>>>>>>>>>>> +{
>>>>>>>>>>> +     while(npkts--)
>>>>>>>>>>> +             rte_pktmbuf_free(*m_list++);
>>>>>>>>>>> +}
>>>>>>>>>>> +
>>>>>>>>>>> #ifdef RTE_MBUF_REFCNT
>>>>>>>>>>> 
>>>>>>>>>>> /**
>>>>>>>>>>> --
>>>>>>>>>>> 2.1.0
>>>>>>>>>> 
>>>>>>>>> 
>>>>>>>>> Keith Wiles, Principal Technologist with CTO office, Wind River mobile 972-213-5533
>>>>>> 
>>>>>> Keith Wiles, Principal Technologist with CTO office, Wind River mobile 972-213-5533
>>>>> 
>>>>> Keith Wiles, Principal Technologist with CTO office, Wind River mobile 972-213-5533
>>>> 
>>>> Keith Wiles, Principal Technologist with CTO office, Wind River mobile 972-213-5533
>> 
>> Keith Wiles, Principal Technologist with CTO office, Wind River mobile 972-213-5533

Keith Wiles, Principal Technologist with CTO office, Wind River mobile 972-213-5533

Re: [dpdk-dev] [PATCH 2/2] Adding the routines rte_pktmbuf_alloc_bulk() and rte_pktmbuf_free_bulk()

[Ananyev, Konstantin]

> -----Original Message-----
> From: Wiles, Roger Keith [mailto:keith.wiles@windriver.com]
> Sent: Tuesday, October 07, 2014 4:56 PM
> To: Ananyev, Konstantin
> Cc: dev@dpdk.org
> Subject: Re: [dpdk-dev] [PATCH 2/2] Adding the routines rte_pktmbuf_alloc_bulk() and rte_pktmbuf_free_bulk()
> 
> 
> On Oct 7, 2014, at 10:42 AM, Ananyev, Konstantin <konstantin.ananyev@intel.com> wrote:
> 
> > Hi Keith,
> >
> >> -----Original Message-----
> >> From: Wiles, Roger Keith [mailto:keith.wiles@windriver.com]
> >> Sent: Tuesday, October 07, 2014 3:22 PM
> >> To: Ananyev, Konstantin
> >> Cc: dev@dpdk.org
> >> Subject: Re: [dpdk-dev] [PATCH 2/2] Adding the routines rte_pktmbuf_alloc_bulk() and rte_pktmbuf_free_bulk()
> >>
> >>
> >> On Oct 7, 2014, at 4:09 AM, Ananyev, Konstantin <konstantin.ananyev@intel.com> wrote:
> >>
> >>>
> >>>
> >>>> -----Original Message-----
> >>>> From: Wiles, Roger Keith [mailto:keith.wiles@windriver.com]
> >>>> Sent: Monday, October 06, 2014 9:08 PM
> >>>> To: Ananyev, Konstantin
> >>>> Cc: dev@dpdk.org
> >>>> Subject: Re: [dpdk-dev] [PATCH 2/2] Adding the routines rte_pktmbuf_alloc_bulk() and rte_pktmbuf_free_bulk()
> >>>>
> >>>> Attaching to the list does not work. If you want the code let me know it is only about 5K in size.
> >>>>
> >>>> On Oct 6, 2014, at 2:45 PM, Wiles, Roger Keith <keith.wiles@windriver.com> wrote:
> >>>>
> >>>>>
> >>>>> On Oct 6, 2014, at 11:13 AM, Wiles, Roger Keith <keith.wiles@windriver.com> wrote:
> >>>>>
> >>>>>>
> >>>>>> On Oct 6, 2014, at 10:54 AM, Ananyev, Konstantin <konstantin.ananyev@intel.com> wrote:
> >>>>>>
> >>>>>>>> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Bruce Richardson
> >>>>>>>> Sent: Monday, October 06, 2014 3:54 PM
> >>>>>>>> To: Wiles, Roger Keith (Wind River)
> >>>>>>>> Cc: dev@dpdk.org
> >>>>>>>> Subject: Re: [dpdk-dev] [PATCH 2/2] Adding the routines rte_pktmbuf_alloc_bulk() and rte_pktmbuf_free_bulk()
> >>>>>>>>
> >>>>>>>> On Mon, Oct 06, 2014 at 03:50:38PM +0100, Wiles, Roger Keith wrote:
> >>>>>>>>> Hi Bruce,
> >>>>>>>>>
> >>>>>>>>> Do I need to reject the for the new routines or just make sure the vector driver does not get updated to use those
> routines?
> >>>>>>>>>
> >>>>>>>>
> >>>>>>>> The new routines are probably useful in the general case. I see no issue
> >>>>>>>> with having them in the code, so long as the vector driver is not modified
> >>>>>>>> to use them.
> >>>>>>>
> >>>>>>> I 'd say the same thing for non-vector RX/TX PMD code-paths too.
> >>>>>>>
> >>>>>>> BTW, are the new functions comments valid?
> >>>>>>>
> >>>>>>> + * @return
> >>>>>>> + *   - 0 if the number of mbufs allocated was ok
> >>>>>>> + *   - <0 is an ERROR.
> >>>>>>> + */
> >>>>>>> +static inline int __rte_mbuf_raw_alloc_bulk(
> >>>>>>>
> >>>>>>> Though, as I can see __rte_mbuf_raw_alloc_bulk() returns either:
> >>>>>>> - number of  allocated mbuf (cnt)
> >>>>>>> - negative error code
> >>>>>>
> >>>>>> Let me fix up the comments.
> >>>>>>>
> >>>>>>> And:
> >>>>>>> + * @return
> >>>>>>> + *   - The number of valid mbufs pointers in the m_list array.
> >>>>>>> + *   - Zero if the request cnt could not be allocated.
> >>>>>>> + */
> >>>>>>> +static inline int __attribute__((always_inline))
> >>>>>>> +rte_pktmbuf_alloc_bulk(struct rte_mempool *mp, struct rte_mbuf *m_list[], int16_t cnt)
> >>>>>>> +{
> >>>>>>> +     return __rte_mbuf_raw_alloc_bulk(mp, m_list, cnt);
> >>>>>>> +}
> >>>>>>>
> >>>>>>> Shouldn't be "less than zero if the request cnt could not be allocated."?
> >>>>>>>
> >>>>>>> BTW, is there any point to have __rte_mbuf_raw_alloc_bulk() at all?
> >>>>>>> After all, as you are calling rte_pktmbuf_reset() inside it, it doesn't look __raw__ any more.
> >>>>>>> Might be just put its content into rte_pktmbuf_alloc_bulk() and get rid of it.
> >>>>>>>
> >>>>>> I was just following the non-bulk routine style __rte_mbuf_raw_alloc(), but I can pull that into a single routine.
> >>>>>>
> >>>>>>> Also wonder, what is the advantage of having multiple counters inside the same loop?
> >>>>>>> i.e:
> >>>>>>> +             for(i = 0; i < cnt; i++) {
> >>>>>>> +                     m = *m_list++;
> >>>>>>>
> >>>>>>> Why not just:
> >>>>>>>
> >>>>>>> for(i = 0; i < cnt; i++) {
> >>>>>>> m = &m_list[i];
> >>>>>>>
> >>>>>>> Same for free:
> >>>>>>> +     while(npkts--)
> >>>>>>> +             rte_pktmbuf_free(*m_list++);
> >>>>>>>
> >>>>>>> While not just:
> >>>>>>> for (i = 0; i < npkts; i++)
> >>>>>>>  rte_pktmbuf_free(&m_list[i]);
> >>>>>>
> >>>>>> Maybe I have it wrong or the compilers are doing the right thing now, but at one point the &m_list[i] would cause the
> compiler
> >> to
> >>>> generate a shift or multiple of 'i' and then add it to the base of m_list. If that is not the case anymore then I can update the code
> as
> >>>> you suggested. Using the *m_list++ just adds the size of a pointer to a register and continues.
> >>>>>
> >>>>> I compared the clang assembler (.s file) output from an example test code I wrote to see if we have any differences in the code
> >>>> using the two styles and I found no difference and the code looked the same. I am not a Intel assembler expert and I would
> >> suggest
> >>>> someone else determine if it generates different code. I tried to compare the GCC outputs and it did look the same to me.
> >>>
> >>> That's was my question:
> >>> Modern compilers are able to generate a good code for a simple loop as above.
> >>> So what's the point to use 2 iterators inside the loop, when just one is enough?
> >>> Nothing wrong technically, but makes code a bit harder to follow.
> >>> Plus, in general, it is a good practise to minimise number of iterators inside the loop, when possible.
> >>>
> >>> Konstantin
> >>
> >> Hi Konstantin,
> >>
> >> I really do not understand the concern if the code is the same, as it appears to me the current patch is very clean and simple.
> Maybe
> >> you have not seen the v2 patch and now v3 patch I sent this morning to fix Bruce's comment suggestion.
> >>
> >> For the case of the free routine your suggestion would require an extra counter/variable a bit more code a 'for' loop instead of a
> >> 'while' loop.
> >
> > My point was that just one iterator for both loops is enough.
> > In general, it is a good practise to minimise number of iterators per loop if possible:
> > in some cases  compiler might get confused and wouldn't be able to eliminate redundant  iterators itself.
> 
> I learned a while back to not to be a compiler, but a programmer :-) Now a days the compilers handle the basic cases we have here

Yes, in most cases they would.
That's why I don't insist.
Konstantin

> and for the special cases we need to be aware of how the compiler generates code. I agree having less iterators per loop is cleaner,
> but in this case I do not think it matters.
> > Though yes - technically there is nothing wrong with your approach.
> > So if you prefer to keep it as it is - I wouldn't insist.
> >
> > Konstantin
> >
> >> +static inline void __attribute__((always_inline))
> >> +rte_pktmbuf_free_bulk(struct rte_mbuf *m_list[], int16_t npkts)
> >> +{
> >> +     while(npkts--)
> >> +             rte_pktmbuf_free(*m_list++);
> >> +}
> >>
> >> For the case of the alloc routine I did remove the rte_mbuf * m variable and now I believe it is very clean and changing it to use
> index
> >> variables is just a personal preference. I personal preference of this type is not useful IMO and does not cause any harm. Unless
> you
> >> can suggest a good technical reason to change I am going to leave the patch as is.
> >>
> >> +static inline int __attribute__((always_inline))
> >> +rte_pktmbuf_alloc_bulk(struct rte_mempool *mp, struct rte_mbuf *m_list[], int16_t cnt)
> >> +{
> >> +   int     ret;
> >> +
> >> +   ret = rte_mempool_get_bulk(mp, (void **)m_list, cnt);
> >> +   if ( ret == 0 ) {
> >> +       ret = cnt;
> >> +       while(cnt--) {
> >> +#ifdef RTE_MBUF_REFCNT
> >> +           rte_mbuf_refcnt_set(*m_list, 1);
> >> +#endif /* RTE_MBUF_REFCNT */
> >> +           rte_pktmbuf_reset(*m_list++);
> >> +       }
> >> +   }
> >> +   return ret;
> >> +}
> >>
> >>>>>
> >>>>> I have attached the code and output, please let me know if I did something wrong, but as it stands using the original style is
> what I
> >>>> want to go with.
> >>>>>
> >>>>>>>
> >>>>>>> Konstantin
> >>>>>>>
> >>>>>>>>
> >>>>>>>> /Bruce
> >>>>>>>>
> >>>>>>>>> Thanks
> >>>>>>>>> ++Keith
> >>>>>>>>>
> >>>>>>>>> On Oct 6, 2014, at 3:56 AM, Richardson, Bruce <bruce.richardson@intel.com> wrote:
> >>>>>>>>>
> >>>>>>>>>>
> >>>>>>>>>>
> >>>>>>>>>>> -----Original Message-----
> >>>>>>>>>>> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Keith Wiles
> >>>>>>>>>>> Sent: Sunday, October 05, 2014 12:10 AM
> >>>>>>>>>>> To: dev@dpdk.org
> >>>>>>>>>>> Subject: [dpdk-dev] [PATCH 2/2] Adding the routines rte_pktmbuf_alloc_bulk()
> >>>>>>>>>>> and rte_pktmbuf_free_bulk()
> >>>>>>>>>>>
> >>>>>>>>>>> Minor helper routines to mirror the mempool routines and remove the code
> >>>>>>>>>>> from applications. The ixgbe_rxtx_vec.c routine could be changed to use
> >>>>>>>>>>> the ret_pktmbuf_alloc_bulk() routine inplace of rte_mempool_get_bulk().
> >>>>>>>>>>>
> >>>>>>>>>>
> >>>>>>>>>> I believe such a change would cause a performance regression, as the extra init code in the alloc_bulk() function would
> >> take
> >>>>>>>> additional cycles and is not needed. The vector routines use the mempool function directly, so that there is no overhead of
> >>>> mbuf
> >>>>>>>> initialization, as the vector routines use their additional "knowledge" of what the mbufs will be used for to init them in a
> faster
> >>>> manner
> >>>>>>>> than can be done inside the mbuf library.
> >>>>>>>>>>
> >>>>>>>>>> /Bruce
> >>>>>>>>>>
> >>>>>>>>>>> Signed-off-by: Keith Wiles <keith.wiles@windriver.com>
> >>>>>>>>>>> ---
> >>>>>>>>>>> lib/librte_mbuf/rte_mbuf.h | 77
> >>>>>>>>>>> ++++++++++++++++++++++++++++++++++++++++++++++
> >>>>>>>>>>> 1 file changed, 77 insertions(+)
> >>>>>>>>>>>
> >>>>>>>>>>> diff --git a/lib/librte_mbuf/rte_mbuf.h b/lib/librte_mbuf/rte_mbuf.h
> >>>>>>>>>>> index 1c6e115..f298621 100644
> >>>>>>>>>>> --- a/lib/librte_mbuf/rte_mbuf.h
> >>>>>>>>>>> +++ b/lib/librte_mbuf/rte_mbuf.h
> >>>>>>>>>>> @@ -546,6 +546,41 @@ static inline void rte_pktmbuf_reset(struct rte_mbuf
> >>>>>>>>>>> *m)
> >>>>>>>>>>> }
> >>>>>>>>>>>
> >>>>>>>>>>> /**
> >>>>>>>>>>> + * @internal Allocate a list of mbufs from mempool *mp*.
> >>>>>>>>>>> + * The use of that function is reserved for RTE internal needs.
> >>>>>>>>>>> + * Please use rte_pktmbuf_alloc_bulk().
> >>>>>>>>>>> + *
> >>>>>>>>>>> + * @param mp
> >>>>>>>>>>> + *   The mempool from which mbuf is allocated.
> >>>>>>>>>>> + * @param m_list
> >>>>>>>>>>> + *   The array to place the allocated rte_mbufs pointers.
> >>>>>>>>>>> + * @param cnt
> >>>>>>>>>>> + *   The number of mbufs to allocate
> >>>>>>>>>>> + * @return
> >>>>>>>>>>> + *   - 0 if the number of mbufs allocated was ok
> >>>>>>>>>>> + *   - <0 is an ERROR.
> >>>>>>>>>>> + */
> >>>>>>>>>>> +static inline int __rte_mbuf_raw_alloc_bulk(struct rte_mempool *mp, struct
> >>>>>>>>>>> rte_mbuf *m_list[], int cnt)
> >>>>>>>>>>> +{
> >>>>>>>>>>> +     struct rte_mbuf *m;
> >>>>>>>>>>> +     int             ret;
> >>>>>>>>>>> +
> >>>>>>>>>>> +     ret = rte_mempool_get_bulk(mp, (void **)m_list, cnt);
> >>>>>>>>>>> +     if ( ret == 0 ) {
> >>>>>>>>>>> +             int             i;
> >>>>>>>>>>> +             for(i = 0; i < cnt; i++) {
> >>>>>>>>>>> +                     m = *m_list++;
> >>>>>>>>>>> +#ifdef RTE_MBUF_REFCNT
> >>>>>>>>>>> +                     rte_mbuf_refcnt_set(m, 1);
> >>>>>>>>>>> +#endif /* RTE_MBUF_REFCNT */
> >>>>>>>>>>> +                     rte_pktmbuf_reset(m);
> >>>>>>>>>>> +             }
> >>>>>>>>>>> +             ret = cnt;
> >>>>>>>>>>> +     }
> >>>>>>>>>>> +     return ret;
> >>>>>>>>>>> +}
> >>>>>>>>>>> +
> >>>>>>>>>>> +/**
> >>>>>>>>>>> * Allocate a new mbuf from a mempool.
> >>>>>>>>>>> *
> >>>>>>>>>>> * This new mbuf contains one segment, which has a length of 0. The pointer
> >>>>>>>>>>> @@ -671,6 +706,32 @@ __rte_pktmbuf_prefree_seg(struct rte_mbuf *m)
> >>>>>>>>>>> }
> >>>>>>>>>>>
> >>>>>>>>>>> /**
> >>>>>>>>>>> + * Allocate a list of mbufs from a mempool into a mbufs array.
> >>>>>>>>>>> + *
> >>>>>>>>>>> + * This mbuf list contains one segment per mbuf, which has a length of 0. The
> >>>>>>>>>>> pointer
> >>>>>>>>>>> + * to data is initialized to have some bytes of headroom in the buffer
> >>>>>>>>>>> + * (if buffer size allows).
> >>>>>>>>>>> + *
> >>>>>>>>>>> + * The routine is just a simple wrapper routine to reduce code in the application
> >>>>>>>>>>> and
> >>>>>>>>>>> + * provide a cleaner API for multiple mbuf requests.
> >>>>>>>>>>> + *
> >>>>>>>>>>> + * @param mp
> >>>>>>>>>>> + *   The mempool from which the mbuf is allocated.
> >>>>>>>>>>> + * @param m_list
> >>>>>>>>>>> + *   An array of mbuf pointers, cnt must be less then or equal to the size of the
> >>>>>>>>>>> list.
> >>>>>>>>>>> + * @param cnt
> >>>>>>>>>>> + *   Number of slots in the m_list array to fill.
> >>>>>>>>>>> + * @return
> >>>>>>>>>>> + *   - The number of valid mbufs pointers in the m_list array.
> >>>>>>>>>>> + *   - Zero if the request cnt could not be allocated.
> >>>>>>>>>>> + */
> >>>>>>>>>>> +static inline int __attribute__((always_inline))
> >>>>>>>>>>> +rte_pktmbuf_alloc_bulk(struct rte_mempool *mp, struct rte_mbuf *m_list[],
> >>>>>>>>>>> int16_t cnt)
> >>>>>>>>>>> +{
> >>>>>>>>>>> +     return __rte_mbuf_raw_alloc_bulk(mp, m_list, cnt);
> >>>>>>>>>>> +}
> >>>>>>>>>>> +
> >>>>>>>>>>> +/**
> >>>>>>>>>>> * Free a segment of a packet mbuf into its original mempool.
> >>>>>>>>>>> *
> >>>>>>>>>>> * Free an mbuf, without parsing other segments in case of chained
> >>>>>>>>>>> @@ -708,6 +769,22 @@ static inline void rte_pktmbuf_free(struct rte_mbuf
> >>>>>>>>>>> *m)
> >>>>>>>>>>> }
> >>>>>>>>>>> }
> >>>>>>>>>>>
> >>>>>>>>>>> +/**
> >>>>>>>>>>> + * Free a list of packet mbufs back into its original mempool.
> >>>>>>>>>>> + *
> >>>>>>>>>>> + * Free a list of mbufs by calling rte_pktmbuf_free() in a loop as a wrapper
> >>>>>>>>>>> function.
> >>>>>>>>>>> + *
> >>>>>>>>>>> + * @param m_list
> >>>>>>>>>>> + *   An array of rte_mbuf pointers to be freed.
> >>>>>>>>>>> + * @param npkts
> >>>>>>>>>>> + *   Number of packets to free in list.
> >>>>>>>>>>> + */
> >>>>>>>>>>> +static inline void rte_pktmbuf_free_bulk(struct rte_mbuf *m_list[], int16_t
> >>>>>>>>>>> npkts)
> >>>>>>>>>>> +{
> >>>>>>>>>>> +     while(npkts--)
> >>>>>>>>>>> +             rte_pktmbuf_free(*m_list++);
> >>>>>>>>>>> +}
> >>>>>>>>>>> +
> >>>>>>>>>>> #ifdef RTE_MBUF_REFCNT
> >>>>>>>>>>>
> >>>>>>>>>>> /**
> >>>>>>>>>>> --
> >>>>>>>>>>> 2.1.0
> >>>>>>>>>>
> >>>>>>>>>
> >>>>>>>>> Keith Wiles, Principal Technologist with CTO office, Wind River mobile 972-213-5533
> >>>>>>
> >>>>>> Keith Wiles, Principal Technologist with CTO office, Wind River mobile 972-213-5533
> >>>>>
> >>>>> Keith Wiles, Principal Technologist with CTO office, Wind River mobile 972-213-5533
> >>>>
> >>>> Keith Wiles, Principal Technologist with CTO office, Wind River mobile 972-213-5533
> >>
> >> Keith Wiles, Principal Technologist with CTO office, Wind River mobile 972-213-5533
> 
> Keith Wiles, Principal Technologist with CTO office, Wind River mobile 972-213-5533
> 

Re: [dpdk-dev] [PATCH 1/2] Move the error check inside __mempool_check_cookies()

[Wiles, Roger Keith]

Self Nack this one and will resend the patch as a single patch. :-(

On Oct 4, 2014, at 6:10 PM, Keith Wiles <keith.wiles@windriver.com> wrote:

> Three places check for the return value from __mempool_get_bulk to be zero
> and then call the debug routine __mempool_check_cookies(). The test is
> not required if moved into the debug routine. Minor cleanup and mostly
> does not effect performance, unless the is not removed by the compiler
> in the case where teh debug routine is not defined.
> 
> Signed-off-by: Keith Wiles <keith.wiles@windriver.com>
> ---
> lib/librte_mempool/rte_mempool.h | 12 ++++++------
> 1 file changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/lib/librte_mempool/rte_mempool.h b/lib/librte_mempool/rte_mempool.h
> index 597cf4f..154fdd4 100644
> --- a/lib/librte_mempool/rte_mempool.h
> +++ b/lib/librte_mempool/rte_mempool.h
> @@ -325,6 +325,9 @@ static inline void __mempool_check_cookies(const struct rte_mempool *mp,
> 	void *obj;
> 	void **obj_table;
> 
> +	if ( n < 0 )
> +		return;
> +
> 	/* Force to drop the "const" attribute. This is done only when
> 	 * DEBUG is enabled */
> 	tmp = (void *) obj_table_const;
> @@ -1029,8 +1032,7 @@ rte_mempool_mc_get_bulk(struct rte_mempool *mp, void **obj_table, unsigned n)
> {
> 	int ret;
> 	ret = __mempool_get_bulk(mp, obj_table, n, 1);
> -	if (ret == 0)
> -		__mempool_check_cookies(mp, obj_table, n, 1);
> +	__mempool_check_cookies(mp, obj_table, n, 1);
> 	return ret;
> }
> 
> @@ -1058,8 +1060,7 @@ rte_mempool_sc_get_bulk(struct rte_mempool *mp, void **obj_table, unsigned n)
> {
> 	int ret;
> 	ret = __mempool_get_bulk(mp, obj_table, n, 0);
> -	if (ret == 0)
> -		__mempool_check_cookies(mp, obj_table, n, 1);
> +	__mempool_check_cookies(mp, obj_table, n, 1);
> 	return ret;
> }
> 
> @@ -1091,8 +1092,7 @@ rte_mempool_get_bulk(struct rte_mempool *mp, void **obj_table, unsigned n)
> 	int ret;
> 	ret = __mempool_get_bulk(mp, obj_table, n,
> 				 !(mp->flags & MEMPOOL_F_SC_GET));
> -	if (ret == 0)
> -		__mempool_check_cookies(mp, obj_table, n, 1);
> +	__mempool_check_cookies(mp, obj_table, n, 1);
> 	return ret;
> }
> 
> -- 
> 2.1.0
> 

Keith Wiles, Principal Technologist with CTO office, Wind River mobile 972-213-5533