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

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

[-- Attachment #1: Type: text/plain, Size: 2056 bytes --]

Gilles Chanteperdrix wrote:
> Jan Kiszka wrote:
>> Gilles Chanteperdrix wrote:
>>> Jan Kiszka wrote:
>>>> OTOH, we can safe some text size which is precious as well. So I'm
>>>> convinced to go your way (with a modification):
>>> My approach sucks: we get a silly atomic_cmpxchg if the mutex is already
>>> claimed, which is as least as much a common case as a currently
>>> unclaimed mutex. Need to think a bit. But I think a good solution is to
>>> re-read only if the mutex has been seen as already claimed.
>> That makes no difference as then we will go through the cmpxchg path anyway.
>>
>> There is _no_ way around re-reading under nklock, all we can avoid is
>> atomic cmpxchg in the case of >1 waiters. But that would come at the
>> price of more complexity for all waiter.
>>
>> However, let's find some solution. I bet things will look different
>> again when we start fiddling with a generic lock + the additional bit to
>> replace XNWAKEN.
> 
> What I meant is that if the claimed bit is already set, we can avoid the
> cmpxchg altogether, which was the intent of the original code. So I propose
> the following version:
> 
> 	if(test_claimed(owner))
> 		owner = xnarch_atomic_intptr_get(mutex->owner);

This version lacks a test for owner == 0 here, otherwise you re-create
my old bug that bite me today.

> 	while(!test_claimed(owner)) {
> 		old = xnarch_atomic_intptr_cmpxchg(mutex->owner,
> 						   owner, set_claimed(owner, 1));
> 		if (likely(old == owner))
> 			break;
> 		if (old == NULL) {
> 			/* Owner called fast mutex_unlock
> 			   (on another cpu) */
> 			xnlock_put_irqrestore(&nklock, s);
> 			goto retry_lock;
> 		}
> 		owner = old;
> 	}
> 
> The compiler rearranges things correctly (at least on ARM), and avoids the
> redundant test.
> 

My latest concern remains: Is all this additional complexity, are all
these conditional branches and the text size increase worth the effort?
How much cycles or micoseconds would be gain on the most suffering
architecture?

Jan


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 258 bytes --]