From owner-freebsd-hackers@FreeBSD.ORG Wed May 29 05:08:59 2013 Return-Path: Delivered-To: hackers@freebsd.org Received: from mx1.freebsd.org (mx1.FreeBSD.org [8.8.178.115]) by hub.freebsd.org (Postfix) with ESMTP id F15B8575; Wed, 29 May 2013 05:08:58 +0000 (UTC) (envelope-from kostikbel@gmail.com) Received: from kib.kiev.ua (kib.kiev.ua [IPv6:2001:470:d5e7:1::1]) by mx1.freebsd.org (Postfix) with ESMTP id 6770299E; Wed, 29 May 2013 05:08:58 +0000 (UTC) Received: from tom.home (kostik@localhost [127.0.0.1]) by kib.kiev.ua (8.14.7/8.14.7) with ESMTP id r4T58Z8A062005; Wed, 29 May 2013 08:08:35 +0300 (EEST) (envelope-from kostikbel@gmail.com) DKIM-Filter: OpenDKIM Filter v2.8.3 kib.kiev.ua r4T58Z8A062005 Received: (from kostik@localhost) by tom.home (8.14.7/8.14.7/Submit) id r4T58ZtY062004; Wed, 29 May 2013 08:08:35 +0300 (EEST) (envelope-from kostikbel@gmail.com) X-Authentication-Warning: tom.home: kostik set sender to kostikbel@gmail.com using -f Date: Wed, 29 May 2013 08:08:35 +0300 From: Konstantin Belousov To: Alfred Perlstein Subject: Re: please review, patch for lost camisr Message-ID: <20130529050835.GP3047@kib.kiev.ua> References: <51A44B0C.8010908@ixsystems.com> <201305281204.14146.jhb@freebsd.org> <51A505A5.7030105@ixsystems.com> <201305281613.32414.jhb@freebsd.org> <51A514F5.9050405@ixsystems.com> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="0b/DLdpkOOCEj3jc" Content-Disposition: inline In-Reply-To: <51A514F5.9050405@ixsystems.com> User-Agent: Mutt/1.5.21 (2010-09-15) X-Spam-Status: No, score=-2.0 required=5.0 tests=ALL_TRUSTED,BAYES_00, DKIM_ADSP_CUSTOM_MED,FREEMAIL_FROM,NML_ADSP_CUSTOM_MED autolearn=no version=3.3.2 X-Spam-Checker-Version: SpamAssassin 3.3.2 (2011-06-06) on tom.home Cc: Alexander Motin , hackers@freebsd.org, Xin Li X-BeenThere: freebsd-hackers@freebsd.org X-Mailman-Version: 2.1.14 Precedence: list List-Id: Technical Discussions relating to FreeBSD List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 29 May 2013 05:08:59 -0000 --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--