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

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

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] {

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.

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

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.

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

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?

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

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

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

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

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.

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.

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?

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.

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

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

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

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.

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/

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/

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/

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).

👍

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.

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.

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
>

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.

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.

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`.

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.

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.

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
>