From owner-freebsd-hackers@FreeBSD.ORG Tue Feb 12 15:59:56 2013 Return-Path: Delivered-To: freebsd-hackers@FreeBSD.org Received: from mx1.freebsd.org (mx1.FreeBSD.org [8.8.178.115]) by hub.freebsd.org (Postfix) with ESMTP id D877C3AF for ; Tue, 12 Feb 2013 15:59:56 +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 87EB2DB6 for ; Tue, 12 Feb 2013 15:59:56 +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 r1CFxjnB083115 for ; Tue, 12 Feb 2013 08:59:52 -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 r1CFxNDa041137; Tue, 12 Feb 2013 08:59:23 -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: <20130210104145.GJ2522@kib.kiev.ua> References: <1360125698.93359.566.camel@revolution.hippie.lan> <20130206155830.GX2522@kib.kiev.ua> <1360365220.4545.42.camel@revolution.hippie.lan> <20130210104145.GJ2522@kib.kiev.ua> Content-Type: text/plain; charset="us-ascii" Date: Tue, 12 Feb 2013 08:59:23 -0700 Message-ID: <1360684763.4545.167.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: Tue, 12 Feb 2013 15:59:56 -0000 On Sun, 2013-02-10 at 12:41 +0200, Konstantin Belousov wrote: > On Fri, Feb 08, 2013 at 04:13:40PM -0700, Ian Lepore wrote: > > 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? > > No guarantees, but I noted in the original reply that this is about the > quality of the implementation and not about correctness. > > As I said there as well, I am not sure that any locking can be useful > for the situation at all. Well then I guess I don't understand what you mean by the term "quality". Apparently you use it as some form of jargon rather than its usual accepted meaning in everyday English? Or, more directly: are you implying something should be changed to make the code better? -- Ian