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

[Jan Kiszka <jan.kiszka@domain.hid>]

Gilles Chanteperdrix wrote:
> 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

None of both. All atomic_read for arm use plain ((v)->counter), powerpc
simply uses assembly to avoid volatile atomic_t types. Do you happen to
confuse atomic_read and set here?

> 
>>>>  	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.

The tiny slowdown (a simple, typically cached read and test) is
negligible compared to the full slow path, even if there is only one
waiter. Atomic ops are heavier, sleeping is much heavier.

Jan

-- 
Siemens AG, Corporate Technology, CT SE 2
Corporate Competence Center Embedded Linux