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 <rst...@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@freebsd.org mailing list
> > https://lists.freebsd.org/mailman/listinfo/svn-src-all
> > To unsubscribe, send any mail to "svn-src-all-unsubscr...@freebsd.org"
>
>
>
> --
> Tomorrow Will Never Die
--
Oleg.
================================================================
=== Oleg Bulyzhin -- OBUL-RIPN -- OBUL-RIPE -- o...@rinet.ru ===
================================================================
_______________________________________________
svn-src-all@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-all
To unsubscribe, send any mail to "svn-src-all-unsubscr...@freebsd.org"
