please review, patch for lost camisr

[Konstantin Belousov]

On Tue, May 28, 2013 at 10:31:40PM -0700, Alfred Perlstein wrote:
> On 5/28/13 10:08 PM, Konstantin Belousov wrote:
> > 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.
> If I remember the code correctly that would probably explain why we see 
> it only on 9.1 system.
> >
> > On the other hand, the John' suggestion to move the manipulations of
> > it_need under the lock is probably the best anyway.
> >
> I was wondering if it would be lower latency to maintain it_need, 
> however to keep another variable it_needlocked under the thread lock.  
> This would result in potential superfluous interrupts, however under 
> load you would allow the ithread to loop without taking the thread lock 
> some number of times.
> 
> I am not really sure if this is really worth the optimization 
> (especially since it can result in superfluous interrupts) however it 
> may reduce latency and that might be important.
> 
> Is there some people that I can pass the patch onto for help with 
> performance once we confirm that this is the actual bug?   We can do 
> internal testing, but I am worried about regressing performance of any 
> form of IO for the kernel.
> 
> I'll show the patch soon.
> 
> Thank you for the information.  This is promising.

Well, if you and I are right, the minimal patch should be

diff --git a/sys/kern/kern_intr.c b/sys/kern/kern_intr.c
index 8d63c9b..7c21015 100644
--- a/sys/kern/kern_intr.c
+++ b/sys/kern/kern_intr.c
@@ -1349,7 +1349,7 @@ ithread_loop(void *arg)
 		 * we are running, it will set it_need to note that we
 		 * should make another pass.
 		 */
-		while (ithd->it_need) {
+		while (atomic_load_acq_int(&ithd->it_need)) {
 			/*
 			 * This might need a full read and write barrier
 			 * to make sure that this write posts before any

-------------- 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/9fec102a/attachment-0001.sig>