On 07/06/2022 11.14, Thomas Gleixner wrote:
Alexei,
On Mon, Jun 06 2022 at 08:57, Alexei Starovoitov wrote:
On Mon, Jun 6, 2022 at 3:38 AM Kurt Kanzenbach <kurt@xxxxxxxxxxxxx> wrote:
From: Jesper Dangaard Brouer <brouer@xxxxxxxxxx>
Commit 3dc6ffae2da2 ("timekeeping: Introduce fast accessor to clock tai")
introduced a fast and NMI-safe accessor for CLOCK_TAI. Especially in time
sensitive networks (TSN), where all nodes are synchronized by Precision Time
Protocol (PTP), it's helpful to have the possibility to generate timestamps
based on CLOCK_TAI instead of CLOCK_MONOTONIC. With a BPF helper for TAI in
place, it becomes very convenient to correlate activity across different
machines in the network.
That's a fresh feature. It feels risky to bake it into uapi already.
What? That's just support for a different CLOCK. What's so risky about
it?
I didn't think it was "risky" as this is already exported as:
EXPORT_SYMBOL_GPL(ktime_get_tai_fast_ns);
Correct me if I'm wrong, but this simple gives BPF access to CLOCK_TAI
(see man clock_gettime(2)), right?
And CLOCK_TAI is not really a new/fresh type of CLOCK.
Especially for networking we need this CLOCK_TAI time as HW LaunchTime
need this (e.g. see qdisc's sch_etf.c and sch_taprio.c).
imo it would be better to annotate tk_core variable in vmlinux BTF.
Then progs will be able to read all possible timekeeper offsets.
We are exposing APIs. APIs can be supported, but exposing implementation
details creates ABIs of the worst sort because that prevents the kernel
from changing the implementation. We've seen the fallout with the recent
tracepoint changes already.
Hmm... annotate tk_core variable in vmlinux BTF and letting BPF progs
access this seems like an unsafe approach and we tempt BPF-developers to
think other parts are okay to access.
Accessing timekeeper->offs_tai might be okay as it is already "marked"
with data_race(tk->offs_tai), but I'm not sure about other members, as
I'm not expert in this area.
I assume that the include filename <linux/timekeeper_internal.h>
indicate that the maintainers don't want to open up access to struct
timekeeper...
--Jesper
