Received-SPF: pass (google.com: domain of linux-kernel+bounces-128837-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) client-ip=147.75.80.249;
Precedence: bulk
MIME-Version: 1.0
References: <CAMuE1bHBky9NGP22PVHKdi2+WniwxiLSmMnwRM6wm36sU8W4jA@mail.gmail.com>
 <878r29hjds.ffs@tglx> <CAMuE1bF9ioo39_08Eh26X4WOtnvJ1geJ=WRVt5DhU8gEbYJNdA@mail.gmail.com>
 <87o7asdd65.ffs@tglx> <CAF2d9jjA8iM1AoPUhQPK62tdd7gPnCnt51f_NMhOAs546rU3dA@mail.gmail.com>
 <87il10ce1g.ffs@tglx> <CAF2d9jj6km7aVSqgcOE-b-A-WDH2TJNGzGy-5MRyw5HrzbqhaA@mail.gmail.com>
 <877chfcrx3.ffs@tglx>
In-Reply-To: <877chfcrx3.ffs@tglx>
From: =?UTF-8?B?TWFoZXNoIEJhbmRld2FyICjgpK7gpLngpYfgpLYg4KSs4KSC4KSh4KWH4KS14KS+4KSwKQ==?= <maheshb@google.com>
Date: Tue, 2 Apr 2024 16:37:27 -0700
Message-ID: <CAF2d9jjg0PEgPorXdrBHVkvz-fmUV7UXUPqnpQGVEvgXTpHY0A@mail.gmail.com>
Subject: Re: [PATCH v7] posix-timers: add clock_compare system call
To: Thomas Gleixner <tglx@linutronix.de>
Cc: Sagi Maimon <maimon.sagi@gmail.com>, richardcochran@gmail.com, luto@kernel.org, 
	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 Tue, Apr 2, 2024 at 3:37=E2=80=AFPM Thomas Gleixner <tglx@linutronix.de>=
 wrote:
>
> On Tue, Apr 02 2024 at 14:16, Mahesh Bandewar (=E0=A4=AE=E0=A4=B9=E0=A5=
=87=E0=A4=B6 =E0=A4=AC=E0=A4=82=E0=A4=A1=E0=A5=87=E0=A4=B5=E0=A4=BE=E0=A4=
=B0) wrote:
> > On Tue, Apr 2, 2024 at 2:25=E2=80=AFAM Thomas Gleixner <tglx@linutronix=
de> wrote:
> >> Works as well. I'm not seing the point for CLOCK_MONOTONIC and the
> >> change logs are not really telling anything about the problem being
> >> solved....
> >>
> > https://lore.kernel.org/lkml/20240104212431.3275688-1-maheshb@google.co=
m/T/#:~:text=3D*%20[PATCHv3%20net%2Dnext%200/3]%20add%20ptp_gettimex64any()=
%20API,21:24%20Mahesh%20Bandewar%200%20siblings%2C%200%20replies;
> >
> > This is the cover letter where I tried to explain the need for this.
>
> The justification for a patch needs to be in the change log and not in
> the cover letter because the cover letter is not part of the git
> history.
>
ack

> > Granted, my current use case is for CLOCK_MONOTONIC_RAW but just
> > because I don't have a use case doesn't mean someone else may not have
> > it and hence added it.
>
> Then why did you not five other clock IDs? Someone else might have a
> use case, no?
>
> While a syscall/ioctl should be flexible for future use, the kernel does
> not add features just because there might be some use case. It's
> documented how this works.
>
I see your point. I don't mind removing the CLOCK_MONOTONIC for now
and just have CLOCK_REALTIME and CLOCK_MONOTONIC_RAW support. Also as
I mentioned, it will be just a matter of adding new clock-ids and
support for the pre/post-ts for respective clock-ids if needed in the
future.

The modification that you have proposed (in a couple of posts back)
would work but it's still not ideal since the pre/post ts are not
close enough as they are currently  (properly implemented!)
gettimex64() would have. The only way to do that would be to have
another ioctl as I have proposed which is a superset of current
gettimex64 and pre-post collection is the closest possible.

Here is my sample mlx4 (since I use that) of the new ioctl method
(just for the reference)

--- a/drivers/net/ethernet/mellanox/mlx4/main.c
+++ b/drivers/net/ethernet/mellanox/mlx4/main.c
@@ -43,6 +43,7 @@
 #include <linux/io-mapping.h>
 #include <linux/delay.h>
 #include <linux/etherdevice.h>
+#include <linux/ptp_clock_kernel.h>
 #include <net/devlink.h>

 #include <uapi/rdma/mlx4-abi.h>
@@ -1929,7 +1930,7 @@ static void unmap_bf_area(struct mlx4_dev *dev)
                io_mapping_free(mlx4_priv(dev)->bf_mapping);
 }

-u64 mlx4_read_clock(struct mlx4_dev *dev)
+u64 mlx4_read_clock(struct mlx4_dev *dev, struct ptp_system_timestamp
*sts, int clkid)
 {
        u32 clockhi, clocklo, clockhi1;
        u64 cycles;
@@ -1937,7 +1938,13 @@ u64 mlx4_read_clock(struct mlx4_dev *dev)
        struct mlx4_priv *priv =3D mlx4_priv(dev);

        for (i =3D 0; i < 10; i++) {
-               clockhi =3D swab32(readl(priv->clock_mapping));
+               if (sts) {
+                       ptp_read_any_prets(sts, clkid);
+                       clockhi =3D swab32(readl(priv->clock_mapping));
+                       ptp_read_any_postts(sts, clkid);
+               } else {
+                       clockhi =3D swab32(readl(priv->clock_mapping));
+               }
                clocklo =3D swab32(readl(priv->clock_mapping + 4));
                clockhi1 =3D swab32(readl(priv->clock_mapping));
                if (clockhi =3D=3D clockhi1)

Having said that, the 'flag' modification proposal is a good backup
for the drivers that don't have good implementation (close enough but
not ideal). Also, you don't need a new ioctl-op. So if we really want
precision, I believe, we need a new ioctl op (with supporting
implementation similar to the mlx4 code above). but we want to save
the new ioctl-op and have less precision then proposed modification
would work fine.

> Thanks,
>
>         tglx
>