Re: [PATCH] mbuf: add comment explaining confusing code

[Thomas Monjalon <thomas.monjalon-pdR9zngts4EAvxtiuMwx3w@public.gmane.org>]

2015-03-27 16:56, Richardson, Bruce:
> 
> > -----Original Message-----
> > From: Neil Horman [mailto:nhorman-2XuSBdqkA4R54TAoqtyWWQ@public.gmane.org]
> > Sent: Friday, March 27, 2015 4:44 PM
> > To: Richardson, Bruce
> > Cc: dev-VfR2kkLFssw@public.gmane.org
> > Subject: Re: [dpdk-dev] [PATCH] mbuf: add comment explaining confusing
> > code
> > 
> > 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
> > > > > > > > > <bruce.richardson-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
> > > > > > > > > ---
> > > > > > > > >  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
> > 
> > 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)==1 ||
> > atomic_ref_dec(mbuf)==0
> 
> 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. 
> The comment should adequately cover things, I think, and for cases where it doesn't we now have this thread to refer to also. :-)
> 
> > 
> > regardless, you've convinced me that its not broken.
> > Acked-by: Neil Horman <nhorman-2XuSBdqkA4R54TAoqtyWWQ@public.gmane.org>
> 
> Thanks,
> /Bruce

Applied, thanks