From mboxrd@z Thu Jan 1 00:00:00 1970 From: Neil Horman Subject: Re: [PATCH] mbuf: optimize refcnt handling during free Date: Fri, 27 Mar 2015 06:25:33 -0400 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-Transfer-Encoding: quoted-printable Cc: "dev-VfR2kkLFssw@public.gmane.org" To: "Wiles, Keith" Return-path: Content-Disposition: inline In-Reply-To: List-Id: patches and discussions about DPDK List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces-VfR2kkLFssw@public.gmane.org Sender: "dev" On Thu, Mar 26, 2015 at 09:00:33PM +0000, Wiles, Keith wrote: >=20 >=20 > On 3/26/15, 1:10 PM, "Zoltan Kiss" wrote: >=20 > >The current way is not the most efficient: if m->refcnt is 1, the seco= nd > >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 remo= ve > >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); > >=20 > >- if (likely (rte_mbuf_refcnt_read(m) =3D=3D 1) || > >- likely (rte_mbuf_refcnt_update(m, -1) =3D=3D 0)) { > >- > >- rte_mbuf_refcnt_set(m, 0); > >+ if (likely (rte_mbuf_refcnt_update(m, -1) =3D=3D 0)) { > >=20 > > /* if this is an indirect mbuf, then > > * - detach mbuf >=20 > I fell for this one too, but read Bruce=B9s email > http://dpdk.org/ml/archives/dev/2015-March/014481.html This is still the right thing to do though, Bruce's reasoning is erroneou= s. 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) =3D=3D 1) || likely (rte_mbuf_refcnt_update(m, -1) =3D=3D 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 sk= ipped 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 th= at 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) =3D=3D 1) || likely (rte_mbuf_refcnt_update(m, -1) =3D=3D 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 =3D RTE_MBUF_FROM_BADDR(m->bu= f_addr); rte_pktmbuf_detach(m); if (rte_mbuf_refcnt_update(md, -1) =3D=3D 0) __rte_mbuf_raw_free(md); } return(m); } return (NULL); } If the buffer is indirect, another refcnt update occurs to the buf_addr m= buf, and in the scenario I outlined above, that refcnt will underflow, likely = causing a buffer leak. Additionally, the return code of this function is designe= d 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 wr= ong. Zoltans patch is a good fix Acked-by: Neil Horman