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

utils: overhaul ImmutAfterInitCell #595

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

msft-jlange
Copy link
Collaborator

This change updates the design of ImmutAfterInitCell to resolve the issues described in #591, making it behave much more like a Sync version of OnceCell.

@msft-jlange msft-jlange force-pushed the immut branch 2 times, most recently from 2f1186b to 8d20675 Compare January 22, 2025 05:30
Copy link
Member

@joergroedel joergroedel left a comment

Choose a reason for hiding this comment

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

Overall the changes look good to me. There are a few open questions about using atomics and ordering I'd like to get sorted before merging.

kernel/src/console.rs Show resolved Hide resolved
kernel/src/utils/immut_after_init.rs Outdated Show resolved Hide resolved
kernel/src/utils/immut_after_init.rs Show resolved Hide resolved
@joergroedel joergroedel added the in-review PR is under active review and not yet approved label Jan 22, 2025
The reinitialization behavior of `ImmutAfterInitCell` is not safe
because it permits safe code to reinitialize a cell while another thread
may be borrowing the contents of the cell.  The most effective way to
avoid this conflict is to remove the support for reinitialization.  This
also means that there is no need to support construction of an
`ImmutAfterInitCell` with initialized contents.

Signed-off-by: Jon Lange <[email protected]>
Safety requires enforcement of soundness rules in all configurations,
not just those that enable debug assertions.  The initialization guards
of `ImmutAfterInitCell` should therefore not rely on debug assertions.
In addition, they should be enforced using atomic initialization guards
to ensure that initialization is safe with respect to possible
preemption by interrupt or exception handlers.  By making initialization
fully `Sync` compliant, there is no need to require initialization to
complete prior to multi-processor startup.

Signed-off-by: Jon Lange <[email protected]>
The `try_get_inner()` function is used to obtain the contents of an
`ImmutAfterInitCell` (or to return an error).  It should be possible for
a caller to obtain a `Result` instead of being forced to derefence and
panic if the cell is not initialized.

Signed-off-by: Jon Lange <[email protected]>
For maximum usefulness, it should be possible to support
`ImmutAfterInitCell` for types that do not implement `Copy`.  For
example, some structures that require global initialization may contain
atomics, which do not support `Copy`.  However, it is also important not
to require all initialization to be performed using moves, since a move
into a cell requires a temporary copy on the stack, and some global
types are especially large.  Therefore `ImmutAfterInitCell` can support
move-based initialization for all types, and by-reference initialization
for types that support `Copy`.

Signed-off-by: Jon Lange <[email protected]>
Add unit tests to validate the behavior of `ImmutAfterInitCell`.

Signed-off-by: Jon Lange <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
in-review PR is under active review and not yet approved
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants