Re: [PATCH net-next 1/5] timecounter: Add helper for reconstructing partial timestamps

[Vladimir Oltean]

On Wed, 29 May 2019 at 05:14, John Stultz <john.stultz@xxxxxxxxxx> wrote:
>
> On Tue, May 28, 2019 at 4:58 PM Vladimir Oltean <olteanv@xxxxxxxxx> wrote:
> >
> > Some PTP hardware offers a 64-bit free-running counter whose snapshots
> > are used for timestamping, but only makes part of that snapshot
> > available as timestamps (low-order bits).
> >
> > In that case, timecounter/cyclecounter users must bring the cyclecounter
> > and timestamps to the same bit width, and they currently have two
> > options of doing so:
> >
> > - Trim the higher bits of the timecounter itself to the number of bits
> >   of the timestamps.  This might work for some setups, but if the
> >   wraparound of the timecounter in this case becomes high (~10 times per
> >   second) then this causes additional strain on the system, which must
> >   read the clock that often just to avoid missing the wraparounds.
> >
> > - Reconstruct the timestamp by racing to read the PTP time within one
> >   wraparound cycle since the timestamp was generated.  This is
> >   preferable when the wraparound time is small (do a time-critical
> >   readout once vs doing it periodically), and it has no drawback even
> >   when the wraparound is comfortably sized.
> >
> > Signed-off-by: Vladimir Oltean <olteanv@xxxxxxxxx>
> > ---
> >  include/linux/timecounter.h |  7 +++++++
> >  kernel/time/timecounter.c   | 33 +++++++++++++++++++++++++++++++++
> >  2 files changed, 40 insertions(+)
> >
> > diff --git a/include/linux/timecounter.h b/include/linux/timecounter.h
> > index 2496ad4cfc99..03eab1f3bb9c 100644
> > --- a/include/linux/timecounter.h
> > +++ b/include/linux/timecounter.h
> > @@ -30,6 +30,9 @@
> >   *     by the implementor and user of specific instances of this API.
> >   *
> >   * @read:              returns the current cycle value
> > + * @partial_tstamp_mask:bitmask in case the hardware emits timestamps
> > + *                     which only capture low-order bits of the full
> > + *                     counter, and should be reconstructed.
> >   * @mask:              bitmask for two's complement
> >   *                     subtraction of non 64 bit counters,
> >   *                     see CYCLECOUNTER_MASK() helper macro
> > @@ -38,6 +41,7 @@
> >   */
> >  struct cyclecounter {
> >         u64 (*read)(const struct cyclecounter *cc);
> > +       u64 partial_tstamp_mask;
> >         u64 mask;
> >         u32 mult;
> >         u32 shift;
> > @@ -136,4 +140,7 @@ extern u64 timecounter_read(struct timecounter *tc);
> >  extern u64 timecounter_cyc2time(struct timecounter *tc,
> >                                 u64 cycle_tstamp);
> >
> > +extern u64 cyclecounter_reconstruct(const struct cyclecounter *cc,
> > +                                   u64 ts_partial);
> > +
> >  #endif
> > diff --git a/kernel/time/timecounter.c b/kernel/time/timecounter.c
> > index 85b98e727306..d4657d64e38d 100644
> > --- a/kernel/time/timecounter.c
> > +++ b/kernel/time/timecounter.c
> > @@ -97,3 +97,36 @@ u64 timecounter_cyc2time(struct timecounter *tc,
> >         return nsec;
> >  }
> >  EXPORT_SYMBOL_GPL(timecounter_cyc2time);
> > +
> > +/**
> > + * cyclecounter_reconstruct - reconstructs @ts_partial
> > + * @cc:                Pointer to cycle counter.
> > + * @ts_partial:        Typically RX or TX NIC timestamp, provided by hardware as
> > + *             the lower @partial_tstamp_mask bits of the cycle counter,
> > + *             sampled at the time the timestamp was collected.
> > + *             To reconstruct into a full @mask bit-wide timestamp, the
> > + *             cycle counter is read and the high-order bits (up to @mask) are
> > + *             filled in.
> > + *             Must be called within one wraparound of @partial_tstamp_mask
> > + *             bits of the cycle counter.
> > + */
> > +u64 cyclecounter_reconstruct(const struct cyclecounter *cc, u64 ts_partial)
> > +{
> > +       u64 ts_reconstructed;
> > +       u64 cycle_now;
> > +
> > +       cycle_now = cc->read(cc);
> > +
> > +       ts_reconstructed = (cycle_now & ~cc->partial_tstamp_mask) |
> > +                           ts_partial;
> > +
> > +       /* Check lower bits of current cycle counter against the timestamp.
> > +        * If the current cycle counter is lower than the partial timestamp,
> > +        * then wraparound surely occurred and must be accounted for.
> > +        */
> > +       if ((cycle_now & cc->partial_tstamp_mask) <= ts_partial)
> > +               ts_reconstructed -= (cc->partial_tstamp_mask + 1);
> > +
> > +       return ts_reconstructed;
> > +}
> > +EXPORT_SYMBOL_GPL(cyclecounter_reconstruct);
>
> Hrm. Is this actually generic? Would it make more sense to have the
> specific implementations with this quirk implement this in their
> read() handler? If not, why?

Hi John, Richard,

It's not the cycle counter that needs reconstruction, but hardware
timestamps based on it.  Hence not possible to add a workaround in the
read() handler.
If it's not desirable to have this helper function in the cyclecounter
I'll move it to the driver in v2.

Thanks,
-Vladimir

>
> thanks
> -john