Received-SPF: pass (google.com: domain of linux-kernel+bounces-100164-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.48.161 as permitted sender) client-ip=147.75.48.161;
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: <7bf7d444-4a08-4df4-9aa1-9cd28609d166@app.fastmail.com>
In-Reply-To: <20240312095005.8909-1-maimon.sagi@gmail.com>
References: <20240312095005.8909-1-maimon.sagi@gmail.com>
Date: Tue, 12 Mar 2024 12:17:48 +0100
From: "Arnd Bergmann" <arnd@arndb.de>
To: "Sagi Maimon" <maimon.sagi@gmail.com>,
 "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>
Cc: 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 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 avera=
ge 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 storage
> + * @tp_b:		Pointer to a user space timespec64 for clock b storage
> + *
> + * 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=20
> 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=20
> clock_b) and
> + * average these times =E2=80=93 to be as close as possible to the ti=
me we=20
> 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 crosstimespec()
> + **/
> +SYSCALL_DEFINE5(clock_compare, const clockid_t, clock_a, const=20
> clockid_t, clock_b,
> +		struct __kernel_timespec __user *, tp_a, struct __kernel_timespec=20
> __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.

> +	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?

> +	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.

     Arnd