[RFC] linux-dpdk: add --enable-inlines configure option

Message ID 1461348713-28411-1-git-send-email-zoltan.kiss@linaro.org
State New
Headers show

Commit Message

Zoltan Kiss April 22, 2016, 6:11 p.m.
In order to have a fixed ABI, we need each function to be in the library
file. But that hurts performance a bit, as small accessor functions
couldn't be inlined. To have the ability to use both, this patch
creates a new configure option, which enables the use of inline
functions for users who want better performance.
The accessors are moved to a packet_inlines.h file, by default it is
included from odp_packet.c, and they appear as fully fledged functions.
If --enable-inlines=yes is added to ./configure, they won't be compiled
into the .so file, but the application has to add "#define INLINES" before
including odp.h

Signed-off-by: Zoltan Kiss <zoltan.kiss@linaro.org>
---
 configure.ac                                       |  9 +++
 platform/linux-dpdk/Makefile.am                    |  1 +
 platform/linux-dpdk/include/odp/api/packet.h       | 59 +--------------
 .../linux-dpdk/include/odp/api/packet_inlines.h    | 86 ++++++++++++++++++++++
 platform/linux-dpdk/odp_packet.c                   |  3 +
 5 files changed, 102 insertions(+), 56 deletions(-)
 create mode 100644 platform/linux-dpdk/include/odp/api/packet_inlines.h

Comments

Zoltan Kiss April 25, 2016, 1:50 p.m. | #1
Yes, we can have a more general name, but I tend towards using something 
along the lines of --enable-abi-breakage

On 25/04/16 12:59, Bill Fischofer wrote:
> --enable-inlines seems reasonable but is perhaps only one component in
> what's needed to enable abi mode operation.  So do we need/want an
> umbrella --enable-abi option that turns on all sub-options (whatever
> they may be) to achieve ABI mode?
>
> On Mon, Apr 25, 2016 at 6:39 AM, Zoltan Kiss <zoltan.kiss@linaro.org
> <mailto:zoltan.kiss@linaro.org>> wrote:
>
>     Originally I planned to have something like "-DINLINES=0/1", but
>     that would force the application to define this macro in any case.
>     Creating a config.h would be a way to overcome this maybe.
>
>
>
>
>     On 22/04/16 19:11, Zoltan Kiss wrote:
>
>
>         ##########################################################################
>         +# Enable/disable static inlines
>         +##########################################################################
>         +AC_ARG_ENABLE([inlines],
>         +    [  --enable-inlines          allow inlines],
>         +    [if test "x$enableval" = "xyes"; then
>         +        ODP_CFLAGS="$ODP_CFLAGS -DINLINES"
>         +    fi])
>         +
>
>
Mike Holmes April 25, 2016, 2:21 p.m. | #2
On 25 April 2016 at 09:50, Zoltan Kiss <zoltan.kiss@linaro.org> wrote:

> Yes, we can have a more general name, but I tend towards using something

> along the lines of --enable-abi-breakage



I dont like that, enable a negative?
It needs to describe the intent and not a side effect.

Assuming the default is performance - basically today's case, then we need
to just enable abi-compliance or visa versa is enable-performance
optimisations


>

>

> On 25/04/16 12:59, Bill Fischofer wrote:

>

>> --enable-inlines seems reasonable but is perhaps only one component in

>> what's needed to enable abi mode operation.  So do we need/want an

>> umbrella --enable-abi option that turns on all sub-options (whatever

>> they may be) to achieve ABI mode?

>>

>> On Mon, Apr 25, 2016 at 6:39 AM, Zoltan Kiss <zoltan.kiss@linaro.org

>> <mailto:zoltan.kiss@linaro.org>> wrote:

>>

>>     Originally I planned to have something like "-DINLINES=0/1", but

>>     that would force the application to define this macro in any case.

>>     Creating a config.h would be a way to overcome this maybe.

>>

>>

>>

>>

>>     On 22/04/16 19:11, Zoltan Kiss wrote:

>>

>>

>>

>> ##########################################################################

>>         +# Enable/disable static inlines

>>

>> +##########################################################################

>>         +AC_ARG_ENABLE([inlines],

>>         +    [  --enable-inlines          allow inlines],

>>         +    [if test "x$enableval" = "xyes"; then

>>         +        ODP_CFLAGS="$ODP_CFLAGS -DINLINES"

>>         +    fi])

>>         +

>>

>>

>>



-- 
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"
Zoltan Kiss April 26, 2016, 6:22 p.m. | #3
Another option we just discussed with Mike: how about we use the 
--enable-dynamic configure option instead of this? If people want 
dynamic libraries, they want ABI compatibility, if not, they don't. I 
think the two thing are quite directly related to each other.
In that case the question is how do you use that setting in your source 
files ...

On 22/04/16 19:11, Zoltan Kiss wrote:
> In order to have a fixed ABI, we need each function to be in the library
> file. But that hurts performance a bit, as small accessor functions
> couldn't be inlined. To have the ability to use both, this patch
> creates a new configure option, which enables the use of inline
> functions for users who want better performance.
> The accessors are moved to a packet_inlines.h file, by default it is
> included from odp_packet.c, and they appear as fully fledged functions.
> If --enable-inlines=yes is added to ./configure, they won't be compiled
> into the .so file, but the application has to add "#define INLINES" before
> including odp.h
>
> Signed-off-by: Zoltan Kiss <zoltan.kiss@linaro.org>
> ---
>   configure.ac                                       |  9 +++
>   platform/linux-dpdk/Makefile.am                    |  1 +
>   platform/linux-dpdk/include/odp/api/packet.h       | 59 +--------------
>   .../linux-dpdk/include/odp/api/packet_inlines.h    | 86 ++++++++++++++++++++++
>   platform/linux-dpdk/odp_packet.c                   |  3 +
>   5 files changed, 102 insertions(+), 56 deletions(-)
>   create mode 100644 platform/linux-dpdk/include/odp/api/packet_inlines.h
>
> diff --git a/configure.ac b/configure.ac
> index 99772a1..6b8fb76 100644
> --- a/configure.ac
> +++ b/configure.ac
> @@ -173,6 +173,15 @@ AC_ARG_ENABLE([debug],
>   ODP_CFLAGS="$ODP_CFLAGS -DODP_DEBUG=$ODP_DEBUG"
>
>   ##########################################################################
> +# Enable/disable static inlines
> +##########################################################################
> +AC_ARG_ENABLE([inlines],
> +    [  --enable-inlines          allow inlines],
> +    [if test "x$enableval" = "xyes"; then
> +        ODP_CFLAGS="$ODP_CFLAGS -DINLINES"
> +    fi])
> +
> +##########################################################################
>   # Save and set temporary compilation flags
>   ##########################################################################
>   OLD_LDFLAGS=$LDFLAGS
> diff --git a/platform/linux-dpdk/Makefile.am b/platform/linux-dpdk/Makefile.am
> index eadaf63..b8cba23 100644
> --- a/platform/linux-dpdk/Makefile.am
> +++ b/platform/linux-dpdk/Makefile.am
> @@ -49,6 +49,7 @@ odpapiinclude_HEADERS = \
>   		  $(srcdir)/include/odp/api/init.h \
>   		  $(srcdir)/include/odp/api/packet_flags.h \
>   		  $(srcdir)/include/odp/api/packet.h \
> +		  $(srcdir)/include/odp/api/packet_inlines.h \
>   		  $(srcdir)/include/odp/api/packet_io.h \
>   		  $(srcdir)/include/odp/api/pool.h \
>   		  $(srcdir)/include/odp/api/queue.h \
> diff --git a/platform/linux-dpdk/include/odp/api/packet.h b/platform/linux-dpdk/include/odp/api/packet.h
> index 7c0db32..e2ba78a 100644
> --- a/platform/linux-dpdk/include/odp/api/packet.h
> +++ b/platform/linux-dpdk/include/odp/api/packet.h
> @@ -27,62 +27,9 @@ extern "C" {
>   /** @ingroup odp_packet
>    *  @{
>    */
> -
> -extern const unsigned int buf_addr_offset;
> -extern const unsigned int data_off_offset;
> -extern const unsigned int pkt_len_offset;
> -extern const unsigned int seg_len_offset;
> -extern const unsigned int udata_len_offset;
> -extern const unsigned int udata_offset;
> -extern const unsigned int rss_offset;
> -extern const unsigned int ol_flags_offset;
> -extern const uint64_t rss_flag;
> -
> -/*
> - * NOTE: These functions are inlined because they are on a performance hot path.
> - * As we can't force the application to directly include DPDK headers we have to
> - * export these fields through constants calculated compile time in
> - * odp_packet.c, where we can see the DPDK definitions.
> - *
> - */
> -static inline uint32_t odp_packet_len(odp_packet_t pkt)
> -{
> -	return *(uint32_t *)(void *)((char *)pkt + pkt_len_offset);
> -}
> -
> -static inline uint32_t odp_packet_seg_len(odp_packet_t pkt)
> -{
> -	return *(uint16_t *)(void *)((char *)pkt + seg_len_offset);
> -}
> -
> -static inline void *odp_packet_user_area(odp_packet_t pkt)
> -{
> -	return (void *)((char *)pkt + udata_offset);
> -}
> -
> -static inline uint32_t odp_packet_user_area_size(odp_packet_t pkt)
> -{
> -	return *(uint32_t *)(void *)((char *)pkt + udata_len_offset);
> -}
> -
> -static inline void *odp_packet_data(odp_packet_t pkt)
> -{
> -	char** buf_addr = (char **)(void *)((char *)pkt + buf_addr_offset);
> -	uint16_t data_off = *(uint16_t *)(void *)((char *)pkt + data_off_offset);
> -	return (void *)(*buf_addr + data_off);
> -}
> -
> -static inline uint32_t odp_packet_flow_hash(odp_packet_t pkt)
> -{
> -	return *(uint32_t *)(void *)((char *)pkt + rss_offset);
> -}
> -
> -static inline void odp_packet_flow_hash_set(odp_packet_t pkt, uint32_t flow_hash)
> -{
> -	*(uint32_t *)(void *)((char *)pkt + rss_offset) = flow_hash;
> -	*(uint64_t *)(void *)((char *)pkt + ol_flags_offset) |= rss_flag;
> -}
> -
> +#ifdef INLINES
> +#include <odp/api/packet_inlines.h>
> +#endif
>   /**
>    * @}
>    */
> diff --git a/platform/linux-dpdk/include/odp/api/packet_inlines.h b/platform/linux-dpdk/include/odp/api/packet_inlines.h
> new file mode 100644
> index 0000000..3d3ca81
> --- /dev/null
> +++ b/platform/linux-dpdk/include/odp/api/packet_inlines.h
> @@ -0,0 +1,86 @@
> +/* Copyright (c) 2016, Linaro Limited
> + * All rights reserved.
> + *
> + * SPDX-License-Identifier: BSD-3-Clause
> + */
> +
> +/**
> + * @file
> + *
> + * ODP packet inline functions
> + */
> +
> +#ifndef ODP_PLAT_PACKET_INLINES_H_
> +#define ODP_PLAT_PACKET_INLINES_H_
> +
> +#ifdef __cplusplus
> +extern "C" {
> +#endif
> +
> +#ifdef INLINES
> +#define _STATIC static inline
> +
> +extern const unsigned int buf_addr_offset;
> +extern const unsigned int data_off_offset;
> +extern const unsigned int pkt_len_offset;
> +extern const unsigned int seg_len_offset;
> +extern const unsigned int udata_len_offset;
> +extern const unsigned int udata_offset;
> +extern const unsigned int rss_offset;
> +extern const unsigned int ol_flags_offset;
> +extern const uint64_t rss_flag;
> +
> +#else
> +#define _STATIC
> +#endif
> +
> +/*
> + * NOTE: These functions are inlined because they are on a performance hot path.
> + * As we can't force the application to directly include DPDK headers we have to
> + * export these fields through constants calculated compile time in
> + * odp_packet.c, where we can see the DPDK definitions.
> + *
> + */
> +_STATIC uint32_t odp_packet_len(odp_packet_t pkt)
> +{
> +	return *(uint32_t *)(void *)((char *)pkt + pkt_len_offset);
> +}
> +
> +_STATIC uint32_t odp_packet_seg_len(odp_packet_t pkt)
> +{
> +	return *(uint16_t *)(void *)((char *)pkt + seg_len_offset);
> +}
> +
> +_STATIC void *odp_packet_user_area(odp_packet_t pkt)
> +{
> +	return (void *)((char *)pkt + udata_offset);
> +}
> +
> +_STATIC uint32_t odp_packet_user_area_size(odp_packet_t pkt)
> +{
> +	return *(uint32_t *)(void *)((char *)pkt + udata_len_offset);
> +}
> +
> +_STATIC void *odp_packet_data(odp_packet_t pkt)
> +{
> +	char** buf_addr = (char **)(void *)((char *)pkt + buf_addr_offset);
> +	uint16_t data_off = *(uint16_t *)(void *)((char *)pkt + data_off_offset);
> +	return (void *)(*buf_addr + data_off);
> +}
> +
> +_STATIC uint32_t odp_packet_flow_hash(odp_packet_t pkt)
> +{
> +	return *(uint32_t *)(void *)((char *)pkt + rss_offset);
> +}
> +
> +_STATIC void odp_packet_flow_hash_set(odp_packet_t pkt, uint32_t flow_hash)
> +{
> +	*(uint32_t *)(void *)((char *)pkt + rss_offset) = flow_hash;
> +	*(uint64_t *)(void *)((char *)pkt + ol_flags_offset) |= rss_flag;
> +}
> +
> +#ifdef __cplusplus
> +}
> +#endif
> +
> +#endif /* ODP_PLAT_PACKET_INLINES_H_ */
> diff --git a/platform/linux-dpdk/odp_packet.c b/platform/linux-dpdk/odp_packet.c
> index e031b09..456ced6 100644
> --- a/platform/linux-dpdk/odp_packet.c
> +++ b/platform/linux-dpdk/odp_packet.c
> @@ -60,6 +60,9 @@ _ODP_STATIC_ASSERT(sizeof(dummy.hash.rss) == sizeof(uint32_t),
>   _ODP_STATIC_ASSERT(sizeof(dummy.ol_flags) == sizeof(uint64_t),
>   		   "ol_flags should be uint64_t");
>
> +#ifndef INLINES
> +#include <odp/api/packet_inlines.h>
> +#endif
>
>   odp_packet_t _odp_packet_from_buffer(odp_buffer_t buf)
>   {
>

Patch hide | download patch | download mbox

diff --git a/configure.ac b/configure.ac
index 99772a1..6b8fb76 100644
--- a/configure.ac
+++ b/configure.ac
@@ -173,6 +173,15 @@  AC_ARG_ENABLE([debug],
 ODP_CFLAGS="$ODP_CFLAGS -DODP_DEBUG=$ODP_DEBUG"
 
 ##########################################################################
+# Enable/disable static inlines
+##########################################################################
+AC_ARG_ENABLE([inlines],
+    [  --enable-inlines          allow inlines],
+    [if test "x$enableval" = "xyes"; then
+        ODP_CFLAGS="$ODP_CFLAGS -DINLINES"
+    fi])
+
+##########################################################################
 # Save and set temporary compilation flags
 ##########################################################################
 OLD_LDFLAGS=$LDFLAGS
diff --git a/platform/linux-dpdk/Makefile.am b/platform/linux-dpdk/Makefile.am
index eadaf63..b8cba23 100644
--- a/platform/linux-dpdk/Makefile.am
+++ b/platform/linux-dpdk/Makefile.am
@@ -49,6 +49,7 @@  odpapiinclude_HEADERS = \
 		  $(srcdir)/include/odp/api/init.h \
 		  $(srcdir)/include/odp/api/packet_flags.h \
 		  $(srcdir)/include/odp/api/packet.h \
+		  $(srcdir)/include/odp/api/packet_inlines.h \
 		  $(srcdir)/include/odp/api/packet_io.h \
 		  $(srcdir)/include/odp/api/pool.h \
 		  $(srcdir)/include/odp/api/queue.h \
diff --git a/platform/linux-dpdk/include/odp/api/packet.h b/platform/linux-dpdk/include/odp/api/packet.h
index 7c0db32..e2ba78a 100644
--- a/platform/linux-dpdk/include/odp/api/packet.h
+++ b/platform/linux-dpdk/include/odp/api/packet.h
@@ -27,62 +27,9 @@  extern "C" {
 /** @ingroup odp_packet
  *  @{
  */
-
-extern const unsigned int buf_addr_offset;
-extern const unsigned int data_off_offset;
-extern const unsigned int pkt_len_offset;
-extern const unsigned int seg_len_offset;
-extern const unsigned int udata_len_offset;
-extern const unsigned int udata_offset;
-extern const unsigned int rss_offset;
-extern const unsigned int ol_flags_offset;
-extern const uint64_t rss_flag;
-
-/*
- * NOTE: These functions are inlined because they are on a performance hot path.
- * As we can't force the application to directly include DPDK headers we have to
- * export these fields through constants calculated compile time in
- * odp_packet.c, where we can see the DPDK definitions.
- *
- */
-static inline uint32_t odp_packet_len(odp_packet_t pkt)
-{
-	return *(uint32_t *)(void *)((char *)pkt + pkt_len_offset);
-}
-
-static inline uint32_t odp_packet_seg_len(odp_packet_t pkt)
-{
-	return *(uint16_t *)(void *)((char *)pkt + seg_len_offset);
-}
-
-static inline void *odp_packet_user_area(odp_packet_t pkt)
-{
-	return (void *)((char *)pkt + udata_offset);
-}
-
-static inline uint32_t odp_packet_user_area_size(odp_packet_t pkt)
-{
-	return *(uint32_t *)(void *)((char *)pkt + udata_len_offset);
-}
-
-static inline void *odp_packet_data(odp_packet_t pkt)
-{
-	char** buf_addr = (char **)(void *)((char *)pkt + buf_addr_offset);
-	uint16_t data_off = *(uint16_t *)(void *)((char *)pkt + data_off_offset);
-	return (void *)(*buf_addr + data_off);
-}
-
-static inline uint32_t odp_packet_flow_hash(odp_packet_t pkt)
-{
-	return *(uint32_t *)(void *)((char *)pkt + rss_offset);
-}
-
-static inline void odp_packet_flow_hash_set(odp_packet_t pkt, uint32_t flow_hash)
-{
-	*(uint32_t *)(void *)((char *)pkt + rss_offset) = flow_hash;
-	*(uint64_t *)(void *)((char *)pkt + ol_flags_offset) |= rss_flag;
-}
-
+#ifdef INLINES
+#include <odp/api/packet_inlines.h>
+#endif
 /**
  * @}
  */
diff --git a/platform/linux-dpdk/include/odp/api/packet_inlines.h b/platform/linux-dpdk/include/odp/api/packet_inlines.h
new file mode 100644
index 0000000..3d3ca81
--- /dev/null
+++ b/platform/linux-dpdk/include/odp/api/packet_inlines.h
@@ -0,0 +1,86 @@ 
+/* Copyright (c) 2016, Linaro Limited
+ * All rights reserved.
+ *
+ * SPDX-License-Identifier: BSD-3-Clause
+ */
+
+/**
+ * @file
+ *
+ * ODP packet inline functions
+ */
+
+#ifndef ODP_PLAT_PACKET_INLINES_H_
+#define ODP_PLAT_PACKET_INLINES_H_
+
+#ifdef __cplusplus
+extern "C" {
+#endif
+
+#ifdef INLINES
+#define _STATIC static inline
+
+extern const unsigned int buf_addr_offset;
+extern const unsigned int data_off_offset;
+extern const unsigned int pkt_len_offset;
+extern const unsigned int seg_len_offset;
+extern const unsigned int udata_len_offset;
+extern const unsigned int udata_offset;
+extern const unsigned int rss_offset;
+extern const unsigned int ol_flags_offset;
+extern const uint64_t rss_flag;
+
+#else
+#define _STATIC
+#endif
+
+/*
+ * NOTE: These functions are inlined because they are on a performance hot path.
+ * As we can't force the application to directly include DPDK headers we have to
+ * export these fields through constants calculated compile time in
+ * odp_packet.c, where we can see the DPDK definitions.
+ *
+ */
+_STATIC uint32_t odp_packet_len(odp_packet_t pkt)
+{
+	return *(uint32_t *)(void *)((char *)pkt + pkt_len_offset);
+}
+
+_STATIC uint32_t odp_packet_seg_len(odp_packet_t pkt)
+{
+	return *(uint16_t *)(void *)((char *)pkt + seg_len_offset);
+}
+
+_STATIC void *odp_packet_user_area(odp_packet_t pkt)
+{
+	return (void *)((char *)pkt + udata_offset);
+}
+
+_STATIC uint32_t odp_packet_user_area_size(odp_packet_t pkt)
+{
+	return *(uint32_t *)(void *)((char *)pkt + udata_len_offset);
+}
+
+_STATIC void *odp_packet_data(odp_packet_t pkt)
+{
+	char** buf_addr = (char **)(void *)((char *)pkt + buf_addr_offset);
+	uint16_t data_off = *(uint16_t *)(void *)((char *)pkt + data_off_offset);
+	return (void *)(*buf_addr + data_off);
+}
+
+_STATIC uint32_t odp_packet_flow_hash(odp_packet_t pkt)
+{
+	return *(uint32_t *)(void *)((char *)pkt + rss_offset);
+}
+
+_STATIC void odp_packet_flow_hash_set(odp_packet_t pkt, uint32_t flow_hash)
+{
+	*(uint32_t *)(void *)((char *)pkt + rss_offset) = flow_hash;
+	*(uint64_t *)(void *)((char *)pkt + ol_flags_offset) |= rss_flag;
+}
+
+#ifdef __cplusplus
+}
+#endif
+
+#endif /* ODP_PLAT_PACKET_INLINES_H_ */
diff --git a/platform/linux-dpdk/odp_packet.c b/platform/linux-dpdk/odp_packet.c
index e031b09..456ced6 100644
--- a/platform/linux-dpdk/odp_packet.c
+++ b/platform/linux-dpdk/odp_packet.c
@@ -60,6 +60,9 @@  _ODP_STATIC_ASSERT(sizeof(dummy.hash.rss) == sizeof(uint32_t),
 _ODP_STATIC_ASSERT(sizeof(dummy.ol_flags) == sizeof(uint64_t),
 		   "ol_flags should be uint64_t");
 
+#ifndef INLINES
+#include <odp/api/packet_inlines.h>
+#endif
 
 odp_packet_t _odp_packet_from_buffer(odp_buffer_t buf)
 {