Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add ARef::into_raw #1044

Closed
y86-dev opened this issue Dec 2, 2023 · 12 comments · May be fixed by #1056
Closed

Add ARef::into_raw #1044

y86-dev opened this issue Dec 2, 2023 · 12 comments · May be fixed by #1056
Labels
good first issue Good for newcomers • lib Related to the `rust/` library.

Comments

@y86-dev
Copy link
Collaborator

y86-dev commented Dec 2, 2023

Add the function into_raw to ARef<T>, the function should:

  • consume the ARef<T> by value, but not run the destructor, so you need to mem::forget the ARef<T>.
  • return a NonNull<T> or a *mut T, figure out which one works better.
  • have an example and some documentation.

Also adjust instances where ARefs are manually forgotten to use this new function.


This requires submitting a proper patch to the LKML and the Rust for Linux mailing list. Please recall to test your changes, to use a proper title for the commit, to sign your commit under the Developer's Certificate of Origin and so on. Please see https://docs.kernel.org/process/submitting-patches.html and https://rust-for-linux.com/contributing for details.

Please take this issue only if you are new to the kernel development process and you would like to use it as a test to submit your first patch to the kernel.

@y86-dev y86-dev added • lib Related to the `rust/` library. good first issue Good for newcomers labels Dec 2, 2023
@Kartik1397
Copy link

I'll take this one.

@y86-dev
Copy link
Collaborator Author

y86-dev commented Dec 3, 2023

Sure!

Note that there currently are no instances of ARefs getting manually forgotten. I created this issue, while reviewing [PATCH 4/7] rust: file: add FileDescriptorReservation. So depending on which thing makes it first into upstream you will or won't have to adjust other stuff.

@Redhawk18
Copy link

I'll take this one.

Are you currently working on it?

@Kartik1397
Copy link

Yes @Redhawk18, I've the patch ready here, which I'm planning to submit in couple of days once I complete testing of changes in rust/kernel/net/filter.rs and rust/kernel/file.rs files.

@Kartik1397
Copy link

Hi @y86-dev, I've drafted a PR for initial review. Please take a look when you can.
I'll send the patch to the mailing list after that.

vincenzopalazzo added a commit to vincenzopalazzo/linux that referenced this issue Mar 24, 2024
Add the function `into_raw` to `ARef<T>`. This method
can be used to turn an `ARef` into a raw pointer.

Link: Rust-for-Linux#1044
Co-developed-by: Vincenzo Palazzo <vincenzopalazzodev@gmail.com>
Signed-off-by: Kartik Prajapati <kartikprajapati987@gmail.com>
vincenzopalazzo pushed a commit to vincenzopalazzo/linux that referenced this issue Mar 27, 2024
Add the function `into_raw` to `ARef<T>`. This method
can be used to turn an `ARef` into a raw pointer.

Link: Rust-for-Linux#1044
Co-developed-by: Vincenzo Palazzo <vincenzopalazzodev@gmail.com>
Signed-off-by: Kartik Prajapati <kartikprajapati987@gmail.com>
@vincenzopalazzo
Copy link

vincenzopalazzo pushed a commit to vincenzopalazzo/linux that referenced this issue May 30, 2024
Add the function `into_raw` to `ARef<T>`. This method
can be used to turn an `ARef` into a raw pointer.

Link: Rust-for-Linux#1044
Co-developed-by: Vincenzo Palazzo <vincenzopalazzodev@gmail.com>
Signed-off-by: Kartik Prajapati <kartikprajapati987@gmail.com>
vincenzopalazzo pushed a commit to vincenzopalazzo/linux that referenced this issue Jun 8, 2024
Add the function `into_raw` to `ARef<T>`. This method
can be used to turn an `ARef` into a raw pointer.

Link: Rust-for-Linux#1044
Co-developed-by: Vincenzo Palazzo <vincenzopalazzodev@gmail.com>
Signed-off-by: Kartik Prajapati <kartikprajapati987@gmail.com>
vincenzopalazzo pushed a commit to vincenzopalazzo/linux that referenced this issue Jul 12, 2024
Add the function `into_raw` to `ARef<T>`. This method
can be used to turn an `ARef` into a raw pointer.

Link: Rust-for-Linux#1044
Co-developed-by: Vincenzo Palazzo <vincenzopalazzodev@gmail.com>
Signed-off-by: Kartik Prajapati <kartikprajapati987@gmail.com>
@ojeda
Copy link
Member

ojeda commented Aug 20, 2024

@Kartik1397 We are thinking of using NonNull as the return type -- Alice sent an alternative patch (unaware of yours), and I think ideally we would get a combined approach with the best of both, i.e. an example like yours but returning NonNull like she did. Would you be OK to be the primary author of that approach? (i.e. with NonNull)?

https://lore.kernel.org/rust-for-linux/20240801-aref-into-raw-v1-1-33401e2fbac8@google.com/
https://lore.kernel.org/rust-for-linux/20240324231236.145086-1-vincenzopalazzodev@gmail.com/

Alice is happy sending the patch (with you as the primary author), but if you want to send it yourself, that is also fine.

Thanks!

@ojeda
Copy link
Member

ojeda commented Aug 20, 2024

Something like (from Alice):

diff --git a/rust/kernel/types.rs b/rust/kernel/types.rs
index ee7dd1f963ef..9e7ca066355c 100644
--- a/rust/kernel/types.rs
+++ b/rust/kernel/types.rs
@@ -7,7 +7,7 @@
 use core::{
     cell::UnsafeCell,
     marker::{PhantomData, PhantomPinned},
-    mem::MaybeUninit,
+    mem::{ManuallyDrop, MaybeUninit},
     ops::{Deref, DerefMut},
     pin::Pin,
     ptr::NonNull,
@@ -396,6 +396,35 @@ pub unsafe fn from_raw(ptr: NonNull<T>) -> Self {
             _p: PhantomData,
         }
     }
+
+    /// Consumes the `ARef`, returning a raw pointer.
+    ///
+    /// This function does not change the refcount. After calling this function, the caller is
+    /// responsible for the refcount previously managed by the `ARef`.
+    ///
+    /// # Examples
+    ///
+    /// ```
+    /// use core::ptr::NonNull;
+    /// use kernel::types::{ARef, AlwaysRefCounted};
+    ///
+    /// struct Empty {}
+    ///
+    /// unsafe impl AlwaysRefCounted for Empty {
+    ///     fn inc_ref(&self) {}
+    ///     unsafe fn dec_ref(_obj: NonNull<Self>) {}
+    /// }
+    ///
+    /// let mut data = Empty {};
+    /// let ptr = NonNull::<Empty>::new(&mut data as *mut _).unwrap();
+    /// let data_ref: ARef<Empty> = unsafe { ARef::from_raw(ptr) };
+    /// let raw_ptr: NonNull<Empty> = ARef::into_raw(data_ref);
+    ///
+    /// assert_eq!(ptr, raw_ptr);
+    /// ```
+    pub fn into_raw(me: Self) -> NonNull<T> {
+        ManuallyDrop::new(me).ptr
+    }

@ojeda
Copy link
Member

ojeda commented Aug 20, 2024

By the way, I think @vincenzopalazzo did not change the code when he sent @Kartik1397 patch to the mailing list. But if Vincenzo contributed to the patch and should have a Co-developed-by, then please let us know (https://lore.kernel.org/rust-for-linux/CANiq72==OBG+KVJuzW0S5aYowAZ4oq91pk=phRgtedUrGYeHAg@mail.gmail.com/). Thanks!

@vincenzopalazzo
Copy link

@ojeda you are right, I did not change the code I just push it to the finish line through the ML :)

@Kartik1397
Copy link

@ojeda I'm OK with using NonNull as the return type.
I'm also fine with Alice sending the patch with me as the primary author.

Thanks considering my patch!

Darksonn pushed a commit to Darksonn/linux that referenced this issue Aug 21, 2024
Add a method for `ARef` that is analogous to `Arc::into_raw`. It is the
inverse operation of `ARef::from_raw`, and allows you to convert the
`ARef` back into a raw pointer while retaining ownership of the
refcount.

This new function will be used by [1] for converting the type in an
`ARef` using `ARef::from_raw(ARef::into_raw(me).cast())`. The author has
also needed the same function for other use-cases in the past, but [1]
is the first to go upstream.

This was implemented independently by Kartik and Alice. The two versions
were merged by Alice, so all mistakes are Alice's.

Link: https://lore.kernel.org/r/20240801-vma-v3-1-db6c1c0afda9@google.com [1]
Closes: Rust-for-Linux#1044
Signed-off-by: Kartik Prajapati <kartikprajapati987@gmail.com>
Co-developed-by: Alice Ryhl <aliceryhl@google.com>
Signed-off-by: Alice Ryhl <aliceryhl@google.com>
Darksonn pushed a commit to Darksonn/linux that referenced this issue Aug 21, 2024
Add a method for `ARef` that is analogous to `Arc::into_raw`. It is the
inverse operation of `ARef::from_raw`, and allows you to convert the
`ARef` back into a raw pointer while retaining ownership of the
refcount.

This new function will be used by [1] for converting the type in an
`ARef` using `ARef::from_raw(ARef::into_raw(me).cast())`. The author has
also needed the same function for other use-cases in the past, but [1]
is the first to go upstream.

This was implemented independently by Kartik and Alice. The two versions
were merged by Alice, so all mistakes are Alice's.

Link: https://lore.kernel.org/r/20240801-vma-v3-1-db6c1c0afda9@google.com [1]
Closes: Rust-for-Linux#1044
Signed-off-by: Kartik Prajapati <kartikprajapati987@gmail.com>
Co-developed-by: Alice Ryhl <aliceryhl@google.com>
Signed-off-by: Alice Ryhl <aliceryhl@google.com>
Darksonn pushed a commit to Darksonn/linux that referenced this issue Aug 21, 2024
Add a method for `ARef` that is analogous to `Arc::into_raw`. It is the
inverse operation of `ARef::from_raw`, and allows you to convert the
`ARef` back into a raw pointer while retaining ownership of the
refcount.

This new function will be used by [1] for converting the type in an
`ARef` using `ARef::from_raw(ARef::into_raw(me).cast())`. The author has
also needed the same function for other use-cases in the past, but [1]
is the first to go upstream.

This was implemented independently by Kartik and Alice. The two versions
were merged by Alice, so all mistakes are Alice's.

Link: https://lore.kernel.org/r/20240801-vma-v3-1-db6c1c0afda9@google.com [1]
Closes: Rust-for-Linux#1044
Signed-off-by: Kartik Prajapati <kartikprajapati987@gmail.com>
Co-developed-by: Alice Ryhl <aliceryhl@google.com>
Signed-off-by: Alice Ryhl <aliceryhl@google.com>
intel-lab-lkp pushed a commit to intel-lab-lkp/linux that referenced this issue Aug 21, 2024
Add a method for `ARef` that is analogous to `Arc::into_raw`. It is the
inverse operation of `ARef::from_raw`, and allows you to convert the
`ARef` back into a raw pointer while retaining ownership of the
refcount.

This new function will be used by [1] for converting the type in an
`ARef` using `ARef::from_raw(ARef::into_raw(me).cast())`. The author has
also needed the same function for other use-cases in the past, but [1]
is the first to go upstream.

This was implemented independently by Kartik and Alice. The two versions
were merged by Alice, so all mistakes are Alice's.

Link: https://lore.kernel.org/r/20240801-vma-v3-1-db6c1c0afda9@google.com [1]
Closes: Rust-for-Linux#1044
Signed-off-by: Kartik Prajapati <kartikprajapati987@gmail.com>
Co-developed-by: Alice Ryhl <aliceryhl@google.com>
Signed-off-by: Alice Ryhl <aliceryhl@google.com>
ojeda pushed a commit to ojeda/linux that referenced this issue Aug 24, 2024
Add a method for `ARef` that is analogous to `Arc::into_raw`. It is the
inverse operation of `ARef::from_raw`, and allows you to convert the
`ARef` back into a raw pointer while retaining ownership of the
refcount.

This new function will be used by [1] for converting the type in an
`ARef` using `ARef::from_raw(ARef::into_raw(me).cast())`. Alice has
also needed the same function for other use-cases in the past, but [1]
is the first to go upstream.

This was implemented independently by Kartik and Alice. The two versions
were merged by Alice, so all mistakes are Alice's.

Link: https://lore.kernel.org/r/20240801-vma-v3-1-db6c1c0afda9@google.com [1]
Closes: Rust-for-Linux#1044
Signed-off-by: Kartik Prajapati <kartikprajapati987@gmail.com>
Co-developed-by: Alice Ryhl <aliceryhl@google.com>
Signed-off-by: Alice Ryhl <aliceryhl@google.com>
Reviewed-by: Benno Lossin <benno.lossin@proton.me>
[ Reworded to correct the name. - Miguel ]
Signed-off-by: Miguel Ojeda <ojeda@kernel.org>
ojeda pushed a commit that referenced this issue Aug 25, 2024
Add a method for `ARef` that is analogous to `Arc::into_raw`. It is the
inverse operation of `ARef::from_raw`, and allows you to convert the
`ARef` back into a raw pointer while retaining ownership of the
refcount.

This new function will be used by [1] for converting the type in an
`ARef` using `ARef::from_raw(ARef::into_raw(me).cast())`. Alice has
also needed the same function for other use-cases in the past, but [1]
is the first to go upstream.

This was implemented independently by Kartik and Alice. The two versions
were merged by Alice, so all mistakes are Alice's.

Link: https://lore.kernel.org/r/20240801-vma-v3-1-db6c1c0afda9@google.com [1]
Link: #1044
Signed-off-by: Kartik Prajapati <kartikprajapati987@gmail.com>
Co-developed-by: Alice Ryhl <aliceryhl@google.com>
Signed-off-by: Alice Ryhl <aliceryhl@google.com>
Reviewed-by: Benno Lossin <benno.lossin@proton.me>
[ Reworded to correct the author reference and changed tag to Link
  since it is not a bug. - Miguel ]
Signed-off-by: Miguel Ojeda <ojeda@kernel.org>
ojeda pushed a commit that referenced this issue Aug 25, 2024
Add a method for `ARef` that is analogous to `Arc::into_raw`. It is the
inverse operation of `ARef::from_raw`, and allows you to convert the
`ARef` back into a raw pointer while retaining ownership of the
refcount.

This new function will be used by [1] for converting the type in an
`ARef` using `ARef::from_raw(ARef::into_raw(me).cast())`. Alice has
also needed the same function for other use-cases in the past, but [1]
is the first to go upstream.

This was implemented independently by Kartik and Alice. The two versions
were merged by Alice, so all mistakes are Alice's.

Link: https://lore.kernel.org/r/20240801-vma-v3-1-db6c1c0afda9@google.com [1]
Link: #1044
Signed-off-by: Kartik Prajapati <kartikprajapati987@gmail.com>
Co-developed-by: Alice Ryhl <aliceryhl@google.com>
Signed-off-by: Alice Ryhl <aliceryhl@google.com>
Reviewed-by: Benno Lossin <benno.lossin@proton.me>
[ Reworded to correct the author reference and changed tag to Link
  since it is not a bug. - Miguel ]
Signed-off-by: Miguel Ojeda <ojeda@kernel.org>
@ojeda
Copy link
Member

ojeda commented Aug 25, 2024

Applied to rust-next -- thanks everyone!

@ojeda ojeda closed this as completed Aug 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Good for newcomers • lib Related to the `rust/` library.
Development

Successfully merging a pull request may close this issue.

5 participants