Is it a good idea to use a usb-serial adapter for PPS input? Yes, it is.

[Ian Lepore]

On Mon, 2019-09-09 at 19:45 +0300, Konstantin Belousov wrote:
> On Mon, Sep 09, 2019 at 05:20:25PM +0930, O'Connor, Daniel wrote:
> > 
> > 
> > > On 8 Sep 2019, at 23:12, Konstantin Belousov <kostikbel at gmail.com
> > > > wrote:
> > > > I suppose it would be good to change it to the same structure
> > > > as the feed forward clock stuff, that way it is much easier to
> > > > change the number of hands at compile time..
> > > > 
> > > 
> > > The reason to not increase it by default is the same as the
> > > reason why it
> > > was reduced.  But since I did still not provided the analysis why
> > > increasing
> > > timehands count helps for Ian' and your case, I think that making
> > > it easy
> > > to increase the timehands number is due.
> > 
> > I am a bit worried (based on your commit log for r303383) that the
> > code now relies on it being 2 for correct function, although given
> > I increased it to 10 and this system works fine perhaps not :)
> > 
> 
> It means that the sliding window where consumer should reach the
> writer
> to read the current timehands is larger. I think that in reality the
> cases where the latency of get*time*(9) KPI increased are very rare.
> 
> Still the question is why your system have the negative impact from
> reducing the number of timehands.  Interrupt should never interrupt
> tc_windup() because it is protected by spinlock.  Silly question, are
> spinlocks functional on this machine ?
> 

<dropping usb@ from the cc, since this is purely arm stuff>

Yep, spinlocks work fine.

The problem sequence that happens, as I remember it, is that the system
is sleeping in cpu_idle() which has stopped the single core with the
WFI (wait-for-interrupt) instruction.  The wakeup from WFI is an
eventtimer event that was scheduled to handle state->nexthard to keep
the timecounter updated.  As part of handling that event, the system
calls the timecounter's tc_poll_pps function (which is dmtpps_poll() in
arm/ti/am335x/am335x_dmtpps.c).  That captures the pps event time using
the current timehands, then schedules a taskqueue task to finish
processing the captured data.  By the time the taskqueue task runs,
tc_windup() has been called more times than there are timehands, so
pps_event() rejects the data because th->th_generation captured in
dmtpps_poll() does not match th_generation in that set of timehands
anymore.

When I was trying to track this down a couple years ago, part of why I
gave up was that I couldn't point my finger at anything that was
happening and say "here is the problem".  It's odd to me that
tc_windup() gets called 3 or 4 times so quickly like that, but odd is
not the same as wrong.

Ironically, a very busy system doesn't suffer this problem.  The bad
sequence happens only when the system is very quiet, spending most of
its time in cpu_idle().  It's something about that path where you come
out of cpu_idle() to process a hardclock event followed by a taskqueue
task that causes the multiple tc_windup calls to happen.  If hardclock
events are interrupting other processes, you somehow don't get as many
calls to tc_windup between the pps_capture() at interrupt time and the
pps_event() call from the taskqueue.

The problem also doesn't happen on multi-core systems, perhaps because
the taskqueue event begins running sooner on a different core (that is
a wild guess).  It requires the combo of single core, mostly-idle, and
pps-capture driver.

-- Ian