rust: add new api for initialization #909

y86-dev · 2022-10-07T13:17:22Z

This is my newest version of the pinned-initialization API. You can find it as a stand-alone crate here: pinned-init.

I plan to squash 276ae43 onto 50f2121. I separated the commits for better earlier review.

I also need to provide a better commit message better explaining the API (linking to my blog might help, but it is a long read, so...).

I removed all unsafe initialization API from the sync module. If we still want to keep the old API and we want to deprecate it instead just let me know.

Closes #772

rust/kernel/sync/arc.rs

wedsonaf

Just did a quick scan of the first few files and left a few comments.

This is a huge improvement over what we have now.

(I'll allocate some time to go over the rest.)

drivers/android/context.rs

wedsonaf · 2022-10-07T14:44:03Z

drivers/android/context.rs

-            // SAFETY: Init is called below.
-            manager: unsafe {
-                Mutex::new(Manager {
+        let ctx = UniqueArc::pin_init::<core::convert::Infallible>(pin_init!(Self {


The reason this was a Pin<UniqueArc> originally was because we had to initialise the mutex in it. Since we don't need this anymore, couldn't this just be an Arc?

This also feels a bit too verbose (e.g., having to specify core::convert::Infallible). I haven't looked at the details yet, but it would be great if we could avoid this.

Currently I have not implemented InPlaceInit for Arc, since it does not fit the API (you can only call pin_init on it). But since we have full control over it, we could just add it as a method. Will do so 👍

And yeah sadly we have to specify the error type.

This is just an unfortunate result of using ? (which is why I chose to not have ? in my pin-init crate but instead make error propagation automatic).

Basically since all error return paths use ? which expands to a From::from, the error type never unifies with anything and stay as an inference variable, hence the explicit specification.

If we had the never type this would be a much more bearable ::<!>

Does it work with the never type (even if it is not stable), or are you triggering some compiler bug etc. there?

It works with the never type

wedsonaf · 2022-10-07T14:49:22Z

drivers/android/node.rs

-    /// The caller must call `NodeDeath::init` before using the notification object.
-    pub(crate) unsafe fn new(node: Arc<Node>, process: Arc<Process>, cookie: usize) -> Self {
-        Self {
+    #[allow(clippy::new_ret_no_self)]


Is there a way to avoid having to disable this for every function where we return impl InInit<Self>?

Not naming it new? I am not sure if we should even name them new, maybe new_in_place or something might be more expressive. AFAIK we could add #![allow(clippy::new_ret_no_self)] to lib.rs, but then it would not warn any longer in other places... Maybe we could get in touch with the clippy folks and ask for some workaround (I am not aware of any currently). There is precedent for this: rust-lang/rust-clippy#3313

wedsonaf · 2022-10-07T14:51:51Z

drivers/android/process.rs

+                    self.clone(),
+                    cookie,
+                ))
+                .map_err(|(i, _)| i)?,


What is the pair returned here?

It is (E, UniqueArc<T>) and E is Infallible.

the function called is UniqueArc::<MaybeUninit<T>>::pin_init_now(self, impl PinInit<T, E>) -> Result<UniqueArc<T>, (E, Self)> and if the initializer fails I wanted to give a way to still keep the allocated memory.

This is the only place this function is used, because I did not want to change the code. I thought that there might be a reason that the memory is first allocated and then some checks are run (lines 701-707). If we can refactor this code to use UniqueArc::pin_init then we could remove the pin_init_now function (which I would prefer).

drivers/android/thread.rs

drivers/android/process.rs

wedsonaf · 2022-10-07T14:53:36Z

drivers/android/transaction.rs

            node_ref: None,
            stack_next: None,
            from: from.clone(),
            to,
            code: tr.code,
            flags: tr.flags,
-            data_size: tr.data_size as _,
+            data_size: tr.data_size as usize,


Is this needed?

(To clarify, I'm not going into the question of whether it is better/worse to be explicit about the type, we can discuss that elsewhere. I'm asking if the new macros made things such that the compiler can't infer the types anymore.)

Yes this is needed, otherwise we get a type inference error (have not investigated why).

y86-dev · 2022-10-07T15:08:51Z

What is the reason that the CI checks were cancelled?

wedsonaf · 2022-10-07T15:10:56Z

What is the reason that the CI checks were cancelled?

Not sure. Let me ask them to rerun.

nbdd0121 · 2022-10-07T15:37:48Z

My branch that uses pin-init crate for surface syntax comparison nbdd0121@a1178f8.

nbdd0121 · 2022-10-07T15:48:12Z

@y86-dev it seems that the init syntax has been further changed within the past couple of days? The surface syntax is very similar to pin-init now. Nice :)

One difference is that the thing within pin_init! is not quite a struct expression, when the type has generic involved. E.g. If we have Foo<T>, then currently you have to pin_init!(Foo<T> { xxx }) instead of pin_init!(Foo { xxx }).

y86-dev · 2022-10-07T15:52:40Z

Not sure which part of the syntax you are referring to... If you look at https://github.com/y86-dev/pinned-init/blame/main/examples/mutex.rs#L68 that has not changed for 22 days.

One difference is

Yeah, I need that for the struct initializer (otherwise again some inference error).

ojeda

Some nits from a quick look for the next time you force-push.

By the way, the commit splitting looks fine to me! Why do you want to squash the two commits?

As for the commit message and the:

TODO: describe the purpose and functionality of the API better.

I am guessing you may be dreading explaining everything there :) If that is the case, do not worry too much about explaining everything in the commit message. In particular, you don't need to repeat the docs/API since it will be in the source code. Instead, try to focus on the "why do we want/need this change".

drivers/android/transaction.rs

rust/kernel/init/__private.rs

rust/kernel/init.rs

rust/macros/lib.rs

rust/kernel/init.rs

ojeda · 2022-10-07T15:49:17Z

drivers/android/context.rs

-            // SAFETY: Init is called below.
-            manager: unsafe {
-                Mutex::new(Manager {
+        let ctx = UniqueArc::pin_init::<core::convert::Infallible>(pin_init!(Self {


Does it work with the never type (even if it is not stable), or are you triggering some compiler bug etc. there?

y86-dev · 2022-10-07T15:56:46Z

Some nits from a quick look for the next time you force-push.

I plan to do normal commits to have a better review and will rebase at the end

nbdd0121 · 2022-10-07T16:02:19Z

Not sure which part of the syntax you are referring to... If you look at y86-dev/pinned-init@main/examples/mutex.rs#L68 (blame) that has not changed for 22 days.

Oh, sorry! I was under the impression that the syntax on the README was the latest syntax when I checked last time.

Yeah, I need that for the struct initializer (otherwise again some inference error).

That's quite unfortunate. I am a big fan of your approach (todo struct, and the raw-pointer-based API), but I think the current state as it is requires too many explicit annotations.

y86-dev · 2022-10-07T16:09:23Z

Oh, sorry! I was under the impression that the syntax on the README was the latest syntax when I checked last time.

Did you mean the syntax with <-? That was just experimental to see how it could look like as a language feature (I also let the readme go stale...).

nbdd0121 · 2022-10-07T16:13:07Z

One idea about solving the error type inference problem: given that in kernel we usually only use two error types, ! and kernel::Error, maybe we can have two macros, one for each error type? (the suggestion won't help the library for general audience, but should be good enough for us)

We can call the macros and functions like this:

init! for infallible pin-init. (we can drop pin from the name too because in kernel we probably wouldn't need to initialize a struct unpinned; if people need to initialize Unpin struct then they can pin-init it first and then undo the pinning)
try_init! for pin-init with kernel::Error return type.
{Box, UniqueArc}::{init_with, try_init_with} that initialize a Ptr<MaybeUninit<T>> with ! and Error respectively.
{Box, UniqueArc, Arc}::try_new_with that takes both PinInit<T, !> and PinInit<T, Error> (because allocation fail will return an error anyway).

Of course this doesn't help with the inference problem at the struct and field...

rust/kernel/lib.rs

rust/kernel/init.rs

y86-dev · 2022-10-07T16:55:19Z

One idea about solving the error type inference problem: given that in kernel we usually only use two error types, ! and kernel::Error, maybe we can have two macros, one for each error type? (the suggestion won't help the library for general audience, but should be good enough for us)

We can call the macros and functions like this:
* `init!` for infallible pin-init. (we can drop `pin` from the name too because in kernel we probably wouldn't need to initialize a struct unpinned; if people need to initialize `Unpin` struct then they can pin-init it first and then undo the pinning)

* `try_init!` for pin-init with `kernel::Error` return type.

* `{Box, UniqueArc}::{init_with, try_init_with}` that initialize a `Ptr<MaybeUninit<T>>` with `!` and `Error` respectively.

* `{Box, UniqueArc, Arc}::try_new_with` that takes both `PinInit<T, !>` and `PinInit<T, Error>` (because allocation fail will return an error anyway).
Of course this doesn't help with the inference problem at the struct and field...

I am not sure... Users might want to have custom errors that are ZSTs or have more options than error::Error has.

There could be a situation where you want to initialize something !Unpin in-place and then later move it (although I cannot think of anything and this also came up in the internals thread going over my blog.). Not sure if we want to shut this door early and have a lot of refactoring later, or let it stay open and... also have a lot of refactoring...

bjorn3 · 2022-10-07T16:57:10Z

Another option to deal with error type inference would be to allow explicitly specifying the error type in the macro if it couldn't be inferred.

rust/kernel/init.rs

y86-dev · 2022-10-14T18:49:42Z

Summary of the last few commits (up to the last force-push):

back to declarative macro + convenience attribute macro.
the attr macro actually does an important parsing step: separating impl_generics and ty_generics and isolate the where clause
better docs
prevention of Drop and Unpin impls
PinnedDrop
other small changes (the explicit PinInitClosure type is gone)

y86-dev · 2022-10-16T18:51:20Z

rebased commits again that had been accumulating. Also fixed some unsoundness with DropGuard (it was possible to forget it and then error anyway). Also rebased with the newest changes from rust.

drivers/android/process.rs

rust/kernel/sync.rs

rust/macros/pinned_drop.rs

rust/kernel/sync.rs

drivers/android/process.rs

nbdd0121 · 2022-10-26T18:39:40Z

rust/kernel/sync/arc.rs

+        // assert that there are only two fields: refcount and data (done in a closure to avoid
+        // overflowing the stack in debug mode with a big `T`)
+        #[allow(unreachable_code, clippy::diverging_sub_expression)]
+        let _check = || {


I don't feel that this check is necessary given that the definition of ArcInner is in the same file.

What do you think of the implementation now?

rust/kernel/sync/arc.rs

nbdd0121 · 2022-10-26T18:45:15Z

rust/kernel/types.rs

+    ///
+    /// The pointer supplied is valid.
+    pub unsafe fn raw_get(this: *const Self) -> *mut T {
+        UnsafeCell::raw_get(addr_of!((*this).0).cast::<UnsafeCell<T>>())


Maybe

UnsafeCell::raw_get(this.cast::<UnsafeCell<T>>())

? Opaque and MaybeUninit are both #[repr(transparent)] so we can just cast the pointer directly.

Seems reasonable, it was a bit unclear to me if we guarantee that Opaque will always be transparent (I guess changing it would also require changing this method, but wanted to be careful). To be clear: I do not think that there is a reason for making Opaque not transparent, so we could just clarify that in the documentation.

Normally, the UnsafeCell would go outside the MaybeUninit.

nbdd0121 · 2022-10-26T18:53:02Z

rust/kernel/init/pinned_drop.rs

+                $($inner)*
+            }
+
+            fn __ensure_no_unsafe_op_in_drop($self: $st) {


I don't think this is necessary, we enable unsafe_op_in_unsafe_fn globally. If you want to be extra careful you can generate a #[forbid(unsafe_op_in_unsafe_fn)] on the drop function.

You can still silence forbid using --cap-lints allow.

I think we should be extra careful here.

nbdd0121 · 2022-10-26T18:55:51Z

drivers/android/node.rs

-    /// The caller must call `NodeDeath::init` before using the notification object.
-    pub(crate) unsafe fn new(node: Arc<Node>, process: Arc<Process>, cookie: usize) -> Self {
-        Self {
+    #[allow(clippy::new_ret_no_self)]


We probably want to disable this globally until rust-lang/rust-clippy#7344 is fixed.

Filed rust-lang/rust-clippy#9733 to fix the false positive.

Should I just add #![allow(clippy::new_ret_no_self)] to lib.rs?

nbdd0121 · 2022-10-26T19:15:37Z

rust/kernel/sync/condvar.rs

-            _pin: PhantomPinned,
+    #[allow(clippy::new_ret_no_self)]
+    pub const fn new(name: &'static CStr, key: &'static LockClassKey) -> impl PinInit<Self> {
+        fn init_wait_list(


What's the rationale behind having a new function instead of just having

wait_list: unsafe { init::pin_init_from_closure(|| { ... }) }

down there?

here would be the closure version:

pin_init!(Self { wait_list: unsafe { init::pin_init_from_closure::<_, core::convert::Infallible>(move |slot| { bindings::__init_waitqueue_head( Opaque::raw_get(slot), name.as_char_ptr(), key.get(), ); Ok(()) }) }, _pin: PhantomPinned, })

(yes we need to specify the error type 🙄)

Not sure if that is more readable... I could probably create a more general function that would handle this, so along the lines of:

unsafe fn ffi_init1<T, P1>(fun: unsafe fn(*mut T, P1), param1: P1) -> impl PinInit<Opaque<T>>; unsafe fn ffi_init2<T, P1, P2>(fun: unsafe fn(*mut T, P1), param1: P1, param2: P2) -> impl PinInit<Opaque<T>>; unsafe fn ffi_init3<T, P1, P2, P2>(fun: unsafe fn(*mut T, P1), param1: P1, param2: P2, param3: P3) -> impl PinInit<Opaque<T>>;

rust/macros/pin_project.rs

nbdd0121 · 2022-10-26T19:40:27Z

rust/macros/pinned_drop.rs

+    let mut pinned_drop_idx = None;
+    for (i, tt) in toks.iter().enumerate() {
+        match tt {
+            TokenTree::Punct(p) if p.as_char() == '<' => {


Perhaps the angle bracket matching code can be extracted to a function and shared between both macros.

This seems a bit difficult, because in the code for pin_project I am going through the generics list and I am searching for the identifiers. So that code seems a bit too specific to extract that and keep the functionality...

I can still create the function for use in other places though...

rust/macros/pinned_drop.rs

Also added `assume_init` to `UniqueArc<MaybeUninit<T>>`. It assumes that its contents have been initialized and returns `UniqueArc<T>`. `try_new_uninit` is needed, because `try_new(MaybeUninit::uninit())` actually allocates memory on the stack and may result in a stack overflow if `T` is large in size. `try_new_uninit` has been implemented in such a way that this cannot happen. Signed-off-by: Benno Lossin <y86-dev@protonmail.com>

This function does exactly the same thing as `UnsafeCell::raw_get` [1]. It exists to avoid creating intermediate references. Link: https://doc.rust-lang.org/core/cell/struct.UnsafeCell.html#method.raw_get [1] Signed-off-by: Benno Lossin <y86-dev@protonmail.com>

This allows proc macros to refer to `::kernel`. This syntax is needed in order to ensure that we refer to the true kernel crate instead of a local module. Signed-off-by: Benno Lossin <y86-dev@protonmail.com>

This API is used to initialize pinned structs. Before we had to use `unsafe` to first call `Mutex::new` with the promise that we would call `Mutex::init` later (which also requires `unsafe`, because it first needs a pin-projection). This API merges those two functions into one such that it is now a safe operation for the user. It is implemented using a combination of things: - declarative macros used to create initializers (`pin_init!`/`init!`), - declarative macro used to inform about the pinned fields (`pin_project!`), - proc macro shim delegating to the previously mentioned declarative macro (the proc macro does a bit of generics parsing, it is `#[pin_project]`), - traits and a couple of functions to create basic initializers. For details, please read the documentation at [1]. Link: https://rust-for-linux.github.io/docs/kernel/init/index.html [1] Signed-off-by: Benno Lossin <y86-dev@protonmail.com>

This patch changes the API and internal logic such that the new init API is used. This change results in a lot less `unsafe`, because the new API is a safe abstraction over the unsafe interior. Signed-off-by: Benno Lossin <y86-dev@protonmail.com>

nbdd0121 · 2023-01-14T17:03:01Z

I had a brief look and it does indeed seem like the inference will be very limited with current design.

However I think maybe we can have slightly different syntax for direct initialization vs in-place initialization, e.g.

pin_init!(Self {
    foo: 1, // this is direct initialization
    mutex <- new_mutex!(...), // this is in-place initialization
})

this way type inference can work properly for direct initialization, and we also have a clear indication of which one is pinned initialized and which one is not (could be a good thing potentially?)

y86-dev · 2023-01-15T11:18:44Z

This was one of the intermediate states I had, but I changed it, because at kangrejos there were some worries about non-standard syntax. I think that it makes it easier to understand (and as you also said makes inference better) how things are initialized.

y86-dev · 2023-08-14T08:57:33Z

closed since https://lore.kernel.org/rust-for-linux/20230408122429.1103522-1-y86-dev@protonmail.com/ was merged

y86-dev force-pushed the pinned-init branch 2 times, most recently from b77ee65 to 1029159 Compare October 7, 2022 14:08

nbdd0121 reviewed Oct 7, 2022

View reviewed changes

rust/kernel/sync/arc.rs Outdated Show resolved Hide resolved

y86-dev force-pushed the pinned-init branch from 1029159 to da33a3e Compare October 7, 2022 14:38

wedsonaf reviewed Oct 7, 2022

View reviewed changes

ojeda reviewed Oct 7, 2022

View reviewed changes

bjorn3 suggested changes Oct 7, 2022

View reviewed changes

rust/kernel/lib.rs Show resolved Hide resolved

rust/kernel/lib.rs Outdated Show resolved Hide resolved

rust/kernel/lib.rs Show resolved Hide resolved

rust/kernel/init.rs Outdated Show resolved Hide resolved

y86-dev force-pushed the pinned-init branch from a355755 to c1ea0c7 Compare October 12, 2022 13:03

bjorn3 reviewed Oct 12, 2022

View reviewed changes

rust/kernel/init.rs Outdated Show resolved Hide resolved

y86-dev force-pushed the pinned-init branch from 8e3fa16 to 3baa4e6 Compare October 16, 2022 18:48

y86-dev force-pushed the pinned-init branch 3 times, most recently from 30dc92f to b5b8cef Compare October 18, 2022 18:37

bjorn3 reviewed Oct 20, 2022

View reviewed changes

drivers/android/process.rs Outdated Show resolved Hide resolved

bjorn3 reviewed Oct 20, 2022

View reviewed changes

rust/kernel/sync.rs Outdated Show resolved Hide resolved

bjorn3 reviewed Oct 20, 2022

View reviewed changes

rust/macros/pinned_drop.rs Outdated Show resolved Hide resolved

bjorn3 reviewed Oct 22, 2022

View reviewed changes

rust/kernel/sync.rs Outdated Show resolved Hide resolved

y86-dev commented Oct 22, 2022

View reviewed changes

drivers/android/process.rs Show resolved Hide resolved

nbdd0121 reviewed Oct 26, 2022

View reviewed changes

y86-dev force-pushed the pinned-init branch from edb4fc6 to 7a86749 Compare November 9, 2022 12:22

y86-dev force-pushed the pinned-init branch from a8c6814 to 511ba9c Compare November 28, 2022 17:18

y86-dev added 3 commits January 11, 2023 21:16

rust: added extern crate self as kernel

2415d84

This allows proc macros to refer to `::kernel`. This syntax is needed in order to ensure that we refer to the true kernel crate instead of a local module. Signed-off-by: Benno Lossin <y86-dev@protonmail.com>

y86-dev force-pushed the pinned-init branch 4 times, most recently from cf70f5e to 9e122cc Compare January 12, 2023 21:53

y86-dev added 8 commits January 13, 2023 09:42

Added try[_pin]_init! and removed turbofish

48e56dd

Safety comments and using Vec::splice

7a8d191

Fixed stuff forgotten last commit and fixed typos

1c7e882

Implemented review suggestions

901221e

Added ffi_initX used to create initializers from C functions

ae4289f

Renamed pin_project -> pin_data

308d6ee

y86-dev force-pushed the pinned-init branch from 9e122cc to 308d6ee Compare January 13, 2023 08:45

y86-dev closed this Aug 14, 2023

y86-dev deleted the pinned-init branch September 14, 2023 11:29

rust: add new api for initialization #909

rust: add new api for initialization #909

Conversation

y86-dev commented Oct 7, 2022

wedsonaf left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

y86-dev commented Oct 7, 2022 • edited Loading

wedsonaf commented Oct 7, 2022

nbdd0121 commented Oct 7, 2022

nbdd0121 commented Oct 7, 2022

y86-dev commented Oct 7, 2022

ojeda left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

y86-dev commented Oct 7, 2022

nbdd0121 commented Oct 7, 2022

y86-dev commented Oct 7, 2022

nbdd0121 commented Oct 7, 2022

y86-dev commented Oct 7, 2022

bjorn3 commented Oct 7, 2022

y86-dev commented Oct 14, 2022

y86-dev commented Oct 16, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nbdd0121 commented Jan 14, 2023

y86-dev commented Jan 15, 2023

y86-dev commented Aug 14, 2023

y86-dev commented Oct 7, 2022 •

edited

Loading