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.
