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 > -- Philippe. _______________________________________________ Xenomai-core mailing list Xenomai-core@gna.org https://mail.gna.org/listinfo/xenomai-core
