Date: Fri, 08 Feb 2013 16:13:40 -0700 From: Ian Lepore <ian@FreeBSD.org> To: Konstantin Belousov <kostikbel@gmail.com> Cc: "freebsd-hackers@freebsd.org" <freebsd-hackers@FreeBSD.org> Subject: Re: Request for review, time_pps_fetch() enhancement Message-ID: <1360365220.4545.42.camel@revolution.hippie.lan> 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, 2013-02-06 at 17:58 +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.
> >
> > -- Ian
> >
>
> > 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)
> > +{
> > + int err, timo;
> > + pps_seq_t aseq, cseq;
> > + struct timeval tv;
> > +
> > + if (fapi->tsformat && fapi->tsformat != PPS_TSFMT_TSPEC)
> > + return (EINVAL);
> > +
> > + /*
> > + * If no timeout is requested, immediately return whatever values were
> > + * most recently captured. If timeout seconds is -1, that's a request
> > + * to block without a timeout. WITNESS won't let us sleep forever
> > + * without a lock (we really don't need a lock), so just repeatedly
> > + * sleep a long time.
> > + */
> Regarding no need for the lock, it would just move the implementation into
> the low quality one, for the case when one timestamp capture is lost
> and caller of time_pps_fetch() sleeps until next pps event is generated.
>
> I understand the desire to avoid lock, esp. in the pps_event() called
> from the arbitrary driver context. But the race is also real.
>
What race? A user of the pps interface understands that there is one
event per second, and understands that if you ask to block until the
next event at approximately the time that event is expected to occur,
then it is ambiguous whether the call completes almost-immediately or in
about 1 second.
Looking at it another way, if a blocking call is made right around the
time of the PPS, the thread could get preempted before getting to
pps_fetch() function and not get control again until after the PPS has
occurred. In that case it's going to block for about a full second,
even though the call was made before top-of-second. That situation is
exactly the same with or without locking, so what extra functionality is
gained with locking? What guarantee does locking let us make to the
caller that the lockless code doesn't?
> > + if (fapi->timeout.tv_sec || fapi->timeout.tv_nsec) {
> > + if (fapi->timeout.tv_sec == -1)
> > + timo = 0x7fffffff;
> > + else {
> > + tv.tv_sec = fapi->timeout.tv_sec;
> > + tv.tv_usec = fapi->timeout.tv_nsec / 1000;
> > + timo = tvtohz(&tv);
> > + }
> > + 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.
>
Thank you. I pondered volatility, but was under the impression that the
function call took care of it. I'll fix that.
-- Ian
> > + err = tsleep(pps, PCATCH, "ppsfch", timo);
> > + if (err == EWOULDBLOCK && fapi->timeout.tv_sec == -1) {
> > + continue;
> > + } else if (err != 0) {
> > + return (err);
> > + }
> > + }
> > + }
> > +
> > + pps->ppsinfo.current_mode = pps->ppsparam.mode;
> > + fapi->pps_info_buf = pps->ppsinfo;
> > +
> > + return (0);
> > +}
> > +
> > int
> > pps_ioctl(u_long cmd, caddr_t data, struct pps_state *pps)
> > {
> > @@ -1485,13 +1529,7 @@
> > return (0);
> > case PPS_IOC_FETCH:
> > fapi = (struct pps_fetch_args *)data;
> > - if (fapi->tsformat && fapi->tsformat != PPS_TSFMT_TSPEC)
> > - return (EINVAL);
> > - if (fapi->timeout.tv_sec || fapi->timeout.tv_nsec)
> > - return (EOPNOTSUPP);
> > - pps->ppsinfo.current_mode = pps->ppsparam.mode;
> > - fapi->pps_info_buf = pps->ppsinfo;
> > - return (0);
> > + return (pps_fetch(fapi, pps));
> > #ifdef FFCLOCK
> > case PPS_IOC_FETCH_FFCOUNTER:
> > fapi_ffc = (struct pps_fetch_ffc_args *)data;
> > @@ -1540,7 +1578,7 @@
> > void
> > pps_init(struct pps_state *pps)
> > {
> > - pps->ppscap |= PPS_TSFMT_TSPEC;
> > + pps->ppscap |= PPS_TSFMT_TSPEC | PPS_CANWAIT;
> > if (pps->ppscap & PPS_CAPTUREASSERT)
> > pps->ppscap |= PPS_OFFSETASSERT;
> > if (pps->ppscap & PPS_CAPTURECLEAR)
> > @@ -1680,6 +1718,9 @@
> > hardpps(tsp, ts.tv_nsec + 1000000000 * ts.tv_sec);
> > }
> > #endif
> > +
> > + /* Wakeup anyone sleeping in pps_fetch(). */
> > + wakeup(pps);
> > }
> >
> > /*
>
> > _______________________________________________
> > freebsd-hackers@freebsd.org mailing list
> > http://lists.freebsd.org/mailman/listinfo/freebsd-hackers
> > To unsubscribe, send any mail to "freebsd-hackers-unsubscribe@freebsd.org"
>
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?1360365220.4545.42.camel>