linux-kernel - Re: [PATCH v7] posix-timers: add clock

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAF2d9jjg0PEgPorXdrBHVkvz-fmUV7UXUPqnpQGVEvgXTpHY0A@mail.gmail.com>
Date: Tue, 2 Apr 2024 16:37:27 -0700
From: Mahesh Bandewar (महेश बंडेवार) <maheshb@...gle.com>
To: Thomas Gleixner <tglx@...utronix.de>
Cc: Sagi Maimon <maimon.sagi@...il.com>, richardcochran@...il.com, luto@...nel.org, 
	mingo@...hat.com, bp@...en8.de, dave.hansen@...ux.intel.com, x86@...nel.org, 
	hpa@...or.com, arnd@...db.de, geert@...ux-m68k.org, peterz@...radead.org, 
	hannes@...xchg.org, sohil.mehta@...el.com, rick.p.edgecombe@...el.com, 
	nphamcs@...il.com, palmer@...ive.com, keescook@...omium.org, 
	legion@...nel.org, mark.rutland@....com, mszeredi@...hat.com, 
	casey@...aufler-ca.com, reibax@...il.com, davem@...emloft.net, 
	brauner@...nel.org, linux-kernel@...r.kernel.org, linux-api@...r.kernel.org, 
	linux-arch@...r.kernel.org, netdev@...r.kernel.org
Subject: Re: [PATCH v7] posix-timers: add clock_compare system call

On Tue, Apr 2, 2024 at 3:37 PM Thomas Gleixner <tglx@...utronix.de> wrote:
>
> On Tue, Apr 02 2024 at 14:16, Mahesh Bandewar (महेश बंडेवार) wrote:
> > On Tue, Apr 2, 2024 at 2:25 AM Thomas Gleixner <tglx@...utronixde> 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.com/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
>