diff mbox series

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

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

Commit Message

Peter Hilber Aug. 18, 2023, 1:20 a.m. UTC 
  The cycle_between() helper checks if parameter test is in the open interval
(before, after). Colloquially speaking, this also applies to the counter
wrap-around special case before > after. get_device_system_crosststamp()
currently uses cycle_between() at the first call site to decide whether to
interpolate for older counter readings.

get_device_system_crosststamp() has the following problem with
cycle_between() testing against an open interval: Assume that, by chance,
cycles == tk->tkr_mono.cycle_last (in the following, "cycle_last" for
brevity). Then, cycle_between() at the first call site, with effective
argument values cycle_between(cycle_last, cycles, now), returns false,
enabling interpolation. During interpolation,
get_device_system_crosststamp() will then call cycle_between() at the
second call site (if a history_begin was supplied). The effective argument
values are cycle_between(history_begin->cycles, cycles, cycles), since
system_counterval.cycles == interval_start == cycles, per the assumption.
Due to the test against the open interval, cycle_between() returns false
again. This causes get_device_system_crosststamp() to return -EINVAL.

This failure should be avoided, since get_device_system_crosststamp() works
both when cycles follows cycle_last (no interpolation), and when cycles
precedes cycle_last (interpolation). For the case cycles == cycle_last,
interpolation is actually unneeded.

Fix this by disabling interpolation if cycles == cycle_last. Thereby, avoid
the above described corner case interpolation failure.

Fixes: 2c756feb18d9 ("time: Add history to cross timestamp interface supporting slower devices")
Signed-off-by: Peter Hilber <peter.hilber@opensynergy.com>
---

Notes:
    v2:
    
    - try to clarify problem description (John Stultz)
    - simplify fix

 kernel/time/timekeeping.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

Comments

Thomas Gleixner Sept. 15, 2023, 4:10 p.m. UTC | #1 
On Fri, Aug 18 2023 at 03:20, Peter Hilber wrote:
> --- a/kernel/time/timekeeping.c
> +++ b/kernel/time/timekeeping.c
> @@ -1247,7 +1247,8 @@ int get_device_system_crosststamp(int (*get_time_fn)
>  		 */
>  		now = tk_clock_read(&tk->tkr_mono);
>  		interval_start = tk->tkr_mono.cycle_last;
> -		if (!cycle_between(interval_start, cycles, now)) {
> +		if (!cycle_between(interval_start, cycles, now) &&
> +		    cycles != interval_start) {
>  			clock_was_set_seq = tk->clock_was_set_seq;
>  			cs_was_changed_seq = tk->cs_was_changed_seq;
>  			cycles = interval_start;

So the explanation in the changelog makes some sense, but this code
without any further explanation just makes my brain explode.

This whole thing screams for a change to cycle_between() so it becomes:

     timestamp_in_interval(start, end, ts)

and make start inclusive and not exclusive, no?

That's actually correct for both usage sites because for interpolation
the logic is the same. history_begin->cycles is a valid timestamp, no?

Thanks,

        tglx
Peter Hilber Sept. 15, 2023, 5:30 p.m. UTC | #2 
On 15.09.23 18:10, Thomas Gleixner wrote:
> On Fri, Aug 18 2023 at 03:20, Peter Hilber wrote:
>> --- a/kernel/time/timekeeping.c
>> +++ b/kernel/time/timekeeping.c
>> @@ -1247,7 +1247,8 @@ int get_device_system_crosststamp(int (*get_time_fn)
>>  		 */
>>  		now = tk_clock_read(&tk->tkr_mono);
>>  		interval_start = tk->tkr_mono.cycle_last;
>> -		if (!cycle_between(interval_start, cycles, now)) {
>> +		if (!cycle_between(interval_start, cycles, now) &&
>> +		    cycles != interval_start) {
>>  			clock_was_set_seq = tk->clock_was_set_seq;
>>  			cs_was_changed_seq = tk->cs_was_changed_seq;
>>  			cycles = interval_start;
> 
> So the explanation in the changelog makes some sense, but this code
> without any further explanation just makes my brain explode.
> 
> This whole thing screams for a change to cycle_between() so it becomes:
> 
>      timestamp_in_interval(start, end, ts)
> 
> and make start inclusive and not exclusive, no?

I tried like this in v1 (having 'end' inclusive as well), but didn't like
the effect at the second usage site.

> 
> That's actually correct for both usage sites because for interpolation
> the logic is the same. history_begin->cycles is a valid timestamp, no?

AFAIU, with the timestamp_in_interval() change, history_begin->cycles would
become a valid timestamp. To me it looks like
adjust_historical_crosststamp() should then work unmodified for now. But
one would have to be careful with the additional corner case in the future.

So, document the current one-line change, or switch to
timestamp_in_interval()?

Thanks for the review!

Peter
Thomas Gleixner Sept. 15, 2023, 7:02 p.m. UTC | #3 
On Fri, Sep 15 2023 at 19:30, Peter Hilber wrote:
> On 15.09.23 18:10, Thomas Gleixner wrote:
>> So the explanation in the changelog makes some sense, but this code
>> without any further explanation just makes my brain explode.
>> 
>> This whole thing screams for a change to cycle_between() so it becomes:
>> 
>>      timestamp_in_interval(start, end, ts)
>> 
>> and make start inclusive and not exclusive, no?
>
> I tried like this in v1 (having 'end' inclusive as well), but didn't like
> the effect at the second usage site.
>
>> 
>> That's actually correct for both usage sites because for interpolation
>> the logic is the same. history_begin->cycles is a valid timestamp, no?
>
> AFAIU, with the timestamp_in_interval() change, history_begin->cycles would
> become a valid timestamp. To me it looks like
> adjust_historical_crosststamp() should then work unmodified for now. But
> one would have to be careful with the additional corner case in the future.
>
> So, document the current one-line change, or switch to
> timestamp_in_interval()?

I really prefer the consistent function which treats the start as
inclusive as that makes the most sense and is self explanatory.

Thanks,

        tglx
diff mbox series

Patch

diff --git a/kernel/time/timekeeping.c b/kernel/time/timekeeping.c
index cd5c83473bab..70ecd44fdd9e 100644
--- a/kernel/time/timekeeping.c
+++ b/kernel/time/timekeeping.c
@@ -1247,7 +1247,8 @@  int get_device_system_crosststamp(int (*get_time_fn)
 		 */
 		now = tk_clock_read(&tk->tkr_mono);
 		interval_start = tk->tkr_mono.cycle_last;
-		if (!cycle_between(interval_start, cycles, now)) {
+		if (!cycle_between(interval_start, cycles, now) &&
+		    cycles != interval_start) {
 			clock_was_set_seq = tk->clock_was_set_seq;
 			cs_was_changed_seq = tk->cs_was_changed_seq;
 			cycles = interval_start;