| Message ID | 20241016035214.2229-4-fujita.tomonori@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | rust: Add IO polling | expand |
On Wed, Oct 16, 2024 at 5:53 AM FUJITA Tomonori <fujita.tomonori@gmail.com> wrote: > > Change the output type of Ktime's subtraction operation from Ktime to > Delta. Currently, the output is Ktime: > > Ktime = Ktime - Ktime > > It means that Ktime is used to represent timedelta. Delta is > introduced so use it. A typical example is calculating the elapsed > time: > > Delta = current Ktime - past Ktime; > > Signed-off-by: FUJITA Tomonori <fujita.tomonori@gmail.com> So this means that you are repurposing Ktime as a replacement for Instant rather than making both a Delta and Instant type? Okay. That seems reasonable enough. Reviewed-by: Alice Ryhl <aliceryhl@google.com> Alice
On Wed, Oct 16, 2024 at 10:25:11AM +0200, Alice Ryhl wrote: > On Wed, Oct 16, 2024 at 5:53 AM FUJITA Tomonori > <fujita.tomonori@gmail.com> wrote: > > > > Change the output type of Ktime's subtraction operation from Ktime to > > Delta. Currently, the output is Ktime: > > > > Ktime = Ktime - Ktime > > > > It means that Ktime is used to represent timedelta. Delta is > > introduced so use it. A typical example is calculating the elapsed > > time: > > > > Delta = current Ktime - past Ktime; > > > > Signed-off-by: FUJITA Tomonori <fujita.tomonori@gmail.com> > > So this means that you are repurposing Ktime as a replacement for > Instant rather than making both a Delta and Instant type? Okay. That > seems reasonable enough. > I think it's still reasonable to introduce a `Instant<ClockSource>` type (based on `Ktime`) to avoid some mis-uses, but we can do that in the future. Regards, Boqun > Reviewed-by: Alice Ryhl <aliceryhl@google.com> > > Alice >
On Wed, Oct 16, 2024 at 12:52:08PM +0900, FUJITA Tomonori wrote: > Change the output type of Ktime's subtraction operation from Ktime to > Delta. Currently, the output is Ktime: > > Ktime = Ktime - Ktime > > It means that Ktime is used to represent timedelta. Delta is > introduced so use it. A typical example is calculating the elapsed > time: > > Delta = current Ktime - past Ktime; > > Signed-off-by: FUJITA Tomonori <fujita.tomonori@gmail.com> > --- > rust/kernel/time.rs | 10 +++++----- > 1 file changed, 5 insertions(+), 5 deletions(-) > > diff --git a/rust/kernel/time.rs b/rust/kernel/time.rs > index 38a70dc98083..8c00854db58c 100644 > --- a/rust/kernel/time.rs > +++ b/rust/kernel/time.rs > @@ -74,16 +74,16 @@ pub fn to_ms(self) -> i64 { > /// Returns the number of milliseconds between two ktimes. > #[inline] > pub fn ktime_ms_delta(later: Ktime, earlier: Ktime) -> i64 { > - (later - earlier).to_ms() > + (later - earlier).as_millis() > } > > impl core::ops::Sub for Ktime { > - type Output = Ktime; > + type Output = Delta; > > #[inline] > - fn sub(self, other: Ktime) -> Ktime { > - Self { > - inner: self.inner - other.inner, > + fn sub(self, other: Ktime) -> Delta { > + Delta { > + nanos: self.inner - other.inner, My understanding is that ktime_t, when used as a timestamp, can only have a value in range [0, i64::MAX], so this substraction here never overflows, maybe we want add a comment here? We should really differ two cases: 1) user inputs may cause overflow vs 2) implementation errors or unexpected hardware errors cause overflow, I think. Regards, Boqun > } > } > } > -- > 2.43.0 > >
On Wed, 16 Oct 2024 10:25:11 +0200 Alice Ryhl <aliceryhl@google.com> wrote: > On Wed, Oct 16, 2024 at 5:53 AM FUJITA Tomonori > <fujita.tomonori@gmail.com> wrote: >> >> Change the output type of Ktime's subtraction operation from Ktime to >> Delta. Currently, the output is Ktime: >> >> Ktime = Ktime - Ktime >> >> It means that Ktime is used to represent timedelta. Delta is >> introduced so use it. A typical example is calculating the elapsed >> time: >> >> Delta = current Ktime - past Ktime; >> >> Signed-off-by: FUJITA Tomonori <fujita.tomonori@gmail.com> > > So this means that you are repurposing Ktime as a replacement for > Instant rather than making both a Delta and Instant type? Okay. That > seems reasonable enough. Yes. Surely, we could create both Delta and Instant. What is Ktime used for? Both can simply use bindings::ktime_t like the followings? pub struct Instant { inner: bindings::ktime_t, } pub struct Delta { inner: bindings::ktime_t, }
On Thu, Oct 17, 2024 at 9:11 AM FUJITA Tomonori <fujita.tomonori@gmail.com> wrote: > > On Wed, 16 Oct 2024 10:25:11 +0200 > Alice Ryhl <aliceryhl@google.com> wrote: > > > On Wed, Oct 16, 2024 at 5:53 AM FUJITA Tomonori > > <fujita.tomonori@gmail.com> wrote: > >> > >> Change the output type of Ktime's subtraction operation from Ktime to > >> Delta. Currently, the output is Ktime: > >> > >> Ktime = Ktime - Ktime > >> > >> It means that Ktime is used to represent timedelta. Delta is > >> introduced so use it. A typical example is calculating the elapsed > >> time: > >> > >> Delta = current Ktime - past Ktime; > >> > >> Signed-off-by: FUJITA Tomonori <fujita.tomonori@gmail.com> > > > > So this means that you are repurposing Ktime as a replacement for > > Instant rather than making both a Delta and Instant type? Okay. That > > seems reasonable enough. > > Yes. > > Surely, we could create both Delta and Instant. What is Ktime used > for? Both can simply use bindings::ktime_t like the followings? > > pub struct Instant { > inner: bindings::ktime_t, > } > > pub struct Delta { > inner: bindings::ktime_t, > } What you're doing is okay. No complaints. Alice
On Wed, 16 Oct 2024 13:03:48 -0700 Boqun Feng <boqun.feng@gmail.com> wrote: >> impl core::ops::Sub for Ktime { >> - type Output = Ktime; >> + type Output = Delta; >> >> #[inline] >> - fn sub(self, other: Ktime) -> Ktime { >> - Self { >> - inner: self.inner - other.inner, >> + fn sub(self, other: Ktime) -> Delta { >> + Delta { >> + nanos: self.inner - other.inner, > > My understanding is that ktime_t, when used as a timestamp, can only > have a value in range [0, i64::MAX], so this substraction here never > overflows, maybe we want add a comment here? Sure, I'll add.
On Thu, Oct 17, 2024 at 9:11 AM FUJITA Tomonori <fujita.tomonori@gmail.com> wrote: > > Surely, we could create both Delta and Instant. What is Ktime used > for? Both can simply use bindings::ktime_t like the followings? I think it may help having 2 (public) types, rather than reusing the `Ktime` name for one of them, because people may associate several concepts to `ktime_t` which is what they know already, but I would suggest mentioning in the docs clearly that these maps to usecase subsets of `ktime_t` (whether we mention or not that they are supposed to be `ktime_t`s is another thing, even if they are). Whether we have a third private type internally for `Ktime` or not does not matter much, so whatever is best for implementation purposes. And if we do have a private `Ktime`, I would avoid making it public unless there is a good reason for doing so. Cheers, Miguel
On Thu, 17 Oct 2024 18:45:13 +0200 Miguel Ojeda <miguel.ojeda.sandonis@gmail.com> wrote: >> Surely, we could create both Delta and Instant. What is Ktime used >> for? Both can simply use bindings::ktime_t like the followings? > > I think it may help having 2 (public) types, rather than reusing the > `Ktime` name for one of them, because people may associate several > concepts to `ktime_t` which is what they know already, but I would > suggest mentioning in the docs clearly that these maps to usecase > subsets of `ktime_t` (whether we mention or not that they are > supposed to be `ktime_t`s is another thing, even if they are). Sounds good. I'll create both `Delta` and `Instant`. > Whether we have a third private type internally for `Ktime` or not > does not matter much, so whatever is best for implementation purposes. > And if we do have a private `Ktime`, I would avoid making it public > unless there is a good reason for doing so. I don't think implementing `Delta` and `Instant` types on the top of a private Ktime makes sense. I'll just rename the current `Ktime` type to `Instant` type.
diff --git a/rust/kernel/time.rs b/rust/kernel/time.rs index 38a70dc98083..8c00854db58c 100644 --- a/rust/kernel/time.rs +++ b/rust/kernel/time.rs @@ -74,16 +74,16 @@ pub fn to_ms(self) -> i64 { /// Returns the number of milliseconds between two ktimes. #[inline] pub fn ktime_ms_delta(later: Ktime, earlier: Ktime) -> i64 { - (later - earlier).to_ms() + (later - earlier).as_millis() } impl core::ops::Sub for Ktime { - type Output = Ktime; + type Output = Delta; #[inline] - fn sub(self, other: Ktime) -> Ktime { - Self { - inner: self.inner - other.inner, + fn sub(self, other: Ktime) -> Delta { + Delta { + nanos: self.inner - other.inner, } } }
Change the output type of Ktime's subtraction operation from Ktime to Delta. Currently, the output is Ktime: Ktime = Ktime - Ktime It means that Ktime is used to represent timedelta. Delta is introduced so use it. A typical example is calculating the elapsed time: Delta = current Ktime - past Ktime; Signed-off-by: FUJITA Tomonori <fujita.tomonori@gmail.com> --- rust/kernel/time.rs | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-)