netdev - Re: [PATCH v4 02/11] timekeeping: Add function to convert realtime to base 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] [day] [month] [year] [list]

Message-ID: <8734u3m1iw.ffs@tglx>
Date: Thu, 08 Feb 2024 16:14:15 +0100
From: Thomas Gleixner <tglx@...utronix.de>
To: lakshmi.sowjanya.d@...el.com, jstultz@...gle.com, giometti@...eenne.com,
 corbet@....net, linux-kernel@...r.kernel.org
Cc: x86@...nel.org, netdev@...r.kernel.org, linux-doc@...r.kernel.org,
 intel-wired-lan@...ts.osuosl.org, andriy.shevchenko@...ux.intel.com,
 eddie.dong@...el.com, christopher.s.hall@...el.com,
 jesse.brandeburg@...el.com, davem@...emloft.net,
 alexandre.torgue@...s.st.com, joabreu@...opsys.com,
 mcoquelin.stm32@...il.com, perex@...ex.cz, linux-sound@...r.kernel.org,
 anthony.l.nguyen@...el.com, peter.hilber@...nsynergy.com,
 pandith.n@...el.com, mallikarjunappa.sangannavar@...el.com,
 subramanian.mohan@...el.com, thejesh.reddy.t.r@...el.com,
 lakshmi.sowjanya.d@...el.com
Subject: Re: [PATCH v4 02/11] timekeeping: Add function to convert realtime
 to base clock

On Wed, Feb 07 2024 at 11:38, lakshmi sowjanya d. wrote:
> From: Lakshmi Sowjanya D <lakshmi.sowjanya.d@...el.com>
>
> Introduce an interface, ktime_real_to_base_clock() to convert realtime
> to base clock.
>
> Convert the base clock to the system clock using convert_base_to_cs() in
> get_device_system_crosststamp().
>
> Add the helper function timekeeping_clocksource_has_base(), to check
> whether the current clocksource has the same base clock.

Neither ktime_real_to_base_clock() nor
timekeeping_clocksource_has_base() are used anywhere.

What's the point of having them in the first place?

Your changelog explains the WHAT but not the WHY....

> +static bool convert_clock(u64 *val, u32 numerator, u32 denominator)
> +{
> +	u64 rem, res;
> +
> +	if (numerator == 0 || denominator == 0)
> +		return false;

What's wrong with the usual (!numerator || !denominator) notation?

> +
> +	res = div64_u64_rem(*val, denominator, &rem) * numerator;
> +	*val = res + div_u64(rem * numerator, denominator);
> +	return true;
> +}
> +
> +static bool convert_base_to_cs(struct system_counterval_t *scv)
> +{
> +	struct clocksource *cs = tk_core.timekeeper.tkr_mono.clock;
> +	struct clocksource_base *base = cs->base;
> +
> +	/* The timestamp was taken from the time keeper clock source */
> +	if (cs->id == scv->cs_id)
> +		return true;
> +
> +	/* Check whether cs_id matches the base clock */
> +	if (!base || base->id != scv->cs_id)
> +		return false;
> +
> +	/* Avoid conversion to a less precise clock */
> +	if (scv->nsecs && cs->freq_khz != 0 && base->freq_khz < cs->freq_khz) {
> +		if (!convert_clock(&scv->cycles, cs->freq_khz, USEC_PER_SEC))
> +			return false;
> +	} else {
> +		if (scv->nsecs) {
> +			if (!convert_clock(&scv->cycles, base->freq_khz, USEC_PER_SEC))
> +				return false;
> +		}
> +		if (!convert_clock(&scv->cycles, base->numerator, base->denominator))
> +			return false;
> +	}

The above logic makes my brain hurt.

It's a reaonable requirement that cs->freq must be != 0 when sc->base !=
NULL and then converting from nanoseconds can always use cs->freq no
matter what the value of the base frequency is. Even for the case where
the base frequency is larger than cs->freq because the double conversion
does not give you more precision, right?

> +	scv->cycles += base->offset;

So the whole thing can be reduced to:

   nom = scv->nsecs ? cs->freq_khz : base->numerator;
   den = scv->nsecs ? USEC_PER_SEC : base->denominator;

   convert(&scv->cycles, nom, den);
   scv->cycles += base->offset;

Thanks,

        tglx