[PATCH 0/2] rust: alloc: split `Vec::set_len` into `Vec::{inc,dec}_len`

[PATCH 0/2] rust: alloc: split `Vec::set_len` into `Vec::{inc,dec}_len`

[Tamir Duberstein]

This series is the product of a discussion[0] on the safety requirements
of `set_len`.

The patches adding `truncate`[1] and `clear`[2] will need to be updated
in light of this series.

Link: https://lore.kernel.org/all/20250315154436.65065-1-dakr@kernel.org/ [0]
Link: https://lore.kernel.org/all/20250316111644.154602-2-andrewjballance@gmail.com/ [1]
Link: https://lore.kernel.org/all/20250311-iov-iter-v1-4-f6c9134ea824@google.com/ [2]
Signed-off-by: Tamir Duberstein <tamird@gmail.com>
---
Tamir Duberstein (2):
      rust: alloc: replace `Vec::set_len` with `inc_len`
      rust: alloc: add `Vec::dec_len`

 rust/kernel/alloc/kvec.rs | 34 ++++++++++++++++++++++++----------
 rust/kernel/str.rs        |  2 +-
 rust/kernel/uaccess.rs    |  2 +-
 3 files changed, 26 insertions(+), 12 deletions(-)
---
base-commit: cf25bc61f8aecad9b0c45fe32697e35ea4b13378
change-id: 20250316-vec-set-len-99be6cc48374

Best regards,
-- 
Tamir Duberstein <tamird@gmail.com>

[PATCH 1/2] rust: alloc: replace `Vec::set_len` with `inc_len`

[Tamir Duberstein]

Rename `set_len` to `inc_len` and simplify its safety contract.
---
 rust/kernel/alloc/kvec.rs | 19 +++++++++----------
 rust/kernel/str.rs        |  2 +-
 rust/kernel/uaccess.rs    |  2 +-
 3 files changed, 11 insertions(+), 12 deletions(-)

diff --git a/rust/kernel/alloc/kvec.rs b/rust/kernel/alloc/kvec.rs
index ae9d072741ce..d43a1d609434 100644
--- a/rust/kernel/alloc/kvec.rs
+++ b/rust/kernel/alloc/kvec.rs
@@ -183,17 +183,16 @@ pub fn len(&self) -> usize {
         self.len
     }
 
-    /// Forcefully sets `self.len` to `new_len`.
+    /// Increments `self.len` by `additional`.
     ///
     /// # Safety
     ///
-    /// - `new_len` must be less than or equal to [`Self::capacity`].
-    /// - If `new_len` is greater than `self.len`, all elements within the interval
-    ///   [`self.len`,`new_len`) must be initialized.
+    /// - `self.len + additional` must be less than or equal to [`Self::capacity`].
+    /// - All elements within the interval [`self.len`,`self.len + additional`) must be initialized.
     #[inline]
-    pub unsafe fn set_len(&mut self, new_len: usize) {
-        debug_assert!(new_len <= self.capacity());
-        self.len = new_len;
+    pub unsafe fn inc_len(&mut self, additional: usize) {
+        debug_assert!(self.len() + additional <= self.capacity());
+        self.len += additional;
     }
 
     /// Returns a slice of the entire vector.
@@ -298,7 +297,7 @@ pub fn push(&mut self, v: T, flags: Flags) -> Result<(), AllocError> {
         // SAFETY: We just initialised the first spare entry, so it is safe to increase the length
         // by 1. We also know that the new length is <= capacity because of the previous call to
         // `reserve` above.
-        unsafe { self.set_len(self.len() + 1) };
+        unsafe { self.inc_len(1) };
         Ok(())
     }
 
@@ -475,7 +474,7 @@ pub fn extend_with(&mut self, n: usize, value: T, flags: Flags) -> Result<(), Al
         // SAFETY:
         // - `self.len() + n < self.capacity()` due to the call to reserve above,
         // - the loop and the line above initialized the next `n` elements.
-        unsafe { self.set_len(self.len() + n) };
+        unsafe { self.inc_len(n) };
 
         Ok(())
     }
@@ -506,7 +505,7 @@ pub fn extend_from_slice(&mut self, other: &[T], flags: Flags) -> Result<(), All
         //   the length by the same number.
         // - `self.len() + other.len() <= self.capacity()` is guaranteed by the preceding `reserve`
         //   call.
-        unsafe { self.set_len(self.len() + other.len()) };
+        unsafe { self.inc_len(other.len()) };
         Ok(())
     }
 
diff --git a/rust/kernel/str.rs b/rust/kernel/str.rs
index 28e2201604d6..005713839e9e 100644
--- a/rust/kernel/str.rs
+++ b/rust/kernel/str.rs
@@ -840,7 +840,7 @@ pub fn try_from_fmt(args: fmt::Arguments<'_>) -> Result<Self, Error> {
 
         // SAFETY: The number of bytes that can be written to `f` is bounded by `size`, which is
         // `buf`'s capacity. The contents of the buffer have been initialised by writes to `f`.
-        unsafe { buf.set_len(f.bytes_written()) };
+        unsafe { buf.inc_len(f.bytes_written()) };
 
         // Check that there are no `NUL` bytes before the end.
         // SAFETY: The buffer is valid for read because `f.bytes_written()` is bounded by `size`
diff --git a/rust/kernel/uaccess.rs b/rust/kernel/uaccess.rs
index 719b0a48ff55..0aa5455a18be 100644
--- a/rust/kernel/uaccess.rs
+++ b/rust/kernel/uaccess.rs
@@ -291,7 +291,7 @@ pub fn read_all<A: Allocator>(mut self, buf: &mut Vec<u8, A>, flags: Flags) -> R
 
         // SAFETY: Since the call to `read_raw` was successful, so the next `len` bytes of the
         // vector have been initialized.
-        unsafe { buf.set_len(buf.len() + len) };
+        unsafe { buf.inc_len(len) };
         Ok(())
     }
 }

-- 
2.48.1

[PATCH 2/2] rust: alloc: add `Vec::dec_len`

[Tamir Duberstein]

Add `Vec::dec_len` that reduces the length of the receiver. This method
is intended to be used from methods that remove elements from `Vec` such
as `truncate`, `pop`, `remove`, and others. This method is intentionally
not `pub`.

Signed-off-by: Tamir Duberstein <tamird@gmail.com>
---
 rust/kernel/alloc/kvec.rs | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/rust/kernel/alloc/kvec.rs b/rust/kernel/alloc/kvec.rs
index d43a1d609434..5d604e04b9a5 100644
--- a/rust/kernel/alloc/kvec.rs
+++ b/rust/kernel/alloc/kvec.rs
@@ -195,6 +195,21 @@ pub unsafe fn inc_len(&mut self, additional: usize) {
         self.len += additional;
     }
 
+    /// Decreases `self.len` by `count`.
+    ///
+    /// Returns a mutable reference to the removed elements.
+    ///
+    /// # Safety
+    ///
+    /// - `count` must be less than or equal to `self.len`.
+    unsafe fn dec_len(&mut self, count: usize) -> &mut [T] {
+        debug_assert!(count <= self.len());
+        self.len -= count;
+        // SAFETY: The memory between `self.len` and `self.len + count` is guaranteed to contain
+        // `self.len` initialized elements of type `T`.
+        unsafe { slice::from_raw_parts_mut(self.as_mut_ptr().add(self.len), count) }
+    }
+
     /// Returns a slice of the entire vector.
     #[inline]
     pub fn as_slice(&self) -> &[T] {

-- 
2.48.1

Re: [PATCH 2/2] rust: alloc: add `Vec::dec_len`

[Tamir Duberstein]

On Sun, Mar 16, 2025 at 6:32 PM Tamir Duberstein <tamird@gmail.com> wrote:
>
> Add `Vec::dec_len` that reduces the length of the receiver. This method
> is intended to be used from methods that remove elements from `Vec` such
> as `truncate`, `pop`, `remove`, and others. This method is intentionally
> not `pub`.
>
> Signed-off-by: Tamir Duberstein <tamird@gmail.com>
> ---
>  rust/kernel/alloc/kvec.rs | 15 +++++++++++++++
>  1 file changed, 15 insertions(+)
>
> diff --git a/rust/kernel/alloc/kvec.rs b/rust/kernel/alloc/kvec.rs
> index d43a1d609434..5d604e04b9a5 100644
> --- a/rust/kernel/alloc/kvec.rs
> +++ b/rust/kernel/alloc/kvec.rs
> @@ -195,6 +195,21 @@ pub unsafe fn inc_len(&mut self, additional: usize) {
>          self.len += additional;
>      }
>
> +    /// Decreases `self.len` by `count`.
> +    ///
> +    /// Returns a mutable reference to the removed elements.
> +    ///
> +    /// # Safety
> +    ///
> +    /// - `count` must be less than or equal to `self.len`.
> +    unsafe fn dec_len(&mut self, count: usize) -> &mut [T] {
> +        debug_assert!(count <= self.len());
> +        self.len -= count;
> +        // SAFETY: The memory between `self.len` and `self.len + count` is guaranteed to contain
> +        // `self.len` initialized elements of type `T`.

Oops, this should be

>        // SAFETY: The memory after `self.len()` is guaranteed to contain `count` initialized
>        // elements of type `T`.

Let me know if I should respin or if this can be changed when applied.



> +        unsafe { slice::from_raw_parts_mut(self.as_mut_ptr().add(self.len), count) }
> +    }
> +
>      /// Returns a slice of the entire vector.
>      #[inline]
>      pub fn as_slice(&self) -> &[T] {
>
> --
> 2.48.1
>

Re: [PATCH 2/2] rust: alloc: add `Vec::dec_len`

[Danilo Krummrich]

On Sun, Mar 16, 2025 at 06:32:01PM -0400, Tamir Duberstein wrote:
> Add `Vec::dec_len` that reduces the length of the receiver. This method
> is intended to be used from methods that remove elements from `Vec` such
> as `truncate`, `pop`, `remove`, and others. This method is intentionally
> not `pub`.
> 
> Signed-off-by: Tamir Duberstein <tamird@gmail.com>
> ---
>  rust/kernel/alloc/kvec.rs | 15 +++++++++++++++
>  1 file changed, 15 insertions(+)
> 
> diff --git a/rust/kernel/alloc/kvec.rs b/rust/kernel/alloc/kvec.rs
> index d43a1d609434..5d604e04b9a5 100644
> --- a/rust/kernel/alloc/kvec.rs
> +++ b/rust/kernel/alloc/kvec.rs
> @@ -195,6 +195,21 @@ pub unsafe fn inc_len(&mut self, additional: usize) {
>          self.len += additional;
>      }
>  
> +    /// Decreases `self.len` by `count`.
> +    ///
> +    /// Returns a mutable reference to the removed elements.
> +    ///
> +    /// # Safety
> +    ///
> +    /// - `count` must be less than or equal to `self.len`.

Why? We can catch this, no?

We can keep the debug_assert!(), but use self.len.saturating_sub(count) instead.

> +    unsafe fn dec_len(&mut self, count: usize) -> &mut [T] {
> +        debug_assert!(count <= self.len());
> +        self.len -= count;
> +        // SAFETY: The memory between `self.len` and `self.len + count` is guaranteed to contain
> +        // `self.len` initialized elements of type `T`.
> +        unsafe { slice::from_raw_parts_mut(self.as_mut_ptr().add(self.len), count) }
> +    }
> +
>      /// Returns a slice of the entire vector.
>      #[inline]
>      pub fn as_slice(&self) -> &[T] {
> 
> -- 
> 2.48.1
> 

Re: [PATCH 2/2] rust: alloc: add `Vec::dec_len`

[Tamir Duberstein]

On Sun, Mar 16, 2025 at 6:42 PM Danilo Krummrich <dakr@kernel.org> wrote:
>
> On Sun, Mar 16, 2025 at 06:32:01PM -0400, Tamir Duberstein wrote:
> > Add `Vec::dec_len` that reduces the length of the receiver. This method
> > is intended to be used from methods that remove elements from `Vec` such
> > as `truncate`, `pop`, `remove`, and others. This method is intentionally
> > not `pub`.
> >
> > Signed-off-by: Tamir Duberstein <tamird@gmail.com>
> > ---
> >  rust/kernel/alloc/kvec.rs | 15 +++++++++++++++
> >  1 file changed, 15 insertions(+)
> >
> > diff --git a/rust/kernel/alloc/kvec.rs b/rust/kernel/alloc/kvec.rs
> > index d43a1d609434..5d604e04b9a5 100644
> > --- a/rust/kernel/alloc/kvec.rs
> > +++ b/rust/kernel/alloc/kvec.rs
> > @@ -195,6 +195,21 @@ pub unsafe fn inc_len(&mut self, additional: usize) {
> >          self.len += additional;
> >      }
> >
> > +    /// Decreases `self.len` by `count`.
> > +    ///
> > +    /// Returns a mutable reference to the removed elements.
> > +    ///
> > +    /// # Safety
> > +    ///
> > +    /// - `count` must be less than or equal to `self.len`.
>
> Why? We can catch this, no?
>
> We can keep the debug_assert!(), but use self.len.saturating_sub(count) instead.

This is why I didn't want to write this until we had an actual caller :)

We can, but it's not clear why that's better. What does it mean if the
caller asked to decrement by more than self.len? Again, my preference
is that this is introduced when there's a second caller.

Re: [PATCH 2/2] rust: alloc: add `Vec::dec_len`

[Danilo Krummrich]

On Sun, Mar 16, 2025 at 06:47:42PM -0400, Tamir Duberstein wrote:
> On Sun, Mar 16, 2025 at 6:42 PM Danilo Krummrich <dakr@kernel.org> wrote:
> >
> > On Sun, Mar 16, 2025 at 06:32:01PM -0400, Tamir Duberstein wrote:
> > > Add `Vec::dec_len` that reduces the length of the receiver. This method
> > > is intended to be used from methods that remove elements from `Vec` such
> > > as `truncate`, `pop`, `remove`, and others. This method is intentionally
> > > not `pub`.
> > >
> > > Signed-off-by: Tamir Duberstein <tamird@gmail.com>
> > > ---
> > >  rust/kernel/alloc/kvec.rs | 15 +++++++++++++++
> > >  1 file changed, 15 insertions(+)
> > >
> > > diff --git a/rust/kernel/alloc/kvec.rs b/rust/kernel/alloc/kvec.rs
> > > index d43a1d609434..5d604e04b9a5 100644
> > > --- a/rust/kernel/alloc/kvec.rs
> > > +++ b/rust/kernel/alloc/kvec.rs
> > > @@ -195,6 +195,21 @@ pub unsafe fn inc_len(&mut self, additional: usize) {
> > >          self.len += additional;
> > >      }
> > >
> > > +    /// Decreases `self.len` by `count`.
> > > +    ///
> > > +    /// Returns a mutable reference to the removed elements.
> > > +    ///
> > > +    /// # Safety
> > > +    ///
> > > +    /// - `count` must be less than or equal to `self.len`.
> >
> > Why? We can catch this, no?
> >
> > We can keep the debug_assert!(), but use self.len.saturating_sub(count) instead.
> 
> This is why I didn't want to write this until we had an actual caller :)

That just defers this question, the methods you mention in your commit message
will be added, hence I think it's better to do it right away.

> We can, but it's not clear why that's better. What does it mean if the
> caller asked to decrement by more than self.len?

It tells us that the caller is buggy, but that's what the debug_assert!() is
for.

But to me both is fine, it's also good when the caller has to justify.

Forgot to mention, for dec_len(), please add the corresponding invariant comment
when adjusting self.len.

Re: [PATCH 2/2] rust: alloc: add `Vec::dec_len`

[Tamir Duberstein]

On Sun, Mar 16, 2025 at 7:02 PM Danilo Krummrich <dakr@kernel.org> wrote:
>
> On Sun, Mar 16, 2025 at 06:47:42PM -0400, Tamir Duberstein wrote:
> > On Sun, Mar 16, 2025 at 6:42 PM Danilo Krummrich <dakr@kernel.org> wrote:
> > >
> > > On Sun, Mar 16, 2025 at 06:32:01PM -0400, Tamir Duberstein wrote:
> > > > Add `Vec::dec_len` that reduces the length of the receiver. This method
> > > > is intended to be used from methods that remove elements from `Vec` such
> > > > as `truncate`, `pop`, `remove`, and others. This method is intentionally
> > > > not `pub`.
> > > >
> > > > Signed-off-by: Tamir Duberstein <tamird@gmail.com>
> > > > ---
> > > >  rust/kernel/alloc/kvec.rs | 15 +++++++++++++++
> > > >  1 file changed, 15 insertions(+)
> > > >
> > > > diff --git a/rust/kernel/alloc/kvec.rs b/rust/kernel/alloc/kvec.rs
> > > > index d43a1d609434..5d604e04b9a5 100644
> > > > --- a/rust/kernel/alloc/kvec.rs
> > > > +++ b/rust/kernel/alloc/kvec.rs
> > > > @@ -195,6 +195,21 @@ pub unsafe fn inc_len(&mut self, additional: usize) {
> > > >          self.len += additional;
> > > >      }
> > > >
> > > > +    /// Decreases `self.len` by `count`.
> > > > +    ///
> > > > +    /// Returns a mutable reference to the removed elements.
> > > > +    ///
> > > > +    /// # Safety
> > > > +    ///
> > > > +    /// - `count` must be less than or equal to `self.len`.
> > >
> > > Why? We can catch this, no?
> > >
> > > We can keep the debug_assert!(), but use self.len.saturating_sub(count) instead.
> >
> > This is why I didn't want to write this until we had an actual caller :)
>
> That just defers this question, the methods you mention in your commit message
> will be added, hence I think it's better to do it right away.
>
> > We can, but it's not clear why that's better. What does it mean if the
> > caller asked to decrement by more than self.len?
>
> It tells us that the caller is buggy, but that's what the debug_assert!() is
> for.
>
> But to me both is fine, it's also good when the caller has to justify.

Ok! I've left this as-is.

> Forgot to mention, for dec_len(), please add the corresponding invariant comment
> when adjusting self.len.

Does this suit?

>         // INVARIANT: By the safety requirements of this method `self.len - count` represents the
>         // exact number of elements stored within `self`.

Re: [PATCH 1/2] rust: alloc: replace `Vec::set_len` with `inc_len`

[Benno Lossin]

On Sun Mar 16, 2025 at 11:32 PM CET, Tamir Duberstein wrote:
> Rename `set_len` to `inc_len` and simplify its safety contract.
> ---
>  rust/kernel/alloc/kvec.rs | 19 +++++++++----------
>  rust/kernel/str.rs        |  2 +-
>  rust/kernel/uaccess.rs    |  2 +-
>  3 files changed, 11 insertions(+), 12 deletions(-)
>
> diff --git a/rust/kernel/alloc/kvec.rs b/rust/kernel/alloc/kvec.rs
> index ae9d072741ce..d43a1d609434 100644
> --- a/rust/kernel/alloc/kvec.rs
> +++ b/rust/kernel/alloc/kvec.rs
> @@ -183,17 +183,16 @@ pub fn len(&self) -> usize {
>          self.len
>      }
>  
> -    /// Forcefully sets `self.len` to `new_len`.
> +    /// Increments `self.len` by `additional`.

I would keep the "Forcefully".

>      ///
>      /// # Safety
>      ///
> -    /// - `new_len` must be less than or equal to [`Self::capacity`].
> -    /// - If `new_len` is greater than `self.len`, all elements within the interval
> -    ///   [`self.len`,`new_len`) must be initialized.
> +    /// - `self.len + additional` must be less than or equal to [`Self::capacity`].
> +    /// - All elements within the interval [`self.len`,`self.len + additional`) must be initialized.
>      #[inline]
> -    pub unsafe fn set_len(&mut self, new_len: usize) {
> -        debug_assert!(new_len <= self.capacity());
> -        self.len = new_len;
> +    pub unsafe fn inc_len(&mut self, additional: usize) {
> +        debug_assert!(self.len() + additional <= self.capacity());

What if this overflows? Do we always have overflow debugging on when
debug assertions are enabled? If yes, then this is fine.

> +        self.len += additional;
>      }
>  
>      /// Returns a slice of the entire vector.

> diff --git a/rust/kernel/str.rs b/rust/kernel/str.rs
> index 28e2201604d6..005713839e9e 100644
> --- a/rust/kernel/str.rs
> +++ b/rust/kernel/str.rs
> @@ -840,7 +840,7 @@ pub fn try_from_fmt(args: fmt::Arguments<'_>) -> Result<Self, Error> {
>  
>          // SAFETY: The number of bytes that can be written to `f` is bounded by `size`, which is
>          // `buf`'s capacity. The contents of the buffer have been initialised by writes to `f`.
> -        unsafe { buf.set_len(f.bytes_written()) };
> +        unsafe { buf.inc_len(f.bytes_written()) };

This change seems wrong unless the code was wrong to begin with.

Otherwise the change looks good.

---
Cheers,
Benno

>  
>          // Check that there are no `NUL` bytes before the end.
>          // SAFETY: The buffer is valid for read because `f.bytes_written()` is bounded by `size`
> diff --git a/rust/kernel/uaccess.rs b/rust/kernel/uaccess.rs
> index 719b0a48ff55..0aa5455a18be 100644
> --- a/rust/kernel/uaccess.rs
> +++ b/rust/kernel/uaccess.rs
> @@ -291,7 +291,7 @@ pub fn read_all<A: Allocator>(mut self, buf: &mut Vec<u8, A>, flags: Flags) -> R
>  
>          // SAFETY: Since the call to `read_raw` was successful, so the next `len` bytes of the
>          // vector have been initialized.
> -        unsafe { buf.set_len(buf.len() + len) };
> +        unsafe { buf.inc_len(len) };
>          Ok(())
>      }
>  }

Re: [PATCH 2/2] rust: alloc: add `Vec::dec_len`

[Benno Lossin]

On Sun Mar 16, 2025 at 11:32 PM CET, Tamir Duberstein wrote:
> Add `Vec::dec_len` that reduces the length of the receiver. This method
> is intended to be used from methods that remove elements from `Vec` such
> as `truncate`, `pop`, `remove`, and others. This method is intentionally
> not `pub`.

I think it should be `pub`. Otherwise we're loosing functionality
compared to now. If one decides to give the raw pointer to some C API
that takes ownership of the pointer, then I want them to be able to call
`dec_len` manually. 

>
> Signed-off-by: Tamir Duberstein <tamird@gmail.com>
> ---
>  rust/kernel/alloc/kvec.rs | 15 +++++++++++++++
>  1 file changed, 15 insertions(+)
>
> diff --git a/rust/kernel/alloc/kvec.rs b/rust/kernel/alloc/kvec.rs
> index d43a1d609434..5d604e04b9a5 100644
> --- a/rust/kernel/alloc/kvec.rs
> +++ b/rust/kernel/alloc/kvec.rs
> @@ -195,6 +195,21 @@ pub unsafe fn inc_len(&mut self, additional: usize) {
>          self.len += additional;
>      }
>  
> +    /// Decreases `self.len` by `count`.
> +    ///
> +    /// Returns a mutable reference to the removed elements.

s/reference/slice/

I would also mention here that the elements won't be dropped when the
user doesn't do that manually using the slice. So explain that this is a
low-level operation and `clear` or `truncate` should be used instead
where possible.

> +    ///
> +    /// # Safety
> +    ///
> +    /// - `count` must be less than or equal to `self.len`.

I also think that we should use saturating_sub instead and then not have
to worry about this. (It should still be documented in the function
though). That way this can also be a safe function.

---
Cheers,
Benno

> +    unsafe fn dec_len(&mut self, count: usize) -> &mut [T] {
> +        debug_assert!(count <= self.len());
> +        self.len -= count;
> +        // SAFETY: The memory between `self.len` and `self.len + count` is guaranteed to contain
> +        // `self.len` initialized elements of type `T`.
> +        unsafe { slice::from_raw_parts_mut(self.as_mut_ptr().add(self.len), count) }
> +    }
> +
>      /// Returns a slice of the entire vector.
>      #[inline]
>      pub fn as_slice(&self) -> &[T] {

Re: [PATCH 1/2] rust: alloc: replace `Vec::set_len` with `inc_len`

[Miguel Ojeda]

On Mon, Mar 17, 2025 at 10:58 AM Benno Lossin <benno.lossin@proton.me> wrote:
>
> What if this overflows? Do we always have overflow debugging on when
> debug assertions are enabled? If yes, then this is fine.

No, they are independent settings.

> This change seems wrong unless the code was wrong to begin with.

Yeah, even if it is correct, it should definitely be described in the
commit log, because it does look like a functional change.

Cheers,
Miguel

Re: [PATCH 1/2] rust: alloc: replace `Vec::set_len` with `inc_len`

[Alice Ryhl]

On Mon, Mar 17, 2025 at 09:58:35AM +0000, Benno Lossin wrote:
> On Sun Mar 16, 2025 at 11:32 PM CET, Tamir Duberstein wrote:
> > Rename `set_len` to `inc_len` and simplify its safety contract.
> > ---
> >  rust/kernel/alloc/kvec.rs | 19 +++++++++----------
> >  rust/kernel/str.rs        |  2 +-
> >  rust/kernel/uaccess.rs    |  2 +-
> >  3 files changed, 11 insertions(+), 12 deletions(-)
> >
> > diff --git a/rust/kernel/alloc/kvec.rs b/rust/kernel/alloc/kvec.rs
> > index ae9d072741ce..d43a1d609434 100644
> > --- a/rust/kernel/alloc/kvec.rs
> > +++ b/rust/kernel/alloc/kvec.rs
> > @@ -183,17 +183,16 @@ pub fn len(&self) -> usize {
> >          self.len
> >      }
> >  
> > -    /// Forcefully sets `self.len` to `new_len`.
> > +    /// Increments `self.len` by `additional`.
> 
> I would keep the "Forcefully".
> 
> >      ///
> >      /// # Safety
> >      ///
> > -    /// - `new_len` must be less than or equal to [`Self::capacity`].
> > -    /// - If `new_len` is greater than `self.len`, all elements within the interval
> > -    ///   [`self.len`,`new_len`) must be initialized.
> > +    /// - `self.len + additional` must be less than or equal to [`Self::capacity`].
> > +    /// - All elements within the interval [`self.len`,`self.len + additional`) must be initialized.
> >      #[inline]
> > -    pub unsafe fn set_len(&mut self, new_len: usize) {
> > -        debug_assert!(new_len <= self.capacity());
> > -        self.len = new_len;
> > +    pub unsafe fn inc_len(&mut self, additional: usize) {
> > +        debug_assert!(self.len() + additional <= self.capacity());
> 
> What if this overflows? Do we always have overflow debugging on when
> debug assertions are enabled? If yes, then this is fine.

I don't think we do.

> > diff --git a/rust/kernel/str.rs b/rust/kernel/str.rs
> > index 28e2201604d6..005713839e9e 100644
> > --- a/rust/kernel/str.rs
> > +++ b/rust/kernel/str.rs
> > @@ -840,7 +840,7 @@ pub fn try_from_fmt(args: fmt::Arguments<'_>) -> Result<Self, Error> {
> >  
> >          // SAFETY: The number of bytes that can be written to `f` is bounded by `size`, which is
> >          // `buf`'s capacity. The contents of the buffer have been initialised by writes to `f`.
> > -        unsafe { buf.set_len(f.bytes_written()) };
> > +        unsafe { buf.inc_len(f.bytes_written()) };
> 
> This change seems wrong unless the code was wrong to begin with.
> 
> Otherwise the change looks good.

The buffer has length zero as it was just created with:

let mut buf = KVec::with_capacity(size, GFP_KERNEL)?;

Alice

Re: [PATCH 1/2] rust: alloc: replace `Vec::set_len` with `inc_len`

[Alice Ryhl]

On Sun, Mar 16, 2025 at 06:32:00PM -0400, Tamir Duberstein wrote:
> Rename `set_len` to `inc_len` and simplify its safety contract.

You're missing a Signed-off-by tag.

>  rust/kernel/alloc/kvec.rs | 19 +++++++++----------
>  rust/kernel/str.rs        |  2 +-
>  rust/kernel/uaccess.rs    |  2 +-
>  3 files changed, 11 insertions(+), 12 deletions(-)
> 
> diff --git a/rust/kernel/alloc/kvec.rs b/rust/kernel/alloc/kvec.rs
> index ae9d072741ce..d43a1d609434 100644
> --- a/rust/kernel/alloc/kvec.rs
> +++ b/rust/kernel/alloc/kvec.rs
> @@ -183,17 +183,16 @@ pub fn len(&self) -> usize {
>          self.len
>      }
>  
> -    /// Forcefully sets `self.len` to `new_len`.
> +    /// Increments `self.len` by `additional`.
>      ///
>      /// # Safety
>      ///
> -    /// - `new_len` must be less than or equal to [`Self::capacity`].
> -    /// - If `new_len` is greater than `self.len`, all elements within the interval
> -    ///   [`self.len`,`new_len`) must be initialized.
> +    /// - `self.len + additional` must be less than or equal to [`Self::capacity`].
> +    /// - All elements within the interval [`self.len`,`self.len + additional`) must be initialized.
>      #[inline]
> -    pub unsafe fn set_len(&mut self, new_len: usize) {
> -        debug_assert!(new_len <= self.capacity());
> -        self.len = new_len;
> +    pub unsafe fn inc_len(&mut self, additional: usize) {
> +        debug_assert!(self.len() + additional <= self.capacity());
> +        self.len += additional;

I guess we could use an INVARIANT: comment here.

Alice

Re: [PATCH 1/2] rust: alloc: replace `Vec::set_len` with `inc_len`

[Danilo Krummrich]

On Mon, Mar 17, 2025 at 10:50:15AM +0000, Alice Ryhl wrote:
> On Sun, Mar 16, 2025 at 06:32:00PM -0400, Tamir Duberstein wrote:
> > Rename `set_len` to `inc_len` and simplify its safety contract.
> 
> You're missing a Signed-off-by tag.
> 
> >  rust/kernel/alloc/kvec.rs | 19 +++++++++----------
> >  rust/kernel/str.rs        |  2 +-
> >  rust/kernel/uaccess.rs    |  2 +-
> >  3 files changed, 11 insertions(+), 12 deletions(-)
> > 
> > diff --git a/rust/kernel/alloc/kvec.rs b/rust/kernel/alloc/kvec.rs
> > index ae9d072741ce..d43a1d609434 100644
> > --- a/rust/kernel/alloc/kvec.rs
> > +++ b/rust/kernel/alloc/kvec.rs
> > @@ -183,17 +183,16 @@ pub fn len(&self) -> usize {
> >          self.len
> >      }
> >  
> > -    /// Forcefully sets `self.len` to `new_len`.
> > +    /// Increments `self.len` by `additional`.
> >      ///
> >      /// # Safety
> >      ///
> > -    /// - `new_len` must be less than or equal to [`Self::capacity`].
> > -    /// - If `new_len` is greater than `self.len`, all elements within the interval
> > -    ///   [`self.len`,`new_len`) must be initialized.
> > +    /// - `self.len + additional` must be less than or equal to [`Self::capacity`].
> > +    /// - All elements within the interval [`self.len`,`self.len + additional`) must be initialized.
> >      #[inline]
> > -    pub unsafe fn set_len(&mut self, new_len: usize) {
> > -        debug_assert!(new_len <= self.capacity());
> > -        self.len = new_len;
> > +    pub unsafe fn inc_len(&mut self, additional: usize) {
> > +        debug_assert!(self.len() + additional <= self.capacity());
> > +        self.len += additional;
> 
> I guess we could use an INVARIANT: comment here.

Yeah, I fixed that up in a separate patch. I'm fine with Tamir picking it up or
doing it himself in a new one, etc. But I think this patch should only focus on
the rename.

Re: [PATCH 2/2] rust: alloc: add `Vec::dec_len`

[Danilo Krummrich]

On Sun, Mar 16, 2025 at 07:27:22PM -0400, Tamir Duberstein wrote:
>
> Does this suit?

I think for dec_ref() it is not the safety requrement that justifies the
invariant.

I think it should be something along the lines of:

	// INVARIANT: We drop ownership for all elements within the range
	// `[self.len - count, self.len]`, hence the updated value of `set.len`
	// represents the exact number of elements stored within `self`.

> 
> >         // INVARIANT: By the safety requirements of this method `self.len - count` represents the
> >         // exact number of elements stored within `self`.

Please do not use the email quote mechanism for code snippets, it's confusing
for readers to figure out by whom it has been written.

Re: [PATCH 1/2] rust: alloc: replace `Vec::set_len` with `inc_len`

[Tamir Duberstein]

On Mon, Mar 17, 2025 at 6:48 AM Alice Ryhl <aliceryhl@google.com> wrote:
>
> On Mon, Mar 17, 2025 at 09:58:35AM +0000, Benno Lossin wrote:
> > On Sun Mar 16, 2025 at 11:32 PM CET, Tamir Duberstein wrote:
> > > Rename `set_len` to `inc_len` and simplify its safety contract.
> > > ---
> > >  rust/kernel/alloc/kvec.rs | 19 +++++++++----------
> > >  rust/kernel/str.rs        |  2 +-
> > >  rust/kernel/uaccess.rs    |  2 +-
> > >  3 files changed, 11 insertions(+), 12 deletions(-)
> > >
> > > diff --git a/rust/kernel/alloc/kvec.rs b/rust/kernel/alloc/kvec.rs
> > > index ae9d072741ce..d43a1d609434 100644
> > > --- a/rust/kernel/alloc/kvec.rs
> > > +++ b/rust/kernel/alloc/kvec.rs
> > > @@ -183,17 +183,16 @@ pub fn len(&self) -> usize {
> > >          self.len
> > >      }
> > >
> > > -    /// Forcefully sets `self.len` to `new_len`.
> > > +    /// Increments `self.len` by `additional`.
> >
> > I would keep the "Forcefully".

Why? Is it possible to set it any other way?

> >
> > >      ///
> > >      /// # Safety
> > >      ///
> > > -    /// - `new_len` must be less than or equal to [`Self::capacity`].
> > > -    /// - If `new_len` is greater than `self.len`, all elements within the interval
> > > -    ///   [`self.len`,`new_len`) must be initialized.
> > > +    /// - `self.len + additional` must be less than or equal to [`Self::capacity`].
> > > +    /// - All elements within the interval [`self.len`,`self.len + additional`) must be initialized.
> > >      #[inline]
> > > -    pub unsafe fn set_len(&mut self, new_len: usize) {
> > > -        debug_assert!(new_len <= self.capacity());
> > > -        self.len = new_len;
> > > +    pub unsafe fn inc_len(&mut self, additional: usize) {
> > > +        debug_assert!(self.len() + additional <= self.capacity());
> >
> > What if this overflows? Do we always have overflow debugging on when
> > debug assertions are enabled? If yes, then this is fine.
>
> I don't think we do.

Rearranged as

        debug_assert!(additional <= self.capacity() - self.len());

It should be impossible for this to underflow because capacity must be
>= len. Interestingly this isn't a documented invariant, but it is
relied upon by `spare_capacity_mut`.

> > > diff --git a/rust/kernel/str.rs b/rust/kernel/str.rs
> > > index 28e2201604d6..005713839e9e 100644
> > > --- a/rust/kernel/str.rs
> > > +++ b/rust/kernel/str.rs
> > > @@ -840,7 +840,7 @@ pub fn try_from_fmt(args: fmt::Arguments<'_>) -> Result<Self, Error> {
> > >
> > >          // SAFETY: The number of bytes that can be written to `f` is bounded by `size`, which is
> > >          // `buf`'s capacity. The contents of the buffer have been initialised by writes to `f`.
> > > -        unsafe { buf.set_len(f.bytes_written()) };
> > > +        unsafe { buf.inc_len(f.bytes_written()) };
> >
> > This change seems wrong unless the code was wrong to begin with.
> >
> > Otherwise the change looks good.
>
> The buffer has length zero as it was just created with:
>
> let mut buf = KVec::with_capacity(size, GFP_KERNEL)?;

Indeed. Added to the commit message.

Re: [PATCH 1/2] rust: alloc: replace `Vec::set_len` with `inc_len`

[Tamir Duberstein]

On Mon, Mar 17, 2025 at 6:50 AM Alice Ryhl <aliceryhl@google.com> wrote:
>
> On Sun, Mar 16, 2025 at 06:32:00PM -0400, Tamir Duberstein wrote:
> > Rename `set_len` to `inc_len` and simplify its safety contract.
>
> You're missing a Signed-off-by tag.

Thanks, added.

Re: [PATCH 2/2] rust: alloc: add `Vec::dec_len`

[Tamir Duberstein]

On Mon, Mar 17, 2025 at 6:04 AM Benno Lossin <benno.lossin@proton.me> wrote:
>
> On Sun Mar 16, 2025 at 11:32 PM CET, Tamir Duberstein wrote:
> > Add `Vec::dec_len` that reduces the length of the receiver. This method
> > is intended to be used from methods that remove elements from `Vec` such
> > as `truncate`, `pop`, `remove`, and others. This method is intentionally
> > not `pub`.
>
> I think it should be `pub`. Otherwise we're loosing functionality
> compared to now. If one decides to give the raw pointer to some C API
> that takes ownership of the pointer, then I want them to be able to call
> `dec_len` manually.

This is premature. It is trivial to make this function pub when the need arises.

>
> >
> > Signed-off-by: Tamir Duberstein <tamird@gmail.com>
> > ---
> >  rust/kernel/alloc/kvec.rs | 15 +++++++++++++++
> >  1 file changed, 15 insertions(+)
> >
> > diff --git a/rust/kernel/alloc/kvec.rs b/rust/kernel/alloc/kvec.rs
> > index d43a1d609434..5d604e04b9a5 100644
> > --- a/rust/kernel/alloc/kvec.rs
> > +++ b/rust/kernel/alloc/kvec.rs
> > @@ -195,6 +195,21 @@ pub unsafe fn inc_len(&mut self, additional: usize) {
> >          self.len += additional;
> >      }
> >
> > +    /// Decreases `self.len` by `count`.
> > +    ///
> > +    /// Returns a mutable reference to the removed elements.
>
> s/reference/slice/
>
> I would also mention here that the elements won't be dropped when the
> user doesn't do that manually using the slice. So explain that this is a
> low-level operation and `clear` or `truncate` should be used instead
> where possible.

Neither function exists. I've added a description of the semantics of the slice.

> > +    ///
> > +    /// # Safety
> > +    ///
> > +    /// - `count` must be less than or equal to `self.len`.
>
> I also think that we should use saturating_sub instead and then not have
> to worry about this. (It should still be documented in the function
> though). That way this can also be a safe function.

This doesn't seem better to me. I'd prefer to have more rather than
fewer guardrails on such low-level operations.

Re: [PATCH 2/2] rust: alloc: add `Vec::dec_len`

[Tamir Duberstein]

On Mon, Mar 17, 2025 at 7:22 AM Danilo Krummrich <dakr@kernel.org> wrote:
>
> On Sun, Mar 16, 2025 at 07:27:22PM -0400, Tamir Duberstein wrote:
> >
> > Does this suit?
>
> I think for dec_ref() it is not the safety requrement that justifies the
> invariant.
>
> I think it should be something along the lines of:
>
>         // INVARIANT: We drop ownership for all elements within the range
>         // `[self.len - count, self.len]`, hence the updated value of `set.len`
>         // represents the exact number of elements stored within `self`.
>
> >
> > >         // INVARIANT: By the safety requirements of this method `self.len - count` represents the
> > >         // exact number of elements stored within `self`.
>
> Please do not use the email quote mechanism for code snippets, it's confusing
> for readers to figure out by whom it has been written.

Thanks, applied the suggestion.

Re: [PATCH 2/2] rust: alloc: add `Vec::dec_len`

[Alice Ryhl]

On Mon, Mar 17, 2025 at 07:34:44AM -0400, Tamir Duberstein wrote:
> On Mon, Mar 17, 2025 at 6:04 AM Benno Lossin <benno.lossin@proton.me> wrote:
> >
> > On Sun Mar 16, 2025 at 11:32 PM CET, Tamir Duberstein wrote:
> > > Add `Vec::dec_len` that reduces the length of the receiver. This method
> > > is intended to be used from methods that remove elements from `Vec` such
> > > as `truncate`, `pop`, `remove`, and others. This method is intentionally
> > > not `pub`.
> >
> > I think it should be `pub`. Otherwise we're loosing functionality
> > compared to now. If one decides to give the raw pointer to some C API
> > that takes ownership of the pointer, then I want them to be able to call
> > `dec_len` manually.
> 
> This is premature. It is trivial to make this function pub when the need arises.

Normally I'd agree with Benno, but in this case I think having it
private is preferable. The function is safe, so it's too easy for
end-users to confuse it with truncate.

Alice

Re: [PATCH 2/2] rust: alloc: add `Vec::dec_len`

[Alice Ryhl]

On Mon, Mar 17, 2025 at 11:47:50AM +0000, Alice Ryhl wrote:
> On Mon, Mar 17, 2025 at 07:34:44AM -0400, Tamir Duberstein wrote:
> > On Mon, Mar 17, 2025 at 6:04 AM Benno Lossin <benno.lossin@proton.me> wrote:
> > >
> > > On Sun Mar 16, 2025 at 11:32 PM CET, Tamir Duberstein wrote:
> > > > Add `Vec::dec_len` that reduces the length of the receiver. This method
> > > > is intended to be used from methods that remove elements from `Vec` such
> > > > as `truncate`, `pop`, `remove`, and others. This method is intentionally
> > > > not `pub`.
> > >
> > > I think it should be `pub`. Otherwise we're loosing functionality
> > > compared to now. If one decides to give the raw pointer to some C API
> > > that takes ownership of the pointer, then I want them to be able to call
> > > `dec_len` manually.
> > 
> > This is premature. It is trivial to make this function pub when the need arises.
> 
> Normally I'd agree with Benno, but in this case I think having it
> private is preferable. The function is safe, so it's too easy for
> end-users to confuse it with truncate.

Thinking more about this ... I think we should have `set_len` and
`inc_len` instead. That way, both methods are unsafe so people will not
accidentally use `set_len` when they meant to use `truncate`.

Alice

Re: [PATCH 2/2] rust: alloc: add `Vec::dec_len`

[Tamir Duberstein]

On Mon, Mar 17, 2025 at 8:59 AM Alice Ryhl <aliceryhl@google.com> wrote:
>
> On Mon, Mar 17, 2025 at 11:47:50AM +0000, Alice Ryhl wrote:
> > On Mon, Mar 17, 2025 at 07:34:44AM -0400, Tamir Duberstein wrote:
> > > On Mon, Mar 17, 2025 at 6:04 AM Benno Lossin <benno.lossin@proton.me> wrote:
> > > >
> > > > On Sun Mar 16, 2025 at 11:32 PM CET, Tamir Duberstein wrote:
> > > > > Add `Vec::dec_len` that reduces the length of the receiver. This method
> > > > > is intended to be used from methods that remove elements from `Vec` such
> > > > > as `truncate`, `pop`, `remove`, and others. This method is intentionally
> > > > > not `pub`.
> > > >
> > > > I think it should be `pub`. Otherwise we're loosing functionality
> > > > compared to now. If one decides to give the raw pointer to some C API
> > > > that takes ownership of the pointer, then I want them to be able to call
> > > > `dec_len` manually.
> > >
> > > This is premature. It is trivial to make this function pub when the need arises.
> >
> > Normally I'd agree with Benno, but in this case I think having it
> > private is preferable. The function is safe, so it's too easy for
> > end-users to confuse it with truncate.
>
> Thinking more about this ... I think we should have `set_len` and
> `inc_len` instead. That way, both methods are unsafe so people will not
> accidentally use `set_len` when they meant to use `truncate`.
>
> Alice

Isn't it fine to keep this function unsafe given that it can break
invariants in its current form? As expressed earlier, I would prefer
not to make it safe by using saturating_sub.

Re: [PATCH 2/2] rust: alloc: add `Vec::dec_len`

[Benno Lossin]

On Mon Mar 17, 2025 at 12:34 PM CET, Tamir Duberstein wrote:
> On Mon, Mar 17, 2025 at 6:04 AM Benno Lossin <benno.lossin@proton.me> wrote:
>>
>> On Sun Mar 16, 2025 at 11:32 PM CET, Tamir Duberstein wrote:
>> > Add `Vec::dec_len` that reduces the length of the receiver. This method
>> > is intended to be used from methods that remove elements from `Vec` such
>> > as `truncate`, `pop`, `remove`, and others. This method is intentionally
>> > not `pub`.
>>
>> I think it should be `pub`. Otherwise we're loosing functionality
>> compared to now. If one decides to give the raw pointer to some C API
>> that takes ownership of the pointer, then I want them to be able to call
>> `dec_len` manually.
>
> This is premature. It is trivial to make this function pub when the need arises.

And it's trivial to do it now. If it's private now, someone will have to
change this in some random patch and it's annoying.

>> > Signed-off-by: Tamir Duberstein <tamird@gmail.com>
>> > ---
>> >  rust/kernel/alloc/kvec.rs | 15 +++++++++++++++
>> >  1 file changed, 15 insertions(+)
>> >
>> > diff --git a/rust/kernel/alloc/kvec.rs b/rust/kernel/alloc/kvec.rs
>> > index d43a1d609434..5d604e04b9a5 100644
>> > --- a/rust/kernel/alloc/kvec.rs
>> > +++ b/rust/kernel/alloc/kvec.rs
>> > @@ -195,6 +195,21 @@ pub unsafe fn inc_len(&mut self, additional: usize) {
>> >          self.len += additional;
>> >      }
>> >
>> > +    /// Decreases `self.len` by `count`.
>> > +    ///
>> > +    /// Returns a mutable reference to the removed elements.
>>
>> s/reference/slice/
>>
>> I would also mention here that the elements won't be dropped when the
>> user doesn't do that manually using the slice. So explain that this is a
>> low-level operation and `clear` or `truncate` should be used instead
>> where possible.
>
> Neither function exists. I've added a description of the semantics of the slice.

Fair point, would still be nice to point users to these when they exist.

>> > +    ///
>> > +    /// # Safety
>> > +    ///
>> > +    /// - `count` must be less than or equal to `self.len`.
>>
>> I also think that we should use saturating_sub instead and then not have
>> to worry about this. (It should still be documented in the function
>> though). That way this can also be a safe function.
>
> This doesn't seem better to me. I'd prefer to have more rather than
> fewer guardrails on such low-level operations.

Your second sentence seems like an argument for making it safe? I think
it's a lot better as a safe function.

---
Cheers,
Benno

Re: [PATCH 2/2] rust: alloc: add `Vec::dec_len`

[Benno Lossin]

On Mon Mar 17, 2025 at 1:59 PM CET, Alice Ryhl wrote:
> On Mon, Mar 17, 2025 at 11:47:50AM +0000, Alice Ryhl wrote:
>> On Mon, Mar 17, 2025 at 07:34:44AM -0400, Tamir Duberstein wrote:
>> > On Mon, Mar 17, 2025 at 6:04 AM Benno Lossin <benno.lossin@proton.me> wrote:
>> > >
>> > > On Sun Mar 16, 2025 at 11:32 PM CET, Tamir Duberstein wrote:
>> > > > Add `Vec::dec_len` that reduces the length of the receiver. This method
>> > > > is intended to be used from methods that remove elements from `Vec` such
>> > > > as `truncate`, `pop`, `remove`, and others. This method is intentionally
>> > > > not `pub`.
>> > >
>> > > I think it should be `pub`. Otherwise we're loosing functionality
>> > > compared to now. If one decides to give the raw pointer to some C API
>> > > that takes ownership of the pointer, then I want them to be able to call
>> > > `dec_len` manually.
>> > 
>> > This is premature. It is trivial to make this function pub when the need arises.
>> 
>> Normally I'd agree with Benno, but in this case I think having it
>> private is preferable. The function is safe, so it's too easy for
>> end-users to confuse it with truncate.
>
> Thinking more about this ... I think we should have `set_len` and
> `inc_len` instead. That way, both methods are unsafe so people will not
> accidentally use `set_len` when they meant to use `truncate`.

I agree for this on the public API. The way I usually saw `set_len`
being used for decrementing was truncation without dropping the old
values. And that is going to be `vec.dec_len(vec.len())` with the
current design. `vec.set_len(0);` is much clearer in that respect.

But for the internals, I'd say that `dec_len` is nicer, so for `pop` one
would then use `self.dec_len(1)`.

How about we keep `set_len` and make `dec_len` a private, safe helper?

---
Cheers,
Benno

Re: [PATCH 1/2] rust: alloc: replace `Vec::set_len` with `inc_len`

[Benno Lossin]

On Mon Mar 17, 2025 at 11:23 AM CET, Miguel Ojeda wrote:
> On Mon, Mar 17, 2025 at 10:58 AM Benno Lossin <benno.lossin@proton.me> wrote:
>>
>> What if this overflows? Do we always have overflow debugging on when
>> debug assertions are enabled? If yes, then this is fine.
>
> No, they are independent settings.

I see, then this needs to be `checked_add` or some avoid the overflow
some different way.

---
Cheers,
Benno

Re: [PATCH 2/2] rust: alloc: add `Vec::dec_len`

[Tamir Duberstein]

On Mon, Mar 17, 2025 at 10:42 AM Benno Lossin <benno.lossin@proton.me> wrote:
>
> On Mon Mar 17, 2025 at 1:59 PM CET, Alice Ryhl wrote:
> > On Mon, Mar 17, 2025 at 11:47:50AM +0000, Alice Ryhl wrote:
> >> On Mon, Mar 17, 2025 at 07:34:44AM -0400, Tamir Duberstein wrote:
> >> > On Mon, Mar 17, 2025 at 6:04 AM Benno Lossin <benno.lossin@proton.me> wrote:
> >> > >
> >> > > On Sun Mar 16, 2025 at 11:32 PM CET, Tamir Duberstein wrote:
> >> > > > Add `Vec::dec_len` that reduces the length of the receiver. This method
> >> > > > is intended to be used from methods that remove elements from `Vec` such
> >> > > > as `truncate`, `pop`, `remove`, and others. This method is intentionally
> >> > > > not `pub`.
> >> > >
> >> > > I think it should be `pub`. Otherwise we're loosing functionality
> >> > > compared to now. If one decides to give the raw pointer to some C API
> >> > > that takes ownership of the pointer, then I want them to be able to call
> >> > > `dec_len` manually.
> >> >
> >> > This is premature. It is trivial to make this function pub when the need arises.
> >>
> >> Normally I'd agree with Benno, but in this case I think having it
> >> private is preferable. The function is safe, so it's too easy for
> >> end-users to confuse it with truncate.
> >
> > Thinking more about this ... I think we should have `set_len` and
> > `inc_len` instead. That way, both methods are unsafe so people will not
> > accidentally use `set_len` when they meant to use `truncate`.
>
> I agree for this on the public API. The way I usually saw `set_len`
> being used for decrementing was truncation without dropping the old
> values. And that is going to be `vec.dec_len(vec.len())` with the
> current design. `vec.set_len(0);` is much clearer in that respect.
>
> But for the internals, I'd say that `dec_len` is nicer, so for `pop` one
> would then use `self.dec_len(1)`.
>
> How about we keep `set_len` and make `dec_len` a private, safe helper?

This discussion is _way_ too speculative for my taste. If you'd like
to do this kind of thing, I'm happy to drop this patch or the series.
I'm not comfortable adding API whose usage I haven't seen and don't
understand.

Re: [PATCH 1/2] rust: alloc: replace `Vec::set_len` with `inc_len`

[Benno Lossin]

On Mon Mar 17, 2025 at 12:25 PM CET, Tamir Duberstein wrote:
> On Mon, Mar 17, 2025 at 6:48 AM Alice Ryhl <aliceryhl@google.com> wrote:
>>
>> On Mon, Mar 17, 2025 at 09:58:35AM +0000, Benno Lossin wrote:
>> > On Sun Mar 16, 2025 at 11:32 PM CET, Tamir Duberstein wrote:
>> > > Rename `set_len` to `inc_len` and simplify its safety contract.
>> > > ---
>> > >  rust/kernel/alloc/kvec.rs | 19 +++++++++----------
>> > >  rust/kernel/str.rs        |  2 +-
>> > >  rust/kernel/uaccess.rs    |  2 +-
>> > >  3 files changed, 11 insertions(+), 12 deletions(-)
>> > >
>> > > diff --git a/rust/kernel/alloc/kvec.rs b/rust/kernel/alloc/kvec.rs
>> > > index ae9d072741ce..d43a1d609434 100644
>> > > --- a/rust/kernel/alloc/kvec.rs
>> > > +++ b/rust/kernel/alloc/kvec.rs
>> > > @@ -183,17 +183,16 @@ pub fn len(&self) -> usize {
>> > >          self.len
>> > >      }
>> > >
>> > > -    /// Forcefully sets `self.len` to `new_len`.
>> > > +    /// Increments `self.len` by `additional`.
>> >
>> > I would keep the "Forcefully".
>
> Why? Is it possible to set it any other way?

Yeah when `truncate` and `resize` land. It conveys that this is a
low-level operation.

>> > >      ///
>> > >      /// # Safety
>> > >      ///
>> > > -    /// - `new_len` must be less than or equal to [`Self::capacity`].
>> > > -    /// - If `new_len` is greater than `self.len`, all elements within the interval
>> > > -    ///   [`self.len`,`new_len`) must be initialized.
>> > > +    /// - `self.len + additional` must be less than or equal to [`Self::capacity`].
>> > > +    /// - All elements within the interval [`self.len`,`self.len + additional`) must be initialized.
>> > >      #[inline]
>> > > -    pub unsafe fn set_len(&mut self, new_len: usize) {
>> > > -        debug_assert!(new_len <= self.capacity());
>> > > -        self.len = new_len;
>> > > +    pub unsafe fn inc_len(&mut self, additional: usize) {
>> > > +        debug_assert!(self.len() + additional <= self.capacity());
>> >
>> > What if this overflows? Do we always have overflow debugging on when
>> > debug assertions are enabled? If yes, then this is fine.
>>
>> I don't think we do.
>
> Rearranged as
>
>         debug_assert!(additional <= self.capacity() - self.len());

LGTM

> It should be impossible for this to underflow because capacity must be
>>= len. Interestingly this isn't a documented invariant, but it is
> relied upon by `spare_capacity_mut`.

Oh yeah that should be an invariant. Feel free to open an issue or send
a patch.

>> > > diff --git a/rust/kernel/str.rs b/rust/kernel/str.rs
>> > > index 28e2201604d6..005713839e9e 100644
>> > > --- a/rust/kernel/str.rs
>> > > +++ b/rust/kernel/str.rs
>> > > @@ -840,7 +840,7 @@ pub fn try_from_fmt(args: fmt::Arguments<'_>) -> Result<Self, Error> {
>> > >
>> > >          // SAFETY: The number of bytes that can be written to `f` is bounded by `size`, which is
>> > >          // `buf`'s capacity. The contents of the buffer have been initialised by writes to `f`.
>> > > -        unsafe { buf.set_len(f.bytes_written()) };
>> > > +        unsafe { buf.inc_len(f.bytes_written()) };
>> >
>> > This change seems wrong unless the code was wrong to begin with.
>> >
>> > Otherwise the change looks good.
>>
>> The buffer has length zero as it was just created with:
>>
>> let mut buf = KVec::with_capacity(size, GFP_KERNEL)?;

Ahh, I didn't look at the context. Makes sense.

> Indeed. Added to the commit message.

Thanks.

---
Cheers,
Benno

Re: [PATCH 1/2] rust: alloc: replace `Vec::set_len` with `inc_len`

[Tamir Duberstein]

On Mon, Mar 17, 2025 at 10:46 AM Benno Lossin <benno.lossin@proton.me> wrote:
>
> On Mon Mar 17, 2025 at 12:25 PM CET, Tamir Duberstein wrote:
> > On Mon, Mar 17, 2025 at 6:48 AM Alice Ryhl <aliceryhl@google.com> wrote:
> >>
> >> On Mon, Mar 17, 2025 at 09:58:35AM +0000, Benno Lossin wrote:
> >> > On Sun Mar 16, 2025 at 11:32 PM CET, Tamir Duberstein wrote:
> >> > > Rename `set_len` to `inc_len` and simplify its safety contract.
> >> > > ---
> >> > >  rust/kernel/alloc/kvec.rs | 19 +++++++++----------
> >> > >  rust/kernel/str.rs        |  2 +-
> >> > >  rust/kernel/uaccess.rs    |  2 +-
> >> > >  3 files changed, 11 insertions(+), 12 deletions(-)
> >> > >
> >> > > diff --git a/rust/kernel/alloc/kvec.rs b/rust/kernel/alloc/kvec.rs
> >> > > index ae9d072741ce..d43a1d609434 100644
> >> > > --- a/rust/kernel/alloc/kvec.rs
> >> > > +++ b/rust/kernel/alloc/kvec.rs
> >> > > @@ -183,17 +183,16 @@ pub fn len(&self) -> usize {
> >> > >          self.len
> >> > >      }
> >> > >
> >> > > -    /// Forcefully sets `self.len` to `new_len`.
> >> > > +    /// Increments `self.len` by `additional`.
> >> >
> >> > I would keep the "Forcefully".
> >
> > Why? Is it possible to set it any other way?
>
> Yeah when `truncate` and `resize` land. It conveys that this is a
> low-level operation.

It's also an unsafe function whose safety section mentions that the
memory must already be initialized. I don't think this word adds any
value.

>
> >> > >      ///
> >> > >      /// # Safety
> >> > >      ///
> >> > > -    /// - `new_len` must be less than or equal to [`Self::capacity`].
> >> > > -    /// - If `new_len` is greater than `self.len`, all elements within the interval
> >> > > -    ///   [`self.len`,`new_len`) must be initialized.
> >> > > +    /// - `self.len + additional` must be less than or equal to [`Self::capacity`].
> >> > > +    /// - All elements within the interval [`self.len`,`self.len + additional`) must be initialized.
> >> > >      #[inline]
> >> > > -    pub unsafe fn set_len(&mut self, new_len: usize) {
> >> > > -        debug_assert!(new_len <= self.capacity());
> >> > > -        self.len = new_len;
> >> > > +    pub unsafe fn inc_len(&mut self, additional: usize) {
> >> > > +        debug_assert!(self.len() + additional <= self.capacity());
> >> >
> >> > What if this overflows? Do we always have overflow debugging on when
> >> > debug assertions are enabled? If yes, then this is fine.
> >>
> >> I don't think we do.
> >
> > Rearranged as
> >
> >         debug_assert!(additional <= self.capacity() - self.len());
>
> LGTM
>
> > It should be impossible for this to underflow because capacity must be
> >>= len. Interestingly this isn't a documented invariant, but it is
> > relied upon by `spare_capacity_mut`.
>
> Oh yeah that should be an invariant. Feel free to open an issue or send
> a patch.

Will prepend a patch in this series.

Re: [PATCH 2/2] rust: alloc: add `Vec::dec_len`

[Tamir Duberstein]

On Mon, Mar 17, 2025 at 10:39 AM Benno Lossin <benno.lossin@proton.me> wrote:
>
> On Mon Mar 17, 2025 at 12:34 PM CET, Tamir Duberstein wrote:
> > On Mon, Mar 17, 2025 at 6:04 AM Benno Lossin <benno.lossin@proton.me> wrote:
> >>
> >> On Sun Mar 16, 2025 at 11:32 PM CET, Tamir Duberstein wrote:
> >> > Add `Vec::dec_len` that reduces the length of the receiver. This method
> >> > is intended to be used from methods that remove elements from `Vec` such
> >> > as `truncate`, `pop`, `remove`, and others. This method is intentionally
> >> > not `pub`.
> >>
> >> I think it should be `pub`. Otherwise we're loosing functionality
> >> compared to now. If one decides to give the raw pointer to some C API
> >> that takes ownership of the pointer, then I want them to be able to call
> >> `dec_len` manually.
> >
> > This is premature. It is trivial to make this function pub when the need arises.
>
> And it's trivial to do it now. If it's private now, someone will have to
> change this in some random patch and it's annoying.

It is my understanding that the kernel's policy is in general not to
add API surface that doesn't have users. Rust-for-Linux of course
often doesn't honor this by necessity, since many abstractions are
needed before users (drivers) can be upstream. But in this case we
can't even mention a specific use case - so as I mentioned on the
previous reply, I am not comfortable putting my name on such an API.

> >> > +    ///
> >> > +    /// # Safety
> >> > +    ///
> >> > +    /// - `count` must be less than or equal to `self.len`.
> >>
> >> I also think that we should use saturating_sub instead and then not have
> >> to worry about this. (It should still be documented in the function
> >> though). That way this can also be a safe function.
> >
> > This doesn't seem better to me. I'd prefer to have more rather than
> > fewer guardrails on such low-level operations.
>
> Your second sentence seems like an argument for making it safe? I think
> it's a lot better as a safe function.

The guardrail I was referring to is the requirement that the caller
write a safety comment.

Re: [PATCH 2/2] rust: alloc: add `Vec::dec_len`

[Miguel Ojeda]

On Mon, Mar 17, 2025 at 4:38 PM Tamir Duberstein <tamird@gmail.com> wrote:
>
> It is my understanding that the kernel's policy is in general not to
> add API surface that doesn't have users. Rust-for-Linux of course
> often doesn't honor this by necessity, since many abstractions are
> needed before users (drivers) can be upstream. But in this case we
> can't even mention a specific use case - so as I mentioned on the
> previous reply, I am not comfortable putting my name on such an API.

To clarify: as long as the future user is known and agreed upon, it is
fine, i.e. what cannot be done, and we do honor it, is to add things
that have no user in sight at all.

From time to time, but not really often at all, we have added things
that will obviously get users eventually even if there is currently no
one. For instance, all the `pr_*` levels, even if we do not have a
caller yet for some of them (in that case, because it is simpler to
add all at once instead of asking for reviews several times for
essentially the same code).

Cheers,
Miguel

Re: [PATCH 2/2] rust: alloc: add `Vec::dec_len`

[Danilo Krummrich]

On Mon, Mar 17, 2025 at 10:44:25AM -0400, Tamir Duberstein wrote:
> On Mon, Mar 17, 2025 at 10:42 AM Benno Lossin <benno.lossin@proton.me> wrote:
> >
> > On Mon Mar 17, 2025 at 1:59 PM CET, Alice Ryhl wrote:
> > > On Mon, Mar 17, 2025 at 11:47:50AM +0000, Alice Ryhl wrote:
> > >> On Mon, Mar 17, 2025 at 07:34:44AM -0400, Tamir Duberstein wrote:
> > >> > On Mon, Mar 17, 2025 at 6:04 AM Benno Lossin <benno.lossin@proton.me> wrote:
> > >> > >
> > >> > > On Sun Mar 16, 2025 at 11:32 PM CET, Tamir Duberstein wrote:
> > >> > > > Add `Vec::dec_len` that reduces the length of the receiver. This method
> > >> > > > is intended to be used from methods that remove elements from `Vec` such
> > >> > > > as `truncate`, `pop`, `remove`, and others. This method is intentionally
> > >> > > > not `pub`.
> > >> > >
> > >> > > I think it should be `pub`. Otherwise we're loosing functionality
> > >> > > compared to now. If one decides to give the raw pointer to some C API
> > >> > > that takes ownership of the pointer, then I want them to be able to call
> > >> > > `dec_len` manually.
> > >> >
> > >> > This is premature. It is trivial to make this function pub when the need arises.
> > >>
> > >> Normally I'd agree with Benno, but in this case I think having it
> > >> private is preferable. The function is safe, so it's too easy for
> > >> end-users to confuse it with truncate.
> > >
> > > Thinking more about this ... I think we should have `set_len` and
> > > `inc_len` instead. That way, both methods are unsafe so people will not
> > > accidentally use `set_len` when they meant to use `truncate`.
> >
> > I agree for this on the public API. The way I usually saw `set_len`
> > being used for decrementing was truncation without dropping the old
> > values. And that is going to be `vec.dec_len(vec.len())` with the
> > current design. `vec.set_len(0);` is much clearer in that respect.
> >
> > But for the internals, I'd say that `dec_len` is nicer, so for `pop` one
> > would then use `self.dec_len(1)`.
> >
> > How about we keep `set_len` and make `dec_len` a private, safe helper?
> 
> This discussion is _way_ too speculative for my taste. If you'd like
> to do this kind of thing, I'm happy to drop this patch or the series.
> I'm not comfortable adding API whose usage I haven't seen and don't
> understand.

Seems like setting the length of a vector is a hard thing to do. :)

I advocate for a middle ground.

(1) Let's keep dec_len() a private and safe helper, it clearly improves the
internals.

(2) Introduce set_len() as a public API and defer the question on how to support
dec_len() in a public API once the need arises.

Re: [PATCH 2/2] rust: alloc: add `Vec::dec_len`

[Tamir Duberstein]

On Mon, Mar 17, 2025 at 12:17 PM Danilo Krummrich <dakr@kernel.org> wrote:
>
> On Mon, Mar 17, 2025 at 10:44:25AM -0400, Tamir Duberstein wrote:
> > On Mon, Mar 17, 2025 at 10:42 AM Benno Lossin <benno.lossin@proton.me> wrote:
> > >
> > > On Mon Mar 17, 2025 at 1:59 PM CET, Alice Ryhl wrote:
> > > > On Mon, Mar 17, 2025 at 11:47:50AM +0000, Alice Ryhl wrote:
> > > >> On Mon, Mar 17, 2025 at 07:34:44AM -0400, Tamir Duberstein wrote:
> > > >> > On Mon, Mar 17, 2025 at 6:04 AM Benno Lossin <benno.lossin@proton.me> wrote:
> > > >> > >
> > > >> > > On Sun Mar 16, 2025 at 11:32 PM CET, Tamir Duberstein wrote:
> > > >> > > > Add `Vec::dec_len` that reduces the length of the receiver. This method
> > > >> > > > is intended to be used from methods that remove elements from `Vec` such
> > > >> > > > as `truncate`, `pop`, `remove`, and others. This method is intentionally
> > > >> > > > not `pub`.
> > > >> > >
> > > >> > > I think it should be `pub`. Otherwise we're loosing functionality
> > > >> > > compared to now. If one decides to give the raw pointer to some C API
> > > >> > > that takes ownership of the pointer, then I want them to be able to call
> > > >> > > `dec_len` manually.
> > > >> >
> > > >> > This is premature. It is trivial to make this function pub when the need arises.
> > > >>
> > > >> Normally I'd agree with Benno, but in this case I think having it
> > > >> private is preferable. The function is safe, so it's too easy for
> > > >> end-users to confuse it with truncate.
> > > >
> > > > Thinking more about this ... I think we should have `set_len` and
> > > > `inc_len` instead. That way, both methods are unsafe so people will not
> > > > accidentally use `set_len` when they meant to use `truncate`.
> > >
> > > I agree for this on the public API. The way I usually saw `set_len`
> > > being used for decrementing was truncation without dropping the old
> > > values. And that is going to be `vec.dec_len(vec.len())` with the
> > > current design. `vec.set_len(0);` is much clearer in that respect.
> > >
> > > But for the internals, I'd say that `dec_len` is nicer, so for `pop` one
> > > would then use `self.dec_len(1)`.
> > >
> > > How about we keep `set_len` and make `dec_len` a private, safe helper?
> >
> > This discussion is _way_ too speculative for my taste. If you'd like
> > to do this kind of thing, I'm happy to drop this patch or the series.
> > I'm not comfortable adding API whose usage I haven't seen and don't
> > understand.
>
> Seems like setting the length of a vector is a hard thing to do. :)
>
> I advocate for a middle ground.
>
> (1) Let's keep dec_len() a private and safe helper, it clearly improves the
> internals.

I don't agree with making it safe by using saturating_sub. I prefer
that the caller must justify that they aren't calling it with count >
self.len().

>
> (2) Introduce set_len() as a public API and defer the question on how to support
> dec_len() in a public API once the need arises.

Do we have line of sight on a public caller?

Re: [PATCH 2/2] rust: alloc: add `Vec::dec_len`

[Benno Lossin]

On Mon Mar 17, 2025 at 4:37 PM CET, Tamir Duberstein wrote:
> On Mon, Mar 17, 2025 at 10:39 AM Benno Lossin <benno.lossin@proton.me> wrote:
>> On Mon Mar 17, 2025 at 12:34 PM CET, Tamir Duberstein wrote:
>> > On Mon, Mar 17, 2025 at 6:04 AM Benno Lossin <benno.lossin@proton.me> wrote:
>> >> On Sun Mar 16, 2025 at 11:32 PM CET, Tamir Duberstein wrote:
>> >> > +    ///
>> >> > +    /// # Safety
>> >> > +    ///
>> >> > +    /// - `count` must be less than or equal to `self.len`.
>> >>
>> >> I also think that we should use saturating_sub instead and then not have
>> >> to worry about this. (It should still be documented in the function
>> >> though). That way this can also be a safe function.
>> >
>> > This doesn't seem better to me. I'd prefer to have more rather than
>> > fewer guardrails on such low-level operations.
>>
>> Your second sentence seems like an argument for making it safe? I think
>> it's a lot better as a safe function.
>
> The guardrail I was referring to is the requirement that the caller
> write a safety comment.

But saturating_sub is a better guardrail?

---
Cheers,
Benno

Re: [PATCH 2/2] rust: alloc: add `Vec::dec_len`

[Tamir Duberstein]

On Mon, Mar 17, 2025 at 1:25 PM Benno Lossin <benno.lossin@proton.me> wrote:
>
> On Mon Mar 17, 2025 at 4:37 PM CET, Tamir Duberstein wrote:
> > On Mon, Mar 17, 2025 at 10:39 AM Benno Lossin <benno.lossin@proton.me> wrote:
> >> On Mon Mar 17, 2025 at 12:34 PM CET, Tamir Duberstein wrote:
> >> > On Mon, Mar 17, 2025 at 6:04 AM Benno Lossin <benno.lossin@proton.me> wrote:
> >> >> On Sun Mar 16, 2025 at 11:32 PM CET, Tamir Duberstein wrote:
> >> >> > +    ///
> >> >> > +    /// # Safety
> >> >> > +    ///
> >> >> > +    /// - `count` must be less than or equal to `self.len`.
> >> >>
> >> >> I also think that we should use saturating_sub instead and then not have
> >> >> to worry about this. (It should still be documented in the function
> >> >> though). That way this can also be a safe function.
> >> >
> >> > This doesn't seem better to me. I'd prefer to have more rather than
> >> > fewer guardrails on such low-level operations.
> >>
> >> Your second sentence seems like an argument for making it safe? I think
> >> it's a lot better as a safe function.
> >
> > The guardrail I was referring to is the requirement that the caller
> > write a safety comment.
>
> But saturating_sub is a better guardrail?

It's a different kind of guardrail; one that attempts to do something
correct in the presence of incorrect code.

Put another way: do we have line of sight on a caller that wants to
use `dec_len` without already knowing the current length of the
vector?

Re: [PATCH 2/2] rust: alloc: add `Vec::dec_len`

[Alice Ryhl]

On Mon, Mar 17, 2025 at 09:53:04AM -0400, Tamir Duberstein wrote:
> On Mon, Mar 17, 2025 at 8:59 AM Alice Ryhl <aliceryhl@google.com> wrote:
> >
> > On Mon, Mar 17, 2025 at 11:47:50AM +0000, Alice Ryhl wrote:
> > > On Mon, Mar 17, 2025 at 07:34:44AM -0400, Tamir Duberstein wrote:
> > > > On Mon, Mar 17, 2025 at 6:04 AM Benno Lossin <benno.lossin@proton.me> wrote:
> > > > >
> > > > > On Sun Mar 16, 2025 at 11:32 PM CET, Tamir Duberstein wrote:
> > > > > > Add `Vec::dec_len` that reduces the length of the receiver. This method
> > > > > > is intended to be used from methods that remove elements from `Vec` such
> > > > > > as `truncate`, `pop`, `remove`, and others. This method is intentionally
> > > > > > not `pub`.
> > > > >
> > > > > I think it should be `pub`. Otherwise we're loosing functionality
> > > > > compared to now. If one decides to give the raw pointer to some C API
> > > > > that takes ownership of the pointer, then I want them to be able to call
> > > > > `dec_len` manually.
> > > >
> > > > This is premature. It is trivial to make this function pub when the need arises.
> > >
> > > Normally I'd agree with Benno, but in this case I think having it
> > > private is preferable. The function is safe, so it's too easy for
> > > end-users to confuse it with truncate.
> >
> > Thinking more about this ... I think we should have `set_len` and
> > `inc_len` instead. That way, both methods are unsafe so people will not
> > accidentally use `set_len` when they meant to use `truncate`.
> >
> > Alice
> 
> Isn't it fine to keep this function unsafe given that it can break
> invariants in its current form? As expressed earlier, I would prefer
> not to make it safe by using saturating_sub.

I guess that's okay. But I would think that with the exception of
`Vec::pop`, the interface you want for Vec methods that decrease the
length is set_len, not dec_len. That is the case for clear, truncate,
and drain.

Even for the Vec methods that actually use

	set_len(original_len - removed_cnt)

they make this call after having previously called set_len(0).

Alice

Re: [PATCH 2/2] rust: alloc: add `Vec::dec_len`

[Tamir Duberstein]

On Tue, Mar 18, 2025 at 5:30 AM Alice Ryhl <aliceryhl@google.com> wrote:
>
> On Mon, Mar 17, 2025 at 09:53:04AM -0400, Tamir Duberstein wrote:
> > On Mon, Mar 17, 2025 at 8:59 AM Alice Ryhl <aliceryhl@google.com> wrote:
> > >
> > > On Mon, Mar 17, 2025 at 11:47:50AM +0000, Alice Ryhl wrote:
> > > > On Mon, Mar 17, 2025 at 07:34:44AM -0400, Tamir Duberstein wrote:
> > > > > On Mon, Mar 17, 2025 at 6:04 AM Benno Lossin <benno.lossin@proton.me> wrote:
> > > > > >
> > > > > > On Sun Mar 16, 2025 at 11:32 PM CET, Tamir Duberstein wrote:
> > > > > > > Add `Vec::dec_len` that reduces the length of the receiver. This method
> > > > > > > is intended to be used from methods that remove elements from `Vec` such
> > > > > > > as `truncate`, `pop`, `remove`, and others. This method is intentionally
> > > > > > > not `pub`.
> > > > > >
> > > > > > I think it should be `pub`. Otherwise we're loosing functionality
> > > > > > compared to now. If one decides to give the raw pointer to some C API
> > > > > > that takes ownership of the pointer, then I want them to be able to call
> > > > > > `dec_len` manually.
> > > > >
> > > > > This is premature. It is trivial to make this function pub when the need arises.
> > > >
> > > > Normally I'd agree with Benno, but in this case I think having it
> > > > private is preferable. The function is safe, so it's too easy for
> > > > end-users to confuse it with truncate.
> > >
> > > Thinking more about this ... I think we should have `set_len` and
> > > `inc_len` instead. That way, both methods are unsafe so people will not
> > > accidentally use `set_len` when they meant to use `truncate`.
> > >
> > > Alice
> >
> > Isn't it fine to keep this function unsafe given that it can break
> > invariants in its current form? As expressed earlier, I would prefer
> > not to make it safe by using saturating_sub.
>
> I guess that's okay. But I would think that with the exception of
> `Vec::pop`, the interface you want for Vec methods that decrease the
> length is set_len, not dec_len. That is the case for clear, truncate,
> and drain.
>
> Even for the Vec methods that actually use
>
>         set_len(original_len - removed_cnt)
>
> they make this call after having previously called set_len(0).

The methods you're describing are all on Vec, right? In other words,
their usage calls for a private `dec_len` or `set_len`. As I've said
repeatedly in the course of this discussion: I would prefer not to
introduce `dec_len` at all here. It (or `set_len`) can be introduced
in the series that adds truncate or your patch that adds clear, where
its signature can be properly scrutinized in the context of an actual
caller.

Tamir

Re: [PATCH 2/2] rust: alloc: add `Vec::dec_len`

[Alice Ryhl]

On Tue, Mar 18, 2025 at 10:12:28AM -0400, Tamir Duberstein wrote:
> On Tue, Mar 18, 2025 at 5:30 AM Alice Ryhl <aliceryhl@google.com> wrote:
> >
> > On Mon, Mar 17, 2025 at 09:53:04AM -0400, Tamir Duberstein wrote:
> > > On Mon, Mar 17, 2025 at 8:59 AM Alice Ryhl <aliceryhl@google.com> wrote:
> > > >
> > > > On Mon, Mar 17, 2025 at 11:47:50AM +0000, Alice Ryhl wrote:
> > > > > On Mon, Mar 17, 2025 at 07:34:44AM -0400, Tamir Duberstein wrote:
> > > > > > On Mon, Mar 17, 2025 at 6:04 AM Benno Lossin <benno.lossin@proton.me> wrote:
> > > > > > >
> > > > > > > On Sun Mar 16, 2025 at 11:32 PM CET, Tamir Duberstein wrote:
> > > > > > > > Add `Vec::dec_len` that reduces the length of the receiver. This method
> > > > > > > > is intended to be used from methods that remove elements from `Vec` such
> > > > > > > > as `truncate`, `pop`, `remove`, and others. This method is intentionally
> > > > > > > > not `pub`.
> > > > > > >
> > > > > > > I think it should be `pub`. Otherwise we're loosing functionality
> > > > > > > compared to now. If one decides to give the raw pointer to some C API
> > > > > > > that takes ownership of the pointer, then I want them to be able to call
> > > > > > > `dec_len` manually.
> > > > > >
> > > > > > This is premature. It is trivial to make this function pub when the need arises.
> > > > >
> > > > > Normally I'd agree with Benno, but in this case I think having it
> > > > > private is preferable. The function is safe, so it's too easy for
> > > > > end-users to confuse it with truncate.
> > > >
> > > > Thinking more about this ... I think we should have `set_len` and
> > > > `inc_len` instead. That way, both methods are unsafe so people will not
> > > > accidentally use `set_len` when they meant to use `truncate`.
> > > >
> > > > Alice
> > >
> > > Isn't it fine to keep this function unsafe given that it can break
> > > invariants in its current form? As expressed earlier, I would prefer
> > > not to make it safe by using saturating_sub.
> >
> > I guess that's okay. But I would think that with the exception of
> > `Vec::pop`, the interface you want for Vec methods that decrease the
> > length is set_len, not dec_len. That is the case for clear, truncate,
> > and drain.
> >
> > Even for the Vec methods that actually use
> >
> >         set_len(original_len - removed_cnt)
> >
> > they make this call after having previously called set_len(0).
> 
> The methods you're describing are all on Vec, right? In other words,
> their usage calls for a private `dec_len` or `set_len`. As I've said
> repeatedly in the course of this discussion: I would prefer not to
> introduce `dec_len` at all here. It (or `set_len`) can be introduced
> in the series that adds truncate or your patch that adds clear, where
> its signature can be properly scrutinized in the context of an actual
> caller.

Oh I did not see that you said that. Dropping patch 2 is fine with me.

Alice

Re: [PATCH 2/2] rust: alloc: add `Vec::dec_len`

[Tamir Duberstein]

On Tue, Mar 18, 2025 at 10:44 AM Alice Ryhl <aliceryhl@google.com> wrote:
>
> On Tue, Mar 18, 2025 at 10:12:28AM -0400, Tamir Duberstein wrote:\
> >
> > The methods you're describing are all on Vec, right? In other words,
> > their usage calls for a private `dec_len` or `set_len`. As I've said
> > repeatedly in the course of this discussion: I would prefer not to
> > introduce `dec_len` at all here. It (or `set_len`) can be introduced
> > in the series that adds truncate or your patch that adds clear, where
> > its signature can be properly scrutinized in the context of an actual
> > caller.
>
> Oh I did not see that you said that. Dropping patch 2 is fine with me.
>
> Alice

Benno, Danilo: are you both OK with this? I'll discard this patch on
the respin and prepend the patch adding the len <= cap invariant.

Re: [PATCH 2/2] rust: alloc: add `Vec::dec_len`

[Danilo Krummrich]

On Tue, Mar 18, 2025 at 02:28:02PM -0400, Tamir Duberstein wrote:
> On Tue, Mar 18, 2025 at 10:44 AM Alice Ryhl <aliceryhl@google.com> wrote:
> >
> > On Tue, Mar 18, 2025 at 10:12:28AM -0400, Tamir Duberstein wrote:\
> > >
> > > The methods you're describing are all on Vec, right? In other words,
> > > their usage calls for a private `dec_len` or `set_len`. As I've said
> > > repeatedly in the course of this discussion: I would prefer not to
> > > introduce `dec_len` at all here. It (or `set_len`) can be introduced
> > > in the series that adds truncate or your patch that adds clear, where
> > > its signature can be properly scrutinized in the context of an actual
> > > caller.
> >
> > Oh I did not see that you said that. Dropping patch 2 is fine with me.
> >
> > Alice
> 
> Benno, Danilo: are you both OK with this? I'll discard this patch on
> the respin and prepend the patch adding the len <= cap invariant.

I mean, the whole reason to switch set_len() to inc_len() and have a separate
dec_len() was to properly cover things like [1] and Alice' patch by having
dec_len() return the abandoned entries.

If we now only switch set_len() to inc_len() and drop dec_len() then what should
Andrew do?

Maybe we should just revert to Tamir's first proposal, i.e. keep set_len() as it
is, but make it return the abandoned entries, if any.

[1] https://lore.kernel.org/rust-for-linux/20250316111644.154602-1-andrewjballance@gmail.com/

Re: [PATCH 2/2] rust: alloc: add `Vec::dec_len`

[Tamir Duberstein]

On Tue, Mar 18, 2025 at 2:46 PM Danilo Krummrich <dakr@kernel.org> wrote:
>
> On Tue, Mar 18, 2025 at 02:28:02PM -0400, Tamir Duberstein wrote:
> > On Tue, Mar 18, 2025 at 10:44 AM Alice Ryhl <aliceryhl@google.com> wrote:
> > >
> > > On Tue, Mar 18, 2025 at 10:12:28AM -0400, Tamir Duberstein wrote:\
> > > >
> > > > The methods you're describing are all on Vec, right? In other words,
> > > > their usage calls for a private `dec_len` or `set_len`. As I've said
> > > > repeatedly in the course of this discussion: I would prefer not to
> > > > introduce `dec_len` at all here. It (or `set_len`) can be introduced
> > > > in the series that adds truncate or your patch that adds clear, where
> > > > its signature can be properly scrutinized in the context of an actual
> > > > caller.
> > >
> > > Oh I did not see that you said that. Dropping patch 2 is fine with me.
> > >
> > > Alice
> >
> > Benno, Danilo: are you both OK with this? I'll discard this patch on
> > the respin and prepend the patch adding the len <= cap invariant.
>
> I mean, the whole reason to switch set_len() to inc_len() and have a separate
> dec_len() was to properly cover things like [1] and Alice' patch by having
> dec_len() return the abandoned entries.
>
> If we now only switch set_len() to inc_len() and drop dec_len() then what should
> Andrew do?

I'd be completely fine with Andrew (or Alice) taking this patch into
the truncate/resize series[1] (or the series that introduces clear
[2]). It can be properly reviewed there in context.

> Maybe we should just revert to Tamir's first proposal, i.e. keep set_len() as it
> is, but make it return the abandoned entries, if any.

This wouldn't be my preference.

> [1] https://lore.kernel.org/rust-for-linux/20250316111644.154602-1-andrewjballance@gmail.com/

[2] https://lore.kernel.org/all/20250311-iov-iter-v1-4-f6c9134ea824@google.com/

Re: [PATCH 2/2] rust: alloc: add `Vec::dec_len`

[Danilo Krummrich]

On Tue, Mar 18, 2025 at 02:53:48PM -0400, Tamir Duberstein wrote:
> On Tue, Mar 18, 2025 at 2:46 PM Danilo Krummrich <dakr@kernel.org> wrote:
> >
> > On Tue, Mar 18, 2025 at 02:28:02PM -0400, Tamir Duberstein wrote:
> > > On Tue, Mar 18, 2025 at 10:44 AM Alice Ryhl <aliceryhl@google.com> wrote:
> > > >
> > > > On Tue, Mar 18, 2025 at 10:12:28AM -0400, Tamir Duberstein wrote:\
> > > > >
> > > > > The methods you're describing are all on Vec, right? In other words,
> > > > > their usage calls for a private `dec_len` or `set_len`. As I've said
> > > > > repeatedly in the course of this discussion: I would prefer not to
> > > > > introduce `dec_len` at all here. It (or `set_len`) can be introduced
> > > > > in the series that adds truncate or your patch that adds clear, where
> > > > > its signature can be properly scrutinized in the context of an actual
> > > > > caller.
> > > >
> > > > Oh I did not see that you said that. Dropping patch 2 is fine with me.
> > > >
> > > > Alice
> > >
> > > Benno, Danilo: are you both OK with this? I'll discard this patch on
> > > the respin and prepend the patch adding the len <= cap invariant.
> >
> > I mean, the whole reason to switch set_len() to inc_len() and have a separate
> > dec_len() was to properly cover things like [1] and Alice' patch by having
> > dec_len() return the abandoned entries.
> >
> > If we now only switch set_len() to inc_len() and drop dec_len() then what should
> > Andrew do?
> 
> I'd be completely fine with Andrew (or Alice) taking this patch into
> the truncate/resize series[1] (or the series that introduces clear
> [2]). It can be properly reviewed there in context.

Sorry, I'm not willing to make this Andrew's responsibility; set_len() worked
for his patches before.

If you're uncomfortable implementing your proposal without the existence of
truncate(), please rebase onto Andrew's patches.

I think Alice' patch can also go on top of that, since it should just be
truncate(0).

> 
> > Maybe we should just revert to Tamir's first proposal, i.e. keep set_len() as it
> > is, but make it return the abandoned entries, if any.
> 
> This wouldn't be my preference.
> 
> > [1] https://lore.kernel.org/rust-for-linux/20250316111644.154602-1-andrewjballance@gmail.com/
> 
> [2] https://lore.kernel.org/all/20250311-iov-iter-v1-4-f6c9134ea824@google.com/

Re: [PATCH 2/2] rust: alloc: add `Vec::dec_len`

[Tamir Duberstein]

On Tue, Mar 18, 2025 at 3:27 PM Danilo Krummrich <dakr@kernel.org> wrote:
>
> On Tue, Mar 18, 2025 at 02:53:48PM -0400, Tamir Duberstein wrote:
> > On Tue, Mar 18, 2025 at 2:46 PM Danilo Krummrich <dakr@kernel.org> wrote:
> > >
> > > On Tue, Mar 18, 2025 at 02:28:02PM -0400, Tamir Duberstein wrote:
> > > > On Tue, Mar 18, 2025 at 10:44 AM Alice Ryhl <aliceryhl@google.com> wrote:
> > > > >
> > > > > On Tue, Mar 18, 2025 at 10:12:28AM -0400, Tamir Duberstein wrote:\
> > > > > >
> > > > > > The methods you're describing are all on Vec, right? In other words,
> > > > > > their usage calls for a private `dec_len` or `set_len`. As I've said
> > > > > > repeatedly in the course of this discussion: I would prefer not to
> > > > > > introduce `dec_len` at all here. It (or `set_len`) can be introduced
> > > > > > in the series that adds truncate or your patch that adds clear, where
> > > > > > its signature can be properly scrutinized in the context of an actual
> > > > > > caller.
> > > > >
> > > > > Oh I did not see that you said that. Dropping patch 2 is fine with me.
> > > > >
> > > > > Alice
> > > >
> > > > Benno, Danilo: are you both OK with this? I'll discard this patch on
> > > > the respin and prepend the patch adding the len <= cap invariant.
> > >
> > > I mean, the whole reason to switch set_len() to inc_len() and have a separate
> > > dec_len() was to properly cover things like [1] and Alice' patch by having
> > > dec_len() return the abandoned entries.
> > >
> > > If we now only switch set_len() to inc_len() and drop dec_len() then what should
> > > Andrew do?
> >
> > I'd be completely fine with Andrew (or Alice) taking this patch into
> > the truncate/resize series[1] (or the series that introduces clear
> > [2]). It can be properly reviewed there in context.
>
> Sorry, I'm not willing to make this Andrew's responsibility; set_len() worked
> for his patches before.
>
> If you're uncomfortable implementing your proposal without the existence of
> truncate(), please rebase onto Andrew's patches.

This suits me just fine! I tried applying Andrew's patches locally but
I don't have `Documentation/gpu/nova/core/todo.rst`. Do you know what
his base commit is?

> I think Alice' patch can also go on top of that, since it should just be
> truncate(0).

👍

Re: [PATCH 2/2] rust: alloc: add `Vec::dec_len`

[Tamir Duberstein]

On Tue, Mar 18, 2025 at 4:05 PM Tamir Duberstein <tamird@gmail.com> wrote:
>
> On Tue, Mar 18, 2025 at 3:27 PM Danilo Krummrich <dakr@kernel.org> wrote:
> >
> > On Tue, Mar 18, 2025 at 02:53:48PM -0400, Tamir Duberstein wrote:
> > > On Tue, Mar 18, 2025 at 2:46 PM Danilo Krummrich <dakr@kernel.org> wrote:
> > > >
> > > > On Tue, Mar 18, 2025 at 02:28:02PM -0400, Tamir Duberstein wrote:
> > > > > On Tue, Mar 18, 2025 at 10:44 AM Alice Ryhl <aliceryhl@google.com> wrote:
> > > > > >
> > > > > > On Tue, Mar 18, 2025 at 10:12:28AM -0400, Tamir Duberstein wrote:\
> > > > > > >
> > > > > > > The methods you're describing are all on Vec, right? In other words,
> > > > > > > their usage calls for a private `dec_len` or `set_len`. As I've said
> > > > > > > repeatedly in the course of this discussion: I would prefer not to
> > > > > > > introduce `dec_len` at all here. It (or `set_len`) can be introduced
> > > > > > > in the series that adds truncate or your patch that adds clear, where
> > > > > > > its signature can be properly scrutinized in the context of an actual
> > > > > > > caller.
> > > > > >
> > > > > > Oh I did not see that you said that. Dropping patch 2 is fine with me.
> > > > > >
> > > > > > Alice
> > > > >
> > > > > Benno, Danilo: are you both OK with this? I'll discard this patch on
> > > > > the respin and prepend the patch adding the len <= cap invariant.
> > > >
> > > > I mean, the whole reason to switch set_len() to inc_len() and have a separate
> > > > dec_len() was to properly cover things like [1] and Alice' patch by having
> > > > dec_len() return the abandoned entries.
> > > >
> > > > If we now only switch set_len() to inc_len() and drop dec_len() then what should
> > > > Andrew do?
> > >
> > > I'd be completely fine with Andrew (or Alice) taking this patch into
> > > the truncate/resize series[1] (or the series that introduces clear
> > > [2]). It can be properly reviewed there in context.
> >
> > Sorry, I'm not willing to make this Andrew's responsibility; set_len() worked
> > for his patches before.
> >
> > If you're uncomfortable implementing your proposal without the existence of
> > truncate(), please rebase onto Andrew's patches.
>
> This suits me just fine! I tried applying Andrew's patches locally but
> I don't have `Documentation/gpu/nova/core/todo.rst`. Do you know what
> his base commit is?

Nevermind, I can just specify the patch ID.

Re: [PATCH 2/2] rust: alloc: add `Vec::dec_len`

[Danilo Krummrich]

On Tue, Mar 18, 2025 at 04:13:43PM -0400, Tamir Duberstein wrote:
> On Tue, Mar 18, 2025 at 4:05 PM Tamir Duberstein <tamird@gmail.com> wrote:
> >
> > On Tue, Mar 18, 2025 at 3:27 PM Danilo Krummrich <dakr@kernel.org> wrote:
> > >
> > > On Tue, Mar 18, 2025 at 02:53:48PM -0400, Tamir Duberstein wrote:
> > > > On Tue, Mar 18, 2025 at 2:46 PM Danilo Krummrich <dakr@kernel.org> wrote:
> > > > >
> > > > > On Tue, Mar 18, 2025 at 02:28:02PM -0400, Tamir Duberstein wrote:
> > > > > > On Tue, Mar 18, 2025 at 10:44 AM Alice Ryhl <aliceryhl@google.com> wrote:
> > > > > > >
> > > > > > > On Tue, Mar 18, 2025 at 10:12:28AM -0400, Tamir Duberstein wrote:\
> > > > > > > >
> > > > > > > > The methods you're describing are all on Vec, right? In other words,
> > > > > > > > their usage calls for a private `dec_len` or `set_len`. As I've said
> > > > > > > > repeatedly in the course of this discussion: I would prefer not to
> > > > > > > > introduce `dec_len` at all here. It (or `set_len`) can be introduced
> > > > > > > > in the series that adds truncate or your patch that adds clear, where
> > > > > > > > its signature can be properly scrutinized in the context of an actual
> > > > > > > > caller.
> > > > > > >
> > > > > > > Oh I did not see that you said that. Dropping patch 2 is fine with me.
> > > > > > >
> > > > > > > Alice
> > > > > >
> > > > > > Benno, Danilo: are you both OK with this? I'll discard this patch on
> > > > > > the respin and prepend the patch adding the len <= cap invariant.
> > > > >
> > > > > I mean, the whole reason to switch set_len() to inc_len() and have a separate
> > > > > dec_len() was to properly cover things like [1] and Alice' patch by having
> > > > > dec_len() return the abandoned entries.
> > > > >
> > > > > If we now only switch set_len() to inc_len() and drop dec_len() then what should
> > > > > Andrew do?
> > > >
> > > > I'd be completely fine with Andrew (or Alice) taking this patch into
> > > > the truncate/resize series[1] (or the series that introduces clear
> > > > [2]). It can be properly reviewed there in context.
> > >
> > > Sorry, I'm not willing to make this Andrew's responsibility; set_len() worked
> > > for his patches before.
> > >
> > > If you're uncomfortable implementing your proposal without the existence of
> > > truncate(), please rebase onto Andrew's patches.
> >
> > This suits me just fine! I tried applying Andrew's patches locally but
> > I don't have `Documentation/gpu/nova/core/todo.rst`. Do you know what
> > his base commit is?
> 
> Nevermind, I can just specify the patch ID.

Yeah, you can just ignore the nova patch.

Re: [PATCH 2/2] rust: alloc: add `Vec::dec_len`

[kernel test robot]

Hi Tamir,

kernel test robot noticed the following build warnings:

[auto build test WARNING on cf25bc61f8aecad9b0c45fe32697e35ea4b13378]

url:    https://github.com/intel-lab-lkp/linux/commits/Tamir-Duberstein/rust-alloc-replace-Vec-set_len-with-inc_len/20250317-103338
base:   cf25bc61f8aecad9b0c45fe32697e35ea4b13378
patch link:    https://lore.kernel.org/r/20250316-vec-set-len-v1-2-60f98a28723f%40gmail.com
patch subject: [PATCH 2/2] rust: alloc: add `Vec::dec_len`
config: x86_64-rhel-9.4-rust (https://download.01.org/0day-ci/archive/20250320/202503200447.aF1NxDEq-lkp@intel.com/config)
compiler: clang version 18.1.8 (https://github.com/llvm/llvm-project 3b5b5c1ec4a3095ab096dd780e84d7ab81f3d7ff)
rustc: rustc 1.78.0 (9b00956e5 2024-04-29)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20250320/202503200447.aF1NxDEq-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/202503200447.aF1NxDEq-lkp@intel.com/

All warnings (new ones prefixed by >>):

>> warning: method `dec_len` is never used
   --> rust/kernel/alloc/kvec.rs:205:15
   |
   161 | / impl<T, A> Vec<T, A>
   162 | | where
   163 | |     A: Allocator,
   | |_________________- method in this implementation
   ...
   205 |       unsafe fn dec_len(&mut self, count: usize) -> &mut [T] {
   |                 ^^^^^^^
   |
   = note: `#[warn(dead_code)]` on by default

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