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.

-- 
                                            Gilles.

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

Reply via email to