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

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

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

Gilles Chanteperdrix wrote:
> Jan Kiszka wrote:
>> 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.
> 
> Ok, so jumping in the middle of the loop is the best thing to do then.
> 
>>> 	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?
> 
> Since the else and the while are folded together and the loop becomes
> post-tested, and with the help of conditional instructions, I do not
> think there is much more conditional branch (we need to to jump over
> the while loop anyway). However avoiding the cmpxchg is a real gain,
> since it is implemented as irq save/irq restore on an old arm.
> 
> 	if(test_claimed(owner)) {
> 		old = xnarch_atomic_intptr_get(mutex->owner);
> 		goto test_old;
> 	}
> 	while(!test_claimed(owner)) {
> 		old = xnarch_atomic_intptr_cmpxchg(mutex->owner,
> 						   owner, set_claimed(owner, 1));
> 		if (likely(old == owner))
> 			break;
> 	test_old:
> 		if (old == NULL) {
> 			/* Owner called fast mutex_unlock
> 			   (on another cpu) */
> 			xnlock_put_irqrestore(&nklock, s);
> 			goto retry_lock;
> 		}
> 		owner = old;
> 	}

Should work now, but the beautifulness of the code suffers a bit...

What about making the expected compiler optimizations explicit?

	if (test_claimed(owner)) {
		old = xnarch_atomic_intptr_get(mutex->owner);
		goto test_no_owner;
	}
	do {
		old = xnarch_atomic_intptr_cmpxchg(mutex->owner, owner,
						   set_claimed(owner, 1));
		if (likely(old == owner))
			break;
	test_no_owner:
		if (old == NULL) {
			/* Owner called fast mutex_unlock
			   (on another cpu) */
			xnlock_put_irqrestore(&nklock, s);
			goto retry_lock;
		}
		owner = old;
	} while (!test_claimed(owner));

Jan


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