From: Sagi Maimon <maimon.sagi@gmail.com>
To: "Mahesh Bandewar (महेश बंडेवार)" <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
Subject: Re: [PATCH v7] posix-timers: add clock_compare system call
Date: Tue, 16 Apr 2024 11:39:07 +0300 [thread overview]
Message-ID: <CAMuE1bHj2w0HLyxj0eQ-OOg+=qqyFoxeb5Ko0G81DEXVLtNjdw@mail.gmail.com> (raw)
In-Reply-To: <CAF2d9jg0X_HKSZbiwPTEXdmrhY49D1zfT3Q4xzNAqv1z+TYXtA@mail.gmail.com>
Thanks
On Mon, Apr 15, 2024 at 8:23 PM Mahesh Bandewar (महेश बंडेवार)
<maheshb@google.com> wrote:
>
> On Sun, Apr 14, 2024 at 5:22 AM Sagi Maimon <maimon.sagi@gmail.com> wrote:
> >
> > On Thu, Apr 11, 2024 at 7:34 PM Mahesh Bandewar (महेश बंडेवार)
> > <maheshb@google.com> wrote:
> > >
> > > On Thu, Apr 11, 2024 at 12:11 AM Sagi Maimon <maimon.sagi@gmail.com> wrote:
> > > >
> > > > 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,
> > > >
> > > Hi Sagi,
> > >
> > > If you want to pursue the syscall option, then those are tangential
> > > and please go ahead. (I cannot stop you!)
> > > I'm interested in getting the "tight sandwich timestamps" that
> > > gettimex64() ioctl offers and I would want enhancements to
> > > gettimex64() done the way it was discussed in the later half of this
> > > thread. If you want to sign-up for that please let me know.
> > Hi Mahesh
> > I do need to modify the PTP_SYS_OFFSET_EXTENDED ioctl for cases which
> > gettimex64
> > not supported by the driver (look at Thomas suggestion), but I need
> > your changes in ptp_read_system_prets.
> > I like to add my changes above your changes, so we won't do duplicate work.
> > please show me your latest patch and the status of it
> > Once you have upstream yours , I will add my changes on the next patch.
>
> OK, in that case let me post the patch since your changes would need
> pieces from it.
>
> thanks,
> --mahesh..
>
> > BR
> > Sagi
> >
> > >
> > > thanks,
> > > --mahesh..
> > >
> > >
> > > > On Thu, Apr 11, 2024 at 5:56 AM Mahesh Bandewar (महेश बंडेवार)
> > > > <maheshb@google.com> wrote:
> > > > >
> > > > > On Wed, Apr 3, 2024 at 6:48 AM Thomas Gleixner <tglx@linutronix.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@linutronix.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
next prev parent reply other threads:[~2024-04-16 8:39 UTC|newest]
Thread overview: 26+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-03-14 9:05 [PATCH v7] posix-timers: add clock_compare system call Sagi Maimon
2024-03-14 11:12 ` Thomas Gleixner
2024-03-14 12:19 ` Sagi Maimon
2024-03-14 15:59 ` Mark Rutland
2024-03-14 18:08 ` Thomas Gleixner
2024-03-20 14:42 ` Sagi Maimon
2024-03-23 0:38 ` Thomas Gleixner
2024-03-23 0:42 ` Thomas Gleixner
2024-03-24 11:04 ` Kurt Kanzenbach
2024-03-28 15:40 ` Sagi Maimon
2024-04-01 20:46 ` Thomas Gleixner
2024-04-02 5:42 ` Mahesh Bandewar (महेश बंडेवार)
2024-04-02 9:24 ` Thomas Gleixner
2024-04-02 21:16 ` Mahesh Bandewar (महेश बंडेवार)
2024-04-02 22:37 ` Thomas Gleixner
2024-04-02 23:37 ` Mahesh Bandewar (महेश बंडेवार)
2024-04-03 13:48 ` Thomas Gleixner
2024-04-03 15:42 ` Thomas Gleixner
2024-04-11 2:55 ` Mahesh Bandewar (महेश बंडेवार)
2024-04-11 7:11 ` Sagi Maimon
2024-04-11 16:33 ` Mahesh Bandewar (महेश बंडेवार)
2024-04-14 12:22 ` Sagi Maimon
2024-04-15 17:23 ` Mahesh Bandewar (महेश बंडेवार)
2024-04-16 8:39 ` Sagi Maimon [this message]
2024-03-14 15:46 ` Sagi Maimon
2024-03-14 18:42 ` Thomas Gleixner
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to='CAMuE1bHj2w0HLyxj0eQ-OOg+=qqyFoxeb5Ko0G81DEXVLtNjdw@mail.gmail.com' \
--to=maimon.sagi@gmail.com \
--cc=arnd@arndb.de \
--cc=bp@alien8.de \
--cc=brauner@kernel.org \
--cc=casey@schaufler-ca.com \
--cc=dave.hansen@linux.intel.com \
--cc=davem@davemloft.net \
--cc=geert@linux-m68k.org \
--cc=hannes@cmpxchg.org \
--cc=hpa@zytor.com \
--cc=keescook@chromium.org \
--cc=legion@kernel.org \
--cc=linux-api@vger.kernel.org \
--cc=linux-arch@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=luto@kernel.org \
--cc=maheshb@google.com \
--cc=mark.rutland@arm.com \
--cc=mingo@redhat.com \
--cc=mszeredi@redhat.com \
--cc=netdev@vger.kernel.org \
--cc=nphamcs@gmail.com \
--cc=palmer@sifive.com \
--cc=peterz@infradead.org \
--cc=reibax@gmail.com \
--cc=richardcochran@gmail.com \
--cc=rick.p.edgecombe@intel.com \
--cc=sohil.mehta@intel.com \
--cc=tglx@linutronix.de \
--cc=x86@kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).