diff mbox series

[v2,2/2] mbuf: add bulk free function

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

Checks

Context Check Description
ci/checkpatch warning coding style issues
ci/Intel-compilation success Compilation OK

Commit Message

Morten Brørup Sept. 25, 2019, 12:03 p.m. UTC 
  Add function for freeing a bulk of mbufs.

Signed-off-by: Morten Brørup <mb@smartsharesystems.com>
---
 lib/librte_mbuf/rte_mbuf.c | 35 +++++++++++++++++++++++++++++++++++
 lib/librte_mbuf/rte_mbuf.h | 16 +++++-----------
 2 files changed, 40 insertions(+), 11 deletions(-)

Comments

Morten Brørup Sept. 25, 2019, 12:17 p.m. UTC | #1 
Dear Thomas - listed as checkpatch maintainer,

I think this warning is bogus, and is a bug checkpatch.

The line in question was deliberately indented using tabs to the current indentation level, and using spaces for the readability alignment. This makes the code readable in editors with another tab setting than 8 spaces. E.g. 4 spaces is my personal preference.


Med venlig hilsen / kind regards
- Morten Brørup


> -----Original Message-----
> From: checkpatch@dpdk.org [mailto:checkpatch@dpdk.org]
> Sent: Wednesday, September 25, 2019 2:06 PM
> To: test-report@dpdk.org
> Cc: Morten Brørup
> Subject: |WARNING| pw59738 [PATCH v2 2/2] mbuf: add bulk free function
> 
> Test-Label: checkpatch
> Test-Status: WARNING
> http://dpdk.org/patch/59738
> 
> _coding style issues_
> 
> 
> ERROR:CODE_INDENT: code indent should use tabs where possible
> #59: FILE: lib/librte_mbuf/rte_mbuf.c:272:
> +^I^I^I                     (void **)free, nb_free);$
> 
> total: 1 errors, 0 warnings, 67 lines checked
Mattias Rönnblom Sept. 25, 2019, 7:02 p.m. UTC | #2 
On 2019-09-25 14:03, Morten Brørup wrote:
> Add function for freeing a bulk of mbufs.
> 
> Signed-off-by: Morten Brørup <mb@smartsharesystems.com>
> ---
>   lib/librte_mbuf/rte_mbuf.c | 35 +++++++++++++++++++++++++++++++++++
>   lib/librte_mbuf/rte_mbuf.h | 16 +++++-----------
>   2 files changed, 40 insertions(+), 11 deletions(-)
> 
> diff --git a/lib/librte_mbuf/rte_mbuf.c b/lib/librte_mbuf/rte_mbuf.c
> index 37718d49c..b63a0eced 100644
> --- a/lib/librte_mbuf/rte_mbuf.c
> +++ b/lib/librte_mbuf/rte_mbuf.c
> @@ -245,6 +245,41 @@ int rte_mbuf_check(const struct rte_mbuf *m, int is_header,
>   	return 0;
>   }
>   
> +/**
> + * Maximum bulk of mbufs rte_pktmbuf_free_bulk() returns to mempool.
> + */
> +#define RTE_PKTMBUF_FREE_BULK_SZ 64
> +
> +/* Free a bulk of mbufs back into their original mempools. */
> +void rte_pktmbuf_free_bulk(struct rte_mbuf **mbufs, unsigned int count)
> +{
> +	struct rte_mbuf *m, *free[RTE_PKTMBUF_FREE_BULK_SZ];
> +	unsigned int idx, nb_free = 0;
> +
> +	for (idx = 0; idx < count; idx++) {
> +		m = mbufs[idx];
> +		if (unlikely(m == NULL))
> +			continue;
> +
> +		__rte_mbuf_sanity_check(m, 1);
> +		m = rte_pktmbuf_prefree_seg(m);
> +		if (unlikely(m == NULL))
> +			continue;
> +
> +		if (nb_free >= RTE_PKTMBUF_FREE_BULK_SZ ||
> +		    (nb_free > 0 && m->pool != free[0]->pool)) {

Maybe an unlikely() would be in order here?

> +			rte_mempool_put_bulk(free[0]->pool,
> +			                     (void **)free, nb_free);
> +			nb_free = 0;
> +		}
> +
> +		free[nb_free++] = m;
> +	}
> +
> +	if (nb_free > 0)
> +		rte_mempool_put_bulk(free[0]->pool, (void **)free, nb_free);
> +}
> +
>   /* 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 f2e174da1..6910b3fe6 100644
> --- a/lib/librte_mbuf/rte_mbuf.h
> +++ b/lib/librte_mbuf/rte_mbuf.h
> @@ -1908,21 +1908,15 @@ static inline void rte_pktmbuf_free(struct rte_mbuf *m)
>   }
>   
>   /**
> - * Free a bulk of mbufs back into their original mempool.
> + * Free a bulk of mbufs back into their original mempools.
>    *
>    *  @param mbufs
> - *    Array of pointers to mbufs
> + *    Array of pointers to mbufs.
> + *    The array may contain NULL pointers.
>    *  @param count
> - *    Array size
> + *    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]);
> -}
> +void rte_pktmbuf_free_bulk(struct rte_mbuf **mbufs, unsigned int count);
>   
>   /**
>    * Creates a "clone" of the given packet mbuf.
>
Stephen Hemminger Sept. 25, 2019, 11:37 p.m. UTC | #3 
On Wed, 25 Sep 2019 14:17:42 +0200
Morten Brørup <mb@smartsharesystems.com> wrote:

> Dear Thomas - listed as checkpatch maintainer,
> 
> I think this warning is bogus, and is a bug checkpatch.
> 
> The line in question was deliberately indented using tabs to the current indentation level, and using spaces for the readability alignment. This makes the code readable in editors with another tab setting than 8 spaces. E.g. 4 spaces is my personal preference.
> 
> 
> Med venlig hilsen / kind regards
> - Morten Brørup

It is understandable that you have personal preferences, but projects like DPDK rely
on common style across all code. The collective decision has been made to keep a
uniform style similar to the Linux kernel.

No it is not a bug in checkpatch.
Bruce Richardson Sept. 26, 2019, 8:30 a.m. UTC | #4 
On Wed, Sep 25, 2019 at 09:02:28PM +0200, Mattias Rönnblom wrote:
> On 2019-09-25 14:03, Morten Brørup wrote:
> > Add function for freeing a bulk of mbufs.
> > 
> > Signed-off-by: Morten Brørup <mb@smartsharesystems.com>
> > ---
> >   lib/librte_mbuf/rte_mbuf.c | 35 +++++++++++++++++++++++++++++++++++
> >   lib/librte_mbuf/rte_mbuf.h | 16 +++++-----------
> >   2 files changed, 40 insertions(+), 11 deletions(-)
> > 
> > diff --git a/lib/librte_mbuf/rte_mbuf.c b/lib/librte_mbuf/rte_mbuf.c
> > index 37718d49c..b63a0eced 100644
> > --- a/lib/librte_mbuf/rte_mbuf.c
> > +++ b/lib/librte_mbuf/rte_mbuf.c
> > @@ -245,6 +245,41 @@ int rte_mbuf_check(const struct rte_mbuf *m, int is_header,
> >   	return 0;
> >   }
> > +/**
> > + * Maximum bulk of mbufs rte_pktmbuf_free_bulk() returns to mempool.
> > + */
> > +#define RTE_PKTMBUF_FREE_BULK_SZ 64
> > +
> > +/* Free a bulk of mbufs back into their original mempools. */
> > +void rte_pktmbuf_free_bulk(struct rte_mbuf **mbufs, unsigned int count)
> > +{
> > +	struct rte_mbuf *m, *free[RTE_PKTMBUF_FREE_BULK_SZ];
> > +	unsigned int idx, nb_free = 0;
> > +
> > +	for (idx = 0; idx < count; idx++) {
> > +		m = mbufs[idx];
> > +		if (unlikely(m == NULL))
> > +			continue;
> > +
> > +		__rte_mbuf_sanity_check(m, 1);
> > +		m = rte_pktmbuf_prefree_seg(m);
> > +		if (unlikely(m == NULL))
> > +			continue;
> > +
> > +		if (nb_free >= RTE_PKTMBUF_FREE_BULK_SZ ||
> > +		    (nb_free > 0 && m->pool != free[0]->pool)) {
> 
> Maybe an unlikely() would be in order here?
>
I'd caution against it, since it can penalize the cold branch a lot. If a
branch really is predictable the HW branch predictors generally are good
enough to handle it at runtime. So long as a path is a valid path for a
runtime app, i.e. not something like a fatal error only ever hit once in an
entire run, I'd tend to omit likely()/unlikely() calls unless profiling
shows a real performance difference.

/Bruce
Andrew Rybchenko Sept. 26, 2019, 9:26 a.m. UTC | #5 
On 9/25/19 3:03 PM, Morten Brørup wrote:
> Add function for freeing a bulk of mbufs.
>
> Signed-off-by: Morten Brørup <mb@smartsharesystems.com>
> ---
>   lib/librte_mbuf/rte_mbuf.c | 35 +++++++++++++++++++++++++++++++++++
>   lib/librte_mbuf/rte_mbuf.h | 16 +++++-----------
>   2 files changed, 40 insertions(+), 11 deletions(-)
>
> diff --git a/lib/librte_mbuf/rte_mbuf.c b/lib/librte_mbuf/rte_mbuf.c
> index 37718d49c..b63a0eced 100644
> --- a/lib/librte_mbuf/rte_mbuf.c
> +++ b/lib/librte_mbuf/rte_mbuf.c
> @@ -245,6 +245,41 @@ int rte_mbuf_check(const struct rte_mbuf *m, int is_header,
>   	return 0;
>   }
>   
> +/**
> + * Maximum bulk of mbufs rte_pktmbuf_free_bulk() returns to mempool.
> + */
> +#define RTE_PKTMBUF_FREE_BULK_SZ 64
> +
> +/* Free a bulk of mbufs back into their original mempools. */
> +void rte_pktmbuf_free_bulk(struct rte_mbuf **mbufs, unsigned int count)
> +{
> +	struct rte_mbuf *m, *free[RTE_PKTMBUF_FREE_BULK_SZ];
> +	unsigned int idx, nb_free = 0;
> +
> +	for (idx = 0; idx < count; idx++) {
> +		m = mbufs[idx];
> +		if (unlikely(m == NULL))
> +			continue;
> +
> +		__rte_mbuf_sanity_check(m, 1);
> +		m = rte_pktmbuf_prefree_seg(m);

Who cares about next segments if any? It looks like nobody.

> +		if (unlikely(m == NULL))
> +			continue;
> +
> +		if (nb_free >= RTE_PKTMBUF_FREE_BULK_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;
> +	}
> +
> +	if (nb_free > 0)
> +		rte_mempool_put_bulk(free[0]->pool, (void **)free, nb_free);
> +}
> +
>   /* 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 f2e174da1..6910b3fe6 100644
> --- a/lib/librte_mbuf/rte_mbuf.h
> +++ b/lib/librte_mbuf/rte_mbuf.h
> @@ -1908,21 +1908,15 @@ static inline void rte_pktmbuf_free(struct rte_mbuf *m)
>   }
>   
>   /**
> - * Free a bulk of mbufs back into their original mempool.
> + * Free a bulk of mbufs back into their original mempools.
>    *
>    *  @param mbufs
> - *    Array of pointers to mbufs
> + *    Array of pointers to mbufs.
> + *    The array may contain NULL pointers.
>    *  @param count
> - *    Array size
> + *    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]);
> -}
> +void rte_pktmbuf_free_bulk(struct rte_mbuf **mbufs, unsigned int count);
>   
>   /**
>    * Creates a "clone" of the given packet mbuf.

Is it just a mistake that two patches are not squashed?
Ananyev, Konstantin Sept. 26, 2019, 10:23 a.m. UTC | #6 
Hi Morten,

> 
> Add function for freeing a bulk of mbufs.
> 
> Signed-off-by: Morten Brørup <mb@smartsharesystems.com>
> ---
>  lib/librte_mbuf/rte_mbuf.c | 35 +++++++++++++++++++++++++++++++++++
>  lib/librte_mbuf/rte_mbuf.h | 16 +++++-----------
>  2 files changed, 40 insertions(+), 11 deletions(-)
> 
> diff --git a/lib/librte_mbuf/rte_mbuf.c b/lib/librte_mbuf/rte_mbuf.c
> index 37718d49c..b63a0eced 100644
> --- a/lib/librte_mbuf/rte_mbuf.c
> +++ b/lib/librte_mbuf/rte_mbuf.c
> @@ -245,6 +245,41 @@ int rte_mbuf_check(const struct rte_mbuf *m, int is_header,
>  	return 0;
>  }
> 
> +/**
> + * Maximum bulk of mbufs rte_pktmbuf_free_bulk() returns to mempool.
> + */
> +#define RTE_PKTMBUF_FREE_BULK_SZ 64
> +
> +/* Free a bulk of mbufs back into their original mempools. */
> +void rte_pktmbuf_free_bulk(struct rte_mbuf **mbufs, unsigned int count)

As I can see you forgot to handle situation with multi-segs packet.
This one is still I a good one to have, I think.
But probably it should be named rte_pktmbuf_free_seg_bulk()
to avoid any confusion.
Konstantin

> +{
> +	struct rte_mbuf *m, *free[RTE_PKTMBUF_FREE_BULK_SZ];
> +	unsigned int idx, nb_free = 0;
> +
> +	for (idx = 0; idx < count; idx++) {
> +		m = mbufs[idx];
> +		if (unlikely(m == NULL))
> +			continue;
> +
> +		__rte_mbuf_sanity_check(m, 1);
> +		m = rte_pktmbuf_prefree_seg(m);
> +		if (unlikely(m == NULL))
> +			continue;
> +
> +		if (nb_free >= RTE_PKTMBUF_FREE_BULK_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;
> +	}
> +
> +	if (nb_free > 0)
> +		rte_mempool_put_bulk(free[0]->pool, (void **)free, nb_free);
> +}
> +
>  /* 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 f2e174da1..6910b3fe6 100644
> --- a/lib/librte_mbuf/rte_mbuf.h
> +++ b/lib/librte_mbuf/rte_mbuf.h
> @@ -1908,21 +1908,15 @@ static inline void rte_pktmbuf_free(struct rte_mbuf *m)
>  }
> 
>  /**
> - * Free a bulk of mbufs back into their original mempool.
> + * Free a bulk of mbufs back into their original mempools.
>   *
>   *  @param mbufs
> - *    Array of pointers to mbufs
> + *    Array of pointers to mbufs.
> + *    The array may contain NULL pointers.
>   *  @param count
> - *    Array size
> + *    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]);
> -}
> +void rte_pktmbuf_free_bulk(struct rte_mbuf **mbufs, unsigned int count);
> 
>  /**
>   * Creates a "clone" of the given packet mbuf.
> --
> 2.17.1
Morten Brørup Sept. 26, 2019, 3:35 p.m. UTC | #7 
> -----Original Message-----
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Andrew Rybchenko
> Sent: Thursday, September 26, 2019 11:27 AM
> To: Morten Brørup; olivier.matz@6wind.com
> Cc: stephen@networkplumber.org; harry.van.haaren@intel.com;
> konstantin.ananyev@intel.com; dev@dpdk.org
> Subject: Re: [dpdk-dev] [PATCH v2 2/2] mbuf: add bulk free function
> 
> On 9/25/19 3:03 PM, Morten Brørup wrote:
> > Add function for freeing a bulk of mbufs.
> >
> > Signed-off-by: Morten Brørup <mb@smartsharesystems.com>
> > ---
> >   lib/librte_mbuf/rte_mbuf.c | 35 +++++++++++++++++++++++++++++++++++
> >   lib/librte_mbuf/rte_mbuf.h | 16 +++++-----------
> >   2 files changed, 40 insertions(+), 11 deletions(-)
> >
> > diff --git a/lib/librte_mbuf/rte_mbuf.c b/lib/librte_mbuf/rte_mbuf.c
> > index 37718d49c..b63a0eced 100644
> > --- a/lib/librte_mbuf/rte_mbuf.c
> > +++ b/lib/librte_mbuf/rte_mbuf.c
> > @@ -245,6 +245,41 @@ int rte_mbuf_check(const struct rte_mbuf *m, int
> is_header,
> >   	return 0;
> >   }
> >
> > +/**
> > + * Maximum bulk of mbufs rte_pktmbuf_free_bulk() returns to mempool.
> > + */
> > +#define RTE_PKTMBUF_FREE_BULK_SZ 64
> > +
> > +/* Free a bulk of mbufs back into their original mempools. */
> > +void rte_pktmbuf_free_bulk(struct rte_mbuf **mbufs, unsigned int count)
> > +{
> > +	struct rte_mbuf *m, *free[RTE_PKTMBUF_FREE_BULK_SZ];
> > +	unsigned int idx, nb_free = 0;
> > +
> > +	for (idx = 0; idx < count; idx++) {
> > +		m = mbufs[idx];
> > +		if (unlikely(m == NULL))
> > +			continue;
> > +
> > +		__rte_mbuf_sanity_check(m, 1);
> > +		m = rte_pktmbuf_prefree_seg(m);
> 
> Who cares about next segments if any? It looks like nobody.
> 
> > +		if (unlikely(m == NULL))
> > +			continue;
> > +
> > +		if (nb_free >= RTE_PKTMBUF_FREE_BULK_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;
> > +	}
> > +
> > +	if (nb_free > 0)
> > +		rte_mempool_put_bulk(free[0]->pool, (void **)free, nb_free);
> > +}
> > +
> >   /* 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 f2e174da1..6910b3fe6 100644
> > --- a/lib/librte_mbuf/rte_mbuf.h
> > +++ b/lib/librte_mbuf/rte_mbuf.h
> > @@ -1908,21 +1908,15 @@ static inline void rte_pktmbuf_free(struct
> rte_mbuf *m)
> >   }
> >
> >   /**
> > - * Free a bulk of mbufs back into their original mempool.
> > + * Free a bulk of mbufs back into their original mempools.
> >    *
> >    *  @param mbufs
> > - *    Array of pointers to mbufs
> > + *    Array of pointers to mbufs.
> > + *    The array may contain NULL pointers.
> >    *  @param count
> > - *    Array size
> > + *    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]);
> > -}
> > +void rte_pktmbuf_free_bulk(struct rte_mbuf **mbufs, unsigned int count);
> >
> >   /**
> >    * Creates a "clone" of the given packet mbuf.
> 
> Is it just a mistake that two patches are not squashed?
> 

Yes. Plenty of rookie git mistakes by my hand.

We don't use git in-house, and I have no git experience. So I'm trying my best, relying on the DPDK Contributor guide and git documentation. :-)


Med venlig hilsen / kind regards
- Morten Brørup
Mattias Rönnblom Sept. 26, 2019, 8:11 p.m. UTC | #8 
On 2019-09-26 10:30, Bruce Richardson wrote:
> On Wed, Sep 25, 2019 at 09:02:28PM +0200, Mattias Rönnblom wrote:
>> On 2019-09-25 14:03, Morten Brørup wrote:
>>> Add function for freeing a bulk of mbufs.
>>>
>>> Signed-off-by: Morten Brørup <mb@smartsharesystems.com>
>>> ---
>>>    lib/librte_mbuf/rte_mbuf.c | 35 +++++++++++++++++++++++++++++++++++
>>>    lib/librte_mbuf/rte_mbuf.h | 16 +++++-----------
>>>    2 files changed, 40 insertions(+), 11 deletions(-)
>>>
>>> diff --git a/lib/librte_mbuf/rte_mbuf.c b/lib/librte_mbuf/rte_mbuf.c
>>> index 37718d49c..b63a0eced 100644
>>> --- a/lib/librte_mbuf/rte_mbuf.c
>>> +++ b/lib/librte_mbuf/rte_mbuf.c
>>> @@ -245,6 +245,41 @@ int rte_mbuf_check(const struct rte_mbuf *m, int is_header,
>>>    	return 0;
>>>    }
>>> +/**
>>> + * Maximum bulk of mbufs rte_pktmbuf_free_bulk() returns to mempool.
>>> + */
>>> +#define RTE_PKTMBUF_FREE_BULK_SZ 64
>>> +
>>> +/* Free a bulk of mbufs back into their original mempools. */
>>> +void rte_pktmbuf_free_bulk(struct rte_mbuf **mbufs, unsigned int count)
>>> +{
>>> +	struct rte_mbuf *m, *free[RTE_PKTMBUF_FREE_BULK_SZ];
>>> +	unsigned int idx, nb_free = 0;
>>> +
>>> +	for (idx = 0; idx < count; idx++) {
>>> +		m = mbufs[idx];
>>> +		if (unlikely(m == NULL))
>>> +			continue;
>>> +
>>> +		__rte_mbuf_sanity_check(m, 1);
>>> +		m = rte_pktmbuf_prefree_seg(m);
>>> +		if (unlikely(m == NULL))
>>> +			continue;
>>> +
>>> +		if (nb_free >= RTE_PKTMBUF_FREE_BULK_SZ ||
>>> +		    (nb_free > 0 && m->pool != free[0]->pool)) {
>>
>> Maybe an unlikely() would be in order here?
>>
> I'd caution against it, since it can penalize the cold branch a lot. If a
> branch really is predictable the HW branch predictors generally are good
> enough to handle it at runtime. So long as a path is a valid path for a
> runtime app, i.e. not something like a fatal error only ever hit once in an
> entire run, I'd tend to omit likely()/unlikely() calls unless profiling
> shows a real performance difference.
> 

Let's see if I understand you: your worry is that wrapping that 
expression in an unlikely() will lead to code that is slower (than w/o 
the hint), if during runtime the probability turns out to be 50/50?

Wouldn't leaving out unlikely() just lead to the compiler using its 
fancy heuristics in an attempt to come to a conclusion, what path is the 
more likely?

About HW branch prediction - I'm sure it's good, but still the compiler 
needs to decided which code code path requires a branch, and which need 
not. Even if HW branch prediction successfully predicted a branch being 
taken, actually branching is going to be somewhat more expensive than to 
not branch?
Morten Brørup Sept. 27, 2019, 6:42 a.m. UTC | #9 
> -----Original Message-----
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Stephen Hemminger
> Sent: Thursday, September 26, 2019 1:37 AM
> 
> On Wed, 25 Sep 2019 14:17:42 +0200
> Morten Brørup <mb@smartsharesystems.com> wrote:
> 
> > Dear Thomas - listed as checkpatch maintainer,
> >
> > I think this warning is bogus, and is a bug checkpatch.
> >
> > The line in question was deliberately indented using tabs to the
> current indentation level, and using spaces for the readability
> alignment. This makes the code readable in editors with another tab
> setting than 8 spaces. E.g. 4 spaces is my personal preference.
> >
> >
> > Med venlig hilsen / kind regards
> > - Morten Brørup
> 
> It is understandable that you have personal preferences, but projects
> like DPDK rely
> on common style across all code. The collective decision has been made
> to keep a
> uniform style similar to the Linux kernel.
> 
> No it is not a bug in checkpatch.

Thank you for the prompt feedback, Stephen.

I thought I could get the best of both worlds with this method of indentation. But since it's against the style guide, I'll change it to comply.


Med venlig hilsen / kind regards
- Morten Brørup
Bruce Richardson Sept. 27, 2019, 9:09 a.m. UTC | #10 
On Thu, Sep 26, 2019 at 10:11:06PM +0200, Mattias Rönnblom wrote:
> On 2019-09-26 10:30, Bruce Richardson wrote:
> > On Wed, Sep 25, 2019 at 09:02:28PM +0200, Mattias Rönnblom wrote:
> > > On 2019-09-25 14:03, Morten Brørup wrote:
> > > > Add function for freeing a bulk of mbufs.
> > > > 
> > > > Signed-off-by: Morten Brørup <mb@smartsharesystems.com>
> > > > ---
> > > >    lib/librte_mbuf/rte_mbuf.c | 35 +++++++++++++++++++++++++++++++++++
> > > >    lib/librte_mbuf/rte_mbuf.h | 16 +++++-----------
> > > >    2 files changed, 40 insertions(+), 11 deletions(-)
> > > > 
> > > > diff --git a/lib/librte_mbuf/rte_mbuf.c b/lib/librte_mbuf/rte_mbuf.c
> > > > index 37718d49c..b63a0eced 100644
> > > > --- a/lib/librte_mbuf/rte_mbuf.c
> > > > +++ b/lib/librte_mbuf/rte_mbuf.c
> > > > @@ -245,6 +245,41 @@ int rte_mbuf_check(const struct rte_mbuf *m, int is_header,
> > > >    	return 0;
> > > >    }
> > > > +/**
> > > > + * Maximum bulk of mbufs rte_pktmbuf_free_bulk() returns to mempool.
> > > > + */
> > > > +#define RTE_PKTMBUF_FREE_BULK_SZ 64
> > > > +
> > > > +/* Free a bulk of mbufs back into their original mempools. */
> > > > +void rte_pktmbuf_free_bulk(struct rte_mbuf **mbufs, unsigned int count)
> > > > +{
> > > > +	struct rte_mbuf *m, *free[RTE_PKTMBUF_FREE_BULK_SZ];
> > > > +	unsigned int idx, nb_free = 0;
> > > > +
> > > > +	for (idx = 0; idx < count; idx++) {
> > > > +		m = mbufs[idx];
> > > > +		if (unlikely(m == NULL))
> > > > +			continue;
> > > > +
> > > > +		__rte_mbuf_sanity_check(m, 1);
> > > > +		m = rte_pktmbuf_prefree_seg(m);
> > > > +		if (unlikely(m == NULL))
> > > > +			continue;
> > > > +
> > > > +		if (nb_free >= RTE_PKTMBUF_FREE_BULK_SZ ||
> > > > +		    (nb_free > 0 && m->pool != free[0]->pool)) {
> > > 
> > > Maybe an unlikely() would be in order here?
> > > 
> > I'd caution against it, since it can penalize the cold branch a lot. If a
> > branch really is predictable the HW branch predictors generally are good
> > enough to handle it at runtime. So long as a path is a valid path for a
> > runtime app, i.e. not something like a fatal error only ever hit once in an
> > entire run, I'd tend to omit likely()/unlikely() calls unless profiling
> > shows a real performance difference.
> > 
> 
> Let's see if I understand you: your worry is that wrapping that expression
> in an unlikely() will lead to code that is slower (than w/o the hint), if
> during runtime the probability turns out to be 50/50?
> 
While not an expert, I believe that the use of likely/unlikely can cause the
unexpected part of the branch to be moved to a different part of the code
and potentially be more expensive to call, meaning that the performance may be
poorer even if the probability is lower than 50/50.

> Wouldn't leaving out unlikely() just lead to the compiler using its fancy
> heuristics in an attempt to come to a conclusion, what path is the more
> likely?
> 
> About HW branch prediction - I'm sure it's good, but still the compiler
> needs to decided which code code path requires a branch, and which need not.
> Even if HW branch prediction successfully predicted a branch being taken,
> actually branching is going to be somewhat more expensive than to not
> branch?

The cost difference between a taken and untaken branch should be
unnoticable so long as the branch is correctly predicted - which if does
always go one way, it will be each time each time after the first. Overall,
though, I suspect the presence of likely/unlikely is going to make any real
difference, so I'd therefore err on the side of leaving it out in the
absense of evidence that it helps in some cases.

/Bruce
Morten Brørup Sept. 27, 2019, 10:22 a.m. UTC | #11 
> -----Original Message-----
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Ananyev,
> Konstantin
> Sent: Thursday, September 26, 2019 12:23 PM
> 
> 
> Hi Morten,
> 
> >
> > Add function for freeing a bulk of mbufs.
> >
> > Signed-off-by: Morten Brørup <mb@smartsharesystems.com>
> > ---
> >  lib/librte_mbuf/rte_mbuf.c | 35 +++++++++++++++++++++++++++++++++++
> >  lib/librte_mbuf/rte_mbuf.h | 16 +++++-----------
> >  2 files changed, 40 insertions(+), 11 deletions(-)
> >
> > diff --git a/lib/librte_mbuf/rte_mbuf.c b/lib/librte_mbuf/rte_mbuf.c
> > index 37718d49c..b63a0eced 100644
> > --- a/lib/librte_mbuf/rte_mbuf.c
> > +++ b/lib/librte_mbuf/rte_mbuf.c
> > @@ -245,6 +245,41 @@ int rte_mbuf_check(const struct rte_mbuf *m, int
> is_header,
> >  	return 0;
> >  }
> >
> > +/**
> > + * Maximum bulk of mbufs rte_pktmbuf_free_bulk() returns to mempool.
> > + */
> > +#define RTE_PKTMBUF_FREE_BULK_SZ 64
> > +
> > +/* Free a bulk of mbufs back into their original mempools. */
> > +void rte_pktmbuf_free_bulk(struct rte_mbuf **mbufs, unsigned int
> count)
> 
> As I can see you forgot to handle situation with multi-segs packet.
> This one is still I a good one to have, I think.
> But probably it should be named rte_pktmbuf_free_seg_bulk()
> to avoid any confusion.
> Konstantin
> 

Thanks for spotting this bug, Konstantin! I have submitted an updated patch.

I get your point about having two separate functions, and as you can see from my patch, they turned out quite different.

However, am not sure where the rte_pktmbuf_free_seg_bulk() would be used. And I don't think we should add functions to the mbuf library unless we have at least one viable use case.

E.g. refer the ixgbe driver that kicked off the modification to use rte_mempool_put_bulk() instead of a simple loop around rte_pktmbuf_free(). The driver is not using an array of mbufs; it is using an array of its own structure, with one the fields pointing to an mbuf.


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..b63a0eced 100644
--- a/lib/librte_mbuf/rte_mbuf.c
+++ b/lib/librte_mbuf/rte_mbuf.c
@@ -245,6 +245,41 @@  int rte_mbuf_check(const struct rte_mbuf *m, int is_header,
 	return 0;
 }
 
+/**
+ * Maximum bulk of mbufs rte_pktmbuf_free_bulk() returns to mempool.
+ */
+#define RTE_PKTMBUF_FREE_BULK_SZ 64
+
+/* Free a bulk of mbufs back into their original mempools. */
+void rte_pktmbuf_free_bulk(struct rte_mbuf **mbufs, unsigned int count)
+{
+	struct rte_mbuf *m, *free[RTE_PKTMBUF_FREE_BULK_SZ];
+	unsigned int idx, nb_free = 0;
+
+	for (idx = 0; idx < count; idx++) {
+		m = mbufs[idx];
+		if (unlikely(m == NULL))
+			continue;
+
+		__rte_mbuf_sanity_check(m, 1);
+		m = rte_pktmbuf_prefree_seg(m);
+		if (unlikely(m == NULL))
+			continue;
+
+		if (nb_free >= RTE_PKTMBUF_FREE_BULK_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;
+	}
+
+	if (nb_free > 0)
+		rte_mempool_put_bulk(free[0]->pool, (void **)free, nb_free);
+}
+
 /* 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 f2e174da1..6910b3fe6 100644
--- a/lib/librte_mbuf/rte_mbuf.h
+++ b/lib/librte_mbuf/rte_mbuf.h
@@ -1908,21 +1908,15 @@  static inline void rte_pktmbuf_free(struct rte_mbuf *m)
 }
 
 /**
- * Free a bulk of mbufs back into their original mempool.
+ * Free a bulk of mbufs back into their original mempools.
  *
  *  @param mbufs
- *    Array of pointers to mbufs
+ *    Array of pointers to mbufs.
+ *    The array may contain NULL pointers.
  *  @param count
- *    Array size
+ *    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]);
-}
+void rte_pktmbuf_free_bulk(struct rte_mbuf **mbufs, unsigned int count);
 
 /**
  * Creates a "clone" of the given packet mbuf.