| Message ID | 20241005122531.20298-2-fujita.tomonori@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | rust: Add IO polling | expand |
On 5 Oct 2024, at 14:25, FUJITA Tomonori wrote: > Implement PartialEq and PartialOrd trait for Ktime by using C's > ktime_compare function so two Ktime instances can be compared to > determine whether a timeout is met or not. Why is this only PartialEq/PartialOrd? Could we either document why or implement Eq/Ord as well? > > Signed-off-by: FUJITA Tomonori <fujita.tomonori@gmail.com> > --- > rust/helpers/helpers.c | 1 + > rust/helpers/time.c | 8 ++++++++ > rust/kernel/time.rs | 22 ++++++++++++++++++++++ > 3 files changed, 31 insertions(+) > create mode 100644 rust/helpers/time.c > > diff --git a/rust/helpers/helpers.c b/rust/helpers/helpers.c > index 30f40149f3a9..c274546bcf78 100644 > --- a/rust/helpers/helpers.c > +++ b/rust/helpers/helpers.c > @@ -21,6 +21,7 @@ > #include "slab.c" > #include "spinlock.c" > #include "task.c" > +#include "time.c" > #include "uaccess.c" > #include "wait.c" > #include "workqueue.c" > diff --git a/rust/helpers/time.c b/rust/helpers/time.c > new file mode 100644 > index 000000000000..d6f61affb2c3 > --- /dev/null > +++ b/rust/helpers/time.c > @@ -0,0 +1,8 @@ > +// SPDX-License-Identifier: GPL-2.0 > + > +#include <linux/ktime.h> > + > +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 e3bb5e89f88d..c40105941a2c 100644 > --- a/rust/kernel/time.rs > +++ b/rust/kernel/time.rs > @@ -81,3 +81,25 @@ fn sub(self, other: Ktime) -> Ktime { > } > } > } > + > +impl PartialEq for Ktime { > + #[inline] > + fn eq(&self, other: &Self) -> bool { > + // SAFETY: FFI call. > + let ret = unsafe { bindings::ktime_compare(self.inner, other.inner) }; > + ret == 0 > + } > +} > + > +impl PartialOrd for Ktime { > + #[inline] > + fn partial_cmp(&self, other: &Self) -> Option<core::cmp::Ordering> { > + // SAFETY: FFI call. > + let ret = unsafe { bindings::ktime_compare(self.inner, other.inner) }; > + match ret { > + 0 => Some(core::cmp::Ordering::Equal), > + x if x < 0 => Some(core::cmp::Ordering::Less), > + _ => Some(core::cmp::Ordering::Greater), > + } > + } > +} > -- > 2.34.1
On Sun, 06 Oct 2024 12:28:59 +0200 Fiona Behrens <finn@kloenk.dev> wrote: >> Implement PartialEq and PartialOrd trait for Ktime by using C's >> ktime_compare function so two Ktime instances can be compared to >> determine whether a timeout is met or not. > > Why is this only PartialEq/PartialOrd? Could we either document why or implement Eq/Ord as well? Because what we need to do is comparing two Ktime instances so we don't need them?
On 7 Oct 2024, at 7:37, FUJITA Tomonori wrote: > On Sun, 06 Oct 2024 12:28:59 +0200 > Fiona Behrens <finn@kloenk.dev> wrote: > >>> Implement PartialEq and PartialOrd trait for Ktime by using C's >>> ktime_compare function so two Ktime instances can be compared to >>> determine whether a timeout is met or not. >> >> Why is this only PartialEq/PartialOrd? Could we either document why or implement Eq/Ord as well? > > Because what we need to do is comparing two Ktime instances so we > don't need them? Eq is basically just a marker trait, so you could argue we would never need it. I think because those 2 traits mostly just document logic it would make sense to also implement them to not create rethinking if then there is some logic that might want it and then the question is why was it omitted. - Fiona
On Mon, Oct 7, 2024 at 7:37 AM FUJITA Tomonori <fujita.tomonori@gmail.com> wrote: > > On Sun, 06 Oct 2024 12:28:59 +0200 > Fiona Behrens <finn@kloenk.dev> wrote: > > >> Implement PartialEq and PartialOrd trait for Ktime by using C's > >> ktime_compare function so two Ktime instances can be compared to > >> determine whether a timeout is met or not. > > > > Why is this only PartialEq/PartialOrd? Could we either document why or implement Eq/Ord as well? > > Because what we need to do is comparing two Ktime instances so we > don't need them? When you implement PartialEq without Eq, you are telling the reader that this is a weird type such as floats where there exists values that are not equal to themselves. That's not the case here, so don't confuse the reader by leaving out `Eq`. Alice
On Mon, 7 Oct 2024 10:41:23 +0200 Alice Ryhl <aliceryhl@google.com> wrote: >> >> Implement PartialEq and PartialOrd trait for Ktime by using C's >> >> ktime_compare function so two Ktime instances can be compared to >> >> determine whether a timeout is met or not. >> > >> > Why is this only PartialEq/PartialOrd? Could we either document why or implement Eq/Ord as well? >> >> Because what we need to do is comparing two Ktime instances so we >> don't need them? > > When you implement PartialEq without Eq, you are telling the reader > that this is a weird type such as floats where there exists values > that are not equal to themselves. That's not the case here, so don't > confuse the reader by leaving out `Eq`. Understood. I'll add Eq/Ord in the next version.
On Mon, Oct 07, 2024 at 10:41:23AM +0200, Alice Ryhl wrote: > On Mon, Oct 7, 2024 at 7:37 AM FUJITA Tomonori > <fujita.tomonori@gmail.com> wrote: > > > > On Sun, 06 Oct 2024 12:28:59 +0200 > > Fiona Behrens <finn@kloenk.dev> wrote: > > > > >> Implement PartialEq and PartialOrd trait for Ktime by using C's > > >> ktime_compare function so two Ktime instances can be compared to > > >> determine whether a timeout is met or not. > > > > > > Why is this only PartialEq/PartialOrd? Could we either document why or implement Eq/Ord as well? > > > > Because what we need to do is comparing two Ktime instances so we > > don't need them? > > When you implement PartialEq without Eq, you are telling the reader > that this is a weird type such as floats where there exists values > that are not equal to themselves. That's not the case here, so don't > confuse the reader by leaving out `Eq`. This might be one of those areas where there needs to be a difference between C and Rust in terms of kernel rules. For C, there would need to be a user. Here you seem to be saying the type system needs it, for the type to be meaningful, even if there is no user? Without Eq, would the compiler complain on an == operation, saying it is not a valid operation? Is there a clear difference between nobody has implemented this yet, vs such an operation is impossible, such as your float example? Andrew
On Mon, Oct 7, 2024 at 3:16 PM Andrew Lunn <andrew@lunn.ch> wrote: > > On Mon, Oct 07, 2024 at 10:41:23AM +0200, Alice Ryhl wrote: > > On Mon, Oct 7, 2024 at 7:37 AM FUJITA Tomonori > > <fujita.tomonori@gmail.com> wrote: > > > > > > On Sun, 06 Oct 2024 12:28:59 +0200 > > > Fiona Behrens <finn@kloenk.dev> wrote: > > > > > > >> Implement PartialEq and PartialOrd trait for Ktime by using C's > > > >> ktime_compare function so two Ktime instances can be compared to > > > >> determine whether a timeout is met or not. > > > > > > > > Why is this only PartialEq/PartialOrd? Could we either document why or implement Eq/Ord as well? > > > > > > Because what we need to do is comparing two Ktime instances so we > > > don't need them? > > > > When you implement PartialEq without Eq, you are telling the reader > > that this is a weird type such as floats where there exists values > > that are not equal to themselves. That's not the case here, so don't > > confuse the reader by leaving out `Eq`. > > This might be one of those areas where there needs to be a difference > between C and Rust in terms of kernel rules. For C, there would need > to be a user. Here you seem to be saying the type system needs it, for > the type to be meaningful, even if there is no user? > > Without Eq, would the compiler complain on an == operation, saying it > is not a valid operation? Is there a clear difference between nobody > has implemented this yet, vs such an operation is impossible, such as > your float example? Think of it this way: I wrote an implementation of something that works in situations A and B, but I only use it in situation A. Must I write my program in a way to make it impossible to use it in situation B? That's how I see this case. Implementing Eq does not involve adding any new functions. Alice
diff --git a/rust/helpers/helpers.c b/rust/helpers/helpers.c index 30f40149f3a9..c274546bcf78 100644 --- a/rust/helpers/helpers.c +++ b/rust/helpers/helpers.c @@ -21,6 +21,7 @@ #include "slab.c" #include "spinlock.c" #include "task.c" +#include "time.c" #include "uaccess.c" #include "wait.c" #include "workqueue.c" diff --git a/rust/helpers/time.c b/rust/helpers/time.c new file mode 100644 index 000000000000..d6f61affb2c3 --- /dev/null +++ b/rust/helpers/time.c @@ -0,0 +1,8 @@ +// SPDX-License-Identifier: GPL-2.0 + +#include <linux/ktime.h> + +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 e3bb5e89f88d..c40105941a2c 100644 --- a/rust/kernel/time.rs +++ b/rust/kernel/time.rs @@ -81,3 +81,25 @@ fn sub(self, other: Ktime) -> Ktime { } } } + +impl PartialEq for Ktime { + #[inline] + fn eq(&self, other: &Self) -> bool { + // SAFETY: FFI call. + let ret = unsafe { bindings::ktime_compare(self.inner, other.inner) }; + ret == 0 + } +} + +impl PartialOrd for Ktime { + #[inline] + fn partial_cmp(&self, other: &Self) -> Option<core::cmp::Ordering> { + // SAFETY: FFI call. + let ret = unsafe { bindings::ktime_compare(self.inner, other.inner) }; + match ret { + 0 => Some(core::cmp::Ordering::Equal), + x if x < 0 => Some(core::cmp::Ordering::Less), + _ => Some(core::cmp::Ordering::Greater), + } + } +}
Implement PartialEq and PartialOrd trait for Ktime by using C's ktime_compare function so two Ktime instances can be compared to determine whether a timeout is met or not. Signed-off-by: FUJITA Tomonori <fujita.tomonori@gmail.com> --- rust/helpers/helpers.c | 1 + rust/helpers/time.c | 8 ++++++++ rust/kernel/time.rs | 22 ++++++++++++++++++++++ 3 files changed, 31 insertions(+) create mode 100644 rust/helpers/time.c