This is the mail archive of the
libc-alpha@sourceware.org
mailing list for the glibc project.
Re: [PATCH v6 3/3] Mutex: Replace trylock by read only while spinning
- From: "H.J. Lu" <hjl dot tools at gmail dot com>
- To: Kemi Wang <kemi dot wang at intel dot com>
- Cc: Adhemerval Zanella <adhemerval dot zanella at linaro dot org>, Florian Weimer <fweimer at redhat dot com>, Rical Jason <rj at 2c3t dot io>, Carlos Donell <carlos at redhat dot com>, Glibc alpha <libc-alpha at sourceware dot org>, Dave Hansen <dave dot hansen at linux dot intel dot com>, Tim Chen <tim dot c dot chen at intel dot com>, Andi Kleen <andi dot kleen at intel dot com>, Ying Huang <ying dot huang at intel dot com>, Aaron Lu <aaron dot lu at intel dot com>, Lu Aubrey <aubrey dot li at intel dot com>
- Date: Tue, 3 Jul 2018 06:16:59 -0700
- Subject: Re: [PATCH v6 3/3] Mutex: Replace trylock by read only while spinning
- References: <1530520046-18343-1-git-send-email-kemi.wang@intel.com> <1530520046-18343-3-git-send-email-kemi.wang@intel.com> <CAMe9rOoTnUqqW96rVsFuXWjqW_38ziyKFaw7Q-ROL473d7eG6A@mail.gmail.com>
On Tue, Jul 3, 2018 at 5:45 AM, H.J. Lu <hjl.tools@gmail.com> wrote:
> On Mon, Jul 2, 2018 at 1:27 AM, Kemi Wang <kemi.wang@intel.com> wrote:
>> The pthread adaptive spin mutex spins on the lock for a while before
>> calling into the kernel to block. But, in the current implementation of
>> spinning, the spinners go straight back to LLL_MUTEX_TRYLOCK(cmpxchg) when
>> the lock is contended, it is not a good idea on many targets as that will
>> force expensive memory synchronization among processors and penalize other
>> running threads. For example, it constantly floods the system with "read
>> for ownership" requests, which are much more expensive to process than a
>> single read. Thus, we only use MO read until we observe the lock to not be
>> acquired anymore, as suggested by Andi Kleen.
>>
>> Performance impact:
>> Significant mutex performance improvement is not expected for this patch,
>> though, it probably bring some benefit for the scenarios with severe lock
>> contention on many architectures, the whole system performance can benefit
>> from this modification because a number of unnecessary "read for ownership"
>> requests which stress the cache system by broadcasting cache line
>> invalidity are eliminated during spinning.
>> Meanwhile, it may have some tiny performance regression on the lock holder
>> transformation for the case of lock acquisition via spinning gets, because
>> the lock state is checked before acquiring the lock via trylock.
>>
>> Similar mechanism has been implemented for pthread spin lock.
>>
>> Test machine:
>> 2-sockets Skylake platform, 112 cores with 62G RAM
>>
>> Test case: mutex-adaptive-thread (Contended pthread adaptive spin mutex
>> with global update)
>> Usage: make bench BENCHSET=mutex-adaptive-thread
>> Test result:
>> +----------------+-----------------+-----------------+------------+
>> | Configuration | Base | Head | % Change |
>> | | Total iteration | Total iteration | base->head |
>> +----------------+-----------------+-----------------+------------+
>> | | Critical section size: 1x |
>> +----------------+------------------------------------------------+
>> |1 thread | 2.76681e+07 | 2.7965e+07 | +1.1% |
>> |2 threads | 3.29905e+07 | 3.55279e+07 | +7.7% |
>> |3 threads | 4.38102e+07 | 3.98567e+07 | -9.0% |
>> |4 threads | 1.72172e+07 | 2.09498e+07 | +21.7% |
>> |28 threads | 1.03732e+07 | 1.05133e+07 | +1.4% |
>> |56 threads | 1.06308e+07 | 5.06522e+07 | +14.6% |
>> |112 threads | 8.55177e+06 | 1.02954e+07 | +20.4% |
>> +----------------+------------------------------------------------+
>> | | Critical section size: 10x |
>> +----------------+------------------------------------------------+
>> |1 thread | 1.57006e+07 | 1.54727e+07 | -1.5% |
>> |2 threads | 1.8044e+07 | 1.75601e+07 | -2.7% |
>> |3 threads | 1.35634e+07 | 1.46384e+07 | +7.9% |
>> |4 threads | 1.21257e+07 | 1.32046e+07 | +8.9% |
>> |28 threads | 8.09593e+06 | 1.02713e+07 | +26.9% |
>> |56 threads | 9.09907e+06 | 4.16203e+07 | +16.4% |
>> |112 threads | 7.09731e+06 | 8.62406e+06 | +21.5% |
>> +----------------+------------------------------------------------+
>> | | Critical section size: 100x |
>> +----------------+------------------------------------------------+
>> |1 thread | 2.87116e+06 | 2.89188e+06 | +0.7% |
>> |2 threads | 2.23409e+06 | 2.24216e+06 | +0.4% |
>> |3 threads | 2.29888e+06 | 2.29964e+06 | +0.0% |
>> |4 threads | 2.26898e+06 | 2.21394e+06 | -2.4% |
>> |28 threads | 1.03228e+06 | 1.0051e+06 | -2.6% |
>> |56 threads | 1.02953 +06 | 1.6344e+07 | -2.3% |
>> |112 threads | 1.01615e+06 | 1.00134e+06 | -1.5% |
>> +----------------+------------------------------------------------+
>> | | Critical section size: 1000x |
>> +----------------+------------------------------------------------+
>> |1 thread | 316392 | 315635 | -0.2% |
>> |2 threads | 302806 | 303469 | +0.2% |
>> |3 threads | 298506 | 294281 | -1.4% |
>> |4 threads | 292037 | 289945 | -0.7% |
>> |28 threads | 155188 | 155250 | +0.0% |
>> |56 threads | 190657 | 183106 | -4.0% |
>> |112 threads | 210818 | 220342 | +4.5% |
>> +----------------+-----------------+-----------------+------------+
>>
>> * nptl/pthread_mutex_lock.c: Optimize adaptive spin mutex
>> * nptl/pthread_mutex_timedlock.c: Likewise
>> * sysdeps/unix/sysv/linux/x86/pthread_mutex_lock.c: Enable read only
>> while spinning for x86 architecture
>> * sysdeps/unix/sysv/linux/x86/pthread_mutex_cond_lock.c: Likewise
>> * sysdeps/unix/sysv/linux/x86/pthread_mutex_timedlock.c: Likewise
>>
>> ChangLog:
>> V5->V6:
>> no change
>>
>> V4->V5:
>> a) Make the optimization work for pthread mutex_timedlock() in x86
>> architecture.
>> b) Move the READ_ONLY_SPIN macro definition from this patch to the
>> first patch which adds glibc.mutex.spin_count tunable entry
>>
>> V3->V4:
>> a) Make the optimization opt-in, and enable for x86 architecture as
>> default, as suggested by Florian Weimer.
>>
>> V2->V3:
>> a) Drop the idea of blocking spinners if fail to acquire a lock, since
>> this idea would not be an universal winner. E.g. several threads
>> contend for a lock which protects a small critical section, thus,
>> probably any thread can acquire the lock via spinning.
>> b) Fix the format issue AFAIC
>>
>> V1->V2: fix format issue
>>
>> Suggested-by: Andi Kleen <andi.kleen@intel.com>
>> Signed-off-by: Kemi Wang <kemi.wang@intel.com>
>> ---
>> nptl/pthread_mutex_lock.c | 15 +++++++++++++++
>> nptl/pthread_mutex_timedlock.c | 15 +++++++++++++++
>> sysdeps/unix/sysv/linux/x86/pthread_mutex_cond_lock.c | 1 +
>> sysdeps/unix/sysv/linux/x86/pthread_mutex_lock.c | 1 +
>> sysdeps/unix/sysv/linux/x86/pthread_mutex_timedlock.c | 1 +
>> 5 files changed, 33 insertions(+)
>>
>> diff --git a/nptl/pthread_mutex_lock.c b/nptl/pthread_mutex_lock.c
>> index 1519c14..9a7b5c2 100644
>> --- a/nptl/pthread_mutex_lock.c
>> +++ b/nptl/pthread_mutex_lock.c
>> @@ -133,7 +139,16 @@ __pthread_mutex_lock (pthread_mutex_t *mutex)
>> LLL_MUTEX_LOCK (mutex);
>> break;
>> }
>> +#ifdef READ_ONLY_SPIN
>> + do
>> + {
>> + atomic_spin_nop ();
>> + val = atomic_load_relaxed (&mutex->__data.__lock);
>> + }
>> + while (val != 0 && ++cnt < max_cnt);
>> +#else
>> atomic_spin_nop ();
>> +#endif
>> }
>> while (LLL_MUTEX_TRYLOCK (mutex) != 0);
>>
>> diff --git a/nptl/pthread_mutex_timedlock.c b/nptl/pthread_mutex_timedlock.c
>> index 66efd39..dcaeca8 100644
>> --- a/nptl/pthread_mutex_timedlock.c
>> +++ b/nptl/pthread_mutex_timedlock.c
>> @@ -126,7 +132,16 @@ __pthread_mutex_timedlock (pthread_mutex_t *mutex,
>> PTHREAD_MUTEX_PSHARED (mutex));
>> break;
>> }
>> +#ifdef READ_ONLY_SPIN
>> + do
>> + {
>> + atomic_spin_nop ();
>> + val = atomic_load_relaxed (&mutex->__data.__lock);
>> + }
>> + while (val != 0 && ++cnt < max_cnt);
>> +#else
>> atomic_spin_nop ();
>> +#endif
>> }
>> while (lll_trylock (mutex->__data.__lock) != 0);
>>
>
> Please define generic
>
> static inline void __always_inline
> atomic_spin_nop_with_limit (int cnt, int max_cnt, pthread_mutex_t *mutex)
> {
> atomic_spin_nop ();
> }
>
> and x86 version:
>
> static inline void __always_inline
> atomic_spin_nop_with_limit (int cnt, int max_cnt, pthread_mutex_t *mutex)
> {
> do
> {
> atomic_spin_nop ();
> val = atomic_load_relaxed (&mutex->__data.__lock);
> }
> while (val != 0 && ++cnt < max_cnt);
> }
>
> in a standalone patch.
>
Please take a look at
https://github.com/hjl-tools/glibc/tree/hjl/spin/master
I added:
static __always_inline void
atomic_spin_lock (pthread_spinlock_t *lock, int cnt, int max_cnt)
{
int val = 0;
do
{
atomic_spin_nop ();
val = atomic_load_relaxed (lock);
}
while (val != 0 && ++cnt < max_cnt);
}
--
H.J.