Re: [Xenomai-core] hostrt broken on ARM.
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 000..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++;
+
Re: [Xenomai-core] hostrt broken on ARM.
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. ___ Xenomai-core mailing list Xenomai-core@gna.org https://mail.gna.org/listinfo/xenomai-core
Re: [Xenomai-core] hostrt broken on ARM.
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. ___ Xenomai-core mailing list Xenomai-core@gna.org https://mail.gna.org/listinfo/xenomai-core
