Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:2 as permitted sender) client-ip=2620:137:e000::3:2;
From:   Thomas Gleixner <tglx@linutronix.de>
To:     lakshmi.sowjanya.d@intel.com, jstultz@google.com,
        giometti@enneenne.com, corbet@lwn.net, linux-kernel@vger.kernel.org
Cc:     x86@kernel.org, linux-doc@vger.kernel.org,
        andriy.shevchenko@linux.intel.com, eddie.dong@intel.com,
        christopher.s.hall@intel.com, pandith.n@intel.com,
        mallikarjunappa.sangannavar@intel.com, thejesh.reddy.t.r@intel.com,
        lakshmi.sowjanya.d@intel.com
Subject: Re: [PATCH v1 1/6] kernel/time: Add system time to system counter
 conversion
In-Reply-To: <20231017052457.25287-2-lakshmi.sowjanya.d@intel.com>
References: <20231017052457.25287-1-lakshmi.sowjanya.d@intel.com>
 <20231017052457.25287-2-lakshmi.sowjanya.d@intel.com>
Date:   Tue, 17 Oct 2023 10:42:55 +0200
Message-ID: <87zg0h63n4.ffs@tglx>
MIME-Version: 1.0
Content-Type: text/plain
Precedence: bulk

Lakshmi!

On Tue, Oct 17 2023 at 10:54, lakshmi.sowjanya.d@intel.com wrote:

'kernel/time:' is not a proper subsystem prefix.

  https://www.kernel.org/doc/html/latest/process/maintainer-tip.html#patch-subject

> Support system-clock to system-counter conversion. Intel Timed IO
> hardware, using system counter as reference to schedule events.

This tells me WHAT the patch is doing but not at all WHY this is
required and that Intel Timed IO hardware reference is just not cutting
it.

> +extern int ktime_convert_real_to_system_counter(ktime_t sys_realtime,
> +						struct system_counterval_t *ret);

The changelog talks about system-clock. The function name and the
argument tell that this is about real-time. Thats confusing at best.

> +static inline int timekeeping_ns_to_delta(const struct tk_read_base *tkr, u64 nsec,
> +					  u64 *cycles)
> +{
> +	if (BITS_TO_BYTES(fls64(nsec) + tkr->shift) > sizeof(nsec))
> +		return -ERANGE;

Without a comment you will not be able to grok that check in three
months from now.

> +	*cycles = nsec << tkr->shift;
> +	*cycles -= tkr->xtime_nsec;
> +	do_div(*cycles, tkr->mult);
> +
> +	return 0;
> +}
> +
>  static inline u64 timekeeping_delta_to_ns(const struct tk_read_base *tkr, u64 delta)
>  {
>  	u64 nsec;
> @@ -1303,6 +1316,47 @@ int get_device_system_crosststamp(int (*get_time_fn)
>  }
>  EXPORT_SYMBOL_GPL(get_device_system_crosststamp);
>  
> +/**
> + * ktime_convert_real_to_system_counter - Convert system time to system counter

System time is _NOT_ the same as clock realtime, really. What's wrong
with consistently using clock realtime instead of making up some new
terminology?

> + * value

Pointless line break which makes this unreadable.

> + * @sys_realtime:	realtime clock value to convert

What means sys_realtime? The argument supplies an absolute time value
based on clock realtime and not on some magic system realtime.

> + * @ret:		Computed system counter value with clocksource pointer

So @ret is returning the computed value along with a clocksource pointer
or what?

> + * Converts a supplied, future realtime clock value to the corresponding
> + * system counter value.
> + *
> + * Return: 0 on success, -errno on failure.

If this really needs error codes, then please explicitly document
them. If not, then make the function return bool and be done with it.

> + */
> +int ktime_convert_real_to_system_counter(ktime_t sys_realtime,
> +					 struct system_counterval_t *ret)
> +{
> +	struct timekeeper *tk = &tk_core.timekeeper;
> +	ktime_t base_real;
> +	unsigned int seq;
> +	u64 ns_delta;
> +	int err;
> +
> +	do {
> +		seq = read_seqcount_begin(&tk_core.seq);
> +
> +		base_real = ktime_add(tk->tkr_mono.base,
> +				      tk_core.timekeeper.offs_real);
> +		if (ktime_compare(sys_realtime, base_real) < 0)
> +			return -EINVAL;
> +
> +		ret->cs = tk->tkr_mono.clock;
> +		ns_delta = ktime_to_ns(ktime_sub(sys_realtime, base_real));
> +		err = timekeeping_ns_to_delta(&tk->tkr_mono, ns_delta, &ret->cycles);
> +		if (err < 0)
> +			return err;

This is completely uncomprehensible especially with the hidden range
check in the conversion function. All of this can be simplified to:

bool ktime_real_to_system_counter(ktime_t treal, struct system_counterval_t *scv)
{
	struct timekeeper *tk = &tk_core.timekeeper;
	unsigned int seq;
	u64 delta;

	do {
		seq = read_seqcount_begin(&tk_core.seq);

	        delta = (u64)treal - tk->tkr_mono.base_real;

		if (delta > tk->tkr_mono.clock->max_idle_ns)
                	return false;

                scv->cycles = timekeeping_ns_to_delta(&tk->tkr_mono, delta);
                scv->cycles += tk->tkr_mono.cycle_last;

		scv->cs = tk->tkr_mono.clock;
	} while (read_seqcount_retry(&tk_core.seq, seq));

	return true;
}

That gets rid of this hideous range check in the conversion function and
makes the code simple and straight forward, no?

Related question: What guarantee that the supplied value is not in the
past?

Nothing. It only guarantees that it is not before the realtime base
value, which can be up to one second in the past.

Maybe it does not matter, but then the 'future realtime clock value' in
the function documentation is wishful thinking.

Thanks,

        tglx