* [dpdk-dev] [PATCH] mbuf: add bulk free function
@ 2019-09-11 9:19 Morten Brørup
2019-09-11 11:18 ` Stephen Hemminger
0 siblings, 1 reply; 9+ messages in thread
From: Morten Brørup @ 2019-09-11 9:19 UTC (permalink / raw)
To: olivier.matz; +Cc: dev, Morten Brørup
Add function for freeing a bulk of mbufs.
Signed-off-by: Morten Brørup <mb@smartsharesystems.com>
---
lib/librte_mbuf/rte_mbuf.h | 17 +++++++++++++++++
1 file changed, 17 insertions(+)
diff --git a/lib/librte_mbuf/rte_mbuf.h b/lib/librte_mbuf/rte_mbuf.h
index 98225ec80..f2e174da1 100644
--- a/lib/librte_mbuf/rte_mbuf.h
+++ b/lib/librte_mbuf/rte_mbuf.h
@@ -1907,6 +1907,23 @@ static inline void rte_pktmbuf_free(struct rte_mbuf *m)
}
}
+/**
+ * Free a bulk of mbufs back into their original mempool.
+ *
+ * @param mbufs
+ * Array of pointers to mbufs
+ * @param count
+ * Array size
+ */
+static inline void
+rte_pktmbuf_free_bulk(struct rte_mbuf **mbufs, unsigned count)
+{
+ unsigned idx = 0;
+
+ for (idx = 0; idx < count; idx++)
+ rte_pktmbuf_free(mbufs[idx]);
+}
+
/**
* Creates a "clone" of the given packet mbuf.
*
--
2.17.1
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [dpdk-dev] [PATCH] mbuf: add bulk free function
2019-09-11 9:19 [dpdk-dev] [PATCH] mbuf: add bulk free function Morten Brørup
@ 2019-09-11 11:18 ` Stephen Hemminger
2019-09-11 11:29 ` Van Haaren, Harry
2019-09-11 11:33 ` Olivier Matz
0 siblings, 2 replies; 9+ messages in thread
From: Stephen Hemminger @ 2019-09-11 11:18 UTC (permalink / raw)
To: Morten Brørup; +Cc: olivier.matz, dev
On Wed, 11 Sep 2019 09:19:08 +0000
Morten Brørup <mb@smartsharesystems.com> wrote:
> Add function for freeing a bulk of mbufs.
>
> Signed-off-by: Morten Brørup <mb@smartsharesystems.com>
> ---
> lib/librte_mbuf/rte_mbuf.h | 17 +++++++++++++++++
> 1 file changed, 17 insertions(+)
>
> diff --git a/lib/librte_mbuf/rte_mbuf.h b/lib/librte_mbuf/rte_mbuf.h
> index 98225ec80..f2e174da1 100644
> --- a/lib/librte_mbuf/rte_mbuf.h
> +++ b/lib/librte_mbuf/rte_mbuf.h
> @@ -1907,6 +1907,23 @@ static inline void rte_pktmbuf_free(struct rte_mbuf *m)
> }
> }
>
> +/**
> + * Free a bulk of mbufs back into their original mempool.
> + *
> + * @param mbufs
> + * Array of pointers to mbufs
> + * @param count
> + * Array size
> + */
> +static inline void
> +rte_pktmbuf_free_bulk(struct rte_mbuf **mbufs, unsigned count)
> +{
> + unsigned idx = 0;
> +
> + for (idx = 0; idx < count; idx++)
> + rte_pktmbuf_free(mbufs[idx]);
> +}
> +
You can optimize this to use mempool bulk put operation.
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [dpdk-dev] [PATCH] mbuf: add bulk free function
2019-09-11 11:18 ` Stephen Hemminger
@ 2019-09-11 11:29 ` Van Haaren, Harry
2019-09-11 11:41 ` Ananyev, Konstantin
2019-09-11 11:33 ` Olivier Matz
1 sibling, 1 reply; 9+ messages in thread
From: Van Haaren, Harry @ 2019-09-11 11:29 UTC (permalink / raw)
To: Stephen Hemminger, Morten Brørup; +Cc: olivier.matz, dev
> -----Original Message-----
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Stephen Hemminger
> Sent: Wednesday, September 11, 2019 12:19 PM
> To: Morten Brørup <mb@smartsharesystems.com>
> Cc: olivier.matz@6wind.com; dev@dpdk.org
> Subject: Re: [dpdk-dev] [PATCH] mbuf: add bulk free function
>
> On Wed, 11 Sep 2019 09:19:08 +0000
> Morten Brørup <mb@smartsharesystems.com> wrote:
>
> > Add function for freeing a bulk of mbufs.
> >
> > Signed-off-by: Morten Brørup <mb@smartsharesystems.com>
> > ---
> > lib/librte_mbuf/rte_mbuf.h | 17 +++++++++++++++++
> > 1 file changed, 17 insertions(+)
> >
> > diff --git a/lib/librte_mbuf/rte_mbuf.h b/lib/librte_mbuf/rte_mbuf.h
> > index 98225ec80..f2e174da1 100644
> > --- a/lib/librte_mbuf/rte_mbuf.h
> > +++ b/lib/librte_mbuf/rte_mbuf.h
> > @@ -1907,6 +1907,23 @@ static inline void rte_pktmbuf_free(struct rte_mbuf
> *m)
> > }
> > }
> >
> > +/**
> > + * Free a bulk of mbufs back into their original mempool.
> > + *
> > + * @param mbufs
> > + * Array of pointers to mbufs
> > + * @param count
> > + * Array size
> > + */
> > +static inline void
> > +rte_pktmbuf_free_bulk(struct rte_mbuf **mbufs, unsigned count)
> > +{
> > + unsigned idx = 0;
> > +
> > + for (idx = 0; idx < count; idx++)
> > + rte_pktmbuf_free(mbufs[idx]);
> > +}
> > +
>
> You can optimize this to use mempool bulk put operation.
I believe there's a nuance here - not all mbufs may come from the same mempool.
The for() approach will free each to its "home" mempool.
The bulk() approach may return mbufs to pools they didn't originate from.
For performance reasons it would be nice if they did, but we (in the DPDK library)
should not blindly assume that.
We could consider adding a 2nd functions, rte_pktmbuf_free_bulk_to_single_mempool()
or some better descriptive name.
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [dpdk-dev] [PATCH] mbuf: add bulk free function
2019-09-11 11:18 ` Stephen Hemminger
2019-09-11 11:29 ` Van Haaren, Harry
@ 2019-09-11 11:33 ` Olivier Matz
2019-09-11 11:39 ` Stephen Hemminger
2019-09-11 12:14 ` Morten Brørup
1 sibling, 2 replies; 9+ messages in thread
From: Olivier Matz @ 2019-09-11 11:33 UTC (permalink / raw)
To: Stephen Hemminger; +Cc: Morten Brørup, dev
Hi,
On Wed, Sep 11, 2019 at 12:18:34PM +0100, Stephen Hemminger wrote:
> On Wed, 11 Sep 2019 09:19:08 +0000
> Morten Brørup <mb@smartsharesystems.com> wrote:
>
> > Add function for freeing a bulk of mbufs.
> >
> > Signed-off-by: Morten Brørup <mb@smartsharesystems.com>
> > ---
> > lib/librte_mbuf/rte_mbuf.h | 17 +++++++++++++++++
> > 1 file changed, 17 insertions(+)
> >
> > diff --git a/lib/librte_mbuf/rte_mbuf.h b/lib/librte_mbuf/rte_mbuf.h
> > index 98225ec80..f2e174da1 100644
> > --- a/lib/librte_mbuf/rte_mbuf.h
> > +++ b/lib/librte_mbuf/rte_mbuf.h
> > @@ -1907,6 +1907,23 @@ static inline void rte_pktmbuf_free(struct rte_mbuf *m)
> > }
> > }
> >
> > +/**
> > + * Free a bulk of mbufs back into their original mempool.
> > + *
> > + * @param mbufs
> > + * Array of pointers to mbufs
> > + * @param count
> > + * Array size
> > + */
> > +static inline void
> > +rte_pktmbuf_free_bulk(struct rte_mbuf **mbufs, unsigned count)
> > +{
> > + unsigned idx = 0;
> > +
> > + for (idx = 0; idx < count; idx++)
> > + rte_pktmbuf_free(mbufs[idx]);
> > +}
> > +
>
> You can optimize this to use mempool bulk put operation.
A bulk free for mbuf is not as simple as a bulk mempool put, because
of indirect mbufs, and because mbufs may return in different mempools.
Morten, do you have more details about why do you need such a function?
Thanks,
Olivier
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [dpdk-dev] [PATCH] mbuf: add bulk free function
2019-09-11 11:33 ` Olivier Matz
@ 2019-09-11 11:39 ` Stephen Hemminger
2019-09-11 12:14 ` Morten Brørup
1 sibling, 0 replies; 9+ messages in thread
From: Stephen Hemminger @ 2019-09-11 11:39 UTC (permalink / raw)
To: Olivier Matz; +Cc: Morten Brørup, dev
On Wed, 11 Sep 2019 13:33:13 +0200
Olivier Matz <olivier.matz@6wind.com> wrote:
> Hi,
>
> On Wed, Sep 11, 2019 at 12:18:34PM +0100, Stephen Hemminger wrote:
> > On Wed, 11 Sep 2019 09:19:08 +0000
> > Morten Brørup <mb@smartsharesystems.com> wrote:
> >
> > > Add function for freeing a bulk of mbufs.
> > >
> > > Signed-off-by: Morten Brørup <mb@smartsharesystems.com>
> > > ---
> > > lib/librte_mbuf/rte_mbuf.h | 17 +++++++++++++++++
> > > 1 file changed, 17 insertions(+)
> > >
> > > diff --git a/lib/librte_mbuf/rte_mbuf.h b/lib/librte_mbuf/rte_mbuf.h
> > > index 98225ec80..f2e174da1 100644
> > > --- a/lib/librte_mbuf/rte_mbuf.h
> > > +++ b/lib/librte_mbuf/rte_mbuf.h
> > > @@ -1907,6 +1907,23 @@ static inline void rte_pktmbuf_free(struct rte_mbuf *m)
> > > }
> > > }
> > >
> > > +/**
> > > + * Free a bulk of mbufs back into their original mempool.
> > > + *
> > > + * @param mbufs
> > > + * Array of pointers to mbufs
> > > + * @param count
> > > + * Array size
> > > + */
> > > +static inline void
> > > +rte_pktmbuf_free_bulk(struct rte_mbuf **mbufs, unsigned count)
> > > +{
> > > + unsigned idx = 0;
> > > +
> > > + for (idx = 0; idx < count; idx++)
> > > + rte_pktmbuf_free(mbufs[idx]);
> > > +}
> > > +
> >
> > You can optimize this to use mempool bulk put operation.
>
> A bulk free for mbuf is not as simple as a bulk mempool put, because
> of indirect mbufs, and because mbufs may return in different mempools.
>
> Morten, do you have more details about why do you need such a function?
>
> Thanks,
> Olivier
I was thinking of a function that looked at the list and if they were all
the same pool and safe to bulk put, then use that as a fast path. This would
be the most common case.
Also, less inline functions please. When it is an inline it adds more API/ABI
dependencies.
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [dpdk-dev] [PATCH] mbuf: add bulk free function
2019-09-11 11:29 ` Van Haaren, Harry
@ 2019-09-11 11:41 ` Ananyev, Konstantin
2019-09-15 9:07 ` Morten Brørup
2019-09-15 9:24 ` Morten Brørup
0 siblings, 2 replies; 9+ messages in thread
From: Ananyev, Konstantin @ 2019-09-11 11:41 UTC (permalink / raw)
To: Van Haaren, Harry, Stephen Hemminger, Morten Brørup
Cc: olivier.matz, dev
> -----Original Message-----
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Van Haaren, Harry
> Sent: Wednesday, September 11, 2019 12:30 PM
> To: Stephen Hemminger <stephen@networkplumber.org>; Morten Brørup <mb@smartsharesystems.com>
> Cc: olivier.matz@6wind.com; dev@dpdk.org
> Subject: Re: [dpdk-dev] [PATCH] mbuf: add bulk free function
>
> > -----Original Message-----
> > From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Stephen Hemminger
> > Sent: Wednesday, September 11, 2019 12:19 PM
> > To: Morten Brørup <mb@smartsharesystems.com>
> > Cc: olivier.matz@6wind.com; dev@dpdk.org
> > Subject: Re: [dpdk-dev] [PATCH] mbuf: add bulk free function
> >
> > On Wed, 11 Sep 2019 09:19:08 +0000
> > Morten Brørup <mb@smartsharesystems.com> wrote:
> >
> > > Add function for freeing a bulk of mbufs.
> > >
> > > Signed-off-by: Morten Brørup <mb@smartsharesystems.com>
> > > ---
> > > lib/librte_mbuf/rte_mbuf.h | 17 +++++++++++++++++
> > > 1 file changed, 17 insertions(+)
> > >
> > > diff --git a/lib/librte_mbuf/rte_mbuf.h b/lib/librte_mbuf/rte_mbuf.h
> > > index 98225ec80..f2e174da1 100644
> > > --- a/lib/librte_mbuf/rte_mbuf.h
> > > +++ b/lib/librte_mbuf/rte_mbuf.h
> > > @@ -1907,6 +1907,23 @@ static inline void rte_pktmbuf_free(struct rte_mbuf
> > *m)
> > > }
> > > }
> > >
> > > +/**
> > > + * Free a bulk of mbufs back into their original mempool.
> > > + *
> > > + * @param mbufs
> > > + * Array of pointers to mbufs
> > > + * @param count
> > > + * Array size
> > > + */
> > > +static inline void
> > > +rte_pktmbuf_free_bulk(struct rte_mbuf **mbufs, unsigned count)
> > > +{
> > > + unsigned idx = 0;
> > > +
> > > + for (idx = 0; idx < count; idx++)
> > > + rte_pktmbuf_free(mbufs[idx]);
> > > +}
> > > +
> >
> > You can optimize this to use mempool bulk put operation.
>
> I believe there's a nuance here - not all mbufs may come from the same mempool.
> The for() approach will free each to its "home" mempool.
> The bulk() approach may return mbufs to pools they didn't originate from.
>
> For performance reasons it would be nice if they did, but we (in the DPDK library)
> should not blindly assume that.
I suppose Stephen is aware of that and suggests something similar to
What many PMDs are already doing. Let say in ixgbe:
static __rte_always_inline int
ixgbe_tx_free_bufs(struct ixgbe_tx_queue *txq)
{
....
for (i = 0; i < txq->tx_rs_thresh; ++i, ++txep) {
/* free buffers one at a time */
m = rte_pktmbuf_prefree_seg(txep->mbuf);
txep->mbuf = NULL;
if (unlikely(m == NULL))
continue;
if (nb_free >= RTE_IXGBE_TX_MAX_FREE_BUF_SZ ||
(nb_free > 0 && m->pool != free[0]->pool)) {
rte_mempool_put_bulk(free[0]->pool,
(void **)free, nb_free);
nb_free = 0;
}
free[nb_free++] = m;
}
}
Of course generic function will also need to go through all segments in each packet.
> We could consider adding a 2nd functions, rte_pktmbuf_free_bulk_to_single_mempool()
> or some better descriptive name.
Probably a good idea too.
Konstantin
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [dpdk-dev] [PATCH] mbuf: add bulk free function
2019-09-11 11:33 ` Olivier Matz
2019-09-11 11:39 ` Stephen Hemminger
@ 2019-09-11 12:14 ` Morten Brørup
1 sibling, 0 replies; 9+ messages in thread
From: Morten Brørup @ 2019-09-11 12:14 UTC (permalink / raw)
To: Olivier Matz, Stephen Hemminger, Van Haaren, Harry, Konstantin Ananyev
Cc: dev
> -----Original Message-----
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Olivier Matz
>
> Morten, do you have more details about why do you need such a function?
Our application needs it for various purposes, e.g.:
- If rte_eth_tx_burst() returns with not all packets transmitted, our application may decide to drop the unsent packets.
- If there is too much pressure on the system, or an internal queue is full, so it decides to drop packets at some stage. (WAN Optimization appliances tend to buffer large numbers of packets, as opposed to switching/routing appliances.)
Furthermore, try searching DPDK apps/libraries/examples for loops calling rte_pktmbuf_free() with an indexed array... It's copy-paste all over. I would say that this function seems to be missing, and should have been added a long time ago.
>
> Thanks,
> Olivier
Med venlig hilsen / kind regards
- Morten Brørup
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [dpdk-dev] [PATCH] mbuf: add bulk free function
2019-09-11 11:41 ` Ananyev, Konstantin
@ 2019-09-15 9:07 ` Morten Brørup
2019-09-15 9:24 ` Morten Brørup
1 sibling, 0 replies; 9+ messages in thread
From: Morten Brørup @ 2019-09-15 9:07 UTC (permalink / raw)
To: Ananyev, Konstantin, Van Haaren, Harry, Stephen Hemminger
Cc: olivier.matz, dev
> -----Original Message-----
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Ananyev, Konstantin
> Sent: Wednesday, September 11, 2019 1:42 PM
> To: Van Haaren, Harry; Stephen Hemminger; Morten Brørup
> Cc: olivier.matz@6wind.com; dev@dpdk.org
> Subject: Re: [dpdk-dev] [PATCH] mbuf: add bulk free function
>
>
>
> > -----Original Message-----
> > From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Van Haaren, Harry
> > Sent: Wednesday, September 11, 2019 12:30 PM
> > To: Stephen Hemminger <stephen@networkplumber.org>; Morten Brørup
> <mb@smartsharesystems.com>
> > Cc: olivier.matz@6wind.com; dev@dpdk.org
> > Subject: Re: [dpdk-dev] [PATCH] mbuf: add bulk free function
> >
> > > -----Original Message-----
> > > From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Stephen Hemminger
> > > Sent: Wednesday, September 11, 2019 12:19 PM
> > > To: Morten Brørup <mb@smartsharesystems.com>
> > > Cc: olivier.matz@6wind.com; dev@dpdk.org
> > > Subject: Re: [dpdk-dev] [PATCH] mbuf: add bulk free function
> > >
> > > On Wed, 11 Sep 2019 09:19:08 +0000
> > > Morten Brørup <mb@smartsharesystems.com> wrote:
> > >
> > > > Add function for freeing a bulk of mbufs.
> > > >
> > > > Signed-off-by: Morten Brørup <mb@smartsharesystems.com>
> > > > ---
> > > > lib/librte_mbuf/rte_mbuf.h | 17 +++++++++++++++++
> > > > 1 file changed, 17 insertions(+)
> > > >
> > > > diff --git a/lib/librte_mbuf/rte_mbuf.h b/lib/librte_mbuf/rte_mbuf.h
> > > > index 98225ec80..f2e174da1 100644
> > > > --- a/lib/librte_mbuf/rte_mbuf.h
> > > > +++ b/lib/librte_mbuf/rte_mbuf.h
> > > > @@ -1907,6 +1907,23 @@ static inline void rte_pktmbuf_free(struct
> rte_mbuf
> > > *m)
> > > > }
> > > > }
> > > >
> > > > +/**
> > > > + * Free a bulk of mbufs back into their original mempool.
> > > > + *
> > > > + * @param mbufs
> > > > + * Array of pointers to mbufs
> > > > + * @param count
> > > > + * Array size
> > > > + */
> > > > +static inline void
> > > > +rte_pktmbuf_free_bulk(struct rte_mbuf **mbufs, unsigned count)
> > > > +{
> > > > + unsigned idx = 0;
> > > > +
> > > > + for (idx = 0; idx < count; idx++)
> > > > + rte_pktmbuf_free(mbufs[idx]);
> > > > +}
> > > > +
> > >
> > > You can optimize this to use mempool bulk put operation.
> >
> > I believe there's a nuance here - not all mbufs may come from the same
> mempool.
> > The for() approach will free each to its "home" mempool.
> > The bulk() approach may return mbufs to pools they didn't originate from.
> >
> > For performance reasons it would be nice if they did, but we (in the DPDK
> library)
> > should not blindly assume that.
>
> I suppose Stephen is aware of that and suggests something similar to
> What many PMDs are already doing. Let say in ixgbe:
> static __rte_always_inline int
> ixgbe_tx_free_bufs(struct ixgbe_tx_queue *txq)
> {
> ....
> for (i = 0; i < txq->tx_rs_thresh; ++i, ++txep) {
> /* free buffers one at a time */
> m = rte_pktmbuf_prefree_seg(txep->mbuf);
> txep->mbuf = NULL;
>
> if (unlikely(m == NULL))
> continue;
>
> if (nb_free >= RTE_IXGBE_TX_MAX_FREE_BUF_SZ ||
> (nb_free > 0 && m->pool != free[0]->pool)) {
> rte_mempool_put_bulk(free[0]->pool,
> (void **)free, nb_free);
> nb_free = 0;
> }
>
> free[nb_free++] = m;
> }
> }
>
> Of course generic function will also need to go through all segments in
> each packet.
>
Thank you for the clarifying example!
It looks like this optimization behaves as if RTE_LIBRTE_MBUF_DEBUG is not set.
In that case, the existing rte_pktmbuf_free() could be optimized similarly to free multiple segments in bulk. (Except that the use of likely/unlikely in the mbuf library heavily favors single-segment mbufs, so such an optimization would go against this favoritism.)
And yes, I see how rte_pktmbuf_free_bulk() could be optimized similarly. But I don't think it's appropriate for mbuf library functions to assume that RTE_LIBRTE_MBUF_DEBUG is not set. Although it could be implemented both ways, controlled by #ifdef RTE_LIBRTE_MBUF_DEBUG / #else / #endif.
> > We could consider adding a 2nd functions,
> rte_pktmbuf_free_bulk_to_single_mempool()
> > or some better descriptive name.
>
> Probably a good idea too.
> Konstantin
>
Feature creep. I prefer the generic function only.
Med venlig hilsen / kind regards
- Morten Brørup
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [dpdk-dev] [PATCH] mbuf: add bulk free function
2019-09-11 11:41 ` Ananyev, Konstantin
2019-09-15 9:07 ` Morten Brørup
@ 2019-09-15 9:24 ` Morten Brørup
1 sibling, 0 replies; 9+ messages in thread
From: Morten Brørup @ 2019-09-15 9:24 UTC (permalink / raw)
To: Ananyev, Konstantin, Van Haaren, Harry, Stephen Hemminger
Cc: olivier.matz, dev
> -----Original Message-----
> From: Morten Brørup
> Sent: Sunday, September 15, 2019 11:07 AM
[...]
> [...] (Except that the use of likely/unlikely
> in the mbuf library heavily favors single-segment mbufs, so such an
> optimization would go against this favoritism.)
>
Ignore that statement. I got two things mixed up... Single-reference mbufs are favored, not single-segment mbufs.
Med venlig hilsen / kind regards
- Morten Brørup
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2019-09-15 9:24 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-09-11 9:19 [dpdk-dev] [PATCH] mbuf: add bulk free function Morten Brørup
2019-09-11 11:18 ` Stephen Hemminger
2019-09-11 11:29 ` Van Haaren, Harry
2019-09-11 11:41 ` Ananyev, Konstantin
2019-09-15 9:07 ` Morten Brørup
2019-09-15 9:24 ` Morten Brørup
2019-09-11 11:33 ` Olivier Matz
2019-09-11 11:39 ` Stephen Hemminger
2019-09-11 12:14 ` Morten Brørup
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).