[parent not found: <20190925120542.A51B41BE84@dpdk.org>]
* Re: [dpdk-dev] |WARNING| pw59738 [PATCH v2 2/2] mbuf: add bulk free function
[not found] ` <20190925120542.A51B41BE84@dpdk.org>
@ 2019-09-25 12:17 ` Morten Brørup
2019-09-25 23:37 ` Stephen Hemminger
0 siblings, 1 reply; 14+ messages in thread
From: Morten Brørup @ 2019-09-25 12:17 UTC (permalink / raw)
To: Thomas Monjalon; +Cc: checkpatch, test-report, dpdk-dev
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
^ permalink raw reply [flat|nested] 14+ messages in thread* Re: [dpdk-dev] |WARNING| pw59738 [PATCH v2 2/2] mbuf: add bulk free function
2019-09-25 12:17 ` [dpdk-dev] |WARNING| pw59738 " Morten Brørup
@ 2019-09-25 23:37 ` Stephen Hemminger
2019-09-27 6:42 ` Morten Brørup
0 siblings, 1 reply; 14+ messages in thread
From: Stephen Hemminger @ 2019-09-25 23:37 UTC (permalink / raw)
To: Morten Brørup; +Cc: Thomas Monjalon, checkpatch, test-report, dpdk-dev
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.
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [dpdk-dev] |WARNING| pw59738 [PATCH v2 2/2] mbuf: add bulk free function
2019-09-25 23:37 ` Stephen Hemminger
@ 2019-09-27 6:42 ` Morten Brørup
0 siblings, 0 replies; 14+ messages in thread
From: Morten Brørup @ 2019-09-27 6:42 UTC (permalink / raw)
To: Stephen Hemminger, Thomas Monjalon; +Cc: checkpatch, test-report, dpdk-dev
> -----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
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [dpdk-dev] [PATCH v2 2/2] mbuf: add bulk free function
2019-09-25 12:03 ` [dpdk-dev] [PATCH v2 2/2] " Morten Brørup
[not found] ` <20190925120542.A51B41BE84@dpdk.org>
@ 2019-09-25 19:02 ` Mattias Rönnblom
2019-09-26 8:30 ` Bruce Richardson
2019-09-26 9:26 ` Andrew Rybchenko
2019-09-26 10:23 ` Ananyev, Konstantin
3 siblings, 1 reply; 14+ messages in thread
From: Mattias Rönnblom @ 2019-09-25 19:02 UTC (permalink / raw)
To: Morten Brørup, olivier.matz
Cc: stephen, harry.van.haaren, konstantin.ananyev, dev
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.
>
^ permalink raw reply [flat|nested] 14+ messages in thread* Re: [dpdk-dev] [PATCH v2 2/2] mbuf: add bulk free function
2019-09-25 19:02 ` [dpdk-dev] " Mattias Rönnblom
@ 2019-09-26 8:30 ` Bruce Richardson
2019-09-26 20:11 ` Mattias Rönnblom
0 siblings, 1 reply; 14+ messages in thread
From: Bruce Richardson @ 2019-09-26 8:30 UTC (permalink / raw)
To: Mattias Rönnblom
Cc: Morten Brørup, olivier.matz, stephen, harry.van.haaren,
konstantin.ananyev, dev
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
^ permalink raw reply [flat|nested] 14+ messages in thread* Re: [dpdk-dev] [PATCH v2 2/2] mbuf: add bulk free function
2019-09-26 8:30 ` Bruce Richardson
@ 2019-09-26 20:11 ` Mattias Rönnblom
2019-09-27 9:09 ` Bruce Richardson
0 siblings, 1 reply; 14+ messages in thread
From: Mattias Rönnblom @ 2019-09-26 20:11 UTC (permalink / raw)
To: Bruce Richardson
Cc: Morten Brørup, olivier.matz, stephen, harry.van.haaren,
konstantin.ananyev, dev
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?
^ permalink raw reply [flat|nested] 14+ messages in thread* Re: [dpdk-dev] [PATCH v2 2/2] mbuf: add bulk free function
2019-09-26 20:11 ` Mattias Rönnblom
@ 2019-09-27 9:09 ` Bruce Richardson
0 siblings, 0 replies; 14+ messages in thread
From: Bruce Richardson @ 2019-09-27 9:09 UTC (permalink / raw)
To: Mattias Rönnblom
Cc: Morten Brørup, olivier.matz, stephen, harry.van.haaren,
konstantin.ananyev, dev
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
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [dpdk-dev] [PATCH v2 2/2] mbuf: add bulk free function
2019-09-25 12:03 ` [dpdk-dev] [PATCH v2 2/2] " Morten Brørup
[not found] ` <20190925120542.A51B41BE84@dpdk.org>
2019-09-25 19:02 ` [dpdk-dev] " Mattias Rönnblom
@ 2019-09-26 9:26 ` Andrew Rybchenko
2019-09-26 15:35 ` Morten Brørup
2019-09-26 10:23 ` Ananyev, Konstantin
3 siblings, 1 reply; 14+ messages in thread
From: Andrew Rybchenko @ 2019-09-26 9:26 UTC (permalink / raw)
To: Morten Brørup, olivier.matz
Cc: stephen, harry.van.haaren, konstantin.ananyev, dev
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?
^ permalink raw reply [flat|nested] 14+ messages in thread* Re: [dpdk-dev] [PATCH v2 2/2] mbuf: add bulk free function
2019-09-26 9:26 ` Andrew Rybchenko
@ 2019-09-26 15:35 ` Morten Brørup
0 siblings, 0 replies; 14+ messages in thread
From: Morten Brørup @ 2019-09-26 15:35 UTC (permalink / raw)
To: Andrew Rybchenko, olivier.matz
Cc: stephen, harry.van.haaren, konstantin.ananyev, dev
> -----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
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [dpdk-dev] [PATCH v2 2/2] mbuf: add bulk free function
2019-09-25 12:03 ` [dpdk-dev] [PATCH v2 2/2] " Morten Brørup
` (2 preceding siblings ...)
2019-09-26 9:26 ` Andrew Rybchenko
@ 2019-09-26 10:23 ` Ananyev, Konstantin
2019-09-27 10:22 ` Morten Brørup
3 siblings, 1 reply; 14+ messages in thread
From: Ananyev, Konstantin @ 2019-09-26 10:23 UTC (permalink / raw)
To: Morten Brørup, olivier.matz; +Cc: stephen, Van Haaren, Harry, dev
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
^ permalink raw reply [flat|nested] 14+ messages in thread* Re: [dpdk-dev] [PATCH v2 2/2] mbuf: add bulk free function
2019-09-26 10:23 ` Ananyev, Konstantin
@ 2019-09-27 10:22 ` Morten Brørup
0 siblings, 0 replies; 14+ messages in thread
From: Morten Brørup @ 2019-09-27 10:22 UTC (permalink / raw)
To: Ananyev, Konstantin, olivier.matz; +Cc: stephen, Van Haaren, Harry, dev
> -----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
^ permalink raw reply [flat|nested] 14+ messages in thread