From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-wm0-f53.google.com (mail-wm0-f53.google.com [74.125.82.53]) by dpdk.org (Postfix) with ESMTP id 35A64ADC8 for ; Wed, 8 Jun 2016 14:27:26 +0200 (CEST) Received: by mail-wm0-f53.google.com with SMTP id m124so14642564wme.1 for ; Wed, 08 Jun 2016 05:27:26 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=6wind-com.20150623.gappssmtp.com; s=20150623; h=date:from:to:cc:subject:message-id:mail-followup-to:references :mime-version:content-disposition:in-reply-to; bh=+A7kD1eXOl3CXYJHhl3Tem4wyaD8c2wuWRd2NAwiKU8=; b=X3X0nPuYCZiV8BTGjZDdv3HyX2fuV6jr1MSI0LBzs5i6k/NVjlgAxast+lfzRxgH/Q vOs+amRVTemgypkOl30KNvoV+RadOBsVr0X2ozXo78FGmhJOgwlZm57Sc7Pr1Rnn5SEB 9nLGczrZsWB3B9vmFNxwebJIQ6YxbxRrz3SyZwFnyXg9mrEOrOK/YEEFwL/Xctgf0AXJ Kbn5rKaOzvnL8pQSQ31yhffClIsmQLHWqtW46kxRfQWAXAjTu74JvVujiy5waWS2Al8+ AVvvBOpWYG7SWCCXVqSOGYJFqIYvID/yO2OCy6Ksx2BwMaU7gg3b8cC3P0Vb8/sRGJ4t wlwg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:date:from:to:cc:subject:message-id :mail-followup-to:references:mime-version:content-disposition :in-reply-to; bh=+A7kD1eXOl3CXYJHhl3Tem4wyaD8c2wuWRd2NAwiKU8=; b=au9Bku4t3jXwY4VAABvY8YWVXCLnv+ogkR9s+r4Ug4WxIvGam+tVw9r1Ee22C7Fwq0 LwqGgCj4gX+Htr/h5PECOMfUX9A0oldC8BzUzwK+yyM7h7F5Tgj5c9R+o63Fb1HL2YAX 0vAH4r3vbXgW4zCI7eIPHEPkx+KiKrGiX9FGIXj/DgIQ/ibC04R7EooHoWTf74f4r9Q6 e2J8rw6+r9qxsD/YPipCaWco4vvaohhuJSlkpuZP/+4MoSz0xiU5tI2D/luT4pCtK1U5 Hu6OeqMm2HYPlNRtRutqjrvoCDtvoKD1EHvHC8H/sky5kbjnEuEs2SBBrwLj6DH+B0H2 QviQ== X-Gm-Message-State: ALyK8tKumuPrIbzLPQqZxuMGuEUnnD/u6oFOJVSVN9jyibzUCrp7AoiuikGQvW0oNQQnZeoB X-Received: by 10.28.157.23 with SMTP id g23mr7959298wme.34.1465388845955; Wed, 08 Jun 2016 05:27:25 -0700 (PDT) Received: from 6wind.com (guy78-3-82-239-227-177.fbx.proxad.net. [82.239.227.177]) by smtp.gmail.com with ESMTPSA id zg2sm1302864wjb.1.2016.06.08.05.27.24 (version=TLSv1/SSLv3 cipher=OTHER); Wed, 08 Jun 2016 05:27:25 -0700 (PDT) Date: Wed, 8 Jun 2016 14:27:23 +0200 From: Adrien Mazarguil To: "Ananyev, Konstantin" Cc: "dev@dpdk.org" , Olivier Matz Message-ID: <20160608122723.GK7621@6wind.com> Mail-Followup-To: "Ananyev, Konstantin" , "dev@dpdk.org" , Olivier Matz References: <1465374688-11729-1-git-send-email-adrien.mazarguil@6wind.com> <2601191342CEEE43887BDE71AB97725836B6CE18@irsmsx105.ger.corp.intel.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <2601191342CEEE43887BDE71AB97725836B6CE18@irsmsx105.ger.corp.intel.com> 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 12:27:26 -0000 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 > > --- > > 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