| Message ID | 20160719101839.106211-1-brian.brooks@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
On 07/21 18:36:22, Christophe Milard wrote: > Also... Can we do the same with the other symbols: for instance > odp_cpu_pause() as _odp_cpu_pause() in /arch/XXX, and the definition > of odp_cpu_pause as inline _odp_cpu_pause in Include? > same question for other (odp_cpu_cycles, odp_cpu_cycles_max, > odp_cpu_cycles_resolution), where default stubs could be defined when > appropriate If we can assume that ODP implementation libodp.a and ODP application App.exe run on the same CPU, +------------+ +-----------+ | libodp.a | odp.h | App.exe | +------------+ +-----------+ | | | | ============== | CPU | ============== Then things like cache line size and yielding or pausing the CPU core are the *same* for both code bases (internally). So, is it natural for one to define it for the other to use (including for its own internal use)? If not, what to do? Remove it from the API and have each code base define it internally. This may sound like duplication, but it is a step towards loose coupling and simplifies the API. An alternative is to create a new library and API which both libodp.a and App.exe may use. App.exe may optionally use it. If this does seem natural, will App.exe do the required search and replace to use these CPU symbols? And, will libodp.a implementers want to use such external symbols internally? > The idea being to remove all definition of API symbols from this > internal arch directory. It would be nice to find a way to do this without introducing an extra layer of symbol indirection and thus leakage. We agreed upon prefixing such symbols with '_', but perhaps there is a way to prevent leakage? I think it can be done if removed from the API or another library is created as mentioned above.
Hi, Got: Using patch: 0001-linux-generic-fixup-cache-line-size-API.patch Trying to apply patch Patch applied WARNING: Possible unwrapped commit description (prefer a maximum 75 chars per line) Also see my question inline below... On 19 July 2016 at 12:18, Brian Brooks <brian.brooks@linaro.org> wrote: > Define the ODP API for cache line size to the cache line size defined in the > (internal) architecture specific directories. Prefix internal cache line size > identifier with '_' since it leaks into the ODP API header files by inclusion. > > Signed-off-by: Brian Brooks <brian.brooks@linaro.org> > --- > platform/linux-generic/arch/default/odp/api/cpu_arch.h | 10 +--------- > platform/linux-generic/arch/mips64/odp/api/cpu_arch.h | 14 ++++---------- > platform/linux-generic/arch/powerpc/odp/api/cpu_arch.h | 10 +--------- > platform/linux-generic/arch/x86/odp/api/cpu_arch.h | 10 +--------- > platform/linux-generic/include/odp/api/align.h | 8 ++------ > 5 files changed, 9 insertions(+), 43 deletions(-) > > diff --git a/platform/linux-generic/arch/default/odp/api/cpu_arch.h b/platform/linux-generic/arch/default/odp/api/cpu_arch.h > index 29f8889..22b1da2 100644 > --- a/platform/linux-generic/arch/default/odp/api/cpu_arch.h > +++ b/platform/linux-generic/arch/default/odp/api/cpu_arch.h > @@ -11,15 +11,7 @@ > extern "C" { > #endif > > -/** @ingroup odp_compiler_optim > - * @{ > - */ > - > -#define ODP_CACHE_LINE_SIZE 64 > - > -/** > - * @} > - */ > +#define _ODP_CACHE_LINE_SIZE 64 > > static inline void odp_cpu_pause(void) > { > diff --git a/platform/linux-generic/arch/mips64/odp/api/cpu_arch.h b/platform/linux-generic/arch/mips64/odp/api/cpu_arch.h > index 7b5bfd2..1f49cd2 100644 > --- a/platform/linux-generic/arch/mips64/odp/api/cpu_arch.h > +++ b/platform/linux-generic/arch/mips64/odp/api/cpu_arch.h > @@ -11,18 +11,12 @@ > extern "C" { > #endif > > -/** @ingroup odp_compiler_optim > - * @{ > - */ > - > -#if defined __OCTEON__ > -#define ODP_CACHE_LINE_SIZE 128 > +#if defined(__OCTEON__) Any reason I don't know for these parentheses? I am not aware of them doing any difference and it does not seem we use to have then in the rest of the code. Christophe. > +#define _ODP_CACHE_LINE_SIZE 128 > +#else > +#error Please add support for your arch in cpu_arch.h > #endif > > -/** > - * @} > - */ > - > static inline void odp_cpu_pause(void) > { > __asm__ __volatile__ ("nop"); > diff --git a/platform/linux-generic/arch/powerpc/odp/api/cpu_arch.h b/platform/linux-generic/arch/powerpc/odp/api/cpu_arch.h > index 29f8889..22b1da2 100644 > --- a/platform/linux-generic/arch/powerpc/odp/api/cpu_arch.h > +++ b/platform/linux-generic/arch/powerpc/odp/api/cpu_arch.h > @@ -11,15 +11,7 @@ > extern "C" { > #endif > > -/** @ingroup odp_compiler_optim > - * @{ > - */ > - > -#define ODP_CACHE_LINE_SIZE 64 > - > -/** > - * @} > - */ > +#define _ODP_CACHE_LINE_SIZE 64 > > static inline void odp_cpu_pause(void) > { > diff --git a/platform/linux-generic/arch/x86/odp/api/cpu_arch.h b/platform/linux-generic/arch/x86/odp/api/cpu_arch.h > index 3a16fa6..44e6b30 100644 > --- a/platform/linux-generic/arch/x86/odp/api/cpu_arch.h > +++ b/platform/linux-generic/arch/x86/odp/api/cpu_arch.h > @@ -11,15 +11,7 @@ > extern "C" { > #endif > > -/** @ingroup odp_compiler_optim > - * @{ > - */ > - > -#define ODP_CACHE_LINE_SIZE 64 > - > -/** > - * @} > - */ > +#define _ODP_CACHE_LINE_SIZE 64 > > static inline void odp_cpu_pause(void) > { > diff --git a/platform/linux-generic/include/odp/api/align.h b/platform/linux-generic/include/odp/api/align.h > index d8bc653..c36b17b 100644 > --- a/platform/linux-generic/include/odp/api/align.h > +++ b/platform/linux-generic/include/odp/api/align.h > @@ -31,16 +31,12 @@ extern "C" { > > #define ODP_FIELD_SIZEOF(type, member) sizeof(((type *)0)->member) > > -#if defined __arm__ || defined __aarch64__ > - > -#define ODP_CACHE_LINE_SIZE 64 > - > -#endif > - > #else > #error Non-gcc compatible compiler > #endif > > +#define ODP_CACHE_LINE_SIZE _ODP_CACHE_LINE_SIZE > + > #define ODP_PAGE_SIZE 4096 > > #define ODP_ALIGNED_CACHE ODP_ALIGNED(ODP_CACHE_LINE_SIZE) > -- > 2.9.0 >
Also... Can we do the same with the other symbols: for instance odp_cpu_pause() as _odp_cpu_pause() in /arch/XXX, and the definition of odp_cpu_pause as inline _odp_cpu_pause in Include? same question for other (odp_cpu_cycles, odp_cpu_cycles_max, odp_cpu_cycles_resolution), where default stubs could be defined when appropriate The idea being to remove all definition of API symbols from this internal arch directory. Could come as other patch, I guess, but the question remains... Christophe. On 21 July 2016 at 18:26, Christophe Milard <christophe.milard@linaro.org> wrote: > Hi, > > Got: > Using patch: 0001-linux-generic-fixup-cache-line-size-API.patch > Trying to apply patch > Patch applied > WARNING: Possible unwrapped commit description (prefer a maximum 75 > chars per line) > > Also see my question inline below... > > > On 19 July 2016 at 12:18, Brian Brooks <brian.brooks@linaro.org> wrote: >> Define the ODP API for cache line size to the cache line size defined in the >> (internal) architecture specific directories. Prefix internal cache line size >> identifier with '_' since it leaks into the ODP API header files by inclusion. >> >> Signed-off-by: Brian Brooks <brian.brooks@linaro.org> >> --- >> platform/linux-generic/arch/default/odp/api/cpu_arch.h | 10 +--------- >> platform/linux-generic/arch/mips64/odp/api/cpu_arch.h | 14 ++++---------- >> platform/linux-generic/arch/powerpc/odp/api/cpu_arch.h | 10 +--------- >> platform/linux-generic/arch/x86/odp/api/cpu_arch.h | 10 +--------- >> platform/linux-generic/include/odp/api/align.h | 8 ++------ >> 5 files changed, 9 insertions(+), 43 deletions(-) >> >> diff --git a/platform/linux-generic/arch/default/odp/api/cpu_arch.h b/platform/linux-generic/arch/default/odp/api/cpu_arch.h >> index 29f8889..22b1da2 100644 >> --- a/platform/linux-generic/arch/default/odp/api/cpu_arch.h >> +++ b/platform/linux-generic/arch/default/odp/api/cpu_arch.h >> @@ -11,15 +11,7 @@ >> extern "C" { >> #endif >> >> -/** @ingroup odp_compiler_optim >> - * @{ >> - */ >> - >> -#define ODP_CACHE_LINE_SIZE 64 >> - >> -/** >> - * @} >> - */ >> +#define _ODP_CACHE_LINE_SIZE 64 >> >> static inline void odp_cpu_pause(void) >> { >> diff --git a/platform/linux-generic/arch/mips64/odp/api/cpu_arch.h b/platform/linux-generic/arch/mips64/odp/api/cpu_arch.h >> index 7b5bfd2..1f49cd2 100644 >> --- a/platform/linux-generic/arch/mips64/odp/api/cpu_arch.h >> +++ b/platform/linux-generic/arch/mips64/odp/api/cpu_arch.h >> @@ -11,18 +11,12 @@ >> extern "C" { >> #endif >> >> -/** @ingroup odp_compiler_optim >> - * @{ >> - */ >> - >> -#if defined __OCTEON__ >> -#define ODP_CACHE_LINE_SIZE 128 >> +#if defined(__OCTEON__) > > Any reason I don't know for these parentheses? I am not aware of them > doing any difference and it does not seem we use to have then in the > rest of the code. > > Christophe. > >> +#define _ODP_CACHE_LINE_SIZE 128 >> +#else >> +#error Please add support for your arch in cpu_arch.h >> #endif >> >> -/** >> - * @} >> - */ >> - >> static inline void odp_cpu_pause(void) >> { >> __asm__ __volatile__ ("nop"); >> diff --git a/platform/linux-generic/arch/powerpc/odp/api/cpu_arch.h b/platform/linux-generic/arch/powerpc/odp/api/cpu_arch.h >> index 29f8889..22b1da2 100644 >> --- a/platform/linux-generic/arch/powerpc/odp/api/cpu_arch.h >> +++ b/platform/linux-generic/arch/powerpc/odp/api/cpu_arch.h >> @@ -11,15 +11,7 @@ >> extern "C" { >> #endif >> >> -/** @ingroup odp_compiler_optim >> - * @{ >> - */ >> - >> -#define ODP_CACHE_LINE_SIZE 64 >> - >> -/** >> - * @} >> - */ >> +#define _ODP_CACHE_LINE_SIZE 64 >> >> static inline void odp_cpu_pause(void) >> { >> diff --git a/platform/linux-generic/arch/x86/odp/api/cpu_arch.h b/platform/linux-generic/arch/x86/odp/api/cpu_arch.h >> index 3a16fa6..44e6b30 100644 >> --- a/platform/linux-generic/arch/x86/odp/api/cpu_arch.h >> +++ b/platform/linux-generic/arch/x86/odp/api/cpu_arch.h >> @@ -11,15 +11,7 @@ >> extern "C" { >> #endif >> >> -/** @ingroup odp_compiler_optim >> - * @{ >> - */ >> - >> -#define ODP_CACHE_LINE_SIZE 64 >> - >> -/** >> - * @} >> - */ >> +#define _ODP_CACHE_LINE_SIZE 64 >> >> static inline void odp_cpu_pause(void) >> { >> diff --git a/platform/linux-generic/include/odp/api/align.h b/platform/linux-generic/include/odp/api/align.h >> index d8bc653..c36b17b 100644 >> --- a/platform/linux-generic/include/odp/api/align.h >> +++ b/platform/linux-generic/include/odp/api/align.h >> @@ -31,16 +31,12 @@ extern "C" { >> >> #define ODP_FIELD_SIZEOF(type, member) sizeof(((type *)0)->member) >> >> -#if defined __arm__ || defined __aarch64__ >> - >> -#define ODP_CACHE_LINE_SIZE 64 >> - >> -#endif >> - >> #else >> #error Non-gcc compatible compiler >> #endif >> >> +#define ODP_CACHE_LINE_SIZE _ODP_CACHE_LINE_SIZE >> + >> #define ODP_PAGE_SIZE 4096 >> >> #define ODP_ALIGNED_CACHE ODP_ALIGNED(ODP_CACHE_LINE_SIZE) >> -- >> 2.9.0 >>
On 19 July 2016 at 17:23, Brian Brooks <brian.brooks@linaro.org> wrote: > On 07/21 18:36:22, Christophe Milard wrote: >> Also... Can we do the same with the other symbols: for instance >> odp_cpu_pause() as _odp_cpu_pause() in /arch/XXX, and the definition >> of odp_cpu_pause as inline _odp_cpu_pause in Include? >> same question for other (odp_cpu_cycles, odp_cpu_cycles_max, >> odp_cpu_cycles_resolution), where default stubs could be defined when >> appropriate > > If we can assume that ODP implementation libodp.a and ODP application App.exe > run on the same CPU, > > +------------+ +-----------+ > | libodp.a | odp.h | App.exe | > +------------+ +-----------+ > | | > | | > ============== > | CPU | > ============== > > Then things like cache line size and yielding or pausing the CPU core are the > *same* for both code bases (internally). So, is it natural for one to define it > for the other to use (including for its own internal use)? > > If not, what to do? Remove it from the API and have each code base define it > internally. This may sound like duplication, but it is a step towards loose > coupling and simplifies the API. > > An alternative is to create a new library and API which both libodp.a and > App.exe may use. App.exe may optionally use it. > > If this does seem natural, will App.exe do the required search and replace > to use these CPU symbols? And, will libodp.a implementers want to use such > external symbols internally? Not sure I am fully following you here. Having 2 different declaration of these symbols would just make things worse wouldn't it? Maybe we can discuss than on the arch call on monday? > >> The idea being to remove all definition of API symbols from this >> internal arch directory. > > It would be nice to find a way to do this without introducing an extra layer > of symbol indirection and thus leakage. We agreed upon prefixing such symbols > with '_', but perhaps there is a way to prevent leakage? I think it can be done The decision was to prefix leaking symbols with _odp if I remember right. Bill did introduce the "visibility" files (odp/platform/linux-generic/include/odp/visibility_*) which make use of a pragma to control visibility. I thinks this just affects shared lib, though. Christophe > if removed from the API or another library is created as mentioned above.
Another comment: Do we have now any reason to keep these paths: platform/linux-generic/arch/powerpc/odp/api/* I mean, if we intend to suppress the api definition things from this place, should this simply be: platform/linux-generic/arch/powerpc/* ? Christophe On 19 July 2016 at 12:18, Brian Brooks <brian.brooks@linaro.org> wrote: > Define the ODP API for cache line size to the cache line size defined in the > (internal) architecture specific directories. Prefix internal cache line size > identifier with '_' since it leaks into the ODP API header files by inclusion. > > Signed-off-by: Brian Brooks <brian.brooks@linaro.org> > --- > platform/linux-generic/arch/default/odp/api/cpu_arch.h | 10 +--------- > platform/linux-generic/arch/mips64/odp/api/cpu_arch.h | 14 ++++---------- > platform/linux-generic/arch/powerpc/odp/api/cpu_arch.h | 10 +--------- > platform/linux-generic/arch/x86/odp/api/cpu_arch.h | 10 +--------- > platform/linux-generic/include/odp/api/align.h | 8 ++------ > 5 files changed, 9 insertions(+), 43 deletions(-) > > diff --git a/platform/linux-generic/arch/default/odp/api/cpu_arch.h b/platform/linux-generic/arch/default/odp/api/cpu_arch.h > index 29f8889..22b1da2 100644 > --- a/platform/linux-generic/arch/default/odp/api/cpu_arch.h > +++ b/platform/linux-generic/arch/default/odp/api/cpu_arch.h > @@ -11,15 +11,7 @@ > extern "C" { > #endif > > -/** @ingroup odp_compiler_optim > - * @{ > - */ > - > -#define ODP_CACHE_LINE_SIZE 64 > - > -/** > - * @} > - */ > +#define _ODP_CACHE_LINE_SIZE 64 > > static inline void odp_cpu_pause(void) > { > diff --git a/platform/linux-generic/arch/mips64/odp/api/cpu_arch.h b/platform/linux-generic/arch/mips64/odp/api/cpu_arch.h > index 7b5bfd2..1f49cd2 100644 > --- a/platform/linux-generic/arch/mips64/odp/api/cpu_arch.h > +++ b/platform/linux-generic/arch/mips64/odp/api/cpu_arch.h > @@ -11,18 +11,12 @@ > extern "C" { > #endif > > -/** @ingroup odp_compiler_optim > - * @{ > - */ > - > -#if defined __OCTEON__ > -#define ODP_CACHE_LINE_SIZE 128 > +#if defined(__OCTEON__) > +#define _ODP_CACHE_LINE_SIZE 128 > +#else > +#error Please add support for your arch in cpu_arch.h > #endif > > -/** > - * @} > - */ > - > static inline void odp_cpu_pause(void) > { > __asm__ __volatile__ ("nop"); > diff --git a/platform/linux-generic/arch/powerpc/odp/api/cpu_arch.h b/platform/linux-generic/arch/powerpc/odp/api/cpu_arch.h > index 29f8889..22b1da2 100644 > --- a/platform/linux-generic/arch/powerpc/odp/api/cpu_arch.h > +++ b/platform/linux-generic/arch/powerpc/odp/api/cpu_arch.h > @@ -11,15 +11,7 @@ > extern "C" { > #endif > > -/** @ingroup odp_compiler_optim > - * @{ > - */ > - > -#define ODP_CACHE_LINE_SIZE 64 > - > -/** > - * @} > - */ > +#define _ODP_CACHE_LINE_SIZE 64 > > static inline void odp_cpu_pause(void) > { > diff --git a/platform/linux-generic/arch/x86/odp/api/cpu_arch.h b/platform/linux-generic/arch/x86/odp/api/cpu_arch.h > index 3a16fa6..44e6b30 100644 > --- a/platform/linux-generic/arch/x86/odp/api/cpu_arch.h > +++ b/platform/linux-generic/arch/x86/odp/api/cpu_arch.h > @@ -11,15 +11,7 @@ > extern "C" { > #endif > > -/** @ingroup odp_compiler_optim > - * @{ > - */ > - > -#define ODP_CACHE_LINE_SIZE 64 > - > -/** > - * @} > - */ > +#define _ODP_CACHE_LINE_SIZE 64 > > static inline void odp_cpu_pause(void) > { > diff --git a/platform/linux-generic/include/odp/api/align.h b/platform/linux-generic/include/odp/api/align.h > index d8bc653..c36b17b 100644 > --- a/platform/linux-generic/include/odp/api/align.h > +++ b/platform/linux-generic/include/odp/api/align.h > @@ -31,16 +31,12 @@ extern "C" { > > #define ODP_FIELD_SIZEOF(type, member) sizeof(((type *)0)->member) > > -#if defined __arm__ || defined __aarch64__ > - > -#define ODP_CACHE_LINE_SIZE 64 > - > -#endif > - > #else > #error Non-gcc compatible compiler > #endif > > +#define ODP_CACHE_LINE_SIZE _ODP_CACHE_LINE_SIZE > + > #define ODP_PAGE_SIZE 4096 > > #define ODP_ALIGNED_CACHE ODP_ALIGNED(ODP_CACHE_LINE_SIZE) > -- > 2.9.0 >
an another strange thing: Shouldn't the statement: #include <odp/api/cpu_arch.h> defining _ODP_CACHE_LINE_SIZE be at the beginning of platform/linux-generic/include/odp/api/align.h whichb uses it? Seems to work, though. But I really wonder how :-) On 19 July 2016 at 12:18, Brian Brooks <brian.brooks@linaro.org> wrote: > Define the ODP API for cache line size to the cache line size defined in the > (internal) architecture specific directories. Prefix internal cache line size > identifier with '_' since it leaks into the ODP API header files by inclusion. > > Signed-off-by: Brian Brooks <brian.brooks@linaro.org> > --- > platform/linux-generic/arch/default/odp/api/cpu_arch.h | 10 +--------- > platform/linux-generic/arch/mips64/odp/api/cpu_arch.h | 14 ++++---------- > platform/linux-generic/arch/powerpc/odp/api/cpu_arch.h | 10 +--------- > platform/linux-generic/arch/x86/odp/api/cpu_arch.h | 10 +--------- > platform/linux-generic/include/odp/api/align.h | 8 ++------ > 5 files changed, 9 insertions(+), 43 deletions(-) > > diff --git a/platform/linux-generic/arch/default/odp/api/cpu_arch.h b/platform/linux-generic/arch/default/odp/api/cpu_arch.h > index 29f8889..22b1da2 100644 > --- a/platform/linux-generic/arch/default/odp/api/cpu_arch.h > +++ b/platform/linux-generic/arch/default/odp/api/cpu_arch.h > @@ -11,15 +11,7 @@ > extern "C" { > #endif > > -/** @ingroup odp_compiler_optim > - * @{ > - */ > - > -#define ODP_CACHE_LINE_SIZE 64 > - > -/** > - * @} > - */ > +#define _ODP_CACHE_LINE_SIZE 64 > > static inline void odp_cpu_pause(void) > { > diff --git a/platform/linux-generic/arch/mips64/odp/api/cpu_arch.h b/platform/linux-generic/arch/mips64/odp/api/cpu_arch.h > index 7b5bfd2..1f49cd2 100644 > --- a/platform/linux-generic/arch/mips64/odp/api/cpu_arch.h > +++ b/platform/linux-generic/arch/mips64/odp/api/cpu_arch.h > @@ -11,18 +11,12 @@ > extern "C" { > #endif > > -/** @ingroup odp_compiler_optim > - * @{ > - */ > - > -#if defined __OCTEON__ > -#define ODP_CACHE_LINE_SIZE 128 > +#if defined(__OCTEON__) > +#define _ODP_CACHE_LINE_SIZE 128 > +#else > +#error Please add support for your arch in cpu_arch.h > #endif > > -/** > - * @} > - */ > - > static inline void odp_cpu_pause(void) > { > __asm__ __volatile__ ("nop"); > diff --git a/platform/linux-generic/arch/powerpc/odp/api/cpu_arch.h b/platform/linux-generic/arch/powerpc/odp/api/cpu_arch.h > index 29f8889..22b1da2 100644 > --- a/platform/linux-generic/arch/powerpc/odp/api/cpu_arch.h > +++ b/platform/linux-generic/arch/powerpc/odp/api/cpu_arch.h > @@ -11,15 +11,7 @@ > extern "C" { > #endif > > -/** @ingroup odp_compiler_optim > - * @{ > - */ > - > -#define ODP_CACHE_LINE_SIZE 64 > - > -/** > - * @} > - */ > +#define _ODP_CACHE_LINE_SIZE 64 > > static inline void odp_cpu_pause(void) > { > diff --git a/platform/linux-generic/arch/x86/odp/api/cpu_arch.h b/platform/linux-generic/arch/x86/odp/api/cpu_arch.h > index 3a16fa6..44e6b30 100644 > --- a/platform/linux-generic/arch/x86/odp/api/cpu_arch.h > +++ b/platform/linux-generic/arch/x86/odp/api/cpu_arch.h > @@ -11,15 +11,7 @@ > extern "C" { > #endif > > -/** @ingroup odp_compiler_optim > - * @{ > - */ > - > -#define ODP_CACHE_LINE_SIZE 64 > - > -/** > - * @} > - */ > +#define _ODP_CACHE_LINE_SIZE 64 > > static inline void odp_cpu_pause(void) > { > diff --git a/platform/linux-generic/include/odp/api/align.h b/platform/linux-generic/include/odp/api/align.h > index d8bc653..c36b17b 100644 > --- a/platform/linux-generic/include/odp/api/align.h > +++ b/platform/linux-generic/include/odp/api/align.h > @@ -31,16 +31,12 @@ extern "C" { > > #define ODP_FIELD_SIZEOF(type, member) sizeof(((type *)0)->member) > > -#if defined __arm__ || defined __aarch64__ > - > -#define ODP_CACHE_LINE_SIZE 64 > - > -#endif > - > #else > #error Non-gcc compatible compiler > #endif > > +#define ODP_CACHE_LINE_SIZE _ODP_CACHE_LINE_SIZE > + > #define ODP_PAGE_SIZE 4096 > > #define ODP_ALIGNED_CACHE ODP_ALIGNED(ODP_CACHE_LINE_SIZE) > -- > 2.9.0 >
On 07/22 15:00:49, Christophe Milard wrote: > an another strange thing: > Shouldn't the statement: > #include <odp/api/cpu_arch.h> > defining _ODP_CACHE_LINE_SIZE be at the beginning of > platform/linux-generic/include/odp/api/align.h whichb uses it? > > Seems to work, though. But I really wonder how :-) This is what's happening: #define A _A #define _A 5 /* use A */ This is an error: #define A _A /* use A */ #define _A 5
On 07/22 08:39:38, Christophe Milard wrote: > On 19 July 2016 at 17:23, Brian Brooks <brian.brooks@linaro.org> wrote: > > On 07/21 18:36:22, Christophe Milard wrote: > >> Also... Can we do the same with the other symbols: for instance > >> odp_cpu_pause() as _odp_cpu_pause() in /arch/XXX, and the definition > >> of odp_cpu_pause as inline _odp_cpu_pause in Include? > >> same question for other (odp_cpu_cycles, odp_cpu_cycles_max, > >> odp_cpu_cycles_resolution), where default stubs could be defined when > >> appropriate > > > > If we can assume that ODP implementation libodp.a and ODP application App.exe > > run on the same CPU, > > > > +------------+ +-----------+ > > | libodp.a | odp.h | App.exe | > > +------------+ +-----------+ > > | | > > | | > > ============== > > | CPU | > > ============== > > > > Then things like cache line size and yielding or pausing the CPU core are the > > *same* for both code bases (internally). So, is it natural for one to define it > > for the other to use (including for its own internal use)? > > > > If not, what to do? Remove it from the API and have each code base define it > > internally. This may sound like duplication, but it is a step towards loose > > coupling and simplifies the API. > > > > An alternative is to create a new library and API which both libodp.a and > > App.exe may use. App.exe may optionally use it. > > > > If this does seem natural, will App.exe do the required search and replace > > to use these CPU symbols? And, will libodp.a implementers want to use such > > external symbols internally? > > Not sure I am fully following you here. Having 2 different declaration > of these symbols would just make things worse wouldn't it? I don't think it makes things worse. It just makes them more independent and the interface between the two become smaller and easier to understand and use. Things like compiler built-ins, cache line size, and page size are useful when implementing things, but they do not belong in the top-level API for use by other applications. > Maybe we can discuss than on the arch call on monday? > > > > >> The idea being to remove all definition of API symbols from this > >> internal arch directory. > > > > It would be nice to find a way to do this without introducing an extra layer > > of symbol indirection and thus leakage. We agreed upon prefixing such symbols > > with '_', but perhaps there is a way to prevent leakage? I think it can be done > > The decision was to prefix leaking symbols with _odp if I remember right. > Bill did introduce the "visibility" files > (odp/platform/linux-generic/include/odp/visibility_*) which make use > of a pragma to control visibility. I thinks this just affects shared > lib, though. I think so too. Which means it only controls visibility of a subset of the API. It has no effect on APIs which are static inline or #define. To remove these static inlines or #defines from the internal arch/ directory means they will have to be lifted somewhere in: /platform/linux-generic/include/odp/ such as /platform/linux-generic/include/odp/arch/<arch>/cpu.h #define ODP_CACHE_LINE_SIZE And then wrapped in /platform/linux-generic/arch/<arch>/cpu.h #define _ODP_CACHE_LINE_SIZE ODP_CACHE_LINE_SIZE Does this work for APIs defined in the arch directory which are real function function calls e.g. odp_cpu_cycles_diff()? It's possible, but static inline odp_cpu_cycles_diff(a, b) { _odp_cpu_cycles_diff(a, b); } requires visilbity of the internal _odp_cpu_cycles_diff symbol to be leaked.
Brian: can you join the arch call this afternoon (your morning). would be nice to clarify what we want to do here... Thanks Christophe. On 22 July 2016 at 19:25, Brian Brooks <brian.brooks@linaro.org> wrote: > On 07/22 08:39:38, Christophe Milard wrote: >> On 19 July 2016 at 17:23, Brian Brooks <brian.brooks@linaro.org> wrote: >> > On 07/21 18:36:22, Christophe Milard wrote: >> >> Also... Can we do the same with the other symbols: for instance >> >> odp_cpu_pause() as _odp_cpu_pause() in /arch/XXX, and the definition >> >> of odp_cpu_pause as inline _odp_cpu_pause in Include? >> >> same question for other (odp_cpu_cycles, odp_cpu_cycles_max, >> >> odp_cpu_cycles_resolution), where default stubs could be defined when >> >> appropriate >> > >> > If we can assume that ODP implementation libodp.a and ODP application App.exe >> > run on the same CPU, >> > >> > +------------+ +-----------+ >> > | libodp.a | odp.h | App.exe | >> > +------------+ +-----------+ >> > | | >> > | | >> > ============== >> > | CPU | >> > ============== >> > >> > Then things like cache line size and yielding or pausing the CPU core are the >> > *same* for both code bases (internally). So, is it natural for one to define it >> > for the other to use (including for its own internal use)? >> > >> > If not, what to do? Remove it from the API and have each code base define it >> > internally. This may sound like duplication, but it is a step towards loose >> > coupling and simplifies the API. >> > >> > An alternative is to create a new library and API which both libodp.a and >> > App.exe may use. App.exe may optionally use it. >> > >> > If this does seem natural, will App.exe do the required search and replace >> > to use these CPU symbols? And, will libodp.a implementers want to use such >> > external symbols internally? >> >> Not sure I am fully following you here. Having 2 different declaration >> of these symbols would just make things worse wouldn't it? > > I don't think it makes things worse. It just makes them more independent and the > interface between the two become smaller and easier to understand and use. > > Things like compiler built-ins, cache line size, and page size are useful when > implementing things, but they do not belong in the top-level API for use by > other applications. > >> Maybe we can discuss than on the arch call on monday? >> >> > >> >> The idea being to remove all definition of API symbols from this >> >> internal arch directory. >> > >> > It would be nice to find a way to do this without introducing an extra layer >> > of symbol indirection and thus leakage. We agreed upon prefixing such symbols >> > with '_', but perhaps there is a way to prevent leakage? I think it can be done >> >> The decision was to prefix leaking symbols with _odp if I remember right. >> Bill did introduce the "visibility" files >> (odp/platform/linux-generic/include/odp/visibility_*) which make use >> of a pragma to control visibility. I thinks this just affects shared >> lib, though. > > I think so too. Which means it only controls visibility of a subset of the API. > It has no effect on APIs which are static inline or #define. > > To remove these static inlines or #defines from the internal arch/ directory > means they will have to be lifted somewhere in: > > /platform/linux-generic/include/odp/ > > such as > > /platform/linux-generic/include/odp/arch/<arch>/cpu.h > > #define ODP_CACHE_LINE_SIZE > > And then wrapped in > > /platform/linux-generic/arch/<arch>/cpu.h > > #define _ODP_CACHE_LINE_SIZE ODP_CACHE_LINE_SIZE > > Does this work for APIs defined in the arch directory which are real function > function calls e.g. odp_cpu_cycles_diff()? It's possible, but > > static inline odp_cpu_cycles_diff(a, b) { _odp_cpu_cycles_diff(a, b); } > > requires visilbity of the internal _odp_cpu_cycles_diff symbol to be leaked.
diff --git a/platform/linux-generic/arch/default/odp/api/cpu_arch.h b/platform/linux-generic/arch/default/odp/api/cpu_arch.h index 29f8889..22b1da2 100644 --- a/platform/linux-generic/arch/default/odp/api/cpu_arch.h +++ b/platform/linux-generic/arch/default/odp/api/cpu_arch.h @@ -11,15 +11,7 @@ extern "C" { #endif -/** @ingroup odp_compiler_optim - * @{ - */ - -#define ODP_CACHE_LINE_SIZE 64 - -/** - * @} - */ +#define _ODP_CACHE_LINE_SIZE 64 static inline void odp_cpu_pause(void) { diff --git a/platform/linux-generic/arch/mips64/odp/api/cpu_arch.h b/platform/linux-generic/arch/mips64/odp/api/cpu_arch.h index 7b5bfd2..1f49cd2 100644 --- a/platform/linux-generic/arch/mips64/odp/api/cpu_arch.h +++ b/platform/linux-generic/arch/mips64/odp/api/cpu_arch.h @@ -11,18 +11,12 @@ extern "C" { #endif -/** @ingroup odp_compiler_optim - * @{ - */ - -#if defined __OCTEON__ -#define ODP_CACHE_LINE_SIZE 128 +#if defined(__OCTEON__) +#define _ODP_CACHE_LINE_SIZE 128 +#else +#error Please add support for your arch in cpu_arch.h #endif -/** - * @} - */ - static inline void odp_cpu_pause(void) { __asm__ __volatile__ ("nop"); diff --git a/platform/linux-generic/arch/powerpc/odp/api/cpu_arch.h b/platform/linux-generic/arch/powerpc/odp/api/cpu_arch.h index 29f8889..22b1da2 100644 --- a/platform/linux-generic/arch/powerpc/odp/api/cpu_arch.h +++ b/platform/linux-generic/arch/powerpc/odp/api/cpu_arch.h @@ -11,15 +11,7 @@ extern "C" { #endif -/** @ingroup odp_compiler_optim - * @{ - */ - -#define ODP_CACHE_LINE_SIZE 64 - -/** - * @} - */ +#define _ODP_CACHE_LINE_SIZE 64 static inline void odp_cpu_pause(void) { diff --git a/platform/linux-generic/arch/x86/odp/api/cpu_arch.h b/platform/linux-generic/arch/x86/odp/api/cpu_arch.h index 3a16fa6..44e6b30 100644 --- a/platform/linux-generic/arch/x86/odp/api/cpu_arch.h +++ b/platform/linux-generic/arch/x86/odp/api/cpu_arch.h @@ -11,15 +11,7 @@ extern "C" { #endif -/** @ingroup odp_compiler_optim - * @{ - */ - -#define ODP_CACHE_LINE_SIZE 64 - -/** - * @} - */ +#define _ODP_CACHE_LINE_SIZE 64 static inline void odp_cpu_pause(void) { diff --git a/platform/linux-generic/include/odp/api/align.h b/platform/linux-generic/include/odp/api/align.h index d8bc653..c36b17b 100644 --- a/platform/linux-generic/include/odp/api/align.h +++ b/platform/linux-generic/include/odp/api/align.h @@ -31,16 +31,12 @@ extern "C" { #define ODP_FIELD_SIZEOF(type, member) sizeof(((type *)0)->member) -#if defined __arm__ || defined __aarch64__ - -#define ODP_CACHE_LINE_SIZE 64 - -#endif - #else #error Non-gcc compatible compiler #endif +#define ODP_CACHE_LINE_SIZE _ODP_CACHE_LINE_SIZE + #define ODP_PAGE_SIZE 4096 #define ODP_ALIGNED_CACHE ODP_ALIGNED(ODP_CACHE_LINE_SIZE)
Define the ODP API for cache line size to the cache line size defined in the (internal) architecture specific directories. Prefix internal cache line size identifier with '_' since it leaks into the ODP API header files by inclusion. Signed-off-by: Brian Brooks <brian.brooks@linaro.org> --- platform/linux-generic/arch/default/odp/api/cpu_arch.h | 10 +--------- platform/linux-generic/arch/mips64/odp/api/cpu_arch.h | 14 ++++---------- platform/linux-generic/arch/powerpc/odp/api/cpu_arch.h | 10 +--------- platform/linux-generic/arch/x86/odp/api/cpu_arch.h | 10 +--------- platform/linux-generic/include/odp/api/align.h | 8 ++------ 5 files changed, 9 insertions(+), 43 deletions(-) -- 2.9.0