netdev - Re: [PATCH v6 01/11] x86/tsc: Add base clock properties in clocksource structure

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: <87cyqw3nup.ffs@tglx>
Date: Wed, 10 Apr 2024 23:32:46 +0200
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, subramanian.mohan@...el.com,
 thejesh.reddy.t.r@...el.com, lakshmi.sowjanya.d@...el.com
Subject: Re: [PATCH v6 01/11] x86/tsc: Add base clock properties in
 clocksource structure

On Wed, Apr 10 2024 at 17:18, lakshmi.sowjanya.d@...el.com wrote:
> @@ -48,6 +49,7 @@ struct module;
>   * @archdata:		Optional arch-specific data
>   * @max_cycles:		Maximum safe cycle value which won't overflow on
>   *			multiplication
> + * @freq_khz:		Clocksource frequency in khz.
>   * @name:		Pointer to clocksource name
>   * @list:		List head for registration (internal)
>   * @rating:		Rating value for selection (higher is better)
> @@ -70,6 +72,8 @@ struct module;
>   *			validate the clocksource from which the snapshot was
>   *			taken.
>   * @flags:		Flags describing special properties
> + * @base:		Hardware abstraction for clock on which a clocksource
> + *			is based
>   * @enable:		Optional function to enable the clocksource
>   * @disable:		Optional function to disable the clocksource
>   * @suspend:		Optional suspend function for the clocksource
> @@ -105,12 +109,14 @@ struct clocksource {
>  	struct arch_clocksource_data archdata;
>  #endif
>  	u64			max_cycles;
> +	u32			freq_khz;

Q: Why is this a bad place to add this member?

A: Because it creates a 4 byte hole in the data structure.

>  	const char		*name;
>  	struct list_head	list;

While adding it here fills a 4 byte hole.

Hint:

  pahole -c clocksource kernel/time/clocksource.o

would have told you that.

>  	int			rating;
>  	enum clocksource_ids	id;
>  	enum vdso_clock_mode	vdso_clock_mode;
>  	unsigned long		flags;
> +	struct clocksource_base *base;

> diff --git a/kernel/time/timekeeping.c b/kernel/time/timekeeping.c
> index b58dffc58a8f..2542cfefbdee 100644
> --- a/kernel/time/timekeeping.c
> +++ b/kernel/time/timekeeping.c
> @@ -1193,6 +1193,40 @@ static bool timestamp_in_interval(u64 start, u64 end, u64 ts)
>  	return false;
>  }
>  
> +static bool convert_clock(u64 *val, u32 numerator, u32 denominator)
> +{
> +	u64 rem, res;
> +
> +	if (!numerator || !denominator)
> +		return false;
> +
> +	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;
> +	u32 num, den;
> +
> +	/* 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;
> +
> +	num = scv->use_nsecs ? cs->freq_khz : base->numerator;
> +	den = scv->use_nsecs ? USEC_PER_SEC : base->denominator;
> +
> +	convert_clock(&scv->cycles, num, den);

Q: Why does this ignore the return value of convert_clock() ?

A: Because all drivers will correctly fill in everything.

Q: Then why does convert_clock() bother to check and have a return
   value?

A: Because drivers will fail to correctly fill in everything

Thanks,

        tglx