Implement `HasWork::work_container_of` in `impl_has_work!`, narrowing
the interface of `HasWork` and replacing pointer arithmetic with
`container_of!`. Remove the provided implementation of
`HasWork::get_work_offset` without replacement; an implementation is
already generated in `impl_has_work!`. Remove the `Self: Sized` bound on
`HasWork::work_container_of` which was apparently necessary to access
`OFFSET` as `OFFSET` no longer exists.
A similar API change was discussed on the hrtimer series[1].
Link: https://lore.kernel.org/all/20250224-hrtimer-v3-v6-12-rc2-v9-1-5bd3bf0ce6cc@kernel.org/ [1]
Signed-off-by: Tamir Duberstein <tamird@gmail.com>
---
rust/kernel/workqueue.rs | 45 ++++++++++++---------------------------------
1 file changed, 12 insertions(+), 33 deletions(-)
diff --git a/rust/kernel/workqueue.rs b/rust/kernel/workqueue.rs
index 0cd100d2aefb..0e2e0ecc58a6 100644
--- a/rust/kernel/workqueue.rs
+++ b/rust/kernel/workqueue.rs
@@ -429,51 +429,23 @@ pub unsafe fn raw_get(ptr: *const Self) -> *mut bindings::work_struct {
///
/// # Safety
///
-/// The [`OFFSET`] constant must be the offset of a field in `Self` of type [`Work<T, ID>`]. The
-/// methods on this trait must have exactly the behavior that the definitions given below have.
+/// The methods on this trait must have exactly the behavior that the definitions given below have.
///
/// [`impl_has_work!`]: crate::impl_has_work
-/// [`OFFSET`]: HasWork::OFFSET
pub unsafe trait HasWork<T, const ID: u64 = 0> {
- /// The offset of the [`Work<T, ID>`] field.
- const OFFSET: usize;
-
- /// Returns the offset of the [`Work<T, ID>`] field.
- ///
- /// This method exists because the [`OFFSET`] constant cannot be accessed if the type is not
- /// [`Sized`].
- ///
- /// [`OFFSET`]: HasWork::OFFSET
- #[inline]
- fn get_work_offset(&self) -> usize {
- Self::OFFSET
- }
-
/// Returns a pointer to the [`Work<T, ID>`] field.
///
/// # Safety
///
/// The provided pointer must point at a valid struct of type `Self`.
- #[inline]
- unsafe fn raw_get_work(ptr: *mut Self) -> *mut Work<T, ID> {
- // SAFETY: The caller promises that the pointer is valid.
- unsafe { (ptr as *mut u8).add(Self::OFFSET) as *mut Work<T, ID> }
- }
+ unsafe fn raw_get_work(ptr: *mut Self) -> *mut Work<T, ID>;
/// Returns a pointer to the struct containing the [`Work<T, ID>`] field.
///
/// # Safety
///
/// The pointer must point at a [`Work<T, ID>`] field in a struct of type `Self`.
- #[inline]
- unsafe fn work_container_of(ptr: *mut Work<T, ID>) -> *mut Self
- where
- Self: Sized,
- {
- // SAFETY: The caller promises that the pointer points at a field of the right type in the
- // right kind of struct.
- unsafe { (ptr as *mut u8).sub(Self::OFFSET) as *mut Self }
- }
+ unsafe fn work_container_of(ptr: *mut Work<T, ID>) -> *mut Self;
}
/// Used to safely implement the [`HasWork<T, ID>`] trait.
@@ -504,8 +476,6 @@ macro_rules! impl_has_work {
// SAFETY: The implementation of `raw_get_work` only compiles if the field has the right
// type.
unsafe impl$(<$($generics)+>)? $crate::workqueue::HasWork<$work_type $(, $id)?> for $self {
- const OFFSET: usize = ::core::mem::offset_of!(Self, $field) as usize;
-
#[inline]
unsafe fn raw_get_work(ptr: *mut Self) -> *mut $crate::workqueue::Work<$work_type $(, $id)?> {
// SAFETY: The caller promises that the pointer is not dangling.
@@ -513,6 +483,15 @@ unsafe fn raw_get_work(ptr: *mut Self) -> *mut $crate::workqueue::Work<$work_typ
::core::ptr::addr_of_mut!((*ptr).$field)
}
}
+
+ #[inline]
+ unsafe fn work_container_of(
+ ptr: *mut $crate::workqueue::Work<$work_type $(, $id)?>,
+ ) -> *mut Self {
+ // SAFETY: The caller promises that the pointer points at a field of the right type
+ // in the right kind of struct.
+ unsafe { $crate::container_of!(ptr, Self, $field) }
+ }
}
)*};
}
--
2.48.1On Fri, Mar 07, 2025 at 04:58:49PM -0500, Tamir Duberstein wrote: > Implement `HasWork::work_container_of` in `impl_has_work!`, narrowing > the interface of `HasWork` and replacing pointer arithmetic with > `container_of!`. Remove the provided implementation of > `HasWork::get_work_offset` without replacement; an implementation is > already generated in `impl_has_work!`. Remove the `Self: Sized` bound on > `HasWork::work_container_of` which was apparently necessary to access > `OFFSET` as `OFFSET` no longer exists. > > A similar API change was discussed on the hrtimer series[1]. > > Link: https://lore.kernel.org/all/20250224-hrtimer-v3-v6-12-rc2-v9-1-5bd3bf0ce6cc@kernel.org/ [1] > Signed-off-by: Tamir Duberstein <tamird@gmail.com> Overall looks good to me, but please CC the WORKQUEUE maintainers on the next version. Reviewed-by: Alice Ryhl <aliceryhl@google.com> Rust Binder still builds with this change: Tested-by: Alice Ryhl <aliceryhl@google.com> > - where > - Self: Sized, I did have trait object support in mind when I wrote these abstractions, but I don't actually need it and I don't think I actually got it working with trait objects. Alice
On Mon, Mar 17, 2025 at 7:34 AM Alice Ryhl <aliceryhl@google.com> wrote: > > On Fri, Mar 07, 2025 at 04:58:49PM -0500, Tamir Duberstein wrote: > > Implement `HasWork::work_container_of` in `impl_has_work!`, narrowing > > the interface of `HasWork` and replacing pointer arithmetic with > > `container_of!`. Remove the provided implementation of > > `HasWork::get_work_offset` without replacement; an implementation is > > already generated in `impl_has_work!`. Remove the `Self: Sized` bound on > > `HasWork::work_container_of` which was apparently necessary to access > > `OFFSET` as `OFFSET` no longer exists. > > > > A similar API change was discussed on the hrtimer series[1]. > > > > Link: https://lore.kernel.org/all/20250224-hrtimer-v3-v6-12-rc2-v9-1-5bd3bf0ce6cc@kernel.org/ [1] > > Signed-off-by: Tamir Duberstein <tamird@gmail.com> > > Overall looks good to me, but please CC the WORKQUEUE maintainers on the > next version. > > Reviewed-by: Alice Ryhl <aliceryhl@google.com> > > Rust Binder still builds with this change: > > Tested-by: Alice Ryhl <aliceryhl@google.com> > > > - where > > - Self: Sized, > > I did have trait object support in mind when I wrote these abstractions, > but I don't actually need it and I don't think I actually got it working > with trait objects. > > Alice Thanks! Does there need to be another version? No changes have been requested.
On Mon, Mar 17, 2025 at 07:35:55AM -0400, Tamir Duberstein wrote:
> On Mon, Mar 17, 2025 at 7:34 AM Alice Ryhl <aliceryhl@google.com> wrote:
> >
> > Overall looks good to me, but please CC the WORKQUEUE maintainers on the
> > next version.
>
> Thanks! Does there need to be another version? No changes have been requested.
Yes, it needs a new version to fix conflicts with commit 7b948a2af6b5
("rust: pci: fix unrestricted &mut pci::Device") that landed in the
merge window just now. More generally, a new version is also recommended
if maintainers are missing in the CC list.
As an FYI, I'm looking to do some additional work on the workqueue
abstractions to support delayed work items for use by GPU drivers. Since
I agree with this change, I'll base my work on top of your series.
Alice
On Fri Mar 7, 2025 at 10:58 PM CET, Tamir Duberstein wrote:
> Implement `HasWork::work_container_of` in `impl_has_work!`, narrowing
> the interface of `HasWork` and replacing pointer arithmetic with
> `container_of!`. Remove the provided implementation of
> `HasWork::get_work_offset` without replacement; an implementation is
> already generated in `impl_has_work!`. Remove the `Self: Sized` bound on
> `HasWork::work_container_of` which was apparently necessary to access
> `OFFSET` as `OFFSET` no longer exists.
>
> A similar API change was discussed on the hrtimer series[1].
>
> Link: https://lore.kernel.org/all/20250224-hrtimer-v3-v6-12-rc2-v9-1-5bd3bf0ce6cc@kernel.org/ [1]
> Signed-off-by: Tamir Duberstein <tamird@gmail.com>
> ---
> rust/kernel/workqueue.rs | 45 ++++++++++++---------------------------------
> 1 file changed, 12 insertions(+), 33 deletions(-)
What is the motivation of this change? I didn't follow the discussion,
so if you explained it there, it would be nice if you could also add it
to this commit message.
> diff --git a/rust/kernel/workqueue.rs b/rust/kernel/workqueue.rs
> index 0cd100d2aefb..0e2e0ecc58a6 100644
> --- a/rust/kernel/workqueue.rs
> +++ b/rust/kernel/workqueue.rs
> @@ -429,51 +429,23 @@ pub unsafe fn raw_get(ptr: *const Self) -> *mut bindings::work_struct {
> ///
> /// # Safety
> ///
> -/// The [`OFFSET`] constant must be the offset of a field in `Self` of type [`Work<T, ID>`]. The
> -/// methods on this trait must have exactly the behavior that the definitions given below have.
> +/// The methods on this trait must have exactly the behavior that the definitions given below have.
> ///
> /// [`impl_has_work!`]: crate::impl_has_work
> -/// [`OFFSET`]: HasWork::OFFSET
> pub unsafe trait HasWork<T, const ID: u64 = 0> {
> - /// The offset of the [`Work<T, ID>`] field.
> - const OFFSET: usize;
> -
> - /// Returns the offset of the [`Work<T, ID>`] field.
> - ///
> - /// This method exists because the [`OFFSET`] constant cannot be accessed if the type is not
> - /// [`Sized`].
> - ///
> - /// [`OFFSET`]: HasWork::OFFSET
> - #[inline]
> - fn get_work_offset(&self) -> usize {
> - Self::OFFSET
> - }
> -
> /// Returns a pointer to the [`Work<T, ID>`] field.
> ///
> /// # Safety
> ///
> /// The provided pointer must point at a valid struct of type `Self`.
> - #[inline]
> - unsafe fn raw_get_work(ptr: *mut Self) -> *mut Work<T, ID> {
> - // SAFETY: The caller promises that the pointer is valid.
> - unsafe { (ptr as *mut u8).add(Self::OFFSET) as *mut Work<T, ID> }
> - }
> + unsafe fn raw_get_work(ptr: *mut Self) -> *mut Work<T, ID>;
>
> /// Returns a pointer to the struct containing the [`Work<T, ID>`] field.
> ///
> /// # Safety
> ///
> /// The pointer must point at a [`Work<T, ID>`] field in a struct of type `Self`.
> - #[inline]
> - unsafe fn work_container_of(ptr: *mut Work<T, ID>) -> *mut Self
> - where
> - Self: Sized,
This bound is required in order to allow the usage of `dyn HasWork` (ie
object safety), so it should stay.
Maybe add a comment explaining why it's there.
---
Cheers,
Benno
> - {
> - // SAFETY: The caller promises that the pointer points at a field of the right type in the
> - // right kind of struct.
> - unsafe { (ptr as *mut u8).sub(Self::OFFSET) as *mut Self }
> - }
> + unsafe fn work_container_of(ptr: *mut Work<T, ID>) -> *mut Self;
> }
>
> /// Used to safely implement the [`HasWork<T, ID>`] trait.
> @@ -504,8 +476,6 @@ macro_rules! impl_has_work {
> // SAFETY: The implementation of `raw_get_work` only compiles if the field has the right
> // type.
> unsafe impl$(<$($generics)+>)? $crate::workqueue::HasWork<$work_type $(, $id)?> for $self {
> - const OFFSET: usize = ::core::mem::offset_of!(Self, $field) as usize;
> -
> #[inline]
> unsafe fn raw_get_work(ptr: *mut Self) -> *mut $crate::workqueue::Work<$work_type $(, $id)?> {
> // SAFETY: The caller promises that the pointer is not dangling.
> @@ -513,6 +483,15 @@ unsafe fn raw_get_work(ptr: *mut Self) -> *mut $crate::workqueue::Work<$work_typ
> ::core::ptr::addr_of_mut!((*ptr).$field)
> }
> }
> +
> + #[inline]
> + unsafe fn work_container_of(
> + ptr: *mut $crate::workqueue::Work<$work_type $(, $id)?>,
> + ) -> *mut Self {
> + // SAFETY: The caller promises that the pointer points at a field of the right type
> + // in the right kind of struct.
> + unsafe { $crate::container_of!(ptr, Self, $field) }
> + }
> }
> )*};
> }
On Fri, Mar 14, 2025 at 3:20 PM Benno Lossin <benno.lossin@proton.me> wrote:
>
> On Fri Mar 7, 2025 at 10:58 PM CET, Tamir Duberstein wrote:
> > Implement `HasWork::work_container_of` in `impl_has_work!`, narrowing
> > the interface of `HasWork` and replacing pointer arithmetic with
> > `container_of!`. Remove the provided implementation of
> > `HasWork::get_work_offset` without replacement; an implementation is
> > already generated in `impl_has_work!`. Remove the `Self: Sized` bound on
> > `HasWork::work_container_of` which was apparently necessary to access
> > `OFFSET` as `OFFSET` no longer exists.
> >
> > A similar API change was discussed on the hrtimer series[1].
> >
> > Link: https://lore.kernel.org/all/20250224-hrtimer-v3-v6-12-rc2-v9-1-5bd3bf0ce6cc@kernel.org/ [1]
> > Signed-off-by: Tamir Duberstein <tamird@gmail.com>
> > ---
> > rust/kernel/workqueue.rs | 45 ++++++++++++---------------------------------
> > 1 file changed, 12 insertions(+), 33 deletions(-)
>
> What is the motivation of this change? I didn't follow the discussion,
> so if you explained it there, it would be nice if you could also add it
> to this commit message.
The motivation is right at the top: it narrows the interface and
replaces pointer arithmetic with an existing macro, and then deletes
unnecessary code.
> > diff --git a/rust/kernel/workqueue.rs b/rust/kernel/workqueue.rs
> > index 0cd100d2aefb..0e2e0ecc58a6 100644
> > --- a/rust/kernel/workqueue.rs
> > +++ b/rust/kernel/workqueue.rs
> > @@ -429,51 +429,23 @@ pub unsafe fn raw_get(ptr: *const Self) -> *mut bindings::work_struct {
> > ///
> > /// # Safety
> > ///
> > -/// The [`OFFSET`] constant must be the offset of a field in `Self` of type [`Work<T, ID>`]. The
> > -/// methods on this trait must have exactly the behavior that the definitions given below have.
> > +/// The methods on this trait must have exactly the behavior that the definitions given below have.
> > ///
> > /// [`impl_has_work!`]: crate::impl_has_work
> > -/// [`OFFSET`]: HasWork::OFFSET
> > pub unsafe trait HasWork<T, const ID: u64 = 0> {
> > - /// The offset of the [`Work<T, ID>`] field.
> > - const OFFSET: usize;
> > -
> > - /// Returns the offset of the [`Work<T, ID>`] field.
> > - ///
> > - /// This method exists because the [`OFFSET`] constant cannot be accessed if the type is not
> > - /// [`Sized`].
> > - ///
> > - /// [`OFFSET`]: HasWork::OFFSET
> > - #[inline]
> > - fn get_work_offset(&self) -> usize {
> > - Self::OFFSET
> > - }
> > -
> > /// Returns a pointer to the [`Work<T, ID>`] field.
> > ///
> > /// # Safety
> > ///
> > /// The provided pointer must point at a valid struct of type `Self`.
> > - #[inline]
> > - unsafe fn raw_get_work(ptr: *mut Self) -> *mut Work<T, ID> {
> > - // SAFETY: The caller promises that the pointer is valid.
> > - unsafe { (ptr as *mut u8).add(Self::OFFSET) as *mut Work<T, ID> }
> > - }
> > + unsafe fn raw_get_work(ptr: *mut Self) -> *mut Work<T, ID>;
> >
> > /// Returns a pointer to the struct containing the [`Work<T, ID>`] field.
> > ///
> > /// # Safety
> > ///
> > /// The pointer must point at a [`Work<T, ID>`] field in a struct of type `Self`.
> > - #[inline]
> > - unsafe fn work_container_of(ptr: *mut Work<T, ID>) -> *mut Self
> > - where
> > - Self: Sized,
>
> This bound is required in order to allow the usage of `dyn HasWork` (ie
> object safety), so it should stay.
>
> Maybe add a comment explaining why it's there.
I guess a doctest would be better, but I still don't understand why
the bound is needed. Sorry, can you cite something or explain in more
detail please?
On Fri Mar 14, 2025 at 9:44 PM CET, Tamir Duberstein wrote:
> On Fri, Mar 14, 2025 at 3:20 PM Benno Lossin <benno.lossin@proton.me> wrote:
>>
>> On Fri Mar 7, 2025 at 10:58 PM CET, Tamir Duberstein wrote:
>> > Implement `HasWork::work_container_of` in `impl_has_work!`, narrowing
>> > the interface of `HasWork` and replacing pointer arithmetic with
>> > `container_of!`. Remove the provided implementation of
>> > `HasWork::get_work_offset` without replacement; an implementation is
>> > already generated in `impl_has_work!`. Remove the `Self: Sized` bound on
>> > `HasWork::work_container_of` which was apparently necessary to access
>> > `OFFSET` as `OFFSET` no longer exists.
>> >
>> > A similar API change was discussed on the hrtimer series[1].
>> >
>> > Link: https://lore.kernel.org/all/20250224-hrtimer-v3-v6-12-rc2-v9-1-5bd3bf0ce6cc@kernel.org/ [1]
>> > Signed-off-by: Tamir Duberstein <tamird@gmail.com>
>> > ---
>> > rust/kernel/workqueue.rs | 45 ++++++++++++---------------------------------
>> > 1 file changed, 12 insertions(+), 33 deletions(-)
>>
>> What is the motivation of this change? I didn't follow the discussion,
>> so if you explained it there, it would be nice if you could also add it
>> to this commit message.
>
> The motivation is right at the top: it narrows the interface and
> replaces pointer arithmetic with an existing macro, and then deletes
> unnecessary code.
>
>> > diff --git a/rust/kernel/workqueue.rs b/rust/kernel/workqueue.rs
>> > index 0cd100d2aefb..0e2e0ecc58a6 100644
>> > --- a/rust/kernel/workqueue.rs
>> > +++ b/rust/kernel/workqueue.rs
>> > @@ -429,51 +429,23 @@ pub unsafe fn raw_get(ptr: *const Self) -> *mut bindings::work_struct {
>> > ///
>> > /// # Safety
>> > ///
>> > -/// The [`OFFSET`] constant must be the offset of a field in `Self` of type [`Work<T, ID>`]. The
>> > -/// methods on this trait must have exactly the behavior that the definitions given below have.
>> > +/// The methods on this trait must have exactly the behavior that the definitions given below have.
>> > ///
>> > /// [`impl_has_work!`]: crate::impl_has_work
>> > -/// [`OFFSET`]: HasWork::OFFSET
>> > pub unsafe trait HasWork<T, const ID: u64 = 0> {
>> > - /// The offset of the [`Work<T, ID>`] field.
>> > - const OFFSET: usize;
>> > -
>> > - /// Returns the offset of the [`Work<T, ID>`] field.
>> > - ///
>> > - /// This method exists because the [`OFFSET`] constant cannot be accessed if the type is not
>> > - /// [`Sized`].
>> > - ///
>> > - /// [`OFFSET`]: HasWork::OFFSET
>> > - #[inline]
>> > - fn get_work_offset(&self) -> usize {
>> > - Self::OFFSET
>> > - }
>> > -
>> > /// Returns a pointer to the [`Work<T, ID>`] field.
>> > ///
>> > /// # Safety
>> > ///
>> > /// The provided pointer must point at a valid struct of type `Self`.
>> > - #[inline]
>> > - unsafe fn raw_get_work(ptr: *mut Self) -> *mut Work<T, ID> {
>> > - // SAFETY: The caller promises that the pointer is valid.
>> > - unsafe { (ptr as *mut u8).add(Self::OFFSET) as *mut Work<T, ID> }
>> > - }
>> > + unsafe fn raw_get_work(ptr: *mut Self) -> *mut Work<T, ID>;
>> >
>> > /// Returns a pointer to the struct containing the [`Work<T, ID>`] field.
>> > ///
>> > /// # Safety
>> > ///
>> > /// The pointer must point at a [`Work<T, ID>`] field in a struct of type `Self`.
>> > - #[inline]
>> > - unsafe fn work_container_of(ptr: *mut Work<T, ID>) -> *mut Self
>> > - where
>> > - Self: Sized,
>>
>> This bound is required in order to allow the usage of `dyn HasWork` (ie
>> object safety), so it should stay.
>>
>> Maybe add a comment explaining why it's there.
>
> I guess a doctest would be better, but I still don't understand why
> the bound is needed. Sorry, can you cite something or explain in more
> detail please?
Here is a link: https://doc.rust-lang.org/reference/items/traits.html#dyn-compatibility
But I realized that the trait wasn't object safe to begin with due to
the `OFFSET` associated constant. So I'm not sure we need this. Alice,
do you need `dyn HasWork`?
---
Cheers,
Benno
On Sat, Mar 15, 2025 at 5:30 AM Benno Lossin <benno.lossin@proton.me> wrote:
>
> On Fri Mar 14, 2025 at 9:44 PM CET, Tamir Duberstein wrote:
> > On Fri, Mar 14, 2025 at 3:20 PM Benno Lossin <benno.lossin@proton.me> wrote:
> >>
> >> On Fri Mar 7, 2025 at 10:58 PM CET, Tamir Duberstein wrote:
> >> > Implement `HasWork::work_container_of` in `impl_has_work!`, narrowing
> >> > the interface of `HasWork` and replacing pointer arithmetic with
> >> > `container_of!`. Remove the provided implementation of
> >> > `HasWork::get_work_offset` without replacement; an implementation is
> >> > already generated in `impl_has_work!`. Remove the `Self: Sized` bound on
> >> > `HasWork::work_container_of` which was apparently necessary to access
> >> > `OFFSET` as `OFFSET` no longer exists.
> >> >
> >> > A similar API change was discussed on the hrtimer series[1].
> >> >
> >> > Link: https://lore.kernel.org/all/20250224-hrtimer-v3-v6-12-rc2-v9-1-5bd3bf0ce6cc@kernel.org/ [1]
> >> > Signed-off-by: Tamir Duberstein <tamird@gmail.com>
> >> > ---
> >> > rust/kernel/workqueue.rs | 45 ++++++++++++---------------------------------
> >> > 1 file changed, 12 insertions(+), 33 deletions(-)
> >>
> >> What is the motivation of this change? I didn't follow the discussion,
> >> so if you explained it there, it would be nice if you could also add it
> >> to this commit message.
> >
> > The motivation is right at the top: it narrows the interface and
> > replaces pointer arithmetic with an existing macro, and then deletes
> > unnecessary code.
> >
> >> > diff --git a/rust/kernel/workqueue.rs b/rust/kernel/workqueue.rs
> >> > index 0cd100d2aefb..0e2e0ecc58a6 100644
> >> > --- a/rust/kernel/workqueue.rs
> >> > +++ b/rust/kernel/workqueue.rs
> >> > @@ -429,51 +429,23 @@ pub unsafe fn raw_get(ptr: *const Self) -> *mut bindings::work_struct {
> >> > ///
> >> > /// # Safety
> >> > ///
> >> > -/// The [`OFFSET`] constant must be the offset of a field in `Self` of type [`Work<T, ID>`]. The
> >> > -/// methods on this trait must have exactly the behavior that the definitions given below have.
> >> > +/// The methods on this trait must have exactly the behavior that the definitions given below have.
> >> > ///
> >> > /// [`impl_has_work!`]: crate::impl_has_work
> >> > -/// [`OFFSET`]: HasWork::OFFSET
> >> > pub unsafe trait HasWork<T, const ID: u64 = 0> {
> >> > - /// The offset of the [`Work<T, ID>`] field.
> >> > - const OFFSET: usize;
> >> > -
> >> > - /// Returns the offset of the [`Work<T, ID>`] field.
> >> > - ///
> >> > - /// This method exists because the [`OFFSET`] constant cannot be accessed if the type is not
> >> > - /// [`Sized`].
> >> > - ///
> >> > - /// [`OFFSET`]: HasWork::OFFSET
> >> > - #[inline]
> >> > - fn get_work_offset(&self) -> usize {
> >> > - Self::OFFSET
> >> > - }
> >> > -
> >> > /// Returns a pointer to the [`Work<T, ID>`] field.
> >> > ///
> >> > /// # Safety
> >> > ///
> >> > /// The provided pointer must point at a valid struct of type `Self`.
> >> > - #[inline]
> >> > - unsafe fn raw_get_work(ptr: *mut Self) -> *mut Work<T, ID> {
> >> > - // SAFETY: The caller promises that the pointer is valid.
> >> > - unsafe { (ptr as *mut u8).add(Self::OFFSET) as *mut Work<T, ID> }
> >> > - }
> >> > + unsafe fn raw_get_work(ptr: *mut Self) -> *mut Work<T, ID>;
> >> >
> >> > /// Returns a pointer to the struct containing the [`Work<T, ID>`] field.
> >> > ///
> >> > /// # Safety
> >> > ///
> >> > /// The pointer must point at a [`Work<T, ID>`] field in a struct of type `Self`.
> >> > - #[inline]
> >> > - unsafe fn work_container_of(ptr: *mut Work<T, ID>) -> *mut Self
> >> > - where
> >> > - Self: Sized,
> >>
> >> This bound is required in order to allow the usage of `dyn HasWork` (ie
> >> object safety), so it should stay.
> >>
> >> Maybe add a comment explaining why it's there.
> >
> > I guess a doctest would be better, but I still don't understand why
> > the bound is needed. Sorry, can you cite something or explain in more
> > detail please?
>
> Here is a link: https://doc.rust-lang.org/reference/items/traits.html#dyn-compatibility
>
> But I realized that the trait wasn't object safe to begin with due to
> the `OFFSET` associated constant. So I'm not sure we need this. Alice,
> do you need `dyn HasWork`?
I wrote a simple test:
diff --git a/rust/kernel/workqueue.rs b/rust/kernel/workqueue.rs
index 0e2e0ecc58a6..4f2dd2c1ebcb 100644
--- a/rust/kernel/workqueue.rs
+++ b/rust/kernel/workqueue.rs
@@ -448,6 +448,11 @@ pub unsafe trait HasWork<T, const ID: u64 = 0> {
unsafe fn work_container_of(ptr: *mut Work<T, ID>) -> *mut Self;
}
+fn has_work_object_safe<T: HasWork<T>>(has_work: T) {
+ fn _assert_object_safe(_: &dyn HasWork<()>) {}
+ _assert_object_safe(&has_work);
+}
+
/// Used to safely implement the [`HasWork<T, ID>`] trait.
///
/// # Examples
`HasWork` is not object-safe even before this patch:
> error[E0038]: the trait `workqueue::HasWork` cannot be made into an object
> --> ../rust/kernel/workqueue.rs:481:25
> |
> 481 | _assert_object_safe(&has_work);
> | ^^^^^^^^^ `workqueue::HasWork` cannot be made into an object
> |
> note: for a trait to be "dyn-compatible" it needs to allow building a vtable to allow the call to be resolvable dynamically; for more information visit <https://doc.rust-lang.org/reference/items/traits.html#object-safety>
> --> ../rust/kernel/workqueue.rs:439:11
> |
> 437 | pub unsafe trait HasWork<T, const ID: u64 = 0> {
> | ------- this trait cannot be made into an object...
> 438 | /// The offset of the [`Work<T, ID>`] field.
> 439 | const OFFSET: usize;
> | ^^^^^^ ...because it contains this associated `const`
> ...
> 458 | unsafe fn raw_get_work(ptr: *mut Self) -> *mut Work<T, ID> {
> | ^^^^^^^^^^^^ ...because associated function `raw_get_work` has no `self` parameter
> = help: consider moving `OFFSET` to another trait
> = help: only type `workqueue::ClosureWork<T>` is seen to implement the trait in this crate, consider using it directly instead
> = note: `workqueue::HasWork` can be implemented in other crates; if you want to support your users passing their own types here, you can't refer to a specific type
> help: consider turning `raw_get_work` into a method by giving it a `&self` argument
> |
> 458 | unsafe fn raw_get_work(&self, ptr: *mut Self) -> *mut Work<T, ID> {
> | ++++++
> help: alternatively, consider constraining `raw_get_work` so it does not apply to trait objects
> |
> 458 | unsafe fn raw_get_work(ptr: *mut Self) -> *mut Work<T, ID> where Self: Sized {
> | +++++++++++++++++
>
> error: aborting due to 3 previous errors
so I don't think adding the Sized bound makes sense - we'd end up
adding it on every item in the trait.
On Sat Mar 15, 2025 at 4:37 PM CET, Tamir Duberstein wrote: > On Sat, Mar 15, 2025 at 5:30 AM Benno Lossin <benno.lossin@proton.me> wrote: >> >> On Fri Mar 14, 2025 at 9:44 PM CET, Tamir Duberstein wrote: >> > On Fri, Mar 14, 2025 at 3:20 PM Benno Lossin <benno.lossin@proton.me> wrote: >> >> >> >> On Fri Mar 7, 2025 at 10:58 PM CET, Tamir Duberstein wrote: >> >> > /// Returns a pointer to the struct containing the [`Work<T, ID>`] field. >> >> > /// >> >> > /// # Safety >> >> > /// >> >> > /// The pointer must point at a [`Work<T, ID>`] field in a struct of type `Self`. >> >> > - #[inline] >> >> > - unsafe fn work_container_of(ptr: *mut Work<T, ID>) -> *mut Self >> >> > - where >> >> > - Self: Sized, >> >> >> >> This bound is required in order to allow the usage of `dyn HasWork` (ie >> >> object safety), so it should stay. >> >> >> >> Maybe add a comment explaining why it's there. >> > >> > I guess a doctest would be better, but I still don't understand why >> > the bound is needed. Sorry, can you cite something or explain in more >> > detail please? >> >> Here is a link: https://doc.rust-lang.org/reference/items/traits.html#dyn-compatibility >> >> But I realized that the trait wasn't object safe to begin with due to >> the `OFFSET` associated constant. So I'm not sure we need this. Alice, >> do you need `dyn HasWork`? > > I wrote a simple test: [...] > so I don't think adding the Sized bound makes sense - we'd end up > adding it on every item in the trait. Yeah the `Sized` bound was probably to make the cast work, so let's remove it. --- Cheers, Benno
On Sat, Mar 15, 2025 at 2:06 PM Benno Lossin <benno.lossin@proton.me> wrote: > > On Sat Mar 15, 2025 at 4:37 PM CET, Tamir Duberstein wrote: > > On Sat, Mar 15, 2025 at 5:30 AM Benno Lossin <benno.lossin@proton.me> wrote: > >> > >> On Fri Mar 14, 2025 at 9:44 PM CET, Tamir Duberstein wrote: > >> > On Fri, Mar 14, 2025 at 3:20 PM Benno Lossin <benno.lossin@proton.me> wrote: > >> >> > >> >> On Fri Mar 7, 2025 at 10:58 PM CET, Tamir Duberstein wrote: > >> >> > /// Returns a pointer to the struct containing the [`Work<T, ID>`] field. > >> >> > /// > >> >> > /// # Safety > >> >> > /// > >> >> > /// The pointer must point at a [`Work<T, ID>`] field in a struct of type `Self`. > >> >> > - #[inline] > >> >> > - unsafe fn work_container_of(ptr: *mut Work<T, ID>) -> *mut Self > >> >> > - where > >> >> > - Self: Sized, > >> >> > >> >> This bound is required in order to allow the usage of `dyn HasWork` (ie > >> >> object safety), so it should stay. > >> >> > >> >> Maybe add a comment explaining why it's there. > >> > > >> > I guess a doctest would be better, but I still don't understand why > >> > the bound is needed. Sorry, can you cite something or explain in more > >> > detail please? > >> > >> Here is a link: https://doc.rust-lang.org/reference/items/traits.html#dyn-compatibility > >> > >> But I realized that the trait wasn't object safe to begin with due to > >> the `OFFSET` associated constant. So I'm not sure we need this. Alice, > >> do you need `dyn HasWork`? > > > > I wrote a simple test: > > [...] > > > so I don't think adding the Sized bound makes sense - we'd end up > > adding it on every item in the trait. > > Yeah the `Sized` bound was probably to make the cast work, so let's > remove it. It's already removed, right?
On Sat, Mar 15, 2025 at 2:12 PM Tamir Duberstein <tamird@gmail.com> wrote: > > On Sat, Mar 15, 2025 at 2:06 PM Benno Lossin <benno.lossin@proton.me> wrote: > > > > On Sat Mar 15, 2025 at 4:37 PM CET, Tamir Duberstein wrote: > > > On Sat, Mar 15, 2025 at 5:30 AM Benno Lossin <benno.lossin@proton.me> wrote: > > >> > > >> On Fri Mar 14, 2025 at 9:44 PM CET, Tamir Duberstein wrote: > > >> > On Fri, Mar 14, 2025 at 3:20 PM Benno Lossin <benno.lossin@proton.me> wrote: > > >> >> > > >> >> On Fri Mar 7, 2025 at 10:58 PM CET, Tamir Duberstein wrote: > > >> >> > /// Returns a pointer to the struct containing the [`Work<T, ID>`] field. > > >> >> > /// > > >> >> > /// # Safety > > >> >> > /// > > >> >> > /// The pointer must point at a [`Work<T, ID>`] field in a struct of type `Self`. > > >> >> > - #[inline] > > >> >> > - unsafe fn work_container_of(ptr: *mut Work<T, ID>) -> *mut Self > > >> >> > - where > > >> >> > - Self: Sized, > > >> >> > > >> >> This bound is required in order to allow the usage of `dyn HasWork` (ie > > >> >> object safety), so it should stay. > > >> >> > > >> >> Maybe add a comment explaining why it's there. > > >> > > > >> > I guess a doctest would be better, but I still don't understand why > > >> > the bound is needed. Sorry, can you cite something or explain in more > > >> > detail please? > > >> > > >> Here is a link: https://doc.rust-lang.org/reference/items/traits.html#dyn-compatibility > > >> > > >> But I realized that the trait wasn't object safe to begin with due to > > >> the `OFFSET` associated constant. So I'm not sure we need this. Alice, > > >> do you need `dyn HasWork`? > > > > > > I wrote a simple test: > > > > [...] > > > > > so I don't think adding the Sized bound makes sense - we'd end up > > > adding it on every item in the trait. > > > > Yeah the `Sized` bound was probably to make the cast work, so let's > > remove it. > > It's already removed, right? Ping. Can you help me understand what change, if any, you think is required?
On Sun Mar 16, 2025 at 1:55 PM CET, Tamir Duberstein wrote: > On Sat, Mar 15, 2025 at 2:12 PM Tamir Duberstein <tamird@gmail.com> wrote: >> >> On Sat, Mar 15, 2025 at 2:06 PM Benno Lossin <benno.lossin@proton.me> wrote: >> > >> > On Sat Mar 15, 2025 at 4:37 PM CET, Tamir Duberstein wrote: >> > > On Sat, Mar 15, 2025 at 5:30 AM Benno Lossin <benno.lossin@proton.me> wrote: >> > >> >> > >> On Fri Mar 14, 2025 at 9:44 PM CET, Tamir Duberstein wrote: >> > >> > On Fri, Mar 14, 2025 at 3:20 PM Benno Lossin <benno.lossin@proton.me> wrote: >> > >> >> >> > >> >> On Fri Mar 7, 2025 at 10:58 PM CET, Tamir Duberstein wrote: >> > >> >> > /// Returns a pointer to the struct containing the [`Work<T, ID>`] field. >> > >> >> > /// >> > >> >> > /// # Safety >> > >> >> > /// >> > >> >> > /// The pointer must point at a [`Work<T, ID>`] field in a struct of type `Self`. >> > >> >> > - #[inline] >> > >> >> > - unsafe fn work_container_of(ptr: *mut Work<T, ID>) -> *mut Self >> > >> >> > - where >> > >> >> > - Self: Sized, >> > >> >> >> > >> >> This bound is required in order to allow the usage of `dyn HasWork` (ie >> > >> >> object safety), so it should stay. >> > >> >> >> > >> >> Maybe add a comment explaining why it's there. >> > >> > >> > >> > I guess a doctest would be better, but I still don't understand why >> > >> > the bound is needed. Sorry, can you cite something or explain in more >> > >> > detail please? >> > >> >> > >> Here is a link: https://doc.rust-lang.org/reference/items/traits.html#dyn-compatibility >> > >> >> > >> But I realized that the trait wasn't object safe to begin with due to >> > >> the `OFFSET` associated constant. So I'm not sure we need this. Alice, >> > >> do you need `dyn HasWork`? >> > > >> > > I wrote a simple test: >> > >> > [...] >> > >> > > so I don't think adding the Sized bound makes sense - we'd end up >> > > adding it on every item in the trait. >> > >> > Yeah the `Sized` bound was probably to make the cast work, so let's >> > remove it. >> >> It's already removed, right? > > Ping. Can you help me understand what change, if any, you think is required? No change required, with my reply above I intended to take my complaint away :) --- Cheers, Benno
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?
On Sun Mar 16, 2025 at 7:59 PM CET, Tamir Duberstein wrote: > 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? Ah sorry, I didn't end up adding it (: Reviewed-by: Benno Lossin <benno.lossin@proton.me> It would be good if Alice could also take a look. --- Cheers, Benno
© 2016 - 2026 Red Hat, Inc.