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. Jan -- Siemens AG, Corporate Technology, CT SE 2 Corporate Competence Center Embedded Linux _______________________________________________ Xenomai-core mailing list Xenomai-core@gna.org https://mail.gna.org/listinfo/xenomai-core
