Re: [Xenomai-core] [PATCH] POSIX: Fix race when setting claimed bit

[Gilles Chanteperdrix <gilles.chanteperdrix@xenomai.org>]

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.