Received-SPF: pass (google.com: domain of linux-kernel+bounces-103525-linux.lists.archive=gmail.com@vger.kernel.org designates 139.178.88.99 as permitted sender) client-ip=139.178.88.99;
Precedence: bulk
MIME-Version: 1.0
References: <20240314090540.14091-1-maimon.sagi@gmail.com> <87a5n1m5j1.ffs@tglx>
In-Reply-To: <87a5n1m5j1.ffs@tglx>
From: Sagi Maimon <maimon.sagi@gmail.com>
Date: Thu, 14 Mar 2024 17:46:30 +0200
Message-ID: <CAMuE1bHOm2Y1bOpggStMOjZhN5TaxoC1gJea5Mdrc+mormQg0g@mail.gmail.com>
Subject: Re: [PATCH v7] posix-timers: add clock_compare system call
To: Thomas Gleixner <tglx@linutronix.de>
Cc: richardcochran@gmail.com, luto@kernel.org, datglx@linutronix.de, 
	mingo@redhat.com, bp@alien8.de, dave.hansen@linux.intel.com, x86@kernel.org, 
	hpa@zytor.com, arnd@arndb.de, geert@linux-m68k.org, peterz@infradead.org, 
	hannes@cmpxchg.org, sohil.mehta@intel.com, rick.p.edgecombe@intel.com, 
	nphamcs@gmail.com, palmer@sifive.com, keescook@chromium.org, 
	legion@kernel.org, mark.rutland@arm.com, mszeredi@redhat.com, 
	casey@schaufler-ca.com, reibax@gmail.com, davem@davemloft.net, 
	brauner@kernel.org, linux-kernel@vger.kernel.org, linux-api@vger.kernel.org, 
	linux-arch@vger.kernel.org, netdev@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

On Thu, Mar 14, 2024 at 1:12=E2=80=AFPM Thomas Gleixner <tglx@linutronix.de=
> wrote:
>
> On Thu, Mar 14 2024 at 11:05, 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).
>
> I asked for a proper description of the actual problem several times now
> and you still provide some handwaving blurb. Feel free to ignore me, but
> then please don't be surprised if I ignore you too.
>
> Also why does reading two random clocks make any sense at all? Your code
> allows to read CLOCK_TAI and CLOCK_THREAD_CPUTIME_ID. What for?
>
> This is about PTP, no?
>
> Again you completely fail to explain why the existing PTP ioctls are not
> sufficient for the purpose and why you need to provide a new interface
> which is completely ill defined.
>
> > arch/x86/entry/syscalls/syscall_64.tbl |   1 +
> > drivers/ptp/ptp_clock.c                |  34 ++++--
> > include/linux/posix-clock.h            |   2 +
> > include/linux/syscalls.h               |   4 +
> > include/uapi/asm-generic/unistd.h      |   5 +-
> > kernel/time/posix-clock.c              |  25 +++++
> > kernel/time/posix-timers.c             | 138 +++++++++++++++++++++++++
> > kernel/time/posix-timers.h             |   2 +
>
> This needs to be split up into:
>
>      1) Infrastructure in posix-timers.c
>      2) Wire up the syscall in x86
>      3) Infrastructure in posix-clock.c
>      4) Usage in ptp_clock.c
>
> and not as a big lump of everything.
>
> > +/**
> > + * clock_compare - Get couple of clocks time stamps
>
> So the name of the syscall suggest that it compares two clocks, but the
> actual functionality is to read two clocks.
>
> This does not make any sense at all. Naming matters.
>
> > + * @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 M=
onotonic 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, s64 __user *, offs_err)
> > +{
> > +     struct timespec64 ts_a, ts_a1, ts_b, ts_a2;
> > +     struct system_device_crosststamp xtstamp_a1, xtstamp_a2, xtstamp_=
b;
> > +     const struct k_clock *kc_a, *kc_b;
> > +     ktime_t ktime_a;
> > +     s64 ts_offs_err =3D 0;
> > +     int error =3D 0;
> > +     bool crosstime_support_a =3D false;
> > +     bool crosstime_support_b =3D false;
>
> Please read and follow the documentation provided at:
>
> https://www.kernel.org/doc/html/latest/process/maintainer-tip.html
>
I have missed this part on prviews reply.
I have read the documentation above and I think that the variable
declarations at the beginning of a function is in reverse fir tree
order
meaning from big to small, but I guess that I am missing something,
can you please explain what is wrong with the variable declaration,
so I can fix it.
> > +     kc_a =3D clockid_to_kclock(clock_a);
> > +     if (!kc_a) {
> > +             error =3D -EINVAL;
> > +             return error;
>
> What's wrong about 'return -EINVAL;' ?
>
> > +     }
> > +
> > +     kc_b =3D clockid_to_kclock(clock_b);
> > +     if (!kc_b) {
> > +             error =3D -EINVAL;
> > +             return error;
> > +     }
> > +
> > +     // In case crosstimespec supported and b clock is Monotonic raw o=
r system
> > +     // time, synchronously capture system/device time stamp
>
> Please don't use C++ comments.
>
> > +     if (clock_a < 0) {
>
> This is just wrong. posix-clocks ar not the only ones which have a
> negative clock id. See clockid_to_kclock()
>
> > +             error =3D kc_a->clock_get_crosstimespec(clock_a, &xtstamp=
_a1);
>
> What's a crosstimespec?
>
> This function fills in a system_device_crosststamp, so why not name it
> so that the purpose of the function is obvious?
>
> ptp_clock::info::getcrosststamp() has a reasonable name. So why do you
> need to come up with something which makes the code obfuscated?
>
> > +             if (!error) {
> > +                     if (clock_b =3D=3D CLOCK_MONOTONIC_RAW) {
> > +                             ts_b =3D ktime_to_timespec64(xtstamp_a1.s=
ys_monoraw);
> > +                             ts_a1 =3D ktime_to_timespec64(xtstamp_a1.=
device);
> > +                             goto out;
> > +                     } else if (clock_b =3D=3D CLOCK_REALTIME) {
> > +                             ts_b =3D ktime_to_timespec64(xtstamp_a1.s=
ys_realtime);
> > +                             ts_a1 =3D ktime_to_timespec64(xtstamp_a1.=
device);
> > +                             goto out;
> > +                     } else {
> > +                             crosstime_support_a =3D true;
>
> Right. If clock_b is anything else than CLOCK_MONOTONIC_RAW or
> CLOCK_REALTIME then this is true.
>
> > +                     }
> > +             }
>
> So in case of an error, this just keeps going with an uninitialized
> xtstamp_a1 and if the clock_b part succeeds it continues and operates on
> garbage.
>
> > +     }
> > +
> > +     // In case crosstimespec supported and a clock is Monotonic raw o=
r system
> > +     // time, synchronously capture system/device time stamp
> > +     if (clock_b < 0) {
> > +             // Synchronously capture system/device time stamp
> > +             error =3D kc_b->clock_get_crosstimespec(clock_b, &xtstamp=
_b);
> > +             if (!error) {
> > +                     if (clock_a =3D=3D CLOCK_MONOTONIC_RAW) {
> > +                             ts_a1 =3D ktime_to_timespec64(xtstamp_b.s=
ys_monoraw);
> > +                             ts_b =3D ktime_to_timespec64(xtstamp_b.de=
vice);
> > +                             goto out;
> > +                     } else if (clock_a =3D=3D CLOCK_REALTIME) {
> > +                             ts_a1 =3D ktime_to_timespec64(xtstamp_b.s=
ys_realtime);
> > +                             ts_b =3D ktime_to_timespec64(xtstamp_b.de=
vice);
> > +                             goto out;
> > +                     } else {
> > +                             crosstime_support_b =3D true;
> > +                     }
> > +             }
> > +     }
> > +
> > +     if (crosstime_support_a)
> > +             error =3D kc_a->clock_get_crosstimespec(clock_a, &xtstamp=
_a1);
>
> What? crosstime_support_a is only true when the exactly same call
> returned success. Why does it need to be called here again?
>
> > +     else
> > +             error =3D kc_a->clock_get_timespec(clock_a, &ts_a1);
> > +
> > +     if (error)
> > +             return error;
> > +
> > +     if (crosstime_support_b)
> > +             error =3D kc_b->clock_get_crosstimespec(clock_b, &xtstamp=
_b);
> > +     else
> > +             error =3D kc_b->clock_get_timespec(clock_b, &ts_b);
> > +
> > +     if (error)
> > +             return error;
> > +
> > +     if (crosstime_support_a)
> > +             error =3D kc_a->clock_get_crosstimespec(clock_a, &xtstamp=
_a2);
> > +     else
> > +             error =3D kc_a->clock_get_timespec(clock_a, &ts_a2);
> > +
> > +     if (error)
> > +             return error;
>
> The logic and the code flow here are unreadable garbage and there are
> zero comments what this is supposed to do.
>
> > +     if (crosstime_support_a) {
> > +             ktime_a =3D ktime_sub(xtstamp_a2.device, xtstamp_a1.devic=
e);
> > +             ts_offs_err =3D ktime_divns(ktime_a, 2);
> > +             ktime_a =3D ktime_add_ns(xtstamp_a1.device, (u64)ts_offs_=
err);
> > +             ts_a1 =3D ktime_to_timespec64(ktime_a);
>
> This is just wrong.
>
>      read(a1);
>      read(b);
>      read(a2);
>
> You _CANNOT_ assume that (a1 + ((a2 - a1) / 2) is anywhere close to the
> point in time where 'b' is read. This code is preemtible and
> interruptible. I explained this to you before.
>
> Your explanation in the comment above the function is just wishful
> thinking.
>
> > + * 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.
>
> Can you please sit down and provide a precise technical description of
> the problem you are trying to solve and explain your proposed solution
> at the conceptual level instead of throwing out random implementations
> every few days?
>
> Thanks,
>
>         tglx
>
>