Re: [Xenomai-core] [RFC][PATCH 2/3] Switch to handle-based fast mutex owners

2008-08-29 Thread Jan Kiszka
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).

 And well, cases of more than two waiters are not tested as
 well, but I would not think it really matters. It could be extended as
 well to allow switching at start time between the posix api and the
 native api, after all we do not care much about the overhead of calling
 function pointers in a a test application.

For now we have two files already, but maybe we can merge them later
based on your proposal.

 
 As for evolution of xnsynch, I am all for it, but it would break
 anything implemented before, so it will probably be on Jan's critical path.

I have some intermediate version here that breaks up the
retry-on-XNROBBED loop, doing it at skin level instead. This is
targeting the first customer delivery.

I also have two implementations of fast locking now, native and posix,
and they don't look that bad /wrt moving quite a few bits into xnsynch -
later.

Jan

PS: The test cases + my 

Re: [Xenomai-core] [RFC][PATCH 2/3] Switch to handle-based fast mutex owners

2008-08-29 Thread Gilles Chanteperdrix
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.

-- 
Gilles.

___
Xenomai-core mailing list
Xenomai-core@gna.org
https://mail.gna.org/listinfo/xenomai-core


Re: [Xenomai-core] [RFC][PATCH 2/3] Switch to handle-based fast mutex owners

2008-08-29 Thread Jan Kiszka
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.

Jan




signature.asc
Description: OpenPGP digital signature
___
Xenomai-core mailing list
Xenomai-core@gna.org
https://mail.gna.org/listinfo/xenomai-core


Re: [Xenomai-core] [RFC][PATCH 2/3] Switch to handle-based fast mutex owners

2008-08-29 Thread Gilles Chanteperdrix
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.

-- 
 Gilles.

___
Xenomai-core mailing list
Xenomai-core@gna.org
https://mail.gna.org/listinfo/xenomai-core


Re: [Xenomai-core] [RFC][PATCH 2/3] Switch to handle-based fast mutex owners

2008-08-29 Thread Jan Kiszka
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.

Jan

PS: Handle leak fixed, #4156, was SVN-only, I caused it.

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


Re: [Xenomai-core] [RFC][PATCH 2/3] Switch to handle-based fast mutex owners

2008-08-29 Thread Gilles Chanteperdrix
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.

And by the way, I think that with the mutex/xnsynch shared owner, we can
implement stealing without a syscall.

-- 
 

Re: [Xenomai-core] [RFC][PATCH 2/3] Switch to handle-based fast mutex owners

2008-08-29 Thread Jan Kiszka
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.

Once the dust settles here, I will have a look at the stable series as
well, providing a fix for the old mutexes.

 
 And by the way, I 

Re: [Xenomai-core] [RFC][PATCH 2/3] Switch to handle-based fast mutex owners

2008-08-29 Thread Philippe Gerum
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.

 And by the way, I think that with the mutex/xnsynch shared 

Re: [Xenomai-core] [RFC][PATCH 2/3] Switch to handle-based fast mutex owners

2008-08-29 Thread Jan Kiszka
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 

Re: [Xenomai-core] [RFC][PATCH 2/3] Switch to handle-based fast mutex owners

2008-08-29 Thread Gilles Chanteperdrix
Jan Kiszka wrote:
 Probably. But it will definitely need full fast locking support inside
 xnsynch, which is basically about defining and maintaining a generic
 user-shared lock word from within xnsynch services that has at least a
 'claimed' and a 'assignment pending' bit. Looks like we will not run out
 of work soon... ;)

The way xnsynch_sleep_on checks the owner field and retries for the
implementation of the lock stealing already furiously look like a mutex
compare and exchange...


-- 
 Gilles.

___
Xenomai-core mailing list
Xenomai-core@gna.org
https://mail.gna.org/listinfo/xenomai-core


Re: [Xenomai-core] [RFC][PATCH 2/3] Switch to handle-based fast mutex owners

2008-08-29 Thread Philippe Gerum
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 

Re: [Xenomai-core] [RFC][PATCH 2/3] Switch to handle-based fast mutex owners

2008-08-29 Thread Jan Kiszka
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 

Re: [Xenomai-core] [RFC][PATCH 2/3] Switch to handle-based fast mutex owners

2008-08-29 Thread Philippe Gerum
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 

Re: [Xenomai-core] [RFC][PATCH 2/3] Switch to handle-based fast mutex owners

2008-08-29 Thread Jan Kiszka
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 

Re: [Xenomai-core] [RFC][PATCH 2/3] Switch to handle-based fast mutex owners

2008-08-28 Thread Jan Kiszka
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.

 Jan

 ---
  ksrc/skins/posix/mutex.c |   10 +++---
  ksrc/skins/posix/mutex.h |   17 +++--
  2 files changed, 18 insertions(+), 9 deletions(-)

 Index: b/ksrc/skins/posix/mutex.c
 ===
 --- a/ksrc/skins/posix/mutex.c
 +++ b/ksrc/skins/posix/mutex.c
 @@ -117,7 +117,6 @@ int pse51_mutex_init_internal(struct __s
  mutex-attr = *attr;
  mutex-owner = ownerp;
  mutex-owningq = kq;
 -mutex-sleepers = 0;
  xnarch_atomic_set(ownerp, XN_NO_HANDLE);
  
  xnlock_get_irqsave(nklock, s);
 @@ -305,14 +304,12 @@ int pse51_mutex_timedlock_break(struct _
  /* Attempting to relock a normal mutex, deadlock. */
  xnlock_get_irqsave(nklock, s);
  for (;;) {
 -++mutex-sleepers;
  if (timed)
  xnsynch_sleep_on(mutex-synchbase,
   abs_to, XN_REALTIME);
  else
  xnsynch_sleep_on(mutex-synchbase,
   XN_INFINITE, XN_RELATIVE);
 ---mutex-sleepers;
  
  if (xnthread_test_info(cur, XNBREAK)) {
  err = -EINTR;
 @@ -329,6 +326,13 @@ int pse51_mutex_timedlock_break(struct _
  break;
  }
  }
 +if (!xnsynch_nsleepers(mutex-synchbase)) {
 +xnarch_atomic_set
 +(mutex-owner,
 + clear_claimed
 +(xnarch_atomic_get(mutex-owner)));
 +xnsynch_set_owner(mutex-synchbase, NULL);
 +}
 
 I think an atomic_cmpxchg would be better here.
 

Not really. The claimed bit is set, so no one can change the owner
without entering the kernel. But there we are also holding nklock. BTW,
pse51_mutex_timedlock_internal does the same already.

Jan



signature.asc
Description: OpenPGP digital signature
___
Xenomai-core mailing list
Xenomai-core@gna.org
https://mail.gna.org/listinfo/xenomai-core


Re: [Xenomai-core] [RFC][PATCH 2/3] Switch to handle-based fast mutex owners

2008-08-28 Thread Jan Kiszka
Gilles Chanteperdrix wrote:
 Jan Kiszka wrote:
 @@ -329,6 +326,13 @@ int pse51_mutex_timedlock_break(struct _
  break;
  }
  }
 +if (!xnsynch_nsleepers(mutex-synchbase)) {
 +xnarch_atomic_set
 +(mutex-owner,
 + clear_claimed
 +(xnarch_atomic_get(mutex-owner)));
 +xnsynch_set_owner(mutex-synchbase, NULL);
 +}
  xnlock_put_irqrestore(nklock, s);
 
 I do not like this at all. I mean, unless I am mistaken, we loose more
 than we gain, we are adding a couple of atomic, hence heavy, operations
 in a common case for handling a corner case. I still prefer emitting a
 system call in the corner case.

The hunk above is in the mutex' deadlock path - I wouldn't call this a
common case. Moreover, we don't any expensive cmpxchg here.

I haven't counted ops, but my strong feeling is that this patch actually
shortens the common code paths + it safely avoids one syscall in the
lock stealing case.

Jan



signature.asc
Description: OpenPGP digital signature
___
Xenomai-core mailing list
Xenomai-core@gna.org
https://mail.gna.org/listinfo/xenomai-core


Re: [Xenomai-core] [RFC][PATCH 2/3] Switch to handle-based fast mutex owners

2008-08-28 Thread Gilles Chanteperdrix
Jan Kiszka 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.

 Jan

 ---
  ksrc/skins/posix/mutex.c |   10 +++---
  ksrc/skins/posix/mutex.h |   17 +++--
  2 files changed, 18 insertions(+), 9 deletions(-)

 Index: b/ksrc/skins/posix/mutex.c
 ===
 --- a/ksrc/skins/posix/mutex.c
 +++ b/ksrc/skins/posix/mutex.c
 @@ -117,7 +117,6 @@ int pse51_mutex_init_internal(struct __s
 mutex-attr = *attr;
 mutex-owner = ownerp;
 mutex-owningq = kq;
 -   mutex-sleepers = 0;
 xnarch_atomic_set(ownerp, XN_NO_HANDLE);
  
 xnlock_get_irqsave(nklock, s);
 @@ -305,14 +304,12 @@ int pse51_mutex_timedlock_break(struct _
 /* Attempting to relock a normal mutex, deadlock. */
 xnlock_get_irqsave(nklock, s);
 for (;;) {
 -   ++mutex-sleepers;
 if (timed)
 xnsynch_sleep_on(mutex-synchbase,
  abs_to, XN_REALTIME);
 else
 xnsynch_sleep_on(mutex-synchbase,
  XN_INFINITE, XN_RELATIVE);
 -   --mutex-sleepers;
  
 if (xnthread_test_info(cur, XNBREAK)) {
 err = -EINTR;
 @@ -329,6 +326,13 @@ int pse51_mutex_timedlock_break(struct _
 break;
 }
 }
 +   if (!xnsynch_nsleepers(mutex-synchbase)) {
 +   xnarch_atomic_set
 +   (mutex-owner,
 +clear_claimed
 +   (xnarch_atomic_get(mutex-owner)));
 +   xnsynch_set_owner(mutex-synchbase, NULL);
 +   }
 I think an atomic_cmpxchg would be better here.

 
 Not really. The claimed bit is set, so no one can change the owner
 without entering the kernel. But there we are also holding nklock. BTW,
 pse51_mutex_timedlock_internal does the same already.

I meant from an assembly generated code point of view. atomic_set +
atomic_get generates two atomic operations (well, except on x86),
whereas atomic_cmpxchg is just an atomic sequence. Except that we will
have to do the atomic_get also.

And we do not give a damn about this code. It is the implementation of
the deadlock that happens when trying to relock a non-recursive mutex.

-- 

Re: [Xenomai-core] [RFC][PATCH 2/3] Switch to handle-based fast mutex owners

2008-08-28 Thread Gilles Chanteperdrix
Jan Kiszka wrote:
 Gilles Chanteperdrix wrote:
 Jan Kiszka wrote:
 @@ -329,6 +326,13 @@ int pse51_mutex_timedlock_break(struct _
 break;
 }
 }
 +   if (!xnsynch_nsleepers(mutex-synchbase)) {
 +   xnarch_atomic_set
 +   (mutex-owner,
 +clear_claimed
 +   (xnarch_atomic_get(mutex-owner)));
 +   xnsynch_set_owner(mutex-synchbase, NULL);
 +   }
 xnlock_put_irqrestore(nklock, s);
 I do not like this at all. I mean, unless I am mistaken, we loose more
 than we gain, we are adding a couple of atomic, hence heavy, operations
 in a common case for handling a corner case. I still prefer emitting a
 system call in the corner case.
 
 The hunk above is in the mutex' deadlock path - I wouldn't call this a
 common case. Moreover, we don't any expensive cmpxchg here.
 
 I haven't counted ops, but my strong feeling is that this patch actually
 shortens the common code paths + it safely avoids one syscall in the
 lock stealing case.

I have not yet understood how this happens.

-- 
 Gilles.

___
Xenomai-core mailing list
Xenomai-core@gna.org
https://mail.gna.org/listinfo/xenomai-core


Re: [Xenomai-core] [RFC][PATCH 2/3] Switch to handle-based fast mutex owners

2008-08-28 Thread Jan Kiszka
Gilles Chanteperdrix wrote:
 Jan Kiszka wrote:
 Gilles Chanteperdrix wrote:
 Jan Kiszka wrote:
 @@ -329,6 +326,13 @@ int pse51_mutex_timedlock_break(struct _
break;
}
}
 +  if (!xnsynch_nsleepers(mutex-synchbase)) {
 +  xnarch_atomic_set
 +  (mutex-owner,
 +   clear_claimed
 +  (xnarch_atomic_get(mutex-owner)));
 +  xnsynch_set_owner(mutex-synchbase, NULL);
 +  }
xnlock_put_irqrestore(nklock, s);
 I do not like this at all. I mean, unless I am mistaken, we loose more
 than we gain, we are adding a couple of atomic, hence heavy, operations
 in a common case for handling a corner case. I still prefer emitting a
 system call in the corner case.
 The hunk above is in the mutex' deadlock path - I wouldn't call this a
 common case. Moreover, we don't any expensive cmpxchg here.

 I haven't counted ops, but my strong feeling is that this patch actually
 shortens the common code paths + it safely avoids one syscall in the
 lock stealing case.
 
 I have not yet understood how this happens.

Argh, it won't work for all cases: When the robbed owner retries the
acquisition while the current owner still holds the lock, that clearing
of xnsynch_owner has the fatal effect of letting the former thread
proceed as well. So we cannot get around to extend xnsynch, teaching it
about the two-level ownership management - or actually folding fast lock
support into xnsynch.

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


Re: [Xenomai-core] [RFC][PATCH 2/3] Switch to handle-based fast mutex owners

2008-08-28 Thread Philippe Gerum
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.

-- 
Philippe.

___
Xenomai-core mailing list
Xenomai-core@gna.org
https://mail.gna.org/listinfo/xenomai-core


Re: [Xenomai-core] [RFC][PATCH 2/3] Switch to handle-based fast mutex owners

2008-08-28 Thread Jan Kiszka
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.

... but would also not be able to provide PIP. Unfortunately, we want
PIP here, but we do separate owner tracking, so this differentiation
alone won't help.

When it comes to find a minimal invasive approach, I'm more in favor of
breaking up that lock stealing loop in xnsynch_sleep_on, letting the
caller handle XNROBBED instead. That feature could easily be made an
add-on to the current internal handling, without risking regressions for
other users.

 
 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.

Yeah, it's clear. I'm currently considering the options and the efforts.
There are some smaller differences the way POSIX does fast locking right
now and native may do it, so it is also not yet clear to me how to
abstract a generic service. Nevertheless, it would help a lot avoiding
to invent too many skin-specific bugs when porting the algorithms back
and forth...

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


Re: [Xenomai-core] [RFC][PATCH 2/3] Switch to handle-based fast mutex owners

2008-08-28 Thread Philippe Gerum
Jan Kiszka 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.
 
 ... but would also not be able to provide PIP. Unfortunately, we want
 PIP here,

I don't follow you here. You just can't provide PIP if you don't have ownership.

 but we do separate owner tracking, so this differentiation
 alone won't help.
 
 When it comes to find a minimal invasive approach, I'm more in favor of
 breaking up that lock stealing loop in xnsynch_sleep_on, letting the
 caller handle XNROBBED instead. That feature could easily be made an
 add-on to the current internal handling, without risking regressions for
 other users.
 

Mmfff...

 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.
 
 Yeah, it's clear. I'm currently considering the options and the efforts.
 There are some smaller differences the way POSIX does fast locking right
 now and native may do it, so it is also not yet clear to me how to
 abstract a generic service. Nevertheless, it would help a lot avoiding
 to invent too many skin-specific bugs when porting the algorithms back
 and forth...
 

This is why I don't think moving part of the core logic to the skins would be a
good idea.

 Jan
 


-- 
Philippe.

___
Xenomai-core mailing list
Xenomai-core@gna.org
https://mail.gna.org/listinfo/xenomai-core


Re: [Xenomai-core] [RFC][PATCH 2/3] Switch to handle-based fast mutex owners

2008-08-28 Thread Gilles Chanteperdrix
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. And well, cases of more than two waiters are not tested as
well, but I would not think it really matters. It could be extended as
well to allow switching at start time between the posix api and the
native api, after all we do not care much about the overhead of calling
function pointers in a a test application.

As for evolution of xnsynch, I am all for it, but it would break
anything implemented before, so it will probably be on Jan's critical path.

-- 
 Gilles.

___
Xenomai-core mailing list
Xenomai-core@gna.org
https://mail.gna.org/listinfo/xenomai-core


Re: [Xenomai-core] [RFC][PATCH 2/3] Switch to handle-based fast mutex owners

2008-08-27 Thread Jan Kiszka
Gilles Chanteperdrix wrote:
 Jan Kiszka wrote:
 To improve robustness of the fast mutex implementation in POSIX (and
 later on in native), it is better to track the mutex owner by handle
 instead of kernel object pointer. Therefore, this patch changes
 __xn_sys_current (xeno_set_current) so that it returns
 xnthread_handle(current_thread). It furthermore converts the POSIX mutex
 implementation to pick up and store the lock owner as handle in the
 kernel/user-shared mutex. Finally it ensures that at least POSIX threads
 always have an (anonymous) handle assigned.

 As the value stored in the mutex variable is now an integer, we can
 switch over to xnarch_atomic_t, removing all atomic_intptr users.
 
 Ok. I do not know if this should be part of this patch, or in another
 one, but we should call xeno_set_current in the trampolines of all
 skins, so that they can use the native and posix mutexes.
 
 This is another thing that I have left in a state of flux...

Find it below (PATCH 4/3).

BTW, should we better invoke pthread_exit in xeno_set_current in case of 
a failure?

Jan

---
 src/skins/native/task.c |3 +++
 src/skins/psos+/task.c  |3 +++
 src/skins/uitron/task.c |3 +++
 src/skins/vrtx/task.c   |3 +++
 src/skins/vxworks/taskLib.c |3 +++
 5 files changed, 15 insertions(+)

Index: b/src/skins/native/task.c
===
--- a/src/skins/native/task.c
+++ b/src/skins/native/task.c
@@ -26,6 +26,7 @@
 #include limits.h
 #include native/syscall.h
 #include native/task.h
+#include asm-generic/bits/current.h
 #include wrappers.h
 
 extern pthread_key_t __native_tskey;
@@ -88,6 +89,8 @@ static void *rt_task_trampoline(void *co
if (err)
goto fail;
 
+   xeno_set_current();
+
/* Wait on the barrier for the task to be started. The barrier
   could be released in order to process Linux signals while the
   Xenomai shadow is still dormant; in such a case, resume wait. */
Index: b/src/skins/psos+/task.c
===
--- a/src/skins/psos+/task.c
+++ b/src/skins/psos+/task.c
@@ -26,6 +26,7 @@
 #include memory.h
 #include string.h
 #include psos+/psos.h
+#include asm-generic/bits/current.h
 
 extern int __psos_muxid;
 
@@ -89,6 +90,8 @@ static void *psos_task_trampoline(void *
if (err)
goto fail;
 
+   xeno_set_current();
+
/* Wait on the barrier for the task to be started. The barrier
   could be released in order to process Linux signals while the
   Xenomai shadow is still dormant; in such a case, resume wait. */
Index: b/src/skins/uitron/task.c
===
--- a/src/skins/uitron/task.c
+++ b/src/skins/uitron/task.c
@@ -25,6 +25,7 @@
 #include limits.h
 #include asm/xenomai/system.h
 #include uitron/uitron.h
+#include asm-generic/bits/current.h
 
 extern int __uitron_muxid;
 
@@ -89,6 +90,8 @@ static void *uitron_task_trampoline(void
if (err)
goto fail;
 
+   xeno_set_current();
+
/* iargs-pk_ctsk might not be valid anymore, after our parent
   was released from the completion sync, so do not
   dereference this pointer. */
Index: b/src/skins/vrtx/task.c
===
--- a/src/skins/vrtx/task.c
+++ b/src/skins/vrtx/task.c
@@ -27,6 +27,7 @@
 #include errno.h
 #include limits.h
 #include vrtx/vrtx.h
+#include asm-generic/bits/current.h
 
 extern pthread_key_t __vrtx_tskey;
 
@@ -106,6 +107,8 @@ static void *vrtx_task_trampoline(void *
if (err)
goto fail;
 
+   xeno_set_current();
+
/* Wait on the barrier for the task to be started. The barrier
   could be released in order to process Linux signals while the
   Xenomai shadow is still dormant; in such a case, resume wait. */
Index: b/src/skins/vxworks/taskLib.c
===
--- a/src/skins/vxworks/taskLib.c
+++ b/src/skins/vxworks/taskLib.c
@@ -27,6 +27,7 @@
 #include errno.h
 #include limits.h
 #include vxworks/vxworks.h
+#include asm-generic/bits/current.h
 #include wrappers.h
 
 extern pthread_key_t __vxworks_tskey;
@@ -117,6 +118,8 @@ static void *wind_task_trampoline(void *
if (err)
goto fail;
 
+   xeno_set_current();
+
/* Wait on the barrier for the task to be started. The barrier
   could be released in order to process Linux signals while the
   Xenomai shadow is still dormant; in such a case, resume wait. */

___
Xenomai-core mailing list
Xenomai-core@gna.org
https://mail.gna.org/listinfo/xenomai-core


Re: [Xenomai-core] [RFC][PATCH 2/3] Switch to handle-based fast mutex owners

2008-08-27 Thread Gilles Chanteperdrix
Jan Kiszka wrote:
 Gilles Chanteperdrix wrote:
 Jan Kiszka wrote:
 To improve robustness of the fast mutex implementation in POSIX (and
 later on in native), it is better to track the mutex owner by handle
 instead of kernel object pointer. Therefore, this patch changes
 __xn_sys_current (xeno_set_current) so that it returns
 xnthread_handle(current_thread). It furthermore converts the POSIX mutex
 implementation to pick up and store the lock owner as handle in the
 kernel/user-shared mutex. Finally it ensures that at least POSIX threads
 always have an (anonymous) handle assigned.

 As the value stored in the mutex variable is now an integer, we can
 switch over to xnarch_atomic_t, removing all atomic_intptr users.
 Ok. I do not know if this should be part of this patch, or in another
 one, but we should call xeno_set_current in the trampolines of all
 skins, so that they can use the native and posix mutexes.

 This is another thing that I have left in a state of flux...
 
 Find it below (PATCH 4/3).
 
 BTW, should we better invoke pthread_exit in xeno_set_current in case of 
 a failure?

I prefer my application to die. This way I would know that something
went wrong. And by the way, how could this syscall fail ? I mean if
xenomai or the posix skin are not loaded, we would have known it earlier.

-- 
 Gilles.

___
Xenomai-core mailing list
Xenomai-core@gna.org
https://mail.gna.org/listinfo/xenomai-core


Re: [Xenomai-core] [RFC][PATCH 2/3] Switch to handle-based fast mutex owners

2008-08-27 Thread Gilles Chanteperdrix
Jan Kiszka wrote:

 -#define test_claimed(owner) ((long) (owner)  1)
 -#define clear_claimed(owner) ((xnthread_t *) ((long) (owner)  ~1))
 -#define set_claimed(owner, bit) \
 -((xnthread_t *) ((long) clear_claimed(owner) | !!(bit)))
 +#define __CLAIMED_BITXN_HANDLE_SPARE3
 +
 +#define test_claimed(owner)  xnhandle_test_spare(owner, __CLAIMED_BIT)
 +#define clear_claimed(owner) xnhandle_mask_spare(owner)
 +#define set_claimed(owner, bit) ({ \
 + xnhandle_t __tmp = xnhandle_mask_spare(owner); \
 + if (bit) \
 + xnhandle_set_spare(__tmp, __CLAIMED_BIT); \
 + __tmp; \
 +})

I liked doing this with no conditional.

 + xnarch_atomic_set(mutex-owner,
 +   set_claimed(xnthread_handle(owner),
 +   xnsynch_nsleepers(mutex-synchbase)));

Ok. I think you have spotted a bug here. This should be mutex-sleepers
instead of xnsynch_nsleepers.

 + /* Consistency check for owner handle - is the object a thread? */
 + if (unlikely(xnthread_handle(owner) != clear_claimed(ownerh))) {
 + err = -EINVAL;
 + goto error;
   }

What is this ?


 Index: b/ksrc/skins/posix/thread.c
 ===
 --- a/ksrc/skins/posix/thread.c
 +++ b/ksrc/skins/posix/thread.c
 @@ -28,6 +28,7 @@
   * 
   [EMAIL PROTECTED]/
  
 +#include nucleus/registry.h
  #include posix/thread.h
  #include posix/cancel.h
  #include posix/timer.h
 @@ -229,6 +230,13 @@ int pthread_create(pthread_t *tid,
   appendq(thread-container, thread-link);
   xnlock_put_irqrestore(nklock, s);
  
 +#ifdef CONFIG_XENO_OPT_REGISTRY
 + /* We need an anonymous registry entry to obtain a handle for fast
 +mutex locking. */
 + xnregistry_enter(, thread-threadbase,
 +  xnthread_handle(thread-threadbase), NULL);
 +#endif /* CONFIG_XENO_OPT_REGISTRY */
 +

Since we need this for all threads (I want a thread from any skin to be
able to use the posix skin mutexes), why doing it in the skin ?

-- 
 Gilles.

___
Xenomai-core mailing list
Xenomai-core@gna.org
https://mail.gna.org/listinfo/xenomai-core


Re: [Xenomai-core] [RFC][PATCH 2/3] Switch to handle-based fast mutex owners

2008-08-27 Thread Gilles Chanteperdrix
Jan Kiszka wrote:
 To improve robustness of the fast mutex implementation in POSIX (and
 later on in native), it is better to track the mutex owner by handle
 instead of kernel object pointer. Therefore, this patch changes
 __xn_sys_current (xeno_set_current) so that it returns
 xnthread_handle(current_thread). It furthermore converts the POSIX mutex
 implementation to pick up and store the lock owner as handle in the
 kernel/user-shared mutex. Finally it ensures that at least POSIX threads
 always have an (anonymous) handle assigned.
 
 As the value stored in the mutex variable is now an integer, we can
 switch over to xnarch_atomic_t, removing all atomic_intptr users.

Ok. I do not know if this should be part of this patch, or in another
one, but we should call xeno_set_current in the trampolines of all
skins, so that they can use the native and posix mutexes.

This is another thing that I have left in a state of flux...

-- 
 Gilles.

___
Xenomai-core mailing list
Xenomai-core@gna.org
https://mail.gna.org/listinfo/xenomai-core


Re: [Xenomai-core] [RFC][PATCH 2/3] Switch to handle-based fast mutex owners

2008-08-27 Thread Jan Kiszka
Gilles Chanteperdrix wrote:
 Jan Kiszka wrote:
 Gilles Chanteperdrix wrote:
 Jan Kiszka wrote:
 Gilles Chanteperdrix wrote:
 Jan Kiszka wrote:
 Gilles Chanteperdrix wrote:
 Jan Kiszka wrote:

 -#define test_claimed(owner) ((long) (owner)  1)
 -#define clear_claimed(owner) ((xnthread_t *) ((long) (owner)  ~1))
 -#define set_claimed(owner, bit) \
 -((xnthread_t *) ((long) clear_claimed(owner) | !!(bit)))
 +#define __CLAIMED_BIT XN_HANDLE_SPARE3
 +
 +#define test_claimed(owner)   xnhandle_test_spare(owner, 
 __CLAIMED_BIT)
 +#define clear_claimed(owner)  xnhandle_mask_spare(owner)
 +#define set_claimed(owner, bit) ({ \
 +  xnhandle_t __tmp = xnhandle_mask_spare(owner); \
 +  if (bit) \
 +  xnhandle_set_spare(__tmp, __CLAIMED_BIT); \
 +  __tmp; \
 +})
 I liked doing this with no conditional.
 You mean let the caller pass 'bit' as 0 or __CLAIMED_BIT (the latter
 would require some renaming then, I guess)?

 +  xnarch_atomic_set(mutex-owner,
 +set_claimed(xnthread_handle(owner),
 +
 xnsynch_nsleepers(mutex-synchbase)));
 Ok. I think you have spotted a bug here. This should be mutex-sleepers
 instead of xnsynch_nsleepers.
 OK. Will you fix? I will rebase then.

 +  /* Consistency check for owner handle - is the object a thread? 
 */
 +  if (unlikely(xnthread_handle(owner) != clear_claimed(ownerh))) {
 +  err = -EINVAL;
 +  goto error;
}
 What is this ?
 This ensures that we are not dereferences some arbitrary registry object
 due to a borken handle in the mutex variable: The the object registered
 on this handle is an xnthread, we will find the very same handle value
 at the well-known place in the object. Kind of magic for xnthread
 objects (instead of adding a magic field to them).
 Yes, but the point is: how can the handle be borken ? It looks like a
 useless check to me.
 This is user memory. Everything can be borken. And I don't want to
 create yet another source for user-triggered kernel bugs (including
 silent ones) like with our heap management structures that are
 corruptible by userland. That's headache guarantee.
 No, we assume that the user application will only clobber our heap if it
 is broken too. Add this with a XENO_DEBUG option if you want (not
 XENO_DEBUG(POSIX)).
 No problem for me. This check is almost 100% identical (specifically
 overhead-wise) to the usual object magic checks after querying the
 registry, and as those are unconditionally performed, I saw no need to
 handle this case differently.
 
 But the registry lookup is already a validation in itself. I mean, if an
 application uses 5 as a file descriptor, there are only two cases:
 - the file descriptor 5 exists in the application and the kernel has no
 other choice than to believe that the application is using this file
 descriptor, and nothing will go wrong because this is a valid descriptor;
 - the file descriptor does not exist, and we will return EBADF, and
 nothing wrong will happen either...

File descriptors are all identically structured objects, so at worst you
ruin some other app's day. But the registry contains arbitrary objects
with different internal layout. If you start assuming object_a * is
object_b * and use the pointer etc. included in a as if they have the
meaning of b, you quickly ruin the kernel's day as well. Therefore,
native, e.g., does magic checks after fetching from the registry. As I
said, this test here works differently, but it has the same effect and
impact.

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


Re: [Xenomai-core] [RFC][PATCH 2/3] Switch to handle-based fast mutex owners

2008-08-27 Thread Gilles Chanteperdrix
Jan Kiszka wrote:
 Gilles Chanteperdrix wrote:
 Jan Kiszka wrote:
 Gilles Chanteperdrix wrote:
 Jan Kiszka wrote:
 To improve robustness of the fast mutex implementation in POSIX (and
 later on in native), it is better to track the mutex owner by handle
 instead of kernel object pointer. Therefore, this patch changes
 __xn_sys_current (xeno_set_current) so that it returns
 xnthread_handle(current_thread). It furthermore converts the POSIX mutex
 implementation to pick up and store the lock owner as handle in the
 kernel/user-shared mutex. Finally it ensures that at least POSIX threads
 always have an (anonymous) handle assigned.

 As the value stored in the mutex variable is now an integer, we can
 switch over to xnarch_atomic_t, removing all atomic_intptr users.
 Ok. I do not know if this should be part of this patch, or in another
 one, but we should call xeno_set_current in the trampolines of all
 skins, so that they can use the native and posix mutexes.

 This is another thing that I have left in a state of flux...
 Find it below (PATCH 4/3).

 BTW, should we better invoke pthread_exit in xeno_set_current in case of 
 a failure?
 I prefer my application to die. This way I would know that something
 went wrong. And by the way, how could this syscall fail ? I mean if
 xenomai or the posix skin are not loaded, we would have known it earlier.
 
 So far this syscall can trigger a thread handle registration if the
 current thread has no handle yet. But that could go away if all skins
 ensure that there is at least an anonymous handle for their threads
 after creation.

Then it looks redundant with the code that does associate a handle with
a new thread in the posix skin.

-- 
 Gilles.

___
Xenomai-core mailing list
Xenomai-core@gna.org
https://mail.gna.org/listinfo/xenomai-core


Re: [Xenomai-core] [RFC][PATCH 2/3] Switch to handle-based fast mutex owners

2008-08-27 Thread Gilles Chanteperdrix
Jan Kiszka wrote:
 Gilles Chanteperdrix wrote:
 Jan Kiszka wrote:
 Gilles Chanteperdrix wrote:
 Jan Kiszka wrote:
 Gilles Chanteperdrix wrote:
 Jan Kiszka wrote:

 -#define test_claimed(owner) ((long) (owner)  1)
 -#define clear_claimed(owner) ((xnthread_t *) ((long) (owner)  ~1))
 -#define set_claimed(owner, bit) \
 -((xnthread_t *) ((long) clear_claimed(owner) | !!(bit)))
 +#define __CLAIMED_BIT  XN_HANDLE_SPARE3
 +
 +#define test_claimed(owner)xnhandle_test_spare(owner, 
 __CLAIMED_BIT)
 +#define clear_claimed(owner)   xnhandle_mask_spare(owner)
 +#define set_claimed(owner, bit) ({ \
 +   xnhandle_t __tmp = xnhandle_mask_spare(owner); \
 +   if (bit) \
 +   xnhandle_set_spare(__tmp, __CLAIMED_BIT); \
 +   __tmp; \
 +})
 I liked doing this with no conditional.
 You mean let the caller pass 'bit' as 0 or __CLAIMED_BIT (the latter
 would require some renaming then, I guess)?

 +   xnarch_atomic_set(mutex-owner,
 + set_claimed(xnthread_handle(owner),
 + 
 xnsynch_nsleepers(mutex-synchbase)));
 Ok. I think you have spotted a bug here. This should be mutex-sleepers
 instead of xnsynch_nsleepers.
 OK. Will you fix? I will rebase then.

 +   /* Consistency check for owner handle - is the object a thread? 
 */
 +   if (unlikely(xnthread_handle(owner) != clear_claimed(ownerh))) {
 +   err = -EINVAL;
 +   goto error;
 }
 What is this ?
 This ensures that we are not dereferences some arbitrary registry object
 due to a borken handle in the mutex variable: The the object registered
 on this handle is an xnthread, we will find the very same handle value
 at the well-known place in the object. Kind of magic for xnthread
 objects (instead of adding a magic field to them).
 Yes, but the point is: how can the handle be borken ? It looks like a
 useless check to me.
 This is user memory. Everything can be borken. And I don't want to
 create yet another source for user-triggered kernel bugs (including
 silent ones) like with our heap management structures that are
 corruptible by userland. That's headache guarantee.
 No, we assume that the user application will only clobber our heap if it
 is broken too. Add this with a XENO_DEBUG option if you want (not
 XENO_DEBUG(POSIX)).
 
 No problem for me. This check is almost 100% identical (specifically
 overhead-wise) to the usual object magic checks after querying the
 registry, and as those are unconditionally performed, I saw no need to
 handle this case differently.

But the registry lookup is already a validation in itself. I mean, if an
application uses 5 as a file descriptor, there are only two cases:
- the file descriptor 5 exists in the application and the kernel has no
other choice than to believe that the application is using this file
descriptor, and nothing will go wrong because this is a valid descriptor;
- the file descriptor does not exist, and we will return EBADF, and
nothing wrong will happen either...

-- 
 Gilles.

___
Xenomai-core mailing list
Xenomai-core@gna.org
https://mail.gna.org/listinfo/xenomai-core


Re: [Xenomai-core] [RFC][PATCH 2/3] Switch to handle-based fast mutex owners

2008-08-27 Thread Gilles Chanteperdrix
Jan Kiszka wrote:
 Gilles Chanteperdrix wrote:
 Jan Kiszka wrote:
 Gilles Chanteperdrix wrote:
 Jan Kiszka wrote:
 Gilles Chanteperdrix wrote:
 Jan Kiszka wrote:
 Gilles Chanteperdrix wrote:
 Jan Kiszka wrote:

 -#define test_claimed(owner) ((long) (owner)  1)
 -#define clear_claimed(owner) ((xnthread_t *) ((long) (owner)  ~1))
 -#define set_claimed(owner, bit) \
 -((xnthread_t *) ((long) clear_claimed(owner) | !!(bit)))
 +#define __CLAIMED_BITXN_HANDLE_SPARE3
 +
 +#define test_claimed(owner)  xnhandle_test_spare(owner, 
 __CLAIMED_BIT)
 +#define clear_claimed(owner) xnhandle_mask_spare(owner)
 +#define set_claimed(owner, bit) ({ \
 + xnhandle_t __tmp = xnhandle_mask_spare(owner); \
 + if (bit) \
 + xnhandle_set_spare(__tmp, __CLAIMED_BIT); \
 + __tmp; \
 +})
 I liked doing this with no conditional.
 You mean let the caller pass 'bit' as 0 or __CLAIMED_BIT (the latter
 would require some renaming then, I guess)?

 + xnarch_atomic_set(mutex-owner,
 +   set_claimed(xnthread_handle(owner),
 +   
 xnsynch_nsleepers(mutex-synchbase)));
 Ok. I think you have spotted a bug here. This should be mutex-sleepers
 instead of xnsynch_nsleepers.
 OK. Will you fix? I will rebase then.

 + /* Consistency check for owner handle - is the object a thread? 
 */
 + if (unlikely(xnthread_handle(owner) != clear_claimed(ownerh))) {
 + err = -EINVAL;
 + goto error;
   }
 What is this ?
 This ensures that we are not dereferences some arbitrary registry object
 due to a borken handle in the mutex variable: The the object registered
 on this handle is an xnthread, we will find the very same handle value
 at the well-known place in the object. Kind of magic for xnthread
 objects (instead of adding a magic field to them).
 Yes, but the point is: how can the handle be borken ? It looks like a
 useless check to me.
 This is user memory. Everything can be borken. And I don't want to
 create yet another source for user-triggered kernel bugs (including
 silent ones) like with our heap management structures that are
 corruptible by userland. That's headache guarantee.
 No, we assume that the user application will only clobber our heap if it
 is broken too. Add this with a XENO_DEBUG option if you want (not
 XENO_DEBUG(POSIX)).
 No problem for me. This check is almost 100% identical (specifically
 overhead-wise) to the usual object magic checks after querying the
 registry, and as those are unconditionally performed, I saw no need to
 handle this case differently.
 But the registry lookup is already a validation in itself. I mean, if an
 application uses 5 as a file descriptor, there are only two cases:
 - the file descriptor 5 exists in the application and the kernel has no
 other choice than to believe that the application is using this file
 descriptor, and nothing will go wrong because this is a valid descriptor;
 - the file descriptor does not exist, and we will return EBADF, and
 nothing wrong will happen either...
 
 File descriptors are all identically structured objects, so at worst you
 ruin some other app's day. But the registry contains arbitrary objects
 with different internal layout. If you start assuming object_a * is
 object_b * and use the pointer etc. included in a as if they have the
 meaning of b, you quickly ruin the kernel's day as well. Therefore,
 native, e.g., does magic checks after fetching from the registry. As I
 said, this test here works differently, but it has the same effect and
 impact.

Ok. Got it. Leave the check then.

-- 
 Gilles.

___
Xenomai-core mailing list
Xenomai-core@gna.org
https://mail.gna.org/listinfo/xenomai-core


Re: [Xenomai-core] [RFC][PATCH 2/3] Switch to handle-based fast mutex owners

2008-08-27 Thread Gilles Chanteperdrix
Jan Kiszka wrote:
 Gilles Chanteperdrix wrote:
 Jan Kiszka wrote:

 -#define test_claimed(owner) ((long) (owner)  1)
 -#define clear_claimed(owner) ((xnthread_t *) ((long) (owner)  ~1))
 -#define set_claimed(owner, bit) \
 -((xnthread_t *) ((long) clear_claimed(owner) | !!(bit)))
 +#define __CLAIMED_BIT  XN_HANDLE_SPARE3
 +
 +#define test_claimed(owner)xnhandle_test_spare(owner, 
 __CLAIMED_BIT)
 +#define clear_claimed(owner)   xnhandle_mask_spare(owner)
 +#define set_claimed(owner, bit) ({ \
 +   xnhandle_t __tmp = xnhandle_mask_spare(owner); \
 +   if (bit) \
 +   xnhandle_set_spare(__tmp, __CLAIMED_BIT); \
 +   __tmp; \
 +})
 I liked doing this with no conditional.
 
 You mean let the caller pass 'bit' as 0 or __CLAIMED_BIT (the latter
 would require some renaming then, I guess)?
 
 +   xnarch_atomic_set(mutex-owner,
 + set_claimed(xnthread_handle(owner),
 + xnsynch_nsleepers(mutex-synchbase)));
 Ok. I think you have spotted a bug here. This should be mutex-sleepers
 instead of xnsynch_nsleepers.
 
 OK. Will you fix? I will rebase then.
 
 +   /* Consistency check for owner handle - is the object a thread? */
 +   if (unlikely(xnthread_handle(owner) != clear_claimed(ownerh))) {
 +   err = -EINVAL;
 +   goto error;
 }
 What is this ?
 
 This ensures that we are not dereferences some arbitrary registry object
 due to a borken handle in the mutex variable: The the object registered
 on this handle is an xnthread, we will find the very same handle value
 at the well-known place in the object. Kind of magic for xnthread
 objects (instead of adding a magic field to them).
 

 Index: b/ksrc/skins/posix/thread.c
 ===
 --- a/ksrc/skins/posix/thread.c
 +++ b/ksrc/skins/posix/thread.c
 @@ -28,6 +28,7 @@
   * 
   [EMAIL PROTECTED]/
  
 +#include nucleus/registry.h
  #include posix/thread.h
  #include posix/cancel.h
  #include posix/timer.h
 @@ -229,6 +230,13 @@ int pthread_create(pthread_t *tid,
 appendq(thread-container, thread-link);
 xnlock_put_irqrestore(nklock, s);
  
 +#ifdef CONFIG_XENO_OPT_REGISTRY
 +   /* We need an anonymous registry entry to obtain a handle for fast
 +  mutex locking. */
 +   xnregistry_enter(, thread-threadbase,
 +xnthread_handle(thread-threadbase), NULL);
 +#endif /* CONFIG_XENO_OPT_REGISTRY */
 +
 Since we need this for all threads (I want a thread from any skin to be
 able to use the posix skin mutexes), why doing it in the skin ?
 
 Some skins register their threads unconditionally, some based on names,
 some not at all. The not at all case has to be fixed, but we need to
 respect the other two as well.

Ok. But we need to add the register in all skins then.

-- 
 Gilles.

___
Xenomai-core mailing list
Xenomai-core@gna.org
https://mail.gna.org/listinfo/xenomai-core


Re: [Xenomai-core] [RFC][PATCH 2/3] Switch to handle-based fast mutex owners

2008-08-27 Thread Gilles Chanteperdrix
Jan Kiszka wrote:
 Gilles Chanteperdrix wrote:
 Jan Kiszka wrote:
 Gilles Chanteperdrix wrote:
 Jan Kiszka wrote:

 -#define test_claimed(owner) ((long) (owner)  1)
 -#define clear_claimed(owner) ((xnthread_t *) ((long) (owner)  ~1))
 -#define set_claimed(owner, bit) \
 -((xnthread_t *) ((long) clear_claimed(owner) | !!(bit)))
 +#define __CLAIMED_BITXN_HANDLE_SPARE3
 +
 +#define test_claimed(owner)  xnhandle_test_spare(owner, 
 __CLAIMED_BIT)
 +#define clear_claimed(owner) xnhandle_mask_spare(owner)
 +#define set_claimed(owner, bit) ({ \
 + xnhandle_t __tmp = xnhandle_mask_spare(owner); \
 + if (bit) \
 + xnhandle_set_spare(__tmp, __CLAIMED_BIT); \
 + __tmp; \
 +})
 I liked doing this with no conditional.
 You mean let the caller pass 'bit' as 0 or __CLAIMED_BIT (the latter
 would require some renaming then, I guess)?

 + xnarch_atomic_set(mutex-owner,
 +   set_claimed(xnthread_handle(owner),
 +   xnsynch_nsleepers(mutex-synchbase)));
 Ok. I think you have spotted a bug here. This should be mutex-sleepers
 instead of xnsynch_nsleepers.
 OK. Will you fix? I will rebase then.

 + /* Consistency check for owner handle - is the object a thread? */
 + if (unlikely(xnthread_handle(owner) != clear_claimed(ownerh))) {
 + err = -EINVAL;
 + goto error;
   }
 What is this ?
 This ensures that we are not dereferences some arbitrary registry object
 due to a borken handle in the mutex variable: The the object registered
 on this handle is an xnthread, we will find the very same handle value
 at the well-known place in the object. Kind of magic for xnthread
 objects (instead of adding a magic field to them).
 Yes, but the point is: how can the handle be borken ? It looks like a
 useless check to me.
 
 This is user memory. Everything can be borken. And I don't want to
 create yet another source for user-triggered kernel bugs (including
 silent ones) like with our heap management structures that are
 corruptible by userland. That's headache guarantee.

No, we assume that the user application will only clobber our heap if it
is broken too. Add this with a XENO_DEBUG option if you want (not
XENO_DEBUG(POSIX)).

-- 
 Gilles.

___
Xenomai-core mailing list
Xenomai-core@gna.org
https://mail.gna.org/listinfo/xenomai-core


Re: [Xenomai-core] [RFC][PATCH 2/3] Switch to handle-based fast mutex owners

2008-08-27 Thread Gilles Chanteperdrix
Jan Kiszka wrote:
 Gilles Chanteperdrix wrote:
 Jan Kiszka wrote:

 -#define test_claimed(owner) ((long) (owner)  1)
 -#define clear_claimed(owner) ((xnthread_t *) ((long) (owner)  ~1))
 -#define set_claimed(owner, bit) \
 -((xnthread_t *) ((long) clear_claimed(owner) | !!(bit)))
 +#define __CLAIMED_BIT  XN_HANDLE_SPARE3
 +
 +#define test_claimed(owner)xnhandle_test_spare(owner, 
 __CLAIMED_BIT)
 +#define clear_claimed(owner)   xnhandle_mask_spare(owner)
 +#define set_claimed(owner, bit) ({ \
 +   xnhandle_t __tmp = xnhandle_mask_spare(owner); \
 +   if (bit) \
 +   xnhandle_set_spare(__tmp, __CLAIMED_BIT); \
 +   __tmp; \
 +})
 I liked doing this with no conditional.
 
 You mean let the caller pass 'bit' as 0 or __CLAIMED_BIT (the latter
 would require some renaming then, I guess)?

the if is ok. I guess the compiler does !! with an if too...

-- 
 Gilles.

___
Xenomai-core mailing list
Xenomai-core@gna.org
https://mail.gna.org/listinfo/xenomai-core


Re: [Xenomai-core] [RFC][PATCH 2/3] Switch to handle-based fast mutex owners

2008-08-27 Thread Gilles Chanteperdrix
Jan Kiszka wrote:
 File descriptors are all identically structured objects, so at worst you
 ruin some other app's day. But the registry contains arbitrary objects
 with different internal layout. If you start assuming object_a * is
 object_b * and use the pointer etc. included in a as if they have the
 meaning of b, you quickly ruin the kernel's day as well. Therefore,
 native, e.g., does magic checks after fetching from the registry. As I
 said, this test here works differently, but it has the same effect and
 impact.

By the way, would not it make sense to have separate hash tables for
separate objects types ? I mean then we would not need any validation,
and several object types could use the same name.

-- 
 Gilles.

___
Xenomai-core mailing list
Xenomai-core@gna.org
https://mail.gna.org/listinfo/xenomai-core


Re: [Xenomai-core] [RFC][PATCH 2/3] Switch to handle-based fast mutex owners

2008-08-27 Thread Jan Kiszka
Gilles Chanteperdrix wrote:
 Jan Kiszka wrote:
 File descriptors are all identically structured objects, so at worst you
 ruin some other app's day. But the registry contains arbitrary objects
 with different internal layout. If you start assuming object_a * is
 object_b * and use the pointer etc. included in a as if they have the
 meaning of b, you quickly ruin the kernel's day as well. Therefore,
 native, e.g., does magic checks after fetching from the registry. As I
 said, this test here works differently, but it has the same effect and
 impact.
 
 By the way, would not it make sense to have separate hash tables for
 separate objects types ? I mean then we would not need any validation,
 and several object types could use the same name.

From that POV a good idea. The only issue I see is a management problem:
How many mutex, thread, queue, whatever slots do you want in your
system? One knob for them all? Or countless knobs for all object types
of all skins? That's hairy, I'm afraid.

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


Re: [Xenomai-core] [RFC][PATCH 2/3] Switch to handle-based fast mutex owners

2008-08-27 Thread Jan Kiszka
Gilles Chanteperdrix wrote:
 Jan Kiszka wrote:
 +xnarch_atomic_set(mutex-owner,
 +  set_claimed(xnthread_handle(owner),
 +  xnsynch_nsleepers(mutex-synchbase)));
 
 Ok. I think you have spotted a bug here. This should be mutex-sleepers
 instead of xnsynch_nsleepers.

BTW, why do you need to track sleepers separately in POSIX? Native
doesn't do so, e.g.

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


Re: [Xenomai-core] [RFC][PATCH 2/3] Switch to handle-based fast mutex owners

2008-08-27 Thread Jan Kiszka
Gilles Chanteperdrix wrote:
 Jan Kiszka wrote:
 
 -#define test_claimed(owner) ((long) (owner)  1)
 -#define clear_claimed(owner) ((xnthread_t *) ((long) (owner)  ~1))
 -#define set_claimed(owner, bit) \
 -((xnthread_t *) ((long) clear_claimed(owner) | !!(bit)))
 +#define __CLAIMED_BIT   XN_HANDLE_SPARE3
 +
 +#define test_claimed(owner) xnhandle_test_spare(owner, __CLAIMED_BIT)
 +#define clear_claimed(owner)xnhandle_mask_spare(owner)
 +#define set_claimed(owner, bit) ({ \
 +xnhandle_t __tmp = xnhandle_mask_spare(owner); \
 +if (bit) \
 +xnhandle_set_spare(__tmp, __CLAIMED_BIT); \
 +__tmp; \
 +})
 
 I liked doing this with no conditional.

You mean let the caller pass 'bit' as 0 or __CLAIMED_BIT (the latter
would require some renaming then, I guess)?

 
 +xnarch_atomic_set(mutex-owner,
 +  set_claimed(xnthread_handle(owner),
 +  xnsynch_nsleepers(mutex-synchbase)));
 
 Ok. I think you have spotted a bug here. This should be mutex-sleepers
 instead of xnsynch_nsleepers.

OK. Will you fix? I will rebase then.

 
 +/* Consistency check for owner handle - is the object a thread? */
 +if (unlikely(xnthread_handle(owner) != clear_claimed(ownerh))) {
 +err = -EINVAL;
 +goto error;
  }
 
 What is this ?

This ensures that we are not dereferences some arbitrary registry object
due to a borken handle in the mutex variable: The the object registered
on this handle is an xnthread, we will find the very same handle value
at the well-known place in the object. Kind of magic for xnthread
objects (instead of adding a magic field to them).

 
 
 Index: b/ksrc/skins/posix/thread.c
 ===
 --- a/ksrc/skins/posix/thread.c
 +++ b/ksrc/skins/posix/thread.c
 @@ -28,6 +28,7 @@
   * 
   [EMAIL PROTECTED]/
  
 +#include nucleus/registry.h
  #include posix/thread.h
  #include posix/cancel.h
  #include posix/timer.h
 @@ -229,6 +230,13 @@ int pthread_create(pthread_t *tid,
  appendq(thread-container, thread-link);
  xnlock_put_irqrestore(nklock, s);
  
 +#ifdef CONFIG_XENO_OPT_REGISTRY
 +/* We need an anonymous registry entry to obtain a handle for fast
 +   mutex locking. */
 +xnregistry_enter(, thread-threadbase,
 + xnthread_handle(thread-threadbase), NULL);
 +#endif /* CONFIG_XENO_OPT_REGISTRY */
 +
 
 Since we need this for all threads (I want a thread from any skin to be
 able to use the posix skin mutexes), why doing it in the skin ?

Some skins register their threads unconditionally, some based on names,
some not at all. The not at all case has to be fixed, but we need to
respect the other two as well.

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


Re: [Xenomai-core] [RFC][PATCH 2/3] Switch to handle-based fast mutex owners

2008-08-27 Thread Gilles Chanteperdrix
Jan Kiszka wrote:
 Gilles Chanteperdrix wrote:
 Jan Kiszka wrote:
 +   xnarch_atomic_set(mutex-owner,
 + set_claimed(xnthread_handle(owner),
 + xnsynch_nsleepers(mutex-synchbase)));
 Ok. I think you have spotted a bug here. This should be mutex-sleepers
 instead of xnsynch_nsleepers.
 
 BTW, why do you need to track sleepers separately in POSIX? Native
 doesn't do so, e.g.

Because of the syscall-needed-when-unlocking-stolen-mutex issue I
already explained (sleepers - xnsynch_nsleepers is precisely the count
of pending threads which have been awake then robbed the mutex).

-- 
 Gilles.

___
Xenomai-core mailing list
Xenomai-core@gna.org
https://mail.gna.org/listinfo/xenomai-core


Re: [Xenomai-core] [RFC][PATCH 2/3] Switch to handle-based fast mutex owners

2008-08-27 Thread Gilles Chanteperdrix
Jan Kiszka wrote:
 Gilles Chanteperdrix wrote:
 Jan Kiszka wrote:
 File descriptors are all identically structured objects, so at worst you
 ruin some other app's day. But the registry contains arbitrary objects
 with different internal layout. If you start assuming object_a * is
 object_b * and use the pointer etc. included in a as if they have the
 meaning of b, you quickly ruin the kernel's day as well. Therefore,
 native, e.g., does magic checks after fetching from the registry. As I
 said, this test here works differently, but it has the same effect and
 impact.
 By the way, would not it make sense to have separate hash tables for
 separate objects types ? I mean then we would not need any validation,
 and several object types could use the same name.
 
 From that POV a good idea. The only issue I see is a management problem:
 How many mutex, thread, queue, whatever slots do you want in your
 system? One knob for them all? Or countless knobs for all object types
 of all skins? That's hairy, I'm afraid.

xnmalloc, the pool size is the limit.

-- 
 Gilles.

___
Xenomai-core mailing list
Xenomai-core@gna.org
https://mail.gna.org/listinfo/xenomai-core


Re: [Xenomai-core] [RFC][PATCH 2/3] Switch to handle-based fast mutex owners

2008-08-27 Thread Jan Kiszka
Gilles Chanteperdrix wrote:
 Jan Kiszka wrote:
 Gilles Chanteperdrix wrote:
 Jan Kiszka wrote:
 File descriptors are all identically structured objects, so at worst you
 ruin some other app's day. But the registry contains arbitrary objects
 with different internal layout. If you start assuming object_a * is
 object_b * and use the pointer etc. included in a as if they have the
 meaning of b, you quickly ruin the kernel's day as well. Therefore,
 native, e.g., does magic checks after fetching from the registry. As I
 said, this test here works differently, but it has the same effect and
 impact.
 By the way, would not it make sense to have separate hash tables for
 separate objects types ? I mean then we would not need any validation,
 and several object types could use the same name.
 From that POV a good idea. The only issue I see is a management problem:
 How many mutex, thread, queue, whatever slots do you want in your
 system? One knob for them all? Or countless knobs for all object types
 of all skins? That's hairy, I'm afraid.
 
 xnmalloc, the pool size is the limit.

You mean kind of xnrealloc, including atomically copying potentially
large descriptor tables over? Sounds not that attractive.

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


Re: [Xenomai-core] [RFC][PATCH 2/3] Switch to handle-based fast mutex owners

2008-08-27 Thread Gilles Chanteperdrix
Jan Kiszka wrote:
 Gilles Chanteperdrix wrote:
 Jan Kiszka wrote:
 Gilles Chanteperdrix wrote:
 Jan Kiszka wrote:
 File descriptors are all identically structured objects, so at worst you
 ruin some other app's day. But the registry contains arbitrary objects
 with different internal layout. If you start assuming object_a * is
 object_b * and use the pointer etc. included in a as if they have the
 meaning of b, you quickly ruin the kernel's day as well. Therefore,
 native, e.g., does magic checks after fetching from the registry. As I
 said, this test here works differently, but it has the same effect and
 impact.
 By the way, would not it make sense to have separate hash tables for
 separate objects types ? I mean then we would not need any validation,
 and several object types could use the same name.
 From that POV a good idea. The only issue I see is a management problem:
 How many mutex, thread, queue, whatever slots do you want in your
 system? One knob for them all? Or countless knobs for all object types
 of all skins? That's hairy, I'm afraid.
 xnmalloc, the pool size is the limit.
 
 You mean kind of xnrealloc, including atomically copying potentially
 large descriptor tables over? Sounds not that attractive.

No, I mean the hash table contains pointers, and the objects are
allocated dynamically...

-- 
 Gilles.

___
Xenomai-core mailing list
Xenomai-core@gna.org
https://mail.gna.org/listinfo/xenomai-core


Re: [Xenomai-core] [RFC][PATCH 2/3] Switch to handle-based fast mutex owners

2008-08-27 Thread Jan Kiszka
Gilles Chanteperdrix wrote:
 Jan Kiszka wrote:
 Gilles Chanteperdrix wrote:
 Jan Kiszka wrote:
 Gilles Chanteperdrix wrote:
 Jan Kiszka wrote:
 File descriptors are all identically structured objects, so at worst you
 ruin some other app's day. But the registry contains arbitrary objects
 with different internal layout. If you start assuming object_a * is
 object_b * and use the pointer etc. included in a as if they have the
 meaning of b, you quickly ruin the kernel's day as well. Therefore,
 native, e.g., does magic checks after fetching from the registry. As I
 said, this test here works differently, but it has the same effect and
 impact.
 By the way, would not it make sense to have separate hash tables for
 separate objects types ? I mean then we would not need any validation,
 and several object types could use the same name.
 From that POV a good idea. The only issue I see is a management problem:
 How many mutex, thread, queue, whatever slots do you want in your
 system? One knob for them all? Or countless knobs for all object types
 of all skins? That's hairy, I'm afraid.
 xnmalloc, the pool size is the limit.
 You mean kind of xnrealloc, including atomically copying potentially
 large descriptor tables over? Sounds not that attractive.
 
 No, I mean the hash table contains pointers, and the objects are
 allocated dynamically...

Handle lookup is not going through hash tables. It's index based (like
file descriptors).

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


Re: [Xenomai-core] [RFC][PATCH 2/3] Switch to handle-based fast mutex owners

2008-08-27 Thread Jan Kiszka
Gilles Chanteperdrix wrote:
 Jan Kiszka wrote:
 Gilles Chanteperdrix wrote:
 Jan Kiszka wrote:
 To improve robustness of the fast mutex implementation in POSIX (and
 later on in native), it is better to track the mutex owner by handle
 instead of kernel object pointer. Therefore, this patch changes
 __xn_sys_current (xeno_set_current) so that it returns
 xnthread_handle(current_thread). It furthermore converts the POSIX mutex
 implementation to pick up and store the lock owner as handle in the
 kernel/user-shared mutex. Finally it ensures that at least POSIX threads
 always have an (anonymous) handle assigned.

 As the value stored in the mutex variable is now an integer, we can
 switch over to xnarch_atomic_t, removing all atomic_intptr users.
 Ok. I do not know if this should be part of this patch, or in another
 one, but we should call xeno_set_current in the trampolines of all
 skins, so that they can use the native and posix mutexes.

 This is another thing that I have left in a state of flux...
 Find it below (PATCH 4/3).

 BTW, should we better invoke pthread_exit in xeno_set_current in case of 
 a failure?
 
 I prefer my application to die. This way I would know that something
 went wrong. And by the way, how could this syscall fail ? I mean if
 xenomai or the posix skin are not loaded, we would have known it earlier.

So far this syscall can trigger a thread handle registration if the
current thread has no handle yet. But that could go away if all skins
ensure that there is at least an anonymous handle for their threads
after creation.

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


Re: [Xenomai-core] [RFC][PATCH 2/3] Switch to handle-based fast mutex owners

2008-08-27 Thread Jan Kiszka
Gilles Chanteperdrix wrote:
 Jan Kiszka wrote:
 Gilles Chanteperdrix wrote:
 Jan Kiszka wrote:

 -#define test_claimed(owner) ((long) (owner)  1)
 -#define clear_claimed(owner) ((xnthread_t *) ((long) (owner)  ~1))
 -#define set_claimed(owner, bit) \
 -((xnthread_t *) ((long) clear_claimed(owner) | !!(bit)))
 +#define __CLAIMED_BIT XN_HANDLE_SPARE3
 +
 +#define test_claimed(owner)   xnhandle_test_spare(owner, 
 __CLAIMED_BIT)
 +#define clear_claimed(owner)  xnhandle_mask_spare(owner)
 +#define set_claimed(owner, bit) ({ \
 +  xnhandle_t __tmp = xnhandle_mask_spare(owner); \
 +  if (bit) \
 +  xnhandle_set_spare(__tmp, __CLAIMED_BIT); \
 +  __tmp; \
 +})
 I liked doing this with no conditional.
 You mean let the caller pass 'bit' as 0 or __CLAIMED_BIT (the latter
 would require some renaming then, I guess)?

 +  xnarch_atomic_set(mutex-owner,
 +set_claimed(xnthread_handle(owner),
 +xnsynch_nsleepers(mutex-synchbase)));
 Ok. I think you have spotted a bug here. This should be mutex-sleepers
 instead of xnsynch_nsleepers.
 OK. Will you fix? I will rebase then.

 +  /* Consistency check for owner handle - is the object a thread? */
 +  if (unlikely(xnthread_handle(owner) != clear_claimed(ownerh))) {
 +  err = -EINVAL;
 +  goto error;
}
 What is this ?
 This ensures that we are not dereferences some arbitrary registry object
 due to a borken handle in the mutex variable: The the object registered
 on this handle is an xnthread, we will find the very same handle value
 at the well-known place in the object. Kind of magic for xnthread
 objects (instead of adding a magic field to them).
 
 Yes, but the point is: how can the handle be borken ? It looks like a
 useless check to me.

This is user memory. Everything can be borken. And I don't want to
create yet another source for user-triggered kernel bugs (including
silent ones) like with our heap management structures that are
corruptible by userland. That's headache guarantee.

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


Re: [Xenomai-core] [RFC][PATCH 2/3] Switch to handle-based fast mutex owners

2008-08-27 Thread Jan Kiszka
Gilles Chanteperdrix wrote:
 Jan Kiszka wrote:
 Gilles Chanteperdrix wrote:
 Jan Kiszka wrote:
 +  xnarch_atomic_set(mutex-owner,
 +set_claimed(xnthread_handle(owner),
 +xnsynch_nsleepers(mutex-synchbase)));
 Ok. I think you have spotted a bug here. This should be mutex-sleepers
 instead of xnsynch_nsleepers.
 BTW, why do you need to track sleepers separately in POSIX? Native
 doesn't do so, e.g.
 
 Because of the syscall-needed-when-unlocking-stolen-mutex issue I
 already explained (sleepers - xnsynch_nsleepers is precisely the count
 of pending threads which have been awake then robbed the mutex).

Hmm, sounds like the new lock owner should better clear the 'claimed'
bit then, not the old one on return from unlock. Or where is the
pitfall? How does the futex algorithm handle this scenario?

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


Re: [Xenomai-core] [RFC][PATCH 2/3] Switch to handle-based fast mutex owners

2008-08-27 Thread Gilles Chanteperdrix
Jan Kiszka wrote:
 Gilles Chanteperdrix wrote:
 Jan Kiszka wrote:
 Gilles Chanteperdrix wrote:
 Jan Kiszka wrote:
 + xnarch_atomic_set(mutex-owner,
 +   set_claimed(xnthread_handle(owner),
 +   xnsynch_nsleepers(mutex-synchbase)));
 Ok. I think you have spotted a bug here. This should be mutex-sleepers
 instead of xnsynch_nsleepers.
 BTW, why do you need to track sleepers separately in POSIX? Native
 doesn't do so, e.g.
 Because of the syscall-needed-when-unlocking-stolen-mutex issue I
 already explained (sleepers - xnsynch_nsleepers is precisely the count
 of pending threads which have been awake then robbed the mutex).
 
 Hmm, sounds like the new lock owner should better clear the 'claimed'
 bit then, not the old one on return from unlock. Or where is the
 pitfall? How does the futex algorithm handle this scenario?

Ok. Please read my explanation again, I have already explained this in
another mail.

-- 
 Gilles.

___
Xenomai-core mailing list
Xenomai-core@gna.org
https://mail.gna.org/listinfo/xenomai-core


Re: [Xenomai-core] [RFC][PATCH 2/3] Switch to handle-based fast mutex owners

2008-08-27 Thread Jan Kiszka
Gilles Chanteperdrix wrote:
 Jan Kiszka wrote:
 Gilles Chanteperdrix wrote:
 Jan Kiszka wrote:
 Gilles Chanteperdrix wrote:
 Jan Kiszka wrote:

 -#define test_claimed(owner) ((long) (owner)  1)
 -#define clear_claimed(owner) ((xnthread_t *) ((long) (owner)  ~1))
 -#define set_claimed(owner, bit) \
 -((xnthread_t *) ((long) clear_claimed(owner) | !!(bit)))
 +#define __CLAIMED_BIT   XN_HANDLE_SPARE3
 +
 +#define test_claimed(owner) xnhandle_test_spare(owner, 
 __CLAIMED_BIT)
 +#define clear_claimed(owner)xnhandle_mask_spare(owner)
 +#define set_claimed(owner, bit) ({ \
 +xnhandle_t __tmp = xnhandle_mask_spare(owner); \
 +if (bit) \
 +xnhandle_set_spare(__tmp, __CLAIMED_BIT); \
 +__tmp; \
 +})
 I liked doing this with no conditional.
 You mean let the caller pass 'bit' as 0 or __CLAIMED_BIT (the latter
 would require some renaming then, I guess)?

 +xnarch_atomic_set(mutex-owner,
 +  set_claimed(xnthread_handle(owner),
 +  
 xnsynch_nsleepers(mutex-synchbase)));
 Ok. I think you have spotted a bug here. This should be mutex-sleepers
 instead of xnsynch_nsleepers.
 OK. Will you fix? I will rebase then.

 +/* Consistency check for owner handle - is the object a thread? 
 */
 +if (unlikely(xnthread_handle(owner) != clear_claimed(ownerh))) {
 +err = -EINVAL;
 +goto error;
  }
 What is this ?
 This ensures that we are not dereferences some arbitrary registry object
 due to a borken handle in the mutex variable: The the object registered
 on this handle is an xnthread, we will find the very same handle value
 at the well-known place in the object. Kind of magic for xnthread
 objects (instead of adding a magic field to them).
 Yes, but the point is: how can the handle be borken ? It looks like a
 useless check to me.
 This is user memory. Everything can be borken. And I don't want to
 create yet another source for user-triggered kernel bugs (including
 silent ones) like with our heap management structures that are
 corruptible by userland. That's headache guarantee.
 
 No, we assume that the user application will only clobber our heap if it
 is broken too. Add this with a XENO_DEBUG option if you want (not
 XENO_DEBUG(POSIX)).

No problem for me. This check is almost 100% identical (specifically
overhead-wise) to the usual object magic checks after querying the
registry, and as those are unconditionally performed, I saw no need to
handle this case differently.

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


Re: [Xenomai-core] [RFC][PATCH 2/3] Switch to handle-based fast mutex owners

2008-08-27 Thread Jan Kiszka
Gilles Chanteperdrix wrote:
 Jan Kiszka wrote:
 Gilles Chanteperdrix wrote:
 Jan Kiszka wrote:
 Gilles Chanteperdrix wrote:
 Jan Kiszka wrote:
 +xnarch_atomic_set(mutex-owner,
 +  set_claimed(xnthread_handle(owner),
 +  
 xnsynch_nsleepers(mutex-synchbase)));
 Ok. I think you have spotted a bug here. This should be mutex-sleepers
 instead of xnsynch_nsleepers.
 BTW, why do you need to track sleepers separately in POSIX? Native
 doesn't do so, e.g.
 Because of the syscall-needed-when-unlocking-stolen-mutex issue I
 already explained (sleepers - xnsynch_nsleepers is precisely the count
 of pending threads which have been awake then robbed the mutex).
 Hmm, sounds like the new lock owner should better clear the 'claimed'
 bit then, not the old one on return from unlock. Or where is the
 pitfall? How does the futex algorithm handle this scenario?
 
 Ok. Please read my explanation again, I have already explained this in
 another mail.

I did this, but I'm unable to derive the answer for my question from it.
Let's go through it in more details:

When we pass a mutex to a new owner, we set its reference in the fast
lock variable + set the claimed bit if there are more waiters. Instead,
I would simple set that bit if there is a new owner. That owner will
then pick up the mutex eventually and clear 'claimed' on exit from it
lock service (if there are no further waiters then). If the new owner is
not able to run and we steal the lock, we simple keep the 'claimed' bit
as is. On exit from the stolen lock we find it set, thus we are forced
to issue a syscall as it should be.

OK, what happens if some waiter wants to leave the party while we are
holding the stolen lock? Then the sleeper number must be correct - that
is one pitfall!

I will have to dig into this more deeply, considering more cases. But
the additional sleepers field remains at least misplaced IMHO.
xnsynch_sleepers should better be fixed to respect lock stealing, as
lock stealing is an xnsynch property, nothing POSIX-specific.

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


Re: [Xenomai-core] [RFC][PATCH 2/3] Switch to handle-based fast mutex owners

2008-08-27 Thread Gilles Chanteperdrix
Jan Kiszka wrote:
 Gilles Chanteperdrix wrote:
 Jan Kiszka wrote:
 Gilles Chanteperdrix wrote:
 Jan Kiszka wrote:
 Gilles Chanteperdrix wrote:
 Jan Kiszka wrote:
 +   xnarch_atomic_set(mutex-owner,
 + set_claimed(xnthread_handle(owner),
 + 
 xnsynch_nsleepers(mutex-synchbase)));
 Ok. I think you have spotted a bug here. This should be mutex-sleepers
 instead of xnsynch_nsleepers.
 BTW, why do you need to track sleepers separately in POSIX? Native
 doesn't do so, e.g.
 Because of the syscall-needed-when-unlocking-stolen-mutex issue I
 already explained (sleepers - xnsynch_nsleepers is precisely the count
 of pending threads which have been awake then robbed the mutex).
 Hmm, sounds like the new lock owner should better clear the 'claimed'
 bit then, not the old one on return from unlock. Or where is the
 pitfall? How does the futex algorithm handle this scenario?
 Ok. Please read my explanation again, I have already explained this in
 another mail.
 
 I did this, but I'm unable to derive the answer for my question from it.
 Let's go through it in more details:
 
 When we pass a mutex to a new owner, we set its reference in the fast
 lock variable + set the claimed bit if there are more waiters. Instead,
 I would simple set that bit if there is a new owner. That owner will
 then pick up the mutex eventually and clear 'claimed' on exit from it
 lock service (if there are no further waiters then). If the new owner is
 not able to run and we steal the lock, we simple keep the 'claimed' bit
 as is. On exit from the stolen lock we find it set, thus we are forced
 to issue a syscall as it should be.
 
 OK, what happens if some waiter wants to leave the party while we are
 holding the stolen lock? Then the sleeper number must be correct - that
 is one pitfall!
 
 I will have to dig into this more deeply, considering more cases. But
 the additional sleepers field remains at least misplaced IMHO.
 xnsynch_sleepers should better be fixed to respect lock stealing, as
 lock stealing is an xnsynch property, nothing POSIX-specific.

Ok. I have read this but did not get what you mean. I will read it again
 quietly from home.

-- 
 Gilles.

___
Xenomai-core mailing list
Xenomai-core@gna.org
https://mail.gna.org/listinfo/xenomai-core


Re: [Xenomai-core] [RFC][PATCH 2/3] Switch to handle-based fast mutex owners

2008-08-27 Thread Jan Kiszka
Gilles Chanteperdrix wrote:
 Jan Kiszka wrote:
 Gilles Chanteperdrix wrote:
 Jan Kiszka wrote:
 Gilles Chanteperdrix wrote:
 Jan Kiszka wrote:
 Gilles Chanteperdrix wrote:
 Jan Kiszka wrote:
 +  xnarch_atomic_set(mutex-owner,
 +set_claimed(xnthread_handle(owner),
 +
 xnsynch_nsleepers(mutex-synchbase)));
 Ok. I think you have spotted a bug here. This should be mutex-sleepers
 instead of xnsynch_nsleepers.
 BTW, why do you need to track sleepers separately in POSIX? Native
 doesn't do so, e.g.
 Because of the syscall-needed-when-unlocking-stolen-mutex issue I
 already explained (sleepers - xnsynch_nsleepers is precisely the count
 of pending threads which have been awake then robbed the mutex).
 Hmm, sounds like the new lock owner should better clear the 'claimed'
 bit then, not the old one on return from unlock. Or where is the
 pitfall? How does the futex algorithm handle this scenario?
 Ok. Please read my explanation again, I have already explained this in
 another mail.
 I did this, but I'm unable to derive the answer for my question from it.
 Let's go through it in more details:

 When we pass a mutex to a new owner, we set its reference in the fast
 lock variable + set the claimed bit if there are more waiters. Instead,
 I would simple set that bit if there is a new owner. That owner will
 then pick up the mutex eventually and clear 'claimed' on exit from it
 lock service (if there are no further waiters then). If the new owner is
 not able to run and we steal the lock, we simple keep the 'claimed' bit
 as is. On exit from the stolen lock we find it set, thus we are forced
 to issue a syscall as it should be.

 OK, what happens if some waiter wants to leave the party while we are
 holding the stolen lock? Then the sleeper number must be correct - that
 is one pitfall!

 I will have to dig into this more deeply, considering more cases. But
 the additional sleepers field remains at least misplaced IMHO.
 xnsynch_sleepers should better be fixed to respect lock stealing, as
 lock stealing is an xnsynch property, nothing POSIX-specific.
 
 Ok. I have read this but did not get what you mean. I will read it again
  quietly from home.

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.

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.

Jan



signature.asc
Description: OpenPGP digital signature
___
Xenomai-core mailing list
Xenomai-core@gna.org
https://mail.gna.org/listinfo/xenomai-core


Re: [Xenomai-core] [RFC][PATCH 2/3] Switch to handle-based fast mutex owners

2008-08-27 Thread Gilles Chanteperdrix
Jan Kiszka wrote:
 Gilles Chanteperdrix wrote:
 Jan Kiszka wrote:
 Gilles Chanteperdrix wrote:
 Jan Kiszka wrote:
 Gilles Chanteperdrix wrote:
 Jan Kiszka wrote:
 Gilles Chanteperdrix wrote:
 Jan Kiszka wrote:
 + xnarch_atomic_set(mutex-owner,
 +   set_claimed(xnthread_handle(owner),
 +   
 xnsynch_nsleepers(mutex-synchbase)));
 Ok. I think you have spotted a bug here. This should be mutex-sleepers
 instead of xnsynch_nsleepers.
 BTW, why do you need to track sleepers separately in POSIX? Native
 doesn't do so, e.g.
 Because of the syscall-needed-when-unlocking-stolen-mutex issue I
 already explained (sleepers - xnsynch_nsleepers is precisely the count
 of pending threads which have been awake then robbed the mutex).
 Hmm, sounds like the new lock owner should better clear the 'claimed'
 bit then, not the old one on return from unlock. Or where is the
 pitfall? How does the futex algorithm handle this scenario?
 Ok. Please read my explanation again, I have already explained this in
 another mail.
 I did this, but I'm unable to derive the answer for my question from it.
 Let's go through it in more details:

 When we pass a mutex to a new owner, we set its reference in the fast
 lock variable + set the claimed bit if there are more waiters. Instead,
 I would simple set that bit if there is a new owner. That owner will
 then pick up the mutex eventually and clear 'claimed' on exit from it
 lock service (if there are no further waiters then). If the new owner is
 not able to run and we steal the lock, we simple keep the 'claimed' bit
 as is. On exit from the stolen lock we find it set, thus we are forced
 to issue a syscall as it should be.

 OK, what happens if some waiter wants to leave the party while we are
 holding the stolen lock? Then the sleeper number must be correct - that
 is one pitfall!

 I will have to dig into this more deeply, considering more cases. But
 the additional sleepers field remains at least misplaced IMHO.
 xnsynch_sleepers should better be fixed to respect lock stealing, as
 lock stealing is an xnsynch property, nothing POSIX-specific.
 Ok. I have read this but did not get what you mean. I will read it again
  quietly from home.
 
 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.

-- 
Gilles.

___
Xenomai-core mailing list
Xenomai-core@gna.org
https://mail.gna.org/listinfo/xenomai-core


Re: [Xenomai-core] [RFC][PATCH 2/3] Switch to handle-based fast mutex owners

2008-08-27 Thread Gilles Chanteperdrix
Gilles Chanteperdrix wrote:
 Jan Kiszka wrote:
 Gilles Chanteperdrix wrote:
 Jan Kiszka wrote:
 Gilles Chanteperdrix wrote:
 Jan Kiszka wrote:
 Gilles Chanteperdrix wrote:
 Jan Kiszka wrote:
 Gilles Chanteperdrix wrote:
 Jan Kiszka wrote:
 +xnarch_atomic_set(mutex-owner,
 +  set_claimed(xnthread_handle(owner),
 +  
 xnsynch_nsleepers(mutex-synchbase)));
 Ok. I think you have spotted a bug here. This should be 
 mutex-sleepers
 instead of xnsynch_nsleepers.
 BTW, why do you need to track sleepers separately in POSIX? Native
 doesn't do so, e.g.
 Because of the syscall-needed-when-unlocking-stolen-mutex issue I
 already explained (sleepers - xnsynch_nsleepers is precisely the count
 of pending threads which have been awake then robbed the mutex).
 Hmm, sounds like the new lock owner should better clear the 'claimed'
 bit then, not the old one on return from unlock. Or where is the
 pitfall? How does the futex algorithm handle this scenario?
 Ok. Please read my explanation again, I have already explained this in
 another mail.
 I did this, but I'm unable to derive the answer for my question from it.
 Let's go through it in more details:

 When we pass a mutex to a new owner, we set its reference in the fast
 lock variable + set the claimed bit if there are more waiters. Instead,
 I would simple set that bit if there is a new owner. That owner will
 then pick up the mutex eventually and clear 'claimed' on exit from it
 lock service (if there are no further waiters then). If the new owner is
 not able to run and we steal the lock, we simple keep the 'claimed' bit
 as is. On exit from the stolen lock we find it set, thus we are forced
 to issue a syscall as it should be.

 OK, what happens if some waiter wants to leave the party while we are
 holding the stolen lock? Then the sleeper number must be correct - that
 is one pitfall!

 I will have to dig into this more deeply, considering more cases. But
 the additional sleepers field remains at least misplaced IMHO.
 xnsynch_sleepers should better be fixed to respect lock stealing, as
 lock stealing is an xnsynch property, nothing POSIX-specific.
 Ok. I have read this but did not get what you mean. I will read it again
  quietly from home.
 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.

By the way, I think we should stop sending mails to our personal
addresses in addition to the mailing list, because this results in
mailing list mails being received out of orders, which make the threads
hard to follow.

-- 
Gilles.

___
Xenomai-core mailing list
Xenomai-core@gna.org
https://mail.gna.org/listinfo/xenomai-core


Re: [Xenomai-core] [RFC][PATCH 2/3] Switch to handle-based fast mutex owners

2008-08-27 Thread Jan Kiszka
Gilles Chanteperdrix wrote:
 Gilles Chanteperdrix wrote:
 Jan Kiszka wrote:
 Gilles Chanteperdrix wrote:
 Jan Kiszka wrote:
 Gilles Chanteperdrix wrote:
 Jan Kiszka wrote:
 Gilles Chanteperdrix wrote:
 Jan Kiszka wrote:
 Gilles Chanteperdrix wrote:
 Jan Kiszka wrote:
 +   xnarch_atomic_set(mutex-owner,
 + set_claimed(xnthread_handle(owner),
 + 
 xnsynch_nsleepers(mutex-synchbase)));
 Ok. I think you have spotted a bug here. This should be 
 mutex-sleepers
 instead of xnsynch_nsleepers.
 BTW, why do you need to track sleepers separately in POSIX? Native
 doesn't do so, e.g.
 Because of the syscall-needed-when-unlocking-stolen-mutex issue I
 already explained (sleepers - xnsynch_nsleepers is precisely the count
 of pending threads which have been awake then robbed the mutex).
 Hmm, sounds like the new lock owner should better clear the 'claimed'
 bit then, not the old one on return from unlock. Or where is the
 pitfall? How does the futex algorithm handle this scenario?
 Ok. Please read my explanation again, I have already explained this in
 another mail.
 I did this, but I'm unable to derive the answer for my question from it.
 Let's go through it in more details:

 When we pass a mutex to a new owner, we set its reference in the fast
 lock variable + set the claimed bit if there are more waiters. Instead,
 I would simple set that bit if there is a new owner. That owner will
 then pick up the mutex eventually and clear 'claimed' on exit from it
 lock service (if there are no further waiters then). If the new owner is
 not able to run and we steal the lock, we simple keep the 'claimed' bit
 as is. On exit from the stolen lock we find it set, thus we are forced
 to issue a syscall as it should be.

 OK, what happens if some waiter wants to leave the party while we are
 holding the stolen lock? Then the sleeper number must be correct - that
 is one pitfall!

 I will have to dig into this more deeply, considering more cases. But
 the additional sleepers field remains at least misplaced IMHO.
 xnsynch_sleepers should better be fixed to respect lock stealing, as
 lock stealing is an xnsynch property, nothing POSIX-specific.
 Ok. I have read this but did not get what you mean. I will read it again
  quietly from home.
 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.
 
 By the way, I think we should stop sending mails to our personal
 addresses in addition to the mailing list, because this results in
 mailing list mails being received out of orders, which make the threads
 hard to follow.

That's common practice on most mailing lists I know of, and I personally
don't want to change this. IMO, it would only make replying more
complicated, and it would bear the risk to drop CCs to non-subscribers.

I think the problem was only temporarily, maybe caused by some weird
interaction of gna.org and the Siemens mailserver (we were too fast for
them). Meanwhile, archives and inboxes should contain all messages.
Gmane, e.g., lists them in the correct order now.

Jan



signature.asc
Description: OpenPGP digital signature

Re: [Xenomai-core] [RFC][PATCH 2/3] Switch to handle-based fast mutex owners

2008-08-27 Thread Jan Kiszka
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.

Jan

---
 ksrc/skins/posix/mutex.c |   10 +++---
 ksrc/skins/posix/mutex.h |   17 +++--
 2 files changed, 18 insertions(+), 9 deletions(-)

Index: b/ksrc/skins/posix/mutex.c
===
--- a/ksrc/skins/posix/mutex.c
+++ b/ksrc/skins/posix/mutex.c
@@ -117,7 +117,6 @@ int pse51_mutex_init_internal(struct __s
mutex-attr = *attr;
mutex-owner = ownerp;
mutex-owningq = kq;
-   mutex-sleepers = 0;
xnarch_atomic_set(ownerp, XN_NO_HANDLE);
 
xnlock_get_irqsave(nklock, s);
@@ -305,14 +304,12 @@ int pse51_mutex_timedlock_break(struct _
/* Attempting to relock a normal mutex, deadlock. */
xnlock_get_irqsave(nklock, s);
for (;;) {
-   ++mutex-sleepers;
if (timed)
xnsynch_sleep_on(mutex-synchbase,
 abs_to, XN_REALTIME);
else
xnsynch_sleep_on(mutex-synchbase,
 XN_INFINITE, XN_RELATIVE);
-   --mutex-sleepers;
 
if (xnthread_test_info(cur, XNBREAK)) {
err = -EINTR;
@@ -329,6 +326,13 @@ int pse51_mutex_timedlock_break(struct _
break;
}
}
+   if (!xnsynch_nsleepers(mutex-synchbase)) {
+   xnarch_atomic_set
+   (mutex-owner,
+clear_claimed
+   (xnarch_atomic_get(mutex-owner)));
+   xnsynch_set_owner(mutex-synchbase, NULL);
+   }
xnlock_put_irqrestore(nklock, s);
 
break;
Index: b/ksrc/skins/posix/mutex.h
===
--- a/ksrc/skins/posix/mutex.h
+++ b/ksrc/skins/posix/mutex.h
@@ -57,7 +57,6 @@ typedef struct pse51_mutex {
 
xnarch_atomic_t *owner;
pthread_mutexattr_t attr;
-   unsigned sleepers;
pse51_kqueues_t *owningq;
 } pse51_mutex_t;
 
@@ -172,12 +171,10 @@ static inline int pse51_mutex_timedlock_
}
 
xnsynch_set_owner(mutex-synchbase, owner);
-   ++mutex-sleepers;
if (timed)
xnsynch_sleep_on(mutex-synchbase, abs_to, 

Re: [Xenomai-core] [RFC][PATCH 2/3] Switch to handle-based fast mutex owners

2008-08-27 Thread Gilles Chanteperdrix
Jan Kiszka wrote:
 Gilles Chanteperdrix wrote:
 Gilles Chanteperdrix wrote:
 Jan Kiszka wrote:
 Gilles Chanteperdrix wrote:
 Jan Kiszka wrote:
 Gilles Chanteperdrix wrote:
 Jan Kiszka wrote:
 Gilles Chanteperdrix wrote:
 Jan Kiszka wrote:
 Gilles Chanteperdrix wrote:
 Jan Kiszka wrote:
 +  xnarch_atomic_set(mutex-owner,
 +set_claimed(xnthread_handle(owner),
 +
 xnsynch_nsleepers(mutex-synchbase)));
 Ok. I think you have spotted a bug here. This should be 
 mutex-sleepers
 instead of xnsynch_nsleepers.
 BTW, why do you need to track sleepers separately in POSIX? Native
 doesn't do so, e.g.
 Because of the syscall-needed-when-unlocking-stolen-mutex issue I
 already explained (sleepers - xnsynch_nsleepers is precisely the count
 of pending threads which have been awake then robbed the mutex).
 Hmm, sounds like the new lock owner should better clear the 'claimed'
 bit then, not the old one on return from unlock. Or where is the
 pitfall? How does the futex algorithm handle this scenario?
 Ok. Please read my explanation again, I have already explained this in
 another mail.
 I did this, but I'm unable to derive the answer for my question from it.
 Let's go through it in more details:

 When we pass a mutex to a new owner, we set its reference in the fast
 lock variable + set the claimed bit if there are more waiters. Instead,
 I would simple set that bit if there is a new owner. That owner will
 then pick up the mutex eventually and clear 'claimed' on exit from it
 lock service (if there are no further waiters then). If the new owner is
 not able to run and we steal the lock, we simple keep the 'claimed' bit
 as is. On exit from the stolen lock we find it set, thus we are forced
 to issue a syscall as it should be.

 OK, what happens if some waiter wants to leave the party while we are
 holding the stolen lock? Then the sleeper number must be correct - that
 is one pitfall!

 I will have to dig into this more deeply, considering more cases. But
 the additional sleepers field remains at least misplaced IMHO.
 xnsynch_sleepers should better be fixed to respect lock stealing, as
 lock stealing is an xnsynch property, nothing POSIX-specific.
 Ok. I have read this but did not get what you mean. I will read it again
  quietly from home.
 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.
 By the way, I think we should stop sending mails to our personal
 addresses in addition to the mailing list, because this results in
 mailing list mails being received out of orders, which make the threads
 hard to follow.
 
 That's common practice on most mailing lists I know of, and I personally
 don't want to change this. IMO, it would only make replying more
 complicated, and it would bear the risk to drop CCs to non-subscribers.
 
 I think the problem was only temporarily, maybe caused by some weird
 interaction of gna.org and the Siemens mailserver (we were too fast for
 them). Meanwhile, archives and inboxes should contain all messages.
 Gmane, e.g., lists them in the correct order now.

The weird interaction is actually a feature and 

Re: [Xenomai-core] [RFC][PATCH 2/3] Switch to handle-based fast mutex owners

2008-08-27 Thread Jan Kiszka
Gilles Chanteperdrix wrote:
 Jan Kiszka wrote:
 Gilles Chanteperdrix wrote:
 Gilles Chanteperdrix wrote:
 Jan Kiszka wrote:
 Gilles Chanteperdrix wrote:
 Jan Kiszka wrote:
 Gilles Chanteperdrix wrote:
 Jan Kiszka wrote:
 Gilles Chanteperdrix wrote:
 Jan Kiszka wrote:
 Gilles Chanteperdrix wrote:
 Jan Kiszka wrote:
 + xnarch_atomic_set(mutex-owner,
 +   set_claimed(xnthread_handle(owner),
 +   
 xnsynch_nsleepers(mutex-synchbase)));
 Ok. I think you have spotted a bug here. This should be 
 mutex-sleepers
 instead of xnsynch_nsleepers.
 BTW, why do you need to track sleepers separately in POSIX? Native
 doesn't do so, e.g.
 Because of the syscall-needed-when-unlocking-stolen-mutex issue I
 already explained (sleepers - xnsynch_nsleepers is precisely the 
 count
 of pending threads which have been awake then robbed the mutex).
 Hmm, sounds like the new lock owner should better clear the 'claimed'
 bit then, not the old one on return from unlock. Or where is the
 pitfall? How does the futex algorithm handle this scenario?
 Ok. Please read my explanation again, I have already explained this in
 another mail.
 I did this, but I'm unable to derive the answer for my question from it.
 Let's go through it in more details:

 When we pass a mutex to a new owner, we set its reference in the fast
 lock variable + set the claimed bit if there are more waiters. Instead,
 I would simple set that bit if there is a new owner. That owner will
 then pick up the mutex eventually and clear 'claimed' on exit from it
 lock service (if there are no further waiters then). If the new owner is
 not able to run and we steal the lock, we simple keep the 'claimed' bit
 as is. On exit from the stolen lock we find it set, thus we are forced
 to issue a syscall as it should be.

 OK, what happens if some waiter wants to leave the party while we are
 holding the stolen lock? Then the sleeper number must be correct - that
 is one pitfall!

 I will have to dig into this more deeply, considering more cases. But
 the additional sleepers field remains at least misplaced IMHO.
 xnsynch_sleepers should better be fixed to respect lock stealing, as
 lock stealing is an xnsynch property, nothing POSIX-specific.
 Ok. I have read this but did not get what you mean. I will read it again
  quietly from home.
 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.
 By the way, I think we should stop sending mails to our personal
 addresses in addition to the mailing list, because this results in
 mailing list mails being received out of orders, which make the threads
 hard to follow.
 That's common practice on most mailing lists I know of, and I personally
 don't want to change this. IMO, it would only make replying more
 complicated, and it would bear the risk to drop CCs to non-subscribers.

 I think the problem was only temporarily, maybe caused by some weird
 interaction of gna.org and the Siemens mailserver (we were too fast for
 them). Meanwhile, archives and inboxes should contain all messages.
 Gmane, e.g., lists them in the correct order now.
 
 The weird 

Re: [Xenomai-core] [RFC][PATCH 2/3] Switch to handle-based fast mutex owners

2008-08-27 Thread Gilles Chanteperdrix
Jan Kiszka wrote:
 Gilles Chanteperdrix wrote:
 Jan Kiszka wrote:
 Gilles Chanteperdrix wrote:
 Gilles Chanteperdrix wrote:
 Jan Kiszka wrote:
 Gilles Chanteperdrix wrote:
 Jan Kiszka wrote:
 Gilles Chanteperdrix wrote:
 Jan Kiszka wrote:
 Gilles Chanteperdrix wrote:
 Jan Kiszka wrote:
 Gilles Chanteperdrix wrote:
 Jan Kiszka wrote:
 +xnarch_atomic_set(mutex-owner,
 +  set_claimed(xnthread_handle(owner),
 +  
 xnsynch_nsleepers(mutex-synchbase)));
 Ok. I think you have spotted a bug here. This should be 
 mutex-sleepers
 instead of xnsynch_nsleepers.
 BTW, why do you need to track sleepers separately in POSIX? Native
 doesn't do so, e.g.
 Because of the syscall-needed-when-unlocking-stolen-mutex issue I
 already explained (sleepers - xnsynch_nsleepers is precisely the 
 count
 of pending threads which have been awake then robbed the mutex).
 Hmm, sounds like the new lock owner should better clear the 'claimed'
 bit then, not the old one on return from unlock. Or where is the
 pitfall? How does the futex algorithm handle this scenario?
 Ok. Please read my explanation again, I have already explained this in
 another mail.
 I did this, but I'm unable to derive the answer for my question from 
 it.
 Let's go through it in more details:

 When we pass a mutex to a new owner, we set its reference in the fast
 lock variable + set the claimed bit if there are more waiters. Instead,
 I would simple set that bit if there is a new owner. That owner will
 then pick up the mutex eventually and clear 'claimed' on exit from it
 lock service (if there are no further waiters then). If the new owner 
 is
 not able to run and we steal the lock, we simple keep the 'claimed' bit
 as is. On exit from the stolen lock we find it set, thus we are forced
 to issue a syscall as it should be.

 OK, what happens if some waiter wants to leave the party while we are
 holding the stolen lock? Then the sleeper number must be correct - that
 is one pitfall!

 I will have to dig into this more deeply, considering more cases. But
 the additional sleepers field remains at least misplaced IMHO.
 xnsynch_sleepers should better be fixed to respect lock stealing, as
 lock stealing is an xnsynch property, nothing POSIX-specific.
 Ok. I have read this but did not get what you mean. I will read it again
  quietly from home.
 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.
 By the way, I think we should stop sending mails to our personal
 addresses in addition to the mailing list, because this results in
 mailing list mails being received out of orders, which make the threads
 hard to follow.
 That's common practice on most mailing lists I know of, and I personally
 don't want to change this. IMO, it would only make replying more
 complicated, and it would bear the risk to drop CCs to non-subscribers.

 I think the problem was only temporarily, maybe caused by some weird
 interaction of gna.org and the Siemens mailserver (we were too fast for
 them). Meanwhile, archives and inboxes should contain all messages.
 Gmane, e.g., lists them in 

Re: [Xenomai-core] [RFC][PATCH 2/3] Switch to handle-based fast mutex owners

2008-08-27 Thread Gilles Chanteperdrix
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.

-- 
Gilles.

___
Xenomai-core mailing list
Xenomai-core@gna.org
https://mail.gna.org/listinfo/xenomai-core


Re: [Xenomai-core] [RFC][PATCH 2/3] Switch to handle-based fast mutex owners

2008-08-27 Thread Gilles Chanteperdrix
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.
 
 Jan
 
 ---
  ksrc/skins/posix/mutex.c |   10 +++---
  ksrc/skins/posix/mutex.h |   17 +++--
  2 files changed, 18 insertions(+), 9 deletions(-)
 
 Index: b/ksrc/skins/posix/mutex.c
 ===
 --- a/ksrc/skins/posix/mutex.c
 +++ b/ksrc/skins/posix/mutex.c
 @@ -117,7 +117,6 @@ int pse51_mutex_init_internal(struct __s
   mutex-attr = *attr;
   mutex-owner = ownerp;
   mutex-owningq = kq;
 - mutex-sleepers = 0;
   xnarch_atomic_set(ownerp, XN_NO_HANDLE);
  
   xnlock_get_irqsave(nklock, s);
 @@ -305,14 +304,12 @@ int pse51_mutex_timedlock_break(struct _
   /* Attempting to relock a normal mutex, deadlock. */
   xnlock_get_irqsave(nklock, s);
   for (;;) {
 - ++mutex-sleepers;
   if (timed)
   xnsynch_sleep_on(mutex-synchbase,
abs_to, XN_REALTIME);
   else
   xnsynch_sleep_on(mutex-synchbase,
XN_INFINITE, XN_RELATIVE);
 - --mutex-sleepers;
  
   if (xnthread_test_info(cur, XNBREAK)) {
   err = -EINTR;
 @@ -329,6 +326,13 @@ int pse51_mutex_timedlock_break(struct _
   break;
   }
   }
 + if (!xnsynch_nsleepers(mutex-synchbase)) {
 + xnarch_atomic_set
 + (mutex-owner,
 +  clear_claimed
 + (xnarch_atomic_get(mutex-owner)));
 + xnsynch_set_owner(mutex-synchbase, NULL);
 + }

I think an atomic_cmpxchg would be better here.

-- 
Gilles.

___
Xenomai-core mailing list
Xenomai-core@gna.org
https://mail.gna.org/listinfo/xenomai-core


Re: [Xenomai-core] [RFC][PATCH 2/3] Switch to handle-based fast mutex owners

2008-08-27 Thread Gilles Chanteperdrix
Jan Kiszka wrote:
 @@ -329,6 +326,13 @@ int pse51_mutex_timedlock_break(struct _
   break;
   }
   }
 + if (!xnsynch_nsleepers(mutex-synchbase)) {
 + xnarch_atomic_set
 + (mutex-owner,
 +  clear_claimed
 + (xnarch_atomic_get(mutex-owner)));
 + xnsynch_set_owner(mutex-synchbase, NULL);
 + }
   xnlock_put_irqrestore(nklock, s);

I do not like this at all. I mean, unless I am mistaken, we loose more
than we gain, we are adding a couple of atomic, hence heavy, operations
in a common case for handling a corner case. I still prefer emitting a
system call in the corner case.

-- 
Gilles.

___
Xenomai-core mailing list
Xenomai-core@gna.org
https://mail.gna.org/listinfo/xenomai-core


Re: [Xenomai-core] [RFC][PATCH 2/3] Switch to handle-based fast mutex owners

2008-08-27 Thread Gilles Chanteperdrix
Jan Kiszka wrote:
 Gilles Chanteperdrix wrote:
 Jan Kiszka wrote:
 Gilles Chanteperdrix wrote:
 Jan Kiszka wrote:
 Gilles Chanteperdrix wrote:
 Jan Kiszka wrote:
 File descriptors are all identically structured objects, so at worst you
 ruin some other app's day. But the registry contains arbitrary objects
 with different internal layout. If you start assuming object_a * is
 object_b * and use the pointer etc. included in a as if they have the
 meaning of b, you quickly ruin the kernel's day as well. Therefore,
 native, e.g., does magic checks after fetching from the registry. As I
 said, this test here works differently, but it has the same effect and
 impact.
 By the way, would not it make sense to have separate hash tables for
 separate objects types ? I mean then we would not need any validation,
 and several object types could use the same name.
 From that POV a good idea. The only issue I see is a management problem:
 How many mutex, thread, queue, whatever slots do you want in your
 system? One knob for them all? Or countless knobs for all object types
 of all skins? That's hairy, I'm afraid.
 xnmalloc, the pool size is the limit.
 You mean kind of xnrealloc, including atomically copying potentially
 large descriptor tables over? Sounds not that attractive.
 No, I mean the hash table contains pointers, and the objects are
 allocated dynamically...
 
 Handle lookup is not going through hash tables. It's index based (like
 file descriptors).

Ok, but using hash tables instead of static tables allow you to have
more objects in the hash than hash buckets, and a registry slot
occupies simply a bit in a multi-level bitfield when free, so you can
have preposterous registry slots limits like 2048 or 4096.

-- 
Gilles.

___
Xenomai-core mailing list
Xenomai-core@gna.org
https://mail.gna.org/listinfo/xenomai-core


Re: [Xenomai-core] [RFC][PATCH 2/3] Switch to handle-based fast mutex owners

2008-08-27 Thread Gilles Chanteperdrix
Jan Kiszka wrote:
 Gilles Chanteperdrix wrote:
 Jan Kiszka wrote:
 Gilles Chanteperdrix wrote:
 Jan Kiszka wrote:
 To improve robustness of the fast mutex implementation in POSIX (and
 later on in native), it is better to track the mutex owner by handle
 instead of kernel object pointer. Therefore, this patch changes
 __xn_sys_current (xeno_set_current) so that it returns
 xnthread_handle(current_thread). It furthermore converts the POSIX mutex
 implementation to pick up and store the lock owner as handle in the
 kernel/user-shared mutex. Finally it ensures that at least POSIX threads
 always have an (anonymous) handle assigned.

 As the value stored in the mutex variable is now an integer, we can
 switch over to xnarch_atomic_t, removing all atomic_intptr users.
 Ok. I do not know if this should be part of this patch, or in another
 one, but we should call xeno_set_current in the trampolines of all
 skins, so that they can use the native and posix mutexes.

 This is another thing that I have left in a state of flux...
 Find it below (PATCH 4/3).

 BTW, should we better invoke pthread_exit in xeno_set_current in case of 
 a failure?
 I prefer my application to die. This way I would know that something
 went wrong. And by the way, how could this syscall fail ? I mean if
 xenomai or the posix skin are not loaded, we would have known it earlier.
 
 So far this syscall can trigger a thread handle registration if the
 current thread has no handle yet. But that could go away if all skins
 ensure that there is at least an anonymous handle for their threads
 after creation.

Yes please rework that and do the registration at thread creation in
kernel-space. Since the system call is not called for kernel-space
threads, the on-demand registry registration would not work for them,
unless no, I can not imagine that.

-- 
Gilles.

___
Xenomai-core mailing list
Xenomai-core@gna.org
https://mail.gna.org/listinfo/xenomai-core