On 10/8/19 11:12 PM, Morten Brørup wrote:
-----Original Message-----
From: dev [mailto:[email protected]] On Behalf Of Andrew Rybchenko
Sent: Tuesday, October 8, 2019 7:26 PM
On 10/7/19 1:14 PM, Morten Brørup wrote:
Add function for freeing a bulk of mbufs.
v4:
* Marked as experimental by adding __rte_experimental.
* Add function to experimental section of map file.
* Fix source code formatting regarding pointer to pointer.
* Squashed multiple modifications into one.
v3:
* Bugfix: Handle pakets with multiple segments.
* Added inline helper function, mainly for readability.
* Fix source code formatting regarding indentation.
v2:
* Function is not inline.
* Optimized 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.
* Fixed a typo in the description headline: mempools is plural.
Signed-off-by: Morten Brørup <[email protected]>
---
lib/librte_mbuf/rte_mbuf.c | 50 ++++++++++++++++++++++++++++
lib/librte_mbuf/rte_mbuf.h | 12 +++++++
lib/librte_mbuf/rte_mbuf_version.map | 1 +
3 files changed, 63 insertions(+)
diff --git a/lib/librte_mbuf/rte_mbuf.c b/lib/librte_mbuf/rte_mbuf.c
index 37718d49c..0088117a3 100644
--- a/lib/librte_mbuf/rte_mbuf.c
+++ b/lib/librte_mbuf/rte_mbuf.c
@@ -245,6 +245,56 @@ int rte_mbuf_check(const struct rte_mbuf *m, int
is_header,
return 0;
}
+/**
+ * Size of the array holding mbufs from the same membool to be freed in
bulk.
+ */
+#define RTE_PKTMBUF_FREE_BULK_SZ 64
How is the constant chosen? When we benchmarked similar thing in
net/sfc, we finally stick to 32 as the best option.
I took it from the ixgbe driver that was the inspiration:
http://code.dpdk.org/dpdk/latest/source/drivers/net/ixgbe/ixgbe_rxtx.c#L111
http://code.dpdk.org/dpdk/latest/source/drivers/net/ixgbe/ixgbe_rxtx.h#L32
If only DPDK provided a common #define for recommended burst array sizes.
However, I vaguely recall a discussion recommending it to be dependent on the
application and various drivers due to requirements of 100 Gbps drivers. I
guess it probably also depends on the CPU being used. I have no qualified
opinion about this constant, so I can change it to anything you recommend.
No, no, it is OK for me as is. Just curiosity.
+
+/**
+ * @internal helper function for freeing a bulk of mbufs via an array
holding
+ * mbufs from the same mempool.
+ */
+static __rte_always_inline void
+rte_pktmbuf_free_seg_via_array(struct rte_mbuf *m,
Other internal functions in the file have __ (two underscore) prefix.
The other internal function is public, i.e. not private (static). However, this
might also go public one day, so I'll update it in the next version.
+ struct rte_mbuf ** const free, unsigned int * const nb_free)
free is bad variable name because of libc free() function.
It is a bit confusing. Same below.
I agree. I simply took it from the ixgbe driver without thinking further about it. How about
"pending" or "bulk", or do you have a better suggestion?
Both "pending" and "bulk" are OK for me. Up to you.
+{
+ m = rte_pktmbuf_prefree_seg(m);
+ if (likely(m != NULL)) {
I'm not sure about likely() here, but hope that compiler is wise
enough to inline rte_pktmbuf_prefree_seg() here and avoid the
branch completely.
I somewhat agree with you about this likely() being questionable, and also hope
for the compiler to be wise enough anyway.
This line of code in my function stems from unfolding rte_pktmbuf_free_seg(),
which has the same likely(), so I kept it.
Apparently, the DPDK mbuf library has a strong preference for single-reference
mbufs, with lots of likely()/unlikely() assuming that the m.refcnt is never
more than one. rte_pktmbuf_prefree_seg() has the same preference. So I think
that we should follow the library's convention, respecting that this function
is at the very core of the data plane.
Makes sense.
The DPDK rationale behind these likely()/unlikely() hints must be that
multicasting, flooding and mirroring packets are rare exceptions. We have
previously discussed this internally in our organization, and we tend to agree
under normal circumstances. It is just an unfortunate side effect that a common
network debugging feature - mirroring - introduces an additional performance
penalty of having likely()/unlikely() go in the wrong direction, which might
again affect what is being debugged.
Thanks,
Andrew.
