[Xenomai-core] [PATCH 0/3] Pull request: Build fix and RTDM improvements

[Xenomai-core] [PATCH 0/3] Pull request: Build fix and RTDM improvements

[Jan Kiszka]

Please pull from

    git://xenomai.org/xenomai-jki.git for-upstream


Jan Kiszka (3):
      nucleus: Add missing header for isspace
      RTDM: Instrument rtdm_lock_get for proper use
      RTDM: Avoid namespace pollution in RTDM_EXECUTE_ATOMICALLY

 include/rtdm/rtdm_driver.h |   22 +++++++++++++++-------
 ksrc/nucleus/intr.c        |    1 +
 2 files changed, 16 insertions(+), 7 deletions(-)

--
Siemens AG, Corporate Technology, CT SE 2
Corporate Competence Center Embedded Linux

[Xenomai-core] [PATCH 1/3] nucleus: Add missing header for isspace

[Jan Kiszka]

Signed-off-by: Jan Kiszka <jan.kiszka@domain.hid>
---

 ksrc/nucleus/intr.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/ksrc/nucleus/intr.c b/ksrc/nucleus/intr.c
index 81db8bc..97458f5 100644
--- a/ksrc/nucleus/intr.c
+++ b/ksrc/nucleus/intr.c
@@ -973,6 +973,7 @@ int xnintr_query_next(int irq, xnintr_iterator_t *iterator, char *name_buf)
 #ifdef CONFIG_PROC_FS
 
 #include <linux/proc_fs.h>
+#include <linux/ctype.h>
 
 static int format_irq_proc(unsigned int irq, char *str)
 {

[Xenomai-core] [PATCH 2/3] RTDM: Instrument rtdm_lock_get for proper use

[Jan Kiszka]

In case the user thinks rtdm_lock_get could be used like spin_lock or
messes up the IRQ protection for other reasons, catch this with a
XENO_BUGON.

Signed-off-by: Jan Kiszka <jan.kiszka@domain.hid>
---

 include/rtdm/rtdm_driver.h |    8 ++++++++
 1 files changed, 8 insertions(+), 0 deletions(-)

diff --git a/include/rtdm/rtdm_driver.h b/include/rtdm/rtdm_driver.h
index 058a9f8..fe42eea 100644
--- a/include/rtdm/rtdm_driver.h
+++ b/include/rtdm/rtdm_driver.h
@@ -655,7 +655,15 @@ typedef unsigned long rtdm_lockctx_t;
  *
  * Rescheduling: never.
  */
+#ifdef DOXYGEN_CPP /* Beautify doxygen output */
 #define rtdm_lock_get(lock)	rthal_spin_lock(lock)
+#else /* This is how it really works */
+#define rtdm_lock_get(lock)					\
+	do {							\
+		XENO_BUGON(RTDM, rthal_local_irq_test());	\
+		rthal_spin_lock(lock);				\
+	} while (0)
+#endif
 
 /**
  * Release lock without preemption restoration

[Xenomai-core] [PATCH 3/3] RTDM: Avoid namespace pollution in RTDM_EXECUTE_ATOMICALLY

[Jan Kiszka]

Signed-off-by: Jan Kiszka <jan.kiszka@domain.hid>
---

 include/rtdm/rtdm_driver.h |   14 +++++++-------
 1 files changed, 7 insertions(+), 7 deletions(-)

diff --git a/include/rtdm/rtdm_driver.h b/include/rtdm/rtdm_driver.h
index fe42eea..9680010 100644
--- a/include/rtdm/rtdm_driver.h
+++ b/include/rtdm/rtdm_driver.h
@@ -595,13 +595,13 @@ int rtdm_select_bind(int fd, rtdm_selector_t *selector,
 	<LEAVE_ATOMIC_SECTION>			\
 }
 #else /* This is how it really works */
-#define RTDM_EXECUTE_ATOMICALLY(code_block)	\
-{						\
-	spl_t s;				\
-						\
-	xnlock_get_irqsave(&nklock, s);		\
-	code_block;				\
-	xnlock_put_irqrestore(&nklock, s);	\
+#define RTDM_EXECUTE_ATOMICALLY(code_block)		\
+{							\
+	spl_t __rtdm_s;					\
+							\
+	xnlock_get_irqsave(&nklock, __rtdm_s);		\
+	code_block;					\
+	xnlock_put_irqrestore(&nklock, __rtdm_s);	\
 }
 #endif
 /** @} Global Lock across Scheduler Invocation */

Re: [Xenomai-core] [PATCH 2/3] RTDM: Instrument rtdm_lock_get for proper use

[Gilles Chanteperdrix]

Jan Kiszka wrote:
> In case the user thinks rtdm_lock_get could be used like spin_lock or
> messes up the IRQ protection for other reasons, catch this with a
> XENO_BUGON.
> 
> Signed-off-by: Jan Kiszka <jan.kiszka@domain.hid>
> ---
> 
>  include/rtdm/rtdm_driver.h |    8 ++++++++
>  1 files changed, 8 insertions(+), 0 deletions(-)
> 
> diff --git a/include/rtdm/rtdm_driver.h b/include/rtdm/rtdm_driver.h
> index 058a9f8..fe42eea 100644
> --- a/include/rtdm/rtdm_driver.h
> +++ b/include/rtdm/rtdm_driver.h
> @@ -655,7 +655,15 @@ typedef unsigned long rtdm_lockctx_t;
>   *
>   * Rescheduling: never.
>   */
> +#ifdef DOXYGEN_CPP /* Beautify doxygen output */
>  #define rtdm_lock_get(lock)	rthal_spin_lock(lock)
> +#else /* This is how it really works */
> +#define rtdm_lock_get(lock)					\
> +	do {							\
> +		XENO_BUGON(RTDM, rthal_local_irq_test());	\
> +		rthal_spin_lock(lock);				\
> +	} while (0)
> +#endif

Why is it a problem to call rthal_spin_lock with irqs off?

I also belived that you can avoid the #ifdef DOXYGEN_CPP.

-- 
					    Gilles.

Re: [Xenomai-core] [PATCH 2/3] RTDM: Instrument rtdm_lock_get for proper use

[Jan Kiszka]

Gilles Chanteperdrix wrote:
> Jan Kiszka wrote:
>> In case the user thinks rtdm_lock_get could be used like spin_lock or
>> messes up the IRQ protection for other reasons, catch this with a
>> XENO_BUGON.
>>
>> Signed-off-by: Jan Kiszka <jan.kiszka@domain.hid>
>> ---
>>
>>  include/rtdm/rtdm_driver.h |    8 ++++++++
>>  1 files changed, 8 insertions(+), 0 deletions(-)
>>
>> diff --git a/include/rtdm/rtdm_driver.h b/include/rtdm/rtdm_driver.h
>> index 058a9f8..fe42eea 100644
>> --- a/include/rtdm/rtdm_driver.h
>> +++ b/include/rtdm/rtdm_driver.h
>> @@ -655,7 +655,15 @@ typedef unsigned long rtdm_lockctx_t;
>>   *
>>   * Rescheduling: never.
>>   */
>> +#ifdef DOXYGEN_CPP /* Beautify doxygen output */
>>  #define rtdm_lock_get(lock)	rthal_spin_lock(lock)
>> +#else /* This is how it really works */
>> +#define rtdm_lock_get(lock)					\
>> +	do {							\
>> +		XENO_BUGON(RTDM, rthal_local_irq_test());	\
>> +		rthal_spin_lock(lock);				\
>> +	} while (0)
>> +#endif
> 
> Why is it a problem to call rthal_spin_lock with irqs off?

Did I messed it up again? I meant it is a problem to call it with irqs
*on*. Checking what rthal_local_irq_test() actually means... Hmm, I
still think it's correct. Maybe we should rename rthal_local_irq_test to
rthal_local_irq_enabled to clarify the usage.

> 
> I also belived that you can avoid the #ifdef DOXYGEN_CPP.

Haven't tried recently, but so far doxygen pulled the whole definition
into its documents, and I don't when the BUGON to appear there.

Jan

-- 
Siemens AG, Corporate Technology, CT SE 2
Corporate Competence Center Embedded Linux

Re: [Xenomai-core] [PATCH 2/3] RTDM: Instrument rtdm_lock_get for proper use

[Gilles Chanteperdrix]

Jan Kiszka wrote:
> Gilles Chanteperdrix wrote:
>> Jan Kiszka wrote:
>>> In case the user thinks rtdm_lock_get could be used like spin_lock or
>>> messes up the IRQ protection for other reasons, catch this with a
>>> XENO_BUGON.
>>>
>>> Signed-off-by: Jan Kiszka <jan.kiszka@domain.hid>
>>> ---
>>>
>>>  include/rtdm/rtdm_driver.h |    8 ++++++++
>>>  1 files changed, 8 insertions(+), 0 deletions(-)
>>>
>>> diff --git a/include/rtdm/rtdm_driver.h b/include/rtdm/rtdm_driver.h
>>> index 058a9f8..fe42eea 100644
>>> --- a/include/rtdm/rtdm_driver.h
>>> +++ b/include/rtdm/rtdm_driver.h
>>> @@ -655,7 +655,15 @@ typedef unsigned long rtdm_lockctx_t;
>>>   *
>>>   * Rescheduling: never.
>>>   */
>>> +#ifdef DOXYGEN_CPP /* Beautify doxygen output */
>>>  #define rtdm_lock_get(lock)	rthal_spin_lock(lock)
>>> +#else /* This is how it really works */
>>> +#define rtdm_lock_get(lock)					\
>>> +	do {							\
>>> +		XENO_BUGON(RTDM, rthal_local_irq_test());	\
>>> +		rthal_spin_lock(lock);				\
>>> +	} while (0)
>>> +#endif
>> Why is it a problem to call rthal_spin_lock with irqs off?
> 
> Did I messed it up again? I meant it is a problem to call it with irqs
> *on*. Checking what rthal_local_irq_test() actually means... Hmm, I
> still think it's correct. Maybe we should rename rthal_local_irq_test to
> rthal_local_irq_enabled to clarify the usage.

Ok. Got it. So, maybe, what you want is:

if (rthal_local_irq_test())
	xnpod_lock_sched();
rthal_spin_lock

?

-- 
					    Gilles.

Re: [Xenomai-core] [PATCH 2/3] RTDM: Instrument rtdm_lock_get for proper use

[Jan Kiszka]

Gilles Chanteperdrix wrote:
> Jan Kiszka wrote:
>> Gilles Chanteperdrix wrote:
>>> Jan Kiszka wrote:
>>>> In case the user thinks rtdm_lock_get could be used like spin_lock or
>>>> messes up the IRQ protection for other reasons, catch this with a
>>>> XENO_BUGON.
>>>>
>>>> Signed-off-by: Jan Kiszka <jan.kiszka@domain.hid>
>>>> ---
>>>>
>>>>  include/rtdm/rtdm_driver.h |    8 ++++++++
>>>>  1 files changed, 8 insertions(+), 0 deletions(-)
>>>>
>>>> diff --git a/include/rtdm/rtdm_driver.h b/include/rtdm/rtdm_driver.h
>>>> index 058a9f8..fe42eea 100644
>>>> --- a/include/rtdm/rtdm_driver.h
>>>> +++ b/include/rtdm/rtdm_driver.h
>>>> @@ -655,7 +655,15 @@ typedef unsigned long rtdm_lockctx_t;
>>>>   *
>>>>   * Rescheduling: never.
>>>>   */
>>>> +#ifdef DOXYGEN_CPP /* Beautify doxygen output */
>>>>  #define rtdm_lock_get(lock)	rthal_spin_lock(lock)
>>>> +#else /* This is how it really works */
>>>> +#define rtdm_lock_get(lock)					\
>>>> +	do {							\
>>>> +		XENO_BUGON(RTDM, rthal_local_irq_test());	\
>>>> +		rthal_spin_lock(lock);				\
>>>> +	} while (0)
>>>> +#endif
>>> Why is it a problem to call rthal_spin_lock with irqs off?
>> Did I messed it up again? I meant it is a problem to call it with irqs
>> *on*. Checking what rthal_local_irq_test() actually means... Hmm, I
>> still think it's correct. Maybe we should rename rthal_local_irq_test to
>> rthal_local_irq_enabled to clarify the usage.
> 
> Ok. Got it. So, maybe, what you want is:
> 
> if (rthal_local_irq_test())
> 	xnpod_lock_sched();
> rthal_spin_lock
> 
> ?

That would be a semantical enhancement of rtdm_spin_lock/unlock, but I'm
not sure we want it. Because then you can run into bugs if the user
forgets to pick the irqsave version for task context when there is also
an IRQ context use of the same lock.

So far the semantic of rtdm_lock was very simple: cross-CPU protection
via spin lock, local preemption protection via IRQ lock. And that
pattern could easily be validated with the instrumentation I posted. And
so far no one asked for more. And finally: xnpod_lock/unlock_sched won't
be be cheaper than irqsave/restore as it involves a full nklock
acquisition - with irqsave/restore...

Jan

-- 
Siemens AG, Corporate Technology, CT SE 2
Corporate Competence Center Embedded Linux

Re: [Xenomai-core] [PATCH 2/3] RTDM: Instrument rtdm_lock_get for proper use

[Philippe Gerum]

On Wed, 2009-06-03 at 15:07 +0200, Jan Kiszka wrote:
> Gilles Chanteperdrix wrote:
> > Jan Kiszka wrote:
> >> In case the user thinks rtdm_lock_get could be used like spin_lock or
> >> messes up the IRQ protection for other reasons, catch this with a
> >> XENO_BUGON.
> >>
> >> Signed-off-by: Jan Kiszka <jan.kiszka@domain.hid>
> >> ---
> >>
> >>  include/rtdm/rtdm_driver.h |    8 ++++++++
> >>  1 files changed, 8 insertions(+), 0 deletions(-)
> >>
> >> diff --git a/include/rtdm/rtdm_driver.h b/include/rtdm/rtdm_driver.h
> >> index 058a9f8..fe42eea 100644
> >> --- a/include/rtdm/rtdm_driver.h
> >> +++ b/include/rtdm/rtdm_driver.h
> >> @@ -655,7 +655,15 @@ typedef unsigned long rtdm_lockctx_t;
> >>   *
> >>   * Rescheduling: never.
> >>   */
> >> +#ifdef DOXYGEN_CPP /* Beautify doxygen output */
> >>  #define rtdm_lock_get(lock)	rthal_spin_lock(lock)
> >> +#else /* This is how it really works */
> >> +#define rtdm_lock_get(lock)					\
> >> +	do {							\
> >> +		XENO_BUGON(RTDM, rthal_local_irq_test());	\
> >> +		rthal_spin_lock(lock);				\
> >> +	} while (0)
> >> +#endif
> > 
> > Why is it a problem to call rthal_spin_lock with irqs off?
> 
> Did I messed it up again? I meant it is a problem to call it with irqs
> *on*. Checking what rthal_local_irq_test() actually means...

It means "is the stall bit set for the Xenomai pipeline stage", so your
test is inverted.

>  Hmm, I
> still think it's correct. Maybe we should rename rthal_local_irq_test to
> rthal_local_irq_enabled to clarify the usage.
> 

No, because this is aligned on the I-pipe internal logic, but we could
define rthal_local_irq_disabled() as an alias.

> > 
> > I also belived that you can avoid the #ifdef DOXYGEN_CPP.
> 
> Haven't tried recently, but so far doxygen pulled the whole definition
> into its documents, and I don't when the BUGON to appear there.
> 
> Jan
> 
-- 
Philippe.

Re: [Xenomai-core] [PATCH 2/3] RTDM: Instrument rtdm_lock_get for proper use

[Jan Kiszka]

Philippe Gerum wrote:
> On Wed, 2009-06-03 at 15:07 +0200, Jan Kiszka wrote:
>> Gilles Chanteperdrix wrote:
>>> Jan Kiszka wrote:
>>>> In case the user thinks rtdm_lock_get could be used like spin_lock or
>>>> messes up the IRQ protection for other reasons, catch this with a
>>>> XENO_BUGON.
>>>>
>>>> Signed-off-by: Jan Kiszka <jan.kiszka@domain.hid>
>>>> ---
>>>>
>>>>  include/rtdm/rtdm_driver.h |    8 ++++++++
>>>>  1 files changed, 8 insertions(+), 0 deletions(-)
>>>>
>>>> diff --git a/include/rtdm/rtdm_driver.h b/include/rtdm/rtdm_driver.h
>>>> index 058a9f8..fe42eea 100644
>>>> --- a/include/rtdm/rtdm_driver.h
>>>> +++ b/include/rtdm/rtdm_driver.h
>>>> @@ -655,7 +655,15 @@ typedef unsigned long rtdm_lockctx_t;
>>>>   *
>>>>   * Rescheduling: never.
>>>>   */
>>>> +#ifdef DOXYGEN_CPP /* Beautify doxygen output */
>>>>  #define rtdm_lock_get(lock)	rthal_spin_lock(lock)
>>>> +#else /* This is how it really works */
>>>> +#define rtdm_lock_get(lock)					\
>>>> +	do {							\
>>>> +		XENO_BUGON(RTDM, rthal_local_irq_test());	\
>>>> +		rthal_spin_lock(lock);				\
>>>> +	} while (0)
>>>> +#endif
>>> Why is it a problem to call rthal_spin_lock with irqs off?
>> Did I messed it up again? I meant it is a problem to call it with irqs
>> *on*. Checking what rthal_local_irq_test() actually means...
> 
> It means "is the stall bit set for the Xenomai pipeline stage", so your
> test is inverted.

Ah, OK, now I got it. A terrible interface.

> 
>>  Hmm, I
>> still think it's correct. Maybe we should rename rthal_local_irq_test to
>> rthal_local_irq_enabled to clarify the usage.
>>
> 
> No, because this is aligned on the I-pipe internal logic, but we could
> define rthal_local_irq_disabled() as an alias.

I think at least Xenomai internal users would not really miss
rthal_local_irq_test. However, I will add a proper alias called
rthal_local_irq_disabled and clean up at least RTDM's users. spltest
would not really benefit from being switched over...

Jan

-- 
Siemens AG, Corporate Technology, CT SE 2
Corporate Competence Center Embedded Linux

Re: [Xenomai-core] [PATCH 2/3] RTDM: Instrument rtdm_lock_get for proper use

[Gilles Chanteperdrix]

Jan Kiszka wrote:
> Gilles Chanteperdrix wrote:
>> Jan Kiszka wrote:
>>> Gilles Chanteperdrix wrote:
>>>> Jan Kiszka wrote:
>>>>> In case the user thinks rtdm_lock_get could be used like spin_lock or
>>>>> messes up the IRQ protection for other reasons, catch this with a
>>>>> XENO_BUGON.
>>>>>
>>>>> Signed-off-by: Jan Kiszka <jan.kiszka@domain.hid>
>>>>> ---
>>>>>
>>>>>  include/rtdm/rtdm_driver.h |    8 ++++++++
>>>>>  1 files changed, 8 insertions(+), 0 deletions(-)
>>>>>
>>>>> diff --git a/include/rtdm/rtdm_driver.h b/include/rtdm/rtdm_driver.h
>>>>> index 058a9f8..fe42eea 100644
>>>>> --- a/include/rtdm/rtdm_driver.h
>>>>> +++ b/include/rtdm/rtdm_driver.h
>>>>> @@ -655,7 +655,15 @@ typedef unsigned long rtdm_lockctx_t;
>>>>>   *
>>>>>   * Rescheduling: never.
>>>>>   */
>>>>> +#ifdef DOXYGEN_CPP /* Beautify doxygen output */
>>>>>  #define rtdm_lock_get(lock)	rthal_spin_lock(lock)
>>>>> +#else /* This is how it really works */
>>>>> +#define rtdm_lock_get(lock)					\
>>>>> +	do {							\
>>>>> +		XENO_BUGON(RTDM, rthal_local_irq_test());	\
>>>>> +		rthal_spin_lock(lock);				\
>>>>> +	} while (0)
>>>>> +#endif
>>>> Why is it a problem to call rthal_spin_lock with irqs off?
>>> Did I messed it up again? I meant it is a problem to call it with irqs
>>> *on*. Checking what rthal_local_irq_test() actually means... Hmm, I
>>> still think it's correct. Maybe we should rename rthal_local_irq_test to
>>> rthal_local_irq_enabled to clarify the usage.
>> Ok. Got it. So, maybe, what you want is:
>>
>> if (rthal_local_irq_test())
>> 	xnpod_lock_sched();
>> rthal_spin_lock
>>
>> ?
> 
> That would be a semantical enhancement of rtdm_spin_lock/unlock, but I'm
> not sure we want it. Because then you can run into bugs if the user
> forgets to pick the irqsave version for task context when there is also
> an IRQ context use of the same lock.
> 
> So far the semantic of rtdm_lock was very simple: cross-CPU protection
> via spin lock, local preemption protection via IRQ lock. And that
> pattern could easily be validated with the instrumentation I posted. And
> so far no one asked for more. And finally: xnpod_lock/unlock_sched won't
> be be cheaper than irqsave/restore as it involves a full nklock
> acquisition - with irqsave/restore...

On the other hand, I already had to port some plain Linux drivers to
RTDM, and from this perspective, having a one to one mapping of the
services is a real win. I also think that equivalents to
preempt_enable() and preempt_disable() are missing in the RTDM
interface. I found myself calling
xnpod_lock_sched()/xnpod_unlock_sched() in some RTDM drivers, which is
bad, I know...

-- 
					    Gilles.

Re: [Xenomai-core] [PATCH 2/3] RTDM: Instrument rtdm_lock_get for proper use

[Jan Kiszka]

Gilles Chanteperdrix wrote:
> Jan Kiszka wrote:
>> Gilles Chanteperdrix wrote:
>>> Jan Kiszka wrote:
>>>> Gilles Chanteperdrix wrote:
>>>>> Jan Kiszka wrote:
>>>>>> In case the user thinks rtdm_lock_get could be used like spin_lock or
>>>>>> messes up the IRQ protection for other reasons, catch this with a
>>>>>> XENO_BUGON.
>>>>>>
>>>>>> Signed-off-by: Jan Kiszka <jan.kiszka@domain.hid>
>>>>>> ---
>>>>>>
>>>>>>  include/rtdm/rtdm_driver.h |    8 ++++++++
>>>>>>  1 files changed, 8 insertions(+), 0 deletions(-)
>>>>>>
>>>>>> diff --git a/include/rtdm/rtdm_driver.h b/include/rtdm/rtdm_driver.h
>>>>>> index 058a9f8..fe42eea 100644
>>>>>> --- a/include/rtdm/rtdm_driver.h
>>>>>> +++ b/include/rtdm/rtdm_driver.h
>>>>>> @@ -655,7 +655,15 @@ typedef unsigned long rtdm_lockctx_t;
>>>>>>   *
>>>>>>   * Rescheduling: never.
>>>>>>   */
>>>>>> +#ifdef DOXYGEN_CPP /* Beautify doxygen output */
>>>>>>  #define rtdm_lock_get(lock)	rthal_spin_lock(lock)
>>>>>> +#else /* This is how it really works */
>>>>>> +#define rtdm_lock_get(lock)					\
>>>>>> +	do {							\
>>>>>> +		XENO_BUGON(RTDM, rthal_local_irq_test());	\
>>>>>> +		rthal_spin_lock(lock);				\
>>>>>> +	} while (0)
>>>>>> +#endif
>>>>> Why is it a problem to call rthal_spin_lock with irqs off?
>>>> Did I messed it up again? I meant it is a problem to call it with irqs
>>>> *on*. Checking what rthal_local_irq_test() actually means... Hmm, I
>>>> still think it's correct. Maybe we should rename rthal_local_irq_test to
>>>> rthal_local_irq_enabled to clarify the usage.
>>> Ok. Got it. So, maybe, what you want is:
>>>
>>> if (rthal_local_irq_test())
>>> 	xnpod_lock_sched();
>>> rthal_spin_lock
>>>
>>> ?
>> That would be a semantical enhancement of rtdm_spin_lock/unlock, but I'm
>> not sure we want it. Because then you can run into bugs if the user
>> forgets to pick the irqsave version for task context when there is also
>> an IRQ context use of the same lock.
>>
>> So far the semantic of rtdm_lock was very simple: cross-CPU protection
>> via spin lock, local preemption protection via IRQ lock. And that
>> pattern could easily be validated with the instrumentation I posted. And
>> so far no one asked for more. And finally: xnpod_lock/unlock_sched won't
>> be be cheaper than irqsave/restore as it involves a full nklock
>> acquisition - with irqsave/restore...
> 
> On the other hand, I already had to port some plain Linux drivers to
> RTDM, and from this perspective, having a one to one mapping of the
> services is a real win. I also think that equivalents to
> preempt_enable() and preempt_disable() are missing in the RTDM
> interface. I found myself calling
> xnpod_lock_sched()/xnpod_unlock_sched() in some RTDM drivers, which is
> bad, I know...

Yes, but as long as we have no comparably cheap preempt_enable/disable
in Xenomai, I think it is counterproductive to motivate its (potentially
heavy) use in drivers.

Jan

-- 
Siemens AG, Corporate Technology, CT SE 2
Corporate Competence Center Embedded Linux

Re: [Xenomai-core] [PATCH 2/3] RTDM: Instrument rtdm_lock_get for proper use

[Gilles Chanteperdrix]

Jan Kiszka wrote:
> Gilles Chanteperdrix wrote:
>> Jan Kiszka wrote:
>>> Gilles Chanteperdrix wrote:
>>>> Jan Kiszka wrote:
>>>>> Gilles Chanteperdrix wrote:
>>>>>> Jan Kiszka wrote:
>>>>>>> In case the user thinks rtdm_lock_get could be used like spin_lock or
>>>>>>> messes up the IRQ protection for other reasons, catch this with a
>>>>>>> XENO_BUGON.
>>>>>>>
>>>>>>> Signed-off-by: Jan Kiszka <jan.kiszka@domain.hid>
>>>>>>> ---
>>>>>>>
>>>>>>>  include/rtdm/rtdm_driver.h |    8 ++++++++
>>>>>>>  1 files changed, 8 insertions(+), 0 deletions(-)
>>>>>>>
>>>>>>> diff --git a/include/rtdm/rtdm_driver.h b/include/rtdm/rtdm_driver.h
>>>>>>> index 058a9f8..fe42eea 100644
>>>>>>> --- a/include/rtdm/rtdm_driver.h
>>>>>>> +++ b/include/rtdm/rtdm_driver.h
>>>>>>> @@ -655,7 +655,15 @@ typedef unsigned long rtdm_lockctx_t;
>>>>>>>   *
>>>>>>>   * Rescheduling: never.
>>>>>>>   */
>>>>>>> +#ifdef DOXYGEN_CPP /* Beautify doxygen output */
>>>>>>>  #define rtdm_lock_get(lock)	rthal_spin_lock(lock)
>>>>>>> +#else /* This is how it really works */
>>>>>>> +#define rtdm_lock_get(lock)					\
>>>>>>> +	do {							\
>>>>>>> +		XENO_BUGON(RTDM, rthal_local_irq_test());	\
>>>>>>> +		rthal_spin_lock(lock);				\
>>>>>>> +	} while (0)
>>>>>>> +#endif
>>>>>> Why is it a problem to call rthal_spin_lock with irqs off?
>>>>> Did I messed it up again? I meant it is a problem to call it with irqs
>>>>> *on*. Checking what rthal_local_irq_test() actually means... Hmm, I
>>>>> still think it's correct. Maybe we should rename rthal_local_irq_test to
>>>>> rthal_local_irq_enabled to clarify the usage.
>>>> Ok. Got it. So, maybe, what you want is:
>>>>
>>>> if (rthal_local_irq_test())
>>>> 	xnpod_lock_sched();
>>>> rthal_spin_lock
>>>>
>>>> ?
>>> That would be a semantical enhancement of rtdm_spin_lock/unlock, but I'm
>>> not sure we want it. Because then you can run into bugs if the user
>>> forgets to pick the irqsave version for task context when there is also
>>> an IRQ context use of the same lock.
>>>
>>> So far the semantic of rtdm_lock was very simple: cross-CPU protection
>>> via spin lock, local preemption protection via IRQ lock. And that
>>> pattern could easily be validated with the instrumentation I posted. And
>>> so far no one asked for more. And finally: xnpod_lock/unlock_sched won't
>>> be be cheaper than irqsave/restore as it involves a full nklock
>>> acquisition - with irqsave/restore...
>> On the other hand, I already had to port some plain Linux drivers to
>> RTDM, and from this perspective, having a one to one mapping of the
>> services is a real win. I also think that equivalents to
>> preempt_enable() and preempt_disable() are missing in the RTDM
>> interface. I found myself calling
>> xnpod_lock_sched()/xnpod_unlock_sched() in some RTDM drivers, which is
>> bad, I know...
> 
> Yes, but as long as we have no comparably cheap preempt_enable/disable
> in Xenomai, I think it is counterproductive to motivate its (potentially
> heavy) use in drivers.

I do not think the fact that xnpod_lock_sched is heavier than
preempt_disable is much of a problem: it is not that much heavier, and
if you use it, it is for lengthy critical sections for which you want
interrupts enabled, so, its cost is amortized. The problem is that
locking the scheduler for a long period of time is bad for the
scheduling, it can cause a kind of priority inversions. So, when porting
drivers, we should make the critical sections short and use
rtdm_lock_get_irqsave, period.

-- 
					    Gilles.