From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga02.intel.com (mga02.intel.com [134.134.136.20]) by dpdk.org (Postfix) with ESMTP id 4856A5A73 for ; Fri, 27 Mar 2015 17:57:07 +0100 (CET) Received: from orsmga003.jf.intel.com ([10.7.209.27]) by orsmga101.jf.intel.com with ESMTP; 27 Mar 2015 09:56:47 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.11,479,1422950400"; d="scan'208";a="547341039" Received: from irsmsx151.ger.corp.intel.com ([163.33.192.59]) by orsmga003.jf.intel.com with ESMTP; 27 Mar 2015 09:56:46 -0700 Received: from irsmsx103.ger.corp.intel.com ([169.254.3.143]) by IRSMSX151.ger.corp.intel.com ([169.254.4.52]) with mapi id 14.03.0224.002; Fri, 27 Mar 2015 16:56:45 +0000 From: "Richardson, Bruce" To: Neil Horman Thread-Topic: [dpdk-dev] [PATCH] mbuf: add comment explaining confusing code Thread-Index: AQHQaAnr79ziUpJ9n0K14QHq8Vusnp0wIdwAgAARhACAACtLgIAABn6AgAACM4CAAASuAIAAHlMAgAACi2A= Date: Fri, 27 Mar 2015 16:56:45 +0000 Message-ID: <59AF69C657FD0841A61C55336867B5B0344F112F@IRSMSX103.ger.corp.intel.com> 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> <20150327145526.GA10332@bricha3-MOBL3> <20150327164358.GI5375@hmsreliant.think-freely.org> In-Reply-To: <20150327164358.GI5375@hmsreliant.think-freely.org> Accept-Language: en-GB, en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-originating-ip: [163.33.239.182] Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 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 16:57:08 -0000 > -----Original Message----- > From: Neil Horman [mailto:nhorman@tuxdriver.com] > Sent: Friday, March 27, 2015 4:44 PM > To: Richardson, Bruce > Cc: dev@dpdk.org > Subject: Re: [dpdk-dev] [PATCH] mbuf: add comment explaining confusing > code >=20 > On Fri, Mar 27, 2015 at 02:55:27PM +0000, Bruce Richardson wrote: > > 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 =3D=3D= 1. > > > > > > > > + * If however, ref_cnt !=3D 1, it's still possible that w= e > 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) =3D=3D 1) || > > > > > > > > likely (rte_mbuf_refcnt_update(m, -1) =3D=3D 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 =3D=3D 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 =3D=3D 1, there is only one mbuf owner. = If > > > > refcnt =3D=3D 1 and we are currently executing in prefree_seg, we a= re > > > > 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 !=3D 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 >=20 > Sigh, ok, so that makes some sense. This thing is entirely for the > purposes of special casing the single use case? That seems like alot of > effort and confusion to go through for this. Perhaps macrotizing it for > multiple use cases would clarify it: > #define mbuf_orphaned(mbuf) atomic_ref_read(mbuf)=3D=3D1 || > atomic_ref_dec(mbuf)=3D=3D0 Yes, we could, except it's not "orphaned" since it has got a single thread = owner, and this is the normal use-case we are special-casing.=20 The comment should adequately cover things, I think, and for cases where it= doesn't we now have this thread to refer to also. :-) >=20 > regardless, you've convinced me that its not broken. > Acked-by: Neil Horman Thanks, /Bruce