Message-ID: <878r29hjds.ffs@tglx>
Date: Sat, 23 Mar 2024 01:38:55 +0100
From: Thomas Gleixner <tglx@...utronix.de>
To: Sagi Maimon <maimon.sagi@...il.com>
Cc: richardcochran@...il.com, luto@...nel.org, mingo@...hat.com,
 bp@...en8.de, dave.hansen@...ux.intel.com, x86@...nel.org, hpa@...or.com,
 arnd@...db.de, geert@...ux-m68k.org, peterz@...radead.org,
 hannes@...xchg.org, sohil.mehta@...el.com, rick.p.edgecombe@...el.com,
 nphamcs@...il.com, palmer@...ive.com, keescook@...omium.org,
 legion@...nel.org, mark.rutland@....com, mszeredi@...hat.com,
 casey@...aufler-ca.com, reibax@...il.com, davem@...emloft.net,
 brauner@...nel.org, linux-kernel@...r.kernel.org,
 linux-api@...r.kernel.org, linux-arch@...r.kernel.org,
 netdev@...r.kernel.org
Subject: Re: [PATCH v7] posix-timers: add clock_compare system call

Sagi!

On Wed, Mar 20 2024 at 16:42, Sagi Maimon wrote:
> On Thu, Mar 14, 2024 at 8:08 PM Thomas Gleixner <tglx@...utronix.de> 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.

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.

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.

Powered by blists - more mailing lists