| Message ID | 20241005122531.20298-4-fujita.tomonori@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | rust: Add IO polling | expand |
On Sat, Oct 05, 2024 at 09:25:28PM +0900, FUJITA Tomonori wrote: > Implement Add<Delta> for Ktime to support the operation: > > Ktime = Ktime + Delta > > This is used to calculate the future time when the timeout will occur. Since Delta can be negative, it could also be a passed time. For a timeout, that does not make much sense. > +impl core::ops::Add<Delta> for Ktime { > + type Output = Ktime; > + > + #[inline] > + fn add(self, delta: Delta) -> Ktime { > + // SAFETY: FFI call. > + let t = unsafe { bindings::ktime_add_ns(self.inner, delta.as_nanos() as u64) }; So you are throwing away the sign bit. What does Rust in the kernel do if it was a negative delta? I think the types being used here need more consideration. Andrew
On Sat, Oct 5, 2024 at 2:26 PM FUJITA Tomonori <fujita.tomonori@gmail.com> wrote: > > + fn add(self, delta: Delta) -> Ktime { > + // SAFETY: FFI call. > + let t = unsafe { bindings::ktime_add_ns(self.inner, delta.as_nanos() as u64) }; > + Ktime::from_raw(t) > + } I wonder if we want to use the `ktime` macros/operations for this type or not (even if we still promise it is the same type underneath). It means having to define helpers, adding `unsafe` code and `SAFETY` comments, a call penalty in non-LTO, losing overflow checking (if we want it for these types), and so on. (And at least C is `-fno-strict-overflow`, otherwise it would be even subtler). Cheers, Miguel
On 5 Oct 2024, at 20:07, Andrew Lunn wrote: > On Sat, Oct 05, 2024 at 09:25:28PM +0900, FUJITA Tomonori wrote: >> Implement Add<Delta> for Ktime to support the operation: >> >> Ktime = Ktime + Delta >> >> This is used to calculate the future time when the timeout will occur. > > Since Delta can be negative, it could also be a passed time. For a > timeout, that does not make much sense. > Are there more usecases than Delta? Would it make sense in that case to also implement Sub as well? Thanks, Fiona >> +impl core::ops::Add<Delta> for Ktime { >> + type Output = Ktime; >> + >> + #[inline] >> + fn add(self, delta: Delta) -> Ktime { >> + // SAFETY: FFI call. >> + let t = unsafe { bindings::ktime_add_ns(self.inner, delta.as_nanos() as u64) }; > > So you are throwing away the sign bit. What does Rust in the kernel do > if it was a negative delta? > > I think the types being used here need more consideration. > > Andrew
On Sun, 06 Oct 2024 12:45:06 +0200 Fiona Behrens <me@kloenk.dev> wrote: >> On Sat, Oct 05, 2024 at 09:25:28PM +0900, FUJITA Tomonori wrote: >>> Implement Add<Delta> for Ktime to support the operation: >>> >>> Ktime = Ktime + Delta >>> >>> This is used to calculate the future time when the timeout will occur. >> >> Since Delta can be negative, it could also be a passed time. For a >> timeout, that does not make much sense. >> > > Are there more usecases than Delta? Would it make sense in that case to also implement Sub as well? We might add the api to calculate the elapsed time when it becomes necessary: Delta = Ktime - Ktime
On Sat, 5 Oct 2024 20:36:44 +0200 Miguel Ojeda <miguel.ojeda.sandonis@gmail.com> wrote: > On Sat, Oct 5, 2024 at 2:26 PM FUJITA Tomonori > <fujita.tomonori@gmail.com> wrote: >> >> + fn add(self, delta: Delta) -> Ktime { >> + // SAFETY: FFI call. >> + let t = unsafe { bindings::ktime_add_ns(self.inner, delta.as_nanos() as u64) }; >> + Ktime::from_raw(t) >> + } > > I wonder if we want to use the `ktime` macros/operations for this type > or not (even if we still promise it is the same type underneath). It > means having to define helpers, adding `unsafe` code and `SAFETY` > comments, a call penalty in non-LTO, losing overflow checking (if we > want it for these types), and so on. Yeah, if we are allowed to touch ktime_t directly instead of using the accessors, it's great for the rust side. The timers maintainers, what do you think?
On Mon, Oct 7, 2024 at 8:17 AM FUJITA Tomonori <fujita.tomonori@gmail.com> wrote: > > On Sat, 5 Oct 2024 20:36:44 +0200 > Miguel Ojeda <miguel.ojeda.sandonis@gmail.com> wrote: > > > On Sat, Oct 5, 2024 at 2:26 PM FUJITA Tomonori > > <fujita.tomonori@gmail.com> wrote: > >> > >> + fn add(self, delta: Delta) -> Ktime { > >> + // SAFETY: FFI call. > >> + let t = unsafe { bindings::ktime_add_ns(self.inner, delta.as_nanos() as u64) }; > >> + Ktime::from_raw(t) > >> + } > > > > I wonder if we want to use the `ktime` macros/operations for this type > > or not (even if we still promise it is the same type underneath). It > > means having to define helpers, adding `unsafe` code and `SAFETY` > > comments, a call penalty in non-LTO, losing overflow checking (if we > > want it for these types), and so on. > > Yeah, if we are allowed to touch ktime_t directly instead of using the > accessors, it's great for the rust side. > > The timers maintainers, what do you think? We already do that in the existing code. The Ktime::sub method touches the ktime_t directly and performs a subtraction using the - operator rather than call a ktime_ method for it. Alice
On Mon, 7 Oct 2024 16:24:28 +0200 Alice Ryhl <aliceryhl@google.com> wrote: >> > or not (even if we still promise it is the same type underneath). It >> > means having to define helpers, adding `unsafe` code and `SAFETY` >> > comments, a call penalty in non-LTO, losing overflow checking (if we >> > want it for these types), and so on. >> >> Yeah, if we are allowed to touch ktime_t directly instead of using the >> accessors, it's great for the rust side. >> >> The timers maintainers, what do you think? > > We already do that in the existing code. The Ktime::sub method touches > the ktime_t directly and performs a subtraction using the - operator > rather than call a ktime_ method for it. I'll touch ktime_t directly in the next version.
diff --git a/rust/helpers/time.c b/rust/helpers/time.c index d6f61affb2c3..60dee69f4efc 100644 --- a/rust/helpers/time.c +++ b/rust/helpers/time.c @@ -2,6 +2,11 @@ #include <linux/ktime.h> +ktime_t rust_helper_ktime_add_ns(const ktime_t kt, const u64 nsec) +{ + return ktime_add_ns(kt, nsec); +} + int rust_helper_ktime_compare(const ktime_t cmp1, const ktime_t cmp2) { return ktime_compare(cmp1, cmp2); diff --git a/rust/kernel/time.rs b/rust/kernel/time.rs index 6c5a1c50c5f1..3e00ad22ed89 100644 --- a/rust/kernel/time.rs +++ b/rust/kernel/time.rs @@ -167,3 +167,14 @@ pub fn as_micros(self) -> i64 { self.nanos / NSEC_PER_USEC } } + +impl core::ops::Add<Delta> for Ktime { + type Output = Ktime; + + #[inline] + fn add(self, delta: Delta) -> Ktime { + // SAFETY: FFI call. + let t = unsafe { bindings::ktime_add_ns(self.inner, delta.as_nanos() as u64) }; + Ktime::from_raw(t) + } +}
Implement Add<Delta> for Ktime to support the operation: Ktime = Ktime + Delta This is used to calculate the future time when the timeout will occur. Signed-off-by: FUJITA Tomonori <fujita.tomonori@gmail.com> --- rust/helpers/time.c | 5 +++++ rust/kernel/time.rs | 11 +++++++++++ 2 files changed, 16 insertions(+)