Patch/pinned init v3 by BennoLossin · Pull Request #989 · Rust-for-Linux/linux

BennoLossin · 2023-03-22T16:40:38Z

No description provided.

BennoLossin · 2023-03-27T10:27:00Z

for anyone wanting to see the individual commits, see https://github.com/y86-dev/linux/tree/patch/pinned-init-v3-not-squashed

metaspace · 2023-03-27T13:41:39Z

I think we should split up rust: add pin-init API in multiple patches. I realize that many of the types used by the macros are not meant to be used by consumers of this API, but I think this would be easier to review if the types used by the macros were added on their own, along with examples of intended use. The macros could then be added in a separate patch as a means to automate the examples.

BennoLossin · 2023-03-27T14:31:41Z

I think we should split up rust: add pin-init API in multiple patches. I realize that many of the types used by the macros are not meant to be used by consumers of this API, but I think this would be easier to review if the types used by the macros were added on their own, along with examples of intended use. The macros could then be added in a separate patch as a means to automate the examples.

Not sure if I understand correctly, are you talking about the types inside of __internal.rs, or the other types as well? What do you mean by examples of intended use? The __internal.rs types mostly serve a single purpose and are only intended to be used inside of the macros. I could give some macro expanded code as examples of how the macro uses these internal types if that is what you mean, but I dont know where to put them for the patches.

metaspace · 2023-03-28T07:12:18Z

Not sure if I understand correctly, are you talking about the types inside of __internal.rs, or the other types as well? What do you mean by examples of intended use? The __internal.rs types mostly serve a single purpose and are only intended to be used inside of the macros. I could give some macro expanded code as examples of how the macro uses these internal types if that is what you mean, but I dont know where to put them for the patches.

I think the rust: add pin-init API commit is too big. For me it was difficult to digest, so I think a bit more help to the reader would be nice. I would suggest the following:

Splitting the commit in at least two: One part that supports pin_init_from_closure and one part that adds macros that create the initializer
Even though __internal is not intended to be used by consumers of the init API, it is a public module that is going to be accessible to the user, right? So the user could write something like the expanded macro text, calling into init::__internal. Adding __internal in one commit along with examples of how the expanded macros will use it would be a major help in understanding the macros. The examples could be delivered in the module documentation.
I would suggest splitting in even more pieces, like one adding init! and one adding pin_init!
The commit message is nice. I would love to have that as module documentation. I think the commit message is better than the module level intro.
It was difficult for me to parse the macros. I am not a macro expert, I had to look up quite a bit of mechanics to follow along. One thing that would have been a great help is having expanded examples in the documentation. It was difficult for me to read the macros without knowing where it was trying to end up. I know there are tools for looking at expanded macros, but I still think some small examples in the documentation would be nice.
I did not immediately understand some of the traits that are used to help type inference. It would be great to document why and how this works in the expanded macro examples.

I hope this makes more sense :) I am not sure who is going to review this in the end, but if it is people that are not long time Rust experts, we need to give them all the help we can think of.

nbdd0121 · 2023-03-28T10:18:18Z

Types used only internally by the macro should not be a separate commit. Each logical change should be a commit, no matter how big or small it is.

Splitting the commit into a core PinInit/Init/StackInit/pin_init_from_closure commit and a macro commit, though, does make sense.

ojeda

Some trivial comments from last week from when the GH outage happened, please ignore any if they are not relevant after the later push(es).

ojeda · 2023-03-28T14:25:10Z

Each logical change should be a commit, no matter how big or small it is.

This is true, and it simplifies reverting changes later and so on. However, big patches may be split into smaller ones to make things more understandable for reviewers, even if they could also be understood to go together as a unit too, as long as the series does not break anything after each step (so fairly easy when introducing something new).

Having said that, 2k lines is still within reason (even if already within the top percentile) and several versions have been in the list for a while, but if you (@y86-dev) can do a logical split somewhere that makes it easier to understand (taking into account the commit messages too, especially given the one you have there), please go for it for v4 since some people will appreciate it (not just now, but in the future too).

ojeda · 2023-03-28T15:11:03Z

GitHub's "Compare" button says from_value is gone.

Also "rust: add pin-init API items to prelude" -> "rust: prelude: add pin-init API items".

BennoLossin · 2023-03-28T15:17:55Z

GitHub's "Compare" button says from_value is gone.

Also "rust: add pin-init API items to prelude" -> "rust: prelude: add pin-init API items".

Yeah that was replaced by impl Init<T> for T, had forgotten to remove it when I added that. Also some other things are not completely done, still rebasing stuff.

ojeda · 2023-03-28T15:23:22Z

I saw it (from_value) referenced from intra-doc links too.

BennoLossin · 2023-03-28T18:51:35Z

@metaspace I added a macro expansion example, let me know what you think of it.

Not sure if the __internal.rs docs is the best place for it, since that is #[doc(hidden)], maybe I could create a doc module just for that purpose? Also not sure into which commit this should go, since PinnedDrop is now split from pin_init!.

metaspace · 2023-03-29T08:55:30Z

@metaspace I added a macro expansion example, let me know what you think of it.

Not sure if the __internal.rs docs is the best place for it, since that is #[doc(hidden)], maybe I could create a doc module just for that purpose? Also not sure into which commit this should go, since PinnedDrop is now split from pin_init!.

I think this is really helpful. I think having it where you put it is fine. It's not something a user would read anyway.

Add the `quote!` macro for creating `TokenStream`s directly via the given Rust tokens. It also supports repetitions using iterators. It will be used by the pin-init API proc-macros to generate code. Signed-off-by: Gary Guo <gary@garyguo.net> Signed-off-by: Benno Lossin <y86-dev@protonmail.com>

Adds the `assume_init` function to `UniqueArc<MaybeUninit<T>>` that unsafely assumes the value to be initialized and yields a value of type `UniqueArc<T>`. This function is used when manually initializing the pointee of an `UniqueArc`. Signed-off-by: Benno Lossin <y86-dev@protonmail.com>

This function mirrors `UnsafeCell::raw_get`. It avoids creating a reference and allows solely using raw pointers. The `pin-init` API will be using this, since uninitialized memory requires raw pointers. Signed-off-by: Benno Lossin <y86-dev@protonmail.com>

This API is used to facilitate safe pinned initialization of structs. It replaces cumbersome `unsafe` manual initialization with elegant safe macro invocations. Due to the size of this change it has been split into six commits: 1. This commit introducing the basic public interface: traits and functions to represent and create initializers. 2. Adds the `#[pin_data]`, `pin_init!`, `try_pin_init!`, `init!` and `try_init!` macros along with their internal types. 3. Adds the `InPlaceInit` trait that allows using an initializer to create an object inside of a `Box<T>` and other smart pointers. 4. Adds the `PinnedDrop` trait and adds macro support for it in the `#[pin_data]` macro. 5. Adds the `stack_pin_init!` macro allowing to pin-initialize a struct on the stack. 6. Adds the `Zeroable` trait and `init::zeroed` function to initialize types that have `0x00` in all bytes as a valid bit pattern. -- In this section the problem that the new pin-init API solves is outlined. This message describes the entirety of the API, not just the parts introduced in this commit. For a more granular explanation and additional information on pinning and this issue, view [1]. Pinning is Rust's way of enforcing the address stability of a value. When a value gets pinned it will be impossible for safe code to move it to another location. This is done by wrapping pointers to said object with `Pin<P>`. This wrapper prevents safe code from creating mutable references to the object, preventing mutable access, which is needed to move the value. `Pin<P>` provides `unsafe` functions to circumvent this and allow modifications regardless. It is then the programmer's responsibility to uphold the pinning guarantee. Many kernel data structures require a stable address, because there are foreign pointers to them which would get invalidated by moving the structure. Since these data structures are usually embedded in structs to use them, this pinning property propagates to the container struct. Resulting in most structs in both Rust and C code needing to be pinned. So if we want to have a `mutex` field in a Rust struct, this struct also needs to be pinned, because a `mutex` contains a `list_head`. Additionally initializing a `list_head` requires already having the final memory location available, because it is initialized by pointing it to itself. But this presents another challenge in Rust: values have to be initialized at all times. There is the `MaybeUninit<T>` wrapper type, which allows handling uninitialized memory, but this requires using the `unsafe` raw pointers and a casting the type to the initialized variant. This problem gets exacerbated when considering encapsulation and the normal safety requirements of Rust code. The fields of the Rust `Mutex<T>` should not be accessible to normal driver code. After all if anyone can modify the fields, there is no way to ensure the invariants of the `Mutex<T>` are upheld. But if the fields are inaccessible, then initialization of a `Mutex<T>` needs to be somehow achieved via a function or a macro. Because the `Mutex<T>` must be pinned in memory, the function cannot return it by value. It also cannot allocate a `Box` to put the `Mutex<T>` into, because that is an unnecessary allocation and indirection which would hurt performance. The current solution was to split this function into two parts: 1. A `new` function that returns a partially initialized `Mutex<T>`, 2. An `init` function that requires the `Mutex<T>` to be pinned and that fully initializes the `Mutex<T>`. Both of these functions have to be marked `unsafe`, since a call to `new` needs to be accompanied with a call to `init`, otherwise using the `Mutex<T>` could result in UB. And because calling `init` twice also is not safe. While `Mutex<T>` initialization cannot fail, other structs might also have to allocate memory, which would result in conditional successful initialization requiring even more manual accommodation work. Combine this with the problem of pin-projections -- the way of accessing fields of a pinned struct -- which also have an `unsafe` API, pinned initialization is riddled with `unsafe` resulting in very poor ergonomics. Not only that, but also having to call two functions possibly multiple lines apart makes it very easy to forget it outright or during refactoring. Here is an example of the current way of initializing a struct with two synchronization primitives (see [2] for the full example): struct SharedState { state_changed: CondVar, inner: Mutex<SharedStateInner>, } impl SharedState { fn try_new() -> Result<Arc<Self>> { let mut state = Pin::from(UniqueArc::try_new(Self { // SAFETY: `condvar_init!` is called below. state_changed: unsafe { CondVar::new() }, // SAFETY: `mutex_init!` is called below. inner: unsafe { Mutex::new(SharedStateInner { token_count: 0 }) }, })?); // SAFETY: `state_changed` is pinned when `state` is. let pinned = unsafe { state.as_mut().map_unchecked_mut(|s| &mut s.state_changed) }; kernel::condvar_init!(pinned, "SharedState::state_changed"); // SAFETY: `inner` is pinned when `state` is. let pinned = unsafe { state.as_mut().map_unchecked_mut(|s| &mut s.inner) }; kernel::mutex_init!(pinned, "SharedState::inner"); Ok(state.into()) } } The pin-init API of this patch solves this issue by providing a comprehensive solution comprised of macros and traits. Here is the example from above using the pin-init API: #[pin_data] struct SharedState { #[pin] state_changed: CondVar, #[pin] inner: Mutex<SharedStateInner>, } impl SharedState { fn new() -> impl PinInit<Self> { pin_init!(Self { state_changed <- new_condvar!("SharedState::state_changed"), inner <- new_mutex!( SharedStateInner { token_count: 0 }, "SharedState::inner", ), }) } } Notably the way the macro is used here requires no `unsafe` and thus comes with the usual Rust promise of safe code not introducing any memory violations. Additionally it is now up to the caller of `new()` to decide the memory location of the `SharedState`. They can choose at the moment `Arc<T>`, `Box<T>` or the stack. -- The API has the following architecture: 1. Initializer traits `PinInit<T, E>` and `Init<T, E>` that act like closures. 2. Macros to create these initializer traits safely. 3. Functions to allow manually writing initializers. The initializers (an `impl PinInit<T, E>`) receive a raw pointer pointing to uninitialized memory and their job is to fully initialize a `T` at that location. If initialization fails, they return an error (`E`) by value. This way of initializing cannot be safely exposed to the user, since it relies upon these properties outside of the control of the trait: - the memory location (slot) needs to be valid memory, - if initialization fails, the slot should not be read from, - the value in the slot should be pinned, so it cannot move and the memory cannot be deallocated until the value is dropped. This is why using an initializer is facilitated by another trait that ensures these requirements. These initializers can be created manually by just supplying a closure that fulfills the same safety requirements as `PinInit<T, E>`. But this is an `unsafe` operation. To allow safe initializer creation, the `pin_init!` is provided along with three other variants: `try_pin_init!`, `try_init!` and `init!`. These take a modified struct initializer as a parameter and generate a closure that initializes the fields in sequence. The macros take great care in upholding the safety requirements: - A shadowed struct type is used as the return type of the closure instead of `()`. This is to prevent early returns, as these would prevent full initialization. - To ensure every field is only initialized once, a normal struct initializer is placed in unreachable code. The type checker will emit errors if a field is missing or specified multiple times. - When initializing a field fails, the whole initializer will fail and automatically drop fields that have been initialized earlier. - Only the correct initializer type is allowed for unpinned fields. You cannot use a `impl PinInit<T, E>` to initialize a structurally not pinned field. To ensure the last point, an additional macro `#[pin_data]` is needed. This macro annotates the struct itself and the user specifies structurally pinned and not pinned fields. Because dropping a pinned struct is also not allowed to break the pinning invariants, another macro attribute `#[pinned_drop]` is needed. This macro is introduced in a following commit. These two macros also have mechanisms to ensure the overall safety of the API. Additionally, they utilize a combined proc-macro, declarative macro design: first a proc-macro enables the outer attribute syntax `#[...]` and does some important pre-parsing. Notably this prepares the generics such that the declarative macro can handle them using token trees. Then the actual parsing of the structure and the emission of code is handled by a declarative macro. For pin-projections the crates `pin-project` [3] and `pin-project-lite` [4] had been considered, but were ultimately rejected: - `pin-project` depends on `syn` [5] which is a very big dependency, around 50k lines of code. - `pin-project-lite` is a more reasonable 5k lines of code, but contains a very complex declarative macro to parse generics. On top of that it would require modification that would need to be maintained independently. Link: https://rust-for-linux.com/the-safe-pinned-initialization-problem [1] Link: https://github.com/Rust-for-Linux/linux/blob/f509ede33fc10a07eba3da14aa00302bd4b5dddd/samples/rust/rust_miscdev.rs [2] Link: https://crates.io/crates/pin-project [3] Link: https://crates.io/crates/pin-project-lite [4] Link: https://crates.io/crates/syn [5] Co-developed-by: Gary Guo <gary@garyguo.net> Signed-off-by: Gary Guo <gary@garyguo.net> Signed-off-by: Benno Lossin <y86-dev@protonmail.com>

Add the following initializer macros: - `#[pin_data]` to annotate structurally pinned fields of structs, needed for `pin_init!` and `try_pin_init!` to select the correct initializer of fields. - `pin_init!` create a pin-initializer for a struct with the `Infallible` error type. - `try_pin_init!` create a pin-initializer for a struct with a custom error type (`kernel::error::Error` is the default). - `init!` create an in-place-initializer for a struct with the `Infallible` error type. - `try_init!` create an in-place-initializer for a struct with a custom error type (`kernel::error::Error` is the default). Also add their needed internal helper traits and structs. Co-developed-by: Gary Guo <gary@garyguo.net> Signed-off-by: Gary Guo <gary@garyguo.net> Signed-off-by: Benno Lossin <y86-dev@protonmail.com>

…ters The `InPlaceInit` trait that provides two functions, for initializing using `PinInit<T, E>` and `Init<T>`. It is implemented by `Arc<T>`, `UniqueArc<T>` and `Box<T>`. Signed-off-by: Benno Lossin <y86-dev@protonmail.com>

The `PinnedDrop` trait that facilitates destruction of pinned types. It has to be implemented via the `#[pinned_drop]` macro, since the `drop` function should not be called by normal code, only by other destructors. It also only works on structs that are annotated with `#[pin_data(PinnedDrop)]`. Co-developed-by: Gary Guo <gary@garyguo.net> Signed-off-by: Gary Guo <gary@garyguo.net> Signed-off-by: Benno Lossin <y86-dev@protonmail.com>

The `stack_pin_init!` macro allows pin-initializing a value on the stack. It accepts a `impl PinInit<T, E>` to initialize a `T`. It allows propagating any errors via `?` or handling it normally via `match`. Signed-off-by: Benno Lossin <y86-dev@protonmail.com>

Add the `Zeroable` trait which marks types that can be initialized by writing `0x00` to every byte of the type. Also add the `init::zeroed` function that creates an initializer for a `Zeroable` type that writes `0x00` to every byte. Signed-off-by: Benno Lossin <y86-dev@protonmail.com>

Add `pin-init` API macros and traits to the prelude. Signed-off-by: Benno Lossin <y86-dev@protonmail.com>

Add helper functions to more easily initialize `Opaque<T>` via FFI and rust raw initializer functions. These functions take a function pointer to the FFI/raw initialization function and take between 0-4 other arguments. It then returns an initializer that uses the FFI/raw initialization function along with the given arguments to initialize an `Opaque<T>`. Signed-off-by: Benno Lossin <y86-dev@protonmail.com>

`UniqueArc::try_new_uninit` calls `Arc::try_new(MaybeUninit::uninit())`. This results in the uninitialized memory being placed on the stack, which may be arbitrarily large due to the generic `T` and thus could cause a stack overflow for large types. Change the implementation to use the pin-init API which enables in-place initialization. In particular it avoids having to first construct and then move the uninitialized memory from the stack into the final location. Signed-off-by: Benno Lossin <y86-dev@protonmail.com>

Add two functions `init_with` and `pin_init_with` to `UniqueArc<MaybeUninit<T>>` to initialize the memory of already allocated `UniqueArc`s. This is useful when you want to allocate memory check some condition inside of a context where allocation is forbidden and then conditionally initialize an object. Signed-off-by: Benno Lossin <y86-dev@protonmail.com>

BennoLossin · 2023-03-30T17:40:27Z

continued at #991

BennoLossin mentioned this pull request Mar 22, 2023

Patch/pinned init v2 #986

Closed

metaspace reviewed Mar 24, 2023

View reviewed changes

Comment thread rust/macros/pin_data.rs Outdated

BennoLossin force-pushed the patch/pinned-init-v3 branch from 1d24ca9 to 8d692dd Compare March 27, 2023 10:26

BennoLossin commented Mar 27, 2023

View reviewed changes

Comment thread rust/kernel/lib.rs

Comment thread scripts/Makefile.build

BennoLossin force-pushed the patch/pinned-init-v3 branch from 8d692dd to 47cfe4b Compare March 27, 2023 10:53

metaspace reviewed Mar 27, 2023

View reviewed changes

Comment thread rust/kernel/init.rs Outdated

BennoLossin force-pushed the patch/pinned-init-v3 branch from 47cfe4b to 0020c06 Compare March 28, 2023 11:23

ojeda reviewed Mar 28, 2023

View reviewed changes

BennoLossin force-pushed the patch/pinned-init-v3 branch from 0020c06 to 6c73c39 Compare March 28, 2023 14:53

BennoLossin force-pushed the patch/pinned-init-v3 branch from 6c73c39 to 9413e0a Compare March 28, 2023 15:17

BennoLossin force-pushed the patch/pinned-init-v3 branch 4 times, most recently from 44ede9b to 9d38670 Compare March 28, 2023 16:04

BennoLossin force-pushed the patch/pinned-init-v3 branch 2 times, most recently from d5ca4c9 to 66a0c75 Compare March 29, 2023 11:34

ojeda reviewed Mar 29, 2023

View reviewed changes

Comment thread rust/kernel/init/macros.rs Outdated

BennoLossin force-pushed the patch/pinned-init-v3 branch from 66a0c75 to e9b1858 Compare March 29, 2023 14:03

BennoLossin force-pushed the patch/pinned-init-v3 branch 2 times, most recently from 33b0c69 to a82a1e8 Compare March 29, 2023 14:12

fbq reviewed Mar 29, 2023

View reviewed changes

Comment thread rust/kernel/sync/arc.rs Outdated

BennoLossin added 2 commits March 29, 2023 21:45

BennoLossin force-pushed the patch/pinned-init-v3 branch from a82a1e8 to 2eaa36c Compare March 29, 2023 19:49

BennoLossin force-pushed the patch/pinned-init-v3 branch from 2eaa36c to 7792e6c Compare March 29, 2023 20:48

BennoLossin added 9 commits March 29, 2023 22:58

rust: prelude: add pin-init API items to prelude

f32ecde

Add `pin-init` API macros and traits to the prelude. Signed-off-by: Benno Lossin <y86-dev@protonmail.com>

BennoLossin force-pushed the patch/pinned-init-v3 branch from 7792e6c to 6b64aab Compare March 29, 2023 21:00

BennoLossin closed this Mar 30, 2023

BennoLossin deleted the patch/pinned-init-v3 branch September 14, 2023 11:30

Conversation

BennoLossin commented Mar 22, 2023

Uh oh!

Uh oh!

BennoLossin commented Mar 27, 2023

Uh oh!

Uh oh!

Uh oh!

Uh oh!

metaspace commented Mar 27, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

BennoLossin commented Mar 27, 2023

Uh oh!

metaspace commented Mar 28, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nbdd0121 commented Mar 28, 2023

Uh oh!

ojeda left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ojeda commented Mar 28, 2023

Uh oh!

ojeda commented Mar 28, 2023

Uh oh!

BennoLossin commented Mar 28, 2023

Uh oh!

ojeda commented Mar 28, 2023

Uh oh!

BennoLossin commented Mar 28, 2023

Uh oh!

metaspace commented Mar 29, 2023

Uh oh!

Uh oh!

Uh oh!

BennoLossin commented Mar 30, 2023

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

5 participants

metaspace commented Mar 27, 2023 •

edited

Loading

metaspace commented Mar 28, 2023 •

edited

Loading