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 <[email protected]>
---
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
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 <[email protected]>
LGTM, but let's wait a bit for Richard ack.
@Richard, could you please have look?
Paolo
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
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
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
On Tue, May 7, 2024 at 9:44 PM Richard Cochran <[email protected]> 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
>
On Wed, May 8, 2024 at 12:35 AM Thomas Gleixner <[email protected]> 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