diff mbox

[1/4] linux-generic: define cache line size in one place

Message ID 1468862126-24982-1-git-send-email-brian.brooks@linaro.org
State New
Headers show

Commit Message

Brian Brooks July 18, 2016, 5:15 p.m. UTC 
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  | 12 ------------
 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 +-------
 platform/linux-generic/include/odp/api/cpu.h           | 17 +++++++++++++++++
 6 files changed, 18 insertions(+), 49 deletions(-)

-- 
2.9.0

Comments

Mike Holmes July 18, 2016, 5:54 p.m. UTC | #1 
Is there a performance benifit to this ?

I would rather we have non conflicting per architecture files, we had all
the defines in one file before and it caused issues when people sent
patches that conflicted, I now have to review an x86 change even if I am on
an ARM platform just incase something was broken.

At a personal level which is not a great reason for change I admit I hate
nests of #defines and trying to decide which ones are being built in, I
love that it is really clear form the compile log when they are separate

I would actually like to see all the per arch stuff pulled out period.  CPU
architecture can be its own lib with the differences for ARM, X86 all
gathered in there and reduce the amount of code in linux-generic, given the
debian packaging this can be installed correctly once for all arm, x86 etc
in a system with no need to include the code in your platform at all.

I see this architecture specific lib implementing say odp_crypto in the
most SW efficient way possible, same for other architectures where this is
a performance improved SW solution using arch specific instructions rather
than HW accelerators.


On 18 July 2016 at 13:15, Brian Brooks <brian.brooks@linaro.org> wrote:

> 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  | 12 ------------

>  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 +-------

>  platform/linux-generic/include/odp/api/cpu.h           | 17

> +++++++++++++++++

>  6 files changed, 18 insertions(+), 49 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..1c79f87 100644

> --- a/platform/linux-generic/arch/default/odp/api/cpu_arch.h

> +++ b/platform/linux-generic/arch/default/odp/api/cpu_arch.h

> @@ -11,16 +11,6 @@

>  extern "C" {

>  #endif

>

> -/** @ingroup odp_compiler_optim

> - *  @{

> - */

> -

> -#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..3bfa0dc 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,6 @@

>  extern "C" {

>  #endif

>

> -/** @ingroup odp_compiler_optim

> - *  @{

> - */

> -

> -#if defined __OCTEON__

> -#define ODP_CACHE_LINE_SIZE 128

> -#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..1c79f87 100644

> --- a/platform/linux-generic/arch/powerpc/odp/api/cpu_arch.h

> +++ b/platform/linux-generic/arch/powerpc/odp/api/cpu_arch.h

> @@ -11,16 +11,6 @@

>  extern "C" {

>  #endif

>

> -/** @ingroup odp_compiler_optim

> - *  @{

> - */

> -

> -#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..997a954 100644

> --- a/platform/linux-generic/arch/x86/odp/api/cpu_arch.h

> +++ b/platform/linux-generic/arch/x86/odp/api/cpu_arch.h

> @@ -11,16 +11,6 @@

>  extern "C" {

>  #endif

>

> -/** @ingroup odp_compiler_optim

> - *  @{

> - */

> -

> -#define ODP_CACHE_LINE_SIZE 64

> -

> -/**

> - * @}

> - */

> -

>  static inline void odp_cpu_pause(void)

>  {

>  #ifdef __SSE2__

> diff --git a/platform/linux-generic/include/odp/api/align.h

> b/platform/linux-generic/include/odp/api/align.h

> index d8bc653..2e7da14 100644

> --- a/platform/linux-generic/include/odp/api/align.h

> +++ b/platform/linux-generic/include/odp/api/align.h

> @@ -31,12 +31,6 @@ 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

> @@ -52,7 +46,7 @@ extern "C" {

>   */

>

>  #include <odp/api/spec/align.h>

> -#include <odp/api/cpu_arch.h>

> +#include <odp/api/cpu.h>

>

>  #ifdef __cplusplus

>  }

> diff --git a/platform/linux-generic/include/odp/api/cpu.h

> b/platform/linux-generic/include/odp/api/cpu.h

> index d49c782..246b932 100644

> --- a/platform/linux-generic/include/odp/api/cpu.h

> +++ b/platform/linux-generic/include/odp/api/cpu.h

> @@ -19,6 +19,23 @@ extern "C" {

>

>  #include <odp/api/cpu_arch.h>

>

> +/** @ingroup odp_compiler_optim

> + *  @{

> + */

> +

> +#if !defined(ODP_CACHE_LINE_SIZE)

> +/* Allow cache line size to be passed in via compiler define or

> elsewhere. */

> +#if defined(__OCTEON__) || defined(__powerpc64__)

> +#define ODP_CACHE_LINE_SIZE 128

> +#else

> +#define ODP_CACHE_LINE_SIZE 64

> +#endif

> +#endif

> +

> +/**

> + *  @}

> + */

> +

>  #include <odp/api/spec/cpu.h>

>

>  #ifdef __cplusplus

> --

> 2.9.0

>

>



-- 
Mike Holmes
Technical Manager - Linaro Networking Group
Linaro.org <http://www.linaro.org/> *│ *Open source software for ARM SoCs
"Work should be fun and collaborative, the rest follows"
Brian Brooks July 18, 2016, 10:17 p.m. UTC | #2 
On 07/18 13:54:25, Mike Holmes wrote:
> Is there a performance benifit to this ?

> 

> I would rather we have non conflicting per architecture files, we had all

> the defines in one file before and it caused issues when people sent

> patches that conflicted, I now have to review an x86 change even if I am on

> an ARM platform just incase something was broken.

> 

> At a personal level which is not a great reason for change I admit I hate

> nests of #defines and trying to decide which ones are being built in, I

> love that it is really clear form the compile log when they are separate

> 

> I would actually like to see all the per arch stuff pulled out period.  CPU

> architecture can be its own lib with the differences for ARM, X86 all

> gathered in there and reduce the amount of code in linux-generic, given the

> debian packaging this can be installed correctly once for all arm, x86 etc

> in a system with no need to include the code in your platform at all.

> 

> I see this architecture specific lib implementing say odp_crypto in the

> most SW efficient way possible, same for other architectures where this is

> a performance improved SW solution using arch specific instructions rather

> than HW accelerators.


The benefit is that something simple is defined in one file instead of five
files, and with less than half the amount of code.

This makes for more succinct, more portable, and less error prone code. There
actually isn't a lot of arch specific code (when merged) as this patch
demonstrates.

The define in align.h is a programming error because it is a duplicate, but
is legal because the values are the same.
diff mbox

Patch

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..1c79f87 100644
--- a/platform/linux-generic/arch/default/odp/api/cpu_arch.h
+++ b/platform/linux-generic/arch/default/odp/api/cpu_arch.h
@@ -11,16 +11,6 @@ 
 extern "C" {
 #endif
 
-/** @ingroup odp_compiler_optim
- *  @{
- */
-
-#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..3bfa0dc 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,6 @@ 
 extern "C" {
 #endif
 
-/** @ingroup odp_compiler_optim
- *  @{
- */
-
-#if defined __OCTEON__
-#define ODP_CACHE_LINE_SIZE 128
-#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..1c79f87 100644
--- a/platform/linux-generic/arch/powerpc/odp/api/cpu_arch.h
+++ b/platform/linux-generic/arch/powerpc/odp/api/cpu_arch.h
@@ -11,16 +11,6 @@ 
 extern "C" {
 #endif
 
-/** @ingroup odp_compiler_optim
- *  @{
- */
-
-#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..997a954 100644
--- a/platform/linux-generic/arch/x86/odp/api/cpu_arch.h
+++ b/platform/linux-generic/arch/x86/odp/api/cpu_arch.h
@@ -11,16 +11,6 @@ 
 extern "C" {
 #endif
 
-/** @ingroup odp_compiler_optim
- *  @{
- */
-
-#define ODP_CACHE_LINE_SIZE 64
-
-/**
- * @}
- */
-
 static inline void odp_cpu_pause(void)
 {
 #ifdef __SSE2__
diff --git a/platform/linux-generic/include/odp/api/align.h b/platform/linux-generic/include/odp/api/align.h
index d8bc653..2e7da14 100644
--- a/platform/linux-generic/include/odp/api/align.h
+++ b/platform/linux-generic/include/odp/api/align.h
@@ -31,12 +31,6 @@  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
@@ -52,7 +46,7 @@  extern "C" {
  */
 
 #include <odp/api/spec/align.h>
-#include <odp/api/cpu_arch.h>
+#include <odp/api/cpu.h>
 
 #ifdef __cplusplus
 }
diff --git a/platform/linux-generic/include/odp/api/cpu.h b/platform/linux-generic/include/odp/api/cpu.h
index d49c782..246b932 100644
--- a/platform/linux-generic/include/odp/api/cpu.h
+++ b/platform/linux-generic/include/odp/api/cpu.h
@@ -19,6 +19,23 @@  extern "C" {
 
 #include <odp/api/cpu_arch.h>
 
+/** @ingroup odp_compiler_optim
+ *  @{
+ */
+
+#if !defined(ODP_CACHE_LINE_SIZE)
+/* Allow cache line size to be passed in via compiler define or elsewhere. */
+#if defined(__OCTEON__) || defined(__powerpc64__)
+#define ODP_CACHE_LINE_SIZE 128
+#else
+#define ODP_CACHE_LINE_SIZE 64
+#endif
+#endif
+
+/**
+ *  @}
+ */
+
 #include <odp/api/spec/cpu.h>
 
 #ifdef __cplusplus