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

[Thomas Gleixner]

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

Errm. What I posted as sketch _is_ using gettimex64() with the extra
twist of the flag vs. a clockid (which is an implementation detail) and
the difference that I carry the information in ptp_system_timestamp
instead of needing a new argument clockid to all existing callbacks
because the modification to ptp_read_prets() and postts() will just be
sufficient, no?

For the case where the driver does not provide gettimex64() then the
extension of the original offset ioctl is still providing a better
mechanism than the proposed syscall.

I also clearly said that all drivers should be converted over to
gettimex64().

> 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.

I disagree. The existing gettimex64() is good enough if the driver
implements it correctly today. If not then those drivers need to be
fixed independent of this.

So assumed that a driver does:

gettimex64()
   ptp_prets(sts);
   read_clock();
   ptp_postts(sts);
   
today then having:

static inline void ptp_read_system_prets(struct ptp_system_timestamp *sts)
{
	if (sts) {
		if (sts->flags & PTP_SYS_OFFSET_MONO_RAW)
			ktime_get_raw_ts64(&sts->pre_ts);
		else
			ktime_get_real_ts64(&sts->pre_ts);
	}
}

static inline void ptp_read_system_postts(struct ptp_system_timestamp *sts)
{
	if (sts) {
		if (sts->flags & PTP_SYS_OFFSET_MONO_RAW)
			ktime_get_raw_ts64(&sts->post_ts);
		else
			ktime_get_real_ts64(&sts->post_ts);
	}
}

or

static inline void ptp_read_system_prets(struct ptp_system_timestamp *sts)
{
	if (sts) {
		switch (sts->clockid) {
                case CLOCK_MONOTONIC_RAW:
                	time_get_raw_ts64(&sts->pre_ts);
                        break;
                case CLOCK_REALTIME:
			ktime_get_real_ts64(&sts->pre_ts);
                        break;
                }        
	}
}

static inline void ptp_read_system_postts(struct ptp_system_timestamp *sts)
{
	if (sts) {
		switch (sts->clockid) {
                case CLOCK_MONOTONIC_RAW:
                	time_get_raw_ts64(&sts->post_ts);
                        break;
                case CLOCK_REALTIME:
			ktime_get_real_ts64(&sts->post_ts);
                        break;
                }        
	}
}

is doing the exact same thing as your proposal but without touching any
driver which implements gettimex64() correctly at all.

While your proposal requires to touch every single driver for no reason,
no?

It is just an implementation detail whether you use a flag or a
clockid. You can carry the clockid for the clocks which actually can be
read in that context in a reserved field of PTP_SYS_OFFSET_EXTENDED:

struct ptp_sys_offset_extended {
	unsigned int	n_samples; /* Desired number of measurements. */
	clockid_t	clockid;
	unsigned int	rsv[2];    /* Reserved for future use. */
};

and in the IOCTL:

        if (extoff->clockid != CLOCK_MONOTONIC_RAW)
        	return -EINVAL;

	sts.clockid = extoff->clockid;

and it all just works, no?

I have no problem to decide that PTP_SYS_OFFSET will not get this
treatment and the drivers have to be converted over to
PTP_SYS_OFFSET_EXTENDED.

But adding yet another callback just to carry a clockid as argument is a
more than pointless exercise as I demonstrated.

Thanks,

        tglx