On Thu, Mar 14, 2024 at 02:19:39PM +0200, Sagi Maimon wrote:

On Thu, Mar 14, 2024 at 1:12 PM Thomas Gleixner [off-list ref] wrote:

quoted

On Thu, Mar 14 2024 at 11:05, Sagi Maimon wrote:

quoted

+     if (crosstime_support_a) {
+             ktime_a = ktime_sub(xtstamp_a2.device, xtstamp_a1.device);
+             ts_offs_err = ktime_divns(ktime_a, 2);
+             ktime_a = ktime_add_ns(xtstamp_a1.device, (u64)ts_offs_err);
+             ts_a1 = 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.

you explained it before, but still it is better then two consecutive
user space calls which are also preemptible
and the userspace to kernel context switch time is added.

How much "better" is that in reality?

The time for a user<->kernel transition should be trivial relative to the time
a task spends not running after having been preempted.

Either:

(a) Your userspace application can handle the arbitrary delta resulting from a
    preemption, in which case the trivial cost shouldn't matter.

    i.e. this patch *is not necessary* to solve your problem.

(b) Your userspace application cannot handle the arbitrary delta resulting from
    a preemption, in which case you need to do something to handle that, which
    you haven't described at all.
  
    i.e. with the information you have provided so far, this patch is
    *insufficient* to solve your problem.

quoted

quoted

+ * In other cases: Read clock_a twice (before, and after reading clock_b) and
+ * average these times – to be as close as possible to the time 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?

100% agreed.

Please, explain the actual problem you are solving here. What *specifically*
are you trying to do in userspace with these values? "Synchronization" is too
vague a description.

Making what is already the best case *marginally better* without handling the
common and worst cases is a waste of time. It doesn't actually solve the
problem, and it misleads people into thinknig that a problem is solved when it
is not.

Mark.