* Re: [PATCH] rust: file: optimize rust symbol generation for FileDescriptorReservation
From: Kunwu Chan @ 2025-03-17 1:26 UTC (permalink / raw)
To: Alice Ryhl
Cc: ojeda, alex.gaynor, boqun.feng, gary, bjorn3_gh, benno.lossin,
a.hindborg, tmgross, dakr, nathan, nick.desaulniers+lkml, morbo,
justinstitt, rust-for-linux, linux-kernel, llvm, Kunwu Chan,
Grace Deng
In-Reply-To: <CAH5fLgjErD9r8h_tjekZ+sNNDmkU3J3kcSmCG+Mew+-6zQq8tA@mail.gmail.com>
On 2025/3/14 17:21, Alice Ryhl wrote:
> On Fri, Mar 14, 2025 at 3:34 AM Kunwu Chan <kunwu.chan@linux.dev> wrote:
>>> * I think it is easier to read the symbols if you list each sybmol on
>>> one line like this:
>>>
>>> ffff8000805b6ef0 T <kernel::fs::file::FileDescriptorReservation>::fd_install
>>> ffff8000805b6e60 T <kernel::fs::file::FileDescriptorReservation>::get_unused_fd_flags
>>> ffff8000805b6f08 T <kernel::fs::file::FileDescriptorReservation as core::ops::drop::Drop>::drop
>> If in one line, checkpatch.pl will report a warning:
>>
>> WARNING:Prefer a maximum 75 chars per line (possible unwrapped commit
>> description?)
>>
>> If no need to bother with the warning, I'll change it to one line in v2.
> You could do this:
> ... T <kernel::fs::file::FileDescriptorReservation>::fd_install
> but if it's still too long I'd just ignore it.
Thanks, I'll change it in v2.
>
> Alice
--
Thanks,
Kunwu.Chan
^ permalink raw reply
* Re: [PATCH 2/2] rust: alloc: add `Vec::dec_len`
From: Tamir Duberstein @ 2025-03-16 23:27 UTC (permalink / raw)
To: Danilo Krummrich
Cc: Andrew Ballance, Alice Ryhl, Miguel Ojeda, Alex Gaynor,
Boqun Feng, Gary Guo, Björn Roy Baron, Benno Lossin,
Andreas Hindborg, Trevor Gross, rust-for-linux, linux-kernel
In-Reply-To: <Z9dYnSC13ruc-VC5@pollux>
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`.
^ permalink raw reply
* Re: [PATCH 2/2] rust: alloc: add `Vec::dec_len`
From: Danilo Krummrich @ 2025-03-16 23:02 UTC (permalink / raw)
To: Tamir Duberstein
Cc: Andrew Ballance, Alice Ryhl, Miguel Ojeda, Alex Gaynor,
Boqun Feng, Gary Guo, Björn Roy Baron, Benno Lossin,
Andreas Hindborg, Trevor Gross, rust-for-linux, linux-kernel
In-Reply-To: <CAJ-ks9mwuLaULKW0cwD73yb3yH-p9KS3ZFoJJ3OxhvUOpXo3KA@mail.gmail.com>
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.
^ permalink raw reply
* Re: [PATCH 2/2] rust: alloc: add `Vec::dec_len`
From: Tamir Duberstein @ 2025-03-16 22:47 UTC (permalink / raw)
To: Danilo Krummrich
Cc: Andrew Ballance, Alice Ryhl, Miguel Ojeda, Alex Gaynor,
Boqun Feng, Gary Guo, Björn Roy Baron, Benno Lossin,
Andreas Hindborg, Trevor Gross, rust-for-linux, linux-kernel
In-Reply-To: <Z9dTspva0aJRWG3Y@pollux>
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.
^ permalink raw reply
* Re: [PATCH 2/2] rust: alloc: add `Vec::dec_len`
From: Danilo Krummrich @ 2025-03-16 22:41 UTC (permalink / raw)
To: Tamir Duberstein
Cc: Andrew Ballance, Alice Ryhl, Miguel Ojeda, Alex Gaynor,
Boqun Feng, Gary Guo, Björn Roy Baron, Benno Lossin,
Andreas Hindborg, Trevor Gross, rust-for-linux, linux-kernel
In-Reply-To: <20250316-vec-set-len-v1-2-60f98a28723f@gmail.com>
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
>
^ permalink raw reply
* Re: [PATCH 2/2] rust: alloc: add `Vec::dec_len`
From: Tamir Duberstein @ 2025-03-16 22:35 UTC (permalink / raw)
To: Danilo Krummrich, Andrew Ballance, Alice Ryhl, Miguel Ojeda,
Alex Gaynor, Boqun Feng, Gary Guo, Björn Roy Baron,
Benno Lossin, Andreas Hindborg, Trevor Gross
Cc: rust-for-linux, linux-kernel
In-Reply-To: <20250316-vec-set-len-v1-2-60f98a28723f@gmail.com>
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
>
^ permalink raw reply
* [PATCH 2/2] rust: alloc: add `Vec::dec_len`
From: Tamir Duberstein @ 2025-03-16 22:32 UTC (permalink / raw)
To: Danilo Krummrich, Andrew Ballance, Alice Ryhl, Miguel Ojeda,
Alex Gaynor, Boqun Feng, Gary Guo, Björn Roy Baron,
Benno Lossin, Andreas Hindborg, Trevor Gross
Cc: rust-for-linux, linux-kernel, Tamir Duberstein
In-Reply-To: <20250316-vec-set-len-v1-0-60f98a28723f@gmail.com>
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
^ permalink raw reply related
* [PATCH 1/2] rust: alloc: replace `Vec::set_len` with `inc_len`
From: Tamir Duberstein @ 2025-03-16 22:32 UTC (permalink / raw)
To: Danilo Krummrich, Andrew Ballance, Alice Ryhl, Miguel Ojeda,
Alex Gaynor, Boqun Feng, Gary Guo, Björn Roy Baron,
Benno Lossin, Andreas Hindborg, Trevor Gross
Cc: rust-for-linux, linux-kernel, Tamir Duberstein
In-Reply-To: <20250316-vec-set-len-v1-0-60f98a28723f@gmail.com>
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
^ permalink raw reply related
* [PATCH 0/2] rust: alloc: split `Vec::set_len` into `Vec::{inc,dec}_len`
From: Tamir Duberstein @ 2025-03-16 22:31 UTC (permalink / raw)
To: Danilo Krummrich, Andrew Ballance, Alice Ryhl, Miguel Ojeda,
Alex Gaynor, Boqun Feng, Gary Guo, Björn Roy Baron,
Benno Lossin, Andreas Hindborg, Trevor Gross
Cc: rust-for-linux, linux-kernel, 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>
^ permalink raw reply
* Re: [PATCH 1/2] rust: alloc: extend safety requirements of Vec::set_len()
From: Danilo Krummrich @ 2025-03-16 21:59 UTC (permalink / raw)
To: Tamir Duberstein
Cc: Benno Lossin, ojeda, alex.gaynor, boqun.feng, gary, bjorn3_gh,
a.hindborg, aliceryhl, tmgross, andrewjballance, rust-for-linux
In-Reply-To: <CAJ-ks9=OZyRNPxEJqBvjt77Wv=689jHLVFNDCjUE9TM7Vp-00A@mail.gmail.com>
On Sun, Mar 16, 2025 at 05:52:33PM -0400, Tamir Duberstein wrote:
> Actually, I don't think we need `dec_len` after all. `clear` can be
> implemented as `self.truncate(0)`. Do you agree?
Indeed, but I still think it would be nice to have dec_len(). pop() would be
another candidate for instance.
^ permalink raw reply
* Re: [PATCH 1/2] rust: alloc: extend safety requirements of Vec::set_len()
From: Tamir Duberstein @ 2025-03-16 21:52 UTC (permalink / raw)
To: Danilo Krummrich
Cc: Benno Lossin, ojeda, alex.gaynor, boqun.feng, gary, bjorn3_gh,
a.hindborg, aliceryhl, tmgross, andrewjballance, rust-for-linux
In-Reply-To: <CAJ-ks9mBJUReOCbF-ar+Scoo8RAqYu-mc-Ej8UMHoyGHs0H7sw@mail.gmail.com>
On Sun, Mar 16, 2025 at 5:20 PM Tamir Duberstein <tamird@gmail.com> wrote:
>
> On Sun, Mar 16, 2025 at 5:17 PM Danilo Krummrich <dakr@kernel.org> wrote:
> >
> > On Sun, Mar 16, 2025 at 05:10:30PM -0400, Tamir Duberstein wrote:
> > > On Sun, Mar 16, 2025 at 4:54 PM Danilo Krummrich <dakr@kernel.org> wrote:
> > > >
> > > > On Sun, Mar 16, 2025 at 03:30:27PM -0400, Tamir Duberstein wrote:
> > > > > On Sun, Mar 16, 2025 at 3:09 PM Danilo Krummrich <dakr@kernel.org> wrote:
> > > > > >
> > > > > > On Sun, Mar 16, 2025 at 07:59:34PM +0100, Danilo Krummrich wrote:
> > > > > > > But let's define it then; what about:
> > > > > > >
> > > > > > > "[`Vec::set_len`] takes (or kepps) ownership of all elements within the range
> > > > > > > [0; `new_len`] and abandons ownership of all values outside of this range, if
> > > > > > > any."
> > > > > > >
> > > > > > > The caller may take ownership of the abandoned elements."
> > > > > > >
> > > > > > > I'd argue that giving up ownership, while offering someone else to take it means
> > > > > > > that it implies that otherwise we'll just end up forgetting about the value.
> > > > > >
> > > > > > Btw. I'd still prefer if we could enforce that the caller has to document what
> > > > > > should happen to the abandoned value. But I acknowledge that the safety comment
> > > > > > isn't the scope for it.
> > > > > >
> > > > > > It'd be great if e.g. clippy would give us a tool to do something analogous to
> > > > > > safety comments.
> > > > > >
> > > > > > It think it would be useful to enfoce some additional safety documentation. For
> > > > > > instance, I think the kernel would much benefit if we could enforce that
> > > > > > mem::forget() must be justified with a comment, since as mentioned ina previous
> > > > > > mail, it can cause fatal bugs, for instance when used on lock guards.
> > > > >
> > > > > There are other examples; ManuallyDrop and Box::leak are two that
> > > > > immediately come to mind.
> > > > >
> > > > > But focusing on Vec::set_len again, could we return a mut slice to the
> > > > > tail when new_len < old_len? Something like:
> > > > >
> > > > > diff --git a/rust/kernel/alloc/kvec.rs b/rust/kernel/alloc/kvec.rs
> > > > > index c12844764671..e5f857d723ec 100644
> > > > > --- a/rust/kernel/alloc/kvec.rs
> > > > > +++ b/rust/kernel/alloc/kvec.rs
> > > > > @@ -191,9 +191,16 @@ pub fn len(&self) -> usize {
> > > > > /// - If `new_len` is greater than `self.len`, all elements
> > > > > within the interval
> > > > > /// [`self.len`,`new_len`) must be initialized.
> > > > > #[inline]
> > > > > - pub unsafe fn set_len(&mut self, new_len: usize) {
> > > > > + pub unsafe fn set_len(&mut self, new_len: usize) -> &mut [T] {
> > > > > debug_assert!(new_len <= self.capacity());
> > > > > - self.len = new_len;
> > > > > + let old_len = core::mem::replace(&mut self.len, new_len);
> > > > > + match old_len.checked_sub(new_len) {
> > > > > + None => &mut [],
> > > > > + Some(len) => {
> > > > > + // SAFETY: ...
> > > > > + unsafe {
> > > > > slice::from_raw_parts_mut(self.as_mut_ptr().add(new_len), len) }
> > > > > + }
> > > > > + }
> > > > > }
> > > > >
> > > > > Would that sufficiently communicate to the caller that they should
> > > > > deal with this memory?
> > > >
> > > > I think that is a good idea. I'm not sure I like that this is useless when
> > > > new_len > self.len, but it also doesn't hurt too much, I guess.
> > > >
> > > > Feel free to send the corresponding patch. Also, feel free to add the
> > > > corresponding comment about ownership while at it.
> > > >
> > > > I'd just drop this patch then.
> > >
> > > Looking at the usages of `set_len` that we have today (excluding the
> > > pending nova patches for truncate and resize), they all amount to
> > > `v.set_len(v.len() + n)`. What do you think about replacing this
> > > method with `inc_len`? In the nova patches the only need for
> > > decrementing the length would be in `truncate`, so `dec_len` can be
> > > introduced there or the length can be decremented there directly.
> >
> > I think splitting it in inc_len() and dec_len() is good. Note that other methods
> > need dec_len() too, e.g. [1].
> >
> > [1] https://lore.kernel.org/rust-for-linux/20250311-iov-iter-v1-4-f6c9134ea824@google.com/
>
> Ok, I'll send a 2-patch series that adds `dec_len` as a private
> helper. I see no need for it to be pub at this time.
Actually, I don't think we need `dec_len` after all. `clear` can be
implemented as `self.truncate(0)`. Do you agree?
^ permalink raw reply
* Re: [PATCH 1/2] rust: alloc: extend safety requirements of Vec::set_len()
From: Tamir Duberstein @ 2025-03-16 21:20 UTC (permalink / raw)
To: Danilo Krummrich
Cc: Benno Lossin, ojeda, alex.gaynor, boqun.feng, gary, bjorn3_gh,
a.hindborg, aliceryhl, tmgross, andrewjballance, rust-for-linux
In-Reply-To: <Z9c_8AQWgW70XH1U@pollux>
On Sun, Mar 16, 2025 at 5:17 PM Danilo Krummrich <dakr@kernel.org> wrote:
>
> On Sun, Mar 16, 2025 at 05:10:30PM -0400, Tamir Duberstein wrote:
> > On Sun, Mar 16, 2025 at 4:54 PM Danilo Krummrich <dakr@kernel.org> wrote:
> > >
> > > On Sun, Mar 16, 2025 at 03:30:27PM -0400, Tamir Duberstein wrote:
> > > > On Sun, Mar 16, 2025 at 3:09 PM Danilo Krummrich <dakr@kernel.org> wrote:
> > > > >
> > > > > On Sun, Mar 16, 2025 at 07:59:34PM +0100, Danilo Krummrich wrote:
> > > > > > But let's define it then; what about:
> > > > > >
> > > > > > "[`Vec::set_len`] takes (or kepps) ownership of all elements within the range
> > > > > > [0; `new_len`] and abandons ownership of all values outside of this range, if
> > > > > > any."
> > > > > >
> > > > > > The caller may take ownership of the abandoned elements."
> > > > > >
> > > > > > I'd argue that giving up ownership, while offering someone else to take it means
> > > > > > that it implies that otherwise we'll just end up forgetting about the value.
> > > > >
> > > > > Btw. I'd still prefer if we could enforce that the caller has to document what
> > > > > should happen to the abandoned value. But I acknowledge that the safety comment
> > > > > isn't the scope for it.
> > > > >
> > > > > It'd be great if e.g. clippy would give us a tool to do something analogous to
> > > > > safety comments.
> > > > >
> > > > > It think it would be useful to enfoce some additional safety documentation. For
> > > > > instance, I think the kernel would much benefit if we could enforce that
> > > > > mem::forget() must be justified with a comment, since as mentioned ina previous
> > > > > mail, it can cause fatal bugs, for instance when used on lock guards.
> > > >
> > > > There are other examples; ManuallyDrop and Box::leak are two that
> > > > immediately come to mind.
> > > >
> > > > But focusing on Vec::set_len again, could we return a mut slice to the
> > > > tail when new_len < old_len? Something like:
> > > >
> > > > diff --git a/rust/kernel/alloc/kvec.rs b/rust/kernel/alloc/kvec.rs
> > > > index c12844764671..e5f857d723ec 100644
> > > > --- a/rust/kernel/alloc/kvec.rs
> > > > +++ b/rust/kernel/alloc/kvec.rs
> > > > @@ -191,9 +191,16 @@ pub fn len(&self) -> usize {
> > > > /// - If `new_len` is greater than `self.len`, all elements
> > > > within the interval
> > > > /// [`self.len`,`new_len`) must be initialized.
> > > > #[inline]
> > > > - pub unsafe fn set_len(&mut self, new_len: usize) {
> > > > + pub unsafe fn set_len(&mut self, new_len: usize) -> &mut [T] {
> > > > debug_assert!(new_len <= self.capacity());
> > > > - self.len = new_len;
> > > > + let old_len = core::mem::replace(&mut self.len, new_len);
> > > > + match old_len.checked_sub(new_len) {
> > > > + None => &mut [],
> > > > + Some(len) => {
> > > > + // SAFETY: ...
> > > > + unsafe {
> > > > slice::from_raw_parts_mut(self.as_mut_ptr().add(new_len), len) }
> > > > + }
> > > > + }
> > > > }
> > > >
> > > > Would that sufficiently communicate to the caller that they should
> > > > deal with this memory?
> > >
> > > I think that is a good idea. I'm not sure I like that this is useless when
> > > new_len > self.len, but it also doesn't hurt too much, I guess.
> > >
> > > Feel free to send the corresponding patch. Also, feel free to add the
> > > corresponding comment about ownership while at it.
> > >
> > > I'd just drop this patch then.
> >
> > Looking at the usages of `set_len` that we have today (excluding the
> > pending nova patches for truncate and resize), they all amount to
> > `v.set_len(v.len() + n)`. What do you think about replacing this
> > method with `inc_len`? In the nova patches the only need for
> > decrementing the length would be in `truncate`, so `dec_len` can be
> > introduced there or the length can be decremented there directly.
>
> I think splitting it in inc_len() and dec_len() is good. Note that other methods
> need dec_len() too, e.g. [1].
>
> [1] https://lore.kernel.org/rust-for-linux/20250311-iov-iter-v1-4-f6c9134ea824@google.com/
Ok, I'll send a 2-patch series that adds `dec_len` as a private
helper. I see no need for it to be pub at this time.
^ permalink raw reply
* Re: [PATCH 1/2] rust: alloc: extend safety requirements of Vec::set_len()
From: Danilo Krummrich @ 2025-03-16 21:17 UTC (permalink / raw)
To: Tamir Duberstein
Cc: Benno Lossin, ojeda, alex.gaynor, boqun.feng, gary, bjorn3_gh,
a.hindborg, aliceryhl, tmgross, andrewjballance, rust-for-linux
In-Reply-To: <CAJ-ks9=J1hsnFvKzAOwi918UiP49-mmSki=aH2dhiSr7WAqrPQ@mail.gmail.com>
On Sun, Mar 16, 2025 at 05:10:30PM -0400, Tamir Duberstein wrote:
> On Sun, Mar 16, 2025 at 4:54 PM Danilo Krummrich <dakr@kernel.org> wrote:
> >
> > On Sun, Mar 16, 2025 at 03:30:27PM -0400, Tamir Duberstein wrote:
> > > On Sun, Mar 16, 2025 at 3:09 PM Danilo Krummrich <dakr@kernel.org> wrote:
> > > >
> > > > On Sun, Mar 16, 2025 at 07:59:34PM +0100, Danilo Krummrich wrote:
> > > > > But let's define it then; what about:
> > > > >
> > > > > "[`Vec::set_len`] takes (or kepps) ownership of all elements within the range
> > > > > [0; `new_len`] and abandons ownership of all values outside of this range, if
> > > > > any."
> > > > >
> > > > > The caller may take ownership of the abandoned elements."
> > > > >
> > > > > I'd argue that giving up ownership, while offering someone else to take it means
> > > > > that it implies that otherwise we'll just end up forgetting about the value.
> > > >
> > > > Btw. I'd still prefer if we could enforce that the caller has to document what
> > > > should happen to the abandoned value. But I acknowledge that the safety comment
> > > > isn't the scope for it.
> > > >
> > > > It'd be great if e.g. clippy would give us a tool to do something analogous to
> > > > safety comments.
> > > >
> > > > It think it would be useful to enfoce some additional safety documentation. For
> > > > instance, I think the kernel would much benefit if we could enforce that
> > > > mem::forget() must be justified with a comment, since as mentioned ina previous
> > > > mail, it can cause fatal bugs, for instance when used on lock guards.
> > >
> > > There are other examples; ManuallyDrop and Box::leak are two that
> > > immediately come to mind.
> > >
> > > But focusing on Vec::set_len again, could we return a mut slice to the
> > > tail when new_len < old_len? Something like:
> > >
> > > diff --git a/rust/kernel/alloc/kvec.rs b/rust/kernel/alloc/kvec.rs
> > > index c12844764671..e5f857d723ec 100644
> > > --- a/rust/kernel/alloc/kvec.rs
> > > +++ b/rust/kernel/alloc/kvec.rs
> > > @@ -191,9 +191,16 @@ pub fn len(&self) -> usize {
> > > /// - If `new_len` is greater than `self.len`, all elements
> > > within the interval
> > > /// [`self.len`,`new_len`) must be initialized.
> > > #[inline]
> > > - pub unsafe fn set_len(&mut self, new_len: usize) {
> > > + pub unsafe fn set_len(&mut self, new_len: usize) -> &mut [T] {
> > > debug_assert!(new_len <= self.capacity());
> > > - self.len = new_len;
> > > + let old_len = core::mem::replace(&mut self.len, new_len);
> > > + match old_len.checked_sub(new_len) {
> > > + None => &mut [],
> > > + Some(len) => {
> > > + // SAFETY: ...
> > > + unsafe {
> > > slice::from_raw_parts_mut(self.as_mut_ptr().add(new_len), len) }
> > > + }
> > > + }
> > > }
> > >
> > > Would that sufficiently communicate to the caller that they should
> > > deal with this memory?
> >
> > I think that is a good idea. I'm not sure I like that this is useless when
> > new_len > self.len, but it also doesn't hurt too much, I guess.
> >
> > Feel free to send the corresponding patch. Also, feel free to add the
> > corresponding comment about ownership while at it.
> >
> > I'd just drop this patch then.
>
> Looking at the usages of `set_len` that we have today (excluding the
> pending nova patches for truncate and resize), they all amount to
> `v.set_len(v.len() + n)`. What do you think about replacing this
> method with `inc_len`? In the nova patches the only need for
> decrementing the length would be in `truncate`, so `dec_len` can be
> introduced there or the length can be decremented there directly.
I think splitting it in inc_len() and dec_len() is good. Note that other methods
need dec_len() too, e.g. [1].
[1] https://lore.kernel.org/rust-for-linux/20250311-iov-iter-v1-4-f6c9134ea824@google.com/
^ permalink raw reply
* Re: [PATCH 1/2] rust: alloc: extend safety requirements of Vec::set_len()
From: Tamir Duberstein @ 2025-03-16 21:10 UTC (permalink / raw)
To: Danilo Krummrich
Cc: Benno Lossin, ojeda, alex.gaynor, boqun.feng, gary, bjorn3_gh,
a.hindborg, aliceryhl, tmgross, andrewjballance, rust-for-linux
In-Reply-To: <Z9c6e8cbdFMTT6K0@pollux>
On Sun, Mar 16, 2025 at 4:54 PM Danilo Krummrich <dakr@kernel.org> wrote:
>
> On Sun, Mar 16, 2025 at 03:30:27PM -0400, Tamir Duberstein wrote:
> > On Sun, Mar 16, 2025 at 3:09 PM Danilo Krummrich <dakr@kernel.org> wrote:
> > >
> > > On Sun, Mar 16, 2025 at 07:59:34PM +0100, Danilo Krummrich wrote:
> > > > But let's define it then; what about:
> > > >
> > > > "[`Vec::set_len`] takes (or kepps) ownership of all elements within the range
> > > > [0; `new_len`] and abandons ownership of all values outside of this range, if
> > > > any."
> > > >
> > > > The caller may take ownership of the abandoned elements."
> > > >
> > > > I'd argue that giving up ownership, while offering someone else to take it means
> > > > that it implies that otherwise we'll just end up forgetting about the value.
> > >
> > > Btw. I'd still prefer if we could enforce that the caller has to document what
> > > should happen to the abandoned value. But I acknowledge that the safety comment
> > > isn't the scope for it.
> > >
> > > It'd be great if e.g. clippy would give us a tool to do something analogous to
> > > safety comments.
> > >
> > > It think it would be useful to enfoce some additional safety documentation. For
> > > instance, I think the kernel would much benefit if we could enforce that
> > > mem::forget() must be justified with a comment, since as mentioned ina previous
> > > mail, it can cause fatal bugs, for instance when used on lock guards.
> >
> > There are other examples; ManuallyDrop and Box::leak are two that
> > immediately come to mind.
> >
> > But focusing on Vec::set_len again, could we return a mut slice to the
> > tail when new_len < old_len? Something like:
> >
> > diff --git a/rust/kernel/alloc/kvec.rs b/rust/kernel/alloc/kvec.rs
> > index c12844764671..e5f857d723ec 100644
> > --- a/rust/kernel/alloc/kvec.rs
> > +++ b/rust/kernel/alloc/kvec.rs
> > @@ -191,9 +191,16 @@ pub fn len(&self) -> usize {
> > /// - If `new_len` is greater than `self.len`, all elements
> > within the interval
> > /// [`self.len`,`new_len`) must be initialized.
> > #[inline]
> > - pub unsafe fn set_len(&mut self, new_len: usize) {
> > + pub unsafe fn set_len(&mut self, new_len: usize) -> &mut [T] {
> > debug_assert!(new_len <= self.capacity());
> > - self.len = new_len;
> > + let old_len = core::mem::replace(&mut self.len, new_len);
> > + match old_len.checked_sub(new_len) {
> > + None => &mut [],
> > + Some(len) => {
> > + // SAFETY: ...
> > + unsafe {
> > slice::from_raw_parts_mut(self.as_mut_ptr().add(new_len), len) }
> > + }
> > + }
> > }
> >
> > Would that sufficiently communicate to the caller that they should
> > deal with this memory?
>
> I think that is a good idea. I'm not sure I like that this is useless when
> new_len > self.len, but it also doesn't hurt too much, I guess.
>
> Feel free to send the corresponding patch. Also, feel free to add the
> corresponding comment about ownership while at it.
>
> I'd just drop this patch then.
Looking at the usages of `set_len` that we have today (excluding the
pending nova patches for truncate and resize), they all amount to
`v.set_len(v.len() + n)`. What do you think about replacing this
method with `inc_len`? In the nova patches the only need for
decrementing the length would be in `truncate`, so `dec_len` can be
introduced there or the length can be decremented there directly.
^ permalink raw reply
* Re: [PATCH v2 00/22] make pin-init into a standalone crate
From: Miguel Ojeda @ 2025-03-16 21:07 UTC (permalink / raw)
To: Benno Lossin
Cc: Miguel Ojeda, Alex Gaynor, Boqun Feng, Gary Guo,
Björn Roy Baron, Andreas Hindborg, Alice Ryhl, Trevor Gross,
rust-for-linux
In-Reply-To: <20250308110339.2997091-1-benno.lossin@proton.me>
On Sat, Mar 8, 2025 at 12:04 PM Benno Lossin <benno.lossin@proton.me> wrote:
>
> Special thanks to Miguel for helping me a lot with getting the series
> ready!
You're welcome -- I hope pin-init's life will be easier now that this
is finally in :)
I had to do a couple re-arrangements to keep it building (and
`rustdoc`) for every commit on top of the `rust-next` we already had
-- please see below.
Applied to `rust-next` -- thanks everyone!
[ Remove the `expect` (and thus the `lint_reasons` feature) since
the tree now uses `quote!` from `rust/macros/export.rs`. Remove the
`TokenStream` import removal, since it is now used as well.
In addition, temporarily (i.e. just for this commit) use an `--extern
force:alloc` to prevent an unknown `new_uninit` error in the `rustdoc`
target. For context, please see a similar case in:
https://lore.kernel.org/lkml/20240422090644.525520-1-ojeda@kernel.org/
And adjusted the message above. - Miguel ]
[ Undo the temporary `--extern force:alloc` since now we have contents
for `alloc` here. - Miguel ]
Cheers,
Miguel
^ permalink raw reply
* Re: [PATCH v4] rust: error: Extend the Result documentation
From: Miguel Ojeda @ 2025-03-16 21:01 UTC (permalink / raw)
To: Dirk Behme; +Cc: rust-for-linux, ojeda, daniel, aliceryhl
In-Reply-To: <20250122054719.595878-1-dirk.behme@de.bosch.com>
On Wed, Jan 22, 2025 at 6:47 AM Dirk Behme <dirk.behme@de.bosch.com> wrote:
>
> Extend the Result documentation by some guidelines and examples how
> to handle Result error cases gracefully. And how to not handle them.
>
> While at it fix one missing `Result` link in the existing documentation.
>
> Link: https://lore.kernel.org/rust-for-linux/CANiq72keOdXy0LFKk9SzYWwSjiD710v=hQO4xi+5E4xNALa6cA@mail.gmail.com/
> Signed-off-by: Dirk Behme <dirk.behme@de.bosch.com>
Applied to `rust-next` -- thanks everyone!
[ Moved links out-of-line for improved readability. Fixed `srctree`
link. Sorted out-of-line links. Added newlines for consistency
with other docs. Applied paragraph break suggestion. Reworded
slightly the docs in a couple places. Added Markdown.
In addition, added `#[allow(clippy::single_match)` for the first
example. It cannot be an `expect` since due to a difference introduced
in Rust 1.85.0 when there are comments in the arms of the `match`.
Reported it upstream, but it was intended:
https://github.com/rust-lang/rust-clippy/issues/14418
Perhaps Clippy will lint about it in the future, but without autofix:
https://github.com/rust-lang/rust-clippy/pull/14420
- Miguel ]
Cheers,
Miguel
^ permalink raw reply
* Re: [PATCH 1/2] rust: alloc: extend safety requirements of Vec::set_len()
From: Danilo Krummrich @ 2025-03-16 20:54 UTC (permalink / raw)
To: Tamir Duberstein
Cc: Benno Lossin, ojeda, alex.gaynor, boqun.feng, gary, bjorn3_gh,
a.hindborg, aliceryhl, tmgross, andrewjballance, rust-for-linux
In-Reply-To: <CAJ-ks9=b-uMCLMMKT-9emgrrUXeaA50DBSEAWjSD+uHHZu-i9Q@mail.gmail.com>
On Sun, Mar 16, 2025 at 03:30:27PM -0400, Tamir Duberstein wrote:
> On Sun, Mar 16, 2025 at 3:09 PM Danilo Krummrich <dakr@kernel.org> wrote:
> >
> > On Sun, Mar 16, 2025 at 07:59:34PM +0100, Danilo Krummrich wrote:
> > > But let's define it then; what about:
> > >
> > > "[`Vec::set_len`] takes (or kepps) ownership of all elements within the range
> > > [0; `new_len`] and abandons ownership of all values outside of this range, if
> > > any."
> > >
> > > The caller may take ownership of the abandoned elements."
> > >
> > > I'd argue that giving up ownership, while offering someone else to take it means
> > > that it implies that otherwise we'll just end up forgetting about the value.
> >
> > Btw. I'd still prefer if we could enforce that the caller has to document what
> > should happen to the abandoned value. But I acknowledge that the safety comment
> > isn't the scope for it.
> >
> > It'd be great if e.g. clippy would give us a tool to do something analogous to
> > safety comments.
> >
> > It think it would be useful to enfoce some additional safety documentation. For
> > instance, I think the kernel would much benefit if we could enforce that
> > mem::forget() must be justified with a comment, since as mentioned ina previous
> > mail, it can cause fatal bugs, for instance when used on lock guards.
>
> There are other examples; ManuallyDrop and Box::leak are two that
> immediately come to mind.
>
> But focusing on Vec::set_len again, could we return a mut slice to the
> tail when new_len < old_len? Something like:
>
> diff --git a/rust/kernel/alloc/kvec.rs b/rust/kernel/alloc/kvec.rs
> index c12844764671..e5f857d723ec 100644
> --- a/rust/kernel/alloc/kvec.rs
> +++ b/rust/kernel/alloc/kvec.rs
> @@ -191,9 +191,16 @@ pub fn len(&self) -> usize {
> /// - If `new_len` is greater than `self.len`, all elements
> within the interval
> /// [`self.len`,`new_len`) must be initialized.
> #[inline]
> - pub unsafe fn set_len(&mut self, new_len: usize) {
> + pub unsafe fn set_len(&mut self, new_len: usize) -> &mut [T] {
> debug_assert!(new_len <= self.capacity());
> - self.len = new_len;
> + let old_len = core::mem::replace(&mut self.len, new_len);
> + match old_len.checked_sub(new_len) {
> + None => &mut [],
> + Some(len) => {
> + // SAFETY: ...
> + unsafe {
> slice::from_raw_parts_mut(self.as_mut_ptr().add(new_len), len) }
> + }
> + }
> }
>
> Would that sufficiently communicate to the caller that they should
> deal with this memory?
I think that is a good idea. I'm not sure I like that this is useless when
new_len > self.len, but it also doesn't hurt too much, I guess.
Feel free to send the corresponding patch. Also, feel free to add the
corresponding comment about ownership while at it.
I'd just drop this patch then.
^ permalink raw reply
* Re: [PATCH 1/2] rust: alloc: extend safety requirements of Vec::set_len()
From: Tamir Duberstein @ 2025-03-16 19:30 UTC (permalink / raw)
To: Danilo Krummrich
Cc: Benno Lossin, ojeda, alex.gaynor, boqun.feng, gary, bjorn3_gh,
a.hindborg, aliceryhl, tmgross, andrewjballance, rust-for-linux
In-Reply-To: <Z9ch0fgUI-V-rlGr@pollux>
On Sun, Mar 16, 2025 at 3:09 PM Danilo Krummrich <dakr@kernel.org> wrote:
>
> On Sun, Mar 16, 2025 at 07:59:34PM +0100, Danilo Krummrich wrote:
> > But let's define it then; what about:
> >
> > "[`Vec::set_len`] takes (or kepps) ownership of all elements within the range
> > [0; `new_len`] and abandons ownership of all values outside of this range, if
> > any."
> >
> > The caller may take ownership of the abandoned elements."
> >
> > I'd argue that giving up ownership, while offering someone else to take it means
> > that it implies that otherwise we'll just end up forgetting about the value.
>
> Btw. I'd still prefer if we could enforce that the caller has to document what
> should happen to the abandoned value. But I acknowledge that the safety comment
> isn't the scope for it.
>
> It'd be great if e.g. clippy would give us a tool to do something analogous to
> safety comments.
>
> It think it would be useful to enfoce some additional safety documentation. For
> instance, I think the kernel would much benefit if we could enforce that
> mem::forget() must be justified with a comment, since as mentioned ina previous
> mail, it can cause fatal bugs, for instance when used on lock guards.
There are other examples; ManuallyDrop and Box::leak are two that
immediately come to mind.
But focusing on Vec::set_len again, could we return a mut slice to the
tail when new_len < old_len? Something like:
diff --git a/rust/kernel/alloc/kvec.rs b/rust/kernel/alloc/kvec.rs
index c12844764671..e5f857d723ec 100644
--- a/rust/kernel/alloc/kvec.rs
+++ b/rust/kernel/alloc/kvec.rs
@@ -191,9 +191,16 @@ pub fn len(&self) -> usize {
/// - If `new_len` is greater than `self.len`, all elements
within the interval
/// [`self.len`,`new_len`) must be initialized.
#[inline]
- pub unsafe fn set_len(&mut self, new_len: usize) {
+ pub unsafe fn set_len(&mut self, new_len: usize) -> &mut [T] {
debug_assert!(new_len <= self.capacity());
- self.len = new_len;
+ let old_len = core::mem::replace(&mut self.len, new_len);
+ match old_len.checked_sub(new_len) {
+ None => &mut [],
+ Some(len) => {
+ // SAFETY: ...
+ unsafe {
slice::from_raw_parts_mut(self.as_mut_ptr().add(new_len), len) }
+ }
+ }
}
Would that sufficiently communicate to the caller that they should
deal with this memory?
^ permalink raw reply related
* Re: [PATCH v2 1/3] rust: alloc: add Vec::truncate method
From: Danilo Krummrich @ 2025-03-16 19:19 UTC (permalink / raw)
To: Andrew Ballance
Cc: airlied, simona, maarten.lankhorst, mripard, tzimmermann, corbet,
ojeda, alex.gaynor, boqun.feng, gary, bjorn3_gh, benno.lossin,
a.hindborg, aliceryhl, tmgross, acourbot, nouveau, dri-devel,
linux-doc, linux-kernel, rust-for-linux
In-Reply-To: <20250316111644.154602-2-andrewjballance@gmail.com>
On Sun, Mar 16, 2025 at 06:16:42AM -0500, Andrew Ballance wrote:
> implement the equivalent to the std's Vec::truncate
> on the kernel's Vec type.
>
> Signed-off-by: Andrew Ballance <andrewjballance@gmail.com>
> ---
> rust/kernel/alloc/kvec.rs | 36 ++++++++++++++++++++++++++++++++++++
> 1 file changed, 36 insertions(+)
>
> diff --git a/rust/kernel/alloc/kvec.rs b/rust/kernel/alloc/kvec.rs
> index ae9d072741ce..18bcc59f0b38 100644
> --- a/rust/kernel/alloc/kvec.rs
> +++ b/rust/kernel/alloc/kvec.rs
> @@ -452,6 +452,42 @@ pub fn reserve(&mut self, additional: usize, flags: Flags) -> Result<(), AllocEr
>
> Ok(())
> }
> +
> + /// Shortens the vector, setting the length to `len` and drops the removed values.
> + /// If `len` is greater than or equal to the current length, this does nothing.
> + ///
> + /// This has no effect on the capacity and will not allocate.
> + /// # Examples
> + ///
> + /// ```
> + /// let mut v = kernel::kvec![1, 2, 3]?;
> + /// v.truncate(1);
> + /// assert_eq!(v.len(), 1);
> + /// assert_eq!(&v, &[1]);
> + ///
> + /// # Ok::<(), Error>(())
> + /// ```
> + pub fn truncate(&mut self, len: usize) {
> + if len >= self.len() {
> + return;
> + }
> +
> + let drop_range = len..self.len();
> +
> + // SAFETY: `drop_range` is a subrange of `[0, len)` by the bounds check above.
> + let ptr: *mut [T] = unsafe { self.get_unchecked_mut(drop_range) };
> +
> + // SAFETY:
> + // - this will always shrink the vector because of the above bounds check
> + // - [`new_len`, `self.len`) will be dropped through the call to `drop_in_place` below
We've just figured out that this part is not needed after all, sorry for the
inconvenience. No need to resend for this though, I can remove this line when
applying the patch.
> + unsafe { self.set_len(len) };
> +
> + // SAFETY:
> + // - the dropped values are valid `T`s by the type invariant
> + // - we are allowed to invalidate [`new_len`, `old_len`) because we just changed the
> + // len, therefore we have exclusive access to [`new_len`, `old_len`)
> + unsafe { ptr::drop_in_place(ptr) };
> + }
> }
>
> impl<T: Clone, A: Allocator> Vec<T, A> {
> --
> 2.48.1
>
^ permalink raw reply
* Re: [PATCH 1/2] rust: alloc: extend safety requirements of Vec::set_len()
From: Danilo Krummrich @ 2025-03-16 19:09 UTC (permalink / raw)
To: Benno Lossin
Cc: Tamir Duberstein, ojeda, alex.gaynor, boqun.feng, gary, bjorn3_gh,
a.hindborg, aliceryhl, tmgross, andrewjballance, rust-for-linux
In-Reply-To: <Z9cfkKTVKtc3imyK@pollux>
On Sun, Mar 16, 2025 at 07:59:34PM +0100, Danilo Krummrich wrote:
> But let's define it then; what about:
>
> "[`Vec::set_len`] takes (or kepps) ownership of all elements within the range
> [0; `new_len`] and abandons ownership of all values outside of this range, if
> any."
>
> The caller may take ownership of the abandoned elements."
>
> I'd argue that giving up ownership, while offering someone else to take it means
> that it implies that otherwise we'll just end up forgetting about the value.
Btw. I'd still prefer if we could enforce that the caller has to document what
should happen to the abandoned value. But I acknowledge that the safety comment
isn't the scope for it.
It'd be great if e.g. clippy would give us a tool to do something analogous to
safety comments.
It think it would be useful to enfoce some additional safety documentation. For
instance, I think the kernel would much benefit if we could enforce that
mem::forget() must be justified with a comment, since as mentioned ina previous
mail, it can cause fatal bugs, for instance when used on lock guards.
^ permalink raw reply
* Re: [PATCH 2/2] rust: workqueue: remove HasWork::OFFSET
From: Tamir Duberstein @ 2025-03-16 18:59 UTC (permalink / raw)
To: Benno Lossin
Cc: Miguel Ojeda, Alex Gaynor, Boqun Feng, Gary Guo,
Björn Roy Baron, Andreas Hindborg, Alice Ryhl, Trevor Gross,
Bjorn Helgaas, Greg Kroah-Hartman, Rafael J. Wysocki,
Danilo Krummrich, rust-for-linux, linux-kernel, linux-pci
In-Reply-To: <D8HVKRW45ESG.3NP8BPWF76RYT@proton.me>
On Sun, Mar 16, 2025 at 1:43 PM Benno Lossin <benno.lossin@proton.me> wrote:
>
> No change required, with my reply above I intended to take my
> complaint away :)
Cool :) is there something else to be done to earn your RB, or do you
mean to defer to Alice?
^ permalink raw reply
* Re: [PATCH 1/2] rust: alloc: extend safety requirements of Vec::set_len()
From: Danilo Krummrich @ 2025-03-16 18:59 UTC (permalink / raw)
To: Benno Lossin
Cc: Tamir Duberstein, ojeda, alex.gaynor, boqun.feng, gary, bjorn3_gh,
a.hindborg, aliceryhl, tmgross, andrewjballance, rust-for-linux
In-Reply-To: <D8HVI9K91JWV.NHOPVELH5JE5@proton.me>
On Sun, Mar 16, 2025 at 05:40:29PM +0000, Benno Lossin wrote:
> On Sun Mar 16, 2025 at 2:46 PM CET, Danilo Krummrich wrote:
> > On Sun, Mar 16, 2025 at 09:13:50AM -0400, Tamir Duberstein wrote:
> >> On Sun, Mar 16, 2025 at 9:01 AM Danilo Krummrich <dakr@kernel.org> wrote:
> >> > On Sun, Mar 16, 2025 at 08:42:43AM -0400, Tamir Duberstein wrote:
> >> > > On Sun, Mar 16, 2025 at 8:31 AM Danilo Krummrich <dakr@kernel.org> wrote:
> >> > >
> >> > > > Let's go with just "must be taken ownership of" then. Unless there's subsequent
> >> > > > feedback, I won't send a new version for this, since you both already gave your
> >> > > > (conditional) RB for this.
>
> I didn't realize the `forget` argument immediately, so I sadly have to
> take my RB back.
>
> >> > > What does it mean to take ownership, if not to run the destructor?
> >> >
> >> > Taking responsibility over the decision of what should happen to the value, i.e.
> >> > forget about the value, drop it right away, keep it alive at a different memory
> >> > location.
> >> >
> >> > > Given Benno's insight, I think the safety text is correct as it
> >> > > existed before this patch.
> >> >
> >> > Without the addition there's no requirement for the caller to take ownership,
> >> > but that's what we want here. Without the requirement it would be on set_len()
> >> > to take a decision on what should happen with the value.
> >>
> >> It isn't a safety requirement, right? I think it's fine to document,
> >> but given that neglecting to run a destructor is safe, it doesn't seem
> >> to affect safety.
> >
> > I don't think there's a rule that safety requirements of unsafe functions must
> > only be used to guarantee memory safety, etc.
>
> Well that's their only point. Safety documentation should only be
> concerned with memory safety.
Oops, I wanted to say "must not only be used to *directly* guarantee memory
safety, etc.".
For instance, it is also used to justify type invariants (that exist to prevent
UB).
So, what I wanted to say is that setting self.len by iteself is never directly a
safety critical thing.
> You can still put it in the normal documentation though.
>
> > But you're right, if set_len() guarantees a default of what happens to the
> > value(s) when the caller doesn't take ownership, we could avoid the safety
> > requirement.
>
> Yeah the default is to forget the value.
It's not really a defined default though, it's just what ultimately happens as
the consequence of how Vec in general and set_len() are implemented.
But let's define it then; what about:
"[`Vec::set_len`] takes (or kepps) ownership of all elements within the range
[0; `new_len`] and abandons ownership of all values outside of this range, if
any."
The caller may take ownership of the abandoned elements."
I'd argue that giving up ownership, while offering someone else to take it means
that it implies that otherwise we'll just end up forgetting about the value.
^ permalink raw reply
* Re: [PATCH v2] rust: uaccess: mark UserSliceWriter method inline
From: Antonio Hickey @ 2025-03-16 18:54 UTC (permalink / raw)
To: benno.lossin
Cc: a.hindborg, alex.gaynor, aliceryhl, bjorn3_gh, boqun.feng,
contact, dakr, gary, justinstitt, linux-kernel, llvm, morbo,
nathan, nick.desaulniers+lkml, ojeda, rust-for-linux, tmgross
In-Reply-To: <D8HVNR3Q3UL8.1007IZIZTQ0XB@proton.me>
On Sun, Mar 16, 2025 at 05:47:40PM +0000, Benno Lossin wrote:
> On Thu Mar 13, 2025 at 3:57 AM CET, Antonio Hickey wrote:
> > When you build the kernel using the llvm-19.1.4-rust-1.83.0-x86_64
> > toolchain provided by kernel.org with ARCH=x86_64, the following symbol
> > is generated:
> >
> > $nm vmlinux | grep ' _R' | rustfilt | rg UserSliceWriter
> > ffffffff817c3390 T <kernel::uaccess::UserSliceWriter>::write_slice
> >
> > However, this Rust symbol is a trivial wrapper around the function
> > copy_to_user. It doesn't make sense to go through a trivial wrapper
> > for this function, so mark it inline.
> >
> > After applying this patch, the above command will produce no output.
> >
> > Suggested-by: Alice Ryhl <aliceryhl@google.com>
> > Link: https://github.com/Rust-for-Linux/linux/issues/1145
> > Signed-off-by: Antonio Hickey <contact@antoniohickey.com>
>
> What about the other methods (like `write` and `read`?) in this file?
Hey Benno,
The other methods in this file were handled with the patch
linked below. This was one of my first patches, so I was
unaware of patch sets and did 2 seperate patches.
Link to other patch: https://lore.kernel.org/all/010001958798b97c-4da7647e-d0bc-4f81-9132-ad24353139cb-000000@email.amazonses.com/
Do you think it would be best to send these as new patch which
includes both of these patches? also if so would it be ok to
start that new patch set at v1 ?
Sorry for the confusion I'm new to kernel dev and patches,
but starting to get the hang of it now.
Thanks,
Antonio
>
> ---
> Cheers,
> Benno
>
^ permalink raw reply
* Re: [PATCH v2 2/3] rust: alloc: add Vec::resize method
From: Benno Lossin @ 2025-03-16 18:01 UTC (permalink / raw)
To: Andrew Ballance, dakr, airlied, simona, maarten.lankhorst,
mripard, tzimmermann, corbet, ojeda, alex.gaynor, boqun.feng,
gary, bjorn3_gh, a.hindborg, aliceryhl, tmgross, acourbot,
nouveau, dri-devel, linux-doc, linux-kernel, rust-for-linux
In-Reply-To: <20250316111644.154602-3-andrewjballance@gmail.com>
On Sun Mar 16, 2025 at 12:16 PM CET, Andrew Ballance wrote:
> implement the equivalent of the rust std's Vec::resize
> on the kernel's Vec type.
>
> Signed-off-by: Andrew Ballance <andrewjballance@gmail.com>
Reviewed-by: Benno Lossin <benno.lossin@proton.me>
---
Cheers,
Benno
> ---
> rust/kernel/alloc/kvec.rs | 26 ++++++++++++++++++++++++++
> 1 file changed, 26 insertions(+)
^ permalink raw reply
* Re: [PATCH v2] rust: uaccess: mark UserSliceWriter method inline
From: Benno Lossin @ 2025-03-16 17:47 UTC (permalink / raw)
To: Antonio Hickey, Miguel Ojeda, Alex Gaynor, Boqun Feng, Gary Guo,
Björn Roy Baron, Andreas Hindborg, Alice Ryhl, Trevor Gross,
Danilo Krummrich, Nathan Chancellor, Nick Desaulniers,
Bill Wendling, Justin Stitt
Cc: linux-kernel, llvm, rust-for-linux
In-Reply-To: <010001958d6f36b2-fb0a1710-a581-4002-889e-e489004bb72d-000000@email.amazonses.com>
On Thu Mar 13, 2025 at 3:57 AM CET, Antonio Hickey wrote:
> When you build the kernel using the llvm-19.1.4-rust-1.83.0-x86_64
> toolchain provided by kernel.org with ARCH=x86_64, the following symbol
> is generated:
>
> $nm vmlinux | grep ' _R' | rustfilt | rg UserSliceWriter
> ffffffff817c3390 T <kernel::uaccess::UserSliceWriter>::write_slice
>
> However, this Rust symbol is a trivial wrapper around the function
> copy_to_user. It doesn't make sense to go through a trivial wrapper
> for this function, so mark it inline.
>
> After applying this patch, the above command will produce no output.
>
> Suggested-by: Alice Ryhl <aliceryhl@google.com>
> Link: https://github.com/Rust-for-Linux/linux/issues/1145
> Signed-off-by: Antonio Hickey <contact@antoniohickey.com>
What about the other methods (like `write` and `read`?) in this file?
---
Cheers,
Benno
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox