Skip to content

Commit

Permalink
[mini] Use atomics, instead of loader lock, for JIT wrappers
Browse files Browse the repository at this point in the history
Related to #93686

While this doesn't eliminate all deadlocks related to the global
loader lock and managed locks, it removes one unneeded use of the
loader lock.

The wrapper (and trampoline) of a JIT icall are only ever set from
NULL to non-NULL.  We can use atomics to deal with races instad of
double checked locking.  This was not the case historically, because
the JIT info was dynamically allocated - so we used the loader lock to
protect the integrity of the hash table
  • Loading branch information
lambdageek committed Apr 11, 2024
1 parent ff3c531 commit 9084c33
Show file tree
Hide file tree
Showing 6 changed files with 20 additions and 29 deletions.
4 changes: 2 additions & 2 deletions src/mono/mono/metadata/class-internals.h
Original file line number Diff line number Diff line change
Expand Up @@ -587,8 +587,8 @@ typedef struct {
// have both of them to be non-NULL.
const char *name;
gconstpointer func;
gconstpointer wrapper;
gconstpointer trampoline;
gconstpointer wrapper__;
gconstpointer trampoline__;
MonoMethodSignature *sig;
const char *c_symbol;
MonoMethod *wrapper_method;
Expand Down
4 changes: 3 additions & 1 deletion src/mono/mono/metadata/icall.c
Original file line number Diff line number Diff line change
Expand Up @@ -7243,6 +7243,8 @@ mono_create_icall_signatures (void)
}
}

/* LOCKING: does not take locks. does not use an atomic write to info->wrapper
*/
void
mono_register_jit_icall_info (MonoJitICallInfo *info, gconstpointer func, const char *name, MonoMethodSignature *sig, gboolean avoid_wrapper, const char *c_symbol)
{
Expand All @@ -7256,7 +7258,7 @@ mono_register_jit_icall_info (MonoJitICallInfo *info, gconstpointer func, const
// Fill in wrapper ahead of time, to just be func, to avoid
// later initializing it to anything else. So therefore, no wrapper.
if (avoid_wrapper) {
info->wrapper = func;
info->wrapper__ = func;
} else {
// Leave it alone in case of a race.
}
Expand Down
4 changes: 2 additions & 2 deletions src/mono/mono/mini/aot-compiler.c
Original file line number Diff line number Diff line change
Expand Up @@ -6942,7 +6942,7 @@ emit_and_reloc_code (MonoAotCompile *acfg, MonoMethod *method, guint8 *code, gui
}
} else if (patch_info->type == MONO_PATCH_INFO_JIT_ICALL_ID) {
MonoJitICallInfo * const info = mono_find_jit_icall_info (patch_info->data.jit_icall_id);
if (!got_only && info->func == info->wrapper) {
if (!got_only && info->func == info->wrapper__) {
const char * sym = NULL;
if (patch_info->data.jit_icall_id == MONO_JIT_ICALL_mono_dummy_runtime_init_callback) {
g_assert (acfg->aot_opts.static_link && acfg->aot_opts.runtime_init_callback != NULL);
Expand Down Expand Up @@ -10583,7 +10583,7 @@ mono_aot_get_direct_call_symbol (MonoJumpInfoType type, gconstpointer data)
sym = lookup_direct_pinvoke_symbol_name_aot (llvm_acfg, method);
} else if (type == MONO_PATCH_INFO_JIT_ICALL_ID && (direct_calls || (MonoJitICallId)(gsize)data == MONO_JIT_ICALL_mono_dummy_runtime_init_callback)) {
MonoJitICallInfo const * const info = mono_find_jit_icall_info ((MonoJitICallId)(gsize)data);
if (info->func == info->wrapper) {
if (info->func == info->wrapper__) {
if ((MonoJitICallId)(gsize)data == MONO_JIT_ICALL_mono_dummy_runtime_init_callback) {
sym = llvm_acfg->aot_opts.runtime_init_callback;
} else {
Expand Down
2 changes: 1 addition & 1 deletion src/mono/mono/mini/mini-amd64.c
Original file line number Diff line number Diff line change
Expand Up @@ -3161,7 +3161,7 @@ emit_call (MonoCompile *cfg, MonoCallInst *call, guint8 *code, MonoJitICallId ji
patch.type = MONO_PATCH_INFO_JIT_ICALL_ID;
patch.target = GUINT_TO_POINTER (jit_icall_id);

if (info->func == info->wrapper) {
if (info->func == info->wrapper__) {
/* No wrapper */
if ((((guint64)info->func) >> 32) == 0)
near_call = TRUE;
Expand Down
4 changes: 2 additions & 2 deletions src/mono/mono/mini/mini-llvm.c
Original file line number Diff line number Diff line change
Expand Up @@ -2168,7 +2168,7 @@ get_callee_llvmonly (EmitContext *ctx, LLVMTypeRef llvm_sig, MonoJumpInfoType ty
if (type == MONO_PATCH_INFO_JIT_ICALL_ID) {
MonoJitICallInfo * const info = mono_find_jit_icall_info ((MonoJitICallId)(gsize)data);
g_assert (info);
if (info->func != info->wrapper) {
if (info->func != info->wrapper__) {
type = MONO_PATCH_INFO_METHOD;
data = mono_icall_get_wrapper_method (info);
callee_name = mono_aot_get_mangled_method_name ((MonoMethod*)data);
Expand Down Expand Up @@ -2211,7 +2211,7 @@ get_callee_llvmonly (EmitContext *ctx, LLVMTypeRef llvm_sig, MonoJumpInfoType ty
if (ctx->module->assembly->image == mono_get_corlib () && type == MONO_PATCH_INFO_JIT_ICALL_ID) {
MonoJitICallInfo * const info = mono_find_jit_icall_info ((MonoJitICallId)(gsize)data);

if (info->func != info->wrapper) {
if (info->func != info->wrapper__) {
type = MONO_PATCH_INFO_METHOD;
data = mono_icall_get_wrapper_method (info);
}
Expand Down
31 changes: 10 additions & 21 deletions src/mono/mono/mini/mini-runtime.c
Original file line number Diff line number Diff line change
Expand Up @@ -654,31 +654,28 @@ mono_icall_get_wrapper_full (MonoJitICallInfo* callinfo, gboolean do_compile)
MonoMethod *wrapper;
gconstpointer addr, trampoline;

if (callinfo->wrapper)
return callinfo->wrapper;
if (callinfo->wrapper__)
return callinfo->wrapper__;

wrapper = mono_icall_get_wrapper_method (callinfo);

if (do_compile) {
addr = mono_compile_method_checked (wrapper, error);
mono_error_assert_ok (error);
mono_memory_barrier ();
callinfo->wrapper = addr;
callinfo->wrapper__ = addr;
return addr;
} else {
if (callinfo->trampoline)
return callinfo->trampoline;
if (callinfo->trampoline__)
return callinfo->trampoline__;
trampoline = mono_create_jit_trampoline (wrapper, error);
mono_error_assert_ok (error);
trampoline = mono_create_ftnptr ((gpointer)trampoline);

mono_loader_lock ();
if (!callinfo->trampoline) {
callinfo->trampoline = trampoline;
}
mono_loader_unlock ();

mono_atomic_cas_ptr ((volatile gpointer*)&callinfo->trampoline__, (void*)trampoline, NULL);

return callinfo->trampoline;
return (gconstpointer)mono_atomic_load_ptr ((volatile gpointer*)&callinfo->trampoline__);
}
}

Expand Down Expand Up @@ -2856,18 +2853,10 @@ mono_jit_compile_method_with_opt (MonoMethod *method, guint32 opt, gboolean jit_
p = mono_create_ftnptr (code);

if (callinfo) {
// FIXME Locking here is somewhat historical due to mono_register_jit_icall_wrapper taking loader lock.
// atomic_compare_exchange should suffice.
mono_loader_lock ();
mono_jit_lock ();
if (!callinfo->wrapper) {
callinfo->wrapper = p;
}
mono_jit_unlock ();
mono_loader_unlock ();
mono_atomic_cas_ptr ((volatile void*)&callinfo->wrapper__, NULL, p);
p = mono_atomic_load_ptr((volatile gpointer*)&callinfo->wrapper__);
}

// FIXME p or callinfo->wrapper or does not matter?
return p;
}

Expand Down

0 comments on commit 9084c33

Please sign in to comment.