linux-kernel - Re: [PATCH v2 2/8] rust: hrtimer: Add HrTimer::raw

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <87plgv2s8g.fsf@kernel.org>
Date: Tue, 29 Apr 2025 12:10:23 +0200
From: Andreas Hindborg <a.hindborg@...nel.org>
To: "Lyude Paul" <lyude@...hat.com>
Cc: <rust-for-linux@...r.kernel.org>,  <linux-kernel@...r.kernel.org>,
  "Boqun Feng" <boqun.feng@...il.com>,  "FUJITA Tomonori"
 <fujita.tomonori@...il.com>,  "Frederic Weisbecker" <frederic@...nel.org>,
  "Thomas Gleixner" <tglx@...utronix.de>,  "Anna-Maria Behnsen"
 <anna-maria@...utronix.de>,  "John Stultz" <jstultz@...gle.com>,  "Stephen
 Boyd" <sboyd@...nel.org>,  "Miguel Ojeda" <ojeda@...nel.org>,  "Alex
 Gaynor" <alex.gaynor@...il.com>,  "Gary Guo" <gary@...yguo.net>,
  Björn
 Roy Baron <bjorn3_gh@...tonmail.com>,  "Benno Lossin"
 <benno.lossin@...ton.me>,  "Alice Ryhl" <aliceryhl@...gle.com>,  "Trevor
 Gross" <tmgross@...ch.edu>,  "Danilo Krummrich" <dakr@...nel.org>
Subject: Re: [PATCH v2 2/8] rust: hrtimer: Add HrTimer::raw_forward() and
 forward()

"Lyude Paul" <lyude@...hat.com> writes:

> oh - nevermind I get it but I think you made a mistake andreas, comment below
>
> On Fri, 2025-04-25 at 17:06 -0400, Lyude Paul wrote:
>>
>>
>> Perhaps I will understand this at some point after sending this email, but as
>> I'm writing this I have to admit I'm very confused. This is the first time
>> I've actually looked directly at the hrtimer_forward() source and I have to
>> say this is 100% not what I expected the term "overrun" to mean. Honestly,
>> enough so I'm kind of wondering if overrun is even the right word for the C
>> documentation to be using here.
>>
>> To make sure I'm understanding this right, an overrun is not "how many times
>> we would have executed the timer between now and the new execution time" (e.g.
>> "how many times did our new expiration value overrun the previous expiry
>> interval"). Instead it's actually "if the timer's next execution time is
>> greater than the previous expiry time then the timer will be forwarded by
>> `interval`, but if the timer's execution time is shorter than the previous
>> expiry time then the new execution time will be determined by figuring out if
>> the timer were to execute at `interval` what the closest expiry time at that
>> interval to the previous expiry time would be". Which, I'm afraid to admit
>> doesn't actually make any sense to me and makes me feel like "overrun" is
>> entirely the wrong word to be used here.
>>
>> I'm having a little trouble understanding how I'd really describe this in the
>> documentation because I'm also having a lot of trouble understanding why this
>> behavior is the way it is and why someone would want it to work like this.
>> Should this be something like "Forward the timer to the closest expiry time to
>> the current expiry time that can be reached if the timer were to execute at
>> the given interval"?. Or should I maybe just copy the C documentation as close
>> as possible and just leave this strange behavior as an exercise for the
>> reader?
>
> Yeah I think you misunderstood how the code works. Going to show how the code
> would run through using the last example you gave of:
>
>>   If the timer expires 5s after `now` and `interval` is 2s, then the
>>   expiry time is moved 4s forward and the return value is 2.
>
> The timer value wouldn't actually be moved forward here and the return value
> would be 0:
>
> u64 hrtimer_forward(struct hrtimer *timer, ktime_t now, ktime_t interval)
> //                                 ^ now+5         ^ 5          ^ 2
> //                                 = 10
> {
> 	u64 orun = 1;
> 	ktime_t delta;
>
> 	//                5  - 10 = -5
> 	delta = ktime_sub(now, hrtimer_get_expires(timer));
>
> 	// -5 < 0 = true
> 	if (delta < 0)
> 		return 0; // 0 overruns, timer executes at the same interval
>
> 	// (we don't execute the rest, so I've ommitted it)
> 	// ...
> }
> EXPORT_SYMBOL_GPL(hrtimer_forward);

Thanks for explaining, that makes a lot more sense 😅 I think I flipped the sign
of `delta`.

However, I still think the documentation is not correct. How is this instead:

  Conditionally forward the timer.

  If the timer expires after `now`, this function does
  nothing and returns 0.

  If the timer expired at or before `now`, this function forwards the timer by `interval`
  until the timer expires after `now` and then returns the number of times
  the timer was forwarded by `interval`.

  This is mainly useful for timer types etc etc ...

Best regards,
Andreas Hindborg