From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail.droids-corp.org (zoll.droids-corp.org [94.23.50.67]) by dpdk.org (Postfix) with ESMTP id 240EC235 for ; Wed, 20 Sep 2017 13:23:14 +0200 (CEST) Received: from lfbn-lil-1-182-75.w90-45.abo.wanadoo.fr ([90.45.31.75] helo=droids-corp.org) by mail.droids-corp.org with esmtpsa (TLS1.0:RSA_AES_256_CBC_SHA1:256) (Exim 4.84_2) (envelope-from ) id 1dudBV-0006rd-9N; Wed, 20 Sep 2017 13:28:55 +0200 Received: by droids-corp.org (sSMTP sendmail emulation); Wed, 20 Sep 2017 13:23:05 +0200 Date: Wed, 20 Sep 2017 13:23:05 +0200 From: Olivier MATZ To: "Charles (Chas) Williams" Cc: dev@dpdk.org Message-ID: <20170920112304.4sitlue2g2kbmndh@platinum> References: <1502122274-15657-1-git-send-email-ciwillia@brocade.com> <1504819311-8441-1-git-send-email-ciwillia@brocade.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1504819311-8441-1-git-send-email-ciwillia@brocade.com> User-Agent: NeoMutt/20170113 (1.7.2) Subject: Re: [dpdk-dev] [PATCH v3] mbuf: use refcnt = 0 when debugging X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 20 Sep 2017 11:23:14 -0000 Hi Chas, On Thu, Sep 07, 2017 at 05:21:51PM -0400, Charles (Chas) Williams wrote: > After commit 8f094a9ac5d7 ("mbuf: set mbuf fields while in pool") is it > much harder to detect a "double free". If the developer makes a copy > of an mbuf pointer and frees it twice, this condition is never detected > and the mbuf gets returned to the pool twice. > > Since this requires extra work to track, make this behavior conditional > on CONFIG_RTE_LIBRTE_MBUF_DEBUG. > > Signed-off-by: Chas Williams > --- > lib/librte_mbuf/rte_mbuf.c | 2 +- > lib/librte_mbuf/rte_mbuf.h | 40 ++++++++++++++++++++++++++++++++++------ > 2 files changed, 35 insertions(+), 7 deletions(-) > > diff --git a/lib/librte_mbuf/rte_mbuf.c b/lib/librte_mbuf/rte_mbuf.c > index 26a62b8..b0d222c 100644 > --- a/lib/librte_mbuf/rte_mbuf.c > +++ b/lib/librte_mbuf/rte_mbuf.c > @@ -145,7 +145,7 @@ rte_pktmbuf_init(struct rte_mempool *mp, > m->pool = mp; > m->nb_segs = 1; > m->port = 0xff; > - rte_mbuf_refcnt_set(m, 1); > + rte_mbuf_refcnt_set(m, RTE_MBUF_UNUSED_CNT); > m->next = NULL; > } > > diff --git a/lib/librte_mbuf/rte_mbuf.h b/lib/librte_mbuf/rte_mbuf.h > index eaed7ee..1400b35 100644 > --- a/lib/librte_mbuf/rte_mbuf.h > +++ b/lib/librte_mbuf/rte_mbuf.h > @@ -671,11 +671,15 @@ struct rte_pktmbuf_pool_private { > > #ifdef RTE_LIBRTE_MBUF_DEBUG > > +#define RTE_MBUF_UNUSED_CNT 0 > + > /** check mbuf type in debug mode */ > #define __rte_mbuf_sanity_check(m, is_h) rte_mbuf_sanity_check(m, is_h) > > #else /* RTE_LIBRTE_MBUF_DEBUG */ > > +#define RTE_MBUF_UNUSED_CNT 1 > + > /** check mbuf type in debug mode */ > #define __rte_mbuf_sanity_check(m, is_h) do { } while (0) > > @@ -721,6 +725,9 @@ rte_mbuf_refcnt_set(struct rte_mbuf *m, uint16_t new_value) > static inline uint16_t > rte_mbuf_refcnt_update(struct rte_mbuf *m, int16_t value) > { > +#ifdef RTE_LIBRTE_MBUF_DEBUG > + RTE_ASSERT(rte_mbuf_refcnt_read(m) != 0); > +#endif > /* > * The atomic_add is an expensive operation, so we don't want to > * call it in the case where we know we are the uniq holder of > @@ -791,10 +798,9 @@ void > rte_mbuf_sanity_check(const struct rte_mbuf *m, int is_header); > > #define MBUF_RAW_ALLOC_CHECK(m) do { \ > - RTE_ASSERT(rte_mbuf_refcnt_read(m) == 1); \ > + RTE_ASSERT(rte_mbuf_refcnt_read(m) == RTE_MBUF_UNUSED_CNT); \ > RTE_ASSERT((m)->next == NULL); \ > RTE_ASSERT((m)->nb_segs == 1); \ > - __rte_mbuf_sanity_check(m, 0); \ > } while (0) > > /** > @@ -825,6 +831,10 @@ static inline struct rte_mbuf *rte_mbuf_raw_alloc(struct rte_mempool *mp) > return NULL; > m = (struct rte_mbuf *)mb; > MBUF_RAW_ALLOC_CHECK(m); > +#ifdef RTE_LIBRTE_MBUF_DEBUG > + rte_mbuf_refcnt_set(m, 1); > +#endif > + __rte_mbuf_sanity_check(m, 0); > return m; > } > > @@ -846,10 +856,9 @@ static __rte_always_inline void > rte_mbuf_raw_free(struct rte_mbuf *m) > { > RTE_ASSERT(RTE_MBUF_DIRECT(m)); > - RTE_ASSERT(rte_mbuf_refcnt_read(m) == 1); > + RTE_ASSERT(rte_mbuf_refcnt_read(m) == RTE_MBUF_UNUSED_CNT); > RTE_ASSERT(m->next == NULL); > RTE_ASSERT(m->nb_segs == 1); > - __rte_mbuf_sanity_check(m, 0); > rte_mempool_put(m->pool, m); > } > > @@ -1159,21 +1168,37 @@ static inline int rte_pktmbuf_alloc_bulk(struct rte_mempool *pool, > case 0: > while (idx != count) { > MBUF_RAW_ALLOC_CHECK(mbufs[idx]); > +#ifdef RTE_LIBRTE_MBUF_DEBUG > + rte_mbuf_refcnt_set(mbufs[idx], 1); > +#endif > + __rte_mbuf_sanity_check(mbufs[idx], 0); > rte_pktmbuf_reset(mbufs[idx]); > idx++; > /* fall-through */ > case 3: > MBUF_RAW_ALLOC_CHECK(mbufs[idx]); > +#ifdef RTE_LIBRTE_MBUF_DEBUG > + rte_mbuf_refcnt_set(mbufs[idx], 1); > +#endif > + __rte_mbuf_sanity_check(mbufs[idx], 0); > rte_pktmbuf_reset(mbufs[idx]); > idx++; > /* fall-through */ > case 2: > MBUF_RAW_ALLOC_CHECK(mbufs[idx]); > +#ifdef RTE_LIBRTE_MBUF_DEBUG > + rte_mbuf_refcnt_set(mbufs[idx], 1); > +#endif > + __rte_mbuf_sanity_check(mbufs[idx], 0); > rte_pktmbuf_reset(mbufs[idx]); > idx++; > /* fall-through */ > case 1: > MBUF_RAW_ALLOC_CHECK(mbufs[idx]); > +#ifdef RTE_LIBRTE_MBUF_DEBUG > + rte_mbuf_refcnt_set(mbufs[idx], 1); > +#endif > + __rte_mbuf_sanity_check(mbufs[idx], 0); > rte_pktmbuf_reset(mbufs[idx]); > idx++; > /* fall-through */ > @@ -1271,7 +1296,7 @@ static inline void rte_pktmbuf_detach(struct rte_mbuf *m) > if (rte_mbuf_refcnt_update(md, -1) == 0) { > md->next = NULL; > md->nb_segs = 1; > - rte_mbuf_refcnt_set(md, 1); > + rte_mbuf_refcnt_set(md, RTE_MBUF_UNUSED_CNT); > rte_mbuf_raw_free(md); > } > } > @@ -1304,6 +1329,9 @@ rte_pktmbuf_prefree_seg(struct rte_mbuf *m) > m->next = NULL; > m->nb_segs = 1; > } > +#ifdef RTE_LIBRTE_MBUF_DEBUG > + rte_mbuf_refcnt_set(m, RTE_MBUF_UNUSED_CNT); > +#endif > > return m; > > @@ -1317,7 +1345,7 @@ rte_pktmbuf_prefree_seg(struct rte_mbuf *m) > m->next = NULL; > m->nb_segs = 1; > } > - rte_mbuf_refcnt_set(m, 1); > + rte_mbuf_refcnt_set(m, RTE_MBUF_UNUSED_CNT); > > return m; > } > -- > 2.1.4 > I'm afraid this won't work for drivers that use rte_mempool_get() instead of rte_mbuf_raw_alloc(), because they will expect that refcount is 1. I didn't try, but an alternative to detect these double-frees could be to enable RTE_LIBRTE_MEMPOOL_DEBUG. What do you think? Olivier