Messages in this thread | | | From | Thomas Gleixner <> | Subject | Re: [PATCH v2 3/7] timecounter: allow for non-power of two overflow | Date | Wed, 24 Nov 2021 21:55:44 +0100 |
| |
Martin,
On Tue, Nov 09 2021 at 10:50, Martin Kaistra wrote: > * see CYCLECOUNTER_MASK() helper macro > * @mult: cycle to nanosecond multiplier > * @shift: cycle to nanosecond divisor (power of two) > + * @overflow_point: non-power of two overflow point (optional), > + * smaller than mask > */ > struct cyclecounter { > u64 (*read)(const struct cyclecounter *cc); > u64 mask; > u32 mult; > u32 shift; > + u64 overflow_point; > }; > > /** > diff --git a/kernel/time/timecounter.c b/kernel/time/timecounter.c > index e6285288d765..afd2910a9724 100644 > --- a/kernel/time/timecounter.c > +++ b/kernel/time/timecounter.c > @@ -39,6 +39,9 @@ static u64 timecounter_read_delta(struct timecounter *tc) > /* calculate the delta since the last timecounter_read_delta(): */ > cycle_delta = (cycle_now - tc->cycle_last) & tc->cc->mask; > > + if (tc->cc->overflow_point && (cycle_now - tc->cycle_last) > tc->cc->mask) > + cycle_delta -= tc->cc->mask - tc->cc->overflow_point;
TBH, this took me more than one twisted braincell to grok.
With support for clocks which do not wrap at power of 2 boundaries we already lose the unconditional fast path no matter what. So what's the point of having two conditions and doing this convoluted math here?
In timecounter_init():
tc->ovfl = cc->ovfl ? cc->ovfl : cc->mask + 1;
which makes it a common path in timecounter_read_delta():
cycle_delta = cycle_now - tc->cycle_last; if ((s64)cycle_delta) < 0) cycle_delta += tc->ovfl;
which produces way better binary code.
The conditional does not really matter for the timecounter use cases as that calculation is noise compared to the actual cc->read() access.
Aside of that the same problem exists in timecounter_cyc2time()...
After that we probably should do a treewide sweep to get rid of cc->mask to avoid confusion and subtle to understand errors when some code uses cc->mask instead of cc->ovfl.
Thanks,
tglx
| |