From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mx.bisdn.de (mx.bisdn.de [185.27.182.31]) by dpdk.org (Postfix) with ESMTP id 40ED55A8C for ; Mon, 30 Mar 2015 21:39:16 +0200 (CEST) Received: from [192.168.1.102] (x5ce54aac.dyn.telefonica.de [92.229.74.172]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by mx.bisdn.de (Postfix) with ESMTPSA id EBE3CA2FB8 for ; Mon, 30 Mar 2015 21:39:15 +0200 (CEST) Message-ID: <5519A661.6060202@bisdn.de> Date: Mon, 30 Mar 2015 21:39:13 +0200 From: Marc Sune User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Icedove/31.5.0 MIME-Version: 1.0 To: dev@dpdk.org References: <1427404494-27256-1-git-send-email-bruce.richardson@intel.com> <20150327102956.GB5375@hmsreliant.think-freely.org> <20150327113238.GA11660@bricha3-MOBL3> <20150327140735.GG5375@hmsreliant.think-freely.org> <20150327143049.GB9972@bricha3-MOBL3> In-Reply-To: <20150327143049.GB9972@bricha3-MOBL3> Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [dpdk-dev] [PATCH] mbuf: add comment explaining confusing code 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: Mon, 30 Mar 2015 19:39:16 -0000 On 27/03/15 15:30, Bruce Richardson wrote: > On Fri, Mar 27, 2015 at 10:07:35AM -0400, Neil Horman wrote: >> On Fri, Mar 27, 2015 at 11:32:38AM +0000, Bruce Richardson wrote: >>> On Fri, Mar 27, 2015 at 06:29:56AM -0400, Neil Horman wrote: >>>> On Thu, Mar 26, 2015 at 09:14:54PM +0000, Bruce Richardson wrote: >>>>> The logic used in the condition check before freeing an mbuf is >>>>> sometimes confusing, so explain it in a proper comment. >>>>> >>>>> Signed-off-by: Bruce Richardson >>>>> --- >>>>> lib/librte_mbuf/rte_mbuf.h | 10 ++++++++++ >>>>> 1 file changed, 10 insertions(+) >>>>> >>>>> diff --git a/lib/librte_mbuf/rte_mbuf.h b/lib/librte_mbuf/rte_mbuf.h >>>>> index 17ba791..0265172 100644 >>>>> --- a/lib/librte_mbuf/rte_mbuf.h >>>>> +++ b/lib/librte_mbuf/rte_mbuf.h >>>>> @@ -764,6 +764,16 @@ __rte_pktmbuf_prefree_seg(struct rte_mbuf *m) >>>>> { >>>>> __rte_mbuf_sanity_check(m, 0); >>>>> >>>>> + /* >>>>> + * Check to see if this is the last reference to the mbuf. >>>>> + * Note: the double check here is deliberate. If the ref_cnt is "atomic" >>>>> + * the call to "refcnt_update" is a very expensive operation, so we >>>>> + * don't want to call it in the case where we know we are the holder >>>>> + * of the last reference to this mbuf i.e. ref_cnt == 1. >>>>> + * If however, ref_cnt != 1, it's still possible that we may still be >>>>> + * the final decrementer of the count, so we need to check that >>>>> + * result also, to make sure the mbuf is freed properly. >>>>> + */ >>>>> if (likely (rte_mbuf_refcnt_read(m) == 1) || >>>>> likely (rte_mbuf_refcnt_update(m, -1) == 0)) { >>>>> >>>>> -- >>>>> 2.1.0 >>>>> >>>>> >>>> NAK >>>> the comment is incorrect, a return code of 1 from rte_mbuf_refcnt_read doesn't >>>> guarantee you are the last holder of the buffer if two contexts have a pointer >>>> to it. >>> If two threads have pointers to it, and are both going to free it, the refcnt >>> must be 2 not one, otherwise the refcnt is meaningless. >>> >> What about the other concrete case that I illustrated, where one context is >> attempting to increment the refcount, while the other is decrementing it with >> the intention to free? By making the read and set operation disctinct here >> you've broken the atomicity of the read and update logic that atomics are there >> for and created a race condition. I don't know how else to explain this to you. >> if(atomic_read == 1) then atomic_set(0), breaks the entire notion of what >> atomics are meant to do (namely update and read state as an atomic unit), you >> just can't get away with not having that atomicity here. If you could, you >> might as well be using plain integers for the reference count, as you're not >> using the atomic properties of the type. >> >> Neil > I disagree. > > A value of one, indicates that there is only one owner of the mbuf, > and therefore since we are in the free routine, we are that owner. This is true if you assume the application is bug-free and two threads will never hold mistakenly the same mbuf pointer with ref_cnt=1. That's ok in general, but if it doesn't, debugging it can be pretty painful. Marc > If there > are to be two owners, the refcnt must be incremented before handing over the > pointer to the other thread - to get to the example you make. If that does not > occur, we can also have the situation where the "sending" thread calls > free - and therefore this function - before the other thread receives the > pointer. In that case, we will have the receiving thread getting a pointer > to an mbuf which is now invalid as it has been put back into the mempool > > Again, in short, if refcnt == 1, there is only one mbuf owner. If refcnt == 1 > and we are currently executing in prefree_seg, we are the owner and no other > thread is allow to muck about with the mbuf. > > /Bruce >