[PATCHv3,net-next,2/3] ptp: add ioctl interface for ptp_gettimex64any()
Commit Message
add an ioctl op PTP_SYS_OFFSET_ANY to support newly added
ptp_gettimex64any() method
Signed-off-by: Mahesh Bandewar <maheshb@google.com>
CC: Richard Cochran <richardcochran@gmail.com>
CC: "David S. Miller" <davem@davemloft.net>
CC: John Stultz <jstultz@google.com>
CC: Jakub Kicinski <kuba@kernel.org>
CC: "Willem de Bruijn" <willemb@google.com>
CC: netdev@vger.kernel.org
---
drivers/ptp/ptp_chardev.c | 37 ++++++++++++++++++++++++++++++++++
include/uapi/linux/ptp_clock.h | 14 +++++++++++++
2 files changed, 51 insertions(+)
Comments
On Thu, Jan 04, 2024 at 01:24:39PM -0800, Mahesh Bandewar wrote:
> @@ -226,6 +238,8 @@ struct ptp_pin_desc {
> _IOWR(PTP_CLK_MAGIC, 18, struct ptp_sys_offset_extended)
> #define PTP_MASK_CLEAR_ALL _IO(PTP_CLK_MAGIC, 19)
> #define PTP_MASK_EN_SINGLE _IOW(PTP_CLK_MAGIC, 20, unsigned int)
> +#define PTP_SYS_OFFSET_ANY \
> + _IOWR(PTP_CLK_MAGIC, 21, struct ptp_sys_offset_any)
As I said before, this functionality really ought to be a new system call.
Did you see these patch series posted on the list?
31.Dec'23 Sagi Maimon [PATCH v4] posix-timers: add multi_clock_gettime system call
31.Dec'23 Andy Lutomirski ├─>
01.Jan'24 Sagi Maimon │ └─>
01.Jan'24 kernel test rob ├─>
01.Jan'24 kernel test rob └─>
02.Jan'24 Sagi Maimon [PATCH v5] posix-timers: add multi_clock_gettime system call
02.Jan'24 Arnd Bergmann ├─>
03.Jan'24 Sagi Maimon │ └─>
04.Jan'24 kernel test rob └─>
I think this will be the way forward.
Please review the multi_clock_gettime series and help refine it.
Thanks,
Richard
On Thu, Jan 4, 2024 at 2:37 PM Richard Cochran <richardcochran@gmail.com> wrote:
>
> On Thu, Jan 04, 2024 at 01:24:39PM -0800, Mahesh Bandewar wrote:
>
> > @@ -226,6 +238,8 @@ struct ptp_pin_desc {
> > _IOWR(PTP_CLK_MAGIC, 18, struct ptp_sys_offset_extended)
> > #define PTP_MASK_CLEAR_ALL _IO(PTP_CLK_MAGIC, 19)
> > #define PTP_MASK_EN_SINGLE _IOW(PTP_CLK_MAGIC, 20, unsigned int)
> > +#define PTP_SYS_OFFSET_ANY \
> > + _IOWR(PTP_CLK_MAGIC, 21, struct ptp_sys_offset_any)
>
> As I said before, this functionality really ought to be a new system call.
>
> Did you see these patch series posted on the list?
>
> 31.Dec'23 Sagi Maimon [PATCH v4] posix-timers: add multi_clock_gettime system call
> 31.Dec'23 Andy Lutomirski ├─>
> 01.Jan'24 Sagi Maimon │ └─>
> 01.Jan'24 kernel test rob ├─>
> 01.Jan'24 kernel test rob └─>
>
> 02.Jan'24 Sagi Maimon [PATCH v5] posix-timers: add multi_clock_gettime system call
> 02.Jan'24 Arnd Bergmann ├─>
> 03.Jan'24 Sagi Maimon │ └─>
> 04.Jan'24 kernel test rob └─>
>
> I think this will be the way forward.
>
> Please review the multi_clock_gettime series and help refine it.
>
While multi_clock_gettime streamlines POSIX clock reads (defined in
posix-timers.h: struct k_time), this proposal targets precision
enhancements for PTP hardware clocks (defined in ptp_clock_kernel.h:
struct ptp_clock_info).
Key distinctions:
1. Target clocks: multi_clock_gettime operates on POSIX clocks
(posix-timers.h), while this proposal focuses on PTP hardware clocks
(ptp_clock_kernel.h).
2. Purpose: multi_clock_gettime addresses scheduling latencies
by consolidating multiple clock reads. This proposal aims to enhance
PTP clock-read precision by measuring syscall width.
POSIX clocks are employed in this series for syscall width
measurement, potentially leading to misunderstandings about
overlapping functionality. However, their roles are distinct and serve
different purposes.
Both approaches address time-related challenges, but with different
goals and techniques.
multi_clock_gettime optimizes scheduling efficiency for POSIX clocks,
while this proposal refines PTP hardware clock-read accuracy.
They complement each other, catering to diverse application needs
within the timekeeping domain.
Hope this helps.
Thanks,
--mahesh..
> Thanks,
> Richard
>
On Fri, Jan 05, 2024 at 09:51:40AM -0800, Mahesh Bandewar (महेश बंडेवार) wrote:
> POSIX clocks are employed in this series for syscall width
> measurement, potentially leading to misunderstandings about
> overlapping functionality. However, their roles are distinct and serve
> different purposes.
I don't see any difference in purposes. The multi_clock_gettime call
is a more general solution. Thus it will obviate the need for any new
PTP ioctls.
Thanks,
Richard
On Fri, Jan 05, 2024 at 08:55:46PM -0800, Richard Cochran wrote:
> On Fri, Jan 05, 2024 at 09:51:40AM -0800, Mahesh Bandewar (महेश बंडेवार) wrote:
>
> > POSIX clocks are employed in this series for syscall width
> > measurement, potentially leading to misunderstandings about
> > overlapping functionality. However, their roles are distinct and serve
> > different purposes.
>
> I don't see any difference in purposes. The multi_clock_gettime call
> is a more general solution. Thus it will obviate the need for any new
> PTP ioctls.
And to be clear, I object to the third patch, the new ioctl.
ptp_clock_info.gettimex64any() can and should be presented as an
optimized back end for the multi_clock_gettime system call.
Thanks,
Richard
On Fri, Jan 5, 2024 at 8:55 PM Richard Cochran <richardcochran@gmail.com> wrote:
>
> On Fri, Jan 05, 2024 at 09:51:40AM -0800, Mahesh Bandewar (महेश बंडेवार) wrote:
>
> > POSIX clocks are employed in this series for syscall width
> > measurement, potentially leading to misunderstandings about
> > overlapping functionality. However, their roles are distinct and serve
> > different purposes.
>
> I don't see any difference in purposes. The multi_clock_gettime call
> is a more general solution. Thus it will obviate the need for any new
> PTP ioctls.
>
I disagree! NICs inherently benefit from bundled PTP devices due to
their superior low-latency, low-overhead, and precise TX/RX
timestamping capabilities. For demanding systems requiring increased
capacity, multiple NICs from various vendors are often deployed.
However, disciplining these diverse PTP devices across the host
demands a flexible approach; a general purpose syscall is not an
answer. The current PHC implementation using ioctls through exported
ptp devices (/dev/ptpX) provides a solid foundation that is per device
(/per NIC).
This series is providing another piece in an existing suite of methods
used for disciplining / precision tuning (along with adjfine, adjtime,
gettime etc.) This addition is to take that precision even further.
Having a general solution for posix timers is a nice addition.
However, expecting a general purpose syscall to eliminate need for
device ioctl is an unreasonable expectation.
Thanks,
--mahesh..
> Thanks,
> Richard
On Sat, Jan 06, 2024 at 12:08:57AM -0800, Mahesh Bandewar (महेश बंडेवार) wrote:
> Having a general solution for posix timers is a nice addition.
> However, expecting a general purpose syscall to eliminate need for
> device ioctl is an unreasonable expectation.
Let me make this clear:
There is no reasonable justification for a new PTP ioctl.
The system call can and should use the the most accurate method
internally to the kernel.
Thanks,
Richard
On Sat, Jan 06, 2024 at 12:08:57AM -0800, Mahesh Bandewar (महेश बंडेवार) wrote:
> I disagree! NICs inherently benefit from bundled PTP devices due to
> their superior low-latency, low-overhead, and precise TX/RX
> timestamping capabilities. For demanding systems requiring increased
> capacity, multiple NICs from various vendors are often deployed.
> However, disciplining these diverse PTP devices across the host
> demands a flexible approach; a general purpose syscall is not an
> answer. The current PHC implementation using ioctls through exported
> ptp devices (/dev/ptpX) provides a solid foundation that is per device
> (/per NIC).
>
> This series is providing another piece in an existing suite of methods
> used for disciplining / precision tuning (along with adjfine, adjtime,
> gettime etc.) This addition is to take that precision even further.
This reads like marketing fluff.
You fail to provide any *technical* reason why your proposed cross
time stamp method can only work with a new ioctl, and not as the back
end of a system call.
Thanks,
Richard
@@ -161,6 +161,7 @@ long ptp_ioctl(struct posix_clock_context *pccontext, unsigned int cmd,
struct ptp_clock *ptp =
container_of(pccontext->clk, struct ptp_clock, clock);
struct ptp_sys_offset_extended *extoff = NULL;
+ struct ptp_sys_offset_any *anyoff = NULL;
struct ptp_sys_offset_precise precise_offset;
struct system_device_crosststamp xtstamp;
struct ptp_clock_info *ops = ptp->info;
@@ -378,6 +379,42 @@ long ptp_ioctl(struct posix_clock_context *pccontext, unsigned int cmd,
err = -EFAULT;
break;
+ case PTP_SYS_OFFSET_ANY:
+ if (!ptp->info->gettimex64any) {
+ err = -EOPNOTSUPP;
+ break;
+ }
+ anyoff = memdup_user((void __user *)arg, sizeof(*anyoff));
+ if (IS_ERR(anyoff)) {
+ err = PTR_ERR(anyoff);
+ anyoff = NULL;
+ break;
+ }
+ if (anyoff->n_samples > PTP_MAX_SAMPLES
+ || anyoff->rsv[0] || anyoff->rsv[1]
+ || (anyoff->clockid != CLOCK_REALTIME
+ && anyoff->clockid != CLOCK_MONOTONIC
+ && anyoff->clockid != CLOCK_MONOTONIC_RAW)) {
+ err = -EINVAL;
+ break;
+ }
+
+ for (i = 0; i < anyoff->n_samples; i++) {
+ err = ptp->info->gettimex64any(ptp->info, &ts, &sts,
+ anyoff->clockid);
+ if (err)
+ goto out;
+ anyoff->ts[i][0].sec = sts.pre_ts.tv_sec;
+ anyoff->ts[i][0].nsec = sts.pre_ts.tv_nsec;
+ anyoff->ts[i][1].sec = ts.tv_sec;
+ anyoff->ts[i][1].nsec = ts.tv_nsec;
+ anyoff->ts[i][2].sec = sts.post_ts.tv_sec;
+ anyoff->ts[i][2].nsec = sts.post_ts.tv_nsec;
+ }
+ if (copy_to_user((void __user *)arg, anyoff, sizeof(*anyoff)))
+ err = -EFAULT;
+ break;
+
case PTP_SYS_OFFSET:
case PTP_SYS_OFFSET2:
sysoff = memdup_user((void __user *)arg, sizeof(*sysoff));
@@ -158,6 +158,18 @@ struct ptp_sys_offset_extended {
struct ptp_clock_time ts[PTP_MAX_SAMPLES][3];
};
+struct ptp_sys_offset_any {
+ unsigned int n_samples; /* Desired number of measurements. */
+ clockid_t clockid; /* One of the supported ClockID */
+ unsigned int rsv[2]; /* Reserved for future use. */
+ /*
+ * Array of [TS, phc, TS] time stamps. The kernel will provide
+ * 3*n_samples time stamps.
+ * TS is any of the ts_type requested.
+ */
+ struct ptp_clock_time ts[PTP_MAX_SAMPLES][3];
+};
+
struct ptp_sys_offset_precise {
struct ptp_clock_time device;
struct ptp_clock_time sys_realtime;
@@ -226,6 +238,8 @@ struct ptp_pin_desc {
_IOWR(PTP_CLK_MAGIC, 18, struct ptp_sys_offset_extended)
#define PTP_MASK_CLEAR_ALL _IO(PTP_CLK_MAGIC, 19)
#define PTP_MASK_EN_SINGLE _IOW(PTP_CLK_MAGIC, 20, unsigned int)
+#define PTP_SYS_OFFSET_ANY \
+ _IOWR(PTP_CLK_MAGIC, 21, struct ptp_sys_offset_any)
struct ptp_extts_event {
struct ptp_clock_time t; /* Time event occured. */