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.

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.


Siemens AG, Corporate Technology, CT SE 2
Corporate Competence Center Embedded Linux

Xenomai-core mailing list

Reply via email to