* [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: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 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 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.