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.

Jan

Attachment: signature.asc
Description: OpenPGP digital signature

_______________________________________________
Xenomai-core mailing list
Xenomai-core@gna.org
https://mail.gna.org/listinfo/xenomai-core

Reply via email to