Re: [dpdk-dev] [PATCH] mbuf: add comment explaining confusing code

[Neil Horman <nhorman@tuxdriver.com>]

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@intel.com>
> > > ---
> > >  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

> /Bruce
> 
> > 
> > Zoltans patch is the correct solution here, expensive or not.  I wrote up my
> > explination in this thread:
> > http://dpdk.org/ml/archives/dev/2015-March/015839.html
> > 
> > 
>