Jan Kiszka wrote:
> Gilles Chanteperdrix wrote:
>> Jan Kiszka wrote:
>>> Patch 2 of my fast lock series actually also contained an attempt to fix
>>> a race I spotted in the code that atomically sets the claimed bit. I
>>> forgot about this fact and even, worse, I only replace the original race
>>> with another one.
>>>
>>> So here comes a new attempt to fix the issue that the lock owner and/or
>>> the claimed bit can change between trylock and the cmpxchg under nklock.
>>> Please have a look and cross-check the logic.
>>>
>>> The patch applies on top of vanilla SVN, so my series has to be rebased
>>> and the fix has to be ported to native as well - where we just found it
>>> in the field.
>> Ok. Got it.
>>
>>> Jan
>>>
>>> ---
>>> ksrc/skins/posix/mutex.h | 11 ++++++-----
>>> 1 file changed, 6 insertions(+), 5 deletions(-)
>>>
>>> Index: b/ksrc/skins/posix/mutex.h
>>> ===================================================================
>>> --- a/ksrc/skins/posix/mutex.h
>>> +++ b/ksrc/skins/posix/mutex.h
>>> @@ -134,17 +134,18 @@ static inline int pse51_mutex_timedlock_
>>> /* Set bit 0, so that mutex_unlock will know that the mutex is claimed.
>>> Hold the nklock, for mutual exclusion with slow mutex_unlock. */
>>> xnlock_get_irqsave(&nklock, s);
>>> + owner = xnarch_atomic_intptr_get(mutex->owner);
>> Bad, this makes two atomic ops. So, I would rather propose:
>
> Err, how many archs perform special dances for atomic_read?
powerpc, arm >= 6
>
>>> while(!test_claimed(owner)) {
>> - while(!test_claimed(owner)) {
>> + do {
>>> - old = xnarch_atomic_intptr_cmpxchg(mutex->owner,
>>> - owner, set_claimed(owner,
>>> 1));
>>> - if (likely(old == owner))
>>> - break;
>>> - if (old == NULL) {
>>> + if (owner == NULL) {
>>> /* Owner called fast mutex_unlock
>>> (on another cpu) */
>>> xnlock_put_irqrestore(&nklock, s);
>>> goto retry_lock;
>>> }
>>> + old = xnarch_atomic_intptr_cmpxchg(mutex->owner,
>>> + owner, set_claimed(owner,
>>> 1));
>>> + if (likely(old == owner))
>>> + break;
>>> owner = old;
>> - }
>> + } while (!test_claimed(owner))
>
> Your version slows down the contended case by performing redundant
> atomic cmpxchgs. My version puts minimal additional overhead in the
> !test_claimed case, but introduces no further atomic ops and avoids to
> much traffic on mutex->owner's cacheline when there are >1 waiters.
But your version slows down the common case where the owner will not
have changed... I do not care for making the uncommon case slower as
long as the common case is fast.
--
Gilles.
_______________________________________________
Xenomai-core mailing list
Xenomai-core@gna.org
https://mail.gna.org/listinfo/xenomai-core
