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