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 B720A5A73 for ; Fri, 27 Mar 2015 15:55:30 +0100 (CET) Received: from orsmga003.jf.intel.com ([10.7.209.27]) by fmsmga102.fm.intel.com with ESMTP; 27 Mar 2015 07:55:29 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.11,479,1422950400"; d="scan'208";a="547275752" Received: from bricha3-mobl3.ger.corp.intel.com ([10.243.20.27]) by orsmga003.jf.intel.com with SMTP; 27 Mar 2015 07:55:27 -0700 Received: by (sSMTP sendmail emulation); Fri, 27 Mar 2015 14:55:27 +0025 Date: Fri, 27 Mar 2015 14:55:27 +0000 From: Bruce Richardson To: Neil Horman Message-ID: <20150327145526.GA10332@bricha3-MOBL3> 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> <20150327143841.GH5375@hmsreliant.think-freely.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20150327143841.GH5375@hmsreliant.think-freely.org> Organization: Intel Shannon Ltd. User-Agent: Mutt/1.5.23 (2014-03-12) Cc: dev@dpdk.org 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: Fri, 27 Mar 2015 14:55:31 -0000 On Fri, Mar 27, 2015 at 10:38:41AM -0400, Neil Horman wrote: > On Fri, Mar 27, 2015 at 02:30:50PM +0000, 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. 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. > > > Then the question remains, why aren't you just using ints here? What is the > purpose of even bothering with atomics, if you don't feel like you need any > reliance on the atomic set and read state, which it was created for?? > > Neil Because for the case where refcnt != 1, you need the atomics. If you have two threads using the mbuf and refcnt is 2, both of them simultaneously can hand over their copies to two more threads. In that case, we need to guarantee refcnt to be 4, so we need to use atomics. Similarly, if both threads attempt to free at the same time, we need to ensure that only one of them actually returns the buf to the mempool - hence the atomic decrement and return value check. /Bruce > > > /Bruce > > > >