* [Xenomai] Rescnt imbalance in rtdm_mutex_timedlock
@ 2014-03-25 11:45 Erwin Pranz
2014-03-26 16:49 ` Henri Roosen
0 siblings, 1 reply; 16+ messages in thread
From: Erwin Pranz @ 2014-03-25 11:45 UTC (permalink / raw)
To: xenomai
Hello,
I am using rtdm_mutex_lock/rtdm_mutex_unlock in a rtdm driver and I get
a signal SIGDEBUG_MIGRATE_PRIOINV when I call the driver from a
priority-0 thread.The signal is sent in xnsynch_release_thread when
rescnt is 0 on a XNOTHER thread, which means that rescnt is not balanced.
I think the reason why rescnt is not balanced is in
rtdm_mutex_timedlock: when the owner of the mutex is NULL, the owner is
changed to the calling thread, but rescnt is not increased.
I suggest the following fix in rtdm_mutex_timedlock:
- else if (likely(xnsynch_owner(&mutex->synch_base) == NULL))
+ else if (likely(xnsynch_owner(&mutex->synch_base) == NULL)) {
xnsynch_set_owner(&mutex->synch_base, curr_thread);
+ if (xnthread_test_state(curr_thread, XNOTHER))
+ xnthread_inc_rescnt(curr_thread);
+ }
Best regards,
--
Erwin Pranz
Software
________________________________
SIGMATEK GmbH & Co KG
Sigmatekstraße 1
5112 Lamprechtshausen Österreich / Austria
Tel.: +43/6274/4321-0
Fax: +43/6274/4321-18
E-Mail: erwin.pranz@sigmatek.at
http://www.sigmatek-automation.com
^ permalink raw reply [flat|nested] 16+ messages in thread* Re: [Xenomai] Rescnt imbalance in rtdm_mutex_timedlock 2014-03-25 11:45 [Xenomai] Rescnt imbalance in rtdm_mutex_timedlock Erwin Pranz @ 2014-03-26 16:49 ` Henri Roosen 2014-03-26 23:07 ` Gilles Chanteperdrix 0 siblings, 1 reply; 16+ messages in thread From: Henri Roosen @ 2014-03-26 16:49 UTC (permalink / raw) To: erwin.pranz; +Cc: Xenomai@xenomai.org On Tue, Mar 25, 2014 at 12:45 PM, Erwin Pranz <erwin.pranz@sigmatek.at>wrote: > Hello, > > I am using rtdm_mutex_lock/rtdm_mutex_unlock in a rtdm driver and I get a > signal SIGDEBUG_MIGRATE_PRIOINV when I call the driver from a priority-0 > thread.The signal is sent in xnsynch_release_thread when rescnt is 0 on a > XNOTHER thread, which means that rescnt is not balanced. > > I think the reason why rescnt is not balanced is in rtdm_mutex_timedlock: > when the owner of the mutex is NULL, the owner is changed to the calling > thread, but rescnt is not increased. > > I suggest the following fix in rtdm_mutex_timedlock: > - else if (likely(xnsynch_owner(&mutex->synch_base) == NULL)) > + else if (likely(xnsynch_owner(&mutex->synch_base) == NULL)) { > xnsynch_set_owner(&mutex->synch_base, curr_thread); > + if (xnthread_test_state(curr_thread, XNOTHER)) > + xnthread_inc_rescnt(curr_thread); > + } > > Hi Gilles, Could you maybe elaborate on this? It seems the resource counting is not in balance. It would be great to know if this the correct way to fix it. The problem should be easily reproducible by having just one shadowed priority 0 thread that somehow calls rtdm_mutex_lock directly followed by rtdm_mutex_unlock. This will trigger the code in ksrc/nucleus/synch.c that was added by commit-486afd15, which sends SIGDEBUG_MIGRATE_PRIOINV on Xenomai v2.6.2.1. It shouldn't send any signal in that case, as nothing is wrong. Note we are on Xenomai v2.6.2.1. Xenomai v2.6.3 sends (the better name for it) SIGDEBUG_RESCNT_IMBALANCE. Thanks for your help, Henri > Best regards, > > -- > Erwin Pranz > Software > ________________________________ > > SIGMATEK GmbH & Co KG > Sigmatekstraße 1 > 5112 Lamprechtshausen Österreich / Austria > > Tel.: +43/6274/4321-0 > Fax: +43/6274/4321-18 > E-Mail: erwin.pranz@sigmatek.at > http://www.sigmatek-automation.com > > > _______________________________________________ > Xenomai mailing list > Xenomai@xenomai.org > http://www.xenomai.org/mailman/listinfo/xenomai > ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [Xenomai] Rescnt imbalance in rtdm_mutex_timedlock 2014-03-26 16:49 ` Henri Roosen @ 2014-03-26 23:07 ` Gilles Chanteperdrix 2014-03-27 9:41 ` Philippe Gerum 0 siblings, 1 reply; 16+ messages in thread From: Gilles Chanteperdrix @ 2014-03-26 23:07 UTC (permalink / raw) To: Henri Roosen; +Cc: Xenomai@xenomai.org On 03/26/2014 05:49 PM, Henri Roosen wrote: > On Tue, Mar 25, 2014 at 12:45 PM, Erwin Pranz <erwin.pranz@sigmatek.at>wrote: > >> Hello, >> >> I am using rtdm_mutex_lock/rtdm_mutex_unlock in a rtdm driver and I get a >> signal SIGDEBUG_MIGRATE_PRIOINV when I call the driver from a priority-0 >> thread.The signal is sent in xnsynch_release_thread when rescnt is 0 on a >> XNOTHER thread, which means that rescnt is not balanced. >> >> I think the reason why rescnt is not balanced is in rtdm_mutex_timedlock: >> when the owner of the mutex is NULL, the owner is changed to the calling >> thread, but rescnt is not increased. >> >> I suggest the following fix in rtdm_mutex_timedlock: >> - else if (likely(xnsynch_owner(&mutex->synch_base) == NULL)) >> + else if (likely(xnsynch_owner(&mutex->synch_base) == NULL)) { >> xnsynch_set_owner(&mutex->synch_base, curr_thread); >> + if (xnthread_test_state(curr_thread, XNOTHER)) >> + xnthread_inc_rescnt(curr_thread); >> + } >> >> > Hi Gilles, > > Could you maybe elaborate on this? It seems the resource counting is not in > balance. It would be great to know if this the correct way to fix it. > > The problem should be easily reproducible by having just one shadowed > priority 0 thread that somehow calls rtdm_mutex_lock directly followed by > rtdm_mutex_unlock. This will trigger the code in ksrc/nucleus/synch.c that > was added by commit-486afd15, which sends SIGDEBUG_MIGRATE_PRIOINV on > Xenomai v2.6.2.1. It shouldn't send any signal in that case, as nothing is > wrong. > > Note we are on Xenomai v2.6.2.1. Xenomai v2.6.3 sends (the better name for > it) SIGDEBUG_RESCNT_IMBALANCE. To be completely frank, I expected Jan to answer this mail as this concerns RTDM. Anyway, I had a look into it, and: - the RTDM skin is not alone in this case, several skins implement the same pattern (if owner == NULL, set owner, then return); - adding calls to xnthread_inc_rescnt everywhere does not seem right, I would prefer to move this inside some xnsynch_ service. The best candidate I found for that is xnsynch_set_owner, though I am not sure if this will work, especially with fastsynchs. -- Gilles. ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [Xenomai] Rescnt imbalance in rtdm_mutex_timedlock 2014-03-26 23:07 ` Gilles Chanteperdrix @ 2014-03-27 9:41 ` Philippe Gerum 2014-03-27 15:02 ` Henri Roosen 0 siblings, 1 reply; 16+ messages in thread From: Philippe Gerum @ 2014-03-27 9:41 UTC (permalink / raw) To: Gilles Chanteperdrix, Henri Roosen; +Cc: Xenomai@xenomai.org On 03/27/2014 12:07 AM, Gilles Chanteperdrix wrote: > On 03/26/2014 05:49 PM, Henri Roosen wrote: >> On Tue, Mar 25, 2014 at 12:45 PM, Erwin Pranz <erwin.pranz@sigmatek.at>wrote: >> >>> Hello, >>> >>> I am using rtdm_mutex_lock/rtdm_mutex_unlock in a rtdm driver and I get a >>> signal SIGDEBUG_MIGRATE_PRIOINV when I call the driver from a priority-0 >>> thread.The signal is sent in xnsynch_release_thread when rescnt is 0 on a >>> XNOTHER thread, which means that rescnt is not balanced. >>> >>> I think the reason why rescnt is not balanced is in rtdm_mutex_timedlock: >>> when the owner of the mutex is NULL, the owner is changed to the calling >>> thread, but rescnt is not increased. >>> >>> I suggest the following fix in rtdm_mutex_timedlock: >>> - else if (likely(xnsynch_owner(&mutex->synch_base) == NULL)) >>> + else if (likely(xnsynch_owner(&mutex->synch_base) == NULL)) { >>> xnsynch_set_owner(&mutex->synch_base, curr_thread); >>> + if (xnthread_test_state(curr_thread, XNOTHER)) >>> + xnthread_inc_rescnt(curr_thread); >>> + } >>> >>> >> Hi Gilles, >> >> Could you maybe elaborate on this? It seems the resource counting is not in >> balance. It would be great to know if this the correct way to fix it. >> >> The problem should be easily reproducible by having just one shadowed >> priority 0 thread that somehow calls rtdm_mutex_lock directly followed by >> rtdm_mutex_unlock. This will trigger the code in ksrc/nucleus/synch.c that >> was added by commit-486afd15, which sends SIGDEBUG_MIGRATE_PRIOINV on >> Xenomai v2.6.2.1. It shouldn't send any signal in that case, as nothing is >> wrong. >> >> Note we are on Xenomai v2.6.2.1. Xenomai v2.6.3 sends (the better name for >> it) SIGDEBUG_RESCNT_IMBALANCE. > > To be completely frank, I expected Jan to answer this mail as this > concerns RTDM. Anyway, I had a look into it, and: > - the RTDM skin is not alone in this case, several skins implement the > same pattern (if owner == NULL, set owner, then return); > - adding calls to xnthread_inc_rescnt everywhere does not seem right, I > would prefer to move this inside some xnsynch_ service. The best > candidate I found for that is xnsynch_set_owner, though I am not sure if > this will work, especially with fastsynchs. > > Indeed this wouldn't. I suspect the skins mentioned did not have access to xnsynch_acquire(), and were not updated when the auto-relax feature was introduced for XNOTHER threads. We could provide a helper for fast grabbing a mutex which has no fast locking support shared with userland, saving a call to xnsynch_acquire() in the non-contended case, e.g. diff --git a/include/nucleus/synch.h b/include/nucleus/synch.h index 7deae21..87456ea 100644 --- a/include/nucleus/synch.h +++ b/include/nucleus/synch.h @@ -192,6 +192,22 @@ static inline void xnsynch_set_owner(struct xnsynch *synch, synch->owner = thread; } +static inline int xnsynch_fast_grab(struct xnsynch *synch, + struct xnthread *thread) +{ + XENO_BUGON(NUCLEUS, xnsynch_fastlock_p(synch)); + + if (likely(xnsynch_owner(synch) != NULL)) + return 0; + + xnsynch_set_owner(synch, thread); + + if (xnthread_test_state(thread, XNOTHER)) + xnthread_inc_rescnt(thread); + + return 1; +} + static inline void xnsynch_register_cleanup(struct xnsynch *synch, void (*handler)(struct xnsynch *)) { diff --git a/ksrc/skins/rtdm/drvlib.c b/ksrc/skins/rtdm/drvlib.c index 391d45f..338005d 100644 --- a/ksrc/skins/rtdm/drvlib.c +++ b/ksrc/skins/rtdm/drvlib.c @@ -1538,9 +1538,7 @@ int rtdm_mutex_timedlock(rtdm_mutex_t *mutex, nanosecs_rel_t timeout, if (unlikely(xnsynch_test_flags(&mutex->synch_base, RTDM_SYNCH_DELETED))) err = -EIDRM; - else if (likely(xnsynch_owner(&mutex->synch_base) == NULL)) - xnsynch_set_owner(&mutex->synch_base, curr_thread); - else { + else if (!xnsynch_fast_grab(&mutex->synch_base, curr_thread)) { /* Redefinition to clarify XENO_ASSERT output */ #define mutex_owner xnsynch_owner(&mutex->synch_base) XENO_ASSERT(RTDM, mutex_owner != curr_thread, The VxWorks and VRTX skins would have to use it as well. -- Philippe. ^ permalink raw reply related [flat|nested] 16+ messages in thread
* Re: [Xenomai] Rescnt imbalance in rtdm_mutex_timedlock 2014-03-27 9:41 ` Philippe Gerum @ 2014-03-27 15:02 ` Henri Roosen 2014-03-27 16:26 ` Philippe Gerum 0 siblings, 1 reply; 16+ messages in thread From: Henri Roosen @ 2014-03-27 15:02 UTC (permalink / raw) To: Philippe Gerum; +Cc: Xenomai@xenomai.org On Thu, Mar 27, 2014 at 10:41 AM, Philippe Gerum <rpm@xenomai.org> wrote: > On 03/27/2014 12:07 AM, Gilles Chanteperdrix wrote: > >> On 03/26/2014 05:49 PM, Henri Roosen wrote: >> >>> On Tue, Mar 25, 2014 at 12:45 PM, Erwin Pranz <erwin.pranz@sigmatek.at> >>> wrote: >>> >>> Hello, >>>> >>>> I am using rtdm_mutex_lock/rtdm_mutex_unlock in a rtdm driver and I >>>> get a >>>> signal SIGDEBUG_MIGRATE_PRIOINV when I call the driver from a priority-0 >>>> thread.The signal is sent in xnsynch_release_thread when rescnt is 0 on >>>> a >>>> XNOTHER thread, which means that rescnt is not balanced. >>>> >>>> I think the reason why rescnt is not balanced is in >>>> rtdm_mutex_timedlock: >>>> when the owner of the mutex is NULL, the owner is changed to the calling >>>> thread, but rescnt is not increased. >>>> >>>> I suggest the following fix in rtdm_mutex_timedlock: >>>> - else if (likely(xnsynch_owner(&mutex->synch_base) == NULL)) >>>> + else if (likely(xnsynch_owner(&mutex->synch_base) == NULL)) { >>>> xnsynch_set_owner(&mutex->synch_base, curr_thread); >>>> + if (xnthread_test_state(curr_thread, XNOTHER)) >>>> + xnthread_inc_rescnt(curr_thread); >>>> + } >>>> >>>> >>>> Hi Gilles, >>> >>> Could you maybe elaborate on this? It seems the resource counting is not >>> in >>> balance. It would be great to know if this the correct way to fix it. >>> >>> The problem should be easily reproducible by having just one shadowed >>> priority 0 thread that somehow calls rtdm_mutex_lock directly followed by >>> rtdm_mutex_unlock. This will trigger the code in ksrc/nucleus/synch.c >>> that >>> was added by commit-486afd15, which sends SIGDEBUG_MIGRATE_PRIOINV on >>> Xenomai v2.6.2.1. It shouldn't send any signal in that case, as nothing >>> is >>> wrong. >>> >>> Note we are on Xenomai v2.6.2.1. Xenomai v2.6.3 sends (the better name >>> for >>> it) SIGDEBUG_RESCNT_IMBALANCE. >>> >> >> To be completely frank, I expected Jan to answer this mail as this >> concerns RTDM. Anyway, I had a look into it, and: >> - the RTDM skin is not alone in this case, several skins implement the >> same pattern (if owner == NULL, set owner, then return); >> - adding calls to xnthread_inc_rescnt everywhere does not seem right, I >> would prefer to move this inside some xnsynch_ service. The best >> candidate I found for that is xnsynch_set_owner, though I am not sure if >> this will work, especially with fastsynchs. >> >> >> > Indeed this wouldn't. I suspect the skins mentioned did not have access to > xnsynch_acquire(), and were not updated when the auto-relax feature was > introduced for XNOTHER threads. > > We could provide a helper for fast grabbing a mutex which has no fast > locking support shared with userland, saving a call to xnsynch_acquire() in > the non-contended case, e.g. > > diff --git a/include/nucleus/synch.h b/include/nucleus/synch.h > index 7deae21..87456ea 100644 > --- a/include/nucleus/synch.h > +++ b/include/nucleus/synch.h > @@ -192,6 +192,22 @@ static inline void xnsynch_set_owner(struct xnsynch > *synch, > synch->owner = thread; > } > > +static inline int xnsynch_fast_grab(struct xnsynch *synch, > + struct xnthread *thread) > +{ > + XENO_BUGON(NUCLEUS, xnsynch_fastlock_p(synch)); > + > + if (likely(xnsynch_owner(synch) != NULL)) > + return 0; > + > + xnsynch_set_owner(synch, thread); > + > + if (xnthread_test_state(thread, XNOTHER)) > + xnthread_inc_rescnt(thread); > + > + return 1; > +} > + > static inline void xnsynch_register_cleanup(struct xnsynch *synch, > void (*handler)(struct xnsynch > *)) > { > diff --git a/ksrc/skins/rtdm/drvlib.c b/ksrc/skins/rtdm/drvlib.c > index 391d45f..338005d 100644 > --- a/ksrc/skins/rtdm/drvlib.c > +++ b/ksrc/skins/rtdm/drvlib.c > @@ -1538,9 +1538,7 @@ int rtdm_mutex_timedlock(rtdm_mutex_t *mutex, > nanosecs_rel_t timeout, > if (unlikely(xnsynch_test_flags(&mutex->synch_base, > RTDM_SYNCH_DELETED))) > err = -EIDRM; > > - else if (likely(xnsynch_owner(&mutex->synch_base) == NULL)) > - xnsynch_set_owner(&mutex->synch_base, curr_thread); > - else { > + else if (!xnsynch_fast_grab(&mutex->synch_base, curr_thread)) { > /* Redefinition to clarify XENO_ASSERT output */ > #define mutex_owner xnsynch_owner(&mutex->synch_base) > XENO_ASSERT(RTDM, mutex_owner != curr_thread, > > The VxWorks and VRTX skins would have to use it as well. > > Thanks for the replies! We'll give this patch a try for the RTDM case. Thanks, Henri > -- > Philippe. > ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [Xenomai] Rescnt imbalance in rtdm_mutex_timedlock 2014-03-27 15:02 ` Henri Roosen @ 2014-03-27 16:26 ` Philippe Gerum 2014-03-27 18:04 ` Jan Kiszka 2014-03-28 9:52 ` Erwin Pranz 0 siblings, 2 replies; 16+ messages in thread From: Philippe Gerum @ 2014-03-27 16:26 UTC (permalink / raw) To: Henri Roosen; +Cc: Xenomai@xenomai.org On 03/27/2014 04:02 PM, Henri Roosen wrote: > > > > On Thu, Mar 27, 2014 at 10:41 AM, Philippe Gerum <rpm@xenomai.org > <mailto:rpm@xenomai.org>> wrote: > > On 03/27/2014 12:07 AM, Gilles Chanteperdrix wrote: > > On 03/26/2014 05:49 PM, Henri Roosen wrote: > > On Tue, Mar 25, 2014 at 12:45 PM, Erwin Pranz > <erwin.pranz@sigmatek.at > <mailto:erwin.pranz@sigmatek.at>>__wrote: > > Hello, > > I am using rtdm_mutex_lock/rtdm_mutex___unlock in a rtdm > driver and I get a > signal SIGDEBUG_MIGRATE_PRIOINV when I call the driver > from a priority-0 > thread.The signal is sent in xnsynch_release_thread when > rescnt is 0 on a > XNOTHER thread, which means that rescnt is not balanced. > > I think the reason why rescnt is not balanced is in > rtdm_mutex_timedlock: > when the owner of the mutex is NULL, the owner is > changed to the calling > thread, but rescnt is not increased. > > I suggest the following fix in rtdm_mutex_timedlock: > - else if > (likely(xnsynch_owner(&mutex->__synch_base) == NULL)) > + else if > (likely(xnsynch_owner(&mutex->__synch_base) == NULL)) { > > xnsynch_set_owner(&mutex->__synch_base, curr_thread); > + if (xnthread_test_state(curr___thread, > XNOTHER)) > + xnthread_inc_rescnt(curr___thread); > + } > > > Hi Gilles, > > Could you maybe elaborate on this? It seems the resource > counting is not in > balance. It would be great to know if this the correct way > to fix it. > > The problem should be easily reproducible by having just one > shadowed > priority 0 thread that somehow calls rtdm_mutex_lock > directly followed by > rtdm_mutex_unlock. This will trigger the code in > ksrc/nucleus/synch.c that > was added by commit-486afd15, which sends > SIGDEBUG_MIGRATE_PRIOINV on > Xenomai v2.6.2.1. It shouldn't send any signal in that case, > as nothing is > wrong. > > Note we are on Xenomai v2.6.2.1. Xenomai v2.6.3 sends (the > better name for > it) SIGDEBUG_RESCNT_IMBALANCE. > > > To be completely frank, I expected Jan to answer this mail as this > concerns RTDM. Anyway, I had a look into it, and: > - the RTDM skin is not alone in this case, several skins > implement the > same pattern (if owner == NULL, set owner, then return); > - adding calls to xnthread_inc_rescnt everywhere does not seem > right, I > would prefer to move this inside some xnsynch_ service. The best > candidate I found for that is xnsynch_set_owner, though I am not > sure if > this will work, especially with fastsynchs. > > > > Indeed this wouldn't. I suspect the skins mentioned did not have > access to xnsynch_acquire(), and were not updated when the > auto-relax feature was introduced for XNOTHER threads. > > We could provide a helper for fast grabbing a mutex which has no > fast locking support shared with userland, saving a call to > xnsynch_acquire() in the non-contended case, e.g. > > diff --git a/include/nucleus/synch.h b/include/nucleus/synch.h > index 7deae21..87456ea 100644 > --- a/include/nucleus/synch.h > +++ b/include/nucleus/synch.h > @@ -192,6 +192,22 @@ static inline void xnsynch_set_owner(struct > xnsynch *synch, > synch->owner = thread; > } > > +static inline int xnsynch_fast_grab(struct xnsynch *synch, > + struct xnthread *thread) > +{ > + XENO_BUGON(NUCLEUS, xnsynch_fastlock_p(synch)); > + > + if (likely(xnsynch_owner(synch) != NULL)) > + return 0; > + > + xnsynch_set_owner(synch, thread); > + > + if (xnthread_test_state(thread, XNOTHER)) > + xnthread_inc_rescnt(thread); > + > + return 1; > +} > + > static inline void xnsynch_register_cleanup(__struct xnsynch *synch, > void (*handler)(struct > xnsynch *)) > { > diff --git a/ksrc/skins/rtdm/drvlib.c b/ksrc/skins/rtdm/drvlib.c > index 391d45f..338005d 100644 > --- a/ksrc/skins/rtdm/drvlib.c > +++ b/ksrc/skins/rtdm/drvlib.c > @@ -1538,9 +1538,7 @@ int rtdm_mutex_timedlock(rtdm___mutex_t > *mutex, nanosecs_rel_t timeout, > if (unlikely(xnsynch_test_flags(&__mutex->synch_base, > RTDM_SYNCH_DELETED))) > err = -EIDRM; > > - else if (likely(xnsynch_owner(&mutex->__synch_base) == NULL)) > - xnsynch_set_owner(&mutex->__synch_base, curr_thread); > - else { > + else if (!xnsynch_fast_grab(&mutex->__synch_base, > curr_thread)) { > /* Redefinition to clarify XENO_ASSERT output */ > #define mutex_owner xnsynch_owner(&mutex->synch___base) > XENO_ASSERT(RTDM, mutex_owner != curr_thread, > > The VxWorks and VRTX skins would have to use it as well. > > > Thanks for the replies! We'll give this patch a try for the RTDM case. > Minor points, but a compiler hint above was inverted and eventually useless, and the naming not descriptive enough. Take #2: diff --git a/include/nucleus/synch.h b/include/nucleus/synch.h index 7deae21..09ebdbf 100644 --- a/include/nucleus/synch.h +++ b/include/nucleus/synch.h @@ -192,6 +192,22 @@ static inline void xnsynch_set_owner(struct xnsynch *synch, synch->owner = thread; } +static inline int xnsynch_try_grab(struct xnsynch *synch, + struct xnthread *thread) +{ + XENO_BUGON(NUCLEUS, xnsynch_fastlock_p(synch)); + + if (xnsynch_owner(synch) != NULL) + return 0; + + xnsynch_set_owner(synch, thread); + + if (xnthread_test_state(thread, XNOTHER)) + xnthread_inc_rescnt(thread); + + return 1; +} + static inline void xnsynch_register_cleanup(struct xnsynch *synch, void (*handler)(struct xnsynch *)) { diff --git a/ksrc/skins/rtdm/drvlib.c b/ksrc/skins/rtdm/drvlib.c index 391d45f..aa85d11 100644 --- a/ksrc/skins/rtdm/drvlib.c +++ b/ksrc/skins/rtdm/drvlib.c @@ -1538,9 +1538,7 @@ int rtdm_mutex_timedlock(rtdm_mutex_t *mutex, nanosecs_rel_t timeout, if (unlikely(xnsynch_test_flags(&mutex->synch_base, RTDM_SYNCH_DELETED))) err = -EIDRM; - else if (likely(xnsynch_owner(&mutex->synch_base) == NULL)) - xnsynch_set_owner(&mutex->synch_base, curr_thread); - else { + else if (!xnsynch_try_grab(&mutex->synch_base, curr_thread)) { /* Redefinition to clarify XENO_ASSERT output */ #define mutex_owner xnsynch_owner(&mutex->synch_base) XENO_ASSERT(RTDM, mutex_owner != curr_thread, -- Philippe. ^ permalink raw reply related [flat|nested] 16+ messages in thread
* Re: [Xenomai] Rescnt imbalance in rtdm_mutex_timedlock 2014-03-27 16:26 ` Philippe Gerum @ 2014-03-27 18:04 ` Jan Kiszka 2014-03-27 18:24 ` Philippe Gerum 2014-03-28 9:52 ` Erwin Pranz 1 sibling, 1 reply; 16+ messages in thread From: Jan Kiszka @ 2014-03-27 18:04 UTC (permalink / raw) To: Philippe Gerum, Henri Roosen; +Cc: Xenomai@xenomai.org On 2014-03-27 17:26, Philippe Gerum wrote: > On 03/27/2014 04:02 PM, Henri Roosen wrote: >> >> >> >> On Thu, Mar 27, 2014 at 10:41 AM, Philippe Gerum <rpm@xenomai.org >> <mailto:rpm@xenomai.org>> wrote: >> >> On 03/27/2014 12:07 AM, Gilles Chanteperdrix wrote: >> >> On 03/26/2014 05:49 PM, Henri Roosen wrote: >> >> On Tue, Mar 25, 2014 at 12:45 PM, Erwin Pranz >> <erwin.pranz@sigmatek.at >> <mailto:erwin.pranz@sigmatek.at>>__wrote: >> >> Hello, >> >> I am using rtdm_mutex_lock/rtdm_mutex___unlock in a rtdm >> driver and I get a >> signal SIGDEBUG_MIGRATE_PRIOINV when I call the driver >> from a priority-0 >> thread.The signal is sent in xnsynch_release_thread when >> rescnt is 0 on a >> XNOTHER thread, which means that rescnt is not balanced. >> >> I think the reason why rescnt is not balanced is in >> rtdm_mutex_timedlock: >> when the owner of the mutex is NULL, the owner is >> changed to the calling >> thread, but rescnt is not increased. >> >> I suggest the following fix in rtdm_mutex_timedlock: >> - else if >> (likely(xnsynch_owner(&mutex->__synch_base) == NULL)) >> + else if >> (likely(xnsynch_owner(&mutex->__synch_base) == NULL)) { >> >> xnsynch_set_owner(&mutex->__synch_base, curr_thread); >> + if (xnthread_test_state(curr___thread, >> XNOTHER)) >> + xnthread_inc_rescnt(curr___thread); >> + } >> >> >> Hi Gilles, >> >> Could you maybe elaborate on this? It seems the resource >> counting is not in >> balance. It would be great to know if this the correct way >> to fix it. >> >> The problem should be easily reproducible by having just one >> shadowed >> priority 0 thread that somehow calls rtdm_mutex_lock >> directly followed by >> rtdm_mutex_unlock. This will trigger the code in >> ksrc/nucleus/synch.c that >> was added by commit-486afd15, which sends >> SIGDEBUG_MIGRATE_PRIOINV on >> Xenomai v2.6.2.1. It shouldn't send any signal in that case, >> as nothing is >> wrong. >> >> Note we are on Xenomai v2.6.2.1. Xenomai v2.6.3 sends (the >> better name for >> it) SIGDEBUG_RESCNT_IMBALANCE. >> >> >> To be completely frank, I expected Jan to answer this mail as this >> concerns RTDM. Anyway, I had a look into it, and: >> - the RTDM skin is not alone in this case, several skins >> implement the >> same pattern (if owner == NULL, set owner, then return); >> - adding calls to xnthread_inc_rescnt everywhere does not seem >> right, I >> would prefer to move this inside some xnsynch_ service. The best >> candidate I found for that is xnsynch_set_owner, though I am not >> sure if >> this will work, especially with fastsynchs. >> >> >> >> Indeed this wouldn't. I suspect the skins mentioned did not have >> access to xnsynch_acquire(), and were not updated when the >> auto-relax feature was introduced for XNOTHER threads. >> >> We could provide a helper for fast grabbing a mutex which has no >> fast locking support shared with userland, saving a call to >> xnsynch_acquire() in the non-contended case, e.g. >> >> diff --git a/include/nucleus/synch.h b/include/nucleus/synch.h >> index 7deae21..87456ea 100644 >> --- a/include/nucleus/synch.h >> +++ b/include/nucleus/synch.h >> @@ -192,6 +192,22 @@ static inline void xnsynch_set_owner(struct >> xnsynch *synch, >> synch->owner = thread; >> } >> >> +static inline int xnsynch_fast_grab(struct xnsynch *synch, >> + struct xnthread *thread) >> +{ >> + XENO_BUGON(NUCLEUS, xnsynch_fastlock_p(synch)); >> + >> + if (likely(xnsynch_owner(synch) != NULL)) >> + return 0; >> + >> + xnsynch_set_owner(synch, thread); >> + >> + if (xnthread_test_state(thread, XNOTHER)) >> + xnthread_inc_rescnt(thread); >> + >> + return 1; >> +} >> + >> static inline void xnsynch_register_cleanup(__struct xnsynch *synch, >> void (*handler)(struct >> xnsynch *)) >> { >> diff --git a/ksrc/skins/rtdm/drvlib.c b/ksrc/skins/rtdm/drvlib.c >> index 391d45f..338005d 100644 >> --- a/ksrc/skins/rtdm/drvlib.c >> +++ b/ksrc/skins/rtdm/drvlib.c >> @@ -1538,9 +1538,7 @@ int rtdm_mutex_timedlock(rtdm___mutex_t >> *mutex, nanosecs_rel_t timeout, >> if (unlikely(xnsynch_test_flags(&__mutex->synch_base, >> RTDM_SYNCH_DELETED))) >> err = -EIDRM; >> >> - else if (likely(xnsynch_owner(&mutex->__synch_base) == NULL)) >> - xnsynch_set_owner(&mutex->__synch_base, curr_thread); >> - else { >> + else if (!xnsynch_fast_grab(&mutex->__synch_base, >> curr_thread)) { >> /* Redefinition to clarify XENO_ASSERT output */ >> #define mutex_owner xnsynch_owner(&mutex->synch___base) >> XENO_ASSERT(RTDM, mutex_owner != curr_thread, >> >> The VxWorks and VRTX skins would have to use it as well. >> >> >> Thanks for the replies! We'll give this patch a try for the RTDM case. >> > > Minor points, but a compiler hint above was inverted and eventually useless, and the naming not descriptive enough. Take #2: > > diff --git a/include/nucleus/synch.h b/include/nucleus/synch.h > index 7deae21..09ebdbf 100644 > --- a/include/nucleus/synch.h > +++ b/include/nucleus/synch.h > @@ -192,6 +192,22 @@ static inline void xnsynch_set_owner(struct xnsynch *synch, > synch->owner = thread; > } > > +static inline int xnsynch_try_grab(struct xnsynch *synch, > + struct xnthread *thread) > +{ > + XENO_BUGON(NUCLEUS, xnsynch_fastlock_p(synch)); > + > + if (xnsynch_owner(synch) != NULL) if (unlikely(...)) - locks are generally unlikely to be contended (unless something is suboptimally designed at the user site). > + return 0; > + > + xnsynch_set_owner(synch, thread); > + > + if (xnthread_test_state(thread, XNOTHER)) > + xnthread_inc_rescnt(thread); > + > + return 1; > +} > + > static inline void xnsynch_register_cleanup(struct xnsynch *synch, > void (*handler)(struct xnsynch *)) > { > diff --git a/ksrc/skins/rtdm/drvlib.c b/ksrc/skins/rtdm/drvlib.c > index 391d45f..aa85d11 100644 > --- a/ksrc/skins/rtdm/drvlib.c > +++ b/ksrc/skins/rtdm/drvlib.c > @@ -1538,9 +1538,7 @@ int rtdm_mutex_timedlock(rtdm_mutex_t *mutex, nanosecs_rel_t timeout, > if (unlikely(xnsynch_test_flags(&mutex->synch_base, > RTDM_SYNCH_DELETED))) > err = -EIDRM; > - else if (likely(xnsynch_owner(&mutex->synch_base) == NULL)) > - xnsynch_set_owner(&mutex->synch_base, curr_thread); > - else { > + else if (!xnsynch_try_grab(&mutex->synch_base, curr_thread)) { > /* Redefinition to clarify XENO_ASSERT output */ > #define mutex_owner xnsynch_owner(&mutex->synch_base) > XENO_ASSERT(RTDM, mutex_owner != curr_thread, > Looks good otherwise. Jan -------------- next part -------------- A non-text attachment was scrubbed... Name: signature.asc Type: application/pgp-signature Size: 263 bytes Desc: OpenPGP digital signature URL: <http://www.xenomai.org/pipermail/xenomai/attachments/20140327/6f8e6a10/attachment.sig> ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [Xenomai] Rescnt imbalance in rtdm_mutex_timedlock 2014-03-27 18:04 ` Jan Kiszka @ 2014-03-27 18:24 ` Philippe Gerum 2014-03-27 18:31 ` Jan Kiszka 2014-03-27 18:35 ` Gilles Chanteperdrix 0 siblings, 2 replies; 16+ messages in thread From: Philippe Gerum @ 2014-03-27 18:24 UTC (permalink / raw) To: Jan Kiszka, Henri Roosen; +Cc: Xenomai@xenomai.org On 03/27/2014 07:04 PM, Jan Kiszka wrote: > On 2014-03-27 17:26, Philippe Gerum wrote: >> On 03/27/2014 04:02 PM, Henri Roosen wrote: >>> >>> >>> >>> On Thu, Mar 27, 2014 at 10:41 AM, Philippe Gerum <rpm@xenomai.org >>> <mailto:rpm@xenomai.org>> wrote: >>> >>> On 03/27/2014 12:07 AM, Gilles Chanteperdrix wrote: >>> >>> On 03/26/2014 05:49 PM, Henri Roosen wrote: >>> >>> On Tue, Mar 25, 2014 at 12:45 PM, Erwin Pranz >>> <erwin.pranz@sigmatek.at >>> <mailto:erwin.pranz@sigmatek.at>>__wrote: >>> >>> Hello, >>> >>> I am using rtdm_mutex_lock/rtdm_mutex___unlock in a rtdm >>> driver and I get a >>> signal SIGDEBUG_MIGRATE_PRIOINV when I call the driver >>> from a priority-0 >>> thread.The signal is sent in xnsynch_release_thread when >>> rescnt is 0 on a >>> XNOTHER thread, which means that rescnt is not balanced. >>> >>> I think the reason why rescnt is not balanced is in >>> rtdm_mutex_timedlock: >>> when the owner of the mutex is NULL, the owner is >>> changed to the calling >>> thread, but rescnt is not increased. >>> >>> I suggest the following fix in rtdm_mutex_timedlock: >>> - else if >>> (likely(xnsynch_owner(&mutex->__synch_base) == NULL)) >>> + else if >>> (likely(xnsynch_owner(&mutex->__synch_base) == NULL)) { >>> >>> xnsynch_set_owner(&mutex->__synch_base, curr_thread); >>> + if (xnthread_test_state(curr___thread, >>> XNOTHER)) >>> + xnthread_inc_rescnt(curr___thread); >>> + } >>> >>> >>> Hi Gilles, >>> >>> Could you maybe elaborate on this? It seems the resource >>> counting is not in >>> balance. It would be great to know if this the correct way >>> to fix it. >>> >>> The problem should be easily reproducible by having just one >>> shadowed >>> priority 0 thread that somehow calls rtdm_mutex_lock >>> directly followed by >>> rtdm_mutex_unlock. This will trigger the code in >>> ksrc/nucleus/synch.c that >>> was added by commit-486afd15, which sends >>> SIGDEBUG_MIGRATE_PRIOINV on >>> Xenomai v2.6.2.1. It shouldn't send any signal in that case, >>> as nothing is >>> wrong. >>> >>> Note we are on Xenomai v2.6.2.1. Xenomai v2.6.3 sends (the >>> better name for >>> it) SIGDEBUG_RESCNT_IMBALANCE. >>> >>> >>> To be completely frank, I expected Jan to answer this mail as this >>> concerns RTDM. Anyway, I had a look into it, and: >>> - the RTDM skin is not alone in this case, several skins >>> implement the >>> same pattern (if owner == NULL, set owner, then return); >>> - adding calls to xnthread_inc_rescnt everywhere does not seem >>> right, I >>> would prefer to move this inside some xnsynch_ service. The best >>> candidate I found for that is xnsynch_set_owner, though I am not >>> sure if >>> this will work, especially with fastsynchs. >>> >>> >>> >>> Indeed this wouldn't. I suspect the skins mentioned did not have >>> access to xnsynch_acquire(), and were not updated when the >>> auto-relax feature was introduced for XNOTHER threads. >>> >>> We could provide a helper for fast grabbing a mutex which has no >>> fast locking support shared with userland, saving a call to >>> xnsynch_acquire() in the non-contended case, e.g. >>> >>> diff --git a/include/nucleus/synch.h b/include/nucleus/synch.h >>> index 7deae21..87456ea 100644 >>> --- a/include/nucleus/synch.h >>> +++ b/include/nucleus/synch.h >>> @@ -192,6 +192,22 @@ static inline void xnsynch_set_owner(struct >>> xnsynch *synch, >>> synch->owner = thread; >>> } >>> >>> +static inline int xnsynch_fast_grab(struct xnsynch *synch, >>> + struct xnthread *thread) >>> +{ >>> + XENO_BUGON(NUCLEUS, xnsynch_fastlock_p(synch)); >>> + >>> + if (likely(xnsynch_owner(synch) != NULL)) >>> + return 0; >>> + >>> + xnsynch_set_owner(synch, thread); >>> + >>> + if (xnthread_test_state(thread, XNOTHER)) >>> + xnthread_inc_rescnt(thread); >>> + >>> + return 1; >>> +} >>> + >>> static inline void xnsynch_register_cleanup(__struct xnsynch *synch, >>> void (*handler)(struct >>> xnsynch *)) >>> { >>> diff --git a/ksrc/skins/rtdm/drvlib.c b/ksrc/skins/rtdm/drvlib.c >>> index 391d45f..338005d 100644 >>> --- a/ksrc/skins/rtdm/drvlib.c >>> +++ b/ksrc/skins/rtdm/drvlib.c >>> @@ -1538,9 +1538,7 @@ int rtdm_mutex_timedlock(rtdm___mutex_t >>> *mutex, nanosecs_rel_t timeout, >>> if (unlikely(xnsynch_test_flags(&__mutex->synch_base, >>> RTDM_SYNCH_DELETED))) >>> err = -EIDRM; >>> >>> - else if (likely(xnsynch_owner(&mutex->__synch_base) == NULL)) >>> - xnsynch_set_owner(&mutex->__synch_base, curr_thread); >>> - else { >>> + else if (!xnsynch_fast_grab(&mutex->__synch_base, >>> curr_thread)) { >>> /* Redefinition to clarify XENO_ASSERT output */ >>> #define mutex_owner xnsynch_owner(&mutex->synch___base) >>> XENO_ASSERT(RTDM, mutex_owner != curr_thread, >>> >>> The VxWorks and VRTX skins would have to use it as well. >>> >>> >>> Thanks for the replies! We'll give this patch a try for the RTDM case. >>> >> >> Minor points, but a compiler hint above was inverted and eventually useless, and the naming not descriptive enough. Take #2: >> >> diff --git a/include/nucleus/synch.h b/include/nucleus/synch.h >> index 7deae21..09ebdbf 100644 >> --- a/include/nucleus/synch.h >> +++ b/include/nucleus/synch.h >> @@ -192,6 +192,22 @@ static inline void xnsynch_set_owner(struct xnsynch *synch, >> synch->owner = thread; >> } >> >> +static inline int xnsynch_try_grab(struct xnsynch *synch, >> + struct xnthread *thread) >> +{ >> + XENO_BUGON(NUCLEUS, xnsynch_fastlock_p(synch)); >> + >> + if (xnsynch_owner(synch) != NULL) > > if (unlikely(...)) - locks are generally unlikely to be contended > (unless something is suboptimally designed at the user site). > I'm unsure we really need to take a chance at giving such hint. Modern CPUs tend to have branch prediction right. -- Philippe. ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [Xenomai] Rescnt imbalance in rtdm_mutex_timedlock 2014-03-27 18:24 ` Philippe Gerum @ 2014-03-27 18:31 ` Jan Kiszka 2014-03-27 18:39 ` Gilles Chanteperdrix ` (2 more replies) 2014-03-27 18:35 ` Gilles Chanteperdrix 1 sibling, 3 replies; 16+ messages in thread From: Jan Kiszka @ 2014-03-27 18:31 UTC (permalink / raw) To: Philippe Gerum, Henri Roosen; +Cc: Xenomai@xenomai.org On 2014-03-27 19:24, Philippe Gerum wrote: > On 03/27/2014 07:04 PM, Jan Kiszka wrote: >> On 2014-03-27 17:26, Philippe Gerum wrote: >>> On 03/27/2014 04:02 PM, Henri Roosen wrote: >>>> >>>> >>>> >>>> On Thu, Mar 27, 2014 at 10:41 AM, Philippe Gerum <rpm@xenomai.org >>>> <mailto:rpm@xenomai.org>> wrote: >>>> >>>> On 03/27/2014 12:07 AM, Gilles Chanteperdrix wrote: >>>> >>>> On 03/26/2014 05:49 PM, Henri Roosen wrote: >>>> >>>> On Tue, Mar 25, 2014 at 12:45 PM, Erwin Pranz >>>> <erwin.pranz@sigmatek.at >>>> <mailto:erwin.pranz@sigmatek.at>>__wrote: >>>> >>>> Hello, >>>> >>>> I am using rtdm_mutex_lock/rtdm_mutex___unlock in a >>>> rtdm >>>> driver and I get a >>>> signal SIGDEBUG_MIGRATE_PRIOINV when I call the driver >>>> from a priority-0 >>>> thread.The signal is sent in xnsynch_release_thread >>>> when >>>> rescnt is 0 on a >>>> XNOTHER thread, which means that rescnt is not >>>> balanced. >>>> >>>> I think the reason why rescnt is not balanced is in >>>> rtdm_mutex_timedlock: >>>> when the owner of the mutex is NULL, the owner is >>>> changed to the calling >>>> thread, but rescnt is not increased. >>>> >>>> I suggest the following fix in rtdm_mutex_timedlock: >>>> - else if >>>> (likely(xnsynch_owner(&mutex->__synch_base) == NULL)) >>>> + else if >>>> (likely(xnsynch_owner(&mutex->__synch_base) == >>>> NULL)) { >>>> >>>> xnsynch_set_owner(&mutex->__synch_base, >>>> curr_thread); >>>> + if (xnthread_test_state(curr___thread, >>>> XNOTHER)) >>>> + >>>> xnthread_inc_rescnt(curr___thread); >>>> + } >>>> >>>> >>>> Hi Gilles, >>>> >>>> Could you maybe elaborate on this? It seems the resource >>>> counting is not in >>>> balance. It would be great to know if this the correct way >>>> to fix it. >>>> >>>> The problem should be easily reproducible by having >>>> just one >>>> shadowed >>>> priority 0 thread that somehow calls rtdm_mutex_lock >>>> directly followed by >>>> rtdm_mutex_unlock. This will trigger the code in >>>> ksrc/nucleus/synch.c that >>>> was added by commit-486afd15, which sends >>>> SIGDEBUG_MIGRATE_PRIOINV on >>>> Xenomai v2.6.2.1. It shouldn't send any signal in that >>>> case, >>>> as nothing is >>>> wrong. >>>> >>>> Note we are on Xenomai v2.6.2.1. Xenomai v2.6.3 sends (the >>>> better name for >>>> it) SIGDEBUG_RESCNT_IMBALANCE. >>>> >>>> >>>> To be completely frank, I expected Jan to answer this mail >>>> as this >>>> concerns RTDM. Anyway, I had a look into it, and: >>>> - the RTDM skin is not alone in this case, several skins >>>> implement the >>>> same pattern (if owner == NULL, set owner, then return); >>>> - adding calls to xnthread_inc_rescnt everywhere does not seem >>>> right, I >>>> would prefer to move this inside some xnsynch_ service. The >>>> best >>>> candidate I found for that is xnsynch_set_owner, though I >>>> am not >>>> sure if >>>> this will work, especially with fastsynchs. >>>> >>>> >>>> >>>> Indeed this wouldn't. I suspect the skins mentioned did not have >>>> access to xnsynch_acquire(), and were not updated when the >>>> auto-relax feature was introduced for XNOTHER threads. >>>> >>>> We could provide a helper for fast grabbing a mutex which has no >>>> fast locking support shared with userland, saving a call to >>>> xnsynch_acquire() in the non-contended case, e.g. >>>> >>>> diff --git a/include/nucleus/synch.h b/include/nucleus/synch.h >>>> index 7deae21..87456ea 100644 >>>> --- a/include/nucleus/synch.h >>>> +++ b/include/nucleus/synch.h >>>> @@ -192,6 +192,22 @@ static inline void xnsynch_set_owner(struct >>>> xnsynch *synch, >>>> synch->owner = thread; >>>> } >>>> >>>> +static inline int xnsynch_fast_grab(struct xnsynch *synch, >>>> + struct xnthread *thread) >>>> +{ >>>> + XENO_BUGON(NUCLEUS, xnsynch_fastlock_p(synch)); >>>> + >>>> + if (likely(xnsynch_owner(synch) != NULL)) >>>> + return 0; >>>> + >>>> + xnsynch_set_owner(synch, thread); >>>> + >>>> + if (xnthread_test_state(thread, XNOTHER)) >>>> + xnthread_inc_rescnt(thread); >>>> + >>>> + return 1; >>>> +} >>>> + >>>> static inline void xnsynch_register_cleanup(__struct xnsynch >>>> *synch, >>>> void >>>> (*handler)(struct >>>> xnsynch *)) >>>> { >>>> diff --git a/ksrc/skins/rtdm/drvlib.c b/ksrc/skins/rtdm/drvlib.c >>>> index 391d45f..338005d 100644 >>>> --- a/ksrc/skins/rtdm/drvlib.c >>>> +++ b/ksrc/skins/rtdm/drvlib.c >>>> @@ -1538,9 +1538,7 @@ int rtdm_mutex_timedlock(rtdm___mutex_t >>>> *mutex, nanosecs_rel_t timeout, >>>> if (unlikely(xnsynch_test_flags(&__mutex->synch_base, >>>> RTDM_SYNCH_DELETED))) >>>> err = -EIDRM; >>>> >>>> - else if (likely(xnsynch_owner(&mutex->__synch_base) == >>>> NULL)) >>>> - xnsynch_set_owner(&mutex->__synch_base, >>>> curr_thread); >>>> - else { >>>> + else if (!xnsynch_fast_grab(&mutex->__synch_base, >>>> curr_thread)) { >>>> /* Redefinition to clarify XENO_ASSERT output */ >>>> #define mutex_owner >>>> xnsynch_owner(&mutex->synch___base) >>>> XENO_ASSERT(RTDM, mutex_owner != curr_thread, >>>> >>>> The VxWorks and VRTX skins would have to use it as well. >>>> >>>> >>>> Thanks for the replies! We'll give this patch a try for the RTDM case. >>>> >>> >>> Minor points, but a compiler hint above was inverted and eventually >>> useless, and the naming not descriptive enough. Take #2: >>> >>> diff --git a/include/nucleus/synch.h b/include/nucleus/synch.h >>> index 7deae21..09ebdbf 100644 >>> --- a/include/nucleus/synch.h >>> +++ b/include/nucleus/synch.h >>> @@ -192,6 +192,22 @@ static inline void xnsynch_set_owner(struct >>> xnsynch *synch, >>> synch->owner = thread; >>> } >>> >>> +static inline int xnsynch_try_grab(struct xnsynch *synch, >>> + struct xnthread *thread) >>> +{ >>> + XENO_BUGON(NUCLEUS, xnsynch_fastlock_p(synch)); >>> + >>> + if (xnsynch_owner(synch) != NULL) >> >> if (unlikely(...)) - locks are generally unlikely to be contended >> (unless something is suboptimally designed at the user site). >> > > I'm unsure we really need to take a chance at giving such hint. Modern > CPUs tend to have branch prediction right. Then likely/unlikely will be emptied by Linux. But I'm not aware of any plans in the kernel community to phase out this tag. Jan -------------- next part -------------- A non-text attachment was scrubbed... Name: signature.asc Type: application/pgp-signature Size: 263 bytes Desc: OpenPGP digital signature URL: <http://www.xenomai.org/pipermail/xenomai/attachments/20140327/b68e5621/attachment.sig> ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [Xenomai] Rescnt imbalance in rtdm_mutex_timedlock 2014-03-27 18:31 ` Jan Kiszka @ 2014-03-27 18:39 ` Gilles Chanteperdrix 2014-03-27 20:18 ` Lowell Gilbert 2014-03-28 8:32 ` Philippe Gerum 2 siblings, 0 replies; 16+ messages in thread From: Gilles Chanteperdrix @ 2014-03-27 18:39 UTC (permalink / raw) To: Jan Kiszka; +Cc: Xenomai@xenomai.org On 03/27/2014 07:31 PM, Jan Kiszka wrote: > On 2014-03-27 19:24, Philippe Gerum wrote: >> On 03/27/2014 07:04 PM, Jan Kiszka wrote: >>> On 2014-03-27 17:26, Philippe Gerum wrote: >>>> On 03/27/2014 04:02 PM, Henri Roosen wrote: >>>>> >>>>> >>>>> >>>>> On Thu, Mar 27, 2014 at 10:41 AM, Philippe Gerum <rpm@xenomai.org >>>>> <mailto:rpm@xenomai.org>> wrote: >>>>> >>>>> On 03/27/2014 12:07 AM, Gilles Chanteperdrix wrote: >>>>> >>>>> On 03/26/2014 05:49 PM, Henri Roosen wrote: >>>>> >>>>> On Tue, Mar 25, 2014 at 12:45 PM, Erwin Pranz >>>>> <erwin.pranz@sigmatek.at >>>>> <mailto:erwin.pranz@sigmatek.at>>__wrote: >>>>> >>>>> Hello, >>>>> >>>>> I am using rtdm_mutex_lock/rtdm_mutex___unlock in a >>>>> rtdm >>>>> driver and I get a >>>>> signal SIGDEBUG_MIGRATE_PRIOINV when I call the driver >>>>> from a priority-0 >>>>> thread.The signal is sent in xnsynch_release_thread >>>>> when >>>>> rescnt is 0 on a >>>>> XNOTHER thread, which means that rescnt is not >>>>> balanced. >>>>> >>>>> I think the reason why rescnt is not balanced is in >>>>> rtdm_mutex_timedlock: >>>>> when the owner of the mutex is NULL, the owner is >>>>> changed to the calling >>>>> thread, but rescnt is not increased. >>>>> >>>>> I suggest the following fix in rtdm_mutex_timedlock: >>>>> - else if >>>>> (likely(xnsynch_owner(&mutex->__synch_base) == NULL)) >>>>> + else if >>>>> (likely(xnsynch_owner(&mutex->__synch_base) == >>>>> NULL)) { >>>>> >>>>> xnsynch_set_owner(&mutex->__synch_base, >>>>> curr_thread); >>>>> + if (xnthread_test_state(curr___thread, >>>>> XNOTHER)) >>>>> + >>>>> xnthread_inc_rescnt(curr___thread); >>>>> + } >>>>> >>>>> >>>>> Hi Gilles, >>>>> >>>>> Could you maybe elaborate on this? It seems the resource >>>>> counting is not in >>>>> balance. It would be great to know if this the correct way >>>>> to fix it. >>>>> >>>>> The problem should be easily reproducible by having >>>>> just one >>>>> shadowed >>>>> priority 0 thread that somehow calls rtdm_mutex_lock >>>>> directly followed by >>>>> rtdm_mutex_unlock. This will trigger the code in >>>>> ksrc/nucleus/synch.c that >>>>> was added by commit-486afd15, which sends >>>>> SIGDEBUG_MIGRATE_PRIOINV on >>>>> Xenomai v2.6.2.1. It shouldn't send any signal in that >>>>> case, >>>>> as nothing is >>>>> wrong. >>>>> >>>>> Note we are on Xenomai v2.6.2.1. Xenomai v2.6.3 sends (the >>>>> better name for >>>>> it) SIGDEBUG_RESCNT_IMBALANCE. >>>>> >>>>> >>>>> To be completely frank, I expected Jan to answer this mail >>>>> as this >>>>> concerns RTDM. Anyway, I had a look into it, and: >>>>> - the RTDM skin is not alone in this case, several skins >>>>> implement the >>>>> same pattern (if owner == NULL, set owner, then return); >>>>> - adding calls to xnthread_inc_rescnt everywhere does not seem >>>>> right, I >>>>> would prefer to move this inside some xnsynch_ service. The >>>>> best >>>>> candidate I found for that is xnsynch_set_owner, though I >>>>> am not >>>>> sure if >>>>> this will work, especially with fastsynchs. >>>>> >>>>> >>>>> >>>>> Indeed this wouldn't. I suspect the skins mentioned did not have >>>>> access to xnsynch_acquire(), and were not updated when the >>>>> auto-relax feature was introduced for XNOTHER threads. >>>>> >>>>> We could provide a helper for fast grabbing a mutex which has no >>>>> fast locking support shared with userland, saving a call to >>>>> xnsynch_acquire() in the non-contended case, e.g. >>>>> >>>>> diff --git a/include/nucleus/synch.h b/include/nucleus/synch.h >>>>> index 7deae21..87456ea 100644 >>>>> --- a/include/nucleus/synch.h >>>>> +++ b/include/nucleus/synch.h >>>>> @@ -192,6 +192,22 @@ static inline void xnsynch_set_owner(struct >>>>> xnsynch *synch, >>>>> synch->owner = thread; >>>>> } >>>>> >>>>> +static inline int xnsynch_fast_grab(struct xnsynch *synch, >>>>> + struct xnthread *thread) >>>>> +{ >>>>> + XENO_BUGON(NUCLEUS, xnsynch_fastlock_p(synch)); >>>>> + >>>>> + if (likely(xnsynch_owner(synch) != NULL)) >>>>> + return 0; >>>>> + >>>>> + xnsynch_set_owner(synch, thread); >>>>> + >>>>> + if (xnthread_test_state(thread, XNOTHER)) >>>>> + xnthread_inc_rescnt(thread); >>>>> + >>>>> + return 1; >>>>> +} >>>>> + >>>>> static inline void xnsynch_register_cleanup(__struct xnsynch >>>>> *synch, >>>>> void >>>>> (*handler)(struct >>>>> xnsynch *)) >>>>> { >>>>> diff --git a/ksrc/skins/rtdm/drvlib.c b/ksrc/skins/rtdm/drvlib.c >>>>> index 391d45f..338005d 100644 >>>>> --- a/ksrc/skins/rtdm/drvlib.c >>>>> +++ b/ksrc/skins/rtdm/drvlib.c >>>>> @@ -1538,9 +1538,7 @@ int rtdm_mutex_timedlock(rtdm___mutex_t >>>>> *mutex, nanosecs_rel_t timeout, >>>>> if (unlikely(xnsynch_test_flags(&__mutex->synch_base, >>>>> RTDM_SYNCH_DELETED))) >>>>> err = -EIDRM; >>>>> >>>>> - else if (likely(xnsynch_owner(&mutex->__synch_base) == >>>>> NULL)) >>>>> - xnsynch_set_owner(&mutex->__synch_base, >>>>> curr_thread); >>>>> - else { >>>>> + else if (!xnsynch_fast_grab(&mutex->__synch_base, >>>>> curr_thread)) { >>>>> /* Redefinition to clarify XENO_ASSERT output */ >>>>> #define mutex_owner >>>>> xnsynch_owner(&mutex->synch___base) >>>>> XENO_ASSERT(RTDM, mutex_owner != curr_thread, >>>>> >>>>> The VxWorks and VRTX skins would have to use it as well. >>>>> >>>>> >>>>> Thanks for the replies! We'll give this patch a try for the RTDM case. >>>>> >>>> >>>> Minor points, but a compiler hint above was inverted and eventually >>>> useless, and the naming not descriptive enough. Take #2: >>>> >>>> diff --git a/include/nucleus/synch.h b/include/nucleus/synch.h >>>> index 7deae21..09ebdbf 100644 >>>> --- a/include/nucleus/synch.h >>>> +++ b/include/nucleus/synch.h >>>> @@ -192,6 +192,22 @@ static inline void xnsynch_set_owner(struct >>>> xnsynch *synch, >>>> synch->owner = thread; >>>> } >>>> >>>> +static inline int xnsynch_try_grab(struct xnsynch *synch, >>>> + struct xnthread *thread) >>>> +{ >>>> + XENO_BUGON(NUCLEUS, xnsynch_fastlock_p(synch)); >>>> + >>>> + if (xnsynch_owner(synch) != NULL) >>> >>> if (unlikely(...)) - locks are generally unlikely to be contended >>> (unless something is suboptimally designed at the user site). >>> >> >> I'm unsure we really need to take a chance at giving such hint. Modern >> CPUs tend to have branch prediction right. > > Then likely/unlikely will be emptied by Linux. But I'm not aware of any > plans in the kernel community to phase out this tag. On ARM at least, I have seen situation where gcc does not take likely/unlikely into account. The same code is simply generated. -- Gilles. ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [Xenomai] Rescnt imbalance in rtdm_mutex_timedlock 2014-03-27 18:31 ` Jan Kiszka 2014-03-27 18:39 ` Gilles Chanteperdrix @ 2014-03-27 20:18 ` Lowell Gilbert 2014-03-28 8:32 ` Philippe Gerum 2 siblings, 0 replies; 16+ messages in thread From: Lowell Gilbert @ 2014-03-27 20:18 UTC (permalink / raw) To: Jan Kiszka; +Cc: Xenomai@xenomai.org Jan Kiszka <jan.kiszka@web.de> writes: > On 2014-03-27 19:24, Philippe Gerum wrote: >> On 03/27/2014 07:04 PM, Jan Kiszka wrote: >>> if (unlikely(...)) - locks are generally unlikely to be contended >>> (unless something is suboptimally designed at the user site). >>> >> >> I'm unsure we really need to take a chance at giving such hint. Modern >> CPUs tend to have branch prediction right. > > Then likely/unlikely will be emptied by Linux. But I'm not aware of any > plans in the kernel community to phase out this tag. Hints tend to be overused. They are only useful if they not only correct a branch assumption that was incorrect, but also allow for a performance improvement as well. Aside from the fact that sometimes the optimal code is identical either way, it will often differ by just an instruction or two without even stalling a pipeline. But hints won't go away, either, for many reasons. Obviously, not all CPUs that run Linux have great branch prediction. The compiler can do some optimizations by rearranging the code to keep the likely path in the cache, so even if the CPU will predict the branch correctly, the compiler may not be able to, and an opportunity is missed. Even the best dynamic branch profiling has limits, and a priori knowledge (like the fact that locks are usually available) may be able to outperform anything available. Et cetera. ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [Xenomai] Rescnt imbalance in rtdm_mutex_timedlock 2014-03-27 18:31 ` Jan Kiszka 2014-03-27 18:39 ` Gilles Chanteperdrix 2014-03-27 20:18 ` Lowell Gilbert @ 2014-03-28 8:32 ` Philippe Gerum 2 siblings, 0 replies; 16+ messages in thread From: Philippe Gerum @ 2014-03-28 8:32 UTC (permalink / raw) To: Jan Kiszka, Henri Roosen; +Cc: Xenomai@xenomai.org On 03/27/2014 07:31 PM, Jan Kiszka wrote: > On 2014-03-27 19:24, Philippe Gerum wrote: >> On 03/27/2014 07:04 PM, Jan Kiszka wrote: >>> On 2014-03-27 17:26, Philippe Gerum wrote: >>>> On 03/27/2014 04:02 PM, Henri Roosen wrote: >>>>> >>>>> >>>>> >>>>> On Thu, Mar 27, 2014 at 10:41 AM, Philippe Gerum <rpm@xenomai.org >>>>> <mailto:rpm@xenomai.org>> wrote: >>>>> >>>>> On 03/27/2014 12:07 AM, Gilles Chanteperdrix wrote: >>>>> >>>>> On 03/26/2014 05:49 PM, Henri Roosen wrote: >>>>> >>>>> On Tue, Mar 25, 2014 at 12:45 PM, Erwin Pranz >>>>> <erwin.pranz@sigmatek.at >>>>> <mailto:erwin.pranz@sigmatek.at>>__wrote: >>>>> >>>>> Hello, >>>>> >>>>> I am using rtdm_mutex_lock/rtdm_mutex___unlock in a >>>>> rtdm >>>>> driver and I get a >>>>> signal SIGDEBUG_MIGRATE_PRIOINV when I call the driver >>>>> from a priority-0 >>>>> thread.The signal is sent in xnsynch_release_thread >>>>> when >>>>> rescnt is 0 on a >>>>> XNOTHER thread, which means that rescnt is not >>>>> balanced. >>>>> >>>>> I think the reason why rescnt is not balanced is in >>>>> rtdm_mutex_timedlock: >>>>> when the owner of the mutex is NULL, the owner is >>>>> changed to the calling >>>>> thread, but rescnt is not increased. >>>>> >>>>> I suggest the following fix in rtdm_mutex_timedlock: >>>>> - else if >>>>> (likely(xnsynch_owner(&mutex->__synch_base) == NULL)) >>>>> + else if >>>>> (likely(xnsynch_owner(&mutex->__synch_base) == >>>>> NULL)) { >>>>> >>>>> xnsynch_set_owner(&mutex->__synch_base, >>>>> curr_thread); >>>>> + if (xnthread_test_state(curr___thread, >>>>> XNOTHER)) >>>>> + >>>>> xnthread_inc_rescnt(curr___thread); >>>>> + } >>>>> >>>>> >>>>> Hi Gilles, >>>>> >>>>> Could you maybe elaborate on this? It seems the resource >>>>> counting is not in >>>>> balance. It would be great to know if this the correct way >>>>> to fix it. >>>>> >>>>> The problem should be easily reproducible by having >>>>> just one >>>>> shadowed >>>>> priority 0 thread that somehow calls rtdm_mutex_lock >>>>> directly followed by >>>>> rtdm_mutex_unlock. This will trigger the code in >>>>> ksrc/nucleus/synch.c that >>>>> was added by commit-486afd15, which sends >>>>> SIGDEBUG_MIGRATE_PRIOINV on >>>>> Xenomai v2.6.2.1. It shouldn't send any signal in that >>>>> case, >>>>> as nothing is >>>>> wrong. >>>>> >>>>> Note we are on Xenomai v2.6.2.1. Xenomai v2.6.3 sends (the >>>>> better name for >>>>> it) SIGDEBUG_RESCNT_IMBALANCE. >>>>> >>>>> >>>>> To be completely frank, I expected Jan to answer this mail >>>>> as this >>>>> concerns RTDM. Anyway, I had a look into it, and: >>>>> - the RTDM skin is not alone in this case, several skins >>>>> implement the >>>>> same pattern (if owner == NULL, set owner, then return); >>>>> - adding calls to xnthread_inc_rescnt everywhere does not seem >>>>> right, I >>>>> would prefer to move this inside some xnsynch_ service. The >>>>> best >>>>> candidate I found for that is xnsynch_set_owner, though I >>>>> am not >>>>> sure if >>>>> this will work, especially with fastsynchs. >>>>> >>>>> >>>>> >>>>> Indeed this wouldn't. I suspect the skins mentioned did not have >>>>> access to xnsynch_acquire(), and were not updated when the >>>>> auto-relax feature was introduced for XNOTHER threads. >>>>> >>>>> We could provide a helper for fast grabbing a mutex which has no >>>>> fast locking support shared with userland, saving a call to >>>>> xnsynch_acquire() in the non-contended case, e.g. >>>>> >>>>> diff --git a/include/nucleus/synch.h b/include/nucleus/synch.h >>>>> index 7deae21..87456ea 100644 >>>>> --- a/include/nucleus/synch.h >>>>> +++ b/include/nucleus/synch.h >>>>> @@ -192,6 +192,22 @@ static inline void xnsynch_set_owner(struct >>>>> xnsynch *synch, >>>>> synch->owner = thread; >>>>> } >>>>> >>>>> +static inline int xnsynch_fast_grab(struct xnsynch *synch, >>>>> + struct xnthread *thread) >>>>> +{ >>>>> + XENO_BUGON(NUCLEUS, xnsynch_fastlock_p(synch)); >>>>> + >>>>> + if (likely(xnsynch_owner(synch) != NULL)) >>>>> + return 0; >>>>> + >>>>> + xnsynch_set_owner(synch, thread); >>>>> + >>>>> + if (xnthread_test_state(thread, XNOTHER)) >>>>> + xnthread_inc_rescnt(thread); >>>>> + >>>>> + return 1; >>>>> +} >>>>> + >>>>> static inline void xnsynch_register_cleanup(__struct xnsynch >>>>> *synch, >>>>> void >>>>> (*handler)(struct >>>>> xnsynch *)) >>>>> { >>>>> diff --git a/ksrc/skins/rtdm/drvlib.c b/ksrc/skins/rtdm/drvlib.c >>>>> index 391d45f..338005d 100644 >>>>> --- a/ksrc/skins/rtdm/drvlib.c >>>>> +++ b/ksrc/skins/rtdm/drvlib.c >>>>> @@ -1538,9 +1538,7 @@ int rtdm_mutex_timedlock(rtdm___mutex_t >>>>> *mutex, nanosecs_rel_t timeout, >>>>> if (unlikely(xnsynch_test_flags(&__mutex->synch_base, >>>>> RTDM_SYNCH_DELETED))) >>>>> err = -EIDRM; >>>>> >>>>> - else if (likely(xnsynch_owner(&mutex->__synch_base) == >>>>> NULL)) >>>>> - xnsynch_set_owner(&mutex->__synch_base, >>>>> curr_thread); >>>>> - else { >>>>> + else if (!xnsynch_fast_grab(&mutex->__synch_base, >>>>> curr_thread)) { >>>>> /* Redefinition to clarify XENO_ASSERT output */ >>>>> #define mutex_owner >>>>> xnsynch_owner(&mutex->synch___base) >>>>> XENO_ASSERT(RTDM, mutex_owner != curr_thread, >>>>> >>>>> The VxWorks and VRTX skins would have to use it as well. >>>>> >>>>> >>>>> Thanks for the replies! We'll give this patch a try for the RTDM case. >>>>> >>>> >>>> Minor points, but a compiler hint above was inverted and eventually >>>> useless, and the naming not descriptive enough. Take #2: >>>> >>>> diff --git a/include/nucleus/synch.h b/include/nucleus/synch.h >>>> index 7deae21..09ebdbf 100644 >>>> --- a/include/nucleus/synch.h >>>> +++ b/include/nucleus/synch.h >>>> @@ -192,6 +192,22 @@ static inline void xnsynch_set_owner(struct >>>> xnsynch *synch, >>>> synch->owner = thread; >>>> } >>>> >>>> +static inline int xnsynch_try_grab(struct xnsynch *synch, >>>> + struct xnthread *thread) >>>> +{ >>>> + XENO_BUGON(NUCLEUS, xnsynch_fastlock_p(synch)); >>>> + >>>> + if (xnsynch_owner(synch) != NULL) >>> >>> if (unlikely(...)) - locks are generally unlikely to be contended >>> (unless something is suboptimally designed at the user site). >>> >> >> I'm unsure we really need to take a chance at giving such hint. Modern >> CPUs tend to have branch prediction right. > > Then likely/unlikely will be emptied by Linux. But I'm not aware of any > plans in the kernel community to phase out this tag. > Don't get me wrong. I'm not questioning the use of hints when they express an obvious expectation, such as tagging a branch that should not run under normal condition as unlikely. However, I would not use them when the outcome is significantly more uncertain otherwise, because I don't feel challenging the branch predictor in those cases. This said, this hint is a minor point. -- Philippe. ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [Xenomai] Rescnt imbalance in rtdm_mutex_timedlock 2014-03-27 18:24 ` Philippe Gerum 2014-03-27 18:31 ` Jan Kiszka @ 2014-03-27 18:35 ` Gilles Chanteperdrix 1 sibling, 0 replies; 16+ messages in thread From: Gilles Chanteperdrix @ 2014-03-27 18:35 UTC (permalink / raw) To: Philippe Gerum; +Cc: Jan Kiszka, Xenomai@xenomai.org On 03/27/2014 07:24 PM, Philippe Gerum wrote: > I'm unsure we really need to take a chance at giving such hint. Modern > CPUs tend to have branch prediction right. Not to mention the fact that the compiler may not use the information used by likely/unlikely, or may not be able use it in a sensible way. Typically, for code such as if (unlikely(condition)) return 0; I do not see that it makes sense for the compiler to "jump out of hot path", for a return 0, it will put the return 0 in place, there is nothing to win by jumping away to then just return. -- Gilles. ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [Xenomai] Rescnt imbalance in rtdm_mutex_timedlock 2014-03-27 16:26 ` Philippe Gerum 2014-03-27 18:04 ` Jan Kiszka @ 2014-03-28 9:52 ` Erwin Pranz 2014-03-28 10:14 ` Philippe Gerum 1 sibling, 1 reply; 16+ messages in thread From: Erwin Pranz @ 2014-03-28 9:52 UTC (permalink / raw) To: xenomai Am 27.03.2014 17:26, schrieb Philippe Gerum: > On 03/27/2014 04:02 PM, Henri Roosen wrote: >> >> >> On Thu, Mar 27, 2014 at 10:41 AM, Philippe Gerum <rpm@xenomai.org >> <mailto:rpm@xenomai.org>> wrote: >> >> On 03/27/2014 12:07 AM, Gilles Chanteperdrix wrote: >> >> On 03/26/2014 05:49 PM, Henri Roosen wrote: >> >> On Tue, Mar 25, 2014 at 12:45 PM, Erwin Pranz >> <erwin.pranz@sigmatek.at >> <mailto:erwin.pranz@sigmatek.at>>__wrote: >> >> Hello, >> >> I am using rtdm_mutex_lock/rtdm_mutex___unlock in a rtdm >> driver and I get a >> signal SIGDEBUG_MIGRATE_PRIOINV when I call the driver >> from a priority-0 >> thread.The signal is sent in xnsynch_release_thread when >> rescnt is 0 on a >> XNOTHER thread, which means that rescnt is not balanced. >> >> I think the reason why rescnt is not balanced is in >> rtdm_mutex_timedlock: >> when the owner of the mutex is NULL, the owner is >> changed to the calling >> thread, but rescnt is not increased. >> >> I suggest the following fix in rtdm_mutex_timedlock: >> - else if >> (likely(xnsynch_owner(&mutex->__synch_base) == NULL)) >> + else if >> (likely(xnsynch_owner(&mutex->__synch_base) == NULL)) { >> >> xnsynch_set_owner(&mutex->__synch_base, curr_thread); >> + if (xnthread_test_state(curr___thread, >> XNOTHER)) >> + xnthread_inc_rescnt(curr___thread); >> + } >> >> >> Hi Gilles, >> >> Could you maybe elaborate on this? It seems the resource >> counting is not in >> balance. It would be great to know if this the correct way >> to fix it. >> >> The problem should be easily reproducible by having just one >> shadowed >> priority 0 thread that somehow calls rtdm_mutex_lock >> directly followed by >> rtdm_mutex_unlock. This will trigger the code in >> ksrc/nucleus/synch.c that >> was added by commit-486afd15, which sends >> SIGDEBUG_MIGRATE_PRIOINV on >> Xenomai v2.6.2.1. It shouldn't send any signal in that case, >> as nothing is >> wrong. >> >> Note we are on Xenomai v2.6.2.1. Xenomai v2.6.3 sends (the >> better name for >> it) SIGDEBUG_RESCNT_IMBALANCE. >> >> >> To be completely frank, I expected Jan to answer this mail as this >> concerns RTDM. Anyway, I had a look into it, and: >> - the RTDM skin is not alone in this case, several skins >> implement the >> same pattern (if owner == NULL, set owner, then return); >> - adding calls to xnthread_inc_rescnt everywhere does not seem >> right, I >> would prefer to move this inside some xnsynch_ service. The best >> candidate I found for that is xnsynch_set_owner, though I am not >> sure if >> this will work, especially with fastsynchs. >> >> >> >> Indeed this wouldn't. I suspect the skins mentioned did not have >> access to xnsynch_acquire(), and were not updated when the >> auto-relax feature was introduced for XNOTHER threads. >> >> We could provide a helper for fast grabbing a mutex which has no >> fast locking support shared with userland, saving a call to >> xnsynch_acquire() in the non-contended case, e.g. >> >> diff --git a/include/nucleus/synch.h b/include/nucleus/synch.h >> index 7deae21..87456ea 100644 >> --- a/include/nucleus/synch.h >> +++ b/include/nucleus/synch.h >> @@ -192,6 +192,22 @@ static inline void xnsynch_set_owner(struct >> xnsynch *synch, >> synch->owner = thread; >> } >> >> +static inline int xnsynch_fast_grab(struct xnsynch *synch, >> + struct xnthread *thread) >> +{ >> + XENO_BUGON(NUCLEUS, xnsynch_fastlock_p(synch)); >> + >> + if (likely(xnsynch_owner(synch) != NULL)) >> + return 0; >> + >> + xnsynch_set_owner(synch, thread); >> + >> + if (xnthread_test_state(thread, XNOTHER)) >> + xnthread_inc_rescnt(thread); >> + >> + return 1; >> +} >> + >> static inline void xnsynch_register_cleanup(__struct xnsynch *synch, >> void (*handler)(struct >> xnsynch *)) >> { >> diff --git a/ksrc/skins/rtdm/drvlib.c b/ksrc/skins/rtdm/drvlib.c >> index 391d45f..338005d 100644 >> --- a/ksrc/skins/rtdm/drvlib.c >> +++ b/ksrc/skins/rtdm/drvlib.c >> @@ -1538,9 +1538,7 @@ int rtdm_mutex_timedlock(rtdm___mutex_t >> *mutex, nanosecs_rel_t timeout, >> if (unlikely(xnsynch_test_flags(&__mutex->synch_base, >> RTDM_SYNCH_DELETED))) >> err = -EIDRM; >> >> - else if (likely(xnsynch_owner(&mutex->__synch_base) == NULL)) >> - xnsynch_set_owner(&mutex->__synch_base, curr_thread); >> - else { >> + else if (!xnsynch_fast_grab(&mutex->__synch_base, >> curr_thread)) { >> /* Redefinition to clarify XENO_ASSERT output */ >> #define mutex_owner xnsynch_owner(&mutex->synch___base) >> XENO_ASSERT(RTDM, mutex_owner != curr_thread, >> >> The VxWorks and VRTX skins would have to use it as well. >> >> >> Thanks for the replies! We'll give this patch a try for the RTDM case. >> > Minor points, but a compiler hint above was inverted and eventually useless, and the naming not descriptive enough. Take #2: > > diff --git a/include/nucleus/synch.h b/include/nucleus/synch.h > index 7deae21..09ebdbf 100644 > --- a/include/nucleus/synch.h > +++ b/include/nucleus/synch.h > @@ -192,6 +192,22 @@ static inline void xnsynch_set_owner(struct xnsynch *synch, > synch->owner = thread; > } > > +static inline int xnsynch_try_grab(struct xnsynch *synch, > + struct xnthread *thread) > +{ > + XENO_BUGON(NUCLEUS, xnsynch_fastlock_p(synch)); > + > + if (xnsynch_owner(synch) != NULL) > + return 0; > + > + xnsynch_set_owner(synch, thread); > + > + if (xnthread_test_state(thread, XNOTHER)) > + xnthread_inc_rescnt(thread); > + > + return 1; > +} > + > static inline void xnsynch_register_cleanup(struct xnsynch *synch, > void (*handler)(struct xnsynch *)) > { > diff --git a/ksrc/skins/rtdm/drvlib.c b/ksrc/skins/rtdm/drvlib.c > index 391d45f..aa85d11 100644 > --- a/ksrc/skins/rtdm/drvlib.c > +++ b/ksrc/skins/rtdm/drvlib.c > @@ -1538,9 +1538,7 @@ int rtdm_mutex_timedlock(rtdm_mutex_t *mutex, nanosecs_rel_t timeout, > if (unlikely(xnsynch_test_flags(&mutex->synch_base, > RTDM_SYNCH_DELETED))) > err = -EIDRM; > - else if (likely(xnsynch_owner(&mutex->synch_base) == NULL)) > - xnsynch_set_owner(&mutex->synch_base, curr_thread); > - else { > + else if (!xnsynch_try_grab(&mutex->synch_base, curr_thread)) { > /* Redefinition to clarify XENO_ASSERT output */ > #define mutex_owner xnsynch_owner(&mutex->synch_base) > XENO_ASSERT(RTDM, mutex_owner != curr_thread, > Hello, I tried this patch, but it did not compile: In file included from include/xenomai/nucleus/registry.h:33:0, from include/xenomai/nucleus/thread.h:160, from include/xenomai/nucleus/sched.h:31, from include/xenomai/nucleus/pod.h:34, from include/xenomai/nucleus/xenomai.h:23, from include/xenomai/rtdm/rtdm_driver.h:37, from drivers/xenomai/can/rtcan_internal.h:30, from drivers/xenomai/can/rtcan_dev.c:32: include/xenomai/nucleus/synch.h: In function 'xnsynch_try_grab': include/xenomai/nucleus/synch.h:205:2: error: implicit declaration of function 'xnthread_test_state' [-Werror=implicit-function-declaration] include/xenomai/nucleus/synch.h:206:3: error: implicit declaration of function 'xnthread_inc_rescnt' [-Werror=implicit-function-declaration] I think there is a circular dependency problem in the header files: - thread.h includes sync.h via registry.h at a point before xnthread_test_state/xnthread_inc_rescnt is defined - synch.h needs the defines from thread.h (xnthread_test_state/xnthread_inc_rescnt) I could not resolve this dependency Erwin ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [Xenomai] Rescnt imbalance in rtdm_mutex_timedlock 2014-03-28 9:52 ` Erwin Pranz @ 2014-03-28 10:14 ` Philippe Gerum 2014-03-28 11:14 ` Erwin Pranz 0 siblings, 1 reply; 16+ messages in thread From: Philippe Gerum @ 2014-03-28 10:14 UTC (permalink / raw) To: erwin.pranz, xenomai On 03/28/2014 10:52 AM, Erwin Pranz wrote: > Am 27.03.2014 17:26, schrieb Philippe Gerum: >> On 03/27/2014 04:02 PM, Henri Roosen wrote: >>> >>> >>> On Thu, Mar 27, 2014 at 10:41 AM, Philippe Gerum <rpm@xenomai.org >>> <mailto:rpm@xenomai.org>> wrote: >>> >>> On 03/27/2014 12:07 AM, Gilles Chanteperdrix wrote: >>> >>> On 03/26/2014 05:49 PM, Henri Roosen wrote: >>> >>> On Tue, Mar 25, 2014 at 12:45 PM, Erwin Pranz >>> <erwin.pranz@sigmatek.at >>> <mailto:erwin.pranz@sigmatek.at>>__wrote: >>> >>> Hello, >>> >>> I am using rtdm_mutex_lock/rtdm_mutex___unlock in a >>> rtdm >>> driver and I get a >>> signal SIGDEBUG_MIGRATE_PRIOINV when I call the driver >>> from a priority-0 >>> thread.The signal is sent in xnsynch_release_thread >>> when >>> rescnt is 0 on a >>> XNOTHER thread, which means that rescnt is not >>> balanced. >>> >>> I think the reason why rescnt is not balanced is in >>> rtdm_mutex_timedlock: >>> when the owner of the mutex is NULL, the owner is >>> changed to the calling >>> thread, but rescnt is not increased. >>> >>> I suggest the following fix in rtdm_mutex_timedlock: >>> - else if >>> (likely(xnsynch_owner(&mutex->__synch_base) == NULL)) >>> + else if >>> (likely(xnsynch_owner(&mutex->__synch_base) == NULL)) { >>> xnsynch_set_owner(&mutex->__synch_base, curr_thread); >>> + if (xnthread_test_state(curr___thread, >>> XNOTHER)) >>> + >>> xnthread_inc_rescnt(curr___thread); >>> + } >>> >>> >>> Hi Gilles, >>> >>> Could you maybe elaborate on this? It seems the resource >>> counting is not in >>> balance. It would be great to know if this the correct way >>> to fix it. >>> >>> The problem should be easily reproducible by having just >>> one >>> shadowed >>> priority 0 thread that somehow calls rtdm_mutex_lock >>> directly followed by >>> rtdm_mutex_unlock. This will trigger the code in >>> ksrc/nucleus/synch.c that >>> was added by commit-486afd15, which sends >>> SIGDEBUG_MIGRATE_PRIOINV on >>> Xenomai v2.6.2.1. It shouldn't send any signal in that >>> case, >>> as nothing is >>> wrong. >>> >>> Note we are on Xenomai v2.6.2.1. Xenomai v2.6.3 sends (the >>> better name for >>> it) SIGDEBUG_RESCNT_IMBALANCE. >>> >>> >>> To be completely frank, I expected Jan to answer this mail >>> as this >>> concerns RTDM. Anyway, I had a look into it, and: >>> - the RTDM skin is not alone in this case, several skins >>> implement the >>> same pattern (if owner == NULL, set owner, then return); >>> - adding calls to xnthread_inc_rescnt everywhere does not seem >>> right, I >>> would prefer to move this inside some xnsynch_ service. The >>> best >>> candidate I found for that is xnsynch_set_owner, though I am >>> not >>> sure if >>> this will work, especially with fastsynchs. >>> >>> >>> >>> Indeed this wouldn't. I suspect the skins mentioned did not have >>> access to xnsynch_acquire(), and were not updated when the >>> auto-relax feature was introduced for XNOTHER threads. >>> >>> We could provide a helper for fast grabbing a mutex which has no >>> fast locking support shared with userland, saving a call to >>> xnsynch_acquire() in the non-contended case, e.g. >>> >>> diff --git a/include/nucleus/synch.h b/include/nucleus/synch.h >>> index 7deae21..87456ea 100644 >>> --- a/include/nucleus/synch.h >>> +++ b/include/nucleus/synch.h >>> @@ -192,6 +192,22 @@ static inline void xnsynch_set_owner(struct >>> xnsynch *synch, >>> synch->owner = thread; >>> } >>> >>> +static inline int xnsynch_fast_grab(struct xnsynch *synch, >>> + struct xnthread *thread) >>> +{ >>> + XENO_BUGON(NUCLEUS, xnsynch_fastlock_p(synch)); >>> + >>> + if (likely(xnsynch_owner(synch) != NULL)) >>> + return 0; >>> + >>> + xnsynch_set_owner(synch, thread); >>> + >>> + if (xnthread_test_state(thread, XNOTHER)) >>> + xnthread_inc_rescnt(thread); >>> + >>> + return 1; >>> +} >>> + >>> static inline void xnsynch_register_cleanup(__struct xnsynch >>> *synch, >>> void (*handler)(struct >>> xnsynch *)) >>> { >>> diff --git a/ksrc/skins/rtdm/drvlib.c b/ksrc/skins/rtdm/drvlib.c >>> index 391d45f..338005d 100644 >>> --- a/ksrc/skins/rtdm/drvlib.c >>> +++ b/ksrc/skins/rtdm/drvlib.c >>> @@ -1538,9 +1538,7 @@ int rtdm_mutex_timedlock(rtdm___mutex_t >>> *mutex, nanosecs_rel_t timeout, >>> if (unlikely(xnsynch_test_flags(&__mutex->synch_base, >>> RTDM_SYNCH_DELETED))) >>> err = -EIDRM; >>> >>> - else if (likely(xnsynch_owner(&mutex->__synch_base) == >>> NULL)) >>> - xnsynch_set_owner(&mutex->__synch_base, >>> curr_thread); >>> - else { >>> + else if (!xnsynch_fast_grab(&mutex->__synch_base, >>> curr_thread)) { >>> /* Redefinition to clarify XENO_ASSERT output */ >>> #define mutex_owner >>> xnsynch_owner(&mutex->synch___base) >>> XENO_ASSERT(RTDM, mutex_owner != curr_thread, >>> >>> The VxWorks and VRTX skins would have to use it as well. >>> >>> >>> Thanks for the replies! We'll give this patch a try for the RTDM case. >>> >> Minor points, but a compiler hint above was inverted and eventually >> useless, and the naming not descriptive enough. Take #2: >> >> diff --git a/include/nucleus/synch.h b/include/nucleus/synch.h >> index 7deae21..09ebdbf 100644 >> --- a/include/nucleus/synch.h >> +++ b/include/nucleus/synch.h >> @@ -192,6 +192,22 @@ static inline void xnsynch_set_owner(struct >> xnsynch *synch, >> synch->owner = thread; >> } >> +static inline int xnsynch_try_grab(struct xnsynch *synch, >> + struct xnthread *thread) >> +{ >> + XENO_BUGON(NUCLEUS, xnsynch_fastlock_p(synch)); >> + >> + if (xnsynch_owner(synch) != NULL) >> + return 0; >> + >> + xnsynch_set_owner(synch, thread); >> + >> + if (xnthread_test_state(thread, XNOTHER)) >> + xnthread_inc_rescnt(thread); >> + >> + return 1; >> +} >> + >> static inline void xnsynch_register_cleanup(struct xnsynch *synch, >> void (*handler)(struct xnsynch *)) >> { >> diff --git a/ksrc/skins/rtdm/drvlib.c b/ksrc/skins/rtdm/drvlib.c >> index 391d45f..aa85d11 100644 >> --- a/ksrc/skins/rtdm/drvlib.c >> +++ b/ksrc/skins/rtdm/drvlib.c >> @@ -1538,9 +1538,7 @@ int rtdm_mutex_timedlock(rtdm_mutex_t *mutex, >> nanosecs_rel_t timeout, >> if (unlikely(xnsynch_test_flags(&mutex->synch_base, >> RTDM_SYNCH_DELETED))) >> err = -EIDRM; >> - else if (likely(xnsynch_owner(&mutex->synch_base) == NULL)) >> - xnsynch_set_owner(&mutex->synch_base, curr_thread); >> - else { >> + else if (!xnsynch_try_grab(&mutex->synch_base, curr_thread)) { >> /* Redefinition to clarify XENO_ASSERT output */ >> #define mutex_owner xnsynch_owner(&mutex->synch_base) >> XENO_ASSERT(RTDM, mutex_owner != curr_thread, >> > Hello, > > I tried this patch, but it did not compile: > In file included from include/xenomai/nucleus/registry.h:33:0, > from include/xenomai/nucleus/thread.h:160, > from include/xenomai/nucleus/sched.h:31, > from include/xenomai/nucleus/pod.h:34, > from include/xenomai/nucleus/xenomai.h:23, > from include/xenomai/rtdm/rtdm_driver.h:37, > from drivers/xenomai/can/rtcan_internal.h:30, > from drivers/xenomai/can/rtcan_dev.c:32: > include/xenomai/nucleus/synch.h: In function 'xnsynch_try_grab': > include/xenomai/nucleus/synch.h:205:2: error: implicit declaration of > function 'xnthread_test_state' [-Werror=implicit-function-declaration] > include/xenomai/nucleus/synch.h:206:3: error: implicit declaration of > function 'xnthread_inc_rescnt' [-Werror=implicit-function-declaration] > > I think there is a circular dependency problem in the header files: > - thread.h includes sync.h via registry.h at a point before > xnthread_test_state/xnthread_inc_rescnt is defined > - synch.h needs the defines from thread.h > (xnthread_test_state/xnthread_inc_rescnt) > > I could not resolve this dependency > Then let's make this a thread-based helper, this is logically correct too, and there can't be any ambiguity with respect to which kind of resources can be possibly grabbed by a core thread (and this is better than messing up with macros to postpone the references). diff --git a/include/nucleus/thread.h b/include/nucleus/thread.h index 4a740a0..aca5f90 100644 --- a/include/nucleus/thread.h +++ b/include/nucleus/thread.h @@ -436,6 +436,22 @@ struct xnthread *xnthread_lookup(xnhandle_t threadh) return (thread && xnthread_handle(thread) == threadh) ? thread : NULL; } +static inline int xnthread_try_grab(struct xnthread *thread, + struct xnsynch *synch) +{ + XENO_BUGON(NUCLEUS, xnsynch_fastlock_p(synch)); + + if (xnsynch_owner(synch) != NULL) + return 0; + + xnsynch_set_owner(synch, thread); + + if (xnthread_test_state(thread, XNOTHER)) + xnthread_inc_rescnt(thread); + + return 1; +} + #ifdef __cplusplus extern "C" { #endif diff --git a/ksrc/skins/rtdm/drvlib.c b/ksrc/skins/rtdm/drvlib.c index 391d45f..2e0f14b 100644 --- a/ksrc/skins/rtdm/drvlib.c +++ b/ksrc/skins/rtdm/drvlib.c @@ -1538,9 +1538,7 @@ int rtdm_mutex_timedlock(rtdm_mutex_t *mutex, nanosecs_rel_t timeout, if (unlikely(xnsynch_test_flags(&mutex->synch_base, RTDM_SYNCH_DELETED))) err = -EIDRM; - else if (likely(xnsynch_owner(&mutex->synch_base) == NULL)) - xnsynch_set_owner(&mutex->synch_base, curr_thread); - else { + else if (!xnthread_try_grab(curr_thread, &mutex->synch_base)) { /* Redefinition to clarify XENO_ASSERT output */ #define mutex_owner xnsynch_owner(&mutex->synch_base) XENO_ASSERT(RTDM, mutex_owner != curr_thread, -- Philippe. ^ permalink raw reply related [flat|nested] 16+ messages in thread
* Re: [Xenomai] Rescnt imbalance in rtdm_mutex_timedlock 2014-03-28 10:14 ` Philippe Gerum @ 2014-03-28 11:14 ` Erwin Pranz 0 siblings, 0 replies; 16+ messages in thread From: Erwin Pranz @ 2014-03-28 11:14 UTC (permalink / raw) To: rpm, xenomai Am 28.03.2014 11:14, schrieb Philippe Gerum: > On 03/28/2014 10:52 AM, Erwin Pranz wrote: >> Am 27.03.2014 17:26, schrieb Philippe Gerum: >>> On 03/27/2014 04:02 PM, Henri Roosen wrote: >>>> >>>> On Thu, Mar 27, 2014 at 10:41 AM, Philippe Gerum <rpm@xenomai.org >>>> <mailto:rpm@xenomai.org>> wrote: >>>> >>>> On 03/27/2014 12:07 AM, Gilles Chanteperdrix wrote: >>>> >>>> On 03/26/2014 05:49 PM, Henri Roosen wrote: >>>> >>>> On Tue, Mar 25, 2014 at 12:45 PM, Erwin Pranz >>>> <erwin.pranz@sigmatek.at >>>> <mailto:erwin.pranz@sigmatek.at>>__wrote: >>>> >>>> Hello, >>>> >>>> I am using rtdm_mutex_lock/rtdm_mutex___unlock in a >>>> rtdm >>>> driver and I get a >>>> signal SIGDEBUG_MIGRATE_PRIOINV when I call the driver >>>> from a priority-0 >>>> thread.The signal is sent in xnsynch_release_thread >>>> when >>>> rescnt is 0 on a >>>> XNOTHER thread, which means that rescnt is not >>>> balanced. >>>> >>>> I think the reason why rescnt is not balanced is in >>>> rtdm_mutex_timedlock: >>>> when the owner of the mutex is NULL, the owner is >>>> changed to the calling >>>> thread, but rescnt is not increased. >>>> >>>> I suggest the following fix in rtdm_mutex_timedlock: >>>> - else if >>>> (likely(xnsynch_owner(&mutex->__synch_base) == NULL)) >>>> + else if >>>> (likely(xnsynch_owner(&mutex->__synch_base) == NULL)) { >>>> xnsynch_set_owner(&mutex->__synch_base, curr_thread); >>>> + if (xnthread_test_state(curr___thread, >>>> XNOTHER)) >>>> + >>>> xnthread_inc_rescnt(curr___thread); >>>> + } >>>> >>>> >>>> Hi Gilles, >>>> >>>> Could you maybe elaborate on this? It seems the resource >>>> counting is not in >>>> balance. It would be great to know if this the correct way >>>> to fix it. >>>> >>>> The problem should be easily reproducible by having just >>>> one >>>> shadowed >>>> priority 0 thread that somehow calls rtdm_mutex_lock >>>> directly followed by >>>> rtdm_mutex_unlock. This will trigger the code in >>>> ksrc/nucleus/synch.c that >>>> was added by commit-486afd15, which sends >>>> SIGDEBUG_MIGRATE_PRIOINV on >>>> Xenomai v2.6.2.1. It shouldn't send any signal in that >>>> case, >>>> as nothing is >>>> wrong. >>>> >>>> Note we are on Xenomai v2.6.2.1. Xenomai v2.6.3 sends (the >>>> better name for >>>> it) SIGDEBUG_RESCNT_IMBALANCE. >>>> >>>> >>>> To be completely frank, I expected Jan to answer this mail >>>> as this >>>> concerns RTDM. Anyway, I had a look into it, and: >>>> - the RTDM skin is not alone in this case, several skins >>>> implement the >>>> same pattern (if owner == NULL, set owner, then return); >>>> - adding calls to xnthread_inc_rescnt everywhere does not seem >>>> right, I >>>> would prefer to move this inside some xnsynch_ service. The >>>> best >>>> candidate I found for that is xnsynch_set_owner, though I am >>>> not >>>> sure if >>>> this will work, especially with fastsynchs. >>>> >>>> >>>> >>>> Indeed this wouldn't. I suspect the skins mentioned did not have >>>> access to xnsynch_acquire(), and were not updated when the >>>> auto-relax feature was introduced for XNOTHER threads. >>>> >>>> We could provide a helper for fast grabbing a mutex which has no >>>> fast locking support shared with userland, saving a call to >>>> xnsynch_acquire() in the non-contended case, e.g. >>>> >>>> diff --git a/include/nucleus/synch.h b/include/nucleus/synch.h >>>> index 7deae21..87456ea 100644 >>>> --- a/include/nucleus/synch.h >>>> +++ b/include/nucleus/synch.h >>>> @@ -192,6 +192,22 @@ static inline void xnsynch_set_owner(struct >>>> xnsynch *synch, >>>> synch->owner = thread; >>>> } >>>> >>>> +static inline int xnsynch_fast_grab(struct xnsynch *synch, >>>> + struct xnthread *thread) >>>> +{ >>>> + XENO_BUGON(NUCLEUS, xnsynch_fastlock_p(synch)); >>>> + >>>> + if (likely(xnsynch_owner(synch) != NULL)) >>>> + return 0; >>>> + >>>> + xnsynch_set_owner(synch, thread); >>>> + >>>> + if (xnthread_test_state(thread, XNOTHER)) >>>> + xnthread_inc_rescnt(thread); >>>> + >>>> + return 1; >>>> +} >>>> + >>>> static inline void xnsynch_register_cleanup(__struct xnsynch >>>> *synch, >>>> void (*handler)(struct >>>> xnsynch *)) >>>> { >>>> diff --git a/ksrc/skins/rtdm/drvlib.c b/ksrc/skins/rtdm/drvlib.c >>>> index 391d45f..338005d 100644 >>>> --- a/ksrc/skins/rtdm/drvlib.c >>>> +++ b/ksrc/skins/rtdm/drvlib.c >>>> @@ -1538,9 +1538,7 @@ int rtdm_mutex_timedlock(rtdm___mutex_t >>>> *mutex, nanosecs_rel_t timeout, >>>> if (unlikely(xnsynch_test_flags(&__mutex->synch_base, >>>> RTDM_SYNCH_DELETED))) >>>> err = -EIDRM; >>>> >>>> - else if (likely(xnsynch_owner(&mutex->__synch_base) == >>>> NULL)) >>>> - xnsynch_set_owner(&mutex->__synch_base, >>>> curr_thread); >>>> - else { >>>> + else if (!xnsynch_fast_grab(&mutex->__synch_base, >>>> curr_thread)) { >>>> /* Redefinition to clarify XENO_ASSERT output */ >>>> #define mutex_owner >>>> xnsynch_owner(&mutex->synch___base) >>>> XENO_ASSERT(RTDM, mutex_owner != curr_thread, >>>> >>>> The VxWorks and VRTX skins would have to use it as well. >>>> >>>> >>>> Thanks for the replies! We'll give this patch a try for the RTDM case. >>>> >>> Minor points, but a compiler hint above was inverted and eventually >>> useless, and the naming not descriptive enough. Take #2: >>> >>> diff --git a/include/nucleus/synch.h b/include/nucleus/synch.h >>> index 7deae21..09ebdbf 100644 >>> --- a/include/nucleus/synch.h >>> +++ b/include/nucleus/synch.h >>> @@ -192,6 +192,22 @@ static inline void xnsynch_set_owner(struct >>> xnsynch *synch, >>> synch->owner = thread; >>> } >>> +static inline int xnsynch_try_grab(struct xnsynch *synch, >>> + struct xnthread *thread) >>> +{ >>> + XENO_BUGON(NUCLEUS, xnsynch_fastlock_p(synch)); >>> + >>> + if (xnsynch_owner(synch) != NULL) >>> + return 0; >>> + >>> + xnsynch_set_owner(synch, thread); >>> + >>> + if (xnthread_test_state(thread, XNOTHER)) >>> + xnthread_inc_rescnt(thread); >>> + >>> + return 1; >>> +} >>> + >>> static inline void xnsynch_register_cleanup(struct xnsynch *synch, >>> void (*handler)(struct xnsynch *)) >>> { >>> diff --git a/ksrc/skins/rtdm/drvlib.c b/ksrc/skins/rtdm/drvlib.c >>> index 391d45f..aa85d11 100644 >>> --- a/ksrc/skins/rtdm/drvlib.c >>> +++ b/ksrc/skins/rtdm/drvlib.c >>> @@ -1538,9 +1538,7 @@ int rtdm_mutex_timedlock(rtdm_mutex_t *mutex, >>> nanosecs_rel_t timeout, >>> if (unlikely(xnsynch_test_flags(&mutex->synch_base, >>> RTDM_SYNCH_DELETED))) >>> err = -EIDRM; >>> - else if (likely(xnsynch_owner(&mutex->synch_base) == NULL)) >>> - xnsynch_set_owner(&mutex->synch_base, curr_thread); >>> - else { >>> + else if (!xnsynch_try_grab(&mutex->synch_base, curr_thread)) { >>> /* Redefinition to clarify XENO_ASSERT output */ >>> #define mutex_owner xnsynch_owner(&mutex->synch_base) >>> XENO_ASSERT(RTDM, mutex_owner != curr_thread, >>> >> Hello, >> >> I tried this patch, but it did not compile: >> In file included from include/xenomai/nucleus/registry.h:33:0, >> from include/xenomai/nucleus/thread.h:160, >> from include/xenomai/nucleus/sched.h:31, >> from include/xenomai/nucleus/pod.h:34, >> from include/xenomai/nucleus/xenomai.h:23, >> from include/xenomai/rtdm/rtdm_driver.h:37, >> from drivers/xenomai/can/rtcan_internal.h:30, >> from drivers/xenomai/can/rtcan_dev.c:32: >> include/xenomai/nucleus/synch.h: In function 'xnsynch_try_grab': >> include/xenomai/nucleus/synch.h:205:2: error: implicit declaration of >> function 'xnthread_test_state' [-Werror=implicit-function-declaration] >> include/xenomai/nucleus/synch.h:206:3: error: implicit declaration of >> function 'xnthread_inc_rescnt' [-Werror=implicit-function-declaration] >> >> I think there is a circular dependency problem in the header files: >> - thread.h includes sync.h via registry.h at a point before >> xnthread_test_state/xnthread_inc_rescnt is defined >> - synch.h needs the defines from thread.h >> (xnthread_test_state/xnthread_inc_rescnt) >> >> I could not resolve this dependency >> > Then let's make this a thread-based helper, this is logically correct too, and there can't be any ambiguity with respect to which kind of resources can be possibly grabbed by a core thread (and this is better than messing up with macros to postpone the references). > > diff --git a/include/nucleus/thread.h b/include/nucleus/thread.h > index 4a740a0..aca5f90 100644 > --- a/include/nucleus/thread.h > +++ b/include/nucleus/thread.h > @@ -436,6 +436,22 @@ struct xnthread *xnthread_lookup(xnhandle_t threadh) > return (thread && xnthread_handle(thread) == threadh) ? thread : NULL; > } > > +static inline int xnthread_try_grab(struct xnthread *thread, > + struct xnsynch *synch) > +{ > + XENO_BUGON(NUCLEUS, xnsynch_fastlock_p(synch)); > + > + if (xnsynch_owner(synch) != NULL) > + return 0; > + > + xnsynch_set_owner(synch, thread); > + > + if (xnthread_test_state(thread, XNOTHER)) > + xnthread_inc_rescnt(thread); > + > + return 1; > +} > + > #ifdef __cplusplus > extern "C" { > #endif > diff --git a/ksrc/skins/rtdm/drvlib.c b/ksrc/skins/rtdm/drvlib.c > index 391d45f..2e0f14b 100644 > --- a/ksrc/skins/rtdm/drvlib.c > +++ b/ksrc/skins/rtdm/drvlib.c > @@ -1538,9 +1538,7 @@ int rtdm_mutex_timedlock(rtdm_mutex_t *mutex, nanosecs_rel_t timeout, > if (unlikely(xnsynch_test_flags(&mutex->synch_base, > RTDM_SYNCH_DELETED))) > err = -EIDRM; > - else if (likely(xnsynch_owner(&mutex->synch_base) == NULL)) > - xnsynch_set_owner(&mutex->synch_base, curr_thread); > - else { > + else if (!xnthread_try_grab(curr_thread, &mutex->synch_base)) { > /* Redefinition to clarify XENO_ASSERT output */ > #define mutex_owner xnsynch_owner(&mutex->synch_base) > XENO_ASSERT(RTDM, mutex_owner != curr_thread, > Hello Philippe, This patch works and SIGDEBUG_MIGRATE_PRIOINV is not sent. Thanks for your help, Erwin ^ permalink raw reply [flat|nested] 16+ messages in thread
end of thread, other threads:[~2014-03-28 11:14 UTC | newest] Thread overview: 16+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2014-03-25 11:45 [Xenomai] Rescnt imbalance in rtdm_mutex_timedlock Erwin Pranz 2014-03-26 16:49 ` Henri Roosen 2014-03-26 23:07 ` Gilles Chanteperdrix 2014-03-27 9:41 ` Philippe Gerum 2014-03-27 15:02 ` Henri Roosen 2014-03-27 16:26 ` Philippe Gerum 2014-03-27 18:04 ` Jan Kiszka 2014-03-27 18:24 ` Philippe Gerum 2014-03-27 18:31 ` Jan Kiszka 2014-03-27 18:39 ` Gilles Chanteperdrix 2014-03-27 20:18 ` Lowell Gilbert 2014-03-28 8:32 ` Philippe Gerum 2014-03-27 18:35 ` Gilles Chanteperdrix 2014-03-28 9:52 ` Erwin Pranz 2014-03-28 10:14 ` Philippe Gerum 2014-03-28 11:14 ` Erwin Pranz
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.