Jan Kiszka wrote:
> Gilles Chanteperdrix wrote:
>> Jan Kiszka wrote:
>>> Gilles Chanteperdrix wrote:
>>>> Jan Kiszka wrote:
>>> ...
>>>>> I think I'm getting closer to the issue. Our actual problem comes from
>>>>> the fact that the xnsynch_owner is easily out of sync with the real
>>>>> owner, it even sometimes points to a former owner:
>>>>>
>>>>> Thread A releases a mutex on which thread B pends. It wakes up B,
>>>>> causing it to become the new xnsynch owner, and clears the claimed bit
>>>>> as there are no further sleepers. B returns, and when it wants to
>>>>> release the mutex, it does this happily in user space because claimed is
>>>>> not set. Now the fast lock variable is 'unlocked', while xnsynch still
>>>>> reports B being the owner. This is no problem as the next time two
>>>>> threads fight over this lock the waiter will simply overwrite the
>>>>> xnsynch_owner before it falls asleep. But this "trick" doesn't work for
>>>>> waiters that have been robbed. They will spin inside xnsynch_sleep_on
>>>>> and stumble over this inconsistency.
>>>>>
>>>>> I have two approaches in mind now: First one is something like
>>>>> XNSYNCH_STEALNOINFORM, i.e. causing xnsynch_sleep_on to not set XNROBBED
>>>>> so that the robbed thread spins one level higher in the skin code -
>>>>> which would have to be extended a bit.
>>>> No, the stealing is the xnsynch job.
>>>>
>>>>> Option two is to clear xnsynch_owner once a new owner is about to return
>>>>> from kernel with the lock held while there are no more xnsynch_sleepers.
>>>>> That should work with even less changes and save us one syscall in the
>>>>> robbed case. Need to think about it more, though.
>>>> In fact the only time when the owner is required to be in sync is when
>>>> PIP occurs, and this is guaranteed to work, because when PIP is needed a
>>>> syscall is emitted anyway. To the extent that xnsynch does not even
>>>> track the owner on non PIP synch (which is why the posix skin originally
>>>> forcibly set the synch owner, and it was simply kept to get the fastsem
>>>> stuff working).
>>>>
>>>> Ok. And what about the idea of the xnsynch bit to tell him "hey, the
>>>> owner is tracked in the upper layer, go there to find it".
>>> I'm yet having difficulties to imagine how this should look like when
>>> it's implemented. Would it be simpler than my second idea?
>>>
>>> Anyway, here is a patch (on top of my handle-based lock series) for the
>>> approach that clears xnsynch_owner when there are no waiters. At least
>>> it causes no regression based on your test, but I haven't checked lock
>>> stealing yet. In theory, everything still appears to be fine to me. This
>>> approach basically restores the state we find when some thread just
>>> acquired the lock in user space.
>>>
>>> Jan
>>>
>>> ---
>>> ksrc/skins/posix/mutex.c | 10 +++++++---
>>> ksrc/skins/posix/mutex.h | 17 +++++++++++------
>>> 2 files changed, 18 insertions(+), 9 deletions(-)
>>>
>>> Index: b/ksrc/skins/posix/mutex.c
>>> ===================================================================
>>> --- a/ksrc/skins/posix/mutex.c
>>> +++ b/ksrc/skins/posix/mutex.c
>>> @@ -117,7 +117,6 @@ int pse51_mutex_init_internal(struct __s
>>> mutex->attr = *attr;
>>> mutex->owner = ownerp;
>>> mutex->owningq = kq;
>>> - mutex->sleepers = 0;
>>> xnarch_atomic_set(ownerp, XN_NO_HANDLE);
>>>
>>> xnlock_get_irqsave(&nklock, s);
>>> @@ -305,14 +304,12 @@ int pse51_mutex_timedlock_break(struct _
>>> /* Attempting to relock a normal mutex, deadlock. */
>>> xnlock_get_irqsave(&nklock, s);
>>> for (;;) {
>>> - ++mutex->sleepers;
>>> if (timed)
>>> xnsynch_sleep_on(&mutex->synchbase,
>>> abs_to, XN_REALTIME);
>>> else
>>> xnsynch_sleep_on(&mutex->synchbase,
>>> XN_INFINITE, XN_RELATIVE);
>>> - --mutex->sleepers;
>>>
>>> if (xnthread_test_info(cur, XNBREAK)) {
>>> err = -EINTR;
>>> @@ -329,6 +326,13 @@ int pse51_mutex_timedlock_break(struct _
>>> break;
>>> }
>>> }
>>> + if (!xnsynch_nsleepers(&mutex->synchbase)) {
>>> + xnarch_atomic_set
>>> + (mutex->owner,
>>> + clear_claimed
>>> + (xnarch_atomic_get(mutex->owner)));
>>> + xnsynch_set_owner(&mutex->synchbase, NULL);
>>> + }
>> I think an atomic_cmpxchg would be better here.
>>
>
> Not really. The claimed bit is set, so no one can change the owner
> without entering the kernel. But there we are also holding nklock. BTW,
> pse51_mutex_timedlock_internal does the same already.
I meant from an assembly generated code point of view. atomic_set +
atomic_get generates two atomic operations (well, except on x86),
whereas atomic_cmpxchg is just an atomic sequence. Except that we will
have to do the atomic_get also.
And we do not give a damn about this code. It is the implementation of
the deadlock that happens when trying to relock a non-recursive mutex.
--
Gilles.
_______________________________________________
Xenomai-core mailing list
Xenomai-core@gna.org
https://mail.gna.org/listinfo/xenomai-core
