From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp.tuxdriver.com (charlotte.tuxdriver.com [70.61.120.58]) by dpdk.org (Postfix) with ESMTP id 93F393B5 for ; Fri, 27 Mar 2015 11:25:43 +0100 (CET) Received: from hmsreliant.think-freely.org ([2001:470:8:a08:7aac:c0ff:fec2:933b] helo=localhost) by smtp.tuxdriver.com with esmtpsa (TLSv1:AES128-SHA:128) (Exim 4.63) (envelope-from ) id 1YbRSI-0003jC-Et; Fri, 27 Mar 2015 06:25:40 -0400 Date: Fri, 27 Mar 2015 06:25:33 -0400 From: Neil Horman To: "Wiles, Keith" Message-ID: <20150327102533.GA5375@hmsreliant.think-freely.org> References: <1427393457-7080-1-git-send-email-zoltan.kiss@linaro.org> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: User-Agent: Mutt/1.5.23 (2014-03-12) X-Spam-Score: -2.9 (--) X-Spam-Status: No Cc: "dev@dpdk.org" Subject: Re: [dpdk-dev] [PATCH] mbuf: optimize refcnt handling during free 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: Fri, 27 Mar 2015 10:25:44 -0000 On Thu, Mar 26, 2015 at 09:00:33PM +0000, Wiles, Keith wrote: > > > On 3/26/15, 1:10 PM, "Zoltan Kiss" wrote: > > >The current way is not the most efficient: if m->refcnt is 1, the second > >condition never evaluates, and we set it to 0. If refcnt > 1, the 2nd > >condition fails again, although the code suggest otherwise to branch > >prediction. Instead we should keep the second condition only, and remove > >the > >duplicate set to zero. > > > >Signed-off-by: Zoltan Kiss > >--- > > lib/librte_mbuf/rte_mbuf.h | 5 +---- > > 1 file changed, 1 insertion(+), 4 deletions(-) > > > >diff --git a/lib/librte_mbuf/rte_mbuf.h b/lib/librte_mbuf/rte_mbuf.h > >index 17ba791..3ec4024 100644 > >--- a/lib/librte_mbuf/rte_mbuf.h > >+++ b/lib/librte_mbuf/rte_mbuf.h > >@@ -764,10 +764,7 @@ __rte_pktmbuf_prefree_seg(struct rte_mbuf *m) > > { > > __rte_mbuf_sanity_check(m, 0); > > > >- if (likely (rte_mbuf_refcnt_read(m) == 1) || > >- likely (rte_mbuf_refcnt_update(m, -1) == 0)) { > >- > >- rte_mbuf_refcnt_set(m, 0); > >+ if (likely (rte_mbuf_refcnt_update(m, -1) == 0)) { > > > > /* if this is an indirect mbuf, then > > * - detach mbuf > > I fell for this one too, but read Brucešs email > http://dpdk.org/ml/archives/dev/2015-March/014481.html This is still the right thing to do though, Bruce's reasoning is erroneous. Just because the return from rte_mbuf_refcnt_read returns 1, doesn't mean you are the last user of the mbuf, you are only guaranteed that if the update operation returns zero. In other words: rte_mbuf_refcnt_update(m, -1) is an atomic operation if (likely (rte_mbuf_refcnt_read(m) == 1) || likely (rte_mbuf_refcnt_update(m, -1) == 0)) { is not. To illustrate, on two cpus, this might occur: CPU0 CPU1 rte_mbuf_refcnt_read ... returns 1 rte_mbuf_refcnt_read ... returns 1 execute if clause execute if clause In the above scenario both cpus fell into the if clause because they both held a pointer to the same buffer and both got a return value of one, so they skipped the update portion of the if clause and both executed the internal block of the conditional expression. you might be tempted to think thats ok, since that block just sets the refcnt to zero, and doing so twice isn't harmful, but the entire purpose of that if conditional above was to ensure that only one execution context ever executed the conditional for a given buffer. Look at what else happens in that conditional: static inline struct rte_mbuf* __attribute__((always_inline)) __rte_pktmbuf_prefree_seg(struct rte_mbuf *m) { __rte_mbuf_sanity_check(m, 0); if (likely (rte_mbuf_refcnt_read(m) == 1) || likely (rte_mbuf_refcnt_update(m, -1) == 0)) { rte_mbuf_refcnt_set(m, 0); /* if this is an indirect mbuf, then * - detach mbuf * - free attached mbuf segment */ if (RTE_MBUF_INDIRECT(m)) { struct rte_mbuf *md = RTE_MBUF_FROM_BADDR(m->buf_addr); rte_pktmbuf_detach(m); if (rte_mbuf_refcnt_update(md, -1) == 0) __rte_mbuf_raw_free(md); } return(m); } return (NULL); } If the buffer is indirect, another refcnt update occurs to the buf_addr mbuf, and in the scenario I outlined above, that refcnt will underflow, likely causing a buffer leak. Additionally, the return code of this function is designed to indicate to the caller if they were the last user of the buffer. In the above scenario, two execution contexts will be told that they were, which is wrong. Zoltans patch is a good fix Acked-by: Neil Horman