[Xenomai-core] [PATCH] Add support for sharing kernel/userland data between Xenomai and Linux

[Xenomai-core] [PATCH] Add support for sharing kernel/userland data between Xenomai and Linux

[wolfgang.mauerer]

From: Wolfgang Mauerer <wolfgang.mauerer@domain.hid>

A new structure (struct xnshared) is introduced. It allows us to share
generic data between user and kernel of Linux and Xenomai; a bitmap
of feature flags located at the beginning of the structure identifies
which data are shared. The structure is allocated in the global semaphore
heap, and xnsysinfo.xnshared_offset identifies the offset of the
structure within the heap.

Currently, no shared features are yet supported - the patch only
introduces the necessary ABI changes. When the need arises
to share data between said entities, the structure must
be accordingly extended, and a new feature bit must be added
to the flags.

Signed-off-by: Wolfgang Mauerer <wolfgang.mauerer@domain.hid>
Signed-off-by: Jan Kiszka <jan.kiszka@domain.hid>
---
 include/asm-generic/syscall.h |    1 +
 include/nucleus/xnshared.h    |   33 +++++++++++++++++++++++++++++++++
 ksrc/nucleus/module.c         |   23 +++++++++++++++++++++++
 ksrc/nucleus/shadow.c         |    4 ++++
 4 files changed, 61 insertions(+), 0 deletions(-)
 create mode 100644 include/nucleus/xnshared.h

diff --git a/include/asm-generic/syscall.h b/include/asm-generic/syscall.h
index 483b99f..7d68580 100644
--- a/include/asm-generic/syscall.h
+++ b/include/asm-generic/syscall.h
@@ -53,6 +53,7 @@
 typedef struct xnsysinfo {
 	unsigned long long cpufreq;	/* CPU frequency */
 	unsigned long tickval;		/* Tick duration (ns) */
+	unsigned long xnshared_offset;  /* Offset of xnshared in the sem heap */
 } xnsysinfo_t;
 
 #define SIGSHADOW  SIGWINCH
diff --git a/include/nucleus/xnshared.h b/include/nucleus/xnshared.h
new file mode 100644
index 0000000..4293cda
--- /dev/null
+++ b/include/nucleus/xnshared.h
@@ -0,0 +1,33 @@
+#ifndef XNSHARED_H
+#define XNSHARED_H
+
+/*
+ * Data shared between Xenomai kernel/userland and the Linux kernel/userland
+ * on the global semaphore heap. The features element indicates which data are
+ * shared. Notice that xnshared may only grow, but never shrink.
+ */
+struct xnshared {
+	unsigned long long features;
+
+	/* Embed domain specific structures that describe the
+	 * shared data here */
+};
+
+/*
+ * For each shared feature, add a flag below. For now, it is still
+ * empty.
+ */
+enum xnshared_features {
+/*	XNSHARED_FEAT_A = 1,
+	XNSHARED_FEAT_B, */
+	XNSHARED_MAX_FEATURES,
+};
+
+extern struct xnshared *xnshared;
+
+static inline int xnshared_test_feature(enum xnshared_features feature)
+{
+	return xnshared && xnshared->features & (1 << feature);
+}
+
+#endif
diff --git a/ksrc/nucleus/module.c b/ksrc/nucleus/module.c
index 5404182..3963fbd 100644
--- a/ksrc/nucleus/module.c
+++ b/ksrc/nucleus/module.c
@@ -34,6 +34,7 @@
 #include <nucleus/pipe.h>
 #endif /* CONFIG_XENO_OPT_PIPE */
 #include <nucleus/select.h>
+#include <nucleus/xnshared.h>
 #include <asm/xenomai/bits/init.h>
 
 MODULE_DESCRIPTION("Xenomai nucleus");
@@ -51,6 +52,9 @@ u_long xnmod_sysheap_size;
 
 int xeno_nucleus_status = -EINVAL;
 
+struct xnshared *xnshared;
+EXPORT_SYMBOL_GPL(xnshared);
+
 struct xnsys_ppd __xnsys_global_ppd;
 EXPORT_SYMBOL_GPL(__xnsys_global_ppd);
 
@@ -82,6 +86,23 @@ void xnmod_alloc_glinks(xnqueue_t *freehq)
 }
 EXPORT_SYMBOL_GPL(xnmod_alloc_glinks);
 
+/*
+ * We re-use the global semaphore heap to provide a multi-purpose shared
+ * memory area between Xenomai and Linux - for both kernel and userland
+ */
+void __init xnheap_init_shared(void)
+{
+	xnshared = (struct xnshared *)xnheap_alloc(&__xnsys_global_ppd.sem_heap,
+						  sizeof(struct xnshared));
+
+	if (!xnshared)
+		xnpod_fatal("Xenomai: cannot allocate memory for xnshared!\n");
+
+	/* Currently, we don't support any sharing, but later versions will
+	 * add flags here */
+	xnshared->features = 0ULL;
+}
+
 int __init __xeno_sys_init(void)
 {
 	int ret;
@@ -106,6 +127,8 @@ int __init __xeno_sys_init(void)
 		goto cleanup_arch;
 
 	xnheap_set_label(&__xnsys_global_ppd.sem_heap, "global sem heap");
+
+	xnheap_init_shared();
 #endif
 	
 #ifdef __KERNEL__
diff --git a/ksrc/nucleus/shadow.c b/ksrc/nucleus/shadow.c
index 0c94a60..0373a8d 100644
--- a/ksrc/nucleus/shadow.c
+++ b/ksrc/nucleus/shadow.c
@@ -50,6 +50,7 @@
 #include <nucleus/trace.h>
 #include <nucleus/stat.h>
 #include <nucleus/sys_ppd.h>
+#include <nucleus/xnshared.h>
 #include <asm/xenomai/features.h>
 #include <asm/xenomai/syscall.h>
 #include <asm/xenomai/bits/shadow.h>
@@ -1746,6 +1747,9 @@ static int xnshadow_sys_info(struct pt_regs *regs)
 
 	info.cpufreq = xnarch_get_cpu_freq();
 
+	info.xnshared_offset =
+	  xnheap_mapped_offset(&xnsys_ppd_get(1)->sem_heap, xnshared);
+
 	return __xn_safe_copy_to_user((void __user *)infarg, &info, sizeof(info));
 }
 
-- 
1.6.4

Re: [Xenomai-core] [PATCH] Add support for sharing kernel/userland data between Xenomai and Linux

[Gilles Chanteperdrix]

wolfgang.mauerer@domain.hid wrote:
> From: Wolfgang Mauerer <wolfgang.mauerer@domain.hid>
> 
> A new structure (struct xnshared) is introduced. It allows us to share
> generic data between user and kernel of Linux and Xenomai; a bitmap
> of feature flags located at the beginning of the structure identifies
> which data are shared. The structure is allocated in the global semaphore
> heap, and xnsysinfo.xnshared_offset identifies the offset of the
> structure within the heap.
> 
> Currently, no shared features are yet supported - the patch only
> introduces the necessary ABI changes. When the need arises
> to share data between said entities, the structure must
> be accordingly extended, and a new feature bit must be added
> to the flags.

Hi,

it is nice that you proposed that patch, I wanted to do it, but did not
yet. Some comments however.

First, I think xnshared/xnshared_offset is a bit of a long name. I was
thinking about xnvdso, but is is only slightly shorter. Any other idea,
anyone?

> 
> Signed-off-by: Wolfgang Mauerer <wolfgang.mauerer@domain.hid>
> Signed-off-by: Jan Kiszka <jan.kiszka@domain.hid>
> ---
>  include/asm-generic/syscall.h |    1 +
>  include/nucleus/xnshared.h    |   33 +++++++++++++++++++++++++++++++++
>  ksrc/nucleus/module.c         |   23 +++++++++++++++++++++++
>  ksrc/nucleus/shadow.c         |    4 ++++
>  4 files changed, 61 insertions(+), 0 deletions(-)
>  create mode 100644 include/nucleus/xnshared.h
> 
> diff --git a/include/asm-generic/syscall.h b/include/asm-generic/syscall.h
> index 483b99f..7d68580 100644
> --- a/include/asm-generic/syscall.h
> +++ b/include/asm-generic/syscall.h
> @@ -53,6 +53,7 @@
>  typedef struct xnsysinfo {
>  	unsigned long long cpufreq;	/* CPU frequency */
>  	unsigned long tickval;		/* Tick duration (ns) */
> +	unsigned long xnshared_offset;  /* Offset of xnshared in the sem heap */
>  } xnsysinfo_t;
>  
>  #define SIGSHADOW  SIGWINCH
> diff --git a/include/nucleus/xnshared.h b/include/nucleus/xnshared.h
> new file mode 100644
> index 0000000..4293cda
> --- /dev/null
> +++ b/include/nucleus/xnshared.h
> @@ -0,0 +1,33 @@
> +#ifndef XNSHARED_H
> +#define XNSHARED_H
> +
> +/*
> + * Data shared between Xenomai kernel/userland and the Linux kernel/userland
> + * on the global semaphore heap. The features element indicates which data are
> + * shared. Notice that xnshared may only grow, but never shrink.
> + */
> +struct xnshared {
> +	unsigned long long features;
> +
> +	/* Embed domain specific structures that describe the
> +	 * shared data here */
> +};
> +
> +/*
> + * For each shared feature, add a flag below. For now, it is still
> + * empty.
> + */
> +enum xnshared_features {
> +/*	XNSHARED_FEAT_A = 1,
> +	XNSHARED_FEAT_B, */
> +	XNSHARED_MAX_FEATURES,
> +};

Xenomai style is to use defines bit with the bit shifted directly, so,
XNSHARED_FEAT_A would rather be 0x0000000000000001, and I am not sure an
enum can be 64 bits.

> +
> +extern struct xnshared *xnshared;
> +
> +static inline int xnshared_test_feature(enum xnshared_features feature)
> +{
> +	return xnshared && xnshared->features & (1 << feature);
> +}

xnshared can not be null (you panic when allocation fails), and please
use testbits for this function.

> +
> +#endif
> diff --git a/ksrc/nucleus/module.c b/ksrc/nucleus/module.c
> index 5404182..3963fbd 100644
> --- a/ksrc/nucleus/module.c
> +++ b/ksrc/nucleus/module.c
> @@ -34,6 +34,7 @@
>  #include <nucleus/pipe.h>
>  #endif /* CONFIG_XENO_OPT_PIPE */
>  #include <nucleus/select.h>
> +#include <nucleus/xnshared.h>
>  #include <asm/xenomai/bits/init.h>
>  
>  MODULE_DESCRIPTION("Xenomai nucleus");
> @@ -51,6 +52,9 @@ u_long xnmod_sysheap_size;
>  
>  int xeno_nucleus_status = -EINVAL;
>  
> +struct xnshared *xnshared;
> +EXPORT_SYMBOL_GPL(xnshared);
> +
>  struct xnsys_ppd __xnsys_global_ppd;
>  EXPORT_SYMBOL_GPL(__xnsys_global_ppd);
>  
> @@ -82,6 +86,23 @@ void xnmod_alloc_glinks(xnqueue_t *freehq)
>  }
>  EXPORT_SYMBOL_GPL(xnmod_alloc_glinks);
>  
> +/*
> + * We re-use the global semaphore heap to provide a multi-purpose shared
> + * memory area between Xenomai and Linux - for both kernel and userland
> + */
> +void __init xnheap_init_shared(void)
> +{
> +	xnshared = (struct xnshared *)xnheap_alloc(&__xnsys_global_ppd.sem_heap,
> +						  sizeof(struct xnshared));
> +
> +	if (!xnshared)
> +		xnpod_fatal("Xenomai: cannot allocate memory for xnshared!\n");
> +
> +	/* Currently, we don't support any sharing, but later versions will
> +	 * add flags here */
> +	xnshared->features = 0ULL;
> +}
> +
>  int __init __xeno_sys_init(void)
>  {
>  	int ret;
> @@ -106,6 +127,8 @@ int __init __xeno_sys_init(void)
>  		goto cleanup_arch;
>  
>  	xnheap_set_label(&__xnsys_global_ppd.sem_heap, "global sem heap");
> +
> +	xnheap_init_shared();
>  #endif

Ok. There is a real question here. Whether we want to put this code in
module.c or shadow.c. I have no definite answer. Arguments for each file:

module.c: the shared area will be used for NTP by the clock/timer
subsystem, so will be used in kernel-space, even without
CONFIG_XENO_OPT_PERVASIVE.

shadow.c: global shared heap is uncached on ARM, so, I am not sure we
really want the timer/clock system to use the same area as user-space.
So, we are probably better off using different copies of the same data
in kernel-space and user-space (with a price of a copy every time the
data change and CONFIG_XENO_OPT_PERVASIVE is enabled). This would allow
us to move that code in shadow.c

>  	
>  #ifdef __KERNEL__
> diff --git a/ksrc/nucleus/shadow.c b/ksrc/nucleus/shadow.c
> index 0c94a60..0373a8d 100644
> --- a/ksrc/nucleus/shadow.c
> +++ b/ksrc/nucleus/shadow.c
> @@ -50,6 +50,7 @@
>  #include <nucleus/trace.h>
>  #include <nucleus/stat.h>
>  #include <nucleus/sys_ppd.h>
> +#include <nucleus/xnshared.h>
>  #include <asm/xenomai/features.h>
>  #include <asm/xenomai/syscall.h>
>  #include <asm/xenomai/bits/shadow.h>
> @@ -1746,6 +1747,9 @@ static int xnshadow_sys_info(struct pt_regs *regs)
>  
>  	info.cpufreq = xnarch_get_cpu_freq();
>  
> +	info.xnshared_offset =
> +	  xnheap_mapped_offset(&xnsys_ppd_get(1)->sem_heap, xnshared);
> +
>  	return __xn_safe_copy_to_user((void __user *)infarg, &info, sizeof(info));
>  }
>  

And the user-space part? It would be nice to at least define the
structure in user-space and read the "features" member, to check that
the inclusion of the nucleus/xnshared header works and that we read 0
and there is no gag on all architectures

-- 
					    Gilles.

Re: [Xenomai-core] [PATCH] Add support for sharing kernel/userland data between Xenomai and Linux

[Gilles Chanteperdrix]

Gilles Chanteperdrix wrote:
> wolfgang.mauerer@domain.hid wrote:
>> +/*
>> + * We re-use the global semaphore heap to provide a multi-purpose shared
>> + * memory area between Xenomai and Linux - for both kernel and userland
>> + */
>> +void __init xnheap_init_shared(void)
>> +{
>> +	xnshared = (struct xnshared *)xnheap_alloc(&__xnsys_global_ppd.sem_heap,
>> +						  sizeof(struct xnshared));

Also:

Xenomai style: s/sizeof(struct xnshared)/sizeof(*xnshared)/

-- 
					    Gilles.

Re: [Xenomai-core] [PATCH] Add support for sharing kernel/userland data between Xenomai and Linux

[Wolfgang Mauerer]

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

Gilles Chanteperdrix wrote:
> wolfgang.mauerer@domain.hid wrote:
>> From: Wolfgang Mauerer <wolfgang.mauerer@domain.hid>
>>
>> A new structure (struct xnshared) is introduced. It allows us to share
>> generic data between user and kernel of Linux and Xenomai; a bitmap
>> of feature flags located at the beginning of the structure identifies
>> which data are shared. The structure is allocated in the global semaphore
>> heap, and xnsysinfo.xnshared_offset identifies the offset of the
>> structure within the heap.
>>
>> Currently, no shared features are yet supported - the patch only
>> introduces the necessary ABI changes. When the need arises
>> to share data between said entities, the structure must
>> be accordingly extended, and a new feature bit must be added
>> to the flags.
> 
> Hi,
> 
> it is nice that you proposed that patch, I wanted to do it, but did not
> yet. Some comments however.

you're welcome ;-) I have some more patches pending that I have not
sent yet because they need some polishing, so it might make
sense to coordinate things first before you start to work on any of
the issues.

A patch including the changes you requested is attached.
> 
> First, I think xnshared/xnshared_offset is a bit of a long name. I was
> thinking about xnvdso, but is is only slightly shorter. Any other idea,
> anyone?
we can use xnvdso and xnvdso_off to save a few chars, I don't have any
real preferences wrt. naming.
> 
>> Signed-off-by: Wolfgang Mauerer <wolfgang.mauerer@domain.hid>
>> Signed-off-by: Jan Kiszka <jan.kiszka@domain.hid>
>> ---
>>  include/asm-generic/syscall.h |    1 +
>>  include/nucleus/xnshared.h    |   33 +++++++++++++++++++++++++++++++++
>>  ksrc/nucleus/module.c         |   23 +++++++++++++++++++++++
>>  ksrc/nucleus/shadow.c         |    4 ++++
>>  4 files changed, 61 insertions(+), 0 deletions(-)
>>  create mode 100644 include/nucleus/xnshared.h
>>
>> diff --git a/include/asm-generic/syscall.h b/include/asm-generic/syscall.h
>> index 483b99f..7d68580 100644
>> --- a/include/asm-generic/syscall.h
>> +++ b/include/asm-generic/syscall.h
>> @@ -53,6 +53,7 @@
>>  typedef struct xnsysinfo {
>>  	unsigned long long cpufreq;	/* CPU frequency */
>>  	unsigned long tickval;		/* Tick duration (ns) */
>> +	unsigned long xnshared_offset;  /* Offset of xnshared in the sem heap */
>>  } xnsysinfo_t;
>>  
>>  #define SIGSHADOW  SIGWINCH
>> diff --git a/include/nucleus/xnshared.h b/include/nucleus/xnshared.h
>> new file mode 100644
>> index 0000000..4293cda
>> --- /dev/null
>> +++ b/include/nucleus/xnshared.h
>> @@ -0,0 +1,33 @@
>> +#ifndef XNSHARED_H
>> +#define XNSHARED_H
>> +
>> +/*
>> + * Data shared between Xenomai kernel/userland and the Linux kernel/userland
>> + * on the global semaphore heap. The features element indicates which data are
>> + * shared. Notice that xnshared may only grow, but never shrink.
>> + */
>> +struct xnshared {
>> +	unsigned long long features;
>> +
>> +	/* Embed domain specific structures that describe the
>> +	 * shared data here */
>> +};
>> +
>> +/*
>> + * For each shared feature, add a flag below. For now, it is still
>> + * empty.
>> + */
>> +enum xnshared_features {
>> +/*	XNSHARED_FEAT_A = 1,
>> +	XNSHARED_FEAT_B, */
>> +	XNSHARED_MAX_FEATURES,
>> +};
> 
> Xenomai style is to use defines bit with the bit shifted directly, so,
> XNSHARED_FEAT_A would rather be 0x0000000000000001, and I am not sure an
> enum can be 64 bits.

oh yeah, enums are always ints, as a superficial look into the
ANSI standard tells me. Wasn't thinking about that. Using
your direct definition makes sense (although it's a bit
unlikely that there will ever be more than 2^32 shared
features...)
> 
>> +
>> +extern struct xnshared *xnshared;
>> +
>> +static inline int xnshared_test_feature(enum xnshared_features feature)
>> +{
>> +	return xnshared && xnshared->features & (1 << feature);
>> +}
> 
> xnshared can not be null (you panic when allocation fails), and please
> use testbits for this function.
okay
> 
>> +
>> +#endif
>> diff --git a/ksrc/nucleus/module.c b/ksrc/nucleus/module.c
>> index 5404182..3963fbd 100644
>> --- a/ksrc/nucleus/module.c
>> +++ b/ksrc/nucleus/module.c
>> @@ -34,6 +34,7 @@
>>  #include <nucleus/pipe.h>
>>  #endif /* CONFIG_XENO_OPT_PIPE */
>>  #include <nucleus/select.h>
>> +#include <nucleus/xnshared.h>
>>  #include <asm/xenomai/bits/init.h>
>>  
>>  MODULE_DESCRIPTION("Xenomai nucleus");
>> @@ -51,6 +52,9 @@ u_long xnmod_sysheap_size;
>>  
>>  int xeno_nucleus_status = -EINVAL;
>>  
>> +struct xnshared *xnshared;
>> +EXPORT_SYMBOL_GPL(xnshared);
>> +
>>  struct xnsys_ppd __xnsys_global_ppd;
>>  EXPORT_SYMBOL_GPL(__xnsys_global_ppd);
>>  
>> @@ -82,6 +86,23 @@ void xnmod_alloc_glinks(xnqueue_t *freehq)
>>  }
>>  EXPORT_SYMBOL_GPL(xnmod_alloc_glinks);
>>  
>> +/*
>> + * We re-use the global semaphore heap to provide a multi-purpose shared
>> + * memory area between Xenomai and Linux - for both kernel and userland
>> + */
>> +void __init xnheap_init_shared(void)
>> +{
>> +	xnshared = (struct xnshared *)xnheap_alloc(&__xnsys_global_ppd.sem_heap,
>> +						  sizeof(struct xnshared));
>> +
>> +	if (!xnshared)
>> +		xnpod_fatal("Xenomai: cannot allocate memory for xnshared!\n");
>> +
>> +	/* Currently, we don't support any sharing, but later versions will
>> +	 * add flags here */
>> +	xnshared->features = 0ULL;
>> +}
>> +
>>  int __init __xeno_sys_init(void)
>>  {
>>  	int ret;
>> @@ -106,6 +127,8 @@ int __init __xeno_sys_init(void)
>>  		goto cleanup_arch;
>>  
>>  	xnheap_set_label(&__xnsys_global_ppd.sem_heap, "global sem heap");
>> +
>> +	xnheap_init_shared();
>>  #endif
> 
> Ok. There is a real question here. Whether we want to put this code in
> module.c or shadow.c. I have no definite answer. Arguments for each file:
> 
> module.c: the shared area will be used for NTP by the clock/timer
> subsystem, so will be used in kernel-space, even without
> CONFIG_XENO_OPT_PERVASIVE.
> 
> shadow.c: global shared heap is uncached on ARM, so, I am not sure we
> really want the timer/clock system to use the same area as user-space.
> So, we are probably better off using different copies of the same data
> in kernel-space and user-space (with a price of a copy every time the
> data change and CONFIG_XENO_OPT_PERVASIVE is enabled). This would allow
> us to move that code in shadow.c

Is there any compelling advantage for shadow.c? Using a copy operation
every time the data change seems a bit contrary to the reason for
the new mechanism in the first place, namely to have a light-weight
means of sharing data.
> 
>>  	
>>  #ifdef __KERNEL__
>> diff --git a/ksrc/nucleus/shadow.c b/ksrc/nucleus/shadow.c
>> index 0c94a60..0373a8d 100644
>> --- a/ksrc/nucleus/shadow.c
>> +++ b/ksrc/nucleus/shadow.c
>> @@ -50,6 +50,7 @@
>>  #include <nucleus/trace.h>
>>  #include <nucleus/stat.h>
>>  #include <nucleus/sys_ppd.h>
>> +#include <nucleus/xnshared.h>
>>  #include <asm/xenomai/features.h>
>>  #include <asm/xenomai/syscall.h>
>>  #include <asm/xenomai/bits/shadow.h>
>> @@ -1746,6 +1747,9 @@ static int xnshadow_sys_info(struct pt_regs *regs)
>>  
>>  	info.cpufreq = xnarch_get_cpu_freq();
>>  
>> +	info.xnshared_offset =
>> +	  xnheap_mapped_offset(&xnsys_ppd_get(1)->sem_heap, xnshared);
>> +
>>  	return __xn_safe_copy_to_user((void __user *)infarg, &info, sizeof(info));
>>  }
>>  
> 
> And the user-space part? It would be nice to at least define the
> structure in user-space and read the "features" member, to check that
> the inclusion of the nucleus/xnshared header works and that we read 0
> and there is no gag on all architectures

I haven't included the userland part on purpose because I wanted
to make sure that the ABI change is merged for 2.5 - all the rest
can be added later without problems. It will follow once I've
polished the code (if having the userland part should be
a prerequisite for merging the ABI change, I can try to speed
things up a bit; otherwise, I'd submit the userland read side
together with the NTP base mechanics).

> Also:
>
> Xenomai style: s/sizeof(struct xnshared)/sizeof(*xnshared)/

okay, changed.

Cheers, Wolfgang

[-- Attachment #2: xnvdso.diff --]
[-- Type: text/x-patch, Size: 4588 bytes --]

commit c291a380d610060240c76fc3cadfce6450abfdc8
Author: Wolfgang Mauerer <wolfgang.mauerer@domain.hid>
Date:   Fri Dec 11 10:59:00 2009 +0100

    Add support for sharing kernel/userland data between Xenomai and Linux
    
    A new structure (struct xnshared) is introduced. It allows us to share
    generic data between user and kernel of Linux and Xenomai; a bitmap
    of feature flags located at the beginning of the structure identifies
    which data are shared. The structure is allocated in the global semaphore
    heap, and xnsysinfo.xnshared_offset identifies the offset of the
    structure within the heap.
    
    Currently, no shared features are yet supported - the patch only
    introduces the necessary ABI changes. When the need arises
    to share data between said entities, the structure must
    be accordingly extended, and a new feature bit must be added
    to the flags.
    
    Signed-off-by: Wolfgang Mauerer <wolfgang.mauerer@domain.hid>
    Signed-off-by: Jan Kiszka <jan.kiszka@domain.hid>

diff --git a/include/asm-generic/syscall.h b/include/asm-generic/syscall.h
index 483b99f..8f1ddc6 100644
--- a/include/asm-generic/syscall.h
+++ b/include/asm-generic/syscall.h
@@ -53,6 +53,7 @@
 typedef struct xnsysinfo {
 	unsigned long long cpufreq;	/* CPU frequency */
 	unsigned long tickval;		/* Tick duration (ns) */
+	unsigned long xnvdso_off;  	/* Offset of xnvdso in the sem heap */
 } xnsysinfo_t;
 
 #define SIGSHADOW  SIGWINCH
diff --git a/include/nucleus/xnvdso.h b/include/nucleus/xnvdso.h
new file mode 100644
index 0000000..cc2ec20
--- /dev/null
+++ b/include/nucleus/xnvdso.h
@@ -0,0 +1,36 @@
+#ifndef XNVDSO_H
+#define XNVDSO_H
+
+/*
+ * Data shared between Xenomai kernel/userland and the Linux kernel/userland
+ * on the global semaphore heap. The features element indicates which data are
+ * shared. Notice that struct xnvdso may only grow, but never shrink.
+ */
+struct xnvdso {
+	unsigned long long features;
+
+	/* Embed domain specific structures that describe the
+	 * shared data here */
+};
+
+/*
+ * For each shared feature, add a flag below. For now, the set is still
+ * empty.
+ */
+/*
+#define XNVDSO_FEAT_A	0x0000000000000001
+#define XNVDSO_FEAT_B	0x0000000000000002
+#define XNVDSO_FEAT_C	0x0000000000000004
+#define XNVDSO_FEATURES	(XNVDSO_FEAT_A | XNVDSO_FEAT_B | XVDSO_FEAT_C)
+*/
+
+#define XNVDSO_FEATURES 0x0000000000000000
+
+extern struct xnvdso *xnvdso;
+
+static inline int xnvdso_test_feature(unsigned long long feature)
+{
+	return testbits(xnvdso->features, feature);
+}
+
+#endif
diff --git a/ksrc/nucleus/module.c b/ksrc/nucleus/module.c
index 5404182..40ee9a6 100644
--- a/ksrc/nucleus/module.c
+++ b/ksrc/nucleus/module.c
@@ -34,6 +34,7 @@
 #include <nucleus/pipe.h>
 #endif /* CONFIG_XENO_OPT_PIPE */
 #include <nucleus/select.h>
+#include <nucleus/xnvdso.h>
 #include <asm/xenomai/bits/init.h>
 
 MODULE_DESCRIPTION("Xenomai nucleus");
@@ -51,6 +52,9 @@ u_long xnmod_sysheap_size;
 
 int xeno_nucleus_status = -EINVAL;
 
+struct xnvdso *xnvdso;
+EXPORT_SYMBOL_GPL(xnvdso);
+
 struct xnsys_ppd __xnsys_global_ppd;
 EXPORT_SYMBOL_GPL(__xnsys_global_ppd);
 
@@ -82,6 +86,21 @@ void xnmod_alloc_glinks(xnqueue_t *freehq)
 }
 EXPORT_SYMBOL_GPL(xnmod_alloc_glinks);
 
+/*
+ * We re-use the global semaphore heap to provide a multi-purpose shared
+ * memory area between Xenomai and Linux - for both kernel and userland
+ */
+void __init xnheap_init_shared(void)
+{
+	xnvdso = (struct xnvdso *)xnheap_alloc(&__xnsys_global_ppd.sem_heap,
+						  sizeof(*xnvdso));
+
+	if (!xnvdso)
+		xnpod_fatal("Xenomai: cannot allocate memory for xnvdso!\n");
+
+	xnvdso->features = XNVDSO_FEATURES;
+}
+
 int __init __xeno_sys_init(void)
 {
 	int ret;
@@ -106,6 +125,8 @@ int __init __xeno_sys_init(void)
 		goto cleanup_arch;
 
 	xnheap_set_label(&__xnsys_global_ppd.sem_heap, "global sem heap");
+
+	xnheap_init_shared();
 #endif
 	
 #ifdef __KERNEL__
diff --git a/ksrc/nucleus/shadow.c b/ksrc/nucleus/shadow.c
index 0c94a60..d8bced3 100644
--- a/ksrc/nucleus/shadow.c
+++ b/ksrc/nucleus/shadow.c
@@ -50,6 +50,7 @@
 #include <nucleus/trace.h>
 #include <nucleus/stat.h>
 #include <nucleus/sys_ppd.h>
+#include <nucleus/xnvdso.h>
 #include <asm/xenomai/features.h>
 #include <asm/xenomai/syscall.h>
 #include <asm/xenomai/bits/shadow.h>
@@ -1746,6 +1747,9 @@ static int xnshadow_sys_info(struct pt_regs *regs)
 
 	info.cpufreq = xnarch_get_cpu_freq();
 
+	info.xnvdso_off =
+	  xnheap_mapped_offset(&xnsys_ppd_get(1)->sem_heap, xnvdso);
+
 	return __xn_safe_copy_to_user((void __user *)infarg, &info, sizeof(info));
 }
 

Re: [Xenomai-core] [PATCH] Add support for sharing kernel/userland data between Xenomai and Linux

[Gilles Chanteperdrix]

Wolfgang Mauerer wrote:
>>> +enum xnshared_features {
>>> +/*	XNSHARED_FEAT_A = 1,
>>> +	XNSHARED_FEAT_B, */
>>> +	XNSHARED_MAX_FEATURES,
>>> +};
>> Xenomai style is to use defines bit with the bit shifted directly, so,
>> XNSHARED_FEAT_A would rather be 0x0000000000000001, and I am not sure an
>> enum can be 64 bits.
> 
> oh yeah, enums are always ints, as a superficial look into the
> ANSI standard tells me. Wasn't thinking about that. Using
> your direct definition makes sense (although it's a bit
> unlikely that there will ever be more than 2^32 shared
> features...)

Err, since features are bits, the limit is 32 features, not 2^32, and if
we make the features member an unsigned long long in the first place, it
is because we expect to be able to use the 64 bits, no?

>> Ok. There is a real question here. Whether we want to put this code in
>> module.c or shadow.c. I have no definite answer. Arguments for each file:
>>
>> module.c: the shared area will be used for NTP by the clock/timer
>> subsystem, so will be used in kernel-space, even without
>> CONFIG_XENO_OPT_PERVASIVE.
>>
>> shadow.c: global shared heap is uncached on ARM, so, I am not sure we
>> really want the timer/clock system to use the same area as user-space.
>> So, we are probably better off using different copies of the same data
>> in kernel-space and user-space (with a price of a copy every time the
>> data change and CONFIG_XENO_OPT_PERVASIVE is enabled). This would allow
>> us to move that code in shadow.c
> 
> Is there any compelling advantage for shadow.c? Using a copy operation
> every time the data change seems a bit contrary to the reason for
> the new mechanism in the first place, namely to have a light-weight
> means of sharing data.

The way I see it, the NTP data will be used by the nucleus clock and
timer subsystem, so several times for each timer tick, so having them on
an uncached memory will have a performance hit.

The copy, on the other hand, would take place over a non real-time
context, only once every update of NTP data, i.e. once every 10ms.

So, let us move it to shadow.c.

>>>  	
>>>  #ifdef __KERNEL__
>>> diff --git a/ksrc/nucleus/shadow.c b/ksrc/nucleus/shadow.c
>>> index 0c94a60..0373a8d 100644
>>> --- a/ksrc/nucleus/shadow.c
>>> +++ b/ksrc/nucleus/shadow.c
>>> @@ -50,6 +50,7 @@
>>>  #include <nucleus/trace.h>
>>>  #include <nucleus/stat.h>
>>>  #include <nucleus/sys_ppd.h>
>>> +#include <nucleus/xnshared.h>
>>>  #include <asm/xenomai/features.h>
>>>  #include <asm/xenomai/syscall.h>
>>>  #include <asm/xenomai/bits/shadow.h>
>>> @@ -1746,6 +1747,9 @@ static int xnshadow_sys_info(struct pt_regs *regs)
>>>  
>>>  	info.cpufreq = xnarch_get_cpu_freq();
>>>  
>>> +	info.xnshared_offset =
>>> +	  xnheap_mapped_offset(&xnsys_ppd_get(1)->sem_heap, xnshared);
>>> +
>>>  	return __xn_safe_copy_to_user((void __user *)infarg, &info, sizeof(info));
>>>  }
>>>  
>> And the user-space part? It would be nice to at least define the
>> structure in user-space and read the "features" member, to check that
>> the inclusion of the nucleus/xnshared header works and that we read 0
>> and there is no gag on all architectures

We can not expect to read 0, since we want this user-space to continue
working with later versions of the ABI. Actually, we can not expect
features to be of an specific form (I was thinking (1 << max_bit) - 1,
but it would mean that we can not remove feature bits).

> 
> I haven't included the userland part on purpose because I wanted
> to make sure that the ABI change is merged for 2.5 - all the rest
> can be added later without problems. It will follow once I've
> polished the code (if having the userland part should be
> a prerequisite for merging the ABI change, I can try to speed
> things up a bit; otherwise, I'd submit the userland read side
> together with the NTP base mechanics).

It would be embarassing (though unlikely) to think having changed the
ABI, and finally find out, when we start to use it, that the ABI change
does not work. So, please add the user-space code. So, that our run-time
tests on various architectures can be used to check that the
modification actually works.

The really really nice thing to do, would be a unit test which checks
for a specific value of the ABI features. So, we can run
"check_xeno_abi_features 0", to check that the thing actually works with
release 2.5.0.

-- 
					    Gilles.

Re: [Xenomai-core] [PATCH] Add support for sharing kernel/userland data between Xenomai and Linux

[Wolfgang Mauerer]

Gilles Chanteperdrix wrote:
> Wolfgang Mauerer wrote:
>>>> +enum xnshared_features {
>>>> +/*	XNSHARED_FEAT_A = 1,
>>>> +	XNSHARED_FEAT_B, */
>>>> +	XNSHARED_MAX_FEATURES,
>>>> +};
>>> Xenomai style is to use defines bit with the bit shifted directly, so,
>>> XNSHARED_FEAT_A would rather be 0x0000000000000001, and I am not sure an
>>> enum can be 64 bits.
>> oh yeah, enums are always ints, as a superficial look into the
>> ANSI standard tells me. Wasn't thinking about that. Using
>> your direct definition makes sense (although it's a bit
>> unlikely that there will ever be more than 2^32 shared
>> features...)
> 
> Err, since features are bits, the limit is 32 features, not 2^32, and if
> we make the features member an unsigned long long in the first place, it
> is because we expect to be able to use the 64 bits, no?

Argl. Yes. /me realises that christmas holidays are severely required...
> 
>>> Ok. There is a real question here. Whether we want to put this code in
>>> module.c or shadow.c. I have no definite answer. Arguments for each file:
>>>
>>> module.c: the shared area will be used for NTP by the clock/timer
>>> subsystem, so will be used in kernel-space, even without
>>> CONFIG_XENO_OPT_PERVASIVE.
>>>
>>> shadow.c: global shared heap is uncached on ARM, so, I am not sure we
>>> really want the timer/clock system to use the same area as user-space.
>>> So, we are probably better off using different copies of the same data
>>> in kernel-space and user-space (with a price of a copy every time the
>>> data change and CONFIG_XENO_OPT_PERVASIVE is enabled). This would allow
>>> us to move that code in shadow.c
>> Is there any compelling advantage for shadow.c? Using a copy operation
>> every time the data change seems a bit contrary to the reason for
>> the new mechanism in the first place, namely to have a light-weight
>> means of sharing data.
> 
> The way I see it, the NTP data will be used by the nucleus clock and
> timer subsystem, so several times for each timer tick, so having them on
> an uncached memory will have a performance hit.
> 
> The copy, on the other hand, would take place over a non real-time
> context, only once every update of NTP data, i.e. once every 10ms.

> So, let us move it to shadow.c.

I see your point, but it has the disadvantage that the time spent
in the Linux kernel for the copy operations in the vsyscall is
increased -- which might affect the Linux domain, since this is
an optimised hot path. Naturally, in the absence of any quantitative
measurements, this is all just speculation, but if access to the global
semaphore heap is unchached only on ARM, we provide an unnecessary
optimisation for n-1 out of n architectures supported by Xenomai.

Having the time-keeping code in the Xenomai kernel benefit from NTP
corrections provided by the Linux side is a bit separate from the
ABI change, which would be the same irregardless of whether we
use a second copy for kernel-side NTP operations or not. Would it
therefore make sense to restrict the data exchange to the global heap
semaphore, and add an architecture-specific hook for ARM
later on to generate an extra kernel space copy of the NTP data? The
Linux vsyscall time keeping information is architecture-specific, so
arch specific hooks make sense in this area anyway. We could therefore
restrict the second copy to ARM without much effort. Or am I missing
something in your considerations?

>>>>  	
>>>>  #ifdef __KERNEL__
>>>> diff --git a/ksrc/nucleus/shadow.c b/ksrc/nucleus/shadow.c
>>>> index 0c94a60..0373a8d 100644
>>>> --- a/ksrc/nucleus/shadow.c
>>>> +++ b/ksrc/nucleus/shadow.c
>>>> @@ -50,6 +50,7 @@
>>>>  #include <nucleus/trace.h>
>>>>  #include <nucleus/stat.h>
>>>>  #include <nucleus/sys_ppd.h>
>>>> +#include <nucleus/xnshared.h>
>>>>  #include <asm/xenomai/features.h>
>>>>  #include <asm/xenomai/syscall.h>
>>>>  #include <asm/xenomai/bits/shadow.h>
>>>> @@ -1746,6 +1747,9 @@ static int xnshadow_sys_info(struct pt_regs *regs)
>>>>  
>>>>  	info.cpufreq = xnarch_get_cpu_freq();
>>>>  
>>>> +	info.xnshared_offset =
>>>> +	  xnheap_mapped_offset(&xnsys_ppd_get(1)->sem_heap, xnshared);
>>>> +
>>>>  	return __xn_safe_copy_to_user((void __user *)infarg, &info, sizeof(info));
>>>>  }
>>>>  
>>> And the user-space part? It would be nice to at least define the
>>> structure in user-space and read the "features" member, to check that
>>> the inclusion of the nucleus/xnshared header works and that we read 0
>>> and there is no gag on all architectures
> 
> We can not expect to read 0, since we want this user-space to continue
> working with later versions of the ABI. Actually, we can not expect
> features to be of an specific form (I was thinking (1 << max_bit) - 1,
> but it would mean that we can not remove feature bits).
> 
>> I haven't included the userland part on purpose because I wanted
>> to make sure that the ABI change is merged for 2.5 - all the rest
>> can be added later without problems. It will follow once I've
>> polished the code (if having the userland part should be
>> a prerequisite for merging the ABI change, I can try to speed
>> things up a bit; otherwise, I'd submit the userland read side
>> together with the NTP base mechanics).
> 
> It would be embarassing (though unlikely) to think having changed the
> ABI, and finally find out, when we start to use it, that the ABI change
> does not work. So, please add the user-space code. So, that our run-time
> tests on various architectures can be used to check that the
> modification actually works.
> 
> The really really nice thing to do, would be a unit test which checks
> for a specific value of the ABI features. So, we can run
> "check_xeno_abi_features 0", to check that the thing actually works with
> release 2.5.0.

okay, so I'll make sure to have the userland bits ready in time
before the 2.5.0 release.

Cheers, Wolfgang

Re: [Xenomai-core] [PATCH] Add support for sharing kernel/userland data between Xenomai and Linux

[Gilles Chanteperdrix]

Wolfgang Mauerer wrote:
> Gilles Chanteperdrix wrote:
>> Wolfgang Mauerer wrote:
>>>>> +enum xnshared_features {
>>>>> +/*	XNSHARED_FEAT_A = 1,
>>>>> +	XNSHARED_FEAT_B, */
>>>>> +	XNSHARED_MAX_FEATURES,
>>>>> +};
>>>> Xenomai style is to use defines bit with the bit shifted directly, so,
>>>> XNSHARED_FEAT_A would rather be 0x0000000000000001, and I am not sure an
>>>> enum can be 64 bits.
>>> oh yeah, enums are always ints, as a superficial look into the
>>> ANSI standard tells me. Wasn't thinking about that. Using
>>> your direct definition makes sense (although it's a bit
>>> unlikely that there will ever be more than 2^32 shared
>>> features...)
>> Err, since features are bits, the limit is 32 features, not 2^32, and if
>> we make the features member an unsigned long long in the first place, it
>> is because we expect to be able to use the 64 bits, no?
> 
> Argl. Yes. /me realises that christmas holidays are severely required...
>>>> Ok. There is a real question here. Whether we want to put this code in
>>>> module.c or shadow.c. I have no definite answer. Arguments for each file:
>>>>
>>>> module.c: the shared area will be used for NTP by the clock/timer
>>>> subsystem, so will be used in kernel-space, even without
>>>> CONFIG_XENO_OPT_PERVASIVE.
>>>>
>>>> shadow.c: global shared heap is uncached on ARM, so, I am not sure we
>>>> really want the timer/clock system to use the same area as user-space.
>>>> So, we are probably better off using different copies of the same data
>>>> in kernel-space and user-space (with a price of a copy every time the
>>>> data change and CONFIG_XENO_OPT_PERVASIVE is enabled). This would allow
>>>> us to move that code in shadow.c
>>> Is there any compelling advantage for shadow.c? Using a copy operation
>>> every time the data change seems a bit contrary to the reason for
>>> the new mechanism in the first place, namely to have a light-weight
>>> means of sharing data.
>> The way I see it, the NTP data will be used by the nucleus clock and
>> timer subsystem, so several times for each timer tick, so having them on
>> an uncached memory will have a performance hit.
>>
>> The copy, on the other hand, would take place over a non real-time
>> context, only once every update of NTP data, i.e. once every 10ms.
> 
>> So, let us move it to shadow.c.
> 
> I see your point, but it has the disadvantage that the time spent
> in the Linux kernel for the copy operations in the vsyscall is
> increased -- which might affect the Linux domain, since this is
> an optimised hot path. Naturally, in the absence of any quantitative
> measurements, this is all just speculation, but if access to the global
> semaphore heap is unchached only on ARM, we provide an unnecessary
> optimisation for n-1 out of n architectures supported by Xenomai.
> 
> Having the time-keeping code in the Xenomai kernel benefit from NTP
> corrections provided by the Linux side is a bit separate from the
> ABI change, which would be the same irregardless of whether we
> use a second copy for kernel-side NTP operations or not.

Exactly, the issue are distinct, the ABI change is a pure user-space
issue, so, should only be compiled if CONFIG_XENO_OPT_PERVASIVE is defined.

 Would it
> therefore make sense to restrict the data exchange to the global heap
> semaphore, and add an architecture-specific hook for ARM
> later on to generate an extra kernel space copy of the NTP data? The
> Linux vsyscall time keeping information is architecture-specific, so

No. We will make it generic. Nothing arch specific is needed. We will
not copy the vsyscall time keeping information, we will copy the data
passed to update_vsyscall, which are the same on all architectures.

> arch specific hooks make sense in this area anyway. We could therefore
> restrict the second copy to ARM without much effort. Or am I missing
> something in your considerations?

This xnvdso feature is needed for user-space only. Let us move it to
shadow.c.

>> The really really nice thing to do, would be a unit test which checks
>> for a specific value of the ABI features. So, we can run
>> "check_xeno_abi_features 0", to check that the thing actually works with
>> release 2.5.0.
> 
> okay, so I'll make sure to have the userland bits ready in time
> before the 2.5.0 release.

You can implement the user-space part only in a unit test, if it is
easier. All that I ask, is to be able to test that it works.

> 
> Cheers, Wolfgang


-- 
					    Gilles.

Re: [Xenomai-core] [PATCH] Add support for sharing kernel/userland data between Xenomai and Linux

[Wolfgang Mauerer]

Gilles Chanteperdrix wrote:
> Wolfgang Mauerer wrote:
>> Gilles Chanteperdrix wrote:
>>> Wolfgang Mauerer wrote:
>>>>>> +enum xnshared_features {
>>>>>> +/*	XNSHARED_FEAT_A = 1,
>>>>>> +	XNSHARED_FEAT_B, */
>>>>>> +	XNSHARED_MAX_FEATURES,
>>>>>> +};
>>>>> Xenomai style is to use defines bit with the bit shifted directly, so,
>>>>> XNSHARED_FEAT_A would rather be 0x0000000000000001, and I am not sure an
>>>>> enum can be 64 bits.
>>>> oh yeah, enums are always ints, as a superficial look into the
>>>> ANSI standard tells me. Wasn't thinking about that. Using
>>>> your direct definition makes sense (although it's a bit
>>>> unlikely that there will ever be more than 2^32 shared
>>>> features...)
>>> Err, since features are bits, the limit is 32 features, not 2^32, and if
>>> we make the features member an unsigned long long in the first place, it
>>> is because we expect to be able to use the 64 bits, no?
>> Argl. Yes. /me realises that christmas holidays are severely required...
>>>>> Ok. There is a real question here. Whether we want to put this code in
>>>>> module.c or shadow.c. I have no definite answer. Arguments for each file:
>>>>>
>>>>> module.c: the shared area will be used for NTP by the clock/timer
>>>>> subsystem, so will be used in kernel-space, even without
>>>>> CONFIG_XENO_OPT_PERVASIVE.
>>>>>
>>>>> shadow.c: global shared heap is uncached on ARM, so, I am not sure we
>>>>> really want the timer/clock system to use the same area as user-space.
>>>>> So, we are probably better off using different copies of the same data
>>>>> in kernel-space and user-space (with a price of a copy every time the
>>>>> data change and CONFIG_XENO_OPT_PERVASIVE is enabled). This would allow
>>>>> us to move that code in shadow.c
>>>> Is there any compelling advantage for shadow.c? Using a copy operation
>>>> every time the data change seems a bit contrary to the reason for
>>>> the new mechanism in the first place, namely to have a light-weight
>>>> means of sharing data.
>>> The way I see it, the NTP data will be used by the nucleus clock and
>>> timer subsystem, so several times for each timer tick, so having them on
>>> an uncached memory will have a performance hit.
>>>
>>> The copy, on the other hand, would take place over a non real-time
>>> context, only once every update of NTP data, i.e. once every 10ms.
>>> So, let us move it to shadow.c.
>> I see your point, but it has the disadvantage that the time spent
>> in the Linux kernel for the copy operations in the vsyscall is
>> increased -- which might affect the Linux domain, since this is
>> an optimised hot path. Naturally, in the absence of any quantitative
>> measurements, this is all just speculation, but if access to the global
>> semaphore heap is unchached only on ARM, we provide an unnecessary
>> optimisation for n-1 out of n architectures supported by Xenomai.
>>
>> Having the time-keeping code in the Xenomai kernel benefit from NTP
>> corrections provided by the Linux side is a bit separate from the
>> ABI change, which would be the same irregardless of whether we
>> use a second copy for kernel-side NTP operations or not.
> 
> Exactly, the issue are distinct, the ABI change is a pure user-space
> issue, so, should only be compiled if CONFIG_XENO_OPT_PERVASIVE is defined.
okay, that makes sense.

> 
>  Would it
>> therefore make sense to restrict the data exchange to the global heap
>> semaphore, and add an architecture-specific hook for ARM
>> later on to generate an extra kernel space copy of the NTP data? The
>> Linux vsyscall time keeping information is architecture-specific, so
> 
> No. We will make it generic. Nothing arch specific is needed. We will
> not copy the vsyscall time keeping information, we will copy the data
> passed to update_vsyscall, which are the same on all architectures.

Having the possibility to use gettimeofday() without switching to
kernel mode is all about speed, and I don't see why re-using the
optimised data structures offered by the Linux kernel, together with the
already existing optimised read routines for this purpose
should not be an option for the Xenomai userland. But again, the
difference between the alternatives is hard to judge without
quantitative measurements. I'll maybe come back to this.

> 
>> arch specific hooks make sense in this area anyway. We could therefore
>> restrict the second copy to ARM without much effort. Or am I missing
>> something in your considerations?
> 
> This xnvdso feature is needed for user-space only. Let us move it to
> shadow.c.
okay.

Regards, Wolfgang

Re: [Xenomai-core] [PATCH] Add support for sharing kernel/userland data between Xenomai and Linux

[Gilles Chanteperdrix]

Wolfgang Mauerer wrote:
> Gilles Chanteperdrix wrote:
>> Wolfgang Mauerer wrote:
>>> Gilles Chanteperdrix wrote:
>>>> Wolfgang Mauerer wrote:
>>>>>>> +enum xnshared_features {
>>>>>>> +/*	XNSHARED_FEAT_A = 1,
>>>>>>> +	XNSHARED_FEAT_B, */
>>>>>>> +	XNSHARED_MAX_FEATURES,
>>>>>>> +};
>>>>>> Xenomai style is to use defines bit with the bit shifted directly, so,
>>>>>> XNSHARED_FEAT_A would rather be 0x0000000000000001, and I am not sure an
>>>>>> enum can be 64 bits.
>>>>> oh yeah, enums are always ints, as a superficial look into the
>>>>> ANSI standard tells me. Wasn't thinking about that. Using
>>>>> your direct definition makes sense (although it's a bit
>>>>> unlikely that there will ever be more than 2^32 shared
>>>>> features...)
>>>> Err, since features are bits, the limit is 32 features, not 2^32, and if
>>>> we make the features member an unsigned long long in the first place, it
>>>> is because we expect to be able to use the 64 bits, no?
>>> Argl. Yes. /me realises that christmas holidays are severely required...
>>>>>> Ok. There is a real question here. Whether we want to put this code in
>>>>>> module.c or shadow.c. I have no definite answer. Arguments for each file:
>>>>>>
>>>>>> module.c: the shared area will be used for NTP by the clock/timer
>>>>>> subsystem, so will be used in kernel-space, even without
>>>>>> CONFIG_XENO_OPT_PERVASIVE.
>>>>>>
>>>>>> shadow.c: global shared heap is uncached on ARM, so, I am not sure we
>>>>>> really want the timer/clock system to use the same area as user-space.
>>>>>> So, we are probably better off using different copies of the same data
>>>>>> in kernel-space and user-space (with a price of a copy every time the
>>>>>> data change and CONFIG_XENO_OPT_PERVASIVE is enabled). This would allow
>>>>>> us to move that code in shadow.c
>>>>> Is there any compelling advantage for shadow.c? Using a copy operation
>>>>> every time the data change seems a bit contrary to the reason for
>>>>> the new mechanism in the first place, namely to have a light-weight
>>>>> means of sharing data.
>>>> The way I see it, the NTP data will be used by the nucleus clock and
>>>> timer subsystem, so several times for each timer tick, so having them on
>>>> an uncached memory will have a performance hit.
>>>>
>>>> The copy, on the other hand, would take place over a non real-time
>>>> context, only once every update of NTP data, i.e. once every 10ms.
>>>> So, let us move it to shadow.c.
>>> I see your point, but it has the disadvantage that the time spent
>>> in the Linux kernel for the copy operations in the vsyscall is
>>> increased -- which might affect the Linux domain, since this is
>>> an optimised hot path. Naturally, in the absence of any quantitative
>>> measurements, this is all just speculation, but if access to the global
>>> semaphore heap is unchached only on ARM, we provide an unnecessary
>>> optimisation for n-1 out of n architectures supported by Xenomai.
>>>
>>> Having the time-keeping code in the Xenomai kernel benefit from NTP
>>> corrections provided by the Linux side is a bit separate from the
>>> ABI change, which would be the same irregardless of whether we
>>> use a second copy for kernel-side NTP operations or not.
>> Exactly, the issue are distinct, the ABI change is a pure user-space
>> issue, so, should only be compiled if CONFIG_XENO_OPT_PERVASIVE is defined.
> okay, that makes sense.
> 
>>  Would it
>>> therefore make sense to restrict the data exchange to the global heap
>>> semaphore, and add an architecture-specific hook for ARM
>>> later on to generate an extra kernel space copy of the NTP data? The
>>> Linux vsyscall time keeping information is architecture-specific, so
>> No. We will make it generic. Nothing arch specific is needed. We will
>> not copy the vsyscall time keeping information, we will copy the data
>> passed to update_vsyscall, which are the same on all architectures.
> 
> Having the possibility to use gettimeofday() without switching to
> kernel mode is all about speed, and I don't see why re-using the
> optimised data structures offered by the Linux kernel, together with the
> already existing optimised read routines for this purpose
> should not be an option for the Xenomai userland. But again, the
> difference between the alternatives is hard to judge without
> quantitative measurements. I'll maybe come back to this.

I thought we had been through this already and that we agreed. So, let
us make things clear, for once, and I will not be able to exchange much
further mails this afternoon.

We are not talking about speed. We already have speed:
clock_gettime(CLOCK_MONOTONIC) just reads the tsc, and converts the
result to a struct timespec, without a syscall, without even a division.
This is about as fast as it can be, and enough for 99% of real-time
applications. What we are talking about, is synchronizing
CLOCK_MONOTONIC and CLOCK_REALTIME with NTP corrections, without
sacrificing too much performance. Because, yes,  we are going to
sacrifice performance, the computations using NTP data will be slower
than our current multiplication only conversion. This, for a very
specific kind of application.

Of the 5 architectures on which Xenomai is currently ported, only two
implement the update_vsyscall() function. That is a minority. And we are
not interested in yet-another x86-only hack. What we want, because we
want the same level of support for all architectures, that will be
easier to maintain, is a solution as much generic as possible. Minimal
#ifdefs, minimal architecture code. After all, this feature is only
needed for a few specific applications, so, there is no reason for it to
become a maintenance burden.

Let us compare what we are talking about. My idea, is to get the I-pipe
patches emit an event when update_vsyscall is called, and in the xenomai
handler for this event, do the computations of the constants which will
be used by the clock and timer operations until next update. This
computation will happen with a xeno-seqlock locked irqs off.

Actually, I thought there were some computations when starting this
mail, but the computations we are talking about are in fact the copy of
a handful of 32 bits and 64 bits variables. What you are talking about
is, in the xenomai handler for the NTP I-pipe event, call an arch
dependent hook which will copy the data from the x86 specific
vsyscall_gtod_data structure. Unless I misunderstood you, there is no
difference in terms of performance between the two implementations. And
even if there was a difference, we are talking about code which is run
as part of Linux timer handler, that is, when Xenomai tasks have
relinquished the system. This only adds to the overhead of something
that is already of some importance.

To make things even more clear, the code in question on x86_64:

void update_vsyscall(struct timespec *wall_time, struct clocksource *clock)
{
        unsigned long flags;

        write_seqlock_irqsave(&vsyscall_gtod_data.lock, flags);
        /* copy vsyscall data */
        vsyscall_gtod_data.clock.vread = clock->vread;
        vsyscall_gtod_data.clock.cycle_last = clock->cycle_last;
        vsyscall_gtod_data.clock.mask = clock->mask;
        vsyscall_gtod_data.clock.mult = clock->mult;
        vsyscall_gtod_data.clock.shift = clock->shift;
        vsyscall_gtod_data.wall_time_sec = wall_time->tv_sec;
        vsyscall_gtod_data.wall_time_nsec = wall_time->tv_nsec;
        vsyscall_gtod_data.wall_to_monotonic = wall_to_monotonic;
        write_sequnlock_irqrestore(&vsyscall_gtod_data.lock, flags);
}

Why bother copying arch-specific vsyscall_gtod_data.clock.shift whereas
you can get generic clock->shift ?


-- 
					    Gilles.

Re: [Xenomai-core] [PATCH] Add support for sharing kernel/userland data between Xenomai and Linux

[Wolfgang Mauerer]

Gilles Chanteperdrix wrote:
> Wolfgang Mauerer wrote:
>> Gilles Chanteperdrix wrote:
>>> Wolfgang Mauerer wrote:
>>>> Gilles Chanteperdrix wrote:
>>>>> Wolfgang Mauerer wrote:

(...)

>>>  Would it
>>>> therefore make sense to restrict the data exchange to the global heap
>>>> semaphore, and add an architecture-specific hook for ARM
>>>> later on to generate an extra kernel space copy of the NTP data? The
>>>> Linux vsyscall time keeping information is architecture-specific, so
>>> No. We will make it generic. Nothing arch specific is needed. We will
>>> not copy the vsyscall time keeping information, we will copy the data
>>> passed to update_vsyscall, which are the same on all architectures.
>> Having the possibility to use gettimeofday() without switching to
>> kernel mode is all about speed, and I don't see why re-using the
>> optimised data structures offered by the Linux kernel, together with the
>> already existing optimised read routines for this purpose
>> should not be an option for the Xenomai userland. But again, the
>> difference between the alternatives is hard to judge without
>> quantitative measurements. I'll maybe come back to this.
> 
> I thought we had been through this already and that we agreed. So, let
> us make things clear, for once, and I will not be able to exchange much
> further mails this afternoon.

well, the agreement was on  "3- implement the nucleus callback
for the I-pipe event which copies relevant data", and
obviously we were taking different things to be the
relevant data ;-)
> 
> We are not talking about speed. We already have speed:
> clock_gettime(CLOCK_MONOTONIC) just reads the tsc, and converts the
> result to a struct timespec, without a syscall, without even a division.
> This is about as fast as it can be, and enough for 99% of real-time
> applications. What we are talking about, is synchronizing
> CLOCK_MONOTONIC and CLOCK_REALTIME with NTP corrections, without
> sacrificing too much performance. Because, yes,  we are going to
> sacrifice performance, the computations using NTP data will be slower
> than our current multiplication only conversion. This, for a very
> specific kind of application.
> 
> Of the 5 architectures on which Xenomai is currently ported, only two
> implement the update_vsyscall() function. That is a minority. And we are
> not interested in yet-another x86-only hack. What we want, because we
> want the same level of support for all architectures, that will be
> easier to maintain, is a solution as much generic as possible. Minimal
> #ifdefs, minimal architecture code. After all, this feature is only
> needed for a few specific applications, so, there is no reason for it to
> become a maintenance burden.
> 
> Let us compare what we are talking about. My idea, is to get the I-pipe
> patches emit an event when update_vsyscall is called, and in the xenomai
> handler for this event, do the computations of the constants which will
> be used by the clock and timer operations until next update. This
> computation will happen with a xeno-seqlock locked irqs off.
> 
> Actually, I thought there were some computations when starting this
> mail, but the computations we are talking about are in fact the copy of
> a handful of 32 bits and 64 bits variables. What you are talking about
> is, in the xenomai handler for the NTP I-pipe event, call an arch
> dependent hook which will copy the data from the x86 specific
> vsyscall_gtod_data structure. Unless I misunderstood you, there is no
> difference in terms of performance between the two implementations. And
> even if there was a difference, we are talking about code which is run
> as part of Linux timer handler, that is, when Xenomai tasks have
> relinquished the system. This only adds to the overhead of something
> that is already of some importance.

there are indeed no calculations for AMD64, just for PPC. Since
some architectures also require arch-specific hooks for
gettimeofday() (albeit non of those with vsyscall and supported
by Xenomai), I was trying to design a solution that could also
cope with future archs that would require such a hook. But when
easier maintainability is more important than optimal performance,
I agree that the generic solution is by far better. So let's do it
this way.

Best, Wolfgang