> -----Original Message-----
> From: Thomas Gleixner <[email protected]>
> Sent: Thursday, April 11, 2024 3:03 AM
> To: D, Lakshmi Sowjanya <[email protected]>;
> [email protected]; [email protected]; [email protected]; linux-
> [email protected]
> Cc: [email protected]; [email protected]; [email protected]; intel-
> [email protected]; [email protected]; Dong, Eddie
> <[email protected]>; Hall, Christopher S <[email protected]>;
> Brandeburg, Jesse <[email protected]>; [email protected];
> [email protected]; [email protected];
> [email protected]; [email protected]; [email protected];
> Nguyen, Anthony L <[email protected]>;
> [email protected]; N, Pandith <[email protected]>; Mohan,
> Subramanian <[email protected]>; T R, Thejesh Reddy
> <[email protected]>; D, Lakshmi Sowjanya
> <[email protected]>
> Subject: Re: [PATCH v6 01/11] x86/tsc: Add base clock properties in
> clocksource
> structure
>
> On Wed, Apr 10 2024 at 17:18, [email protected] 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
Agreed.
Will add a check for error case:
if (!convert_clock(&scv->cycles, num, den))
return false;
Thanks,
Sowjanya
>
> Thanks,
>
> tglx
