From: Adrien Mazarguil <adrien.mazarguil@6wind.com>
To: "Ananyev, Konstantin" <konstantin.ananyev@intel.com>
Cc: "dev@dpdk.org" <dev@dpdk.org>, Olivier Matz <olivier.matz@6wind.com>
Subject: Re: [dpdk-dev] [PATCH] mbuf: remove inconsistent assert statements
Date: Wed, 8 Jun 2016 14:27:23 +0200 [thread overview]
Message-ID: <20160608122723.GK7621@6wind.com> (raw)
In-Reply-To: <2601191342CEEE43887BDE71AB97725836B6CE18@irsmsx105.ger.corp.intel.com>
Hi Konstantin,
On Wed, Jun 08, 2016 at 10:34:17AM +0000, Ananyev, Konstantin wrote:
> Hi Adrien,
>
> >
> > An assertion failure occurs in __rte_mbuf_raw_free() (called by a few PMDs)
> > when compiling DPDK with CONFIG_RTE_LOG_LEVEL=RTE_LOG_DEBUG and starting
> > applications with a log level high enough to trigger it.
> >
> > While rte_mbuf_raw_alloc() sets refcount to 1, __rte_mbuf_raw_free()
> > expects it to be 0.
> >Considering users are not expected to reset the
> > reference count to satisfy assert() and that raw functions are designed on
> > purpose without safety belts, remove these checks.
>
> Yes, it refcnt supposed to be set to 0 by __rte_pktmbuf_prefree_seg().
> Wright now, it is a user responsibility to make sure refcnt==0 before pushing
> mbuf back to the pool.
> Not sure why do you consider that wrong?
I do not consider this wrong and I'm all for using assert() to catch
programming errors, however in this specific case, I think they are
inconsistent and misleading.
> If the user calls __rte_mbuf_raw_free() manualy it is his responsibility to make
> sure mbuf's refcn==0.
Sure, however what harm does it cause (besides assert() to fail), since the
allocation function sets refcount to 1?
Why having the allocation function set the refcount if we are sure it is
already 0 (assert() proves it). Removing rte_mbuf_refcnt_set(m, 1) can
surely improve performance.
> BTW, why are you doing it?
> The comment clearly states that the function is for internal use:
> /**
> * @internal Put mbuf back into its original mempool.
> * The use of that function is reserved for RTE internal needs.
> * Please use rte_pktmbuf_free().
> *
> * @param m
> * The mbuf to be freed.
> */
> static inline void __attribute__((always_inline))
> __rte_mbuf_raw_free(struct rte_mbuf *m)
Several PMDs are using it anyway (won't name names, but I know one of them
quite well). I chose to modify this code instead of its users for the
following reasons:
- Considering their names, these functions should be opposites and able to
work together like malloc()/free().
- PMDs are relying on these functions for performance reasons, we can assume
they took the extra care necessary to make sure it would work properly.
- Preventing it would make these PMDs slower and is not acceptable either.
What remains is the consistency issue, I think these statements were only
added to catch multiple frees, and those should be caught at a higher
level, where other consistency checks are also performed.
> > Signed-off-by: Adrien Mazarguil <adrien.mazarguil@6wind.com>
> > ---
> > lib/librte_mbuf/rte_mbuf.h | 2 --
> > 1 file changed, 2 deletions(-)
> >
> > diff --git a/lib/librte_mbuf/rte_mbuf.h b/lib/librte_mbuf/rte_mbuf.h
> > index 11fa06d..7070bb8 100644
> > --- a/lib/librte_mbuf/rte_mbuf.h
> > +++ b/lib/librte_mbuf/rte_mbuf.h
> > @@ -1108,7 +1108,6 @@ static inline struct rte_mbuf *rte_mbuf_raw_alloc(struct rte_mempool *mp)
> > if (rte_mempool_get(mp, &mb) < 0)
> > return NULL;
> > m = (struct rte_mbuf *)mb;
> > - RTE_ASSERT(rte_mbuf_refcnt_read(m) == 0);
> > rte_mbuf_refcnt_set(m, 1);
> > __rte_mbuf_sanity_check(m, 0);
> >
> > @@ -1133,7 +1132,6 @@ __rte_mbuf_raw_alloc(struct rte_mempool *mp)
> > static inline void __attribute__((always_inline))
> > __rte_mbuf_raw_free(struct rte_mbuf *m)
> > {
> > - RTE_ASSERT(rte_mbuf_refcnt_read(m) == 0);
> > rte_mempool_put(m->pool, m);
> > }
> >
> > --
> > 2.1.4
>
--
Adrien Mazarguil
6WIND
next prev parent reply other threads:[~2016-06-08 12:27 UTC|newest]
Thread overview: 14+ messages / expand[flat|nested] mbox.gz Atom feed top
2016-06-08 8:31 Adrien Mazarguil
2016-06-08 10:34 ` Ananyev, Konstantin
2016-06-08 12:27 ` Adrien Mazarguil [this message]
2016-06-08 13:09 ` Ananyev, Konstantin
2016-06-08 13:57 ` Adrien Mazarguil
2016-06-08 14:11 ` Olivier Matz
2016-06-08 16:07 ` Ananyev, Konstantin
2016-06-09 7:46 ` Olivier Matz
2016-06-09 13:21 ` Ananyev, Konstantin
2016-06-09 14:19 ` Bruce Richardson
2016-06-09 15:27 ` Thomas Monjalon
2016-06-09 15:45 ` Ananyev, Konstantin
2016-06-09 18:42 ` Don Provan
2016-06-20 13:49 ` Adrien Mazarguil
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20160608122723.GK7621@6wind.com \
--to=adrien.mazarguil@6wind.com \
--cc=dev@dpdk.org \
--cc=konstantin.ananyev@intel.com \
--cc=olivier.matz@6wind.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
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).