diff mbox series

[v5,1/1] mbuf: add bulk free function

Message ID 20191009135511.84547-2-mb@smartsharesystems.com (mailing list archive)
State Superseded, archived
Headers
Series mbuf: add bulk free function |

Checks

Context Check Description
ci/checkpatch success coding style OK
ci/Intel-compilation success Compilation OK
ci/iol-compilation success Compile Testing PASS
ci/iol-intel-Performance success Performance Testing PASS
ci/iol-mellanox-Performance success Performance Testing PASS

Commit Message

Morten Brørup Oct. 9, 2019, 1:55 p.m. UTC 
  Add function for freeing a bulk of mbufs.

v5:
* Rename variables from "free" to "pending" for improved readability.
* Add prefix __ to rte_pktmbuf_free_seg_via_array().
* Add array size parameter to __rte_pktmbuf_free_seg_via_array().
  The compiler will optimize the parameter away anyway.
* Add description to __rte_pktmbuf_free_seg_via_array().
* Minor description updates.
v4:
* Mark as experimental by adding __rte_experimental.
* Add function to experimental section of map file.
* Fix source code formatting regarding pointer to pointer.
* Squash multiple modifications into one.
v3:
* Bugfix: Handle pakets with multiple segments.
* Add inline helper function, mainly for readability.
* Fix source code formatting regarding indentation.
v2:
* Function is not inline.
* Optimize to free multible mbufs belonging to the same mempool in
  bulk. Inspired by ixgbe_tx_free_bufs(), but allowing NULL pointers
  in the array, just like rte_pktmbuf_free() can take a NULL pointer.
* Use unsigned int instead of unsigned. Passes checkpatch, but
  mismatches the original coding style of the modified files.
* Fix a typo in the description headline: mempools is plural.

Signed-off-by: Morten Brørup <mb@smartsharesystems.com>
---
 lib/librte_mbuf/rte_mbuf.c           | 66 ++++++++++++++++++++++++++++
 lib/librte_mbuf/rte_mbuf.h           | 15 +++++++
 lib/librte_mbuf/rte_mbuf_version.map |  1 +
 3 files changed, 82 insertions(+)

Comments

Stephen Hemminger Oct. 9, 2019, 10:54 p.m. UTC | #1 
On Wed,  9 Oct 2019 13:55:11 +0000
Morten Brørup <mb@smartsharesystems.com> wrote:

>  
> +/**
> + * @internal helper function for freeing a bulk of packet mbuf segments
> + * via an array holding the packet mbuf segments from the same mempool
> + * pending to be freed.
> + *
> + * @param m
> + *  The packet mbuf segment to be freed.
> + * @param pending
> + *  Pointer to the array of packet mbuf segments pending to be freed.
> + * @param nb_pending
> + *  Pointer to the number of elements held in the array.
> + * @param pending_sz
> + *  Number of elements the array can hold.
> + *  Note: The compiler should optimize this parameter away when using a
> + *  constant value, such as RTE_PKTMBUF_FREE_PENDING_SZ.
> + */
> +static __rte_always_inline void
> +__rte_pktmbuf_free_seg_via_array(struct rte_mbuf *m,

Overall the patch looks good, but don't think always_inline
is required here. That should be reserved for things that use
inline assembly or other stuff that would be broken if it wasn't
inlined.

Most compilers would inline it without any modifier anyway.
Ananyev, Konstantin Oct. 10, 2019, 12:01 a.m. UTC | #2 
> Add function for freeing a bulk of mbufs.
> 
> v5:
> * Rename variables from "free" to "pending" for improved readability.
> * Add prefix __ to rte_pktmbuf_free_seg_via_array().
> * Add array size parameter to __rte_pktmbuf_free_seg_via_array().
>   The compiler will optimize the parameter away anyway.
> * Add description to __rte_pktmbuf_free_seg_via_array().
> * Minor description updates.
> v4:
> * Mark as experimental by adding __rte_experimental.
> * Add function to experimental section of map file.
> * Fix source code formatting regarding pointer to pointer.
> * Squash multiple modifications into one.
> v3:
> * Bugfix: Handle pakets with multiple segments.
> * Add inline helper function, mainly for readability.
> * Fix source code formatting regarding indentation.
> v2:
> * Function is not inline.
> * Optimize to free multible mbufs belonging to the same mempool in
>   bulk. Inspired by ixgbe_tx_free_bufs(), but allowing NULL pointers
>   in the array, just like rte_pktmbuf_free() can take a NULL pointer.
> * Use unsigned int instead of unsigned. Passes checkpatch, but
>   mismatches the original coding style of the modified files.
> * Fix a typo in the description headline: mempools is plural.
> 
> Signed-off-by: Morten Brørup <mb@smartsharesystems.com>
> ---
>  lib/librte_mbuf/rte_mbuf.c           | 66 ++++++++++++++++++++++++++++
>  lib/librte_mbuf/rte_mbuf.h           | 15 +++++++
>  lib/librte_mbuf/rte_mbuf_version.map |  1 +
>  3 files changed, 82 insertions(+)
> 
> diff --git a/lib/librte_mbuf/rte_mbuf.c b/lib/librte_mbuf/rte_mbuf.c
> index 37718d49c..de5659baa 100644
> --- a/lib/librte_mbuf/rte_mbuf.c
> +++ b/lib/librte_mbuf/rte_mbuf.c
> @@ -245,6 +245,72 @@ int rte_mbuf_check(const struct rte_mbuf *m, int is_header,
>  	return 0;
>  }
> 
> +/**
> + * @internal helper function for freeing a bulk of packet mbuf segments
> + * via an array holding the packet mbuf segments from the same mempool
> + * pending to be freed.
> + *
> + * @param m
> + *  The packet mbuf segment to be freed.
> + * @param pending
> + *  Pointer to the array of packet mbuf segments pending to be freed.
> + * @param nb_pending
> + *  Pointer to the number of elements held in the array.
> + * @param pending_sz
> + *  Number of elements the array can hold.
> + *  Note: The compiler should optimize this parameter away when using a
> + *  constant value, such as RTE_PKTMBUF_FREE_PENDING_SZ.
> + */
> +static __rte_always_inline void
> +__rte_pktmbuf_free_seg_via_array(struct rte_mbuf *m,
> +	struct rte_mbuf ** const pending, unsigned int * const nb_pending,
> +	const unsigned int pending_sz)
> +{
> +	m = rte_pktmbuf_prefree_seg(m);
> +	if (likely(m != NULL)) {
> +		if (*nb_pending == pending_sz ||
> +		    (*nb_pending > 0 && m->pool != pending[0]->pool)) {
> +			rte_mempool_put_bulk(pending[0]->pool,
> +					(void **)pending, *nb_pending);
> +			*nb_pending = 0;
> +		}
> +
> +		pending[(*nb_pending)++] = m;
> +	}
> +}
> +
> +/**
> + * Size of the array holding mbufs from the same membool pending to be freed
> + * in bulk.
> + */
> +#define RTE_PKTMBUF_FREE_PENDING_SZ 64
> +
> +/* Free a bulk of packet mbufs back into their original mempools. */
> +void rte_pktmbuf_free_bulk(struct rte_mbuf **mbufs, unsigned int count)
> +{
> +	struct rte_mbuf *m, *m_next, *pending[RTE_PKTMBUF_FREE_PENDING_SZ];
> +	unsigned int idx, nb_pending = 0;
> +
> +	for (idx = 0; idx < count; idx++) {
> +		m = mbufs[idx];
> +		if (unlikely(m == NULL))
> +			continue;
> +
> +		__rte_mbuf_sanity_check(m, 1);
> +
> +		do {
> +			m_next = m->next;
> +			__rte_pktmbuf_free_seg_via_array(m,
> +					pending, &nb_pending,
> +					RTE_PKTMBUF_FREE_PENDING_SZ);
> +			m = m_next;
> +		} while (m != NULL);
> +	}
> +
> +	if (nb_pending > 0)
> +		rte_mempool_put_bulk(pending[0]->pool, (void **)pending, nb_pending);
> +}
> +
>  /* dump a mbuf on console */
>  void
>  rte_pktmbuf_dump(FILE *f, const struct rte_mbuf *m, unsigned dump_len)
> diff --git a/lib/librte_mbuf/rte_mbuf.h b/lib/librte_mbuf/rte_mbuf.h
> index 98225ec80..a02c8b12c 100644
> --- a/lib/librte_mbuf/rte_mbuf.h
> +++ b/lib/librte_mbuf/rte_mbuf.h
> @@ -1907,6 +1907,21 @@ static inline void rte_pktmbuf_free(struct rte_mbuf *m)
>  	}
>  }
> 
> +/**
> + * Free a bulk of packet mbufs back into their original mempools.
> + *
> + * Free a bulk of mbufs, and all their segments in case of chained buffers.
> + * Each segment is added back into its original mempool.
> + *
> + *  @param mbufs
> + *    Array of pointers to packet mbufs.
> + *    The array may contain NULL pointers.
> + *  @param count
> + *    Array size.
> + */
> +__rte_experimental
> +void rte_pktmbuf_free_bulk(struct rte_mbuf **mbufs, unsigned int count);
> +
>  /**
>   * Creates a "clone" of the given packet mbuf.
>   *
> diff --git a/lib/librte_mbuf/rte_mbuf_version.map b/lib/librte_mbuf/rte_mbuf_version.map
> index 2662a37bf..cd6154ef2 100644
> --- a/lib/librte_mbuf/rte_mbuf_version.map
> +++ b/lib/librte_mbuf/rte_mbuf_version.map
> @@ -50,4 +50,5 @@ EXPERIMENTAL {
>  	global:
> 
>  	rte_mbuf_check;
> +	rte_pktmbuf_free_bulk;
>  } DPDK_18.08;
> --

Acked-by: Konstantin Ananyev <konstantin.ananyev@intel.com>

> 2.17.1
Andrew Rybchenko Oct. 10, 2019, 6:25 a.m. UTC | #3 
On 10/10/19 1:54 AM, Stephen Hemminger wrote:
> On Wed,  9 Oct 2019 13:55:11 +0000
> Morten Brørup <mb@smartsharesystems.com> wrote:
>
>>   
>> +/**
>> + * @internal helper function for freeing a bulk of packet mbuf segments
>> + * via an array holding the packet mbuf segments from the same mempool
>> + * pending to be freed.
>> + *
>> + * @param m
>> + *  The packet mbuf segment to be freed.
>> + * @param pending
>> + *  Pointer to the array of packet mbuf segments pending to be freed.
>> + * @param nb_pending
>> + *  Pointer to the number of elements held in the array.
>> + * @param pending_sz
>> + *  Number of elements the array can hold.
>> + *  Note: The compiler should optimize this parameter away when using a
>> + *  constant value, such as RTE_PKTMBUF_FREE_PENDING_SZ.
>> + */
>> +static __rte_always_inline void
>> +__rte_pktmbuf_free_seg_via_array(struct rte_mbuf *m,
> Overall the patch looks good, but don't think always_inline
> is required here. That should be reserved for things that use
> inline assembly or other stuff that would be broken if it wasn't
> inlined.
>
> Most compilers would inline it without any modifier anyway.

I agree with Stephen that always_inline is not required here.

Anyway,

Reviewed-by: Andrew Rybchenko <arybchenko@solarflare.com>
Morten Brørup Oct. 10, 2019, 8:33 a.m. UTC | #4 
Add function for freeing a bulk of mbufs.

v6:
* Remove __rte_always_inline from static function.
  The compiler will inline anyway.
v5:
* Rename variables from "free" to "pending" for improved readability.
* Add prefix __ to rte_pktmbuf_free_seg_via_array().
* Add array size parameter to __rte_pktmbuf_free_seg_via_array().
  The compiler will optimize the parameter away anyway.
* Add description to __rte_pktmbuf_free_seg_via_array().
* Minor description updates.
v4:
* Mark as experimental by adding __rte_experimental.
* Add function to experimental section of map file.
* Fix source code formatting regarding pointer to pointer.
* Squash multiple modifications into one.
v3:
* Bugfix: Handle pakets with multiple segments.
* Add inline helper function, mainly for readability.
* Fix source code formatting regarding indentation.
v2:
* Function is not inline.
* Optimize to free multible mbufs belonging to the same mempool in
  bulk. Inspired by ixgbe_tx_free_bufs(), but allowing NULL pointers
  in the array, just like rte_pktmbuf_free() can take a NULL pointer.
* Use unsigned int instead of unsigned. Passes checkpatch, but
  mismatches the original coding style of the modified files.
* Fix a typo in the description headline: mempools is plural.

Morten Brørup (2):
  mbuf: add bulk free function
  mbuf: add bulk free function

 lib/librte_mbuf/rte_mbuf.c           | 66 ++++++++++++++++++++++++++++
 lib/librte_mbuf/rte_mbuf.h           | 15 +++++++
 lib/librte_mbuf/rte_mbuf_version.map |  1 +
 3 files changed, 82 insertions(+)
Morten Brørup Oct. 10, 2019, 8:49 a.m. UTC | #5 
Olivier, Stephen, Harry, Konstantin, Mattias, Bruce and Andrew,

Thank you for all your constructive feedback on my first non-trivial source code contribution to DPDK.

And thank you for tolerating all the noise from my unfamiliarity with git and patchwork.


Med venlig hilsen / kind regards
- Morten Brørup
diff mbox series

Patch

diff --git a/lib/librte_mbuf/rte_mbuf.c b/lib/librte_mbuf/rte_mbuf.c
index 37718d49c..de5659baa 100644
--- a/lib/librte_mbuf/rte_mbuf.c
+++ b/lib/librte_mbuf/rte_mbuf.c
@@ -245,6 +245,72 @@  int rte_mbuf_check(const struct rte_mbuf *m, int is_header,
 	return 0;
 }
 
+/**
+ * @internal helper function for freeing a bulk of packet mbuf segments
+ * via an array holding the packet mbuf segments from the same mempool
+ * pending to be freed.
+ *
+ * @param m
+ *  The packet mbuf segment to be freed.
+ * @param pending
+ *  Pointer to the array of packet mbuf segments pending to be freed.
+ * @param nb_pending
+ *  Pointer to the number of elements held in the array.
+ * @param pending_sz
+ *  Number of elements the array can hold.
+ *  Note: The compiler should optimize this parameter away when using a
+ *  constant value, such as RTE_PKTMBUF_FREE_PENDING_SZ.
+ */
+static __rte_always_inline void
+__rte_pktmbuf_free_seg_via_array(struct rte_mbuf *m,
+	struct rte_mbuf ** const pending, unsigned int * const nb_pending,
+	const unsigned int pending_sz)
+{
+	m = rte_pktmbuf_prefree_seg(m);
+	if (likely(m != NULL)) {
+		if (*nb_pending == pending_sz ||
+		    (*nb_pending > 0 && m->pool != pending[0]->pool)) {
+			rte_mempool_put_bulk(pending[0]->pool,
+					(void **)pending, *nb_pending);
+			*nb_pending = 0;
+		}
+
+		pending[(*nb_pending)++] = m;
+	}
+}
+
+/**
+ * Size of the array holding mbufs from the same membool pending to be freed
+ * in bulk.
+ */
+#define RTE_PKTMBUF_FREE_PENDING_SZ 64
+
+/* Free a bulk of packet mbufs back into their original mempools. */
+void rte_pktmbuf_free_bulk(struct rte_mbuf **mbufs, unsigned int count)
+{
+	struct rte_mbuf *m, *m_next, *pending[RTE_PKTMBUF_FREE_PENDING_SZ];
+	unsigned int idx, nb_pending = 0;
+
+	for (idx = 0; idx < count; idx++) {
+		m = mbufs[idx];
+		if (unlikely(m == NULL))
+			continue;
+
+		__rte_mbuf_sanity_check(m, 1);
+
+		do {
+			m_next = m->next;
+			__rte_pktmbuf_free_seg_via_array(m,
+					pending, &nb_pending,
+					RTE_PKTMBUF_FREE_PENDING_SZ);
+			m = m_next;
+		} while (m != NULL);
+	}
+
+	if (nb_pending > 0)
+		rte_mempool_put_bulk(pending[0]->pool, (void **)pending, nb_pending);
+}
+
 /* dump a mbuf on console */
 void
 rte_pktmbuf_dump(FILE *f, const struct rte_mbuf *m, unsigned dump_len)
diff --git a/lib/librte_mbuf/rte_mbuf.h b/lib/librte_mbuf/rte_mbuf.h
index 98225ec80..a02c8b12c 100644
--- a/lib/librte_mbuf/rte_mbuf.h
+++ b/lib/librte_mbuf/rte_mbuf.h
@@ -1907,6 +1907,21 @@  static inline void rte_pktmbuf_free(struct rte_mbuf *m)
 	}
 }
 
+/**
+ * Free a bulk of packet mbufs back into their original mempools.
+ *
+ * Free a bulk of mbufs, and all their segments in case of chained buffers.
+ * Each segment is added back into its original mempool.
+ *
+ *  @param mbufs
+ *    Array of pointers to packet mbufs.
+ *    The array may contain NULL pointers.
+ *  @param count
+ *    Array size.
+ */
+__rte_experimental
+void rte_pktmbuf_free_bulk(struct rte_mbuf **mbufs, unsigned int count);
+
 /**
  * Creates a "clone" of the given packet mbuf.
  *
diff --git a/lib/librte_mbuf/rte_mbuf_version.map b/lib/librte_mbuf/rte_mbuf_version.map
index 2662a37bf..cd6154ef2 100644
--- a/lib/librte_mbuf/rte_mbuf_version.map
+++ b/lib/librte_mbuf/rte_mbuf_version.map
@@ -50,4 +50,5 @@  EXPERIMENTAL {
 	global:
 
 	rte_mbuf_check;
+	rte_pktmbuf_free_bulk;
 } DPDK_18.08;