From owner-freebsd-hackers@FreeBSD.ORG Sun Feb 10 10:37:54 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 7838328E; Sun, 10 Feb 2013 10:37:54 +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 C6B8B343; Sun, 10 Feb 2013 10:37:53 +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 r1AAbjlb071423; Sun, 10 Feb 2013 12:37:45 +0200 (EET) (envelope-from kostikbel@gmail.com) DKIM-Filter: OpenDKIM Filter v2.7.4 kib.kiev.ua r1AAbjlb071423 Received: (from kostik@localhost) by tom.home (8.14.6/8.14.6/Submit) id r1AAbjRY071422; Sun, 10 Feb 2013 12:37:45 +0200 (EET) (envelope-from kostikbel@gmail.com) X-Authentication-Warning: tom.home: kostik set sender to kostikbel@gmail.com using -f Date: Sun, 10 Feb 2013 12:37:45 +0200 From: Konstantin Belousov To: Jilles Tjoelker Subject: Re: Request for review, time_pps_fetch() enhancement Message-ID: <20130210103745.GI2522@kib.kiev.ua> References: <1360125698.93359.566.camel@revolution.hippie.lan> <20130206155830.GX2522@kib.kiev.ua> <20130209134706.GB19909@stack.nl> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="TMGYF+XY7wpHOFd9" Content-Disposition: inline In-Reply-To: <20130209134706.GB19909@stack.nl> 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" , Ian Lepore 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: Sun, 10 Feb 2013 10:37:54 -0000 --TMGYF+XY7wpHOFd9 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable 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 described = in > > > section 3.4.3 of RFC 2783. The existing implementation can only retu= rn > > > the most recently captured data without blocking. These changes 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 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 ?). More, the standard says that an implementation is allowed to not evaluate part of the expression if no side effects are produced, even by calling a function. I agree that for practical means, the _currently_ used compilers should consider the tsleep() call as the sequential point. But then the volatile qualifier cast applied for the given access would not change the code as well. >=20 > > > + err =3D tsleep(pps, PCATCH, "ppsfch", timo); > > > + if (err =3D=3D EWOULDBLOCK && fapi->timeout.tv_sec =3D=3D -1) { > > > + continue; > > > + } else if (err !=3D 0) { > > > + return (err); > > > + } > > > + } > > > + } > --=20 > Jilles Tjoelker --TMGYF+XY7wpHOFd9 Content-Type: application/pgp-signature -----BEGIN PGP SIGNATURE----- Version: GnuPG v2.0.19 (FreeBSD) iQIcBAEBAgAGBQJRF3h4AAoJEJDCuSvBvK1B+dYP/3Qqs9HWVNiMeSg96SZpBKb6 FPWX1+ORMEg8OCpJwseTgaYivVLIitDs/eoUnfyZnBDtydm1cJq76jT1iMPL58PC xFfqst1viOmq5aj0yBuK5XZGOleLgXQusx7mR6S7FK2S6fL+NdoPTz5fhRY380Jy LdWXkZX+CPnEuC8f3KToASxajbEcVBns2LEsiJ9k4mm28jNhwv9AkUwDLFuvrHlY UJCpBzDwxeET7EZL6GTU8krWpCwTUKzwC+zUQL4D7Fl5CVb7hrzdAklCNKDKHUEE vBadYROFNOeYAVWnqqk//BquXPmkuy6UZhs7/w/QBod7UiYA3GesjHW2+/JmL0pi Zc6ZgWE/WfIAREaTUkUwK5+QZxRkev7fNNX46kJXMkvzr82ko5U28O4EGLyqvDjZ Z6VRZC9NeS2Js24Tl8kbJP2fKTkLMdYzTrW5RqcVSPmiEYLjma4NJ3vrsflqnfmr ZKBnR986q6pcAQK+04Uo2wQ94kFM2Jy09S1LVx2Bcjk+qjpjQ6H68pHYlUa5SHPI u4kzy4sl8nIyb1EvDiryZ2Ds8mC9YbVCBwBJcLBonGx62+jA3YjgnZQDSBfX6oi/ 9qqn7O5EEY0renI+vvpXNR5/dXg2Jmp4E7La9Gr+R/CEZUdr5qZ+NACnPgDc1ExI a1SyRlbJyshZvdpcLwuA =+kNV -----END PGP SIGNATURE----- --TMGYF+XY7wpHOFd9--