Request for review, time_pps_fetch() enhancement

[Konstantin Belousov]

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);
>  }
>  
>  /*

> _______________________________________________
> freebsd-hackers at freebsd.org mailing list
> http://lists.freebsd.org/mailman/listinfo/freebsd-hackers
> To unsubscribe, send any mail to "freebsd-hackers-unsubscribe at freebsd.org"

-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 834 bytes
Desc: not available
URL: <http://lists.freebsd.org/pipermail/freebsd-hackers/attachments/20130206/5e716f67/attachment.sig>