Jan Kiszka wrote:
> Philippe Gerum wrote:
>> 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.
> So we need xnsynch_try_to_steal + lots of new special-case code in the
> skins to re-implement what is already there? No, I can't imagine that
> this is what you want to see in the end.

What I don't want is to see code running a routine called "sleep_on" knowing
that it may not sleep. What I don't want either, is to go through
xnsynch_sleep_on -> xnpod_suspend_thread -> xntimer_start ->
xnsynch_forget_sleeper to get things right with !PIP objects, albeit the caller
knows from the beginning that it does not want to sleep. So much for the fast
path to trylocking.

> Then lets better rename xnsynch_sleep_on to something like
> xnsynch_acquire - will be logically needed anyway when we push fast
> mutex maintenance into it.

This would not fix issue #2. xnsynch_try_acquire() that would attempt stealing
is the way to go. It's even possible that some code could be shared between that
routine, and the resource stealing support currently provided by
xnsynch_sleep_on(). Only resource acquisition routines that enforce exclusive
ownership would be impacted, not each and every xnsynch_sleep_on caller.

> Jan


Xenomai-core mailing list

Reply via email to