Request for review, time_pps_fetch() enhancement

[Konstantin Belousov]

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.
-------------- 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/20130210/04acd190/attachment.sig>