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.

_______________________________________________
Xenomai-core mailing list
Xenomai-core@gna.org
https://mail.gna.org/listinfo/xenomai-core

Reply via email to