Dynlink library: use a global lock#11032
Conversation
otherlibs/dynlink/dynlink_common.ml
Outdated
| with_lock (fun () -> | ||
| if not !inited then begin | ||
| P.init (); | ||
| default_available_units (); |
There was a problem hiding this comment.
default_available_units must only be called when holding the lock. Ideally this would be documented with:
- a documentation comment on the function
- or a name change (
default_available_units_with_lock?) - we could even think of an overkill API where
with_lockprovides an abstract token type (a proof of locking) anddefault_available_unitrequirs it.
There was a problem hiding this comment.
For 3, a simpler API could be to have with_lock provide access to the global state of Dynlink (in a record?).
There was a problem hiding this comment.
I like this suggestion, it would also make good progress towards a better, multicore-friendly API.
| let with_lock lock f = | ||
| Mutex.lock lock; | ||
| Fun.protect f | ||
| ~finally:(fun () -> Mutex.unlock lock) |
There was a problem hiding this comment.
Remark for later: I'd like to have this in Mutex! In the meantime, this could go in Misc.
| Symtable.patch_object code compunit.cu_reloc; | ||
| Symtable.check_global_initialized compunit.cu_reloc; | ||
| Symtable.update_global_table () | ||
| let clos = with_lock lock (fun () -> |
There was a problem hiding this comment.
Can you comment on why you decided to widen the critical section here? Did we miss a protected-state access in the non-locked sections previously, or is it for some other reason?
There was a problem hiding this comment.
The section was widened to protect
let old_state = Symtable.current_state () in
...
if priv then Symtable.hide_additions old_statefrom a concurrent write on the Symtable. And also because it seemed simpler to be as coarse as possible with the lock.
otherlibs/dynlink/dynlink_common.ml
Outdated
|
|
||
| let unsafe_get_global_value ~bytecode_or_asm_symbol = | ||
| with_lock (fun () -> P.unsafe_get_global_value ~bytecode_or_asm_symbol) | ||
| with_lock (fun _ -> P.unsafe_get_global_value ~bytecode_or_asm_symbol) |
There was a problem hiding this comment.
Why do we need to lock here? Is it related to the use of Symtable in the bytecode version?
There was a problem hiding this comment.
Yes, the bytecode version reads the Symtable.
There was a problem hiding this comment.
maybe a comment could be nice.
We could also pass the lock to P.unsafe_get_global_value. No strong opinion, I just found that it was strange to lock and then ignore the state.
gasche
left a comment
There was a problem hiding this comment.
This looks good, thanks a lot! I left a couple minor comments, you can do whatever.
otherlibs/dynlink/dynlink_common.ml
Outdated
|
|
||
| let unsafe_get_global_value ~bytecode_or_asm_symbol = | ||
| with_lock (fun () -> P.unsafe_get_global_value ~bytecode_or_asm_symbol) | ||
| with_lock (fun _ -> P.unsafe_get_global_value ~bytecode_or_asm_symbol) |
There was a problem hiding this comment.
maybe a comment could be nice.
We could also pass the lock to P.unsafe_get_global_value. No strong opinion, I just found that it was strange to lock and then ignore the state.
Allow to dynlink outside of the main domains, but with a global lock
|
@Octachron I think this would deserve a Changes entry. (It's a non-trivial piece of stdlib adaptation.) |
97a7b4d to
ee68e9e
Compare
|
@Octachron you made some changes and the CI is now green. Do you want to merge now? (Otherwise you should say when it's ready, possibly by using the merge-me label.) |
|
I am mostly left wondering if I want to keep the test generator in tree or reduce the parameters of the test. I will merge once I decide that not changing anything is fine. |
|
I have reduced the number of plugins by two (while checking that the test still fail if the bytecode doesn't acquire the lock when modifying the global |
This PR tweaks the current implementation of the Dynlink implementation to allow loading plugins from any domains by adding a global lock to all the library entry point.
There is some subtlety on the bytecode side, where we need to release the lock to run the dynlinked module initialization code, but reacquire it before editing the global
Symtable.The test is probably excessive: it loads a 4 layers of interdependent plugins and spawns 20 domains over its course. However, it is automatically generated and can be reduced. The test generator is currently committed, but I am not sure that we want to keep it in tree.