From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga11.intel.com (mga11.intel.com [192.55.52.93]) by dpdk.org (Postfix) with ESMTP id 34154ADE0 for ; Wed, 8 Jun 2016 15:10:56 +0200 (CEST) Received: from fmsmga004.fm.intel.com ([10.253.24.48]) by fmsmga102.fm.intel.com with ESMTP; 08 Jun 2016 06:09:21 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.26,439,1459839600"; d="scan'208";a="118261859" Received: from irsmsx104.ger.corp.intel.com ([163.33.3.159]) by fmsmga004.fm.intel.com with ESMTP; 08 Jun 2016 06:09:20 -0700 Received: from irsmsx105.ger.corp.intel.com ([169.254.7.51]) by IRSMSX104.ger.corp.intel.com ([163.33.3.159]) with mapi id 14.03.0248.002; Wed, 8 Jun 2016 14:09:19 +0100 From: "Ananyev, Konstantin" To: Adrien Mazarguil CC: "dev@dpdk.org" , Olivier Matz Thread-Topic: [dpdk-dev] [PATCH] mbuf: remove inconsistent assert statements Thread-Index: AQHRwWBHlYh+pJQko0aMG2e1V0BGOZ/fXbUQgAAQ/oCAABYKAA== Date: Wed, 8 Jun 2016 13:09:18 +0000 Message-ID: <2601191342CEEE43887BDE71AB97725836B6CFDD@irsmsx105.ger.corp.intel.com> References: <1465374688-11729-1-git-send-email-adrien.mazarguil@6wind.com> <2601191342CEEE43887BDE71AB97725836B6CE18@irsmsx105.ger.corp.intel.com> <20160608122723.GK7621@6wind.com> In-Reply-To: <20160608122723.GK7621@6wind.com> Accept-Language: en-IE, en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-originating-ip: [163.33.239.181] Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 Subject: Re: [dpdk-dev] [PATCH] mbuf: remove inconsistent assert statements X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: patches and discussions about DPDK List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 08 Jun 2016 13:10:56 -0000 >=20 > Hi Konstantin, >=20 > 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=3DRTE_LOG_DEBUG and sta= rting > > > 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 design= ed 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=3D=3D0 befo= re pushing > > mbuf back to the pool. > > Not sure why do you consider that wrong? >=20 > 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. Honestly, I don't understand why. Right now the rule of thumb is - when mbuf is in the pool, it's refcnt shou= ld be equal zero. Yes, as you pointed below - that rule probably can be changed to:=20 when mbuf is in the pool, it's refcnt should equal one, and that would prob= ably allow us to speedup things a bit, but I suppose that's the matter of another aptch/d= iscussion. >=20 > > If the user calls __rte_mbuf_raw_free() manualy it is his responsibilit= y to make > > sure mbuf's refcn=3D=3D0. >=20 > Sure, however what harm does it cause (besides assert() to fail), since t= he > allocation function sets refcount to 1? >=20 > 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. That's' just an assert() enabled when MBUF_DEBUG is on. Its sole purpose is to help troubleshoot the bugs and help to catch situati= ons when someone silently updates mbufs supposed to be free. =20 >=20 > > 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) >=20 > Several PMDs are using it anyway (won't name names, but I know one of the= m > quite well). Then it probably is a bug in these PMDs that need to be fixed. > I chose to modify this code instead of its users for the > following reasons: >=20 > - Considering their names, these functions should be opposites and able t= o > work together like malloc()/free(). These are internal functions. Comments in mbuf clearly state that library users shouldn't call them direc= tly. They are written to fit internal librte_mbuf needs, and no-one ever promise= d malloc/free() compatibility here.=20 >=20 > - PMDs are relying on these functions for performance reasons, we can ass= ume > they took the extra care necessary to make sure it would work properly. That just doesn't seem correct to me. The proper way to do free fo mbuf segment is: static inline void __attribute__((always_inline)) rte_pktmbuf_free_seg(struct rte_mbuf *m) { if (likely(NULL !=3D (m =3D __rte_pktmbuf_prefree_seg(m)))) { m->next =3D NULL; __rte_mbuf_raw_free(m); } } If by some reason you choose not to use this function, then it is your responsibility to perform similar actions on your own before pushing mbuf i= nto the pool. That's what some TX functions for some Intel NICs do to improve performance= : they call _prefree_seg() manually and try to put mbufs into the pool in gro= ups. >=20 > - Preventing it would make these PMDs slower and is not acceptable either= . I can hardly imagine that __rte_pktmbuf_prefree_seg() impact would be that = severe... But ok, probably you do have some very specific case, but then why you PMD= just doesn't call: rte_mempool_put(m->pool, m);=20 directly? Why instead you choose to change common functions and compromise librte_mbuf debug ability? >=20 > What remains is the consistency issue, I think these statements were only > added to catch multiple frees, Yes these asserts() here to help catch bugs, and I think it is a good thing to have them here.=20 > and those should be caught at a higher > level, where other consistency checks are also performed. Like where? Konstantin >=20 > > > Signed-off-by: Adrien Mazarguil > > > --- > > > 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_all= oc(struct rte_mempool *mp) > > > if (rte_mempool_get(mp, &mb) < 0) > > > return NULL; > > > m =3D (struct rte_mbuf *)mb; > > > - RTE_ASSERT(rte_mbuf_refcnt_read(m) =3D=3D 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) =3D=3D 0); > > > rte_mempool_put(m->pool, m); > > > } > > > > > > -- > > > 2.1.4 > > >=20 > -- > Adrien Mazarguil > 6WIND