Philippe Gerum wrote:
> 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.

OK, point taken. But as this is a generic API issue, I would like to see
it fixed in the generic code, not in some to-be-duplicated branches in
all the skins.

My point is that our timeout API nicely defines the case XN_NONBLOCK
now, throughout all layers. We just need to pass the arguments as-is
down, the core will handle them. Let's do optimizations at the lowest
possible layer, i.e. in xnsynch_sleep_on (or whatever it may be called
better). A simple 'if' before suspend should be enough. And it will keep
the code small.

>> 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.

For now it will be a PIP show, but atomic_cmpxchg-based semaphores might
be an xnsynch extension in the future as well.


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

Xenomai-core mailing list

Reply via email to