Received-SPF: pass (google.com: domain of linux-kernel+bounces-100301-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) client-ip=2604:1380:45e3:2400::1;
Feedback-ID: i56a14606:Fastmail
User-Agent: Cyrus-JMAP/3.11.0-alpha0-251-g8332da0bf6-fm-20240305.001-g8332da0b
Precedence: bulk
MIME-Version: 1.0
Message-Id: <0a4e4505-cf04-4481-955c-1e35cf97ff8d@app.fastmail.com>
In-Reply-To: 
 <CAMuE1bGkZ=ifyofCUfm4JVS__dgYG41kecS4TxBaHJvyJ607PQ@mail.gmail.com>
References: <20240312095005.8909-1-maimon.sagi@gmail.com>
 <7bf7d444-4a08-4df4-9aa1-9cd28609d166@app.fastmail.com>
 <CAMuE1bGkZ=ifyofCUfm4JVS__dgYG41kecS4TxBaHJvyJ607PQ@mail.gmail.com>
Date: Tue, 12 Mar 2024 14:47:32 +0100
From: "Arnd Bergmann" <arnd@arndb.de>
To: "Sagi Maimon" <maimon.sagi@gmail.com>
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
Content-Type: text/plain;charset=utf-8
Content-Transfer-Encoding: quoted-printable

On Tue, Mar 12, 2024, at 13:15, Sagi Maimon wrote:
> On Tue, Mar 12, 2024 at 1:19=E2=80=AFPM Arnd Bergmann <arnd@arndb.de> =
wrote:
>> On Tue, Mar 12, 2024, at 10:50, Sagi Maimon wrote:
>> > +     kc_a =3D clockid_to_kclock(clock_a);
>> > +     if (!kc_a) {
>> > +             error =3D -EINVAL;
>> > +             return error;
>> > +     }
>> > +
>> > +     kc_b =3D clockid_to_kclock(clock_b);
>> > +     if (!kc_b) {
>> > +             error =3D -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=20
clock_get_crosstimespec(). Rather than averaging between
two snapshots here, I would expect this to result in
something like

      ktime_a1 +=3D 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?

>> > +     if (crosstime_support_a) {
>> > +             ktime_a1 =3D xtstamp_a1.device;
>> > +             ktime_a2 =3D xtstamp_a2.device;
>> > +     } else {
>> > +             ktime_a1 =3D timespec64_to_ktime(ts_a1);
>> > +             ktime_a2 =3D timespec64_to_ktime(ts_a2);
>> > +     }
>> > +
>> > +     ktime_a =3D ktime_add(ktime_a1, ktime_a2);
>> > +
>> > +     ts_offs =3D ktime_divns(ktime_a, 2);
>> > +
>> > +     ts_a1 =3D 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.

     Arnd