Messages in this thread | | | From | Thomas Gleixner <> | Subject | Re: [PATCH v4 02/11] timekeeping: Add function to convert realtime to base clock | Date | Thu, 08 Feb 2024 16:14:15 +0100 |
| |
On Wed, Feb 07 2024 at 11:38, lakshmi sowjanya d. wrote: > From: Lakshmi Sowjanya D <lakshmi.sowjanya.d@intel.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
| |