1. Patchew
  2. linux
  3. [PATCH net-next v3 0/8] rust: Add IO polling
  4. View patch

[PATCH net-next v3 3/8] rust: time: Change output of Ktime's sub operation to Delta

FUJITA Tomonori posted 8 patches 1 month, 1 week ago
There is a newer version of this series
[PATCH net-next v3 3/8] rust: time: Change output of Ktime's sub operation to Delta
Posted by FUJITA Tomonori 1 month, 1 week ago 
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,
         }
     }
 }
-- 
2.43.0
Re: [PATCH net-next v3 3/8] rust: time: Change output of Ktime's sub operation to Delta
Posted by Boqun Feng 1 month, 1 week ago 
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
> 
>
Re: [PATCH net-next v3 3/8] rust: time: Change output of Ktime's sub operation to Delta
Posted by FUJITA Tomonori 1 month, 1 week ago 
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.
Re: [PATCH net-next v3 3/8] rust: time: Change output of Ktime's sub operation to Delta
Posted by Alice Ryhl 1 month, 1 week ago 
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
Re: [PATCH net-next v3 3/8] rust: time: Change output of Ktime's sub operation to Delta
Posted by FUJITA Tomonori 1 month, 1 week ago 
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,
}
Re: [PATCH net-next v3 3/8] rust: time: Change output of Ktime's sub operation to Delta
Posted by Miguel Ojeda 1 month, 1 week ago 
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
Re: [PATCH net-next v3 3/8] rust: time: Change output of Ktime's sub operation to Delta
Posted by FUJITA Tomonori 1 month ago 
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.
Re: [PATCH net-next v3 3/8] rust: time: Change output of Ktime's sub operation to Delta
Posted by Alice Ryhl 1 month, 1 week ago 
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
Re: [PATCH net-next v3 3/8] rust: time: Change output of Ktime's sub operation to Delta
Posted by Boqun Feng 1 month, 1 week ago 
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
>

Patchew 2.1-devel-b67e4c2

© 2016 - 2024 Red Hat, Inc.