Jan Kiszka wrote:
> Philippe Gerum wrote:
>> Gilles Chanteperdrix wrote:
>>> Jan Kiszka wrote:
>>>> Gilles Chanteperdrix wrote:
>>>>> Jan Kiszka wrote:
>>>>>> Gilles Chanteperdrix wrote:
>>>>>>> Jan Kiszka wrote:
>>>>>>>> Gilles Chanteperdrix wrote:
>>>>>>>>> Philippe Gerum 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.
>>>>>>>>>>> Yes, I did not think about the stealing when writing my test, but I
>>>>>>>>>>> think it could be a good idea to add it to the test, especially if 
>>>>>>>>>>> you
>>>>>>>>>>> want to port the test to the native API.
>>>>>>>>>>> I let Philippe decide here. He is the one who did the stealing 
>>>>>>>>>>> stuff and
>>>>>>>>>>> probably knows better.
>>>>>>>>>> Currently, the xnsynch strongly couples PIP and ownership, which 
>>>>>>>>>> seems to impede
>>>>>>>>>> your various proposals. I would suggest to decouple that: the basic 
>>>>>>>>>> property of
>>>>>>>>>> some xnsynch that we may want to handle is exclusiveness, then 
>>>>>>>>>> dynamic priority
>>>>>>>>>> inheritance is another property, that could stack its own semantics 
>>>>>>>>>> on top of
>>>>>>>>>> exclusiveness.
>>>>>>>>>> XNSYNCH_EXCLUSIVE would cover all ownership-related actions, 
>>>>>>>>>> XNSYNCH_PIP would
>>>>>>>>>> simply add dynamic priority management. Non exclusive object would 
>>>>>>>>>> not require
>>>>>>>>>> any xnsynch_set_owner() handling.
>>>>>>>>>> Just to give a clear signal here: I will happily consider any change 
>>>>>>>>>> to the
>>>>>>>>>> xnsynch object that may ease the implementation of fast ownership 
>>>>>>>>>> handling (i.e.
>>>>>>>>>> userland-to-userland transfer). The only thing is that such code is 
>>>>>>>>>> very much
>>>>>>>>>> prone to regressions, so a testsuite must come with core changes in 
>>>>>>>>>> that area,
>>>>>>>>>> but I guess you know that already.
>>>>>>>>> Ok. I think unit_mutex.c is a good start. It only lacks testing
>>>>>>>>> XNROBBED.
>>>>>>>> My colleague sent me an extension. It's native-only so far, but it
>>>>>>>> already pointed out a bug in my try-acquire implementation that should
>>>>>>>> be present in posix as well (trylock must go through the slow path).
>>>>>>> I do not see why. If mutex lock can lock without a syscall, the same
>>>>>>> goes for trylock.
>>>>>> Lock stealing requires the slow path.
>>>>> Ah ok. I thought you mean that trylock had to go systematically through
>>>>> syscall.
>>>>> As for lock stealing, I already said that it was not tested in the
>>>>> current test.
>>>> In fact, that bug is also present the current native skin (SVN, 2.4.x
>>>> should suffer as well). Probably also current 2.4.x posix is affected,
>>>> at least from a first glance.
>>> Well, yes, now that you mention it, calling xnsynch_sleep_on does not
>>> seem obvious for the implementation of a trylock.
>> Don't even try calling xnsynch_sleep_on with a nonblocking-type timeout.
> Works nicely today (using (XN_NONBLOCK, XN_RELATIVE)) due to early
> timeout checks in xntimer_start. You just have to translate XNTIMEO into
> EWOULDBLOCK on return.

It's very fundamentally crappy. Please don't do that. xnsynch_sleep_on() must do
what it is supposed to mean.

> Jan


Xenomai-core mailing list

Reply via email to