Skip to content

Add TryPinInitWrapper trait.#37

Merged
BennoLossin merged 5 commits into
Rust-for-Linux:mainfrom
onestacked:add-try-pin-init-wrapper
Apr 19, 2025
Merged

Add TryPinInitWrapper trait.#37
BennoLossin merged 5 commits into
Rust-for-Linux:mainfrom
onestacked:add-try-pin-init-wrapper

Conversation

@onestacked
Copy link
Copy Markdown
Contributor

Implement it for UnsafeCell and UnsafePinned (unsafe-pinned feature is enabled.

There are probably a large amount of other types where a implementation might be useful,
but for now only implement these types. Others can be implemented in follow-ups.

I'm not sure how to handle enabling the UnsafePinned, for now I've just implemented it as a cargo feature.

Copy link
Copy Markdown
Member

@BennoLossin BennoLossin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR!

I have a couple notes. Overall using a cargo feature is the correct design. Do note that I port all commits back into the kernel (but only for the files that are there), so please follow the commit message conventions from there.

I do think that we should also think about which other types could make use of implementing the trait that you introduce here.

Comment thread src/lib.rs Outdated
Comment thread src/lib.rs Outdated
Comment thread Cargo.toml
Comment thread src/lib.rs Outdated
Comment thread src/lib.rs Outdated
Comment thread src/lib.rs Outdated
Comment thread src/lib.rs Outdated
Formatting done by the `Even Better TOML` vs-code extension.

Signed-off-by: Christian Schrefl <chrisi.schrefl@gmail.com>
@onestacked
Copy link
Copy Markdown
Contributor Author

Thanks for the PR!

I have a couple notes. Overall using a cargo feature is the correct design. Do note that I port all commits back into the kernel (but only for the files that are there), so please follow the commit message conventions from there.

I do think that we should also think about which other types could make use of implementing the trait that you introduce here.

In core we could additionally implement it for Option, ManuallyDrop and MaybeUninit (although MaybeUninit would probably be simpler to just initialize it after the PinInit in many cases).

In std we could implement it for the Mutex, Cell and RwLock family of types.

In the kernel for Opaque although if this is needed its probably a misuse of Opaque anyways.

@onestacked onestacked force-pushed the add-try-pin-init-wrapper branch from 196dd27 to ef8d073 Compare April 18, 2025 19:57
@onestacked
Copy link
Copy Markdown
Contributor Author

Split this into three commits:

  1. Format Cargo.toml.
  2. Add the trait and UnsafeCell implementation.
  3. Add the unsafe-pinned cargo feature.

@BennoLossin
Copy link
Copy Markdown
Member

Thanks for the PR!
I have a couple notes. Overall using a cargo feature is the correct design. Do note that I port all commits back into the kernel (but only for the files that are there), so please follow the commit message conventions from there.
I do think that we should also think about which other types could make use of implementing the trait that you introduce here.

In core we could additionally implement it for Option, ManuallyDrop and MaybeUninit (although MaybeUninit would probably be simpler to just initialize it after the PinInit in many cases).

MaybeUninit is the most important one IMO. ManuallyDrop normally is stored on the stack and thus it isn't as useful. Option might be useful, but feel free to leave that for a future PR.

In std we could implement it for the Mutex, Cell and RwLock family of types.

I don't think it's worth it for Mutex/RwLock. Cell needs to store Copy types to be useful and those rarely have a pin-initializer, so I also feel like it's better to add that when someone is using it.

In the kernel for Opaque although if this is needed its probably a misuse of Opaque anyways.

Why? We already have the pin_init function there.

@BennoLossin
Copy link
Copy Markdown
Member

Ah and one more thing, could you add the information about the feature gate to the readme and include an entry in the changelog? Thanks!

@onestacked
Copy link
Copy Markdown
Contributor Author

Should I do that as a separate commit?

Comment thread .github/workflows/ci.yml Outdated
@onestacked
Copy link
Copy Markdown
Contributor Author

onestacked commented Apr 18, 2025

In core we could additionally implement it for Option, ManuallyDrop and MaybeUninit (although MaybeUninit would probably be simpler to just initialize it after the PinInit in many cases).

MaybeUninit is the most important one IMO. ManuallyDrop normally is stored on the stack and thus it isn't as useful. Option might be useful, but feel free to leave that for a future PR.

I'll add MaybeUninit here then.

In std we could implement it for the Mutex, Cell and RwLock family of types.

I don't think it's worth it for Mutex/RwLock. Cell needs to store Copy types to be useful and those rarely have a pin-initializer, so I also feel like it's better to add that when someone is using it.
Possibly useful for RefCell, but yeah best to wait until someone has a use-case.

In the kernel for Opaque although if this is needed its probably a misuse of Opaque anyways.

Why? We already have the pin_init function there.

I guess its useful, I was mostly thinking that Opaques usually shouldn't be used (much) from the Rust side, I didn't think about cases where the C side needs pinning.

@onestacked onestacked force-pushed the add-try-pin-init-wrapper branch from ef8d073 to 3155517 Compare April 18, 2025 23:11
Comment thread build.rs
Copy link
Copy Markdown
Member

@BennoLossin BennoLossin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

a couple more minor changes, thanks for keeping up so quickly!

Comment thread src/lib.rs Outdated
Comment thread src/lib.rs
Comment thread src/lib.rs Outdated
Comment thread src/lib.rs Outdated
Comment thread src/lib.rs Outdated
Comment thread build.rs Outdated
Comment thread build.rs
Comment thread src/lib.rs Outdated
Comment thread README.md
Comment thread CHANGELOG.md Outdated
@onestacked onestacked force-pushed the add-try-pin-init-wrapper branch 2 times, most recently from 9440f53 to 391caae Compare April 19, 2025 08:44
This trait allows crating `PinInitializers` for wrapper or new-type
structs with the inner value structurally pinned, when given the
initializer for the inner value.

This commit implements this trait for `UnsafeCell` and `MaybeUninit`.

Signed-off-by: Christian Schrefl <chrisi.schrefl@gmail.com>
Add the `unsafe-pinned` feature which gates the `Wrapper`
implementation of the `core::pin::UnsafePinned` struct.

For now this is just a cargo feature, but once `core::pin::UnsafePinned`
is stable a config flag can be added to allow the usage of this
implementation in the linux kernel.

Signed-off-by: Christian Schrefl <chrisi.schrefl@gmail.com>
@onestacked onestacked force-pushed the add-try-pin-init-wrapper branch from 391caae to f670aa7 Compare April 19, 2025 12:44
Comment thread CHANGELOG.md Outdated
@onestacked onestacked force-pushed the add-try-pin-init-wrapper branch from f670aa7 to f7f4fc8 Compare April 19, 2025 13:42
Add Changelog entry for the `Wrapper` trait and document the
`unsafe-pinned` feature in the Readme.

Signed-off-by: Christian Schrefl <chrisi.schrefl@gmail.com>
The previous link anchor was broken in rust 1.77, because the
documentation was refactored in upstream rust.
Change the link to refer to the new section in the rust documentation.

Signed-off-by: Christian Schrefl <chrisi.schrefl@gmail.com>
@onestacked onestacked force-pushed the add-try-pin-init-wrapper branch from f7f4fc8 to a146142 Compare April 19, 2025 13:43
Copy link
Copy Markdown
Member

@BennoLossin BennoLossin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All looks good now, thanks a lot!

@BennoLossin BennoLossin merged commit 234ada6 into Rust-for-Linux:main Apr 19, 2025
14 checks passed
@onestacked onestacked deleted the add-try-pin-init-wrapper branch April 19, 2025 16:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

2 participants