[PATCH] rust: introduce KVec::pop()

[PATCH] rust: introduce KVec::pop()

[Bernhard Kauer]

Provides a simple stack together with push().

Signed-off-by: Bernhard Kauer <bk@alpico.io>
---
 rust/kernel/alloc/kvec.rs | 28 ++++++++++++++++++++++++++++
 1 file changed, 28 insertions(+)

diff --git a/rust/kernel/alloc/kvec.rs b/rust/kernel/alloc/kvec.rs
index ae9d072741ce..ba6c265ad5c5 100644
--- a/rust/kernel/alloc/kvec.rs
+++ b/rust/kernel/alloc/kvec.rs
@@ -302,6 +302,34 @@ pub fn push(&mut self, v: T, flags: Flags) -> Result<(), AllocError> {
         Ok(())
     }
 
+    /// Removes the last element from the [`Vec`] instance.
+    ///
+    /// # Examples
+    ///
+    /// ```
+    /// let mut v = KVec::new();
+    /// v.push(1, GFP_KERNEL)?;
+    /// v.push(2, GFP_KERNEL)?;
+    /// assert_eq!(Some(2), v.pop());
+    /// assert_eq!(Some(1), v.pop());
+    /// assert_eq!(None, v.pop());
+    /// assert!(v.is_empty());
+    /// ```
+    pub fn pop(&mut self) -> Option<T> {
+        if self.len() == 0 {
+            return None;
+        }
+        self.len -= 1;
+
+        // SAFETY:
+        // - `ptr` is valid because there is at least one entry stored.
+        let ptr = unsafe { self.as_mut_ptr().add(self.len) };
+
+        // SAFETY:
+        // - `ptr` is properly aligned and valid for reads.
+        Some(unsafe { core::ptr::read(ptr) })
+    }
+
     /// Creates a new [`Vec`] instance with at least the given capacity.
     ///
     /// # Examples
-- 
2.45.2

Re: [PATCH] rust: introduce KVec::pop()

[Greg KH]

On Mon, Dec 02, 2024 at 09:57:58AM +0100, Bernhard Kauer wrote:
> Provides a simple stack together with push().

Why?

> 
> Signed-off-by: Bernhard Kauer <bk@alpico.io>
> ---
>  rust/kernel/alloc/kvec.rs | 28 ++++++++++++++++++++++++++++
>  1 file changed, 28 insertions(+)

What in-kernel code needs this?  Shouldn't you also submit this when
that happens?

thanks,

greg k-h

Re: [PATCH] rust: introduce KVec::pop()

[Bernhard Kauer]

On Mon, Dec 02, 2024 at 10:06:58AM +0100, Greg KH wrote:
> On Mon, Dec 02, 2024 at 09:57:58AM +0100, Bernhard Kauer wrote:
> > Provides a simple stack together with push().
> 
> Why?

Sorry, I forgot the reasoning. 

Updating to v6.13 broke my external Rust module due to the switch from
alloc::vec::Vec to kernel::alloc::Vec.  A trivial /s/Vec/KVec/ fixed most
cases.  However, there is no pop() or any other safe way to remove entries
from the vector.

So it looks like I either have to stick to unsafe code or add the
functionality inside the kernel.


> >  rust/kernel/alloc/kvec.rs | 28 ++++++++++++++++++++++++++++
> >  1 file changed, 28 insertions(+)
> 
> What in-kernel code needs this?  Shouldn't you also submit this when
> that happens?

Fair point.  Unfortunatelly my code is far from ready for inclusion.
Would a samples/rust/some_example.rs be sufficient?


Thanks,

	Bernhard

Re: [PATCH] rust: introduce KVec::pop()

[Danilo Krummrich]

On Mon, Dec 02, 2024 at 10:54:52AM +0100, Bernhard Kauer wrote:
> On Mon, Dec 02, 2024 at 10:06:58AM +0100, Greg KH wrote:
> > On Mon, Dec 02, 2024 at 09:57:58AM +0100, Bernhard Kauer wrote:
> > > Provides a simple stack together with push().
> > 
> > Why?
> 
> Sorry, I forgot the reasoning. 
> 
> Updating to v6.13 broke my external Rust module due to the switch from
> alloc::vec::Vec to kernel::alloc::Vec.  A trivial /s/Vec/KVec/ fixed most
> cases.  However, there is no pop() or any other safe way to remove entries
> from the vector.
> 
> So it looks like I either have to stick to unsafe code or add the
> functionality inside the kernel.
> 
> 
> > >  rust/kernel/alloc/kvec.rs | 28 ++++++++++++++++++++++++++++
> > >  1 file changed, 28 insertions(+)
> > 
> > What in-kernel code needs this?  Shouldn't you also submit this when
> > that happens?
> 
> Fair point.  Unfortunatelly my code is far from ready for inclusion.
> Would a samples/rust/some_example.rs be sufficient?

I don't think this is needed. You're providing an example that can be executed
as KUnit test already with your patch.

This is a pretty basic function and surely gonna be needed for other drivers in
the future as well.

IMHO, *for this* it's enough if you have something that aims for inclusion
upstream, as long as you reference it properly in the commit message.

- Danilo

> 
> 
> Thanks,
> 
> 	Bernhard
> 

Re: [PATCH] rust: introduce KVec::pop()

[Miguel Ojeda]

On Mon, Dec 2, 2024 at 12:00 PM Danilo Krummrich <dakr@kernel.org> wrote:
>
> I don't think this is needed. You're providing an example that can be executed
> as KUnit test already with your patch.

Yeah, the example should be good enough for the time being, as long as
we see an actual user.

Cheers,
Miguel

Re: [PATCH] rust: introduce KVec::pop()

[Danilo Krummrich]

On Mon, Dec 02, 2024 at 09:57:58AM +0100, Bernhard Kauer wrote:
> Provides a simple stack together with push().

Your commit message should also explain the motivation of the patch, i.e. what
do you need this new function for.

> 
> Signed-off-by: Bernhard Kauer <bk@alpico.io>
> ---
>  rust/kernel/alloc/kvec.rs | 28 ++++++++++++++++++++++++++++
>  1 file changed, 28 insertions(+)
> 
> diff --git a/rust/kernel/alloc/kvec.rs b/rust/kernel/alloc/kvec.rs
> index ae9d072741ce..ba6c265ad5c5 100644
> --- a/rust/kernel/alloc/kvec.rs
> +++ b/rust/kernel/alloc/kvec.rs
> @@ -302,6 +302,34 @@ pub fn push(&mut self, v: T, flags: Flags) -> Result<(), AllocError> {
>          Ok(())
>      }
>  
> +    /// Removes the last element from the [`Vec`] instance.

It not only removes it, but also returns the corresponding element (or `None` if
the vector is empty). Please add this to the documentation.

It would also be worth noting that users may want to shrink the vector after
removing elements, however, we don't implement the `shrink_*` functions yet.

> +    ///
> +    /// # Examples
> +    ///
> +    /// ```
> +    /// let mut v = KVec::new();
> +    /// v.push(1, GFP_KERNEL)?;
> +    /// v.push(2, GFP_KERNEL)?;
> +    /// assert_eq!(Some(2), v.pop());
> +    /// assert_eq!(Some(1), v.pop());
> +    /// assert_eq!(None, v.pop());
> +    /// assert!(v.is_empty());
> +    /// ```
> +    pub fn pop(&mut self) -> Option<T> {
> +        if self.len() == 0 {
> +            return None;
> +        }
> +        self.len -= 1;
> +
> +        // SAFETY:
> +        // - `ptr` is valid because there is at least one entry stored.
> +        let ptr = unsafe { self.as_mut_ptr().add(self.len) };
> +
> +        // SAFETY:
> +        // - `ptr` is properly aligned and valid for reads.
> +        Some(unsafe { core::ptr::read(ptr) })
> +    }
> +
>      /// Creates a new [`Vec`] instance with at least the given capacity.
>      ///
>      /// # Examples
> -- 
> 2.45.2
> 

Re: [PATCH] rust: introduce KVec::pop()

[Miguel Ojeda]

On Mon, Dec 2, 2024 at 9:58 AM Bernhard Kauer <bk@alpico.io> wrote:
>
> Provides a simple stack together with push().
>
> Signed-off-by: Bernhard Kauer <bk@alpico.io>

Thanks for the patch!

> +    /// # Examples
> +    ///
> +    /// ```
> +    /// let mut v = KVec::new();
> +    /// v.push(1, GFP_KERNEL)?;
> +    /// v.push(2, GFP_KERNEL)?;
> +    /// assert_eq!(Some(2), v.pop());
> +    /// assert_eq!(Some(1), v.pop());
> +    /// assert_eq!(None, v.pop());
> +    /// assert!(v.is_empty());
> +    /// ```

We could simplify the example using `kvec!`... Though I see other
examples in the file using this pattern, so it is OK for this patch.
Perhaps we could simplify those in another patch series / a "good
first issue" (unless Danilo was following a particular pattern here).

If we want this kind of example, I guess we could perhaps interleave a
push (so that we don't just test adding all and then removing all). Or
we could just go even simpler and do something like `std`'s `pop`
does:

    let mut vec = vec![1, 2, 3];
    assert_eq!(vec.pop(), Some(3));
    assert_eq!(vec, [1, 2]);

(To be clear, more examples are good as long as they show/test
something different, so if you have something that would be good to
test, that is great to add!).

> +        // SAFETY:
> +        // - `ptr` is valid because there is at least one entry stored.

For single-liners, you don't need a list (please see other `SAFETY` comments).

You could also ideally mention why that statement is true, e.g. "...
since the length was just checked above".

Thanks!

Cheers,
Miguel

Re: [PATCH] rust: introduce KVec::pop()

[kernel test robot]

Hi Bernhard,

kernel test robot noticed the following build errors:

[auto build test ERROR on rust/rust-next]
[also build test ERROR on linus/master v6.13-rc1 next-20241128]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Bernhard-Kauer/rust-introduce-KVec-pop/20241202-170049
base:   https://github.com/Rust-for-Linux/linux rust-next
patch link:    https://lore.kernel.org/r/20241202085758.67319-1-bk%40alpico.io
patch subject: [PATCH] rust: introduce KVec::pop()
config: x86_64-buildonly-randconfig-001-20241203 (https://download.01.org/0day-ci/archive/20241203/202412031055.b3nviKAQ-lkp@intel.com/config)
compiler: clang version 19.1.3 (https://github.com/llvm/llvm-project ab51eccf88f5321e7c60591c5546b254b6afab99)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20241203/202412031055.b3nviKAQ-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202412031055.b3nviKAQ-lkp@intel.com/

All errors (new ones prefixed by >>):

>> error[E0277]: the `?` operator can only be used in a function that returns `Result` or `Option` (or another type that implements `FromResidual`)
   --> rust/doctests_kernel_generated.rs:752:22
   |
   750 | fn main() { #[allow(non_snake_case)] fn _doctest_main__kbuild_src_consumer_rust_kernel_alloc_kvec_rs_309_0() {
   |                                      ----------------------------------------------------------------------- this function should return `Result` or `Option` to accept `?`
   751 | let mut v = KVec::new();
   752 | v.push(1, GFP_KERNEL)?;
   |                      ^ cannot use the `?` operator in a function that returns `()`
   |
   = help: the trait `FromResidual<core::result::Result<Infallible, rust_doctest_kernel_alloc_kbox_rs_4::kernel::alloc::AllocError>>` is not implemented for `()`
--
>> error[E0277]: the `?` operator can only be used in a function that returns `Result` or `Option` (or another type that implements `FromResidual`)
   --> rust/doctests_kernel_generated.rs:753:22
   |
   750 | fn main() { #[allow(non_snake_case)] fn _doctest_main__kbuild_src_consumer_rust_kernel_alloc_kvec_rs_309_0() {
   |                                      ----------------------------------------------------------------------- this function should return `Result` or `Option` to accept `?`
   ...
   753 | v.push(2, GFP_KERNEL)?;
   |                      ^ cannot use the `?` operator in a function that returns `()`
   |
   = help: the trait `FromResidual<core::result::Result<Infallible, rust_doctest_kernel_alloc_kbox_rs_4::kernel::alloc::AllocError>>` is not implemented for `()`

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki