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

implement Send + Sync for PhysicalMapping #101

Merged
merged 2 commits into from Jun 11, 2021

Conversation

Freax13
Copy link
Contributor

@Freax13 Freax13 commented Jun 10, 2021

This pr implements Send + Sync for PhysicalMapping and add a test to check for those implementations.

@IsaacWoods
Copy link
Member

IsaacWoods commented Jun 10, 2021

Hmm, I'm not convinced this would be sound. PhysicalMapping is meant as a very naive concept: "we've mapped some physical memory for you, it's your problem now" - we would need to enforce some sort of synchronization if we wanted it to be Sync, which probably belongs at a higher level if we need it.

Otoh, I think it could be Send, but only if T: Send / Sync(?) and H: Sync - however, I think this could just introduce a footgun that we don't need, and I'd need to mull it over a fair bit to work out if I've got those right (I'm a bit rusty atm, sorry about that ;)).

I think it would be helpful to understand what inspired this change; is it just for maximum permissiveness, or does it solve a problem you have?

@Freax13
Copy link
Contributor Author

Freax13 commented Jun 11, 2021

Hmm, I'm not convinced this would be sound. PhysicalMapping is meant as a very naive concept: "we've mapped some physical memory for you, it's your problem now" - we would need to enforce some sort of synchronization if we wanted it to be Sync, which probably belongs at a higher level if we need it.

PhysicalMapping is very similar to Box and Box has those exact implementations. To implement Sync you only need to make sure that you don't use interior mutability in a non-thread safe way (eg Rc or RefCell) or do other non-thread safe things.

I think it would be helpful to understand what inspired this change; is it just for maximum permissiveness, or does it solve a problem you have?

I want to store a PhysicalMapping in a static variable.

@IsaacWoods
Copy link
Member

PhysicalMapping is very similar to Box and Box has those exact implementations

At the moment PhysicalMapping is similar to Box, but this will probably not always be the case (this work hasn't been done just because I don't have the bandwidth currently, and I'm still not 100% on how to do it safely) - at the moment PhysicalMapping only provides immutable access to the underlying data, but this will not always be the case; in the future we will want to provide mutable access to the hardware register blocks and also to the FACS through PhysicalMapping. Because we don't control the underlying data representation, we can't just stick something over T to make that possible if we allow PhysicalMapping to be accessed from multiple threads - that's why I'm reticent about Sync.

I want to store a PhysicalMapping in a static variable.

This is not something I think we'd want to encourage - generally if you want a memory mapping to stick around for the entire duration of your system, I'm not sure our PhysicalMapping is the right building block - you should generally have some mechanism in your virtual memory system to allow this. When PhysicalMapping grows to allow mutable access, this will have similar semantics to static mut.

@Freax13
Copy link
Contributor Author

Freax13 commented Jun 11, 2021

Ok so if I understood you correctly you're saying that PhysicalMapping might possibly have interior mutability or some other non-thread safe operations in the future and that would obviously make it not Sync.

Because we don't control the underlying data representation, we can't just stick something over T to make that possible if we allow PhysicalMapping to be accessed from multiple threads - that's why I'm reticent about Sync.

Why don't you just stick something over PhysicalMapping. Unless you have different PhysicalMappings pointing to the same address that should work.

@IsaacWoods
Copy link
Member

IsaacWoods commented Jun 11, 2021

Ok so if I understood you correctly you're saying that PhysicalMapping might possibly have interior mutability or some other non-thread safe operations in the future and that would obviously make it not Sync.

Yes - sorry that context was probably missing at first.

Why don't you just stick something over PhysicalMapping.

Yes, that could be a solution in that case - the wrapper would then be responsible for making the overall type Sync if needed (see for example how spin::Mutex only requires T: Send for Mutex<T>: Sync). Another would be to add RwLock properties to PhysicalMapping itself - this is the option I've considered in the past, but I'm not convinced this is the right way.

Unless you have different PhysicalMappings pointing to the same address that should work.

A concern I have is that this is also not totally impossible. For example, when tying acpi and aml together, the LoadTable operation can load arbitrary tables from the XSDT, which could potentially map the same table in both acpi and aml.

Complexity like this is why I've been shying away from adding mutable access to PhysicalMapping, but we will need it at some point.

@Freax13
Copy link
Contributor Author

Freax13 commented Jun 11, 2021

That makes sense, thank you for your explanation 👍

In my case I'm storing it behind a SpinMutex anyway, so Send is enough for me. I get that that's perhaps not the best solution for the future but it works for me right now.

@IsaacWoods
Copy link
Member

Cool - I think Send is fine. The only remaining question is whether H needs to be Sync - my thought process here is that if we send one mapping to another thread, which then drops that mapping, we're accessing H from multiple threads. I sometimes find behavior around Send and Sync surprising though, so I might be mistaken?

@Freax13
Copy link
Contributor Author

Freax13 commented Jun 11, 2021

The only remaining question is whether H needs to be Sync - my thought process here is that if we send one mapping to another thread, which then drops that mapping, we're accessing H from multiple threads.

We're not accessing the same H instance from multiple threads though, because each mapping has its own instance, so just requiring Send is fine.

@IsaacWoods
Copy link
Member

Ah of course - thanks for explaining. As I said before, I'm a little rusty ;)

@IsaacWoods IsaacWoods merged commit 306cfe0 into rust-osdev:master Jun 11, 2021
@Freax13 Freax13 deleted the make-physical-mapping-send-sync branch June 11, 2021 17:46
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.

None yet

2 participants