please review, patch for lost camisr

[Konstantin Belousov]

On Tue, May 28, 2013 at 01:35:01PM -0700, Alfred Perlstein wrote:
> [[  moved to hackers@ from private mail. ]]
> 
> On 5/28/13 1:13 PM, John Baldwin wrote:
> > On Tuesday, May 28, 2013 3:29:41 pm Alfred Perlstein wrote:
> >> On 5/28/13 9:04 AM, John Baldwin wrote:
> >>> On Tuesday, May 28, 2013 2:13:32 am Alfred Perlstein wrote:
> >>>> Hey folks,
> >>>>
> >>>> I had a talk with Nathan Whitehorn about the camisr issue.  The issue we
> >>>> are seeing we mostly know, but to summarize, we are losing the camisr
> >>>> signal and the camisr is not being run.
> >>>>
> >>>> I gave him a summary of what we have been seeing and pointed him to the
> >>>> code I am concerned about here:
> >>>> http://pastebin.com/tLKr7mCV  (this is inside of kern_intr.c).
> >>>>
> >>>> What I think that is happening is that the setting of it_need to 0
> >>>> inside of sys/kern/kern_intr.c:ithread_loop() is not being scheduled
> >>>> correctly and it is being delayed until AFTER the call to
> >>>> ithread_execute_handlers() right below the atomic_store_rel_int().
> >>> This seems highly unlikely, to the extent that if this were true all our
> >>> locking primitives would be broken.  The store_rel is actually a release
> >>> barrier which acts like more a read/write fence.  No memory accesses (read or
> >>> write) from before the release can be scheduled after the associated store,
> >>> either by the compiler or CPU.  That is what Konstantin is referring to in his
> >>> commit when he says "release" semantics".
> >> Yes, that makes sense, however does it specify that the writes *must*
> >> occur at that *point*?  If it only enforces ordering then we may have
> >> some issue, specifically because the setting of it to '1' inside of
> >> intr_event_schedule_thread has no barrier other than the acq semantics
> >> of the thread lock.  I am wondering what is forcing out the '1' there.
> > Nothing ever forces writes.  You would have to invalidate the cache to do that
> > and that is horribly expensive.  It is always only about ordering and knowing
> > that if you can complete another operation on the same "cookie" variable with
> > acquire semantics that earlier writes will be visible.
> 
> By cookie, you mean a specific memory address, basically a lock? This is 
> starting to reinforce my suspicions as the setting of it_need is done 
> with release semantics, however the acq on the other CPU is done on the 
> thread lock.  Maybe that is irrelevant.  We will find out shortly.
> 
> >
> >> See below as I think we have proof that this is somehow happening.
> > Having ih_need of 1 and it_need of 0 is certainly busted.  The simplest fix
> > is probably to stop using atomics on it_need and just grab the thread lock
> > in the main ithread loop and hold it while checking and clearing it_need.
> >
> 
> OK, we have some code that will either prove this, or perturb the memory 
> ordering enough to make the bug go away, or prove this assertion wrong.
> 
> We will update on our findings hopefully in the next few days.

IMO the read of it_need in the 'while (ithd->it_need)' should
have acquire semantic, otherwise the future reads in the
ithread_execute_handlers(), in particular, of the ih_need, could pass
the read of it_need and cause the situation you reported.  I do not
see any acquire barrier between a condition in the while() statement
and the read of ih_need in the execute_handlers().

It is probably true that the issue you see was caused by r236456, in the
sense that implicitely locked xchgl instruction on x86 has a full barrier
semantic.  As result, the store_rel() was actually an acquire too, making
this reordering impossible.  I argue that this is not a bug in r236456,
but the issue in the kern_intr.c.

On the other hand, the John' suggestion to move the manipulations of
it_need under the lock is probably the best anyway.

-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 834 bytes
Desc: not available
URL: <http://lists.freebsd.org/pipermail/freebsd-hackers/attachments/20130529/71b4ea9a/attachment.sig>