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
