Received-SPF: pass (google.com: domain of linux-kernel+bounces-139942-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) client-ip=147.75.80.249;
Precedence: bulk
MIME-Version: 1.0
References: <CAMuE1bHBky9NGP22PVHKdi2+WniwxiLSmMnwRM6wm36sU8W4jA@mail.gmail.com>
 <878r29hjds.ffs@tglx> <CAMuE1bF9ioo39_08Eh26X4WOtnvJ1geJ=WRVt5DhU8gEbYJNdA@mail.gmail.com>
 <87o7asdd65.ffs@tglx> <CAF2d9jjA8iM1AoPUhQPK62tdd7gPnCnt51f_NMhOAs546rU3dA@mail.gmail.com>
 <87il10ce1g.ffs@tglx> <CAF2d9jj6km7aVSqgcOE-b-A-WDH2TJNGzGy-5MRyw5HrzbqhaA@mail.gmail.com>
 <877chfcrx3.ffs@tglx> <CAF2d9jjg0PEgPorXdrBHVkvz-fmUV7UXUPqnpQGVEvgXTpHY0A@mail.gmail.com>
 <871q7md0ak.ffs@tglx> <CAF2d9jikELOQa_9Kk+oF_=_7NZTn9DuAw=s9KQR6-EfWTiW5RQ@mail.gmail.com>
In-Reply-To: <CAF2d9jikELOQa_9Kk+oF_=_7NZTn9DuAw=s9KQR6-EfWTiW5RQ@mail.gmail.com>
From: Sagi Maimon <maimon.sagi@gmail.com>
Date: Thu, 11 Apr 2024 10:11:41 +0300
Message-ID: <CAMuE1bFkmj70DO66PfvBPjM1d_JDEwTkOyz6o6wO_C0uyJ_0zw@mail.gmail.com>
Subject: Re: [PATCH v7] posix-timers: add clock_compare system call
To: =?UTF-8?B?TWFoZXNoIEJhbmRld2FyICjgpK7gpLngpYfgpLYg4KSs4KSC4KSh4KWH4KS14KS+4KSwKQ==?= <maheshb@google.com>
Cc: Thomas Gleixner <tglx@linutronix.de>, richardcochran@gmail.com, luto@kernel.org, 
	mingo@redhat.com, bp@alien8.de, dave.hansen@linux.intel.com, x86@kernel.org, 
	hpa@zytor.com, arnd@arndb.de, geert@linux-m68k.org, peterz@infradead.org, 
	hannes@cmpxchg.org, sohil.mehta@intel.com, rick.p.edgecombe@intel.com, 
	nphamcs@gmail.com, palmer@sifive.com, keescook@chromium.org, 
	legion@kernel.org, mark.rutland@arm.com, mszeredi@redhat.com, 
	casey@schaufler-ca.com, reibax@gmail.com, davem@davemloft.net, 
	brauner@kernel.org, linux-kernel@vger.kernel.org, linux-api@vger.kernel.org, 
	linux-arch@vger.kernel.org, netdev@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

Hi Mahesh
What is the status of your patch?
if your patch is upstreamed , then it will have all I need.
But, If not , I will upstream my patch.
BR,

On Thu, Apr 11, 2024 at 5:56=E2=80=AFAM Mahesh Bandewar (=E0=A4=AE=E0=A4=B9=
=E0=A5=87=E0=A4=B6 =E0=A4=AC=E0=A4=82=E0=A4=A1=E0=A5=87=E0=A4=B5=E0=A4=BE=
=E0=A4=B0)
<maheshb@google.com> wrote:
>
> On Wed, Apr 3, 2024 at 6:48=E2=80=AFAM Thomas Gleixner <tglx@linutronix.d=
e> wrote:
> >
> > On Tue, Apr 02 2024 at 16:37, Mahesh Bandewar (=E0=A4=AE=E0=A4=B9=E0=A5=
=87=E0=A4=B6 =E0=A4=AC=E0=A4=82=E0=A4=A1=E0=A5=87=E0=A4=B5=E0=A4=BE=E0=A4=
=B0) wrote:
> > > On Tue, Apr 2, 2024 at 3:37=E2=80=AFPM Thomas Gleixner <tglx@linutron=
ix.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 *s=
ts)
> > {
> >         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 *s=
ts)
> > {
> >         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 !=3D CLOCK_MONOTONIC_RAW)
> >                 return -EINVAL;
> >
> >         sts.clockid =3D 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