Cleanups, improvements and documentation for percore derive - #44
Conversation
qwandor
changed the title
| // Safety: PercoreLocalOffset guarantees a valid offset. | ||
| let percore_ptr = unsafe { NonNull::from_ref(&self.0).byte_offset(percore_local_offset()) }; | ||
|
|
||
| assert!(percore_ptr.is_aligned()); |
There was a problem hiding this comment.
This introduces runtime overhead for every percore variable access. If we can guarantee that the alignment of each percpu section is at least as large as the maximum alignment required by any percore variable, then all percore will always be correctly aligned.
Ideally, this check would be performed at compile time, but I do not see a way to inject a constant here that represents the alignment of the percore section.
I tried to expose the percore section alignment by adding a new function to PercoreLocalOffset and calling it through an extern function. LTO optimizes the call away entirely or reduces it to an unconditional panic, but the check is still conceptually performed at runtime.
I also tried adding this assert to the LD script:
ASSERT(
ALIGNOF(.percore) <= CACHE_WRITEBACK_GRANULE,
".percore contains an object aligned to a larger boundary than the sections alignment"
)
Since the variables are sorted by alignment, that variable with the largest alignment determines the alignment of the section if its greater than the sections default alignment, i.e. CACHE_WRITEBACK_GRANULE. In this case the assert fails. I'm not sure how robust is this approach but it works, and at least it gives a linker error.
Any alternatives?
There was a problem hiding this comment.
I've changed this to a debug_assert!, added your suggestion to the linker script in the examples, and updated the safety comment.
| @@ -81,25 +81,27 @@ each secondary core's per-core area and implement `percore::derive::PercoreLocal | |||
| marked with `#[percore::percore_local_offset]` to retrieve the appropriate offset. | |||
There was a problem hiding this comment.
Please update to use the declarative macro.
| @@ -56,6 +56,8 @@ | |||
|
|
|||
There was a problem hiding this comment.
It would be nice to include README.md so doc-tests can cover the examples too.
There was a problem hiding this comment.
I'd rather not do that, it has extra content that I don't think belongs in the Rustdoc.
This allows the secondary copies to be before the .percore section rather than immediately after it, which may be useful if it they are dynamically allocated.
It must not be called after any access to a percore variable
proc macro. This lets it be separately from the type definition if desired (e.g. for a type provided by another library), and also fixes some namespace issues with how it refers to the percore crate. Also removed the #[inline(always)] attribute, as it doesn't work for exported functions and causes a warning.
…ures. Also added percore_size function, and made aarch64 module public rather than re-exporting its contents.
2 participants
percore_local_offsettoisize, as implementations may want to place a core's copy of percore variables before the.percoresection.percore_local_offseta declarative macro rather than a derive macro, to be more flexible and allow types from other crates to be specified as the desired implementation.percore_copy_secondary_datafor other architectures.