[dpdk-dev] [PATCH] mbuf: add bulk free function

[dpdk-dev] [PATCH] mbuf: add bulk free function

[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

Re: [dpdk-dev] [PATCH] mbuf: add bulk free function

[Stephen Hemminger]

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.

Re: [dpdk-dev] [PATCH] mbuf: add bulk free function

[Van Haaren, Harry]

> -----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.

Re: [dpdk-dev] [PATCH] mbuf: add bulk free function

[Ananyev, Konstantin]

> -----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

Re: [dpdk-dev] [PATCH] mbuf: add bulk free function

[Morten Brørup]

> -----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

Re: [dpdk-dev] [PATCH] mbuf: add bulk free function

[Morten Brørup]

> -----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

Re: [dpdk-dev] [PATCH] mbuf: add bulk free function

[Olivier Matz]

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

Re: [dpdk-dev] [PATCH] mbuf: add bulk free function

[Stephen Hemminger]

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.

Re: [dpdk-dev] [PATCH] mbuf: add bulk free function

[Morten Brørup]

> -----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