From owner-freebsd-hackers@FreeBSD.ORG Fri Feb 8 23:13:48 2013 Return-Path: Delivered-To: freebsd-hackers@FreeBSD.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:1900:2254:206a::19:1]) by hub.freebsd.org (Postfix) with ESMTP id 03C6A8B4 for ; Fri, 8 Feb 2013 23:13:48 +0000 (UTC) (envelope-from ian@FreeBSD.org) Received: from duck.symmetricom.us (duck.symmetricom.us [206.168.13.214]) by mx1.freebsd.org (Postfix) with ESMTP id C6D2E8C9 for ; Fri, 8 Feb 2013 23:13:47 +0000 (UTC) Received: from damnhippie.dyndns.org (daffy.symmetricom.us [206.168.13.218]) by duck.symmetricom.us (8.14.6/8.14.6) with ESMTP id r18NDhsd019349 for ; Fri, 8 Feb 2013 16:13:47 -0700 (MST) (envelope-from ian@FreeBSD.org) Received: from [172.22.42.240] (revolution.hippie.lan [172.22.42.240]) by damnhippie.dyndns.org (8.14.3/8.14.3) with ESMTP id r18NDeSJ036364; Fri, 8 Feb 2013 16:13:40 -0700 (MST) (envelope-from ian@FreeBSD.org) Subject: Re: Request for review, time_pps_fetch() enhancement From: Ian Lepore To: Konstantin Belousov In-Reply-To: <20130206155830.GX2522@kib.kiev.ua> References: <1360125698.93359.566.camel@revolution.hippie.lan> <20130206155830.GX2522@kib.kiev.ua> Content-Type: text/plain; charset="us-ascii" Date: Fri, 08 Feb 2013 16:13:40 -0700 Message-ID: <1360365220.4545.42.camel@revolution.hippie.lan> Mime-Version: 1.0 X-Mailer: Evolution 2.32.1 FreeBSD GNOME Team Port Content-Transfer-Encoding: 7bit Cc: "freebsd-hackers@freebsd.org" 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: Fri, 08 Feb 2013 23:13:48 -0000 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" >