Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:7 as permitted sender) client-ip=2620:137:e000::3:7;
MIME-Version: 1.0
References: <20231127153901.6399-1-maimon.sagi@gmail.com> <ZWUwTnWEHipJqHnk@hoboy.vegasvil.org>
In-Reply-To: <ZWUwTnWEHipJqHnk@hoboy.vegasvil.org>
From:   John Stultz <jstultz@google.com>
Date:   Mon, 27 Nov 2023 16:46:03 -0800
Message-ID: <CANDhNCq=iV2_1bzaP=BYuUwJtNsQBdjuYqUUtsiLc-MCNURJ6w@mail.gmail.com>
Subject: Re: [PATCH v2] posix-timers: add multi_clock_gettime system call
To:     Richard Cochran <richardcochran@gmail.com>
Cc:     Sagi Maimon <maimon.sagi@gmail.com>, reibax@gmail.com,
        davem@davemloft.net, rrameshbabu@nvidia.com,
        netdev@vger.kernel.org, linux-kernel@vger.kernel.org,
        maheshb@google.com, Thomas Gleixner <tglx@linutronix.de>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

On Mon, Nov 27, 2023 at 4:12=E2=80=AFPM Richard Cochran
<richardcochran@gmail.com> wrote:
>
> On Mon, Nov 27, 2023 at 05:39:01PM +0200, Sagi Maimon wrote:
> >  Some user space applications need to read some clocks.
> >  Each read requires moving from user space to kernel space.
> >  This asymmetry causes the measured offset to have a significant error.
>
> Adding time/clock gurus (jstultz, tglx) on CC for visibility...
>

Thanks for the heads up! (though, "guru" is just the noise I make
standing up these days)

> >  Introduce a new system call multi_clock_gettime, which can be used to =
measure
> >  the offset between multiple clocks, from variety of types: PHC, virtua=
l PHC
> >  and various system clocks (CLOCK_REALTIME, CLOCK_MONOTONIC, etc).
> >  The offset includes the total time that the driver needs to read the c=
lock
> >  timestamp.

This last bit about "offset includes the total time that the driver
needs to read the clock" is a bit confusing. It seems to suggest there
would be start/stop bookend timestamps so you could bound how long it
took to read all the clocks, but I don't see that in the patch.

> >  New system call allows the reading of a list of clocks - up to PTP_MAX=
_CLOCKS.
> >  Supported clocks IDs: PHC, virtual PHC and various system clocks.
> >  Up to PTP_MAX_SAMPLES times (per clock) in a single system call read.
> >  The system call returns n_clocks timestamps for each measurement:
> >  - clock 0 timestamp
> >  - ...
> >  - clock n timestamp
> >
> > Signed-off-by: Sagi Maimon <maimon.sagi@gmail.com>

Overally, while I understand the intent, I'm pretty hesitant on it
(and "__ptp_multi_clock_get multi_clk_get" has me squinting to find
the actual space amongst all the underscores :).

If the overhead of reading clockids individually is too much, it seems
like the next thing will be folks wanting to export multiple raw
hardware counter values so the counter->ns transformation doesn't get
inbetween each hw clock read, which this interface wouldn't solve, so
we'd have to add yet another interface.

Also, I wonder if trying to get multiple clocks in one read seems
similar to something uio_ring might help with? Though I can't say I'm
very savvy with uio_ring. Have folks looked into that?

thanks
-john