Re: [PATCH v7] posix-timers: add clock_compare system call

[Mahesh Bandewar (महेश बंडेवार)]

On Tue, Apr 2, 2024 at 3:37 PM Thomas Gleixner <tglx@xxxxxxxxxxxxx> wrote:
>
> On Tue, Apr 02 2024 at 14:16, Mahesh Bandewar (महेश बंडेवार) wrote:
> > On Tue, Apr 2, 2024 at 2:25 AM Thomas Gleixner <tglx@linutronixde> 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@xxxxxxxxxx/T/#:~:text=*%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 = mlx4_priv(dev);

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