Re: 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  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"

Re: svn commit: r309372 - head/sys/sys

[Sepherosa Ziehau]

peek_clear_sc is added to address the issue you mentioned.  IMHO, this
commit weakens the proper assertion.

On Fri, Dec 2, 2016 at 5:08 AM, Ryan Stone  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
___
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"