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

Protect VmCtx and ID_COUNTER with RwLock #213

Closed
wants to merge 1 commit into from

Conversation

jarkkojs
Copy link
Collaborator

@jarkkojs jarkkojs commented Nov 10, 2024

    Protect VmCtx and ID_COUNTER with RwLock
    
    AtomicU64 causes compilation issues when used as a subcrate of another
    crate when the toolchain configurations conflict. The problem was observed
    with ID_COUNTER when Polkadot SDK was compiled for riscv32emac, which could
    not be found from core.
    
    The documentation also states that:
    
    "This type is only available on platforms that support atomic loads and
    stores of u64."
    
    Address this by protecting ID_COUNTER with `spin::RwLock` and `VmCtx` with
    `std::sync::RwLock`.

@jarkkojs jarkkojs requested review from athei and koute November 10, 2024 20:28
@jarkkojs jarkkojs force-pushed the master branch 20 times, most recently from b5d428a to a312b50 Compare November 11, 2024 02:11
@jarkkojs jarkkojs marked this pull request as draft November 11, 2024 02:11
@jarkkojs jarkkojs force-pushed the master branch 7 times, most recently from f6b13df to 8018435 Compare November 11, 2024 03:16
@jarkkojs jarkkojs marked this pull request as ready for review November 11, 2024 03:45
@jarkkojs jarkkojs force-pushed the master branch 16 times, most recently from bf04789 to 3e61c95 Compare November 11, 2024 06:00
@jarkkojs
Copy link
Collaborator Author

This is from discussion where the conclusion was that kernel will never use Rust's atomic type, as it BTW does not use C's atomic types:

https://lwn.net/ml/linux-kernel/CAHk-=whY5A=S=bLwCFL=043DoR0TTgSDUmfPDx2rXhkk3KANPQ@mail.gmail.com/

Linus is absolutely correct in what he is saying here. Generally language provided atomic is types are unreliable and should be avoided when there is no actual need. By heavily deploying atomic like it is deployed like now in the code base without compiler we make the code only more sensitive to the compiler bugs. And also incompatibilities as documentation specifically says not to support AtomicU64 on 32-bit architectures.

Also in most cases where atomic is used and where I migrated to plain u64 it has no value. E.g. register state is defined as the atomic snapshot of all registers.

To summarize that using more coarse locking we make the code more robust and compatible despite some minor differences in the environment.

Thus, I ended up that better granularity is to protect VmCtx and ID_COUNTER. This should make the code a factor more reliable too: you can change this with the lock and be sure that all other threads see only the final state.

In the crates I used spin, which I've previous used in https://github.com/enarx/enarx and also Google recommends this crate for bare metal, which also PolkaVM is (it is software-define bare-mental or this is how I like to think of it):

https://google.github.io/comprehensive-rust/bare-metal/useful-crates/spin.html

@jarkkojs jarkkojs force-pushed the master branch 7 times, most recently from 0793137 to 57e9c4d Compare November 12, 2024 04:04
AtomicU64 causes compilation issues when used as a subcrate of another
crate when the toolchain configurations conflict. The problem was observed
with ID_COUNTER when Polkadot SDK was compiled for riscv32emac, which could
not be found from core.

The documentation also states that:

"This type is only available on platforms that support atomic loads and
stores of u64."

Address this by protecting ID_COUNTER with `spin::RwLock` and `VmCtx` with
`std::sync::RwLock`.

Signed-off-by: Jarkko Sakkinen <[email protected]>
@jarkkojs
Copy link
Collaborator Author

I'll close this now based on discussion with @koute. I'll back it up with a branch in my tree

@jarkkojs jarkkojs closed this Nov 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant