Re: [PATCH v7] posix-timers: add clock_compare system call

[Sagi Maimon]

Hi Thomas

On Sat, Mar 23, 2024 at 2:38 AM Thomas Gleixner <tglx@xxxxxxxxxxxxx> wrote:
>
> Sagi!
>
> On Wed, Mar 20 2024 at 16:42, Sagi Maimon wrote:
> > On Thu, Mar 14, 2024 at 8:08 PM Thomas Gleixner <tglx@xxxxxxxxxxxxx> wrote:
> >> Which is maximaly precise under the assumption that in the time between
> >> the sample points a1 and a2 neither the system clock nor the PCH clocks
> >> are changing their frequency significantly. That is a valid assumption
> >> when you put a reasonable upper bound on d2.
> >>
> >
> > You are right.
> > In fact, we are running this calculation on a user space application.
> > We use the new system call to get pairs of mono and PHC and then run
> > that calculation in user space.
> > That is why the system call returns pairs of clock samples and not the
> > diff between them.
>
> Please stop claiming things which are fundamentally wrong:
>
>   The proposed system call returns the PHC sample and the midpoint of
>   two CLOCK_WHATEVER samples which have been sampled before and after
>   the PHC sample.
>
>   That is fundamentally different from a pair of samples as I explained
>   to you in great length more than once by now.
>
> I understand that you can't rely on the PTP_SYS_OFFSET_PRECISE IOCTL
> alone because there is not much hardware support, but what you are
> proposing is way worse than the other two PTP_SYS_OFFSET variants.
>
> PTP_SYS_OFFSET at least gives the caller a choice of analysis of the
> interleaving system timestamps.
>
> PTP_SYS_OFFSET_EXTENDED moves the outer sample points as close as
> possible to the actual PCH read and provides both outer samples to user
> space for analysis. It was introduced for a reason, no?
>
> Your proposed system call is just declaring arbitrarily that the
> CLOCK_WHATEVER sample is exactly the midpoint of the two outer samples
> and is therefore superior and correct.
>
> It is neither superior nor correct because the midpoint is an
> ill-defined assumption as I explained to you multiple times now.
>
> Aside of that the approach loses the extended information of
> PTP_SYS_OFFSET and PTP_SYS_OFFSET_EXTENDED including the more precise
> sampling behaviour of the latter. IOW, it is ignoring and throwing away
> the effort of people who cared about making the best out of the
> limitations of hardware including the already existing algorithms to
> make sense out of it.
>
> The P at the beginning of PTP does not mean 'Potentially precise' and
> the lack of C in PTP does not mean that Correctness is overrated.
>
> The problem is that these non hardware assisted IOCTL variants sample
> only CLOCK_REALTIME and not CLOCK_MONOTONIC_RAW, which is all you need
> to solve your problem at hand, no?
>
> That's absolutely not rocket science to solve. The below sketch does
> exactly that without creating an ill-defined syscall monstrosity, at the
> same time it is fully preserving the benefits of the existing IOCTL
> variants and therefore allows to apply already existing algorithms to
> analyse that data. That's too simple and too obviously correct, right?
>
> The thing is a sketch and it's neither compiled nor tested. It's just
> for illustration and you can keep all bugs you might find in it.
>
> On top this needs an analyis whether any of the gettimex64()
> implementations does something special instead of invoking the
> ptp_read_system_prets() and ptp_read_system_postts() helpers as close as
> possible to the PCH readout, but that's not rocket science either. It's
> just 21 callbacks to look at.
>
I like your suggestion, thanks!
it is what our user space needs from the kernel and with minimum kernel changes.
I will write it, test it and upload it with your permission (it is you
idea after all).
> It might also require a new set of variant '3' IOTCLS to make that flag
> field work, but that's not going to make the change more complex and
> it's an exercise left to the experts of that IOCTL interface.
>
I think that I understand your meaning.
There is a backward compatibility problem here.
Existing user space application using PTP_SYS_OFFSET_EXTENDED ioctl
won't have any problems
because of the "extoff->rsv[0] || extoff->rsv[1] || extoff->rsv[2]"
test,  but what about all
old user space applications using: PTP_SYS_OFFSET ?
How can it be solved?
Can you explain what you meant above regarding the IOCTL?

Thanks,
Sagi



> Thanks,
>
>         tglx
> ---
>  drivers/ptp/ptp_chardev.c        |   36 ++++++++++++++++++++++--------------
>  include/linux/ptp_clock_kernel.h |   28 +++++++++++++++++++---------
>  include/uapi/linux/ptp_clock.h   |   10 ++++++++--
>  3 files changed, 49 insertions(+), 25 deletions(-)
>
> --- a/drivers/ptp/ptp_chardev.c
> +++ b/drivers/ptp/ptp_chardev.c
> @@ -164,9 +164,9 @@ long ptp_ioctl(struct posix_clock_contex
>         struct ptp_sys_offset_precise precise_offset;
>         struct system_device_crosststamp xtstamp;
>         struct ptp_clock_info *ops = ptp->info;
> +       struct ptp_system_timestamp sts = { };
>         struct ptp_sys_offset *sysoff = NULL;
>         struct timestamp_event_queue *tsevq;
> -       struct ptp_system_timestamp sts;
>         struct ptp_clock_request req;
>         struct ptp_clock_caps caps;
>         struct ptp_clock_time *pct;
> @@ -358,11 +358,13 @@ long ptp_ioctl(struct posix_clock_contex
>                         extoff = NULL;
>                         break;
>                 }
> -               if (extoff->n_samples > PTP_MAX_SAMPLES
> -                   || extoff->rsv[0] || extoff->rsv[1] || extoff->rsv[2]) {
> +               if (!extoff->n_samples || extoff->n_samples > PTP_MAX_SAMPLES ||
> +                   (extoff->flags & ~PTP_SYS_OFFSET_VALID_FLAGS) ||
> +                   extoff->rsv[0] || extoff->rsv[1]) {
>                         err = -EINVAL;
>                         break;
>                 }
> +               sts.flags = extoff->flags;
>                 for (i = 0; i < extoff->n_samples; i++) {
>                         err = ptp->info->gettimex64(ptp->info, &ts, &sts);
>                         if (err)
> @@ -386,29 +388,35 @@ long ptp_ioctl(struct posix_clock_contex
>                         sysoff = NULL;
>                         break;
>                 }
> -               if (sysoff->n_samples > PTP_MAX_SAMPLES) {
> +               if (!sysoff->n_samples || sysoff->n_samples > PTP_MAX_SAMPLES ||
> +                   (sysoff->flags & ~PTP_SYS_OFFSET_VALID_FLAGS) ||
> +                   sysoff->rsv[0] || sysoff->rsv[1]) {
>                         err = -EINVAL;
>                         break;
>                 }
> +               sts.flags = sysoff->flags;
>                 pct = &sysoff->ts[0];
>                 for (i = 0; i < sysoff->n_samples; i++) {
> -                       ktime_get_real_ts64(&ts);
> -                       pct->sec = ts.tv_sec;
> -                       pct->nsec = ts.tv_nsec;
> -                       pct++;
> -                       if (ops->gettimex64)
> -                               err = ops->gettimex64(ops, &ts, NULL);
> -                       else
> +                       if (ops->gettimex64) {
> +                               err = ops->gettimex64(ops, &ts, &sts);
> +                       } else {
> +                               ptp_read_system_prets(&sts);
>                                 err = ops->gettime64(ops, &ts);
> +                       }
>                         if (err)
>                                 goto out;
> +
> +                       pct->sec = sts.pre_ts.tv_sec;
> +                       pct->nsec = sts.pre_ts.tv_nsec;
> +                       pct++;
>                         pct->sec = ts.tv_sec;
>                         pct->nsec = ts.tv_nsec;
>                         pct++;
>                 }
> -               ktime_get_real_ts64(&ts);
> -               pct->sec = ts.tv_sec;
> -               pct->nsec = ts.tv_nsec;
> +               if (!ops->gettimex64)
> +                       ptp_read_system_postts(&sts);
> +               pct->sec = sts.post_ts.tv_sec;
> +               pct->nsec = sts.post_ts.tv_nsec;
>                 if (copy_to_user((void __user *)arg, sysoff, sizeof(*sysoff)))
>                         err = -EFAULT;
>                 break;
> --- a/include/linux/ptp_clock_kernel.h
> +++ b/include/linux/ptp_clock_kernel.h
> @@ -44,13 +44,15 @@ struct ptp_clock_request {
>  struct system_device_crosststamp;
>
>  /**
> - * struct ptp_system_timestamp - system time corresponding to a PHC timestamp
> - * @pre_ts: system timestamp before capturing PHC
> - * @post_ts: system timestamp after capturing PHC
> + * struct ptp_system_timestamp - System time corresponding to a PHC timestamp
> + * @flags:     Flags to select the system clock to sample
> + * @pre_ts:    System timestamp before capturing PHC
> + * @post_ts:   System timestamp after capturing PHC
>   */
>  struct ptp_system_timestamp {
> -       struct timespec64 pre_ts;
> -       struct timespec64 post_ts;
> +       unsigned int            flags;
> +       struct timespec64       pre_ts;
> +       struct timespec64       post_ts;
>  };
>
>  /**
> @@ -457,14 +459,22 @@ static inline ktime_t ptp_convert_timest
>
>  static inline void ptp_read_system_prets(struct ptp_system_timestamp *sts)
>  {
> -       if (sts)
> -               ktime_get_real_ts64(&sts->pre_ts);
> +       if (sts) {
> +               if (sts->flags & PTP_SYS_OFFSET_MONO_RAW)
> +                       ktime_get_raw_ts64(&sts->pre_ts);
> +               else
> +                       ktime_get_real_ts64(&sts->pre_ts);
> +       }
>  }
>
>  static inline void ptp_read_system_postts(struct ptp_system_timestamp *sts)
>  {
> -       if (sts)
> -               ktime_get_real_ts64(&sts->post_ts);
> +       if (sts) {
> +               if (sts->flags & PTP_SYS_OFFSET_MONO_RAW)
> +                       ktime_get_raw_ts64(&sts->post_ts);
> +               else
> +                       ktime_get_real_ts64(&sts->post_ts);
> +       }
>  }
>
>  #endif
> --- a/include/uapi/linux/ptp_clock.h
> +++ b/include/uapi/linux/ptp_clock.h
> @@ -76,6 +76,10 @@
>   */
>  #define PTP_PEROUT_V1_VALID_FLAGS      (0)
>
> +/* Bits for PTP_SYS_OFFSET and PTP_SYS_OFFSET_EXTENDED */
> +#define PTP_SYS_OFFSET_MONO_RAW                (1U << 0)
> +#define PTP_SYS_OFFSET_VALID_FLAGS     (PTP_SYS_OFFSET_MONO_RAW)
> +
>  /*
>   * struct ptp_clock_time - represents a time value
>   *
> @@ -146,7 +150,8 @@ struct ptp_perout_request {
>
>  struct ptp_sys_offset {
>         unsigned int n_samples; /* Desired number of measurements. */
> -       unsigned int rsv[3];    /* Reserved for future use. */
> +       unsigned int flags;
> +       unsigned int rsv[2];    /* Reserved for future use. */
>         /*
>          * Array of interleaved system/phc time stamps. The kernel
>          * will provide 2*n_samples + 1 time stamps, with the last
> @@ -157,7 +162,8 @@ struct ptp_sys_offset {
>
>  struct ptp_sys_offset_extended {
>         unsigned int n_samples; /* Desired number of measurements. */
> -       unsigned int rsv[3];    /* Reserved for future use. */
> +       unsigned int flags;
> +       unsigned int rsv[2];    /* Reserved for future use. */
>         /*
>          * Array of [system, phc, system] time stamps. The kernel will provide
>          * 3*n_samples time stamps.