[PATCHv4 net-next] ptp/ioctl: support MONOTONIC_RAW timestamps for PTP_SYS_OFFSET_EXTENDED

[PATCHv4 net-next] ptp/ioctl: support MONOTONIC_RAW timestamps for PTP_SYS_OFFSET_EXTENDED

[Mahesh Bandewar]

The ability to read the PHC (Physical Hardware Clock) alongside
multiple system clocks is currently dependent on the specific
hardware architecture. This limitation restricts the use of
PTP_SYS_OFFSET_PRECISE to certain hardware configurations.

The generic soultion which would work across all architectures
is to read the PHC along with the latency to perform PHC-read as
offered by PTP_SYS_OFFSET_EXTENDED which provides pre and post
timestamps.  However, these timestamps are currently limited
to the CLOCK_REALTIME timebase. Since CLOCK_REALTIME is affected
by NTP (or similar time synchronization services), it can
experience significant jumps forward or backward. This hinders
the precise latency measurements that PTP_SYS_OFFSET_EXTENDED
is designed to provide.

This problem could be addressed by supporting MONOTONIC_RAW
timestamps within PTP_SYS_OFFSET_EXTENDED. Unlike CLOCK_REALTIME
or CLOCK_MONOTONIC, the MONOTONIC_RAW timebase is unaffected
by NTP adjustments.

This enhancement can be implemented by utilizing one of the three
reserved words within the PTP_SYS_OFFSET_EXTENDED struct to pass
the clock-id for timestamps.  The current behavior aligns with
clock-id for CLOCK_REALTIME timebase (value of 0), ensuring
backward compatibility of the UAPI.

Signed-off-by: Mahesh Bandewar <maheshb@google.com>
---
v1 -> v2
   * Code-style fixes.
v2 -> v3
   * Reword commit log
   * Fix the compilation issue by using __kernel_clockid instead of clockid_t
     which has kernel only scope.
v3 -> v4
   * Typo/comment fixes.

 drivers/ptp/ptp_chardev.c        |  7 +++++--
 include/linux/ptp_clock_kernel.h | 30 ++++++++++++++++++++++++++----
 include/uapi/linux/ptp_clock.h   | 27 +++++++++++++++++++++------
 3 files changed, 52 insertions(+), 12 deletions(-)

diff --git a/drivers/ptp/ptp_chardev.c b/drivers/ptp/ptp_chardev.c
index 7513018c9f9a..c109109c9e8e 100644
--- a/drivers/ptp/ptp_chardev.c
+++ b/drivers/ptp/ptp_chardev.c
@@ -358,11 +358,14 @@ long ptp_ioctl(struct posix_clock_context *pccontext, unsigned int cmd,
 			extoff = NULL;
 			break;
 		}
-		if (extoff->n_samples > PTP_MAX_SAMPLES
-		    || extoff->rsv[0] || extoff->rsv[1] || extoff->rsv[2]) {
+		if (extoff->n_samples > PTP_MAX_SAMPLES ||
+		    extoff->rsv[0] || extoff->rsv[1] ||
+		    (extoff->clockid != CLOCK_REALTIME &&
+		     extoff->clockid != CLOCK_MONOTONIC_RAW)) {
 			err = -EINVAL;
 			break;
 		}
+		sts.clockid = extoff->clockid;
 		for (i = 0; i < extoff->n_samples; i++) {
 			err = ptp->info->gettimex64(ptp->info, &ts, &sts);
 			if (err)
diff --git a/include/linux/ptp_clock_kernel.h b/include/linux/ptp_clock_kernel.h
index 6e4b8206c7d0..74ded5f95d95 100644
--- a/include/linux/ptp_clock_kernel.h
+++ b/include/linux/ptp_clock_kernel.h
@@ -47,10 +47,12 @@ 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
+ * @clockid: clock-base used for capturing the system timestamps
  */
 struct ptp_system_timestamp {
 	struct timespec64 pre_ts;
 	struct timespec64 post_ts;
+	clockid_t clockid;
 };
 
 /**
@@ -457,14 +459,34 @@ static inline ktime_t ptp_convert_timestamp(const ktime_t *hwtstamp,
 
 static inline void ptp_read_system_prets(struct ptp_system_timestamp *sts)
 {
-	if (sts)
-		ktime_get_real_ts64(&sts->pre_ts);
+	if (sts) {
+		switch (sts->clockid) {
+		case CLOCK_REALTIME:
+			ktime_get_real_ts64(&sts->pre_ts);
+			break;
+		case CLOCK_MONOTONIC_RAW:
+			ktime_get_raw_ts64(&sts->pre_ts);
+			break;
+		default:
+			break;
+		}
+	}
 }
 
 static inline void ptp_read_system_postts(struct ptp_system_timestamp *sts)
 {
-	if (sts)
-		ktime_get_real_ts64(&sts->post_ts);
+	if (sts) {
+		switch (sts->clockid) {
+		case CLOCK_REALTIME:
+			ktime_get_real_ts64(&sts->post_ts);
+			break;
+		case CLOCK_MONOTONIC_RAW:
+			ktime_get_raw_ts64(&sts->post_ts);
+			break;
+		default:
+			break;
+		}
+	}
 }
 
 #endif
diff --git a/include/uapi/linux/ptp_clock.h b/include/uapi/linux/ptp_clock.h
index 053b40d642de..5e3d70fbc499 100644
--- a/include/uapi/linux/ptp_clock.h
+++ b/include/uapi/linux/ptp_clock.h
@@ -155,13 +155,28 @@ struct ptp_sys_offset {
 	struct ptp_clock_time ts[2 * PTP_MAX_SAMPLES + 1];
 };
 
+/*
+ * ptp_sys_offset_extended - data structure for IOCTL operation
+ *			     PTP_SYS_OFFSET_EXTENDED
+ *
+ * @n_samples:	Desired number of measurements.
+ * @clockid:	clockid of a clock-base used for pre/post timestamps.
+ * @rsv:	Reserved for future use.
+ * @ts:		Array of samples in the form [pre-TS, PHC, post-TS]. The
+ *		kernel provides @n_samples.
+ *
+ * History:
+ * v1: Initial implementation.
+ *
+ * v2: Use the first word of the reserved-field for @clockid. That's
+ *     backward compatible since v1 expects all three reserved words
+ *     (@rsv[3]) to be 0 while the clockid (first word in v2) for
+ *     CLOCK_REALTIME is '0'.
+ */
 struct ptp_sys_offset_extended {
-	unsigned int n_samples; /* Desired number of measurements. */
-	unsigned int rsv[3];    /* Reserved for future use. */
-	/*
-	 * Array of [system, phc, system] time stamps. The kernel will provide
-	 * 3*n_samples time stamps.
-	 */
+	unsigned int n_samples;
+	__kernel_clockid_t clockid;
+	unsigned int rsv[2];
 	struct ptp_clock_time ts[PTP_MAX_SAMPLES][3];
 };
 
-- 
2.45.0.rc1.225.g2a3ae87e7f-goog

Re: [PATCHv4 net-next] ptp/ioctl: support MONOTONIC_RAW timestamps for PTP_SYS_OFFSET_EXTENDED

[Paolo Abeni]

On Thu, 2024-05-02 at 14:10 -0700, Mahesh Bandewar wrote:
> The ability to read the PHC (Physical Hardware Clock) alongside
> multiple system clocks is currently dependent on the specific
> hardware architecture. This limitation restricts the use of
> PTP_SYS_OFFSET_PRECISE to certain hardware configurations.
> 
> The generic soultion which would work across all architectures
> is to read the PHC along with the latency to perform PHC-read as
> offered by PTP_SYS_OFFSET_EXTENDED which provides pre and post
> timestamps.  However, these timestamps are currently limited
> to the CLOCK_REALTIME timebase. Since CLOCK_REALTIME is affected
> by NTP (or similar time synchronization services), it can
> experience significant jumps forward or backward. This hinders
> the precise latency measurements that PTP_SYS_OFFSET_EXTENDED
> is designed to provide.
> 
> This problem could be addressed by supporting MONOTONIC_RAW
> timestamps within PTP_SYS_OFFSET_EXTENDED. Unlike CLOCK_REALTIME
> or CLOCK_MONOTONIC, the MONOTONIC_RAW timebase is unaffected
> by NTP adjustments.
> 
> This enhancement can be implemented by utilizing one of the three
> reserved words within the PTP_SYS_OFFSET_EXTENDED struct to pass
> the clock-id for timestamps.  The current behavior aligns with
> clock-id for CLOCK_REALTIME timebase (value of 0), ensuring
> backward compatibility of the UAPI.
> 
> Signed-off-by: Mahesh Bandewar <maheshb@google.com>

LGTM, but let's wait a bit for Richard ack.

@Richard, could you please have look?

Paolo

Re: [PATCHv4 net-next] ptp/ioctl: support MONOTONIC_RAW timestamps for PTP_SYS_OFFSET_EXTENDED

[Richard Cochran]

On Thu, May 02, 2024 at 02:10:47PM -0700, Mahesh Bandewar wrote:

> @@ -457,14 +459,34 @@ static inline ktime_t ptp_convert_timestamp(const ktime_t *hwtstamp,
>  
>  static inline void ptp_read_system_prets(struct ptp_system_timestamp *sts)
>  {
> -	if (sts)
> -		ktime_get_real_ts64(&sts->pre_ts);
> +	if (sts) {
> +		switch (sts->clockid) {
> +		case CLOCK_REALTIME:
> +			ktime_get_real_ts64(&sts->pre_ts);
> +			break;
> +		case CLOCK_MONOTONIC_RAW:
> +			ktime_get_raw_ts64(&sts->pre_ts);
> +			break;

Why not add CLOCK_MONOTONIC as well?
That would be useful in many cases.

> +/*
> + * ptp_sys_offset_extended - data structure for IOCTL operation
> + *			     PTP_SYS_OFFSET_EXTENDED
> + *
> + * @n_samples:	Desired number of measurements.
> + * @clockid:	clockid of a clock-base used for pre/post timestamps.
> + * @rsv:	Reserved for future use.
> + * @ts:		Array of samples in the form [pre-TS, PHC, post-TS]. The
> + *		kernel provides @n_samples.
> + *
> + * History:
> + * v1: Initial implementation.
> + *
> + * v2: Use the first word of the reserved-field for @clockid. That's
> + *     backward compatible since v1 expects all three reserved words
> + *     (@rsv[3]) to be 0 while the clockid (first word in v2) for
> + *     CLOCK_REALTIME is '0'.

This is not really appropriate for a source code comment.  The
un-merged patch series iterations are preserved at lore.kernel in case
someone needs that.

The "backward compatible" information really wants to be in the commit
message.

Thanks,
Richard

Re: [PATCHv4 net-next] ptp/ioctl: support MONOTONIC_RAW timestamps for PTP_SYS_OFFSET_EXTENDED

[Thomas Gleixner]

On Tue, May 07 2024 at 21:44, Richard Cochran wrote:
> On Thu, May 02, 2024 at 02:10:47PM -0700, Mahesh Bandewar wrote:
>> +/*
>> + * ptp_sys_offset_extended - data structure for IOCTL operation
>> + *			     PTP_SYS_OFFSET_EXTENDED
>> + *
>> + * @n_samples:	Desired number of measurements.
>> + * @clockid:	clockid of a clock-base used for pre/post timestamps.
>> + * @rsv:	Reserved for future use.
>> + * @ts:		Array of samples in the form [pre-TS, PHC, post-TS]. The
>> + *		kernel provides @n_samples.
>> + *
>> + * History:
>> + * v1: Initial implementation.
>> + *
>> + * v2: Use the first word of the reserved-field for @clockid. That's
>> + *     backward compatible since v1 expects all three reserved words
>> + *     (@rsv[3]) to be 0 while the clockid (first word in v2) for
>> + *     CLOCK_REALTIME is '0'.
>
> This is not really appropriate for a source code comment.  The
> un-merged patch series iterations are preserved at lore.kernel in case
> someone needs that.
>
> The "backward compatible" information really wants to be in the commit
> message.

I agree that it wants to be in the commit message, but having the
version information in the kernel-doc which describes the UAPI is
sensible and useful. That's where I'd look first and asking a user to
dig up this information on lore is not really helpful.

Thanks,

        tglx

Re: [PATCHv4 net-next] ptp/ioctl: support MONOTONIC_RAW timestamps for PTP_SYS_OFFSET_EXTENDED

[Richard Cochran]

Thomas,

On Wed, May 08, 2024 at 09:38:58AM +0200, Thomas Gleixner wrote:
> On Tue, May 07 2024 at 21:44, Richard Cochran wrote:
> > On Thu, May 02, 2024 at 02:10:47PM -0700, Mahesh Bandewar wrote:
> >> + * History:
> >> + * v1: Initial implementation.
> >> + *
> >> + * v2: Use the first word of the reserved-field for @clockid. That's
> >> + *     backward compatible since v1 expects all three reserved words
> >> + *     (@rsv[3]) to be 0 while the clockid (first word in v2) for
> >> + *     CLOCK_REALTIME is '0'.

...

> I agree that it wants to be in the commit message, but having the
> version information in the kernel-doc which describes the UAPI is
> sensible and useful. That's where I'd look first and asking a user to
> dig up this information on lore is not really helpful.

But writing "v1, v2" doesn't make sense for this code.  There never
was a "v1" for this ioctl.  At the very least, the change should be
identified by kernel version (or git SHA).

Thanks,
Richard

Re: [PATCHv4 net-next] ptp/ioctl: support MONOTONIC_RAW timestamps for PTP_SYS_OFFSET_EXTENDED

[Thomas Gleixner]

On Thu, May 09 2024 at 21:07, Richard Cochran wrote:

> Thomas,
>
> On Wed, May 08, 2024 at 09:38:58AM +0200, Thomas Gleixner wrote:
>> On Tue, May 07 2024 at 21:44, Richard Cochran wrote:
>> > On Thu, May 02, 2024 at 02:10:47PM -0700, Mahesh Bandewar wrote:
>> >> + * History:
>> >> + * v1: Initial implementation.
>> >> + *
>> >> + * v2: Use the first word of the reserved-field for @clockid. That's
>> >> + *     backward compatible since v1 expects all three reserved words
>> >> + *     (@rsv[3]) to be 0 while the clockid (first word in v2) for
>> >> + *     CLOCK_REALTIME is '0'.
>
> ..
>
>> I agree that it wants to be in the commit message, but having the
>> version information in the kernel-doc which describes the UAPI is
>> sensible and useful. That's where I'd look first and asking a user to
>> dig up this information on lore is not really helpful.
>
> But writing "v1, v2" doesn't make sense for this code.  There never
> was a "v1" for this ioctl.  At the very least, the change should be
> identified by kernel version (or git SHA).

Adding the git SHA before committing the change is going to be
challenging :)

Re: [PATCHv4 net-next] ptp/ioctl: support MONOTONIC_RAW timestamps for PTP_SYS_OFFSET_EXTENDED

[Mahesh Bandewar (महेश बंडेवार)]

On Fri, May 10, 2024 at 12:50 AM Thomas Gleixner <tglx@linutronix.de> wrote:
>
> On Thu, May 09 2024 at 21:07, Richard Cochran wrote:
>
> > Thomas,
> >
> > On Wed, May 08, 2024 at 09:38:58AM +0200, Thomas Gleixner wrote:
> >> On Tue, May 07 2024 at 21:44, Richard Cochran wrote:
> >> > On Thu, May 02, 2024 at 02:10:47PM -0700, Mahesh Bandewar wrote:
> >> >> + * History:
> >> >> + * v1: Initial implementation.
> >> >> + *
> >> >> + * v2: Use the first word of the reserved-field for @clockid. That's
> >> >> + *     backward compatible since v1 expects all three reserved words
> >> >> + *     (@rsv[3]) to be 0 while the clockid (first word in v2) for
> >> >> + *     CLOCK_REALTIME is '0'.
> >
> > ..
> >
> >> I agree that it wants to be in the commit message, but having the
> >> version information in the kernel-doc which describes the UAPI is
> >> sensible and useful. That's where I'd look first and asking a user to
> >> dig up this information on lore is not really helpful.
> >
> > But writing "v1, v2" doesn't make sense for this code.  There never
> > was a "v1" for this ioctl.  At the very least, the change should be
> > identified by kernel version (or git SHA).
>
> Adding the git SHA before committing the change is going to be
> challenging :)

Instead of v1/v2, how about I can make it 'prior to kernel 6.10' and
'from 6.10 onwards' etc. (as Richard proposed)?

Re: [PATCHv4 net-next] ptp/ioctl: support MONOTONIC_RAW timestamps for PTP_SYS_OFFSET_EXTENDED

[Thomas Gleixner]

On Fri, May 10 2024 at 09:45, Mahesh Bandewar (महेश बंडेवार) wrote:

> On Fri, May 10, 2024 at 12:50 AM Thomas Gleixner <tglx@linutronix.de> wrote:
>>
>> On Thu, May 09 2024 at 21:07, Richard Cochran wrote:
>>
>> > Thomas,
>> >
>> > On Wed, May 08, 2024 at 09:38:58AM +0200, Thomas Gleixner wrote:
>> >> On Tue, May 07 2024 at 21:44, Richard Cochran wrote:
>> >> > On Thu, May 02, 2024 at 02:10:47PM -0700, Mahesh Bandewar wrote:
>> >> >> + * History:
>> >> >> + * v1: Initial implementation.
>> >> >> + *
>> >> >> + * v2: Use the first word of the reserved-field for @clockid. That's
>> >> >> + *     backward compatible since v1 expects all three reserved words
>> >> >> + *     (@rsv[3]) to be 0 while the clockid (first word in v2) for
>> >> >> + *     CLOCK_REALTIME is '0'.
>> >
>> > ..
>> >
>> >> I agree that it wants to be in the commit message, but having the
>> >> version information in the kernel-doc which describes the UAPI is
>> >> sensible and useful. That's where I'd look first and asking a user to
>> >> dig up this information on lore is not really helpful.
>> >
>> > But writing "v1, v2" doesn't make sense for this code.  There never
>> > was a "v1" for this ioctl.  At the very least, the change should be
>> > identified by kernel version (or git SHA).
>>
>> Adding the git SHA before committing the change is going to be
>> challenging :)
>
> Instead of v1/v2, how about I can make it 'prior to kernel 6.10' and
> 'from 6.10 onwards' etc. (as Richard proposed)?

Sure

Re: [PATCHv4 net-next] ptp/ioctl: support MONOTONIC_RAW timestamps for PTP_SYS_OFFSET_EXTENDED

[Mahesh Bandewar (महेश बंडेवार)]

On Tue, May 7, 2024 at 9:44 PM Richard Cochran <richardcochran@gmail.com> wrote:
>
> On Thu, May 02, 2024 at 02:10:47PM -0700, Mahesh Bandewar wrote:
>
> > @@ -457,14 +459,34 @@ static inline ktime_t ptp_convert_timestamp(const ktime_t *hwtstamp,
> >
> >  static inline void ptp_read_system_prets(struct ptp_system_timestamp *sts)
> >  {
> > -     if (sts)
> > -             ktime_get_real_ts64(&sts->pre_ts);
> > +     if (sts) {
> > +             switch (sts->clockid) {
> > +             case CLOCK_REALTIME:
> > +                     ktime_get_real_ts64(&sts->pre_ts);
> > +                     break;
> > +             case CLOCK_MONOTONIC_RAW:
> > +                     ktime_get_raw_ts64(&sts->pre_ts);
> > +                     break;
>
> Why not add CLOCK_MONOTONIC as well?
> That would be useful in many cases.
>
In fact my original implementation had it but my use case is really
CLOCK_MONOTONIC_RAW, however, the general opinion on the thread was to
implement what is needed now and if someone needs (CLOCK_MONOTONIC),
it can be added at that time. So I removed it.

> > +/*
> > + * ptp_sys_offset_extended - data structure for IOCTL operation
> > + *                        PTP_SYS_OFFSET_EXTENDED
> > + *
> > + * @n_samples:       Desired number of measurements.
> > + * @clockid: clockid of a clock-base used for pre/post timestamps.
> > + * @rsv:     Reserved for future use.
> > + * @ts:              Array of samples in the form [pre-TS, PHC, post-TS]. The
> > + *           kernel provides @n_samples.
> > + *
> > + * History:
> > + * v1: Initial implementation.
> > + *
> > + * v2: Use the first word of the reserved-field for @clockid. That's
> > + *     backward compatible since v1 expects all three reserved words
> > + *     (@rsv[3]) to be 0 while the clockid (first word in v2) for
> > + *     CLOCK_REALTIME is '0'.
>
> This is not really appropriate for a source code comment.  The
> un-merged patch series iterations are preserved at lore.kernel in case
> someone needs that.
>
This was added in rev3
(Also this is the API version-history which intends to track how the
fields have changed / morphed and not to be confused with the patch
versions)

> The "backward compatible" information really wants to be in the commit
> message.
>
I have the last paragraph in the commit log about compatibility.


Thanks for the comments,
--mahesh..

> Thanks,
> Richard
>

Re: [PATCHv4 net-next] ptp/ioctl: support MONOTONIC_RAW timestamps for PTP_SYS_OFFSET_EXTENDED

[Thomas Gleixner]

On Thu, May 02 2024 at 14:10, Mahesh Bandewar wrote:
> The ability to read the PHC (Physical Hardware Clock) alongside
> multiple system clocks is currently dependent on the specific
> hardware architecture. This limitation restricts the use of
> PTP_SYS_OFFSET_PRECISE to certain hardware configurations.
>
> The generic soultion which would work across all architectures
> is to read the PHC along with the latency to perform PHC-read as
> offered by PTP_SYS_OFFSET_EXTENDED which provides pre and post
> timestamps.  However, these timestamps are currently limited
> to the CLOCK_REALTIME timebase. Since CLOCK_REALTIME is affected
> by NTP (or similar time synchronization services), it can
> experience significant jumps forward or backward. This hinders
> the precise latency measurements that PTP_SYS_OFFSET_EXTENDED
> is designed to provide.

This is really a handwavy argument.

Fact is that the time jumps of CLOCK_REALTIME caused by NTP (etc) are
rare and significant enough to be easily filtered out. That's why this
interface allows you to retrieve more than one sample.

Can you please explain which problem you are actually trying to solve?

It can't be PTP system time synchronization as that obviously requires
CLOCK_REALTIME.

Thanks,

        tglx

Re: [PATCHv4 net-next] ptp/ioctl: support MONOTONIC_RAW timestamps for PTP_SYS_OFFSET_EXTENDED

[Mahesh Bandewar (महेश बंडेवार)]

On Wed, May 8, 2024 at 12:35 AM Thomas Gleixner <tglx@linutronix.de> wrote:
>
> On Thu, May 02 2024 at 14:10, Mahesh Bandewar wrote:
> > The ability to read the PHC (Physical Hardware Clock) alongside
> > multiple system clocks is currently dependent on the specific
> > hardware architecture. This limitation restricts the use of
> > PTP_SYS_OFFSET_PRECISE to certain hardware configurations.
> >
> > The generic soultion which would work across all architectures
> > is to read the PHC along with the latency to perform PHC-read as
> > offered by PTP_SYS_OFFSET_EXTENDED which provides pre and post
> > timestamps.  However, these timestamps are currently limited
> > to the CLOCK_REALTIME timebase. Since CLOCK_REALTIME is affected
> > by NTP (or similar time synchronization services), it can
> > experience significant jumps forward or backward. This hinders
> > the precise latency measurements that PTP_SYS_OFFSET_EXTENDED
> > is designed to provide.
>
> This is really a handwavy argument.
>
> Fact is that the time jumps of CLOCK_REALTIME caused by NTP (etc) are
> rare and significant enough to be easily filtered out. That's why this
> interface allows you to retrieve more than one sample.
>
> Can you please explain which problem you are actually trying to solve?
>
> It can't be PTP system time synchronization as that obviously requires
> CLOCK_REALTIME.
>
Let me add a couple of folks from the clock team. @Yuliang Li  @Don Hatchett
I'm just a nomad-kernel-net guy trying to fill-in gaps :(

> Thanks,
>
>         tglx

Re: [PATCHv4 net-next] ptp/ioctl: support MONOTONIC_RAW timestamps for PTP_SYS_OFFSET_EXTENDED

[Yuliang Li]

Thank you Mahesh. Please see my answers below.

The mono_raw allows PHC to correlate with a non-adjusted time. This
enables other types of clock sync algorithms to be developed.
For example, if we want to measure the drift rate between CPU
oscillator and PHC. We could run a linear regression over multiple
pairs of <phc, sys>. But if sys time itself is being adjusted (e.g.,
clock_realtime), the linear regression is running over a polyline
hence less effective. With mono_raw, linear regression truly measures
the drift rate of the CPU oscillator.
This capability allows more types of clock sync algorithms to be developed.

Thanks,
Yuliang


On Wed, May 8, 2024 at 7:54 PM Mahesh Bandewar (महेश बंडेवार)
<maheshb@google.com> wrote:
>
> On Wed, May 8, 2024 at 12:35 AM Thomas Gleixner <tglx@linutronix.de> wrote:
> >
> > On Thu, May 02 2024 at 14:10, Mahesh Bandewar wrote:
> > > The ability to read the PHC (Physical Hardware Clock) alongside
> > > multiple system clocks is currently dependent on the specific
> > > hardware architecture. This limitation restricts the use of
> > > PTP_SYS_OFFSET_PRECISE to certain hardware configurations.
> > >
> > > The generic soultion which would work across all architectures
> > > is to read the PHC along with the latency to perform PHC-read as
> > > offered by PTP_SYS_OFFSET_EXTENDED which provides pre and post
> > > timestamps.  However, these timestamps are currently limited
> > > to the CLOCK_REALTIME timebase. Since CLOCK_REALTIME is affected
> > > by NTP (or similar time synchronization services), it can
> > > experience significant jumps forward or backward. This hinders
> > > the precise latency measurements that PTP_SYS_OFFSET_EXTENDED
> > > is designed to provide.
> >
> > This is really a handwavy argument.
> >
> > Fact is that the time jumps of CLOCK_REALTIME caused by NTP (etc) are
> > rare and significant enough to be easily filtered out. That's why this
> > interface allows you to retrieve more than one sample.
> >
> > Can you please explain which problem you are actually trying to solve?
> >
> > It can't be PTP system time synchronization as that obviously requires
> > CLOCK_REALTIME.
> >
> Let me add a couple of folks from the clock team. @Yuliang Li  @Don Hatchett
> I'm just a nomad-kernel-net guy trying to fill-in gaps :(
>
> > Thanks,
> >
> >         tglx

Re: [PATCHv4 net-next] ptp/ioctl: support MONOTONIC_RAW timestamps for PTP_SYS_OFFSET_EXTENDED

[Vadim Fedorenko]

On 02/05/2024 22:10, Mahesh Bandewar wrote:
> The ability to read the PHC (Physical Hardware Clock) alongside
> multiple system clocks is currently dependent on the specific
> hardware architecture. This limitation restricts the use of
> PTP_SYS_OFFSET_PRECISE to certain hardware configurations.
> 
> The generic soultion which would work across all architectures
> is to read the PHC along with the latency to perform PHC-read as
> offered by PTP_SYS_OFFSET_EXTENDED which provides pre and post
> timestamps.  However, these timestamps are currently limited
> to the CLOCK_REALTIME timebase. Since CLOCK_REALTIME is affected
> by NTP (or similar time synchronization services), it can
> experience significant jumps forward or backward. This hinders
> the precise latency measurements that PTP_SYS_OFFSET_EXTENDED
> is designed to provide.
> 
> This problem could be addressed by supporting MONOTONIC_RAW
> timestamps within PTP_SYS_OFFSET_EXTENDED. Unlike CLOCK_REALTIME
> or CLOCK_MONOTONIC, the MONOTONIC_RAW timebase is unaffected
> by NTP adjustments.
> 
> This enhancement can be implemented by utilizing one of the three
> reserved words within the PTP_SYS_OFFSET_EXTENDED struct to pass
> the clock-id for timestamps.  The current behavior aligns with
> clock-id for CLOCK_REALTIME timebase (value of 0), ensuring
> backward compatibility of the UAPI.
> 
> Signed-off-by: Mahesh Bandewar <maheshb@google.com>

Hi Manesh!

Any chance there will be v5 soon with the comments addressed?
I don't see any strong objections to the patch, and it looks
like we can benefit from having MONOTONIC_RAW clock for our
project too.

Thanks,
Vadim

> ---
> v1 -> v2
>     * Code-style fixes.
> v2 -> v3
>     * Reword commit log
>     * Fix the compilation issue by using __kernel_clockid instead of clockid_t
>       which has kernel only scope.
> v3 -> v4
>     * Typo/comment fixes.
> 
>   drivers/ptp/ptp_chardev.c        |  7 +++++--
>   include/linux/ptp_clock_kernel.h | 30 ++++++++++++++++++++++++++----
>   include/uapi/linux/ptp_clock.h   | 27 +++++++++++++++++++++------
>   3 files changed, 52 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/ptp/ptp_chardev.c b/drivers/ptp/ptp_chardev.c
> index 7513018c9f9a..c109109c9e8e 100644
> --- a/drivers/ptp/ptp_chardev.c
> +++ b/drivers/ptp/ptp_chardev.c
> @@ -358,11 +358,14 @@ long ptp_ioctl(struct posix_clock_context *pccontext, unsigned int cmd,
>   			extoff = NULL;
>   			break;
>   		}
> -		if (extoff->n_samples > PTP_MAX_SAMPLES
> -		    || extoff->rsv[0] || extoff->rsv[1] || extoff->rsv[2]) {
> +		if (extoff->n_samples > PTP_MAX_SAMPLES ||
> +		    extoff->rsv[0] || extoff->rsv[1] ||
> +		    (extoff->clockid != CLOCK_REALTIME &&
> +		     extoff->clockid != CLOCK_MONOTONIC_RAW)) {
>   			err = -EINVAL;
>   			break;
>   		}
> +		sts.clockid = extoff->clockid;
>   		for (i = 0; i < extoff->n_samples; i++) {
>   			err = ptp->info->gettimex64(ptp->info, &ts, &sts);
>   			if (err)
> diff --git a/include/linux/ptp_clock_kernel.h b/include/linux/ptp_clock_kernel.h
> index 6e4b8206c7d0..74ded5f95d95 100644
> --- a/include/linux/ptp_clock_kernel.h
> +++ b/include/linux/ptp_clock_kernel.h
> @@ -47,10 +47,12 @@ 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
> + * @clockid: clock-base used for capturing the system timestamps
>    */
>   struct ptp_system_timestamp {
>   	struct timespec64 pre_ts;
>   	struct timespec64 post_ts;
> +	clockid_t clockid;
>   };
>   
>   /**
> @@ -457,14 +459,34 @@ static inline ktime_t ptp_convert_timestamp(const ktime_t *hwtstamp,
>   
>   static inline void ptp_read_system_prets(struct ptp_system_timestamp *sts)
>   {
> -	if (sts)
> -		ktime_get_real_ts64(&sts->pre_ts);
> +	if (sts) {
> +		switch (sts->clockid) {
> +		case CLOCK_REALTIME:
> +			ktime_get_real_ts64(&sts->pre_ts);
> +			break;
> +		case CLOCK_MONOTONIC_RAW:
> +			ktime_get_raw_ts64(&sts->pre_ts);
> +			break;
> +		default:
> +			break;
> +		}
> +	}
>   }
>   
>   static inline void ptp_read_system_postts(struct ptp_system_timestamp *sts)
>   {
> -	if (sts)
> -		ktime_get_real_ts64(&sts->post_ts);
> +	if (sts) {
> +		switch (sts->clockid) {
> +		case CLOCK_REALTIME:
> +			ktime_get_real_ts64(&sts->post_ts);
> +			break;
> +		case CLOCK_MONOTONIC_RAW:
> +			ktime_get_raw_ts64(&sts->post_ts);
> +			break;
> +		default:
> +			break;
> +		}
> +	}
>   }
>   
>   #endif
> diff --git a/include/uapi/linux/ptp_clock.h b/include/uapi/linux/ptp_clock.h
> index 053b40d642de..5e3d70fbc499 100644
> --- a/include/uapi/linux/ptp_clock.h
> +++ b/include/uapi/linux/ptp_clock.h
> @@ -155,13 +155,28 @@ struct ptp_sys_offset {
>   	struct ptp_clock_time ts[2 * PTP_MAX_SAMPLES + 1];
>   };
>   
> +/*
> + * ptp_sys_offset_extended - data structure for IOCTL operation
> + *			     PTP_SYS_OFFSET_EXTENDED
> + *
> + * @n_samples:	Desired number of measurements.
> + * @clockid:	clockid of a clock-base used for pre/post timestamps.
> + * @rsv:	Reserved for future use.
> + * @ts:		Array of samples in the form [pre-TS, PHC, post-TS]. The
> + *		kernel provides @n_samples.
> + *
> + * History:
> + * v1: Initial implementation.
> + *
> + * v2: Use the first word of the reserved-field for @clockid. That's
> + *     backward compatible since v1 expects all three reserved words
> + *     (@rsv[3]) to be 0 while the clockid (first word in v2) for
> + *     CLOCK_REALTIME is '0'.
> + */
>   struct ptp_sys_offset_extended {
> -	unsigned int n_samples; /* Desired number of measurements. */
> -	unsigned int rsv[3];    /* Reserved for future use. */
> -	/*
> -	 * Array of [system, phc, system] time stamps. The kernel will provide
> -	 * 3*n_samples time stamps.
> -	 */
> +	unsigned int n_samples;
> +	__kernel_clockid_t clockid;
> +	unsigned int rsv[2];
>   	struct ptp_clock_time ts[PTP_MAX_SAMPLES][3];
>   };
>   

Re: [PATCHv4 net-next] ptp/ioctl: support MONOTONIC_RAW timestamps for PTP_SYS_OFFSET_EXTENDED

[Vadim Fedorenko]

On 22/08/2024 13:26, Vadim Fedorenko wrote:
> On 02/05/2024 22:10, Mahesh Bandewar wrote:
>> The ability to read the PHC (Physical Hardware Clock) alongside
>> multiple system clocks is currently dependent on the specific
>> hardware architecture. This limitation restricts the use of
>> PTP_SYS_OFFSET_PRECISE to certain hardware configurations.
>>
>> The generic soultion which would work across all architectures
>> is to read the PHC along with the latency to perform PHC-read as
>> offered by PTP_SYS_OFFSET_EXTENDED which provides pre and post
>> timestamps.  However, these timestamps are currently limited
>> to the CLOCK_REALTIME timebase. Since CLOCK_REALTIME is affected
>> by NTP (or similar time synchronization services), it can
>> experience significant jumps forward or backward. This hinders
>> the precise latency measurements that PTP_SYS_OFFSET_EXTENDED
>> is designed to provide.
>>
>> This problem could be addressed by supporting MONOTONIC_RAW
>> timestamps within PTP_SYS_OFFSET_EXTENDED. Unlike CLOCK_REALTIME
>> or CLOCK_MONOTONIC, the MONOTONIC_RAW timebase is unaffected
>> by NTP adjustments.
>>
>> This enhancement can be implemented by utilizing one of the three
>> reserved words within the PTP_SYS_OFFSET_EXTENDED struct to pass
>> the clock-id for timestamps.  The current behavior aligns with
>> clock-id for CLOCK_REALTIME timebase (value of 0), ensuring
>> backward compatibility of the UAPI.
>>
>> Signed-off-by: Mahesh Bandewar <maheshb@google.com>
> 
> Hi Mahesh!
> 
> Any chance there will be v5 soon with the comments addressed?
> I don't see any strong objections to the patch, and it looks
> like we can benefit from having MONOTONIC_RAW clock for our
> project too.
> 
> Thanks,
> Vadim
> 

Hi Mahesh!

It's a gentle ping mail...
If you don't have cycles to work on this patch anymore I can jump in and
address the feedback to finally merge the feature.

Thanks,
Vadim



>> ---
>> v1 -> v2
>>     * Code-style fixes.
>> v2 -> v3
>>     * Reword commit log
>>     * Fix the compilation issue by using __kernel_clockid instead of 
>> clockid_t
>>       which has kernel only scope.
>> v3 -> v4
>>     * Typo/comment fixes.
>>
>>   drivers/ptp/ptp_chardev.c        |  7 +++++--
>>   include/linux/ptp_clock_kernel.h | 30 ++++++++++++++++++++++++++----
>>   include/uapi/linux/ptp_clock.h   | 27 +++++++++++++++++++++------
>>   3 files changed, 52 insertions(+), 12 deletions(-)
>>
>> diff --git a/drivers/ptp/ptp_chardev.c b/drivers/ptp/ptp_chardev.c
>> index 7513018c9f9a..c109109c9e8e 100644
>> --- a/drivers/ptp/ptp_chardev.c
>> +++ b/drivers/ptp/ptp_chardev.c
>> @@ -358,11 +358,14 @@ long ptp_ioctl(struct posix_clock_context 
>> *pccontext, unsigned int cmd,
>>               extoff = NULL;
>>               break;
>>           }
>> -        if (extoff->n_samples > PTP_MAX_SAMPLES
>> -            || extoff->rsv[0] || extoff->rsv[1] || extoff->rsv[2]) {
>> +        if (extoff->n_samples > PTP_MAX_SAMPLES ||
>> +            extoff->rsv[0] || extoff->rsv[1] ||
>> +            (extoff->clockid != CLOCK_REALTIME &&
>> +             extoff->clockid != CLOCK_MONOTONIC_RAW)) {
>>               err = -EINVAL;
>>               break;
>>           }
>> +        sts.clockid = extoff->clockid;
>>           for (i = 0; i < extoff->n_samples; i++) {
>>               err = ptp->info->gettimex64(ptp->info, &ts, &sts);
>>               if (err)
>> diff --git a/include/linux/ptp_clock_kernel.h b/include/linux/ 
>> ptp_clock_kernel.h
>> index 6e4b8206c7d0..74ded5f95d95 100644
>> --- a/include/linux/ptp_clock_kernel.h
>> +++ b/include/linux/ptp_clock_kernel.h
>> @@ -47,10 +47,12 @@ 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
>> + * @clockid: clock-base used for capturing the system timestamps
>>    */
>>   struct ptp_system_timestamp {
>>       struct timespec64 pre_ts;
>>       struct timespec64 post_ts;
>> +    clockid_t clockid;
>>   };
>>   /**
>> @@ -457,14 +459,34 @@ static inline ktime_t 
>> ptp_convert_timestamp(const ktime_t *hwtstamp,
>>   static inline void ptp_read_system_prets(struct ptp_system_timestamp 
>> *sts)
>>   {
>> -    if (sts)
>> -        ktime_get_real_ts64(&sts->pre_ts);
>> +    if (sts) {
>> +        switch (sts->clockid) {
>> +        case CLOCK_REALTIME:
>> +            ktime_get_real_ts64(&sts->pre_ts);
>> +            break;
>> +        case CLOCK_MONOTONIC_RAW:
>> +            ktime_get_raw_ts64(&sts->pre_ts);
>> +            break;
>> +        default:
>> +            break;
>> +        }
>> +    }
>>   }
>>   static inline void ptp_read_system_postts(struct 
>> ptp_system_timestamp *sts)
>>   {
>> -    if (sts)
>> -        ktime_get_real_ts64(&sts->post_ts);
>> +    if (sts) {
>> +        switch (sts->clockid) {
>> +        case CLOCK_REALTIME:
>> +            ktime_get_real_ts64(&sts->post_ts);
>> +            break;
>> +        case CLOCK_MONOTONIC_RAW:
>> +            ktime_get_raw_ts64(&sts->post_ts);
>> +            break;
>> +        default:
>> +            break;
>> +        }
>> +    }
>>   }
>>   #endif
>> diff --git a/include/uapi/linux/ptp_clock.h b/include/uapi/linux/ 
>> ptp_clock.h
>> index 053b40d642de..5e3d70fbc499 100644
>> --- a/include/uapi/linux/ptp_clock.h
>> +++ b/include/uapi/linux/ptp_clock.h
>> @@ -155,13 +155,28 @@ struct ptp_sys_offset {
>>       struct ptp_clock_time ts[2 * PTP_MAX_SAMPLES + 1];
>>   };
>> +/*
>> + * ptp_sys_offset_extended - data structure for IOCTL operation
>> + *                 PTP_SYS_OFFSET_EXTENDED
>> + *
>> + * @n_samples:    Desired number of measurements.
>> + * @clockid:    clockid of a clock-base used for pre/post timestamps.
>> + * @rsv:    Reserved for future use.
>> + * @ts:        Array of samples in the form [pre-TS, PHC, post-TS]. The
>> + *        kernel provides @n_samples.
>> + *
>> + * History:
>> + * v1: Initial implementation.
>> + *
>> + * v2: Use the first word of the reserved-field for @clockid. That's
>> + *     backward compatible since v1 expects all three reserved words
>> + *     (@rsv[3]) to be 0 while the clockid (first word in v2) for
>> + *     CLOCK_REALTIME is '0'.
>> + */
>>   struct ptp_sys_offset_extended {
>> -    unsigned int n_samples; /* Desired number of measurements. */
>> -    unsigned int rsv[3];    /* Reserved for future use. */
>> -    /*
>> -     * Array of [system, phc, system] time stamps. The kernel will 
>> provide
>> -     * 3*n_samples time stamps.
>> -     */
>> +    unsigned int n_samples;
>> +    __kernel_clockid_t clockid;
>> +    unsigned int rsv[2];
>>       struct ptp_clock_time ts[PTP_MAX_SAMPLES][3];
>>   };
>