[Xenomai-core] hostrt broken on ARM.

[Xenomai-core] hostrt broken on ARM.

[Gilles Chanteperdrix]

Hi,

the hostrt stuff did not compile on ARM, at this chance, I had a look 
and found a few things which I did not like:
- the kernel-space and user-space did not use seqcount implementations 
from the same header, and what is more, seqcount is not available on 2.4
kernels. Since we provide an implementation anyway, I changed the code 
so that only this implementation is used both in kernel-space and 
user-space. Prefixing everything with xn to avoid namespace clashes.
- the xnarch_hostrt_data also had two different definitions, one for 
user-space, one for kernel-space, since this definition is supposed to 
come from newer I-pipe in kernel-space, the code did not compile with 
older I-pipe patches. I separated xnarch_hostrt_data, the structure 
passed by the I-pipe to do_hostrt_event, from xnvdso_hostrt_data, the 
structure, part of the vdso used for exchanges between user-space and 
kernel-space. We can now get xnvdso_hostrt_data defined all the time. We
can probably even move xnvdso_hostrt_data definition to vdso.h
- avoided cycle_t and u32 in order to avoid user-space namespace 
pollution and need for many typedefs with 2.4 kernels. Using unsigned 
long long directly instead of cycle_t revealed a bug in clocktest.c.

Here is the patch.

diff --git a/include/nucleus/Makefile.am b/include/nucleus/Makefile.am
index 9a72b24..5fc1c21 100644
diff --git a/include/nucleus/Makefile.in b/include/nucleus/Makefile.in
index 5e93dc9..a6fa801 100644
diff --git a/include/nucleus/hostrt.h b/include/nucleus/hostrt.h
index 70ffbfe..85b8e88 100644
--- a/include/nucleus/hostrt.h
+++ b/include/nucleus/hostrt.h
@@ -23,32 +23,24 @@
  * 02111-1307, USA.
  */
 
-#include <nucleus/types.h>
-#ifdef __KERNEL__
-#include <asm-generic/xenomai/system.h>
-#else /* !__KERNEL__ */
+#ifndef __KERNEL__
 #include <time.h>
 #include <sys/types.h>
-#include <nucleus/seqlock_user.h>
-typedef u_int32_t u32;
-typedef u_int64_t cycle_t;
+#else /* __KERNEL__ */
+#include <asm-generic/xenomai/system.h>
+#endif /* __KERNEL__ */
+#include <nucleus/seqlock.h>
 
-/*
- * xnarch_hostrt_data must be kept in sync with the corresponding ipipe
- * definitions in ipipe_tickdev.h We cannot directly include the file
- * here because the definitions are also required in Xenomai userland.
- */
-struct xnarch_hostrt_data {
+struct xnvdso_hostrt_data {
 	short live;
-	seqcount_t seqcount;
+	xnseqcount_t seqcount;
 	time_t wall_time_sec;
-	u32 wall_time_nsec;
+	unsigned wall_time_nsec;
 	struct timespec wall_to_monotonic;
-	cycle_t cycle_last;
-	cycle_t mask;
-	u32 mult;
-	u32 shift;
+	unsigned long long cycle_last;
+	unsigned long long mask;
+	unsigned mult;
+	unsigned shift;
 };
-#endif /* !__KERNEL__ */
 
 #endif
diff --git a/include/nucleus/seqlock.h b/include/nucleus/seqlock.h
new file mode 100644
index 0000000..897d411
--- /dev/null
+++ b/include/nucleus/seqlock.h
@@ -0,0 +1,57 @@
+#ifndef __SEQLOCK_H
+#define __SEQLOCK_H
+
+/* Originally from the linux kernel, adapted for userland and Xenomai */
+
+#include <asm/xenomai/atomic.h>
+
+typedef struct xnseqcount {
+	unsigned sequence;
+} xnseqcount_t;
+
+#define XNSEQCNT_ZERO { 0 }
+#define xnseqcount_init(x) do { *(x) = (xnseqcount_t) SEQCNT_ZERO; } while (0)
+
+/* Start of read using pointer to a sequence counter only.  */
+static inline unsigned xnread_seqcount_begin(const xnseqcount_t *s)
+{
+	unsigned ret;
+
+repeat:
+	ret = s->sequence;
+	xnarch_read_memory_barrier();
+	if (unlikely(ret & 1)) {
+		cpu_relax();
+		goto repeat;
+	}
+	return ret;
+}
+
+/*
+ * Test if reader processed invalid data because sequence number has changed.
+ */
+static inline int xnread_seqcount_retry(const xnseqcount_t *s, unsigned start)
+{
+	xnarch_read_memory_barrier();
+
+	return s->sequence != start;
+}
+
+
+/*
+ * The sequence counter only protects readers from concurrent writers.
+ * Writers must use their own locking.
+ */
+static inline void xnwrite_seqcount_begin(xnseqcount_t *s)
+{
+	s->sequence++;
+	xnarch_write_memory_barrier();
+}
+
+static inline void xnwrite_seqcount_end(xnseqcount_t *s)
+{
+	xnarch_write_memory_barrier();
+	s->sequence++;
+}
+
+#endif /* __SEQLOCK_H */
diff --git a/include/nucleus/seqlock_user.h b/include/nucleus/seqlock_user.h
deleted file mode 100644
index 598d4da..0000000
--- a/include/nucleus/seqlock_user.h
+++ /dev/null
@@ -1,57 +0,0 @@
-#ifndef __SEQLOCK_USER_H
-#define __SEQLOCK_USER_H
-
-/* Originally from the linux kernel, adapted for userland and Xenomai */
-
-#include <asm/xenomai/atomic.h>
-
-typedef struct seqcount {
-	unsigned sequence;
-} seqcount_t;
-
-#define SEQCNT_ZERO { 0 }
-#define seqcount_init(x) do { *(x) = (seqcount_t) SEQCNT_ZERO; } while (0)
-
-/* Start of read using pointer to a sequence counter only.  */
-static inline unsigned read_seqcount_begin(const seqcount_t *s)
-{
-	unsigned ret;
-
-repeat:
-	ret = s->sequence;
-	xnarch_read_memory_barrier();
-	if (unlikely(ret & 1)) {
-		cpu_relax();
-		goto repeat;
-	}
-	return ret;
-}
-
-/*
- * Test if reader processed invalid data because sequence number has changed.
- */
-static inline int read_seqcount_retry(const seqcount_t *s, unsigned start)
-{
-	xnarch_read_memory_barrier();
-
-	return s->sequence != start;
-}
-
-
-/*
- * The sequence counter only protects readers from concurrent writers.
- * Writers must use their own locking.
- */
-static inline void write_seqcount_begin(seqcount_t *s)
-{
-	s->sequence++;
-	xnarch_write_memory_barrier();
-}
-
-static inline void write_seqcount_end(seqcount_t *s)
-{
-	xnarch_write_memory_barrier();
-	s->sequence++;
-}
-
-#endif
diff --git a/include/nucleus/vdso.h b/include/nucleus/vdso.h
index bce4c5a..279574b 100644
--- a/include/nucleus/vdso.h
+++ b/include/nucleus/vdso.h
@@ -34,7 +34,7 @@
 struct xnvdso {
 	unsigned long long features;
 
-	struct xnarch_hostrt_data hostrt_data;
+	struct xnvdso_hostrt_data hostrt_data;
 	/*
 	 * Embed further domain specific structures that
 	 * describe the shared data here
@@ -61,7 +61,7 @@ struct xnvdso {
 
 extern struct xnvdso *nkvdso;
 
-static inline struct xnarch_hostrt_data *get_hostrt_data(void)
+static inline struct xnvdso_hostrt_data *get_hostrt_data(void)
 {
 	return &nkvdso->hostrt_data;
 }
diff --git a/ksrc/nucleus/module.c b/ksrc/nucleus/module.c
index 3f962ba..fc8ceac 100644
--- a/ksrc/nucleus/module.c
+++ b/ksrc/nucleus/module.c
@@ -69,7 +69,7 @@ static inline void do_hostrt_event(struct xnarch_hostrt_data *hostrt)
 	 */
 
 	spin_lock_irqsave(&__hostrtlock, flags);
-	write_seqcount_begin(&nkvdso->hostrt_data.seqcount);
+	xnwrite_seqcount_begin(&nkvdso->hostrt_data.seqcount);
 
 	nkvdso->hostrt_data.live = 1;
 	nkvdso->hostrt_data.cycle_last = hostrt->cycle_last;
@@ -80,7 +80,7 @@ static inline void do_hostrt_event(struct xnarch_hostrt_data *hostrt)
 	nkvdso->hostrt_data.wall_time_nsec = hostrt->wall_time_nsec;
 	nkvdso->hostrt_data.wall_to_monotonic = hostrt->wall_to_monotonic;
 
-	write_seqcount_end(&nkvdso->hostrt_data.seqcount);
+	xnwrite_seqcount_end(&nkvdso->hostrt_data.seqcount);
 	spin_unlock_irqrestore(&__hostrtlock, flags);
 }
 
diff --git a/src/skins/posix/clock.c b/src/skins/posix/clock.c
index 7232c48..4f2171b 100644
--- a/src/skins/posix/clock.c
+++ b/src/skins/posix/clock.c
@@ -25,7 +25,6 @@
 #include <time.h>
 #include <asm/xenomai/arith.h>
 #include <asm-generic/xenomai/timeconv.h>
-#include <nucleus/seqlock_user.h>
 #include <sys/types.h>
 #include <nucleus/vdso.h>
 
@@ -63,9 +62,9 @@ int __do_clock_host_realtime(struct timespec *ts, void *tzp)
 {
 #ifdef XNARCH_HAVE_NONPRIV_TSC
 	unsigned int seq;
-	cycle_t now, base, mask, cycle_delta;
+	unsigned long long now, base, mask, cycle_delta;
 	unsigned long mult, shift, nsec, rem;
-	struct xnarch_hostrt_data *hostrt_data;
+	struct xnvdso_hostrt_data *hostrt_data;
 
 	if (!xnvdso_test_feature(XNVDSO_FEAT_HOST_REALTIME))
 		return -1;
@@ -80,7 +79,7 @@ int __do_clock_host_realtime(struct timespec *ts, void *tzp)
 	 * mechanism in the kernel.
 	 */
 retry:
-	seq = read_seqcount_begin(&hostrt_data->seqcount);
+	seq = xnread_seqcount_begin(&hostrt_data->seqcount);
 
 	now = __xn_rdtsc();
 	base = hostrt_data->cycle_last;
@@ -92,7 +91,7 @@ retry:
 
 	/* If the data changed during the read, try the
 	   alternative data element */
-	if (read_seqcount_retry(&hostrt_data->seqcount, seq))
+	if (xnread_seqcount_retry(&hostrt_data->seqcount, seq))
 		goto retry;
 
 	cycle_delta = (now - base) & mask;
diff --git a/src/testsuite/clocktest/clocktest.c b/src/testsuite/clocktest/clocktest.c
index bf4feb6..038cfbc 100644
--- a/src/testsuite/clocktest/clocktest.c
+++ b/src/testsuite/clocktest/clocktest.c
@@ -99,8 +99,8 @@ static void show_hostrt_diagnostics(void)
 	       (intmax_t)nkvdso->hostrt_data.wall_to_monotonic.tv_sec);
 	printf("         tv_nsec : %ld\n",
 	       nkvdso->hostrt_data.wall_to_monotonic.tv_nsec);
-	printf("cycle_last       : %lu\n", nkvdso->hostrt_data.cycle_last);
-	printf("mask             : 0x%lx\n", nkvdso->hostrt_data.mask);
+	printf("cycle_last       : %Lu\n", nkvdso->hostrt_data.cycle_last);
+	printf("mask             : 0x%Lx\n", nkvdso->hostrt_data.mask);
 	printf("mult             : %u\n", nkvdso->hostrt_data.mult);
 	printf("shift            : %u\n\n", nkvdso->hostrt_data.shift);
 }

Everybody OK?

-- 
                                                                Gilles.

Re: [Xenomai-core] hostrt broken on ARM.

[Wolfgang Mauerer]

Gilles Chanteperdrix wrote:

> the hostrt stuff did not compile on ARM, at this chance, I had a look 
> and found a few things which I did not like:
> - the kernel-space and user-space did not use seqcount implementations 
> from the same header, and what is more, seqcount is not available on 2.4
> kernels. Since we provide an implementation anyway, I changed the code 
> so that only this implementation is used both in kernel-space and 
> user-space. Prefixing everything with xn to avoid namespace clashes.
that makes sense, good point!

> - the xnarch_hostrt_data also had two different definitions, one for 
> user-space, one for kernel-space, since this definition is supposed to 
> come from newer I-pipe in kernel-space, the code did not compile with 
> older I-pipe patches. I separated xnarch_hostrt_data, the structure 
> passed by the I-pipe to do_hostrt_event, from xnvdso_hostrt_data, the 
> structure, part of the vdso used for exchanges between user-space and 
> kernel-space. We can now get xnvdso_hostrt_data defined all the time. We
> can probably even move xnvdso_hostrt_data definition to vdso.h
I assume your main intention is to fix the Xenomai compile when the
underlying ipipe does not support hostrt? But when the ipipe layer is
too old for hostrt, it does not really make sense to include hostrt
support in the vdso -- it may be cleaner to define an empty
stub for struct xnarch_hostrt when ipipe does not provide hostrt info.

> - avoided cycle_t and u32 in order to avoid user-space namespace 
> pollution and need for many typedefs with 2.4 kernels. Using unsigned 
> long long directly instead of cycle_t revealed a bug in clocktest.c.

While typedefs are generally debatable, the intention here was to
create as much symmetry with the Linux kernel implementation as
possible since this code is fairly well tested. I cannot
point to any particular problem with the non-fixed-width definitions,
but with them, we do at least avoid the possibility of introducing
any scaled math issues.

> 
> Here is the patch.
> 
> diff --git a/include/nucleus/Makefile.am b/include/nucleus/Makefile.am
> index 9a72b24..5fc1c21 100644
> diff --git a/include/nucleus/Makefile.in b/include/nucleus/Makefile.in
> index 5e93dc9..a6fa801 100644
> diff --git a/include/nucleus/hostrt.h b/include/nucleus/hostrt.h
> index 70ffbfe..85b8e88 100644
> --- a/include/nucleus/hostrt.h
> +++ b/include/nucleus/hostrt.h
> @@ -23,32 +23,24 @@
>   * 02111-1307, USA.
>   */
>  
> -#include <nucleus/types.h>
> -#ifdef __KERNEL__
> -#include <asm-generic/xenomai/system.h>
> -#else /* !__KERNEL__ */
> +#ifndef __KERNEL__
>  #include <time.h>
>  #include <sys/types.h>
> -#include <nucleus/seqlock_user.h>
> -typedef u_int32_t u32;
> -typedef u_int64_t cycle_t;
> +#else /* __KERNEL__ */
> +#include <asm-generic/xenomai/system.h>
> +#endif /* __KERNEL__ */
> +#include <nucleus/seqlock.h>
>  
> -/*
> - * xnarch_hostrt_data must be kept in sync with the corresponding ipipe
> - * definitions in ipipe_tickdev.h We cannot directly include the file
> - * here because the definitions are also required in Xenomai userland.
> - */
I think this comment may have its value.

Thanks, Wolfgang

> -struct xnarch_hostrt_data {
> +struct xnvdso_hostrt_data {
>  	short live;
> -	seqcount_t seqcount;
> +	xnseqcount_t seqcount;
>  	time_t wall_time_sec;
> -	u32 wall_time_nsec;
> +	unsigned wall_time_nsec;
>  	struct timespec wall_to_monotonic;
> -	cycle_t cycle_last;
> -	cycle_t mask;
> -	u32 mult;
> -	u32 shift;
> +	unsigned long long cycle_last;
> +	unsigned long long mask;
> +	unsigned mult;
> +	unsigned shift;
>  };
> -#endif /* !__KERNEL__ */
>  
>  #endif
> diff --git a/include/nucleus/seqlock.h b/include/nucleus/seqlock.h
> new file mode 100644
> index 0000000..897d411
> --- /dev/null
> +++ b/include/nucleus/seqlock.h
> @@ -0,0 +1,57 @@
> +#ifndef __SEQLOCK_H
> +#define __SEQLOCK_H
> +
> +/* Originally from the linux kernel, adapted for userland and Xenomai */
> +
> +#include <asm/xenomai/atomic.h>
> +
> +typedef struct xnseqcount {
> +	unsigned sequence;
> +} xnseqcount_t;
> +
> +#define XNSEQCNT_ZERO { 0 }
> +#define xnseqcount_init(x) do { *(x) = (xnseqcount_t) SEQCNT_ZERO; } while (0)
> +
> +/* Start of read using pointer to a sequence counter only.  */
> +static inline unsigned xnread_seqcount_begin(const xnseqcount_t *s)
> +{
> +	unsigned ret;
> +
> +repeat:
> +	ret = s->sequence;
> +	xnarch_read_memory_barrier();
> +	if (unlikely(ret & 1)) {
> +		cpu_relax();
> +		goto repeat;
> +	}
> +	return ret;
> +}
> +
> +/*
> + * Test if reader processed invalid data because sequence number has changed.
> + */
> +static inline int xnread_seqcount_retry(const xnseqcount_t *s, unsigned start)
> +{
> +	xnarch_read_memory_barrier();
> +
> +	return s->sequence != start;
> +}
> +
> +
> +/*
> + * The sequence counter only protects readers from concurrent writers.
> + * Writers must use their own locking.
> + */
> +static inline void xnwrite_seqcount_begin(xnseqcount_t *s)
> +{
> +	s->sequence++;
> +	xnarch_write_memory_barrier();
> +}
> +
> +static inline void xnwrite_seqcount_end(xnseqcount_t *s)
> +{
> +	xnarch_write_memory_barrier();
> +	s->sequence++;
> +}
> +
> +#endif /* __SEQLOCK_H */
> diff --git a/include/nucleus/seqlock_user.h b/include/nucleus/seqlock_user.h
> deleted file mode 100644
> index 598d4da..0000000
> --- a/include/nucleus/seqlock_user.h
> +++ /dev/null
> @@ -1,57 +0,0 @@
> -#ifndef __SEQLOCK_USER_H
> -#define __SEQLOCK_USER_H
> -
> -/* Originally from the linux kernel, adapted for userland and Xenomai */
> -
> -#include <asm/xenomai/atomic.h>
> -
> -typedef struct seqcount {
> -	unsigned sequence;
> -} seqcount_t;
> -
> -#define SEQCNT_ZERO { 0 }
> -#define seqcount_init(x) do { *(x) = (seqcount_t) SEQCNT_ZERO; } while (0)
> -
> -/* Start of read using pointer to a sequence counter only.  */
> -static inline unsigned read_seqcount_begin(const seqcount_t *s)
> -{
> -	unsigned ret;
> -
> -repeat:
> -	ret = s->sequence;
> -	xnarch_read_memory_barrier();
> -	if (unlikely(ret & 1)) {
> -		cpu_relax();
> -		goto repeat;
> -	}
> -	return ret;
> -}
> -
> -/*
> - * Test if reader processed invalid data because sequence number has changed.
> - */
> -static inline int read_seqcount_retry(const seqcount_t *s, unsigned start)
> -{
> -	xnarch_read_memory_barrier();
> -
> -	return s->sequence != start;
> -}
> -
> -
> -/*
> - * The sequence counter only protects readers from concurrent writers.
> - * Writers must use their own locking.
> - */
> -static inline void write_seqcount_begin(seqcount_t *s)
> -{
> -	s->sequence++;
> -	xnarch_write_memory_barrier();
> -}
> -
> -static inline void write_seqcount_end(seqcount_t *s)
> -{
> -	xnarch_write_memory_barrier();
> -	s->sequence++;
> -}
> -
> -#endif
> diff --git a/include/nucleus/vdso.h b/include/nucleus/vdso.h
> index bce4c5a..279574b 100644
> --- a/include/nucleus/vdso.h
> +++ b/include/nucleus/vdso.h
> @@ -34,7 +34,7 @@
>  struct xnvdso {
>  	unsigned long long features;
>  
> -	struct xnarch_hostrt_data hostrt_data;
> +	struct xnvdso_hostrt_data hostrt_data;
>  	/*
>  	 * Embed further domain specific structures that
>  	 * describe the shared data here
> @@ -61,7 +61,7 @@ struct xnvdso {
>  
>  extern struct xnvdso *nkvdso;
>  
> -static inline struct xnarch_hostrt_data *get_hostrt_data(void)
> +static inline struct xnvdso_hostrt_data *get_hostrt_data(void)
>  {
>  	return &nkvdso->hostrt_data;
>  }
> diff --git a/ksrc/nucleus/module.c b/ksrc/nucleus/module.c
> index 3f962ba..fc8ceac 100644
> --- a/ksrc/nucleus/module.c
> +++ b/ksrc/nucleus/module.c
> @@ -69,7 +69,7 @@ static inline void do_hostrt_event(struct xnarch_hostrt_data *hostrt)
>  	 */
>  
>  	spin_lock_irqsave(&__hostrtlock, flags);
> -	write_seqcount_begin(&nkvdso->hostrt_data.seqcount);
> +	xnwrite_seqcount_begin(&nkvdso->hostrt_data.seqcount);
>  
>  	nkvdso->hostrt_data.live = 1;
>  	nkvdso->hostrt_data.cycle_last = hostrt->cycle_last;
> @@ -80,7 +80,7 @@ static inline void do_hostrt_event(struct xnarch_hostrt_data *hostrt)
>  	nkvdso->hostrt_data.wall_time_nsec = hostrt->wall_time_nsec;
>  	nkvdso->hostrt_data.wall_to_monotonic = hostrt->wall_to_monotonic;
>  
> -	write_seqcount_end(&nkvdso->hostrt_data.seqcount);
> +	xnwrite_seqcount_end(&nkvdso->hostrt_data.seqcount);
>  	spin_unlock_irqrestore(&__hostrtlock, flags);
>  }
>  
> diff --git a/src/skins/posix/clock.c b/src/skins/posix/clock.c
> index 7232c48..4f2171b 100644
> --- a/src/skins/posix/clock.c
> +++ b/src/skins/posix/clock.c
> @@ -25,7 +25,6 @@
>  #include <time.h>
>  #include <asm/xenomai/arith.h>
>  #include <asm-generic/xenomai/timeconv.h>
> -#include <nucleus/seqlock_user.h>
>  #include <sys/types.h>
>  #include <nucleus/vdso.h>
>  
> @@ -63,9 +62,9 @@ int __do_clock_host_realtime(struct timespec *ts, void *tzp)
>  {
>  #ifdef XNARCH_HAVE_NONPRIV_TSC
>  	unsigned int seq;
> -	cycle_t now, base, mask, cycle_delta;
> +	unsigned long long now, base, mask, cycle_delta;
>  	unsigned long mult, shift, nsec, rem;
> -	struct xnarch_hostrt_data *hostrt_data;
> +	struct xnvdso_hostrt_data *hostrt_data;
>  
>  	if (!xnvdso_test_feature(XNVDSO_FEAT_HOST_REALTIME))
>  		return -1;
> @@ -80,7 +79,7 @@ int __do_clock_host_realtime(struct timespec *ts, void *tzp)
>  	 * mechanism in the kernel.
>  	 */
>  retry:
> -	seq = read_seqcount_begin(&hostrt_data->seqcount);
> +	seq = xnread_seqcount_begin(&hostrt_data->seqcount);
>  
>  	now = __xn_rdtsc();
>  	base = hostrt_data->cycle_last;
> @@ -92,7 +91,7 @@ retry:
>  
>  	/* If the data changed during the read, try the
>  	   alternative data element */
> -	if (read_seqcount_retry(&hostrt_data->seqcount, seq))
> +	if (xnread_seqcount_retry(&hostrt_data->seqcount, seq))
>  		goto retry;
>  
>  	cycle_delta = (now - base) & mask;
> diff --git a/src/testsuite/clocktest/clocktest.c b/src/testsuite/clocktest/clocktest.c
> index bf4feb6..038cfbc 100644
> --- a/src/testsuite/clocktest/clocktest.c
> +++ b/src/testsuite/clocktest/clocktest.c
> @@ -99,8 +99,8 @@ static void show_hostrt_diagnostics(void)
>  	       (intmax_t)nkvdso->hostrt_data.wall_to_monotonic.tv_sec);
>  	printf("         tv_nsec : %ld\n",
>  	       nkvdso->hostrt_data.wall_to_monotonic.tv_nsec);
> -	printf("cycle_last       : %lu\n", nkvdso->hostrt_data.cycle_last);
> -	printf("mask             : 0x%lx\n", nkvdso->hostrt_data.mask);
> +	printf("cycle_last       : %Lu\n", nkvdso->hostrt_data.cycle_last);
> +	printf("mask             : 0x%Lx\n", nkvdso->hostrt_data.mask);
>  	printf("mult             : %u\n", nkvdso->hostrt_data.mult);
>  	printf("shift            : %u\n\n", nkvdso->hostrt_data.shift);
>  }
> 
> Everybody OK?
> 

Re: [Xenomai-core] hostrt broken on ARM.

[Gilles Chanteperdrix]

Wolfgang Mauerer wrote:
> Gilles Chanteperdrix wrote:
> 
>> the hostrt stuff did not compile on ARM, at this chance, I had a look 
>> and found a few things which I did not like:
>> - the kernel-space and user-space did not use seqcount implementations 
>> from the same header, and what is more, seqcount is not available on 2.4
>> kernels. Since we provide an implementation anyway, I changed the code 
>> so that only this implementation is used both in kernel-space and 
>> user-space. Prefixing everything with xn to avoid namespace clashes.
> that makes sense, good point!
> 
>> - the xnarch_hostrt_data also had two different definitions, one for 
>> user-space, one for kernel-space, since this definition is supposed to 
>> come from newer I-pipe in kernel-space, the code did not compile with 
>> older I-pipe patches. I separated xnarch_hostrt_data, the structure 
>> passed by the I-pipe to do_hostrt_event, from xnvdso_hostrt_data, the 
>> structure, part of the vdso used for exchanges between user-space and 
>> kernel-space. We can now get xnvdso_hostrt_data defined all the time. We
>> can probably even move xnvdso_hostrt_data definition to vdso.h
> I assume your main intention is to fix the Xenomai compile when the
> underlying ipipe does not support hostrt? But when the ipipe layer is
> too old for hostrt, it does not really make sense to include hostrt
> support in the vdso -- it may be cleaner to define an empty
> stub for struct xnarch_hostrt when ipipe does not provide hostrt info.

No. The point is to avoid the comment: "hell will break loose if this
struct is not keep in sync with one defined by the I-pipe". Having two
pieces of code relying on different headers to get the same structure
defined is really shooting ourself in the foot and asking for more. The
only case where we can admit that is when one piece of code is assembly.
And even then, it is dangerous (look at the linux kernel code, the
developers devised asm-offsets.h to solve exactly this issue).

Besides, having two structures defined for the two different layers
allows us to modify one structure without any impact on the other.

Keeping the structure defined even if the I-pipe does not define it is
just a side effect which avoids #ifdefery. The vdso is part of a heap
mapped in user-space, so the granularity is 4 Kbi, we do not really care
about a structure we do not use. But this is debatable, I agree.

> 
>> - avoided cycle_t and u32 in order to avoid user-space namespace 
>> pollution and need for many typedefs with 2.4 kernels. Using unsigned 
>> long long directly instead of cycle_t revealed a bug in clocktest.c.
> 
> While typedefs are generally debatable, the intention here was to
> create as much symmetry with the Linux kernel implementation as
> possible since this code is fairly well tested. I cannot
> point to any particular problem with the non-fixed-width definitions,
> but with them, we do at least avoid the possibility of introducing
> any scaled math issues.

unsigned long long is 64 bits
unsigned is 32 bits
unsigned long has the natural processor word size.
So, in reality, they are fixed width definitions. Portable accross all
architectures supported by Linux. No typedefs needed.

> 
>> Here is the patch.
>>
>> diff --git a/include/nucleus/Makefile.am b/include/nucleus/Makefile.am
>> index 9a72b24..5fc1c21 100644
>> diff --git a/include/nucleus/Makefile.in b/include/nucleus/Makefile.in
>> index 5e93dc9..a6fa801 100644
>> diff --git a/include/nucleus/hostrt.h b/include/nucleus/hostrt.h
>> index 70ffbfe..85b8e88 100644
>> --- a/include/nucleus/hostrt.h
>> +++ b/include/nucleus/hostrt.h
>> @@ -23,32 +23,24 @@
>>   * 02111-1307, USA.
>>   */
>>  
>> -#include <nucleus/types.h>
>> -#ifdef __KERNEL__
>> -#include <asm-generic/xenomai/system.h>
>> -#else /* !__KERNEL__ */
>> +#ifndef __KERNEL__
>>  #include <time.h>
>>  #include <sys/types.h>
>> -#include <nucleus/seqlock_user.h>
>> -typedef u_int32_t u32;
>> -typedef u_int64_t cycle_t;
>> +#else /* __KERNEL__ */
>> +#include <asm-generic/xenomai/system.h>
>> +#endif /* __KERNEL__ */
>> +#include <nucleus/seqlock.h>
>>  
>> -/*
>> - * xnarch_hostrt_data must be kept in sync with the corresponding ipipe
>> - * definitions in ipipe_tickdev.h We cannot directly include the file
>> - * here because the definitions are also required in Xenomai userland.
>> - */
> I think this comment may have its value.

No. For the reason explained above.

-- 
                                                                Gilles.

Re: [Xenomai-core] hostrt broken on ARM.

[Wolfgang Mauerer]

Gilles Chanteperdrix wrote:
> Wolfgang Mauerer wrote:
>> Gilles Chanteperdrix wrote:
>>
>>> the hostrt stuff did not compile on ARM, at this chance, I had a look 
>>> and found a few things which I did not like:
>>> - the kernel-space and user-space did not use seqcount implementations 
>>> from the same header, and what is more, seqcount is not available on 2.4
>>> kernels. Since we provide an implementation anyway, I changed the code 
>>> so that only this implementation is used both in kernel-space and 
>>> user-space. Prefixing everything with xn to avoid namespace clashes.
>> that makes sense, good point!
>>
>>> - the xnarch_hostrt_data also had two different definitions, one for 
>>> user-space, one for kernel-space, since this definition is supposed to 
>>> come from newer I-pipe in kernel-space, the code did not compile with 
>>> older I-pipe patches. I separated xnarch_hostrt_data, the structure 
>>> passed by the I-pipe to do_hostrt_event, from xnvdso_hostrt_data, the 
>>> structure, part of the vdso used for exchanges between user-space and 
>>> kernel-space. We can now get xnvdso_hostrt_data defined all the time. We
>>> can probably even move xnvdso_hostrt_data definition to vdso.h
>> I assume your main intention is to fix the Xenomai compile when the
>> underlying ipipe does not support hostrt? But when the ipipe layer is
>> too old for hostrt, it does not really make sense to include hostrt
>> support in the vdso -- it may be cleaner to define an empty
>> stub for struct xnarch_hostrt when ipipe does not provide hostrt info.
> 
> No. The point is to avoid the comment: "hell will break loose if this
> struct is not keep in sync with one defined by the I-pipe". Having two
> pieces of code relying on different headers to get the same structure
> defined is really shooting ourself in the foot and asking for more. The
> only case where we can admit that is when one piece of code is assembly.
> And even then, it is dangerous (look at the linux kernel code, the
> developers devised asm-offsets.h to solve exactly this issue).
> 
> Besides, having two structures defined for the two different layers
> allows us to modify one structure without any impact on the other.
> 
> Keeping the structure defined even if the I-pipe does not define it is
> just a side effect which avoids #ifdefery. The vdso is part of a heap
> mapped in user-space, so the granularity is 4 Kbi, we do not really care
> about a structure we do not use. But this is debatable, I agree.
It'd be nicer to eliminate it when there's no support for it, but no
religious issue here.
> 
>>> - avoided cycle_t and u32 in order to avoid user-space namespace 
>>> pollution and need for many typedefs with 2.4 kernels. Using unsigned 
>>> long long directly instead of cycle_t revealed a bug in clocktest.c.
>> While typedefs are generally debatable, the intention here was to
>> create as much symmetry with the Linux kernel implementation as
>> possible since this code is fairly well tested. I cannot
>> point to any particular problem with the non-fixed-width definitions,
>> but with them, we do at least avoid the possibility of introducing
>> any scaled math issues.
> 
> unsigned long long is 64 bits
> unsigned is 32 bits
> unsigned long has the natural processor word size.
> So, in reality, they are fixed width definitions. Portable accross all
> architectures supported by Linux. No typedefs needed.

that's correct for almost all programming models (and all reasonable
ones, and certainly for everything the kernel runs on AFIK), but I still
don't see a benefit in removing analogies to the kernel for no
particular gain. A few lines before, you argued that things should
resemble the kernel approach.

Best regards,

Wolfgang
> 
>>> Here is the patch.
>>>
>>> diff --git a/include/nucleus/Makefile.am b/include/nucleus/Makefile.am
>>> index 9a72b24..5fc1c21 100644
>>> diff --git a/include/nucleus/Makefile.in b/include/nucleus/Makefile.in
>>> index 5e93dc9..a6fa801 100644
>>> diff --git a/include/nucleus/hostrt.h b/include/nucleus/hostrt.h
>>> index 70ffbfe..85b8e88 100644
>>> --- a/include/nucleus/hostrt.h
>>> +++ b/include/nucleus/hostrt.h
>>> @@ -23,32 +23,24 @@
>>>   * 02111-1307, USA.
>>>   */
>>>  
>>> -#include <nucleus/types.h>
>>> -#ifdef __KERNEL__
>>> -#include <asm-generic/xenomai/system.h>
>>> -#else /* !__KERNEL__ */
>>> +#ifndef __KERNEL__
>>>  #include <time.h>
>>>  #include <sys/types.h>
>>> -#include <nucleus/seqlock_user.h>
>>> -typedef u_int32_t u32;
>>> -typedef u_int64_t cycle_t;
>>> +#else /* __KERNEL__ */
>>> +#include <asm-generic/xenomai/system.h>
>>> +#endif /* __KERNEL__ */
>>> +#include <nucleus/seqlock.h>
>>>  
>>> -/*
>>> - * xnarch_hostrt_data must be kept in sync with the corresponding ipipe
>>> - * definitions in ipipe_tickdev.h We cannot directly include the file
>>> - * here because the definitions are also required in Xenomai userland.
>>> - */
>> I think this comment may have its value.
> 
> No. For the reason explained above.
> 

Re: [Xenomai-core] hostrt broken on ARM.

[Gilles Chanteperdrix]

Wolfgang Mauerer wrote:
> Gilles Chanteperdrix wrote:
>> Wolfgang Mauerer wrote:
>>> Gilles Chanteperdrix wrote:
>>>
>>>> the hostrt stuff did not compile on ARM, at this chance, I had a look 
>>>> and found a few things which I did not like:
>>>> - the kernel-space and user-space did not use seqcount implementations 
>>>> from the same header, and what is more, seqcount is not available on 2.4
>>>> kernels. Since we provide an implementation anyway, I changed the code 
>>>> so that only this implementation is used both in kernel-space and 
>>>> user-space. Prefixing everything with xn to avoid namespace clashes.
>>> that makes sense, good point!
>>>
>>>> - the xnarch_hostrt_data also had two different definitions, one for 
>>>> user-space, one for kernel-space, since this definition is supposed to 
>>>> come from newer I-pipe in kernel-space, the code did not compile with 
>>>> older I-pipe patches. I separated xnarch_hostrt_data, the structure 
>>>> passed by the I-pipe to do_hostrt_event, from xnvdso_hostrt_data, the 
>>>> structure, part of the vdso used for exchanges between user-space and 
>>>> kernel-space. We can now get xnvdso_hostrt_data defined all the time. We
>>>> can probably even move xnvdso_hostrt_data definition to vdso.h
>>> I assume your main intention is to fix the Xenomai compile when the
>>> underlying ipipe does not support hostrt? But when the ipipe layer is
>>> too old for hostrt, it does not really make sense to include hostrt
>>> support in the vdso -- it may be cleaner to define an empty
>>> stub for struct xnarch_hostrt when ipipe does not provide hostrt info.
>> No. The point is to avoid the comment: "hell will break loose if this
>> struct is not keep in sync with one defined by the I-pipe". Having two
>> pieces of code relying on different headers to get the same structure
>> defined is really shooting ourself in the foot and asking for more. The
>> only case where we can admit that is when one piece of code is assembly.
>> And even then, it is dangerous (look at the linux kernel code, the
>> developers devised asm-offsets.h to solve exactly this issue).
>>
>> Besides, having two structures defined for the two different layers
>> allows us to modify one structure without any impact on the other.
>>
>> Keeping the structure defined even if the I-pipe does not define it is
>> just a side effect which avoids #ifdefery. The vdso is part of a heap
>> mapped in user-space, so the granularity is 4 Kbi, we do not really care
>> about a structure we do not use. But this is debatable, I agree.
> It'd be nicer to eliminate it when there's no support for it, but no
> religious issue here.

The point is that if we want to do that, we have a user-space issue,
user-space has no CONFIG_* to know whether the structure definition is
needed. So, having the structure defined at all times is the simpler
solution.

>>>> - avoided cycle_t and u32 in order to avoid user-space namespace 
>>>> pollution and need for many typedefs with 2.4 kernels. Using unsigned 
>>>> long long directly instead of cycle_t revealed a bug in clocktest.c.
>>> While typedefs are generally debatable, the intention here was to
>>> create as much symmetry with the Linux kernel implementation as
>>> possible since this code is fairly well tested. I cannot
>>> point to any particular problem with the non-fixed-width definitions,
>>> but with them, we do at least avoid the possibility of introducing
>>> any scaled math issues.
>> unsigned long long is 64 bits
>> unsigned is 32 bits
>> unsigned long has the natural processor word size.
>> So, in reality, they are fixed width definitions. Portable accross all
>> architectures supported by Linux. No typedefs needed.
> 
> that's correct for almost all programming models (and all reasonable
> ones, and certainly for everything the kernel runs on AFIK), but I still
> don't see a benefit in removing analogies to the kernel for no
> particular gain. A few lines before, you argued that things should
> resemble the kernel approach.

The gain is #ifdefery again, with multiple typedefs which need to be
tested in all conditions in order to be sure that we do not clash or
miss the definition in some conditions (2.4 kernel for instance).
Besides, with the cycle_t define, we had an undetected bug in
clocktest.c. And typedefs would leak in user-space. I do not really like
to take the decision to forbid Xenomai users to use the cycle_t typedef
in their application for their own purposes.

-- 
					    Gilles.