From: Sagi Maimon <maimon.sagi@gmail.com>
To: Arnd Bergmann <arnd@arndb.de>
Cc: Richard Cochran <richardcochran@gmail.com>,
Andy Lutomirski <luto@kernel.org>,
datglx@linutronix.de, Ingo Molnar <mingo@redhat.com>,
Borislav Petkov <bp@alien8.de>,
Dave Hansen <dave.hansen@linux.intel.com>,
x86@kernel.org, "H. Peter Anvin" <hpa@zytor.com>,
Geert Uytterhoeven <geert@linux-m68k.org>,
Peter Zijlstra <peterz@infradead.org>,
Johannes Weiner <hannes@cmpxchg.org>,
Sohil Mehta <sohil.mehta@intel.com>,
Rick Edgecombe <rick.p.edgecombe@intel.com>,
Nhat Pham <nphamcs@gmail.com>,
Palmer Dabbelt <palmer@sifive.com>,
Kees Cook <keescook@chromium.org>,
Alexey Gladkov <legion@kernel.org>,
Mark Rutland <mark.rutland@arm.com>,
Miklos Szeredi <mszeredi@redhat.com>,
Casey Schaufler <casey@schaufler-ca.com>,
reibax@gmail.com, "David S . Miller" <davem@davemloft.net>,
Christian Brauner <brauner@kernel.org>,
linux-kernel@vger.kernel.org, linux-api@vger.kernel.org,
Linux-Arch <linux-arch@vger.kernel.org>,
Netdev <netdev@vger.kernel.org>
Subject: Re: [PATCH v6] posix-timers: add clock_compare system call
Date: Thu, 14 Mar 2024 10:05:04 +0200 [thread overview]
Message-ID: <CAMuE1bEGcPPQUHdZ-sodCZyMragvSRtKfENOZ4wphQggr1fJWg@mail.gmail.com> (raw)
In-Reply-To: <0a4e4505-cf04-4481-955c-1e35cf97ff8d@app.fastmail.com>
On Tue, Mar 12, 2024 at 3:47 PM Arnd Bergmann <arnd@arndb.de> wrote:
>
> On Tue, Mar 12, 2024, at 13:15, Sagi Maimon wrote:
> > On Tue, Mar 12, 2024 at 1:19 PM Arnd Bergmann <arnd@arndb.de> wrote:
> >> On Tue, Mar 12, 2024, at 10:50, Sagi Maimon wrote:
> >> > + kc_a = clockid_to_kclock(clock_a);
> >> > + if (!kc_a) {
> >> > + error = -EINVAL;
> >> > + return error;
> >> > + }
> >> > +
> >> > + kc_b = clockid_to_kclock(clock_b);
> >> > + if (!kc_b) {
> >> > + error = -EINVAL;
> >> > + return error;
> >> > + }
> >>
> >> I'm not sure if we really need to have it generic enough to
> >> support any combination of clocks here. It complicates the
> >> implementation a bit but it also generalizes the user space
> >> side of it.
> >>
> >> Can you think of cases where you want to compare against
> >> something other than CLOCK_MONOTONIC_RAW or CLOCK_REALTIME,
> >> or are these going to be the ones that you expect to
> >> be used anyway?
> >>
> > sure, one example is syncing two different PHCs (which was originally
> > why we needed this syscall)
> > I hope that I have understand your note and that answers your question.
>
> Right, that is clearly a sensible use case.
>
> I'm still trying to understand the implementation for the case
> where you have two different PHCs and both implement
> clock_get_crosstimespec(). Rather than averaging between
> two snapshots here, I would expect this to result in
> something like
>
> ktime_a1 += xtstamp_b.sys_monoraw - xtstamp_a1.sys_monoraw;
>
> in order get two device timestamps ktime_a1 and ktime_b
> that reflect the snapshots as if they were taken
> simulatenously. Am I missing some finer detail here,
> or is this something you should do?
>
Since the raw monotonic clock and the PHC are not synthesized, that
won't be accurate at all and depends on the frequency delta between
them.
> >> > + if (crosstime_support_a) {
> >> > + ktime_a1 = xtstamp_a1.device;
> >> > + ktime_a2 = xtstamp_a2.device;
> >> > + } else {
> >> > + ktime_a1 = timespec64_to_ktime(ts_a1);
> >> > + ktime_a2 = timespec64_to_ktime(ts_a2);
> >> > + }
> >> > +
> >> > + ktime_a = ktime_add(ktime_a1, ktime_a2);
> >> > +
> >> > + ts_offs = ktime_divns(ktime_a, 2);
> >> > +
> >> > + ts_a1 = ns_to_timespec64(ts_offs);
> >>
> >> Converting nanoseconds to timespec64 is rather expensive,
> >> so I wonder if this could be changed to something cheaper,
> >> either by returning nanoseconds in the end and consistently
> >> working on those, or by doing the calculation on the
> >> timespec64 itself.
> >>
> > I prefer returning timespec64, so this system call aligns with other
> > system calls like clock_gettime for example.
> > As far as doing the calculation on timespec64 itself, that looks more
> > expansive to me, but I might be wrong.
>
> In the general case, dividing a 64-bit variable by some other
> variable is really expensive and will take hundreds of cycles.
> This one is a bit cheaper because the division is done using
> a constant divider of NS_PER_SEC, which can get optimized fairly
> well on many systems by turning it into an equivalent 128-bit
> multiplication plus shift.
>
> For the case where you start out with a timespec64, I would
> expect it to be cheaper to calculate the nanosecond difference
> between ts_a1 and ts_a2 to add half of that to the timespec
> than to average two large 64-bit values and convert that back
> to a timespec afterwards. This should be fairly easy to try
> out if you can test a 32-bit kernel. We could decide that
> there is no need to care about anything bug 64-bit kernels
> here, in which case your current version should be just as
> good for both the crosstime_support_a and !crosstime_support_a
> cases.
>
sounds good, it will be done on the next patch.
> Arnd
prev parent reply other threads:[~2024-03-14 8:05 UTC|newest]
Thread overview: 5+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-03-12 9:50 [PATCH v6] posix-timers: add clock_compare system call Sagi Maimon
2024-03-12 11:17 ` Arnd Bergmann
2024-03-12 12:15 ` Sagi Maimon
2024-03-12 13:47 ` Arnd Bergmann
2024-03-14 8:05 ` Sagi Maimon [this message]
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=CAMuE1bEGcPPQUHdZ-sodCZyMragvSRtKfENOZ4wphQggr1fJWg@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=datglx@linutronix.de \
--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=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=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).