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.
> + 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.
> + 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);
> }
>
> /*
> _______________________________________________
> [email protected] mailing list
> http://lists.freebsd.org/mailman/listinfo/freebsd-hackers
> To unsubscribe, send any mail to "[email protected]"
pgpO2zZzzQm3w.pgp
Description: PGP signature
