This adds a common Vec method called `retain` that removes all elements
that don't match a certain condition. Rust Binder uses it to find all
processes that match a given pid.
The stdlib retain method takes &T rather than &mut T and has a separate
retain_mut for the &mut T case. However, this is considered an API
mistake that can't be fixed now due to backwards compatibility. There's
no reason for us to repeat that mistake.
Signed-off-by: Alice Ryhl <aliceryhl@google.com>
---
rust/kernel/alloc/kvec.rs | 72 +++++++++++++++++++++++++++++++++++++++++++++++
1 file changed, 72 insertions(+)
diff --git a/rust/kernel/alloc/kvec.rs b/rust/kernel/alloc/kvec.rs
index 72bc743ec88bf7b91a0a1ffd9f830cfe4f983ffd..357f5a37c7b1d15b709a10c162292841eed0e376 100644
--- a/rust/kernel/alloc/kvec.rs
+++ b/rust/kernel/alloc/kvec.rs
@@ -608,6 +608,29 @@ pub fn drain_all(&mut self) -> DrainAll<'_, T> {
elements: elems.iter_mut(),
}
}
+
+ /// Removes all elements that don't match the provided closure.
+ ///
+ /// # Examples
+ ///
+ /// ```
+ /// let mut v = kernel::kvec![1, 2, 3, 4]?;
+ /// v.retain(|i| *i % 2 == 0);
+ /// assert_eq!(v, [2, 4]);
+ /// # Ok::<(), Error>(())
+ /// ```
+ pub fn retain(&mut self, mut f: impl FnMut(&mut T) -> bool) {
+ let mut num_kept = 0;
+ let mut next_to_check = 0;
+ while let Some(to_check) = self.get_mut(next_to_check) {
+ if f(to_check) {
+ self.swap(num_kept, next_to_check);
+ num_kept += 1;
+ }
+ next_to_check += 1;
+ }
+ self.truncate(num_kept);
+ }
}
impl<T: Clone, A: Allocator> Vec<T, A> {
@@ -1130,3 +1153,52 @@ fn drop(&mut self) {
}
}
}
+
+#[macros::kunit_tests(rust_kvec_kunit)]
+mod tests {
+ use super::*;
+ use crate::prelude::*;
+
+ #[test]
+ fn test_kvec_retain() {
+ /// Verify correctness for one specific function.
+ #[expect(clippy::needless_range_loop)]
+ fn verify(c: &[bool]) {
+ let mut vec1: KVec<usize> = KVec::with_capacity(c.len(), GFP_KERNEL).unwrap();
+ let mut vec2: KVec<usize> = KVec::with_capacity(c.len(), GFP_KERNEL).unwrap();
+
+ for i in 0..c.len() {
+ vec1.push_within_capacity(i).unwrap();
+ if c[i] {
+ vec2.push_within_capacity(i).unwrap();
+ }
+ }
+
+ vec1.retain(|i| c[*i]);
+
+ assert_eq!(vec1, vec2);
+ }
+
+ /// Add one to a binary integer represented as a boolean array.
+ fn add(value: &mut [bool]) {
+ let mut carry = true;
+ for v in value {
+ let new_v = carry != *v;
+ carry = carry && *v;
+ *v = new_v;
+ }
+ }
+
+ // This boolean array represents a function from index to boolean. We check that `retain`
+ // behaves correctly for all possible boolean arrays of every possible length less than
+ // ten.
+ let mut func = KVec::with_capacity(10, GFP_KERNEL).unwrap();
+ for len in 0..10 {
+ for _ in 0u32..1u32 << len {
+ verify(&func);
+ add(&mut func);
+ }
+ func.push_within_capacity(false).unwrap();
+ }
+ }
+}
--
2.49.0.901.g37484f566f-googOn Tue, Apr 29, 2025 at 02:44:25PM +0000, Alice Ryhl wrote:
> This adds a common Vec method called `retain` that removes all elements
> that don't match a certain condition. Rust Binder uses it to find all
> processes that match a given pid.
>
> The stdlib retain method takes &T rather than &mut T and has a separate
> retain_mut for the &mut T case. However, this is considered an API
> mistake that can't be fixed now due to backwards compatibility. There's
> no reason for us to repeat that mistake.
>
> Signed-off-by: Alice Ryhl <aliceryhl@google.com>
> ---
> rust/kernel/alloc/kvec.rs | 72 +++++++++++++++++++++++++++++++++++++++++++++++
> 1 file changed, 72 insertions(+)
>
> diff --git a/rust/kernel/alloc/kvec.rs b/rust/kernel/alloc/kvec.rs
> index 72bc743ec88bf7b91a0a1ffd9f830cfe4f983ffd..357f5a37c7b1d15b709a10c162292841eed0e376 100644
> --- a/rust/kernel/alloc/kvec.rs
> +++ b/rust/kernel/alloc/kvec.rs
> @@ -608,6 +608,29 @@ pub fn drain_all(&mut self) -> DrainAll<'_, T> {
> elements: elems.iter_mut(),
> }
> }
> +
> + /// Removes all elements that don't match the provided closure.
> + ///
> + /// # Examples
> + ///
> + /// ```
> + /// let mut v = kernel::kvec![1, 2, 3, 4]?;
> + /// v.retain(|i| *i % 2 == 0);
NIT: What about making this `v.retain(|&mut i| i % 2 == 0)`?
> + /// assert_eq!(v, [2, 4]);
> + /// # Ok::<(), Error>(())
> + /// ```
> + pub fn retain(&mut self, mut f: impl FnMut(&mut T) -> bool) {
> + let mut num_kept = 0;
> + let mut next_to_check = 0;
> + while let Some(to_check) = self.get_mut(next_to_check) {
> + if f(to_check) {
> + self.swap(num_kept, next_to_check);
> + num_kept += 1;
> + }
> + next_to_check += 1;
> + }
> + self.truncate(num_kept);
> + }
> }
>
> impl<T: Clone, A: Allocator> Vec<T, A> {
> @@ -1130,3 +1153,52 @@ fn drop(&mut self) {
> }
> }
> }
> +
> +#[macros::kunit_tests(rust_kvec_kunit)]
> +mod tests {
> + use super::*;
> + use crate::prelude::*;
> +
> + #[test]
> + fn test_kvec_retain() {
Can we have this return a Result, like doctests can do?
> + /// Verify correctness for one specific function.
> + #[expect(clippy::needless_range_loop)]
> + fn verify(c: &[bool]) {
> + let mut vec1: KVec<usize> = KVec::with_capacity(c.len(), GFP_KERNEL).unwrap();
> + let mut vec2: KVec<usize> = KVec::with_capacity(c.len(), GFP_KERNEL).unwrap();
> +
> + for i in 0..c.len() {
> + vec1.push_within_capacity(i).unwrap();
> + if c[i] {
> + vec2.push_within_capacity(i).unwrap();
> + }
> + }
> +
> + vec1.retain(|i| c[*i]);
> +
> + assert_eq!(vec1, vec2);
Don't we have macros around kunit_assert!() and kunit_assert_eq() outside of
doctests (i.e. dedicated kunit tests)?
I much prefer their output over the kernel panic we get with the "normal"
asserts, unwraps, etc.
Consistently sticking to the same output on failure makes it easier to catch and
easier to setup CI environments.
> + }
> +
> + /// Add one to a binary integer represented as a boolean array.
> + fn add(value: &mut [bool]) {
> + let mut carry = true;
> + for v in value {
> + let new_v = carry != *v;
> + carry = carry && *v;
> + *v = new_v;
> + }
> + }
> +
> + // This boolean array represents a function from index to boolean. We check that `retain`
> + // behaves correctly for all possible boolean arrays of every possible length less than
> + // ten.
> + let mut func = KVec::with_capacity(10, GFP_KERNEL).unwrap();
> + for len in 0..10 {
> + for _ in 0u32..1u32 << len {
> + verify(&func);
> + add(&mut func);
> + }
> + func.push_within_capacity(false).unwrap();
> + }
> + }
> +}
>
> --
> 2.49.0.901.g37484f566f-goog
>
On Wed, 30 Apr 2025 18:26:05 +0200 Danilo Krummrich <dakr@kernel.org> wrote:
>
> Can we have this return a Result, like doctests can do?
>
> Don't we have macros around kunit_assert!() and kunit_assert_eq() outside of
> doctests (i.e. dedicated kunit tests)?
The initial KUnit `#[test]` support that landed was very basic:
https://lore.kernel.org/rust-for-linux/20250330170535.546869-1-ojeda@kernel.org/
The missing `assert*!`s support is definitely annoying. I took a quick look,
and it is not too bad to add the support. The `Result` is then easy on top too.
So I will send the support for both already since I suspect others will hit this
quite soon.
I hope that helps.
Cheers,
Miguel
On Wed, Apr 30, 2025 at 06:26:05PM +0200, Danilo Krummrich wrote:
> On Tue, Apr 29, 2025 at 02:44:25PM +0000, Alice Ryhl wrote:
> > This adds a common Vec method called `retain` that removes all elements
> > that don't match a certain condition. Rust Binder uses it to find all
> > processes that match a given pid.
> >
> > The stdlib retain method takes &T rather than &mut T and has a separate
> > retain_mut for the &mut T case. However, this is considered an API
> > mistake that can't be fixed now due to backwards compatibility. There's
> > no reason for us to repeat that mistake.
> >
> > Signed-off-by: Alice Ryhl <aliceryhl@google.com>
> > ---
> > rust/kernel/alloc/kvec.rs | 72 +++++++++++++++++++++++++++++++++++++++++++++++
> > 1 file changed, 72 insertions(+)
> >
> > diff --git a/rust/kernel/alloc/kvec.rs b/rust/kernel/alloc/kvec.rs
> > index 72bc743ec88bf7b91a0a1ffd9f830cfe4f983ffd..357f5a37c7b1d15b709a10c162292841eed0e376 100644
> > --- a/rust/kernel/alloc/kvec.rs
> > +++ b/rust/kernel/alloc/kvec.rs
> > @@ -608,6 +608,29 @@ pub fn drain_all(&mut self) -> DrainAll<'_, T> {
> > elements: elems.iter_mut(),
> > }
> > }
> > +
> > + /// Removes all elements that don't match the provided closure.
> > + ///
> > + /// # Examples
> > + ///
> > + /// ```
> > + /// let mut v = kernel::kvec![1, 2, 3, 4]?;
> > + /// v.retain(|i| *i % 2 == 0);
>
> NIT: What about making this `v.retain(|&mut i| i % 2 == 0)`?
>
> > + /// assert_eq!(v, [2, 4]);
> > + /// # Ok::<(), Error>(())
> > + /// ```
> > + pub fn retain(&mut self, mut f: impl FnMut(&mut T) -> bool) {
> > + let mut num_kept = 0;
> > + let mut next_to_check = 0;
> > + while let Some(to_check) = self.get_mut(next_to_check) {
> > + if f(to_check) {
> > + self.swap(num_kept, next_to_check);
> > + num_kept += 1;
> > + }
> > + next_to_check += 1;
> > + }
> > + self.truncate(num_kept);
> > + }
> > }
> >
> > impl<T: Clone, A: Allocator> Vec<T, A> {
> > @@ -1130,3 +1153,52 @@ fn drop(&mut self) {
> > }
> > }
> > }
> > +
> > +#[macros::kunit_tests(rust_kvec_kunit)]
> > +mod tests {
> > + use super::*;
> > + use crate::prelude::*;
> > +
> > + #[test]
> > + fn test_kvec_retain() {
>
> Can we have this return a Result, like doctests can do?
I get warning when I try that:
warning: unused `core::result::Result` that must be used
--> rust/kernel/alloc/kvec.rs:1232:1
|
1232 | #[macros::kunit_tests(rust_kvec_kunit)]
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
|
= note: this `Result` may be an `Err` variant, which should be handled
= note: `#[warn(unused_must_use)]` on by default
= note: this warning originates in the attribute macro `macros::kunit_tests`
(in Nightly builds, run with -Z macro-backtrace for more info)
> > + /// Verify correctness for one specific function.
> > + #[expect(clippy::needless_range_loop)]
> > + fn verify(c: &[bool]) {
> > + let mut vec1: KVec<usize> = KVec::with_capacity(c.len(), GFP_KERNEL).unwrap();
> > + let mut vec2: KVec<usize> = KVec::with_capacity(c.len(), GFP_KERNEL).unwrap();
> > +
> > + for i in 0..c.len() {
> > + vec1.push_within_capacity(i).unwrap();
> > + if c[i] {
> > + vec2.push_within_capacity(i).unwrap();
> > + }
> > + }
> > +
> > + vec1.retain(|i| c[*i]);
> > +
> > + assert_eq!(vec1, vec2);
>
> Don't we have macros around kunit_assert!() and kunit_assert_eq() outside of
> doctests (i.e. dedicated kunit tests)?
>
> I much prefer their output over the kernel panic we get with the "normal"
> asserts, unwraps, etc.
>
> Consistently sticking to the same output on failure makes it easier to catch and
> easier to setup CI environments.
The documentation for those macros says "Public but hidden since it
should only be used from generated tests." so I don't think I'm supposed
to use them.
Alice
On Thu, May 01, 2025 at 11:10:12AM +0000, Alice Ryhl wrote:
> On Wed, Apr 30, 2025 at 06:26:05PM +0200, Danilo Krummrich wrote:
> > On Tue, Apr 29, 2025 at 02:44:25PM +0000, Alice Ryhl wrote:
> > > This adds a common Vec method called `retain` that removes all elements
> > > that don't match a certain condition. Rust Binder uses it to find all
> > > processes that match a given pid.
> > >
> > > The stdlib retain method takes &T rather than &mut T and has a separate
> > > retain_mut for the &mut T case. However, this is considered an API
> > > mistake that can't be fixed now due to backwards compatibility. There's
> > > no reason for us to repeat that mistake.
> > >
> > > Signed-off-by: Alice Ryhl <aliceryhl@google.com>
> > > ---
> > > rust/kernel/alloc/kvec.rs | 72 +++++++++++++++++++++++++++++++++++++++++++++++
> > > 1 file changed, 72 insertions(+)
> > >
> > > diff --git a/rust/kernel/alloc/kvec.rs b/rust/kernel/alloc/kvec.rs
> > > index 72bc743ec88bf7b91a0a1ffd9f830cfe4f983ffd..357f5a37c7b1d15b709a10c162292841eed0e376 100644
> > > --- a/rust/kernel/alloc/kvec.rs
> > > +++ b/rust/kernel/alloc/kvec.rs
> > > @@ -608,6 +608,29 @@ pub fn drain_all(&mut self) -> DrainAll<'_, T> {
> > > elements: elems.iter_mut(),
> > > }
> > > }
> > > +
> > > + /// Removes all elements that don't match the provided closure.
> > > + ///
> > > + /// # Examples
> > > + ///
> > > + /// ```
> > > + /// let mut v = kernel::kvec![1, 2, 3, 4]?;
> > > + /// v.retain(|i| *i % 2 == 0);
> >
> > NIT: What about making this `v.retain(|&mut i| i % 2 == 0)`?
> >
> > > + /// assert_eq!(v, [2, 4]);
> > > + /// # Ok::<(), Error>(())
> > > + /// ```
> > > + pub fn retain(&mut self, mut f: impl FnMut(&mut T) -> bool) {
> > > + let mut num_kept = 0;
> > > + let mut next_to_check = 0;
> > > + while let Some(to_check) = self.get_mut(next_to_check) {
> > > + if f(to_check) {
> > > + self.swap(num_kept, next_to_check);
> > > + num_kept += 1;
> > > + }
> > > + next_to_check += 1;
> > > + }
> > > + self.truncate(num_kept);
> > > + }
> > > }
> > >
> > > impl<T: Clone, A: Allocator> Vec<T, A> {
> > > @@ -1130,3 +1153,52 @@ fn drop(&mut self) {
> > > }
> > > }
> > > }
> > > +
> > > +#[macros::kunit_tests(rust_kvec_kunit)]
> > > +mod tests {
> > > + use super::*;
> > > + use crate::prelude::*;
> > > +
> > > + #[test]
> > > + fn test_kvec_retain() {
> >
> > Can we have this return a Result, like doctests can do?
>
> I get warning when I try that:
>
> warning: unused `core::result::Result` that must be used
> --> rust/kernel/alloc/kvec.rs:1232:1
> |
> 1232 | #[macros::kunit_tests(rust_kvec_kunit)]
> | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> |
> = note: this `Result` may be an `Err` variant, which should be handled
> = note: `#[warn(unused_must_use)]` on by default
> = note: this warning originates in the attribute macro `macros::kunit_tests`
> (in Nightly builds, run with -Z macro-backtrace for more info)
Yes, I'm aware, I tried playing with that myself. I really meant the question as
I wrote, not as "Can you please change that?". :-) Sorry for the confusion.
> > > + /// Verify correctness for one specific function.
> > > + #[expect(clippy::needless_range_loop)]
> > > + fn verify(c: &[bool]) {
> > > + let mut vec1: KVec<usize> = KVec::with_capacity(c.len(), GFP_KERNEL).unwrap();
> > > + let mut vec2: KVec<usize> = KVec::with_capacity(c.len(), GFP_KERNEL).unwrap();
> > > +
> > > + for i in 0..c.len() {
> > > + vec1.push_within_capacity(i).unwrap();
> > > + if c[i] {
> > > + vec2.push_within_capacity(i).unwrap();
> > > + }
> > > + }
> > > +
> > > + vec1.retain(|i| c[*i]);
> > > +
> > > + assert_eq!(vec1, vec2);
> >
> > Don't we have macros around kunit_assert!() and kunit_assert_eq() outside of
> > doctests (i.e. dedicated kunit tests)?
> >
> > I much prefer their output over the kernel panic we get with the "normal"
> > asserts, unwraps, etc.
> >
> > Consistently sticking to the same output on failure makes it easier to catch and
> > easier to setup CI environments.
>
> The documentation for those macros says "Public but hidden since it
> should only be used from generated tests." so I don't think I'm supposed
> to use them.
Same here, that's more a fundamental question, rather than something for you to
change right away.
I really like the way doctests implement the assert macros and how they appear
in the kernel log compared to panics through the "real" assert ones, unwraps,
etc.
I also think that avoiding things that directly panic in doctests (i.e. example
code) is the correct thing to do. For KUnit tests it's probably less important,
since they don't directly serve as sample code.
So, I wonder what's our take on that. Do we want to have KUnit and doctests
aligned? I think that'd be a good thing.
On Thu, May 1, 2025 at 1:30 PM Danilo Krummrich <dakr@kernel.org> wrote:
>
> I really like the way doctests implement the assert macros and how they appear
> in the kernel log compared to panics through the "real" assert ones, unwraps,
> etc.
>
> I also think that avoiding things that directly panic in doctests (i.e. example
> code) is the correct thing to do. For KUnit tests it's probably less important,
> since they don't directly serve as sample code.
I hadn't seen these earlier messages -- thanks, I am glad people like
the doctests support :)
Yeah, doing the same for `#[test]`s was planned (see PR), but we
decided to merge roughly what we had from the original author and then
build on top of that (Cc David).
Anyway, I sent it -- hopefully it will mean more people writing lots of tests!
https://lore.kernel.org/rust-for-linux/20250502215133.1923676-1-ojeda@kernel.org/
Cheers,
Miguel
On Thu, May 01, 2025 at 01:30:20PM +0200, Danilo Krummrich wrote:
> On Thu, May 01, 2025 at 11:10:12AM +0000, Alice Ryhl wrote:
> > On Wed, Apr 30, 2025 at 06:26:05PM +0200, Danilo Krummrich wrote:
> > > On Tue, Apr 29, 2025 at 02:44:25PM +0000, Alice Ryhl wrote:
> > > > +#[macros::kunit_tests(rust_kvec_kunit)]
> > > > +mod tests {
> > > > + use super::*;
> > > > + use crate::prelude::*;
> > > > +
> > > > + #[test]
> > > > + fn test_kvec_retain() {
> > >
> > > Can we have this return a Result, like doctests can do?
> >
> > I get warning when I try that:
> >
> > warning: unused `core::result::Result` that must be used
> > --> rust/kernel/alloc/kvec.rs:1232:1
> > |
> > 1232 | #[macros::kunit_tests(rust_kvec_kunit)]
> > | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> > |
> > = note: this `Result` may be an `Err` variant, which should be handled
> > = note: `#[warn(unused_must_use)]` on by default
> > = note: this warning originates in the attribute macro `macros::kunit_tests`
> > (in Nightly builds, run with -Z macro-backtrace for more info)
>
> Yes, I'm aware, I tried playing with that myself. I really meant the question as
> I wrote, not as "Can you please change that?". :-) Sorry for the confusion.
One downside is that returning a Result doesn't show the line of code
where it failed.
> > > > + /// Verify correctness for one specific function.
> > > > + #[expect(clippy::needless_range_loop)]
> > > > + fn verify(c: &[bool]) {
> > > > + let mut vec1: KVec<usize> = KVec::with_capacity(c.len(), GFP_KERNEL).unwrap();
> > > > + let mut vec2: KVec<usize> = KVec::with_capacity(c.len(), GFP_KERNEL).unwrap();
> > > > +
> > > > + for i in 0..c.len() {
> > > > + vec1.push_within_capacity(i).unwrap();
> > > > + if c[i] {
> > > > + vec2.push_within_capacity(i).unwrap();
> > > > + }
> > > > + }
> > > > +
> > > > + vec1.retain(|i| c[*i]);
> > > > +
> > > > + assert_eq!(vec1, vec2);
> > >
> > > Don't we have macros around kunit_assert!() and kunit_assert_eq() outside of
> > > doctests (i.e. dedicated kunit tests)?
> > >
> > > I much prefer their output over the kernel panic we get with the "normal"
> > > asserts, unwraps, etc.
> > >
> > > Consistently sticking to the same output on failure makes it easier to catch and
> > > easier to setup CI environments.
> >
> > The documentation for those macros says "Public but hidden since it
> > should only be used from generated tests." so I don't think I'm supposed
> > to use them.
>
> Same here, that's more a fundamental question, rather than something for you to
> change right away.
>
> I really like the way doctests implement the assert macros and how they appear
> in the kernel log compared to panics through the "real" assert ones, unwraps,
> etc.
>
> I also think that avoiding things that directly panic in doctests (i.e. example
> code) is the correct thing to do. For KUnit tests it's probably less important,
> since they don't directly serve as sample code.
>
> So, I wonder what's our take on that. Do we want to have KUnit and doctests
> aligned? I think that'd be a good thing.
I think that both of these would be reasonable to fix. Also the fact
that I had to do #[macros::kunit_tests()] instead of just #[kunit_tests()].
Alice
© 2016 - 2026 Red Hat, Inc.