From owner-freebsd-hackers@FreeBSD.ORG Tue Feb 12 20:34:14 2013 Return-Path: Delivered-To: freebsd-hackers@FreeBSD.org Received: from mx1.freebsd.org (mx1.FreeBSD.org [8.8.178.115]) by hub.freebsd.org (Postfix) with ESMTP id 1AEA8A71; Tue, 12 Feb 2013 20:34:14 +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 7E31DF54; Tue, 12 Feb 2013 20:34:13 +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 r1CKY8Vm067806; Tue, 12 Feb 2013 22:34:08 +0200 (EET) (envelope-from kostikbel@gmail.com) DKIM-Filter: OpenDKIM Filter v2.7.4 kib.kiev.ua r1CKY8Vm067806 Received: (from kostik@localhost) by tom.home (8.14.6/8.14.6/Submit) id r1CKY8kT067805; Tue, 12 Feb 2013 22:34:08 +0200 (EET) (envelope-from kostikbel@gmail.com) X-Authentication-Warning: tom.home: kostik set sender to kostikbel@gmail.com using -f Date: Tue, 12 Feb 2013 22:34:08 +0200 From: Konstantin Belousov To: Ian Lepore Subject: Re: Request for review, time_pps_fetch() enhancement Message-ID: <20130212203408.GM2522@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> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="Bnxx4g3oiWbQYxPJ" Content-Disposition: inline In-Reply-To: <1360685019.4545.170.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: Tue, 12 Feb 2013 20:34:14 -0000 --Bnxx4g3oiWbQYxPJ Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable 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 wrote: > > > > On Tue, Feb 05, 2013 at 09:41:38PM -0700, Ian Lepore wrote: > > > > > I'd like feedback on the attached patch, which adds support to our > > > > > time_pps_fetch() implementation for the blocking behaviors descri= bed in > > > > > section 3.4.3 of RFC 2783. The existing implementation can only = return > > > > > the most recently captured data without blocking. These changes = add the > > > > > ability to block (forever or with timeout) until a new event occu= rs. > > >=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 pointer= 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 threads > > > 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 access > > 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 evalua= te > > part of the expression if no side effects are produced, even by calling > > a function. > >=20 > > I agree that for practical means, the _currently_ used compilers should > > consider the tsleep() call as the sequential point. But then the volati= le > > qualifier cast applied for the given access would not change the code as > > well. > >=20 >=20 > Doesn't this then imply that essentially every driver has this problem, > and for that matter, every sequence of code anywhere in the base > involving "loop while repeatedly sleeping, then waking and checking the > state of some data for changes"? I sure haven't seen that many volatile > qualifiers scattered around the code. 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. --Bnxx4g3oiWbQYxPJ Content-Type: application/pgp-signature -----BEGIN PGP SIGNATURE----- Version: GnuPG v2.0.19 (FreeBSD) iQIcBAEBAgAGBQJRGqdAAAoJEJDCuSvBvK1BBRUP/2znbzWCkfxBUxHS/1zj4Nba sEIzYHvyzkMGRCXbEh65NoUb2wpNdTTOuqKoCWAVs+Zi81ZJOX8DbC9tol+5gvzj nprF2FOxHLDDykd3E8wo3i9U8CTgZ4VkES/keycRa5ZU7bTj70evNCcv2cZiN45L I07QHbyc85rNwooctagW8c0K6P+r0YPg0Ap0GPHFa3Is00zgSeVtCXUQh1wgLull wi+ClB2ipi+W57jeSu0oD/2toQTq59TKaur1mFHY629BV9rm8c4cEtGSQbZADQgB FFIFgvJmY6PT2xlpSKCxeVcF7hzdjcuWrJGoqDWECrVh6roTKTBGCrQReLF5k+8I Gn627aAzY8A8tm3Na9SrH34mQcSAJWgpMaH43HmH0E70BQdBNGS/KARv1jts9pFQ hoi5AIJiaEaFTz+fKd4m6z6DvzenBMo0cdGrDi+BFIs/5DiXQa0Re/dcK3bbcZWa zWpd1RukjJBbUgaa+A6Z+A5QLZMwoqFCQSf7CbtjW10wbxNiFiAQT+LYSNoCxyQ7 0kLUYARF6OxKJTeiys2lilXD9QTC0G7W+MC5i6qMb76fDvkl/wf/zqd65ZJq/JTJ Ayc9tmuyiUXRguUtcR35zZZTA0og6pHVvGXsw/qc9hToHAP6HSVDidGAHC1e6ErV a40gnMmmeOEKiL4zH5d3 =q+K1 -----END PGP SIGNATURE----- --Bnxx4g3oiWbQYxPJ--