Date: Sat, 9 Feb 2013 14:47:06 +0100 From: Jilles Tjoelker <jilles@stack.nl> To: Konstantin Belousov <kostikbel@gmail.com> Cc: "freebsd-hackers@freebsd.org" <freebsd-hackers@FreeBSD.org>, Ian Lepore <ian@FreeBSD.org> Subject: Re: Request for review, time_pps_fetch() enhancement Message-ID: <20130209134706.GB19909@stack.nl> In-Reply-To: <20130206155830.GX2522@kib.kiev.ua> References: <1360125698.93359.566.camel@revolution.hippie.lan> <20130206155830.GX2522@kib.kiev.ua>
next in thread | previous in thread | raw e-mail | index | archive | help
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 return
> > the most recently captured data without blocking. These changes add the
> > ability to block (forever or with timeout) until a new event occurs.
> > Index: sys/kern/kern_tc.c
> > ===================================================================
> > --- sys/kern/kern_tc.c (revision 246337)
> > +++ sys/kern/kern_tc.c (working copy)
> > @@ -1446,6 +1446,50 @@
> > * RFC 2783 PPS-API implementation.
> > */
> >
> > +static int
> > +pps_fetch(struct pps_fetch_args *fapi, struct pps_state *pps)
> > +{
> > [snip]
> > + aseq = pps->ppsinfo.assert_sequence;
> > + cseq = pps->ppsinfo.clear_sequence;
> > + while (aseq == pps->ppsinfo.assert_sequence &&
> > + cseq == 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.
> I suggest to add volatile casts to pps in the loop condition.
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.
Because volatile does not make concurrent access by multiple threads
defined either, adding it here only seems to slow down the code
(potentially).
> > + err = tsleep(pps, PCATCH, "ppsfch", timo);
> > + if (err == EWOULDBLOCK && fapi->timeout.tv_sec == -1) {
> > + continue;
> > + } else if (err != 0) {
> > + return (err);
> > + }
> > + }
> > + }
--
Jilles Tjoelker
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20130209134706.GB19909>