From owner-freebsd-hackers@FreeBSD.ORG Wed Feb 13 19:38:31 2013 Return-Path: Delivered-To: freebsd-hackers@FreeBSD.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:1900:2254:206a::19:1]) by hub.freebsd.org (Postfix) with ESMTP id BAAD661F; Wed, 13 Feb 2013 19:38:31 +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 11B28B04; Wed, 13 Feb 2013 19:38:30 +0000 (UTC) Received: from tom.home (kostik@localhost [127.0.0.1]) by kib.kiev.ua (8.14.6/8.14.6) with ESMTP id r1DJcQR2045824; Wed, 13 Feb 2013 21:38:26 +0200 (EET) (envelope-from kostikbel@gmail.com) DKIM-Filter: OpenDKIM Filter v2.7.4 kib.kiev.ua r1DJcQR2045824 Received: (from kostik@localhost) by tom.home (8.14.6/8.14.6/Submit) id r1DJcQRT045823; Wed, 13 Feb 2013 21:38:26 +0200 (EET) (envelope-from kostikbel@gmail.com) X-Authentication-Warning: tom.home: kostik set sender to kostikbel@gmail.com using -f Date: Wed, 13 Feb 2013 21:38:26 +0200 From: Konstantin Belousov To: Ian Lepore Subject: Re: Request for review, time_pps_fetch() enhancement Message-ID: <20130213193826.GU2522@kib.kiev.ua> References: <1360125698.93359.566.camel@revolution.hippie.lan> <20130206155830.GX2522@kib.kiev.ua> <20130209134706.GB19909@stack.nl> <20130210103745.GI2522@kib.kiev.ua> <1360685019.4545.170.camel@revolution.hippie.lan> <20130212203408.GM2522@kib.kiev.ua> <1360768592.4545.209.camel@revolution.hippie.lan> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="WRMGsLmc1wQc8NPq" Content-Disposition: inline In-Reply-To: <1360768592.4545.209.camel@revolution.hippie.lan> 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: "freebsd-hackers@freebsd.org" , Jilles Tjoelker 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, 13 Feb 2013 19:38:31 -0000 --WRMGsLmc1wQc8NPq Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Wed, Feb 13, 2013 at 08:16:32AM -0700, Ian Lepore wrote: > On Tue, 2013-02-12 at 22:34 +0200, Konstantin Belousov wrote: > > On Tue, Feb 12, 2013 at 09:03:39AM -0700, Ian Lepore wrote: > > > On Sun, 2013-02-10 at 12:37 +0200, Konstantin Belousov wrote: > > > > On Sat, Feb 09, 2013 at 02:47:06PM +0100, Jilles Tjoelker wrote: > > > > > On Wed, Feb 06, 2013 at 05:58:30PM +0200, Konstantin Belousov wro= te: > > > > > > On Tue, Feb 05, 2013 at 09:41:38PM -0700, Ian Lepore wrote: > > > > > > > I'd like feedback on the attached patch, which adds support t= o our > > > > > > > time_pps_fetch() implementation for the blocking behaviors de= scribed in > > > > > > > section 3.4.3 of RFC 2783. The existing implementation can o= nly return > > > > > > > the most recently captured data without blocking. These chan= ges add the > > > > > > > ability to block (forever or with timeout) until a new event = occurs. > > > > >=20 > > > > > > > Index: sys/kern/kern_tc.c > > > > > > > =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D > > > > > > > --- sys/kern/kern_tc.c (revision 246337) > > > > > > > +++ sys/kern/kern_tc.c (working copy) > > > > > > > @@ -1446,6 +1446,50 @@ > > > > > > > * RFC 2783 PPS-API implementation. > > > > > > > */ > > > > > > > =20 > > > > > > > +static int > > > > > > > +pps_fetch(struct pps_fetch_args *fapi, struct pps_state *pps) > > > > > > > +{ > > > > > > > [snip] > > > > > > > + aseq =3D pps->ppsinfo.assert_sequence; > > > > > > > + cseq =3D pps->ppsinfo.clear_sequence; > > > > > > > + while (aseq =3D=3D pps->ppsinfo.assert_sequence && > > > > > > > + cseq =3D=3D pps->ppsinfo.clear_sequence) { > > > > > > Note that compilers are allowed to optimize these accesses even= over > > > > > > the sequential point, which is the tsleep() call. Only accesses= to > > > > > > volatile objects are forbidden to be rearranged. > > > > >=20 > > > > > > I suggest to add volatile casts to pps in the loop condition. > > > > >=20 > > > > > The memory pointed to by pps is global (other code may have a poi= nter to > > > > > it); therefore, the compiler must assume that the tsleep() call (= which > > > > > invokes code in a different compilation unit) may modify it. > > > > >=20 > > > > > Because volatile does not make concurrent access by multiple thre= ads > > > > > defined either, adding it here only seems to slow down the code > > > > > (potentially). > > > > The volatile guarantees that the compiler indeed reloads the value = on > > > > read access. Conceptually, the tsleep() does not modify or even acc= ess > > > > the checked fields, and compiler is allowed to note this by whatever > > > > methods (LTO ?). > > > >=20 > > > > More, the standard says that an implementation is allowed to not ev= aluate > > > > part of the expression if no side effects are produced, even by cal= ling > > > > a function. > > > >=20 > > > > I agree that for practical means, the _currently_ used compilers sh= ould > > > > consider the tsleep() call as the sequential point. But then the vo= latile > > > > qualifier cast applied for the given access would not change the co= de as > > > > well. > > > >=20 > > >=20 > > > Doesn't this then imply that essentially every driver has this proble= m, > > > and for that matter, every sequence of code anywhere in the base > > > involving "loop while repeatedly sleeping, then waking and checking t= he > > > state of some data for changes"? I sure haven't seen that many volat= ile > > > qualifiers scattered around the code. > >=20 > > No, it does not imply that every driver has this problem. > > A typical driver provides the mutual exclusion for access of > > the shared data, which means using locks. Locks include neccessary > > barries to ensure the visibility of the changes, in particular the > > compiler barriers. >=20 > Ohhhh. I had never considered that using mutexes had other side > effects. So is there a correct MI way to invoke the right barrier magic > in a situation like this? My belief is that you do not need a barrier there. The only (slightly) problematic issue there is a purely theoretical possibility that a very smart compiler would omit the reload step. The volatile qualifier for the dererefence in the loop condition should close this, as I described in the very first reply. --WRMGsLmc1wQc8NPq Content-Type: application/pgp-signature -----BEGIN PGP SIGNATURE----- Version: GnuPG v2.0.19 (FreeBSD) iQIcBAEBAgAGBQJRG+uyAAoJEJDCuSvBvK1BWS0P/i8X5JG/rjIF57jyizv98ZYv MO5sDgLZ1WBffstFqZ6o1ZfUihFuXnRjY20AXgtu13wdPms3PF7WwYH/pnnfn0Nm V6f0FkHpuut3fJLZmHrHNfCDnm3BZf1wdxTeDBL7JQk9FLpFjtLVe0TGqhxthKOr HURDQ8edTSNu8FxDdrcUpnrB8dOvcNxbUXJCFcEs+xbb421GvH82yTWwvUUCQmVG SaAaq/NkdkrhwYej0a+XXSIHMkQxU7/fZ25jerGY4FBK5xcqV52Yh6HzXtr2V40K +1r2QxjT00xrqISBzI+ZUPd5YMicX7JgnCKnhopmRPpjs3lZsDcA7jnElqDowtrW +8DlzOzovnULpIW3kch3xkab3QLyaAq28UBsiDhrS6t/JjiowEo6FsYS7LFoYg9h m1Osp2hSUdU7WAcVVWovIHR2NIz1uF35ncf0Za8jQKUZIryVCXr58ocek8fO6JD2 oZ9WujWp57y69SOEwE3CK4GmZ/hp+3R35+knyvVYB8WsBsd62lIdBzfTYmkLNjFy 0x1msi9HTuU8AAQ4KLVcG8GYRp33BgEmvi0F1/5C36xKsVLcmBHhzmsur1tYQjVN PUZYv2i5jKLEG73+Y1cCZ2V0WhsEDRa1VeTv9rDji0aiNAxXktHA2EO4f0M54QSf vElk+h9s8CY5O/5yy3U7 =c0im -----END PGP SIGNATURE----- --WRMGsLmc1wQc8NPq--