Date: Wed, 29 May 2013 08:08:35 +0300 From: Konstantin Belousov <kostikbel@gmail.com> To: Alfred Perlstein <alfred@ixsystems.com> Cc: Alexander Motin <mav@freebsd.org>, hackers@freebsd.org, Xin Li <delphij@delphij.net> Subject: Re: please review, patch for lost camisr Message-ID: <20130529050835.GP3047@kib.kiev.ua> In-Reply-To: <51A514F5.9050405@ixsystems.com> References: <51A44B0C.8010908@ixsystems.com> <201305281204.14146.jhb@freebsd.org> <51A505A5.7030105@ixsystems.com> <201305281613.32414.jhb@freebsd.org> <51A514F5.9050405@ixsystems.com>
next in thread | previous in thread | raw e-mail | index | archive | help
--0b/DLdpkOOCEj3jc Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Tue, May 28, 2013 at 01:35:01PM -0700, Alfred Perlstein wrote: > [[ moved to hackers@ from private mail. ]] >=20 > 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 issu= e 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 rele= ase > >>> 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 k= nowing > > that if you can complete another operation on the same "cookie" variabl= e with > > acquire semantics that earlier writes will be visible. >=20 > By cookie, you mean a specific memory address, basically a lock? This is= =20 > starting to reinforce my suspicions as the setting of it_need is done=20 > with release semantics, however the acq on the other CPU is done on the= =20 > thread lock. Maybe that is irrelevant. We will find out shortly. >=20 > > > >> 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 l= ock > > in the main ithread loop and hold it while checking and clearing it_nee= d. > > >=20 > OK, we have some code that will either prove this, or perturb the memory= =20 > ordering enough to make the bug go away, or prove this assertion wrong. >=20 > 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. --0b/DLdpkOOCEj3jc Content-Type: application/pgp-signature -----BEGIN PGP SIGNATURE----- Version: GnuPG v2.0.20 (FreeBSD) iQIcBAEBAgAGBQJRpY1SAAoJEJDCuSvBvK1BnGYP/jEd2YKjWUCNIAhpA9U2B0Jq N1ckhmCnmp+NI4ierhsosT0+COAclCrpYFgrq6sEIz0HdU+CmYzjoh04fbN1OOph XY57D6hehssszwGW7NTUn9dMtt0Wiy47D10i9SSF2Gtyj38K+hEZm2y0Pqr1MEOy jMrE6Rfm5F4h2W0Lt3oJZCu8MtgWK0mTy3l6jre1f9uNCJt5ovleO0e6/PvwfIir LRTdBre3ETK3mCz04mAuHqFNb7YYpYiUdE5QYPFricXwXEdcmYQFVlGctUkI36Ue 2sjii/KmCwoXGDIf6+xY0fDWsd7cBsrK0PGWJGhagYIW4eazoBX5mvNY7pq9Md33 cTzVe1KDdtGTOAFBKTBpBlIUkdF4N23sxYO17ZgLXntLCGUIpA40h1BZF7S7BYQ+ hnXu0qCotxVsGDwQVaRaz544L1mRPUwSL8UAS3HuXkSqJ/nbCN6G7NjJ10fXyT+4 hUmXuPsnN5R5dM2yLDywf1Hy3UUBFudUPKk2Bq29fGbXxqTEMr0zMvTY/fGjFQo2 dJGGGD0q2XQnIcrDWpxQKh5z68+hyekocPTwMuoNCoMA+rz4VsDDRIZP26DElRMV 5JZDTalPNgPCDOxR6yymrus1P2BG4TEFl3dhuAaMvWMekBqdURXrSusaqTy3nMfq YcTIVY3XLgUUcaFO+iKf =XR5q -----END PGP SIGNATURE----- --0b/DLdpkOOCEj3jc--
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20130529050835.GP3047>