[Xenomai-core] [PATCH 1/3] decouple spinlock stats from XENO_OPT_STATS

[Xenomai-core] [PATCH 1/3] decouple spinlock stats from XENO_OPT_STATS

[Jan Kiszka]

[-- Attachment #1.1: Type: text/plain, Size: 441 bytes --]

As recently discussed, the current implicit coupling of spinlock
statistics/debugging with the XENO_OPT_STATS switch is suboptimal. The
reason: on SMP, it adds noticeable overhead to the STATS feature which
may only be used for thread statistics collection (as also advertised in
the kconfig help).

This patch makes the spinlock debugging and lock length collection
feature only depend on XENO_OPT_DEBUG (as a first step).

Jan


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1.2: decouple-spinlock-dbg-from-stats.patch --]
[-- Type: text/x-patch; name="decouple-spinlock-dbg-from-stats.patch", Size: 5093 bytes --]

---
 include/asm-generic/system.h |   35 ++++++++++++++++-------------------
 ksrc/nucleus/module.c        |   22 +++++++++++-----------
 2 files changed, 27 insertions(+), 30 deletions(-)

Index: xenomai/include/asm-generic/system.h
===================================================================
--- xenomai.orig/include/asm-generic/system.h
+++ xenomai/include/asm-generic/system.h
@@ -71,9 +71,7 @@ typedef unsigned long spl_t;
 #define spltest()   rthal_local_irq_test()
 #define splget(x)   rthal_local_irq_flags(x)
 
-#if defined(CONFIG_SMP) && \
-    (defined(CONFIG_XENO_OPT_STATS) || defined(CONFIG_XENO_OPT_DEBUG))
-
+#if defined(CONFIG_SMP) && defined(CONFIG_XENO_OPT_DEBUG)
 typedef struct {
 
         unsigned long long spin_time;
@@ -96,25 +94,24 @@ typedef struct {
 
 } xnlock_t;
 
-#define XNARCH_LOCK_UNLOCKED (xnlock_t) {       \
-	{ ~0 },				        \
-        NULL,                                   \
-        NULL,                                   \
-        0,                                      \
-        -1,                                     \
-        0LL,                                    \
-        0LL,                                    \
+#define XNARCH_LOCK_UNLOCKED (xnlock_t) {	\
+	{ ~0 },					\
+	NULL,					\
+	NULL,					\
+	0,					\
+	-1,					\
+	0LL,					\
+	0LL,					\
 }
 
 #define CONFIG_XENO_SPINLOCK_DEBUG  1
 
-#else /* !(CONFIG_XENO_OPT_STATS && CONFIG_SMP) */
+#else /* !(CONFIG_SMP && CONFIG_XENO_OPT_DEBUG) */
 
 typedef struct { atomic_t owner; } xnlock_t;
 
 #define XNARCH_LOCK_UNLOCKED (xnlock_t) { { ~0 } }
-
-#endif /* CONFIG_XENO_OPT_STATS && CONFIG_SMP */
+#endif /* CONFIG_SMP && CONFIG_XENO_OPT_DEBUG */
 
 #define XNARCH_NR_CPUS               RTHAL_NR_CPUS
 
@@ -253,7 +250,7 @@ static inline int __xnlock_get (xnlock_t
 #else /* !CONFIG_XENO_SPINLOCK_DEBUG */
 static inline int __xnlock_get (xnlock_t *lock)
 {
-#endif /* !CONFIG_XENO_SPINLOCK_DEBUG */
+#endif /* CONFIG_XENO_SPINLOCK_DEBUG */
     rthal_declare_cpuid;
     int recursing;
 
@@ -304,7 +301,7 @@ static inline void xnlock_put (xnlock_t 
     rthal_load_cpuid();
     if (likely(atomic_read(&lock->owner) == cpuid)) {
 
-#ifdef CONFIG_XENO_OPT_STATS
+#ifdef CONFIG_XENO_SPINLOCK_DEBUG
 	    extern xnlockinfo_t xnlock_stats[];
 
 	    unsigned long long lock_time = rthal_rdtsc() - lock->lock_date;
@@ -316,7 +313,7 @@ static inline void xnlock_put (xnlock_t 
 		    xnlock_stats[cpuid].function = lock->function;
 		    xnlock_stats[cpuid].line = lock->line;
 	    }
-#endif /* CONFIG_XENO_OPT_STATS */
+#endif /* CONFIG_XENO_SPINLOCK_DEBUG */
 	    atomic_set(&lock->owner, ~0);
     }
 #ifdef CONFIG_XENO_SPINLOCK_DEBUG
@@ -343,7 +340,7 @@ static inline spl_t __xnlock_get_irqsave
 #else /* !CONFIG_XENO_SPINLOCK_DEBUG */
 static inline spl_t __xnlock_get_irqsave (xnlock_t *lock)
 {
-#endif /* !CONFIG_XENO_SPINLOCK_DEBUG */
+#endif /* CONFIG_XENO_SPINLOCK_DEBUG */
     unsigned long flags;
 
     rthal_local_irq_save(flags);
@@ -354,7 +351,7 @@ static inline spl_t __xnlock_get_irqsave
 #else /* !CONFIG_XENO_SPINLOCK_DEBUG */
     if (__xnlock_get(lock))
 	    flags |= 2;
-#endif /* !CONFIG_XENO_SPINLOCK_DEBUG */
+#endif /* CONFIG_XENO_SPINLOCK_DEBUG */
 	
     return flags;
 }
Index: xenomai/ksrc/nucleus/module.c
===================================================================
--- xenomai.orig/ksrc/nucleus/module.c
+++ xenomai/ksrc/nucleus/module.c
@@ -472,7 +472,9 @@ static struct file_operations stat_seq_o
 	.release = seq_release_private,
 };
 
-#ifdef CONFIG_SMP
+#endif /* CONFIG_XENO_OPT_STATS */
+
+#ifdef CONFIG_XENO_SPINLOCK_DEBUG
 
 xnlockinfo_t xnlock_stats[RTHAL_NR_CPUS];
 
@@ -520,9 +522,7 @@ static int lock_read_proc(char *page,
 
 EXPORT_SYMBOL(xnlock_stats);
 
-#endif /* CONFIG_SMP */
-
-#endif /* CONFIG_XENO_OPT_STATS */
+#endif /* CONFIG_XENO_SPINLOCK_DEBUG */
 
 static int latency_read_proc(char *page,
 			     char **start,
@@ -741,12 +741,12 @@ void xnpod_init_proc(void)
 
 #ifdef CONFIG_XENO_OPT_STATS
 	add_proc_fops("stat", &stat_seq_operations, 0, rthal_proc_root);
-#ifdef CONFIG_SMP
-	add_proc_leaf("lock", &lock_read_proc, NULL, NULL, rthal_proc_root);
-#endif /* CONFIG_SMP */
-
 #endif /* CONFIG_XENO_OPT_STATS */
 
+#ifdef CONFIG_XENO_SPINLOCK_DEBUG
+	add_proc_leaf("lock", &lock_read_proc, NULL, NULL, rthal_proc_root);
+#endif /* CONFIG_XENO_SPINLOCK_DEBUG */
+
 	add_proc_leaf("latency",
 		      &latency_read_proc,
 		      &latency_write_proc, NULL, rthal_proc_root);
@@ -786,10 +786,10 @@ void xnpod_delete_proc(void)
 	remove_proc_entry("sched", rthal_proc_root);
 #ifdef CONFIG_XENO_OPT_STATS
 	remove_proc_entry("stat", rthal_proc_root);
-#ifdef CONFIG_SMP
-	remove_proc_entry("lock", rthal_proc_root);
-#endif /* CONFIG_SMP */
 #endif /* CONFIG_XENO_OPT_STATS */
+#ifdef CONFIG_XENO_SPINLOCK_DEBUG
+	remove_proc_entry("lock", rthal_proc_root);
+#endif /* CONFIG_XENO_SPINLOCK_DEBUG */
 }
 
 #ifdef CONFIG_XENO_OPT_PERVASIVE


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 250 bytes --]

Re: [Xenomai-core] [PATCH 1/3] decouple spinlock stats from XENO_OPT_STATS

[Gilles Chanteperdrix]

Jan Kiszka wrote:
>  #ifdef CONFIG_XENO_SPINLOCK_DEBUG
> @@ -343,7 +340,7 @@ static inline spl_t __xnlock_get_irqsave
>  #else /* !CONFIG_XENO_SPINLOCK_DEBUG */
>  static inline spl_t __xnlock_get_irqsave (xnlock_t *lock)
>  {
> -#endif /* !CONFIG_XENO_SPINLOCK_DEBUG */
> +#endif /* CONFIG_XENO_SPINLOCK_DEBUG */

I prefer the notation:

#if cond
#endif /* cond */

and

#if cond
#else /* !cond */
#endif /* !cond */

to

#if cond
#endif /* cond */

and

#if cond
#else /* !cond */
#endif /* cond */

Because in the first case, when I see a #endif in the middle of a lot of
code, I immediately know what predicate is true above the #endif.

-- 
                                                 Gilles Chanteperdrix

Re: [Xenomai-core] [PATCH 1/3] decouple spinlock stats from XENO_OPT_STATS

[Jan Kiszka]

[-- Attachment #1: Type: text/plain, Size: 1147 bytes --]

Gilles Chanteperdrix wrote:
> Jan Kiszka wrote:
>>  #ifdef CONFIG_XENO_SPINLOCK_DEBUG
>> @@ -343,7 +340,7 @@ static inline spl_t __xnlock_get_irqsave
>>  #else /* !CONFIG_XENO_SPINLOCK_DEBUG */
>>  static inline spl_t __xnlock_get_irqsave (xnlock_t *lock)
>>  {
>> -#endif /* !CONFIG_XENO_SPINLOCK_DEBUG */
>> +#endif /* CONFIG_XENO_SPINLOCK_DEBUG */
> 
> I prefer the notation:
> 
> #if cond
> #endif /* cond */
> 
> and
> 
> #if cond
> #else /* !cond */
> #endif /* !cond */
> 
> to
> 
> #if cond
> #endif /* cond */
> 
> and
> 
> #if cond
> #else /* !cond */
> #endif /* cond */
> 
> Because in the first case, when I see a #endif in the middle of a lot of
> code, I immediately know what predicate is true above the #endif.
> 

Ok, I understand the logic. Nevertheless, I had the impression the first
scheme is used far more often than the second one in Xenomai and I-pipe,
so I took the chance to consolidate the code.

So, what should be the common rule from now on? Is there a kernel-style
rule for this issue we should follow, or are we free and should convert
to your preferred scheme?

Jan


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 250 bytes --]

Re: [Xenomai-core] [PATCH 1/3] decouple spinlock stats from XENO_OPT_STATS

[Philippe Gerum]

On Tue, 2006-11-21 at 18:09 +0100, Jan Kiszka wrote:
> Gilles Chanteperdrix wrote:
> > Jan Kiszka wrote:
> >>  #ifdef CONFIG_XENO_SPINLOCK_DEBUG
> >> @@ -343,7 +340,7 @@ static inline spl_t __xnlock_get_irqsave
> >>  #else /* !CONFIG_XENO_SPINLOCK_DEBUG */
> >>  static inline spl_t __xnlock_get_irqsave (xnlock_t *lock)
> >>  {
> >> -#endif /* !CONFIG_XENO_SPINLOCK_DEBUG */
> >> +#endif /* CONFIG_XENO_SPINLOCK_DEBUG */
> > 
> > I prefer the notation:
> > 
> > #if cond
> > #endif /* cond */
> > 
> > and
> > 
> > #if cond
> > #else /* !cond */
> > #endif /* !cond */
> > 
> > to
> > 
> > #if cond
> > #endif /* cond */
> > 
> > and
> > 
> > #if cond
> > #else /* !cond */
> > #endif /* cond */
> > 
> > Because in the first case, when I see a #endif in the middle of a lot of
> > code, I immediately know what predicate is true above the #endif.
> > 
> 
> Ok, I understand the logic. Nevertheless, I had the impression the first
> scheme is used far more often than the second one in Xenomai and I-pipe,
> so I took the chance to consolidate the code.

Basically because I wrote those using the first convention.

> 
> So, what should be the common rule from now on? Is there a kernel-style
> rule for this issue we should follow, or are we free and should convert
> to your preferred scheme?
> 

Let's move to Gilles's convention; it's more readable.

> Jan
> 
> _______________________________________________
> Xenomai-core mailing list
> Xenomai-core@domain.hid
> https://mail.gna.org/listinfo/xenomai-core
-- 
Philippe.

Re: [Xenomai-core] [PATCH 1/3] decouple spinlock stats from XENO_OPT_STATS

[Jan Kiszka]

[-- Attachment #1: Type: text/plain, Size: 95 bytes --]

Here comes version 2 of this patch, now following the new
#if-#else-#endif comment style.

Jan

[-- Attachment #2: decouple-spinlock-dbg-from-stats-v2.patch --]
[-- Type: text/x-patch, Size: 5107 bytes --]

---
 include/asm-generic/system.h |   35 ++++++++++++++++-------------------
 ksrc/nucleus/module.c        |   22 +++++++++++-----------
 2 files changed, 27 insertions(+), 30 deletions(-)

Index: xenomai/include/asm-generic/system.h
===================================================================
--- xenomai.orig/include/asm-generic/system.h
+++ xenomai/include/asm-generic/system.h
@@ -66,14 +66,12 @@ typedef unsigned long spl_t;
 #define splexit(x)  rthal_local_irq_restore((x) & 1)
 #else /* !CONFIG_SMP */
 #define splexit(x)  rthal_local_irq_restore(x)
-#endif /* CONFIG_SMP */
+#endif /* !CONFIG_SMP */
 #define splnone()   rthal_local_irq_enable()
 #define spltest()   rthal_local_irq_test()
 #define splget(x)   rthal_local_irq_flags(x)
 
-#if defined(CONFIG_SMP) && \
-    (defined(CONFIG_XENO_OPT_STATS) || defined(CONFIG_XENO_OPT_DEBUG))
-
+#if defined(CONFIG_SMP) && defined(CONFIG_XENO_OPT_DEBUG)
 typedef struct {
 
         unsigned long long spin_time;
@@ -96,25 +94,24 @@ typedef struct {
 
 } xnlock_t;
 
-#define XNARCH_LOCK_UNLOCKED (xnlock_t) {       \
-	{ ~0 },				        \
-        NULL,                                   \
-        NULL,                                   \
-        0,                                      \
-        -1,                                     \
-        0LL,                                    \
-        0LL,                                    \
+#define XNARCH_LOCK_UNLOCKED (xnlock_t) {	\
+	{ ~0 },					\
+	NULL,					\
+	NULL,					\
+	0,					\
+	-1,					\
+	0LL,					\
+	0LL,					\
 }
 
 #define CONFIG_XENO_SPINLOCK_DEBUG  1
 
-#else /* !(CONFIG_XENO_OPT_STATS && CONFIG_SMP) */
+#else /* !(CONFIG_SMP && CONFIG_XENO_OPT_DEBUG) */
 
 typedef struct { atomic_t owner; } xnlock_t;
 
 #define XNARCH_LOCK_UNLOCKED (xnlock_t) { { ~0 } }
-
-#endif /* CONFIG_XENO_OPT_STATS && CONFIG_SMP */
+#endif /* !(CONFIG_SMP && CONFIG_XENO_OPT_DEBUG) */
 
 #define XNARCH_NR_CPUS               RTHAL_NR_CPUS
 
@@ -231,7 +228,7 @@ static inline int xnarch_setimask (int i
 #else /* !CONFIG_XENO_SPINLOCK_DEBUG */
 #define xnlock_get(lock)            __xnlock_get(lock)
 #define xnlock_get_irqsave(lock,x)  ((x) = __xnlock_get_irqsave(lock))
-#endif /* CONFIG_XENO_SPINLOCK_DEBUG */
+#endif /* !CONFIG_XENO_SPINLOCK_DEBUG */
 #define xnlock_clear_irqoff(lock)   xnlock_put_irqrestore(lock,1)
 #define xnlock_clear_irqon(lock)    xnlock_put_irqrestore(lock,0)
 
@@ -304,7 +301,7 @@ static inline void xnlock_put (xnlock_t 
     rthal_load_cpuid();
     if (likely(atomic_read(&lock->owner) == cpuid)) {
 
-#ifdef CONFIG_XENO_OPT_STATS
+#ifdef CONFIG_XENO_SPINLOCK_DEBUG
 	    extern xnlockinfo_t xnlock_stats[];
 
 	    unsigned long long lock_time = rthal_rdtsc() - lock->lock_date;
@@ -316,7 +313,7 @@ static inline void xnlock_put (xnlock_t 
 		    xnlock_stats[cpuid].function = lock->function;
 		    xnlock_stats[cpuid].line = lock->line;
 	    }
-#endif /* CONFIG_XENO_OPT_STATS */
+#endif /* CONFIG_XENO_SPINLOCK_DEBUG */
 	    atomic_set(&lock->owner, ~0);
     }
 #ifdef CONFIG_XENO_SPINLOCK_DEBUG
@@ -377,7 +374,7 @@ static inline void xnlock_put_irqrestore
 #define xnlock_clear_irqoff(lock)      rthal_local_irq_disable()
 #define xnlock_clear_irqon(lock)       rthal_local_irq_enable()
 
-#endif /* CONFIG_SMP */
+#endif /* !CONFIG_SMP */
 
 static inline int xnarch_remap_vm_page(struct vm_area_struct *vma,
 				       unsigned long from,
Index: xenomai/ksrc/nucleus/module.c
===================================================================
--- xenomai.orig/ksrc/nucleus/module.c
+++ xenomai/ksrc/nucleus/module.c
@@ -472,7 +472,9 @@ static struct file_operations stat_seq_o
 	.release = seq_release_private,
 };
 
-#ifdef CONFIG_SMP
+#endif /* CONFIG_XENO_OPT_STATS */
+
+#ifdef CONFIG_XENO_SPINLOCK_DEBUG
 
 xnlockinfo_t xnlock_stats[RTHAL_NR_CPUS];
 
@@ -520,9 +522,7 @@ static int lock_read_proc(char *page,
 
 EXPORT_SYMBOL(xnlock_stats);
 
-#endif /* CONFIG_SMP */
-
-#endif /* CONFIG_XENO_OPT_STATS */
+#endif /* CONFIG_XENO_SPINLOCK_DEBUG */
 
 static int latency_read_proc(char *page,
 			     char **start,
@@ -741,12 +741,12 @@ void xnpod_init_proc(void)
 
 #ifdef CONFIG_XENO_OPT_STATS
 	add_proc_fops("stat", &stat_seq_operations, 0, rthal_proc_root);
-#ifdef CONFIG_SMP
-	add_proc_leaf("lock", &lock_read_proc, NULL, NULL, rthal_proc_root);
-#endif /* CONFIG_SMP */
-
 #endif /* CONFIG_XENO_OPT_STATS */
 
+#ifdef CONFIG_XENO_SPINLOCK_DEBUG
+	add_proc_leaf("lock", &lock_read_proc, NULL, NULL, rthal_proc_root);
+#endif /* CONFIG_XENO_SPINLOCK_DEBUG */
+
 	add_proc_leaf("latency",
 		      &latency_read_proc,
 		      &latency_write_proc, NULL, rthal_proc_root);
@@ -786,10 +786,10 @@ void xnpod_delete_proc(void)
 	remove_proc_entry("sched", rthal_proc_root);
 #ifdef CONFIG_XENO_OPT_STATS
 	remove_proc_entry("stat", rthal_proc_root);
-#ifdef CONFIG_SMP
-	remove_proc_entry("lock", rthal_proc_root);
-#endif /* CONFIG_SMP */
 #endif /* CONFIG_XENO_OPT_STATS */
+#ifdef CONFIG_XENO_SPINLOCK_DEBUG
+	remove_proc_entry("lock", rthal_proc_root);
+#endif /* CONFIG_XENO_SPINLOCK_DEBUG */
 }
 
 #ifdef CONFIG_XENO_OPT_PERVASIVE

Re: [Xenomai-core] [PATCH 1/3] decouple spinlock stats from XENO_OPT_STATS

[Philippe Gerum]

Ok, I ended up finding such decoupling cleaner, given that we don't drag
the full debug overhead when activating /proc/xenomai/locks in SMP mode,
thanks to the reorganized debug options. Gilles, is this patch series ok
for you too, and particularly the POSIX changes?

-- 
Philippe.

Re: [Xenomai-core] [PATCH 1/3] decouple spinlock stats from XENO_OPT_STATS

[Gilles Chanteperdrix]

Philippe Gerum wrote:
> Ok, I ended up finding such decoupling cleaner, given that we don't drag
> the full debug overhead when activating /proc/xenomai/locks in SMP mode,
> thanks to the reorganized debug options. Gilles, is this patch series ok
> for you too, and particularly the POSIX changes?
> 

Yes, it is Ok.

-- 
                                                 Gilles Chanteperdrix