Messages in this thread | | | From | John Stultz <> | Date | Fri, 7 Jul 2023 16:02:26 -0700 | Subject | Re: [RFC PATCH 2/7] timekeeping: Fix cross-timestamp interpolation corner case decision |
| |
On Fri, Jun 30, 2023 at 10:12 AM Peter Hilber <peter.hilber@opensynergy.com> wrote: > > cycle_between() decides whether get_device_system_crosststamp() will > interpolate for older counter readings. So far, cycle_between() checks if > parameter test is in the open interval (before, after), when disregarding > the special case before > after. > > The only cycle_between() user, get_device_system_crosststamp(), has the > following problem with this: If interval_start == cycles, > cycle_between(interval_start, cycles, now) returns false. If a > history_begin was supplied to get_device_system_crosststamp(), it will > later call cycle_between() again, with effective argument values > cycle_between(history_begin->cycles, cycles, cycles). Due to the test > against the open interval, cycle_between() returns false again, and > get_device_system_crosststamp() returns -EINVAL, when it could have > succeeded. > > Fix this by testing against the closed interval in cycle_between(). This > disables interpolation if interval_start == cycles. For the special case > before > after, similar arguments hold. Fix this in a similar way. > > At the second cycle_between() call site, add an extra condition in order to > effectively check a half-open interval, which keeps the condition > documented above the call site satisfied.
I'm having a little bit of a hard time following this commit message. Do you think you might be able to take another swing at it to make it a bit clearer?
I get you're going from exclusive to inclusive intervals, but it's not very clear why this change is needed.
> Fixes: 2c756feb18d9 ("time: Add history to cross timestamp interface supporting slower devices") > Signed-off-by: Peter Hilber <peter.hilber@opensynergy.com> > --- > kernel/time/timekeeping.c | 7 ++++--- > 1 file changed, 4 insertions(+), 3 deletions(-) > > diff --git a/kernel/time/timekeeping.c b/kernel/time/timekeeping.c > index 8f35455b6250..7e86d5cd784d 100644 > --- a/kernel/time/timekeeping.c > +++ b/kernel/time/timekeeping.c > @@ -1180,13 +1180,13 @@ static int adjust_historical_crosststamp(struct system_time_snapshot *history, > } > > /* > - * cycle_between - true if test occurs chronologically between before and after > + * cycle_between - true if test occurs chronologically in [before, after] > */ > static bool cycle_between(u64 before, u64 test, u64 after) > { > - if (test > before && test < after) > + if (test >= before && test <= after) > return true; > - if (before > after && (test > before || test < after)) > + if (before > after && (test >= before || test <= after)) > return true; > return false; > }
I'm with you here.
> @@ -1282,6 +1282,7 @@ int get_device_system_crosststamp(int (*get_time_fn) > * clocksource change > */ > if (!history_begin || > + history_begin->cycles == system_counterval.cycles || > !cycle_between(history_begin->cycles, > system_counterval.cycles, cycles) || > history_begin->cs_was_changed_seq != cs_was_changed_seq) > --
Roughly I see you're trying to preserve the behavior here for the case a == b, which used to fail with cycles_between(a, b, c) but now passes. But it's unclear *why* we're making the change to begin with.
thanks -john
| |