[Xenomai] [PATCH] rtdm: get spinlocks to lock the scheduler

[Xenomai] [PATCH] rtdm: get spinlocks to lock the scheduler

[Gilles Chanteperdrix]

In a context where callbacks are called with spinlocks held, it is not
possible for drivers callbacks to wake up threads without holding any
spinlock. So, we need a mechanism to lock the scheduler when a spinlock
is grabbed. As xnpod_lock_sched/xnpod_unlock_sched may be a bit heavy
weight for this case, we implement new nucleus services xnpod_spin_locked
and xnpod_spin_unlocked.
---
 include/nucleus/pod.h      |   20 ++++++++++++++++++--
 include/nucleus/sched.h    |    2 ++
 include/rtdm/rtdm_driver.h |   22 +++++++++++++++++-----
 3 files changed, 37 insertions(+), 7 deletions(-)

diff --git a/include/nucleus/pod.h b/include/nucleus/pod.h
index 06361ff..32e6b01 100644
--- a/include/nucleus/pod.h
+++ b/include/nucleus/pod.h
@@ -277,11 +277,11 @@ static inline void xnpod_schedule(void)
 	 * unlocked context switch.
 	 */
 #if XENO_DEBUG(NUCLEUS)
-	if (testbits(sched->status | sched->lflags, XNKCOUT|XNINIRQ|XNSWLOCK))
+	if (testbits(sched->status | sched->lflags, XNKCOUT|XNINIRQ|XNSWLOCK|XNHELDSPIN))
 		return;
 #else /* !XENO_DEBUG(NUCLEUS) */
 	if (testbits(sched->status | sched->lflags,
-		     XNKCOUT|XNINIRQ|XNSWLOCK|XNRESCHED) != XNRESCHED)
+		     XNKCOUT|XNINIRQ|XNSWLOCK|XNHELDSPIN|XNRESCHED) != XNRESCHED)
 		return;
 #endif /* !XENO_DEBUG(NUCLEUS) */
 
@@ -332,6 +332,22 @@ static inline void xnpod_unlock_sched(void)
 	xnlock_put_irqrestore(&nklock, s);
 }
 
+static inline void xnpod_spin_locked(void)
+{
+	struct xnsched *sched = xnpod_current_sched();
+	if (sched->held_spinlocks++ == 0)
+		__setbits(sched->lflags, XNHELDSPIN);
+}
+
+static inline void xnpod_spin_unlocked(void)
+{
+	struct xnsched *sched = xnpod_current_sched();
+	if (--sched->held_spinlocks == 0) {
+		__clrbits(sched->lflags, XNHELDSPIN);
+		xnpod_schedule();
+	}
+}
+
 void xnpod_fire_callouts(xnqueue_t *hookq,
 			 xnthread_t *thread);
 
diff --git a/include/nucleus/sched.h b/include/nucleus/sched.h
index 6ce9228..367c02a 100644
--- a/include/nucleus/sched.h
+++ b/include/nucleus/sched.h
@@ -47,6 +47,7 @@
 #define XNHTICK		0x00008000	/* Host tick pending  */
 #define XNINIRQ		0x00004000	/* In IRQ handling context */
 #define XNHDEFER	0x00002000	/* Host tick deferred */
+#define	XNHELDSPIN	0x00001000	/* Spinlocks held */
 
 /* Sched RPI status flags */
 #define XNRPICK		0x80000000	/* Check RPI state */
@@ -82,6 +83,7 @@ typedef struct xnsched {
 
 	xntimerq_t timerqueue;		/* !< Core timer queue. */
 	volatile unsigned inesting;	/*!< Interrupt nesting level. */
+	unsigned held_spinlocks;	/*!< Number of spinlocks held by curr */
 	struct xntimer htimer;		/*!< Host timer. */
 	struct xnthread *zombie;
 	struct xnthread rootcb;		/*!< Root thread control block. */
diff --git a/include/rtdm/rtdm_driver.h b/include/rtdm/rtdm_driver.h
index 3006d59..a8fa471 100644
--- a/include/rtdm/rtdm_driver.h
+++ b/include/rtdm/rtdm_driver.h
@@ -730,6 +730,7 @@ typedef unsigned long rtdm_lockctx_t;
 	do {							\
 		XENO_BUGON(RTDM, !rthal_local_irq_disabled());	\
 		rthal_spin_lock(lock);				\
+		xnpod_spin_locked();				\
 	} while (0)
 #endif
 
@@ -749,7 +750,11 @@ typedef unsigned long rtdm_lockctx_t;
  *
  * Rescheduling: never.
  */
-#define rtdm_lock_put(lock)	rthal_spin_unlock(lock)
+#define rtdm_lock_put(lock)					\
+	do {							\
+		rthal_spin_unlock(lock);			\
+		xnpod_spin_unlocked();				\
+	} while (0)
 
 /**
  * Acquire lock and disable preemption
@@ -768,8 +773,11 @@ typedef unsigned long rtdm_lockctx_t;
  *
  * Rescheduling: never.
  */
-#define rtdm_lock_get_irqsave(lock, context)	\
-	rthal_spin_lock_irqsave(lock, context)
+#define rtdm_lock_get_irqsave(lock, context)				\
+	do {								\
+		rthal_spin_lock_irqsave(lock, context);			\
+		xnpod_spin_locked();					\
+	} while (0)
 
 /**
  * Release lock and restore preemption state
@@ -788,8 +796,12 @@ typedef unsigned long rtdm_lockctx_t;
  *
  * Rescheduling: possible.
  */
-#define rtdm_lock_put_irqrestore(lock, context)	\
-	rthal_spin_unlock_irqrestore(lock, context)
+#define rtdm_lock_put_irqrestore(lock, context)			\
+	do {							\
+		rthal_spin_unlock(lock);			\
+		xnpod_spin_unlocked();				\
+		rthal_local_irq_restore(context);		\
+	} while (0)
 
 /**
  * Disable preemption locally
-- 
1.7.2.5

Re: [Xenomai] [PATCH] rtdm: get spinlocks to lock the scheduler

[Philippe Gerum]

On 10/11/2012 08:22 AM, Gilles Chanteperdrix wrote:
> In a context where callbacks are called with spinlocks held, it is not
> possible for drivers callbacks to wake up threads without holding any
> spinlock. So, we need a mechanism to lock the scheduler when a spinlock
> is grabbed. As xnpod_lock_sched/xnpod_unlock_sched may be a bit heavy
> weight for this case, we implement new nucleus services xnpod_spin_locked
> and xnpod_spin_unlocked.

The naming is error-prone, this could be interpreted as a test. Besides,
we don't have to explicitly refer to spinlocks, this service is
basically a preemption disabling feature. xnpod_disable/enable_preempt
would reflect this.


> ---
>  include/nucleus/pod.h      |   20 ++++++++++++++++++--
>  include/nucleus/sched.h    |    2 ++
>  include/rtdm/rtdm_driver.h |   22 +++++++++++++++++-----
>  3 files changed, 37 insertions(+), 7 deletions(-)
> 
> diff --git a/include/nucleus/pod.h b/include/nucleus/pod.h
> index 06361ff..32e6b01 100644
> --- a/include/nucleus/pod.h
> +++ b/include/nucleus/pod.h
> @@ -277,11 +277,11 @@ static inline void xnpod_schedule(void)
>  	 * unlocked context switch.
>  	 */
>  #if XENO_DEBUG(NUCLEUS)
> -	if (testbits(sched->status | sched->lflags, XNKCOUT|XNINIRQ|XNSWLOCK))
> +	if (testbits(sched->status | sched->lflags, XNKCOUT|XNINIRQ|XNSWLOCK|XNHELDSPIN))

XNHELD exists and has a significantly different meaning, so let's pick
something else than XNHELDSPIN which would refer to preemption.
Actually, we have too many of these XN*LOCK*.

- XNLOCK refers to the common naming for the scheduler locking services
exposed by traditional RTOS, so this is probably better to keep it that
way. It can't be shared with internal preemption control flag.

- XNSWLOCK seems to be shareable with the new preemption control flag,
with no disable count tracking though. Typically, XNSWLOCK &&
disable_count == 0 would mean that preemption is disabled for the
ongoing unlocked context switch.


>   * Rescheduling: never.
>   */
> -#define rtdm_lock_put(lock)	rthal_spin_unlock(lock)
> +#define rtdm_lock_put(lock)					\
> +	do {							\
> +		rthal_spin_unlock(lock);			\
> +		xnpod_spin_unlocked();				\

This is a general property of xnlocks, this is not RTDM-specific. At any
rate, rescheduling when holding anything else than the nucleus lock is a
bug, so we can just manipulate the preemption counter from the xnlock
helpers directly. Any reference to nklock is constant, so gcc can detect
and optimize out the proper branch in these macros.

-- 
Philippe.

Re: [Xenomai] [PATCH] rtdm: get spinlocks to lock the scheduler

[Gilles Chanteperdrix]

On 10/11/2012 10:15 AM, Philippe Gerum wrote:
> On 10/11/2012 08:22 AM, Gilles Chanteperdrix wrote:
>> In a context where callbacks are called with spinlocks held, it is not
>> possible for drivers callbacks to wake up threads without holding any
>> spinlock. So, we need a mechanism to lock the scheduler when a spinlock
>> is grabbed. As xnpod_lock_sched/xnpod_unlock_sched may be a bit heavy
>> weight for this case, we implement new nucleus services xnpod_spin_locked
>> and xnpod_spin_unlocked.
> 
> The naming is error-prone, this could be interpreted as a test. Besides,
> we don't have to explicitly refer to spinlocks, this service is
> basically a preemption disabling feature. xnpod_disable/enable_preempt
> would reflect this.
> 
> 
>> ---
>>  include/nucleus/pod.h      |   20 ++++++++++++++++++--
>>  include/nucleus/sched.h    |    2 ++
>>  include/rtdm/rtdm_driver.h |   22 +++++++++++++++++-----
>>  3 files changed, 37 insertions(+), 7 deletions(-)
>>
>> diff --git a/include/nucleus/pod.h b/include/nucleus/pod.h
>> index 06361ff..32e6b01 100644
>> --- a/include/nucleus/pod.h
>> +++ b/include/nucleus/pod.h
>> @@ -277,11 +277,11 @@ static inline void xnpod_schedule(void)
>>  	 * unlocked context switch.
>>  	 */
>>  #if XENO_DEBUG(NUCLEUS)
>> -	if (testbits(sched->status | sched->lflags, XNKCOUT|XNINIRQ|XNSWLOCK))
>> +	if (testbits(sched->status | sched->lflags, XNKCOUT|XNINIRQ|XNSWLOCK|XNHELDSPIN))
> 
> XNHELD exists and has a significantly different meaning, so let's pick
> something else than XNHELDSPIN which would refer to preemption.
> Actually, we have too many of these XN*LOCK*.
> 
> - XNLOCK refers to the common naming for the scheduler locking services
> exposed by traditional RTOS, so this is probably better to keep it that
> way. It can't be shared with internal preemption control flag.
> 
> - XNSWLOCK seems to be shareable with the new preemption control flag,
> with no disable count tracking though. Typically, XNSWLOCK &&
> disable_count == 0 would mean that preemption is disabled for the
> ongoing unlocked context switch.
> 
> 
>>   * Rescheduling: never.
>>   */
>> -#define rtdm_lock_put(lock)	rthal_spin_unlock(lock)
>> +#define rtdm_lock_put(lock)					\
>> +	do {							\
>> +		rthal_spin_unlock(lock);			\
>> +		xnpod_spin_unlocked();				\
> 
> This is a general property of xnlocks, this is not RTDM-specific. At any
> rate, rescheduling when holding anything else than the nucleus lock is a
> bug, so we can just manipulate the preemption counter from the xnlock
> helpers directly. Any reference to nklock is constant, so gcc can detect
> and optimize out the proper branch in these macros.

The thing is:
- RTDM does not use xnlocks
- we want to avoid this for the nklock

so, if we build this service into xnlock, we either have to add a test
if lock != &nklock
or create the nklock_get*/nklock_put* services

Ok, I will rethink about this and post a new patch, as I think we can
fold this with xnpod_lock_sched/xnpod_unlock_sched, as I said in another
mail.

-- 
					    Gilles.

Re: [Xenomai] [PATCH] rtdm: get spinlocks to lock the scheduler

[Philippe Gerum]

On 10/11/2012 10:19 AM, Gilles Chanteperdrix wrote:
> On 10/11/2012 10:15 AM, Philippe Gerum wrote:
>> On 10/11/2012 08:22 AM, Gilles Chanteperdrix wrote:
>>> In a context where callbacks are called with spinlocks held, it is not
>>> possible for drivers callbacks to wake up threads without holding any
>>> spinlock. So, we need a mechanism to lock the scheduler when a spinlock
>>> is grabbed. As xnpod_lock_sched/xnpod_unlock_sched may be a bit heavy
>>> weight for this case, we implement new nucleus services xnpod_spin_locked
>>> and xnpod_spin_unlocked.
>>
>> The naming is error-prone, this could be interpreted as a test. Besides,
>> we don't have to explicitly refer to spinlocks, this service is
>> basically a preemption disabling feature. xnpod_disable/enable_preempt
>> would reflect this.
>>
>>
>>> ---
>>>  include/nucleus/pod.h      |   20 ++++++++++++++++++--
>>>  include/nucleus/sched.h    |    2 ++
>>>  include/rtdm/rtdm_driver.h |   22 +++++++++++++++++-----
>>>  3 files changed, 37 insertions(+), 7 deletions(-)
>>>
>>> diff --git a/include/nucleus/pod.h b/include/nucleus/pod.h
>>> index 06361ff..32e6b01 100644
>>> --- a/include/nucleus/pod.h
>>> +++ b/include/nucleus/pod.h
>>> @@ -277,11 +277,11 @@ static inline void xnpod_schedule(void)
>>>  	 * unlocked context switch.
>>>  	 */
>>>  #if XENO_DEBUG(NUCLEUS)
>>> -	if (testbits(sched->status | sched->lflags, XNKCOUT|XNINIRQ|XNSWLOCK))
>>> +	if (testbits(sched->status | sched->lflags, XNKCOUT|XNINIRQ|XNSWLOCK|XNHELDSPIN))
>>
>> XNHELD exists and has a significantly different meaning, so let's pick
>> something else than XNHELDSPIN which would refer to preemption.
>> Actually, we have too many of these XN*LOCK*.
>>
>> - XNLOCK refers to the common naming for the scheduler locking services
>> exposed by traditional RTOS, so this is probably better to keep it that
>> way. It can't be shared with internal preemption control flag.
>>
>> - XNSWLOCK seems to be shareable with the new preemption control flag,
>> with no disable count tracking though. Typically, XNSWLOCK &&
>> disable_count == 0 would mean that preemption is disabled for the
>> ongoing unlocked context switch.
>>
>>
>>>   * Rescheduling: never.
>>>   */
>>> -#define rtdm_lock_put(lock)	rthal_spin_unlock(lock)
>>> +#define rtdm_lock_put(lock)					\
>>> +	do {							\
>>> +		rthal_spin_unlock(lock);			\
>>> +		xnpod_spin_unlocked();				\
>>
>> This is a general property of xnlocks, this is not RTDM-specific. At any
>> rate, rescheduling when holding anything else than the nucleus lock is a
>> bug, so we can just manipulate the preemption counter from the xnlock
>> helpers directly. Any reference to nklock is constant, so gcc can detect
>> and optimize out the proper branch in these macros.
> 
> The thing is:
> - RTDM does not use xnlocks

then fix the RTDM code, it has to use the platform locks. The HAL layer
in its current form is about to vanish in 3.x.

> - we want to avoid this for the nklock
> 
> so, if we build this service into xnlock, we either have to add a test
> if lock != &nklock
> or create the nklock_get*/nklock_put* services
> 

Yes, this is why I mentioned the optimizer. gcc will drop the useless
branch away, &nklock is a constant reference.

> Ok, I will rethink about this and post a new patch, as I think we can
> fold this with xnpod_lock_sched/xnpod_unlock_sched, as I said in another
> mail.
> 


-- 
Philippe.

Re: [Xenomai] [PATCH] rtdm: get spinlocks to lock the scheduler

[Philippe Gerum]

On 10/11/2012 10:20 AM, Philippe Gerum wrote:
> On 10/11/2012 10:19 AM, Gilles Chanteperdrix wrote:
>> On 10/11/2012 10:15 AM, Philippe Gerum wrote:
>>> On 10/11/2012 08:22 AM, Gilles Chanteperdrix wrote:
>>>> In a context where callbacks are called with spinlocks held, it is not
>>>> possible for drivers callbacks to wake up threads without holding any
>>>> spinlock. So, we need a mechanism to lock the scheduler when a spinlock
>>>> is grabbed. As xnpod_lock_sched/xnpod_unlock_sched may be a bit heavy
>>>> weight for this case, we implement new nucleus services xnpod_spin_locked
>>>> and xnpod_spin_unlocked.
>>>
>>> The naming is error-prone, this could be interpreted as a test. Besides,
>>> we don't have to explicitly refer to spinlocks, this service is
>>> basically a preemption disabling feature. xnpod_disable/enable_preempt
>>> would reflect this.
>>>
>>>
>>>> ---
>>>>  include/nucleus/pod.h      |   20 ++++++++++++++++++--
>>>>  include/nucleus/sched.h    |    2 ++
>>>>  include/rtdm/rtdm_driver.h |   22 +++++++++++++++++-----
>>>>  3 files changed, 37 insertions(+), 7 deletions(-)
>>>>
>>>> diff --git a/include/nucleus/pod.h b/include/nucleus/pod.h
>>>> index 06361ff..32e6b01 100644
>>>> --- a/include/nucleus/pod.h
>>>> +++ b/include/nucleus/pod.h
>>>> @@ -277,11 +277,11 @@ static inline void xnpod_schedule(void)
>>>>  	 * unlocked context switch.
>>>>  	 */
>>>>  #if XENO_DEBUG(NUCLEUS)
>>>> -	if (testbits(sched->status | sched->lflags, XNKCOUT|XNINIRQ|XNSWLOCK))
>>>> +	if (testbits(sched->status | sched->lflags, XNKCOUT|XNINIRQ|XNSWLOCK|XNHELDSPIN))
>>>
>>> XNHELD exists and has a significantly different meaning, so let's pick
>>> something else than XNHELDSPIN which would refer to preemption.
>>> Actually, we have too many of these XN*LOCK*.
>>>
>>> - XNLOCK refers to the common naming for the scheduler locking services
>>> exposed by traditional RTOS, so this is probably better to keep it that
>>> way. It can't be shared with internal preemption control flag.
>>>
>>> - XNSWLOCK seems to be shareable with the new preemption control flag,
>>> with no disable count tracking though. Typically, XNSWLOCK &&
>>> disable_count == 0 would mean that preemption is disabled for the
>>> ongoing unlocked context switch.
>>>
>>>
>>>>   * Rescheduling: never.
>>>>   */
>>>> -#define rtdm_lock_put(lock)	rthal_spin_unlock(lock)
>>>> +#define rtdm_lock_put(lock)					\
>>>> +	do {							\
>>>> +		rthal_spin_unlock(lock);			\
>>>> +		xnpod_spin_unlocked();				\
>>>
>>> This is a general property of xnlocks, this is not RTDM-specific. At any
>>> rate, rescheduling when holding anything else than the nucleus lock is a
>>> bug, so we can just manipulate the preemption counter from the xnlock
>>> helpers directly. Any reference to nklock is constant, so gcc can detect
>>> and optimize out the proper branch in these macros.
>>
>> The thing is:
>> - RTDM does not use xnlocks
> 
> then fix the RTDM code, it has to use the platform locks. The HAL layer
> in its current form is about to vanish in 3.x.
>

Crap, RTDM does not want recursive locks. So we have to duplicate the
code insertion into both RTDM like you did, and the xnlock interface.


>> - we want to avoid this for the nklock
>>
>> so, if we build this service into xnlock, we either have to add a test
>> if lock != &nklock
>> or create the nklock_get*/nklock_put* services
>>
> 
> Yes, this is why I mentioned the optimizer. gcc will drop the useless
> branch away, &nklock is a constant reference.
> 
>> Ok, I will rethink about this and post a new patch, as I think we can
>> fold this with xnpod_lock_sched/xnpod_unlock_sched, as I said in another
>> mail.
>>
> 
> 


-- 
Philippe.

Re: [Xenomai] [PATCH] rtdm: get spinlocks to lock the scheduler

[Jan Kiszka]

On 2012-10-11 10:27, Philippe Gerum wrote:
> On 10/11/2012 10:20 AM, Philippe Gerum wrote:
>> On 10/11/2012 10:19 AM, Gilles Chanteperdrix wrote:
>>> On 10/11/2012 10:15 AM, Philippe Gerum wrote:
>>>> On 10/11/2012 08:22 AM, Gilles Chanteperdrix wrote:
>>>>> In a context where callbacks are called with spinlocks held, it is not
>>>>> possible for drivers callbacks to wake up threads without holding any
>>>>> spinlock. So, we need a mechanism to lock the scheduler when a spinlock
>>>>> is grabbed. As xnpod_lock_sched/xnpod_unlock_sched may be a bit heavy
>>>>> weight for this case, we implement new nucleus services xnpod_spin_locked
>>>>> and xnpod_spin_unlocked.
>>>>
>>>> The naming is error-prone, this could be interpreted as a test. Besides,
>>>> we don't have to explicitly refer to spinlocks, this service is
>>>> basically a preemption disabling feature. xnpod_disable/enable_preempt
>>>> would reflect this.
>>>>
>>>>
>>>>> ---
>>>>>  include/nucleus/pod.h      |   20 ++++++++++++++++++--
>>>>>  include/nucleus/sched.h    |    2 ++
>>>>>  include/rtdm/rtdm_driver.h |   22 +++++++++++++++++-----
>>>>>  3 files changed, 37 insertions(+), 7 deletions(-)
>>>>>
>>>>> diff --git a/include/nucleus/pod.h b/include/nucleus/pod.h
>>>>> index 06361ff..32e6b01 100644
>>>>> --- a/include/nucleus/pod.h
>>>>> +++ b/include/nucleus/pod.h
>>>>> @@ -277,11 +277,11 @@ static inline void xnpod_schedule(void)
>>>>>  	 * unlocked context switch.
>>>>>  	 */
>>>>>  #if XENO_DEBUG(NUCLEUS)
>>>>> -	if (testbits(sched->status | sched->lflags, XNKCOUT|XNINIRQ|XNSWLOCK))
>>>>> +	if (testbits(sched->status | sched->lflags, XNKCOUT|XNINIRQ|XNSWLOCK|XNHELDSPIN))
>>>>
>>>> XNHELD exists and has a significantly different meaning, so let's pick
>>>> something else than XNHELDSPIN which would refer to preemption.
>>>> Actually, we have too many of these XN*LOCK*.
>>>>
>>>> - XNLOCK refers to the common naming for the scheduler locking services
>>>> exposed by traditional RTOS, so this is probably better to keep it that
>>>> way. It can't be shared with internal preemption control flag.
>>>>
>>>> - XNSWLOCK seems to be shareable with the new preemption control flag,
>>>> with no disable count tracking though. Typically, XNSWLOCK &&
>>>> disable_count == 0 would mean that preemption is disabled for the
>>>> ongoing unlocked context switch.
>>>>
>>>>
>>>>>   * Rescheduling: never.
>>>>>   */
>>>>> -#define rtdm_lock_put(lock)	rthal_spin_unlock(lock)
>>>>> +#define rtdm_lock_put(lock)					\
>>>>> +	do {							\
>>>>> +		rthal_spin_unlock(lock);			\
>>>>> +		xnpod_spin_unlocked();				\
>>>>
>>>> This is a general property of xnlocks, this is not RTDM-specific. At any
>>>> rate, rescheduling when holding anything else than the nucleus lock is a
>>>> bug, so we can just manipulate the preemption counter from the xnlock
>>>> helpers directly. Any reference to nklock is constant, so gcc can detect
>>>> and optimize out the proper branch in these macros.
>>>
>>> The thing is:
>>> - RTDM does not use xnlocks
>>
>> then fix the RTDM code, it has to use the platform locks. The HAL layer
>> in its current form is about to vanish in 3.x.
>>
> 
> Crap, RTDM does not want recursive locks. So we have to duplicate the
> code insertion into both RTDM like you did, and the xnlock interface.

Err, sorry for the other posting, sequential mail processing...

Jan

-- 
Siemens AG, Corporate Technology, CT RTC ITP SDP-DE
Corporate Competence Center Embedded Linux

Re: [Xenomai] [PATCH] rtdm: get spinlocks to lock the scheduler

[Jan Kiszka]

On 2012-10-11 10:20, Philippe Gerum wrote:
> On 10/11/2012 10:19 AM, Gilles Chanteperdrix wrote:
>> On 10/11/2012 10:15 AM, Philippe Gerum wrote:
>>> On 10/11/2012 08:22 AM, Gilles Chanteperdrix wrote:
>>>> In a context where callbacks are called with spinlocks held, it is not
>>>> possible for drivers callbacks to wake up threads without holding any
>>>> spinlock. So, we need a mechanism to lock the scheduler when a spinlock
>>>> is grabbed. As xnpod_lock_sched/xnpod_unlock_sched may be a bit heavy
>>>> weight for this case, we implement new nucleus services xnpod_spin_locked
>>>> and xnpod_spin_unlocked.
>>>
>>> The naming is error-prone, this could be interpreted as a test. Besides,
>>> we don't have to explicitly refer to spinlocks, this service is
>>> basically a preemption disabling feature. xnpod_disable/enable_preempt
>>> would reflect this.
>>>
>>>
>>>> ---
>>>>  include/nucleus/pod.h      |   20 ++++++++++++++++++--
>>>>  include/nucleus/sched.h    |    2 ++
>>>>  include/rtdm/rtdm_driver.h |   22 +++++++++++++++++-----
>>>>  3 files changed, 37 insertions(+), 7 deletions(-)
>>>>
>>>> diff --git a/include/nucleus/pod.h b/include/nucleus/pod.h
>>>> index 06361ff..32e6b01 100644
>>>> --- a/include/nucleus/pod.h
>>>> +++ b/include/nucleus/pod.h
>>>> @@ -277,11 +277,11 @@ static inline void xnpod_schedule(void)
>>>>  	 * unlocked context switch.
>>>>  	 */
>>>>  #if XENO_DEBUG(NUCLEUS)
>>>> -	if (testbits(sched->status | sched->lflags, XNKCOUT|XNINIRQ|XNSWLOCK))
>>>> +	if (testbits(sched->status | sched->lflags, XNKCOUT|XNINIRQ|XNSWLOCK|XNHELDSPIN))
>>>
>>> XNHELD exists and has a significantly different meaning, so let's pick
>>> something else than XNHELDSPIN which would refer to preemption.
>>> Actually, we have too many of these XN*LOCK*.
>>>
>>> - XNLOCK refers to the common naming for the scheduler locking services
>>> exposed by traditional RTOS, so this is probably better to keep it that
>>> way. It can't be shared with internal preemption control flag.
>>>
>>> - XNSWLOCK seems to be shareable with the new preemption control flag,
>>> with no disable count tracking though. Typically, XNSWLOCK &&
>>> disable_count == 0 would mean that preemption is disabled for the
>>> ongoing unlocked context switch.
>>>
>>>
>>>>   * Rescheduling: never.
>>>>   */
>>>> -#define rtdm_lock_put(lock)	rthal_spin_unlock(lock)
>>>> +#define rtdm_lock_put(lock)					\
>>>> +	do {							\
>>>> +		rthal_spin_unlock(lock);			\
>>>> +		xnpod_spin_unlocked();				\
>>>
>>> This is a general property of xnlocks, this is not RTDM-specific. At any
>>> rate, rescheduling when holding anything else than the nucleus lock is a
>>> bug, so we can just manipulate the preemption counter from the xnlock
>>> helpers directly. Any reference to nklock is constant, so gcc can detect
>>> and optimize out the proper branch in these macros.
>>
>> The thing is:
>> - RTDM does not use xnlocks
> 
> then fix the RTDM code, it has to use the platform locks. The HAL layer
> in its current form is about to vanish in 3.x.

RTDM shouldn't gain current nklock semantics (namely recursiveness, it's
unneeded for properly written drivers). So I'd prefer to export
preempt_disable/enable services and use them as in the above hunk.

Jan

-- 
Siemens AG, Corporate Technology, CT RTC ITP SDP-DE
Corporate Competence Center Embedded Linux

Re: [Xenomai] [PATCH] rtdm: get spinlocks to lock the scheduler

[Philippe Gerum]

On 10/11/2012 12:36 PM, Jan Kiszka wrote:
> On 2012-10-11 10:20, Philippe Gerum wrote:
>> On 10/11/2012 10:19 AM, Gilles Chanteperdrix wrote:
>>> On 10/11/2012 10:15 AM, Philippe Gerum wrote:
>>>> On 10/11/2012 08:22 AM, Gilles Chanteperdrix wrote:
>>>>> In a context where callbacks are called with spinlocks held, it is not
>>>>> possible for drivers callbacks to wake up threads without holding any
>>>>> spinlock. So, we need a mechanism to lock the scheduler when a spinlock
>>>>> is grabbed. As xnpod_lock_sched/xnpod_unlock_sched may be a bit heavy
>>>>> weight for this case, we implement new nucleus services xnpod_spin_locked
>>>>> and xnpod_spin_unlocked.
>>>>
>>>> The naming is error-prone, this could be interpreted as a test. Besides,
>>>> we don't have to explicitly refer to spinlocks, this service is
>>>> basically a preemption disabling feature. xnpod_disable/enable_preempt
>>>> would reflect this.
>>>>
>>>>
>>>>> ---
>>>>>  include/nucleus/pod.h      |   20 ++++++++++++++++++--
>>>>>  include/nucleus/sched.h    |    2 ++
>>>>>  include/rtdm/rtdm_driver.h |   22 +++++++++++++++++-----
>>>>>  3 files changed, 37 insertions(+), 7 deletions(-)
>>>>>
>>>>> diff --git a/include/nucleus/pod.h b/include/nucleus/pod.h
>>>>> index 06361ff..32e6b01 100644
>>>>> --- a/include/nucleus/pod.h
>>>>> +++ b/include/nucleus/pod.h
>>>>> @@ -277,11 +277,11 @@ static inline void xnpod_schedule(void)
>>>>>  	 * unlocked context switch.
>>>>>  	 */
>>>>>  #if XENO_DEBUG(NUCLEUS)
>>>>> -	if (testbits(sched->status | sched->lflags, XNKCOUT|XNINIRQ|XNSWLOCK))
>>>>> +	if (testbits(sched->status | sched->lflags, XNKCOUT|XNINIRQ|XNSWLOCK|XNHELDSPIN))
>>>>
>>>> XNHELD exists and has a significantly different meaning, so let's pick
>>>> something else than XNHELDSPIN which would refer to preemption.
>>>> Actually, we have too many of these XN*LOCK*.
>>>>
>>>> - XNLOCK refers to the common naming for the scheduler locking services
>>>> exposed by traditional RTOS, so this is probably better to keep it that
>>>> way. It can't be shared with internal preemption control flag.
>>>>
>>>> - XNSWLOCK seems to be shareable with the new preemption control flag,
>>>> with no disable count tracking though. Typically, XNSWLOCK &&
>>>> disable_count == 0 would mean that preemption is disabled for the
>>>> ongoing unlocked context switch.
>>>>
>>>>
>>>>>   * Rescheduling: never.
>>>>>   */
>>>>> -#define rtdm_lock_put(lock)	rthal_spin_unlock(lock)
>>>>> +#define rtdm_lock_put(lock)					\
>>>>> +	do {							\
>>>>> +		rthal_spin_unlock(lock);			\
>>>>> +		xnpod_spin_unlocked();				\
>>>>
>>>> This is a general property of xnlocks, this is not RTDM-specific. At any
>>>> rate, rescheduling when holding anything else than the nucleus lock is a
>>>> bug, so we can just manipulate the preemption counter from the xnlock
>>>> helpers directly. Any reference to nklock is constant, so gcc can detect
>>>> and optimize out the proper branch in these macros.
>>>
>>> The thing is:
>>> - RTDM does not use xnlocks
>>
>> then fix the RTDM code, it has to use the platform locks. The HAL layer
>> in its current form is about to vanish in 3.x.
> 
> RTDM shouldn't gain current nklock semantics (namely recursiveness, it's
> unneeded for properly written drivers). So I'd prefer to export
> preempt_disable/enable services and use them as in the above hunk.

I agree, allowing recursive locking in RTDM drivers in only one of its
implementations would kill portability in nasty ways.

> 
> Jan
> 


-- 
Philippe.