| Message ID | 20240104212439.3276458-1-maheshb@google.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | add ptp_gettimex64any() API | expand |
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
diff --git a/drivers/ptp/ptp_chardev.c b/drivers/ptp/ptp_chardev.c index 7513018c9f9a..f20d43c34aec 100644 --- a/drivers/ptp/ptp_chardev.c +++ b/drivers/ptp/ptp_chardev.c @@ -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)); diff --git a/include/uapi/linux/ptp_clock.h b/include/uapi/linux/ptp_clock.h index da700999cad4..a3143df8de2b 100644 --- a/include/uapi/linux/ptp_clock.h +++ b/include/uapi/linux/ptp_clock.h @@ -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. */
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(+)