Re: [gpsd-dev] [PATCH] Refactoring NTPSHM.

[Gary E. Miller]

Yo Michael!

I see some things to like, and some things to hate.  Can you please split
this up into a bunch of little patches so I can evaluate one at a time?

The first thine I see is:

> -     double offset;
> +     double actual_time, integral, fractional;

There are many reasons why not to do that.
        1. floating point math on an ARM is very, very, slow and painful
        2. floating point double is not long enough to carry the
           seconds and nanoseconds without losing the precision we need.

That is why gpsd avoids floats whenever possible.

The gpsd plan is to move as much as possible to timespec's and
timedrift_t.  You are going the opposite direction.

On Tue, 5 Nov 2013 15:33:40 +0400
Michael Tatarinov <address@hidden> wrote:

> ---
>  gpsd.c      |  20 ++++++--
>  gpsd.h-tail |   6 +--
>  ntpshm.c    | 158
> ++++++++++++++---------------------------------------------- 3 files
> changed, 54 insertions(+), 130 deletions(-)
> 
> diff --git a/gpsd.c b/gpsd.c
> index e787765..50a9cfa 100644
> --- a/gpsd.c
> +++ b/gpsd.c
> @@ -704,7 +704,7 @@ static bool open_device( /address@hidden@*/struct
> gps_device_t *device) ntpshm_link_activate(device);
>      gpsd_report(context.debug, LOG_INF, 
>               "NTPD ntpshm_link_activate: %d\n",
> -             (int)device->shmindex >= 0);
> +             (int)device->shmIndex >= 0);
>  #endif /* NTPSHM_ENABLE */
>  
>      gpsd_report(context.debug, LOG_INF, 
> @@ -1530,15 +1530,25 @@ static void all_reports(struct gps_device_t
> *device, gps_mask_t changed) } else if (!device->ship_to_ntpd) {
>       //gpsd_report(context.debug, LOG_PROG, "NTP: No precision
> time report\n"); } else {
> -     double offset;
> +     double actual_time, integral, fractional;
> +     struct timespec actual_ts, clock_ts;
> +
> +     (void)clock_gettime(CLOCK_REALTIME, &clock_ts);
> +     actual_time = device->newdata.time;
>       //gpsd_report(context.debug, LOG_PROG, "NTP: Got one\n");
>       /* assume zero when there's no offset method */
>       if (device->device_type == NULL
>           || device->device_type->time_offset == NULL)
> -         offset = 0.0;
> +         actual_time += 0.0;
>       else
> -         offset = device->device_type->time_offset(device);
> -     (void)ntpshm_put(device, device->newdata.time, offset);
> +         actual_time += device->device_type->time_offset(device);
> +     fractional = modf(actual_time, &integral);
> +     actual_ts.tv_sec = (time_t)integral;
> +     actual_ts.tv_nsec = (long)(fractional * 1e+9);
> +     /* setting the precision here does not seem to help
> anything, too
> +      * hard to calculate properly anyway.  Let ntpd figure it
> out.
> +      * Any NMEA will be about -1 or -2. Garmin GPS-18/USB is
> around -6 or -7. */
> +     (void)ntpshm_put(device, device->shmIndex, &actual_ts,
> &clock_ts, -1); device->last_fixtime = device->newdata.time;
>      }
>  #endif /* NTPSHM_ENABLE */
> diff --git a/gpsd.h-tail b/gpsd.h-tail
> index 831585f..594179f 100644
> --- a/gpsd.h-tail
> +++ b/gpsd.h-tail
> @@ -466,9 +466,9 @@ struct gps_device_t {
>      unsigned long chars;     /* characters in the cycle */
>  #endif /* TIMING_ENABLE */
>  #ifdef NTPSHM_ENABLE
> -    int shmindex;
>      bool ship_to_ntpd;
> -    int shmTimeP;
> +    int shmIndex;
> +    int shmIndexPPS;
>  #endif /* NTPSHM_ENABLE */
>      timestamp_t last_fixtime;                /* so updates happen
> once */ #ifdef PPS_ENABLE
> @@ -820,7 +820,7 @@ extern unsigned int ais_binary_encode(struct
> ais_t *ais, /address@hidden@*/unsigned char 
>  extern void ntpshm_context_init(struct gps_context_t *);
>  extern void ntpshm_session_init(struct gps_device_t *);
> -extern int ntpshm_put(struct gps_device_t *, double, double);
> +extern int ntpshm_put(struct gps_device_t *, int, struct timespec *,
> struct timespec *, int); extern void ntpshm_link_deactivate(struct
> gps_device_t *); extern void ntpshm_link_activate(struct gps_device_t
> *); 
> diff --git a/ntpshm.c b/ntpshm.c
> index abc5908..a091c26 100644
> --- a/ntpshm.c
> +++ b/ntpshm.c
> @@ -52,9 +52,9 @@ struct shmTime
>      int precision;
>      int nsamples;
>      int valid;
> -    unsigned        clockTimeStampNSec;     /* Unsigned ns
> timestamps */
> -    unsigned        receiveTimeStampNSec;   /* Unsigned ns
> timestamps */
> -    int             dummy[8];
> +    unsigned clockTimeStampNSec;     /* Unsigned ns timestamps */
> +    unsigned receiveTimeStampNSec;   /* Unsigned ns timestamps
> */
> +    int pad[8];
>  };
>  
>  /* Note: you can start gpsd as non-root, and have it work with ntpd.
> @@ -214,39 +214,27 @@ void ntpshm_session_init(struct gps_device_t
> *session) {
>  #ifdef NTPSHM_ENABLE
>      /* mark NTPD shared memory segments as unused */
> -    session->shmindex = -1;
> +    session->shmIndex = -1;
>  #endif /* NTPSHM_ENABLE */
>  #ifdef PPS_ENABLE
> -    session->shmTimeP = -1;
> +    session->shmIndexPPS = -1;
>  #endif       /* PPS_ENABLE */
>  }
>  
> -int ntpshm_put(struct gps_device_t *session, double fixtime, double
> fudge) -/* put a received fix time into shared memory for NTP */
> +/* put time into shared memory for NTP */
> +int ntpshm_put(struct gps_device_t *session,
> +                   int index,
> +                   struct timespec *actual_ts,
> +                   struct timespec *clock_ts,
> +                   int precision)
>  {
>      /* shmTime is volatile to try to prevent C compiler from
> reordering
>       * writes, or optimizing some 'dead code'.  but CPU cache may
> still
> -     *write out of order since we do not use memory barriers, yet */
> +     * write out of order since we do not use memory barriers, yet */
>      volatile struct shmTime *shmTime = NULL;
> -    struct timeval tv;
> -    double seconds, microseconds;
> -
> -    // gpsd_report(session->context->debug, LOG_PROG, "NTP: doing
> ntpshm_put(,%g, %g)\n", fixtime, fudge);
> -    if (session->shmindex < 0 ||
> -     (shmTime = session->context->shmTime[session->shmindex]) ==
> NULL) {
> -     gpsd_report(session->context->debug, LOG_RAW,
> -                 "NTPD missing shm\n");
> -     return 0;
> -    }
>  
> -    (void)gettimeofday(&tv, NULL);
> -    fixtime += fudge;
> -    microseconds = 1000000.0 * modf(fixtime, &seconds);
> -    if (shmTime->clockTimeStampSec == (time_t) seconds) {
> -     gpsd_report(session->context->debug, LOG_RAW,
> -                 "NTPD ntpshm_put: skipping duplicate second\n");
> +    if (0 > index || (shmTime = session->context->shmTime[index]) ==
> NULL) return 0;
> -    }
>  
>      /* we use the shmTime mode 1 protocol
>       *
> @@ -261,110 +249,34 @@ int ntpshm_put(struct gps_device_t *session,
> double fixtime, double fudge)
>       *        use values
>       *    clear valid
>       *
> +     * FIXME need a memory barrier here to prevent write reordering
> by
> +     * the compiler or CPU cache
>       */
>      shmTime->valid = 0;
>      shmTime->count++;
> -    /* FIXME need a memory barrier here to prevent write reordering
> by
> -     * the compiler or CPU cache */
> -    shmTime->clockTimeStampSec = (time_t) seconds;
> -    shmTime->clockTimeStampUSec = (int)microseconds;
> -    shmTime->clockTimeStampNSec = (unsigned)(microseconds*1000);
> -    shmTime->receiveTimeStampSec = (time_t) tv.tv_sec;
> -    shmTime->receiveTimeStampUSec = (int)tv.tv_usec;
> -    shmTime->receiveTimeStampNSec = (int)(tv.tv_usec*1000);
> +    shmTime->clockTimeStampSec = (time_t)actual_ts->tv_sec;
> +    shmTime->clockTimeStampUSec = (int)(actual_ts->tv_nsec / 1000);
> +    shmTime->clockTimeStampNSec = (unsigned)(actual_ts->tv_nsec);
> +    shmTime->receiveTimeStampSec = (time_t)clock_ts->tv_sec;
> +    shmTime->receiveTimeStampUSec = (int)(clock_ts->tv_nsec / 1000);
> +    shmTime->receiveTimeStampNSec = (unsigned)(clock_ts->tv_nsec);
>      shmTime->leap = session->context->leap_notify;
> -    /* setting the precision here does not seem to help anything, too
> -     * hard to calculate properly anyway.  Let ntpd figure it out.
> -     * Any NMEA will be about -1 or -2.
> -     * Garmin GPS-18/USB is around -6 or -7.
> -     */
> -    /* FIXME need a memory barrier here to prevent write reordering
> by
> -     * the compiler or CPU cache */
> +    shmTime->precision = precision;
>      shmTime->count++;
>      shmTime->valid = 1;
>  
> -    gpsd_report(session->context->debug, LOG_RAW,
> -             "NTPD ntpshm_put: Clock: %lu.%06lu @ %lu.%06lu,
> fudge: %0.3f\n",
> -             (unsigned long)seconds, (unsigned long)microseconds,
> -             (unsigned long)tv.tv_sec, (unsigned long)tv.tv_usec,
> fudge); -
> -    return 1;
> -}
> -
> -#ifdef PPS_ENABLE
> -/address@hidden@*//* splint is confused here */
> -/* put NTP shared memory info based on received PPS pulse
> - *
> - * good news is that kernel PPS gives us nSec resolution
> - * bad news is that ntpshm only has uSec resolution
> - *
> - * actual_ts is the actual time we think the PPS happened
> - * clock_ts is the time we saw the pulse
> - */
> -static int ntpshm_pps(struct gps_device_t *session,
> -                   struct timespec *actual_ts,
> -                   struct timespec *clock_ts)
> -{
> -    volatile struct shmTime *shmTime = NULL, *shmTimeP = NULL;
> -    struct timeval actual_tv, clock_tv;
> -    int precision;
> -    double offset;
> -
> -    if (0 > session->shmindex || 0 > session->shmTimeP ||
> -     (shmTime = session->context->shmTime[session->shmindex]) ==
> NULL ||
> -     (shmTimeP = session->context->shmTime[session->shmTimeP]) ==
> NULL)
> -     return 0;
> -
> -    /* new ntpd interrace can use uSec andnSec */
> -    /* do NOT use TSTOTV() since that may round up and we need
> seconds to 
> -     * be the same for uSec and nSec.  */
> -
> -    /* we use the shmTime mode 1 protocol
> -     *
> -     * ntpd does this:
> -     *
> -     * reads valid.
> -     * IFF valid is 1
> -     *    reads count
> -     *    reads values
> -     *    reads count
> -     *    IFF count unchanged
> -     *        use values
> -     *    clear valid
> -     *
> -     */
> -    shmTimeP->valid = 0;
> -    shmTimeP->count++;
> -    shmTimeP->clockTimeStampSec = (time_t)actual_ts->tv_sec;
> -    shmTimeP->clockTimeStampUSec = (int)(actual_ts->tv_nsec/1000);
> -    shmTimeP->clockTimeStampUSec = (unsigned)actual_ts->tv_nsec;
> -    shmTimeP->receiveTimeStampSec = (time_t)clock_ts->tv_sec;
> -    shmTimeP->receiveTimeStampUSec = (int)(clock_ts->tv_nsec/1000);
> -    shmTimeP->receiveTimeStampNSec = (unsigned)clock_ts->tv_nsec;
> -    shmTimeP->leap = session->context->leap_notify;
> -    /* precision is a placebo, ntpd does not really use it
> -     * real world accuracy is around 16uS, thus -16 precision */
> -    shmTimeP->precision = -16;
> -    shmTimeP->count++;
> -    shmTimeP->valid = 1;
> -
> -    /* this is more an offset jitter/dispersion than precision,
> -     * but still useful for debug */
> -    offset = fabs((double)(clock_tv.tv_sec - actual_tv.tv_sec)
> -               + ((double)(clock_tv.tv_usec -
> actual_tv.tv_usec) / 1000000.0));
> -    precision = offset != 0 ? (int)(ceil(log(offset) / M_LN2)) : -20;
>      /address@hidden@*//* splint is confused about struct timespec */
>      gpsd_report(session->context->debug, LOG_RAW,
> -             "PPS ntpshm_pps %lu.%03lu @ %lu.%09lu, preci %d\n",
> -             (unsigned long)actual_tv.tv_sec,
> -             (unsigned long)actual_tv.tv_usec,
> +             "ntpshm_put %lu.%09lu @ %lu.%09lu\n",
> +             (unsigned long)actual_ts->tv_sec,
> +             (unsigned long)actual_ts->tv_nsec,
>               (unsigned long)clock_ts->tv_sec,
> -             (unsigned long)clock_ts->tv_nsec,
> -             precision);
> +             (unsigned long)clock_ts->tv_nsec);
>      /address@hidden@*/
>      return 1;
>  }
>  
> +#if defined(PPS_ENABLE)
>  #define SOCK_MAGIC 0x534f434b
>  struct sock_sample {
>      struct timeval tv;
> @@ -466,7 +378,9 @@ static /address@hidden@*/ char *report_hook(struct
> gps_device_t *session, log1 = "accepted chrony sock";
>       chrony_send(session, &td->real, &td->clock, edge_offset);
>      }
> -    (void)ntpshm_pps(session, &td->real, &td->clock);
> +    /* precision is a placebo, ntpd does not really use it
> +     * real world accuracy is around 16uS, thus -16 precision */
> +    (void)ntpshm_put(session, session->shmIndexPPS, &td->real,
> &td->clock, -16); 
>      return log1;
>  }
> @@ -475,11 +389,11 @@ static /address@hidden@*/ char *report_hook(struct
> gps_device_t *session, void ntpshm_link_deactivate(struct
> gps_device_t *session) /* release ntpshm storage for a session */
>  {
> -    (void)ntpshm_free(session->context, session->shmindex);
> +    (void)ntpshm_free(session->context, session->shmIndex);
>  #if defined(PPS_ENABLE)
> -    if (session->shmTimeP != -1) {
> +    if (session->shmIndexPPS != -1) {
>       pps_thread_deactivate(session);
> -     (void)ntpshm_free(session->context, session->shmTimeP);
> +     (void)ntpshm_free(session->context, session->shmIndexPPS);
>      }
>  #endif       /* PPS_ENABLE */
>  }
> @@ -488,9 +402,9 @@ void ntpshm_link_activate(struct gps_device_t
> *session) /* set up ntpshm storage for a session */
>  {
>      /* allocate a shared-memory segment for "NMEA" time data */
> -    session->shmindex = ntpshm_alloc(session->context);
> +    session->shmIndex = ntpshm_alloc(session->context);
>  
> -    if (0 > session->shmindex) {
> +    if (0 > session->shmIndex) {
>       gpsd_report(session->context->debug, LOG_INF, "NTPD
> ntpshm_alloc() failed\n"); #if defined(PPS_ENABLE)
>      } else if (session->sourcetype == source_usb ||
> session->sourcetype == source_rs232) { @@ -498,7 +412,7 @@ void
> ntpshm_link_activate(struct gps_device_t *session)
>        * for the 1pps time data and launch a thread to capture the
> 1pps
>        * transitions
>        */
> -     if ((session->shmTimeP = ntpshm_alloc(session->context)) <
> 0) {
> +     if ((session->shmIndexPPS = ntpshm_alloc(session->context))
> < 0) { gpsd_report(session->context->debug, LOG_INF, "NTPD
> ntpshm_alloc(1) failed\n"); } else {
>           init_hook(session);




RGDS
GARY
---------------------------------------------------------------------------
Gary E. Miller Rellim 109 NW Wilmington Ave., Suite E, Bend, OR 97701
        address@hidden  Tel:+1(541)382-8588

signature.asc
Description: PGP signature