diff mbox series

[RFC,2/7] timekeeping: Fix cross-timestamp interpolation corner case decision

Message ID 20230630171052.985577-3-peter.hilber@opensynergy.com
State New
Headers
Series [RFC,1/7] timekeeping: Fix cross-timestamp interpolation on counter wrap |

Commit Message

Peter Hilber June 30, 2023, 5:10 p.m. UTC 
  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.

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(-)

Comments

John Stultz July 7, 2023, 11:02 p.m. UTC | #1 
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
Peter Hilber July 27, 2023, 10:21 a.m. UTC | #2 
On 08.07.23 01:02, John Stultz wrote:
> 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.
> 

Thanks for the feedback, I'll post v2 soon and will try to come up with
a better commit message.
diff mbox series

Patch

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;
 }
@@ -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)