Received-SPF: pass (google.com: domain of linux-kernel+bounces-100248-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:40f1:3f00::1 as permitted sender) client-ip=2604:1380:40f1:3f00::1;
Precedence: bulk
MIME-Version: 1.0
References: <20240312095005.8909-1-maimon.sagi@gmail.com> <7bf7d444-4a08-4df4-9aa1-9cd28609d166@app.fastmail.com>
In-Reply-To: <7bf7d444-4a08-4df4-9aa1-9cd28609d166@app.fastmail.com>
From: Sagi Maimon <maimon.sagi@gmail.com>
Date: Tue, 12 Mar 2024 14:15:57 +0200
Message-ID: <CAMuE1bGkZ=ifyofCUfm4JVS__dgYG41kecS4TxBaHJvyJ607PQ@mail.gmail.com>
Subject: Re: [PATCH v6] posix-timers: add clock_compare system call
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>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

Hi Arnd
Thanks for you comments.

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:
> > Some user space applications need to read a couple of different clocks.
> > Each read requires moving from user space to kernel space.
> > Reading each clock separately (syscall) introduces extra
> > unpredictable/unmeasurable delay. Minimizing this delay contributes to =
user
> > space actions on these clocks (e.g. synchronization etc).
> >
> > Introduce a new system call clock_compare, which can be used to measure
> > the offset between two clocks, from variety of types: PHC, virtual PHC
> > and various system clocks (CLOCK_REALTIME, CLOCK_MONOTONIC, etc).
> > The system call returns the clocks timestamps.
> >
> > When possible, use crosstimespec to sync read values.
> > Else, read clock A twice (before, and after reading clock B) and averag=
e these
> > times =E2=80=93 to be as close as possible to the time we read clock B.
> >
> > Signed-off-by: Sagi Maimon <maimon.sagi@gmail.com>
>
> I like this a lot better than the previous versions I looked at,
> so just a few ideas here how this might be improved further.
>
> > +/**
> > + * clock_compare - Get couple of clocks time stamps
> > + * @clock_a: clock a ID
> > + * @clock_b: clock b ID
> > + * @tp_a:            Pointer to a user space timespec64 for clock a st=
orage
> > + * @tp_b:            Pointer to a user space timespec64 for clock b st=
orage
> > + *
> > + * clock_compare gets time sample of two clocks.
> > + * Supported clocks IDs: PHC, virtual PHC and various system clocks.
> > + *
> > + * In case of PHC that supports crosstimespec and the other clock is
> > Monotonic raw
> > + * or system time, crosstimespec will be used to synchronously capture
> > + * system/device time stamp.
> > + *
> > + * In other cases: Read clock_a twice (before, and after reading
> > clock_b) and
> > + * average these times =E2=80=93 to be as close as possible to the tim=
e we
> > read clock_b.
> > + *
> > + * Returns:
> > + *   0               Success. @tp_a and @tp_b contains the time stamps
> > + *   -EINVAL         @clock a or b ID is not a valid clock ID
> > + *   -EFAULT         Copying the time stamp to @tp_a or @tp_b faulted
> > + *   -EOPNOTSUPP     Dynamic POSIX clock does not support crosstimespe=
c()
> > + **/
> > +SYSCALL_DEFINE5(clock_compare, const clockid_t, clock_a, const
> > clockid_t, clock_b,
> > +             struct __kernel_timespec __user *, tp_a, struct __kernel_=
timespec
> > __user *,
> > +             tp_b, int64_t __user *, offs_err)
>
> The system call is well-formed in the way that the ABI is the
> same across all supported architectures, good.
>
> A minor issue is the use of int64_t, which in user interfaces
> can cause namespace problems. Please change that to the kernel
> side __s64 type.
>
you are right - it will be fixed on the next patch
> > +     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.
> > +     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.
Sagi
>      Arnd