svn commit: r309372 - head/sys/sys

[Oleg Bulyzhin]

On Fri, Dec 02, 2016 at 04:11:32PM +0800, Sepherosa Ziehau wrote:
> peek_clear_sc is added to address the issue you mentioned.  IMHO, this
> commit weakens the proper assertion.

I would say this check:

#ifdef DEBUG_BUFRING
        int i;
        for (i = br->br_cons_head; i != br->br_prod_head;
             i = ((i + 1) & br->br_cons_mask))
                if(br->br_ring[i] == buf)
                        panic("buf=%p already enqueue at %d prod=%d cons=%d",
                            buf, i, br->br_prod_tail, br->br_cons_tail);
#endif

can't be relied upon:

1) it does not synchronize with anything, neither consumers nor producers.
   So you can read inconsistent data. For example, you are storing
   NULL in buf_ring_peek_clear_sc() but there is no guarantee you will see
   it here.
2) it's not covered by critical section so thread running this check can be
   preempted. Consider the following scenario: 
   a) thread is running this check and gets preempted by other producer
      before i != br->br_prod_head check.
   b) other producer moves br->br_prod_head forward.
   c) if we are unlucky we can spin forever.

Current buf_ring implementation has insufficient memory ordering constraints.
I've tried to fix acq/rel usage here:
https://reviews.freebsd.org/D8637
but didn't get any review yet.

> 
> On Fri, Dec 2, 2016 at 5:08 AM, Ryan Stone <rstone at freebsd.org> wrote:
> > Author: rstone
> > Date: Thu Dec  1 21:08:42 2016
> > New Revision: 309372
> > URL: https://svnweb.freebsd.org/changeset/base/309372
> >
> > Log:
> >   Fix a false positive in a buf_ring assert
> >
> >   buf_ring contains an assert that checks whether an item being
> >   enqueued already exists on the ring.  There is a subtle bug in
> >   this assert.  An item can be returned by a peek() function and
> >   freed, and then the consumer thread can be preempted before
> >   calling advance().  If this happens the item appears to still be
> >   on the queue, but another thread may allocate the item from the
> >   free pool and wind up trying to enqueue it again, causing the
> >   assert to trigger incorrectly.
> >
> >   Fix this by skipping the head of the consumer's portion of the
> >   ring, as this index is what will be returned by peek().
> >
> >   Sponsored by: Dell EMC Isilon
> >   MFC After:    1 week
> >   Differential Revision:        https://reviews.freebsd.org/D8685
> >   Reviewed by:  hselasky
> >
> > Modified:
> >   head/sys/sys/buf_ring.h
> >
> > Modified: head/sys/sys/buf_ring.h
> > ==============================================================================
> > --- head/sys/sys/buf_ring.h     Thu Dec  1 20:36:48 2016        (r309371)
> > +++ head/sys/sys/buf_ring.h     Thu Dec  1 21:08:42 2016        (r309372)
> > @@ -67,11 +67,13 @@ buf_ring_enqueue(struct buf_ring *br, vo
> >         uint32_t prod_head, prod_next, cons_tail;
> >  #ifdef DEBUG_BUFRING
> >         int i;
> > -       for (i = br->br_cons_head; i != br->br_prod_head;
> > -            i = ((i + 1) & br->br_cons_mask))
> > -               if(br->br_ring[i] == buf)
> > -                       panic("buf=%p already enqueue at %d prod=%d cons=%d",
> > -                           buf, i, br->br_prod_tail, br->br_cons_tail);
> > +       if (br->br_cons_head != br->br_prod_head) {
> > +               for (i = (br->br_cons_head + 1) & br->br_cons_mask; i != br->br_prod_head;
> > +                   i = ((i + 1) & br->br_cons_mask))
> > +                       if(br->br_ring[i] == buf)
> > +                               panic("buf=%p already enqueue at %d prod=%d cons=%d",
> > +                                   buf, i, br->br_prod_tail, br->br_cons_tail);
> > +       }
> >  #endif
> >         critical_enter();
> >         do {
> > _______________________________________________
> > svn-src-all at freebsd.org mailing list
> > https://lists.freebsd.org/mailman/listinfo/svn-src-all
> > To unsubscribe, send any mail to "svn-src-all-unsubscribe at freebsd.org"
> 
> 
> 
> -- 
> Tomorrow Will Never Die

-- 
Oleg.

================================================================
=== Oleg Bulyzhin -- OBUL-RIPN -- OBUL-RIPE -- oleg at rinet.ru ===
================================================================