[erlang-bugs] hipe_mfait_lock
Louis-Philippe Gauthier
louis-philippe.gauthier@REDACTED
Mon Feb 23 17:58:13 CET 2015
Hi Mikael,
I got a bit excited and tried it right away. Seems to fix my contention
issue on hipe_mfait_lock.
Here's the new lock count output (hipe_mfait_lock is not there!):
https://gist.github.com/lpgauth/e85efa59b96726af235d
Thanks,
LP
On Mon, Feb 23, 2015 at 9:28 AM, Louis-Philippe Gauthier <
louis-philippe.gauthier@REDACTED> wrote:
> Hi Mikael,
> Awesome work, I'll give the patch a try this week.
>
> Thanks,
> LP
>
> On Mon, Feb 23, 2015 at 6:30 AM, Mikael Pettersson <mikpelinux@REDACTED>
> wrote:
>
>> Louis-Philippe Gauthier writes:
>> > Hi Sverker,
>> > That's not a bad idea, I'll try that approach!
>> >
>> > Thanks,
>> > LP
>> >
>> > On Thu, Jan 15, 2015 at 12:27 AM, Sverker Eriksson <
>> > sverker.eriksson@REDACTED> wrote:
>> >
>> > > One quite simple improvement is to change the hipe_mfait_lock to be
>> a
>> > > read-write mutex (erts_smp_rwmtx_t).
>> > > And then do read-lock at lookup and exclusive write-lock only when
>> > > inserting a new entry in the table.
>> > >
>> > > I don't think that requires much knowledge about the VM to pull off
>> ;-) .
>> > >
>> > > /Sverker, Erlang/OTP
>> > >
>> > >
>> > >
>> > >
>> > > On 01/14/2015 12:34 AM, Louis-Philippe Gauthier wrote:
>> > >
>> > > Hi erlang-bugs,
>> > > I couple of months ago I tried running our full application using
>> HiPE. I
>> > > ran into several issues, some manageable, some not... The most
>> problematic
>> > > issue was due to locking, specifically, the hipe_mfait_lock (you can
>> see
>> > > the lock counter output in the gist bellow).
>> > > https://gist.github.com/lpgauth/2b3220f4bceeed6f62d0
>> > >
>> > > Looking at the code it's obvious that this is a known problem... the
>> > > following comment was added when the lock was added in 2010.
>> > >
>> > > "XXX: Redesign apply et al to avoid those updates."
>> https://github.com/erlang/otp/blob/maint/erts/emulator/hipe/hipe_bif0.c#L1218
>> > >
>> > > Unfortunately, I'm don't know the runtime enough to start patching
>> it... so
>> > > instead I'm reporting it.
>>
>> Please try the attached patch (against 17.4, should apply to older
>> versions too by I haven't checked), and let me know if it makes a
>> noticeable improvement to your use case.
>>
>> Run-time apply:s now do their lookups using a shared read-lock, and
>> only if their lookups fail do they re-lock with an exclusive write-lock
>> in order to insert new data. Module loading functions always take
>> exclusive locks, which matches the current behaviour.
>>
>> /Mikael
>>
>>
>> --- otp_src_17.4/erts/emulator/hipe/hipe_bif0.c.~1~ 2014-12-09
>> 21:11:07.000000000 +0100
>> +++ otp_src_17.4/erts/emulator/hipe/hipe_bif0.c 2015-02-22
>> 21:32:55.361884741 +0100
>> @@ -1217,22 +1217,32 @@ static struct {
>> * they create a new stub for the mfa, which forces locking.
>> * XXX: Redesign apply et al to avoid those updates.
>> */
>> - erts_smp_mtx_t lock;
>> + erts_smp_rwmtx_t lock;
>> } hipe_mfa_info_table;
>>
>> static inline void hipe_mfa_info_table_init_lock(void)
>> {
>> - erts_smp_mtx_init(&hipe_mfa_info_table.lock, "hipe_mfait_lock");
>> + erts_smp_rwmtx_init(&hipe_mfa_info_table.lock, "hipe_mfait_lock");
>> }
>>
>> -static inline void hipe_mfa_info_table_lock(void)
>> +static inline void hipe_mfa_info_table_rlock(void)
>> {
>> - erts_smp_mtx_lock(&hipe_mfa_info_table.lock);
>> + erts_smp_rwmtx_rlock(&hipe_mfa_info_table.lock);
>> }
>>
>> -static inline void hipe_mfa_info_table_unlock(void)
>> +static inline void hipe_mfa_info_table_runlock(void)
>> {
>> - erts_smp_mtx_unlock(&hipe_mfa_info_table.lock);
>> + erts_smp_rwmtx_runlock(&hipe_mfa_info_table.lock);
>> +}
>> +
>> +static inline void hipe_mfa_info_table_rwlock(void)
>> +{
>> + erts_smp_rwmtx_rwlock(&hipe_mfa_info_table.lock);
>> +}
>> +
>> +static inline void hipe_mfa_info_table_rwunlock(void)
>> +{
>> + erts_smp_rwmtx_rwunlock(&hipe_mfa_info_table.lock);
>> }
>>
>> #define HIPE_MFA_HASH(M,F,A) ((M) * (F) + (A))
>> @@ -1333,6 +1343,7 @@ void *hipe_mfa_find_na(Eterm m, Eterm f,
>> }
>> #endif
>>
>> +/* PRE: called with write lock held */
>> static struct hipe_mfa_info *hipe_mfa_info_table_put_locked(Eterm m,
>> Eterm f, unsigned int arity)
>> {
>> unsigned long h;
>> @@ -1362,7 +1373,7 @@ static void hipe_mfa_set_na(Eterm m, Ete
>> {
>> struct hipe_mfa_info *p;
>>
>> - hipe_mfa_info_table_lock();
>> + hipe_mfa_info_table_rwlock();
>> p = hipe_mfa_info_table_put_locked(m, f, arity);
>> #ifdef DEBUG_LINKER
>> printf("%s: ", __FUNCTION__);
>> @@ -1372,7 +1383,7 @@ static void hipe_mfa_set_na(Eterm m, Ete
>> p->local_address = address;
>> if (is_exported)
>> p->remote_address = address;
>> - hipe_mfa_info_table_unlock();
>> + hipe_mfa_info_table_rwunlock();
>> }
>>
>> #if defined(__powerpc__) || defined(__ppc__) || defined(__powerpc64__)
>> || defined(__arm__)
>> @@ -1381,10 +1392,10 @@ void *hipe_mfa_get_trampoline(Eterm m, E
>> struct hipe_mfa_info *p;
>> void *trampoline;
>>
>> - hipe_mfa_info_table_lock();
>> - p = hipe_mfa_info_table_put_locked(m, f, arity);
>> - trampoline = p->trampoline;
>> - hipe_mfa_info_table_unlock();
>> + hipe_mfa_info_table_rlock();
>> + p = hipe_mfa_info_table_get_locked(m, f, arity);
>> + trampoline = p ? p->trampoline : NULL;
>> + hipe_mfa_info_table_runlock();
>> return trampoline;
>> }
>>
>> @@ -1392,10 +1403,10 @@ void hipe_mfa_set_trampoline(Eterm m, Et
>> {
>> struct hipe_mfa_info *p;
>>
>> - hipe_mfa_info_table_lock();
>> + hipe_mfa_info_table_rwlock();
>> p = hipe_mfa_info_table_put_locked(m, f, arity);
>> p->trampoline = trampoline;
>> - hipe_mfa_info_table_unlock();
>> + hipe_mfa_info_table_rwunlock();
>> }
>> #endif
>>
>> @@ -1426,7 +1437,7 @@ BIF_RETTYPE hipe_bifs_invalidate_funinfo
>> struct mfa mfa;
>> struct hipe_mfa_info *p;
>>
>> - hipe_mfa_info_table_lock();
>> + hipe_mfa_info_table_rwlock();
>> lst = BIF_ARG_1;
>> while (is_list(lst)) {
>> if (!term_to_mfa(CAR(list_val(lst)), &mfa))
>> @@ -1455,7 +1466,7 @@ BIF_RETTYPE hipe_bifs_invalidate_funinfo
>> }
>> }
>> }
>> - hipe_mfa_info_table_unlock();
>> + hipe_mfa_info_table_rwunlock();
>> if (is_not_nil(lst))
>> BIF_ERROR(BIF_P, BADARG);
>> BIF_RET(NIL);
>> @@ -1469,7 +1480,7 @@ void hipe_mfa_save_orig_beam_op(Eterm mo
>> orig_beam_op = pc[0];
>> if (orig_beam_op != BeamOpCode(op_hipe_trap_call_closure) &&
>> orig_beam_op != BeamOpCode(op_hipe_trap_call)) {
>> - hipe_mfa_info_table_lock();
>> + hipe_mfa_info_table_rwlock();
>> p = hipe_mfa_info_table_put_locked(mod, fun, ari);
>> #ifdef DEBUG_LINKER
>> printf("%s: ", __FUNCTION__);
>> @@ -1478,7 +1489,7 @@ void hipe_mfa_save_orig_beam_op(Eterm mo
>> #endif
>> p->beam_code = pc;
>> p->orig_beam_op = orig_beam_op;
>> - hipe_mfa_info_table_unlock();
>> + hipe_mfa_info_table_rwunlock();
>> } else {
>> #ifdef DEBUG_LINKER
>> printf("%s: ", __FUNCTION__);
>> @@ -1505,7 +1516,8 @@ static void *hipe_make_stub(Eterm m, Ete
>> return StubAddress;
>> }
>>
>> -static void *hipe_get_na_nofail_locked(Eterm m, Eterm f, unsigned int a,
>> int is_remote)
>> +/* PRE: called with read or write lock held */
>> +static void *hipe_get_na_try_locked(Eterm m, Eterm f, unsigned int a,
>> int is_remote, struct hipe_mfa_info **pp)
>> {
>> struct hipe_mfa_info *p;
>> void *address;
>> @@ -1523,7 +1535,20 @@ static void *hipe_get_na_nofail_locked(E
>> address = p->remote_address;
>> if (address)
>> return address;
>> - } else
>> + }
>> + /* Caller must take the slow path with the write lock held, but allow
>> + it to avoid some work if it already holds the write lock. */
>> + if (pp)
>> + *pp = p;
>> + return NULL;
>> +}
>> +
>> +/* PRE: called with write lock held */
>> +static void *hipe_get_na_slow_locked(Eterm m, Eterm f, unsigned int a,
>> int is_remote, struct hipe_mfa_info *p)
>> +{
>> + void *address;
>> +
>> + if (!p)
>> p = hipe_mfa_info_table_put_locked(m, f, a);
>> address = hipe_make_stub(m, f, a, is_remote);
>> /* XXX: how to tell if a BEAM MFA is exported or not? */
>> @@ -1531,14 +1556,34 @@ static void *hipe_get_na_nofail_locked(E
>> return address;
>> }
>>
>> +/* PRE: called with write lock held */
>> +static void *hipe_get_na_nofail_locked(Eterm m, Eterm f, unsigned int a,
>> int is_remote)
>> +{
>> + struct hipe_mfa_info *p /*= NULL*/;
>> + void *address;
>> +
>> + address = hipe_get_na_try_locked(m, f, a, is_remote, &p);
>> + if (address)
>> + return address;
>> +
>> + address = hipe_get_na_slow_locked(m, f, a, is_remote, p);
>> + return address;
>> +}
>> +
>> static void *hipe_get_na_nofail(Eterm m, Eterm f, unsigned int a, int
>> is_remote)
>> {
>> - void *p;
>> + void *address;
>>
>> - hipe_mfa_info_table_lock();
>> - p = hipe_get_na_nofail_locked(m, f, a, is_remote);
>> - hipe_mfa_info_table_unlock();
>> - return p;
>> + hipe_mfa_info_table_rlock();
>> + address = hipe_get_na_try_locked(m, f, a, is_remote, NULL);
>> + hipe_mfa_info_table_runlock();
>> + if (address)
>> + return address;
>> +
>> + hipe_mfa_info_table_rwlock();
>> + address = hipe_get_na_slow_locked(m, f, a, is_remote, NULL);
>> + hipe_mfa_info_table_rwunlock();
>> + return address;
>> }
>>
>> /* used for apply/3 in hipe_mode_switch */
>> @@ -1617,7 +1662,7 @@ int hipe_find_mfa_from_ra(const void *ra
>> /* Note about locking: the table is only updated from the
>> loader, which runs with the rest of the system suspended. */
>> /* XXX: alas not true; see comment at hipe_mfa_info_table.lock */
>> - hipe_mfa_info_table_lock();
>> + hipe_mfa_info_table_rwlock();
>> bucket = hipe_mfa_info_table.bucket;
>> nrbuckets = 1 << hipe_mfa_info_table.log2size;
>> mfa = NULL;
>> @@ -1638,7 +1683,7 @@ int hipe_find_mfa_from_ra(const void *ra
>> *f = mfa->f;
>> *a = mfa->a;
>> }
>> - hipe_mfa_info_table_unlock();
>> + hipe_mfa_info_table_rwunlock();
>> return mfa ? 1 : 0;
>> }
>>
>> @@ -1715,7 +1760,7 @@ BIF_RETTYPE hipe_bifs_add_ref_2(BIF_ALIS
>> default:
>> goto badarg;
>> }
>> - hipe_mfa_info_table_lock();
>> + hipe_mfa_info_table_rwlock();
>> callee_mfa = hipe_mfa_info_table_put_locked(callee.mod, callee.fun,
>> callee.ari);
>> caller_mfa = hipe_mfa_info_table_put_locked(caller.mod, caller.fun,
>> caller.ari);
>>
>> @@ -1731,7 +1776,7 @@ BIF_RETTYPE hipe_bifs_add_ref_2(BIF_ALIS
>> ref->flags = flags;
>> ref->next = callee_mfa->referred_from;
>> callee_mfa->referred_from = ref;
>> - hipe_mfa_info_table_unlock();
>> + hipe_mfa_info_table_rwunlock();
>>
>> BIF_RET(NIL);
>>
>> @@ -1751,12 +1796,12 @@ BIF_RETTYPE hipe_bifs_mark_referred_from
>>
>> if (!term_to_mfa(BIF_ARG_1, &mfa))
>> BIF_ERROR(BIF_P, BADARG);
>> - hipe_mfa_info_table_lock();
>> + hipe_mfa_info_table_rwlock();
>> p = hipe_mfa_info_table_get_locked(mfa.mod, mfa.fun, mfa.ari);
>> if (p)
>> for (ref = p->referred_from; ref != NULL; ref = ref->next)
>> ref->flags |= REF_FLAG_PENDING_REDIRECT;
>> - hipe_mfa_info_table_unlock();
>> + hipe_mfa_info_table_rwunlock();
>> BIF_RET(NIL);
>> }
>>
>> @@ -1770,7 +1815,7 @@ static void hipe_purge_all_refs(void)
>> struct hipe_mfa_info **bucket;
>> unsigned int i, nrbuckets;
>>
>> - hipe_mfa_info_table_lock();
>> + hipe_mfa_info_table_rwlock();
>>
>> bucket = hipe_mfa_info_table.bucket;
>> nrbuckets = 1 << hipe_mfa_info_table.log2size;
>> @@ -1792,7 +1837,7 @@ static void hipe_purge_all_refs(void)
>> erts_free(ERTS_ALC_T_HIPE, mfa);
>> }
>> }
>> - hipe_mfa_info_table_unlock();
>> + hipe_mfa_info_table_rwunlock();
>> }
>>
>> BIF_RETTYPE hipe_bifs_remove_refs_from_1(BIF_ALIST_1)
>> @@ -1809,7 +1854,7 @@ BIF_RETTYPE hipe_bifs_remove_refs_from_1
>>
>> if (!term_to_mfa(BIF_ARG_1, &mfa))
>> BIF_ERROR(BIF_P, BADARG);
>> - hipe_mfa_info_table_lock();
>> + hipe_mfa_info_table_rwlock();
>> caller_mfa = hipe_mfa_info_table_get_locked(mfa.mod, mfa.fun,
>> mfa.ari);
>> if (caller_mfa) {
>> refers_to = caller_mfa->refers_to;
>> @@ -1840,7 +1885,7 @@ BIF_RETTYPE hipe_bifs_remove_refs_from_1
>> }
>> caller_mfa->refers_to = NULL;
>> }
>> - hipe_mfa_info_table_unlock();
>> + hipe_mfa_info_table_rwunlock();
>> BIF_RET(am_ok);
>> }
>>
>> @@ -1859,7 +1904,7 @@ BIF_RETTYPE hipe_bifs_redirect_referred_
>>
>> if (!term_to_mfa(BIF_ARG_1, &mfa))
>> BIF_ERROR(BIF_P, BADARG);
>> - hipe_mfa_info_table_lock();
>> + hipe_mfa_info_table_rwlock();
>> p = hipe_mfa_info_table_get_locked(mfa.mod, mfa.fun, mfa.ari);
>> if (p) {
>> prev = &p->referred_from;
>> @@ -1890,7 +1935,7 @@ BIF_RETTYPE hipe_bifs_redirect_referred_
>> }
>> }
>> }
>> - hipe_mfa_info_table_unlock();
>> + hipe_mfa_info_table_rwunlock();
>> BIF_RET(NIL);
>> }
>>
>>
>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://erlang.org/pipermail/erlang-bugs/attachments/20150223/6957a650/attachment.htm>
More information about the erlang-bugs
mailing list