Message-ID: <CAF2d9jikELOQa_9Kk+oF_=_7NZTn9DuAw=s9KQR6-EfWTiW5RQ@mail.gmail.com>
Date: Wed, 10 Apr 2024 19:55:50 -0700
From: Mahesh Bandewar (महेश बंडेवार) <maheshb@...gle.com>
To: Thomas Gleixner <tglx@...utronix.de>
Cc: Sagi Maimon <maimon.sagi@...il.com>, 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

On Wed, Apr 3, 2024 at 6:48 AM Thomas Gleixner <tglx@...utronix.de> wrote:
>
> On Tue, Apr 02 2024 at 16:37, Mahesh Bandewar (महेश बंडेवार) wrote:
> > On Tue, Apr 2, 2024 at 3:37 PM Thomas Gleixner <tglx@...utronix.de> wrote:
> > The modification that you have proposed (in a couple of posts back)
> > would work but it's still not ideal since the pre/post ts are not
> > close enough as they are currently  (properly implemented!)
> > gettimex64() would have. The only way to do that would be to have
> > another ioctl as I have proposed which is a superset of current
> > gettimex64 and pre-post collection is the closest possible.
>
> Errm. What I posted as sketch _is_ using gettimex64() with the extra
> twist of the flag vs. a clockid (which is an implementation detail) and
> the difference that I carry the information in ptp_system_timestamp
> instead of needing a new argument clockid to all existing callbacks
> because the modification to ptp_read_prets() and postts() will just be
> sufficient, no?
>
OK, that makes sense.

> For the case where the driver does not provide gettimex64() then the
> extension of the original offset ioctl is still providing a better
> mechanism than the proposed syscall.
>
> I also clearly said that all drivers should be converted over to
> gettimex64().
>
I agree. Honestly that should have been mandatory and
ptp_register_clock() should fail otherwise! Probably should have been
part of gettimex64 implementation :(

I don't think we can do anything other than just hoping all driver
implementations include gettimex64 implementation.

> > Having said that, the 'flag' modification proposal is a good backup
> > for the drivers that don't have good implementation (close enough but
> > not ideal). Also, you don't need a new ioctl-op. So if we really want
> > precision, I believe, we need a new ioctl op (with supporting
> > implementation similar to the mlx4 code above). but we want to save
> > the new ioctl-op and have less precision then proposed modification
> > would work fine.
>
> I disagree. The existing gettimex64() is good enough if the driver
> implements it correctly today. If not then those drivers need to be
> fixed independent of this.
>
> So assumed that a driver does:
>
> gettimex64()
>    ptp_prets(sts);
>    read_clock();
>    ptp_postts(sts);
>
> today then having:
>
> static inline void ptp_read_system_prets(struct ptp_system_timestamp *sts)
> {
>         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) {
>                 if (sts->flags & PTP_SYS_OFFSET_MONO_RAW)
>                         ktime_get_raw_ts64(&sts->post_ts);
>                 else
>                         ktime_get_real_ts64(&sts->post_ts);
>         }
> }
>
> or
>
> static inline void ptp_read_system_prets(struct ptp_system_timestamp *sts)
> {
>         if (sts) {
>                 switch (sts->clockid) {
>                 case CLOCK_MONOTONIC_RAW:
>                         time_get_raw_ts64(&sts->pre_ts);
>                         break;
>                 case CLOCK_REALTIME:
>                         ktime_get_real_ts64(&sts->pre_ts);
>                         break;
>                 }
>         }
> }
>
> static inline void ptp_read_system_postts(struct ptp_system_timestamp *sts)
> {
>         if (sts) {
>                 switch (sts->clockid) {
>                 case CLOCK_MONOTONIC_RAW:
>                         time_get_raw_ts64(&sts->post_ts);
>                         break;
>                 case CLOCK_REALTIME:
>                         ktime_get_real_ts64(&sts->post_ts);
>                         break;
>                 }
>         }
> }
>
> is doing the exact same thing as your proposal but without touching any
> driver which implements gettimex64() correctly at all.
>
I see. Yes, this makes sense.

> While your proposal requires to touch every single driver for no reason,
> no?
>
> It is just an implementation detail whether you use a flag or a
> clockid. You can carry the clockid for the clocks which actually can be
> read in that context in a reserved field of PTP_SYS_OFFSET_EXTENDED:
>
> struct ptp_sys_offset_extended {
>         unsigned int    n_samples; /* Desired number of measurements. */
>         clockid_t       clockid;
>         unsigned int    rsv[2];    /* Reserved for future use. */
> };
>
> and in the IOCTL:
>
>         if (extoff->clockid != CLOCK_MONOTONIC_RAW)
>                 return -EINVAL;
>
>         sts.clockid = extoff->clockid;
>
> and it all just works, no?
>
Yes, this should work. However, I didn't check if struct
ptp_system_timestamp is used in some other context.

> I have no problem to decide that PTP_SYS_OFFSET will not get this
> treatment and the drivers have to be converted over to
> PTP_SYS_OFFSET_EXTENDED.
>
> But adding yet another callback just to carry a clockid as argument is a
> more than pointless exercise as I demonstrated.
>
Agreed. As I said, I thought we cannot change the gettimex64() without
breaking the compatibility but the fact that CLOCK_REALTIME is "0"
works well for the backward compatibility case.

I can spin up an updated patch/series that updates gettimex64
implementation instead of adding a new ioctl-op If you all agree.

thanks,
--mahesh..

> Thanks,
>
>         tglx

Powered by blists - more mailing lists