Messages in this thread | ![/](/images/icornerl.gif) | | From | Thomas Gleixner <> | Subject | Re: [PATCH v6 01/11] x86/tsc: Add base clock properties in clocksource structure | Date | Wed, 10 Apr 2024 23:32:46 +0200 |
| |
On Wed, Apr 10 2024 at 17:18, lakshmi.sowjanya.d@intel.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
| ![\](/images/icornerr.gif) |