[RFC] linux-dpdk: Enable inlines if dynamic library is not used

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

Commit Message

Zoltan Kiss May 25, 2016, 2:13 p.m.
In order to have proper packaging and dynamic linking we need a fixed ABI.
For that, each function has to be in the library file. But that hurts
performance a bit, as small accessor functions couldn't be inlined,
despite it's a viable option with static linking.
This patch lets the platform automatically decide what should be done.
./configure generates inlines.h based on whether --enable-shared=yes was
added or not. If yes, it enables the INLINES macro.
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 INLINES defined, the accessors appear as static inline functions in the
header, and the application can directly inline them.

Signed-off-by: Zoltan Kiss <zoltan.kiss@linaro.org>
---
 .gitignore                                         |  1 +
 configure.ac                                       |  7 ++
 platform/linux-dpdk/Makefile.am                    |  2 +
 platform/linux-dpdk/include/odp/api/inlines.h.in   | 27 +++++++
 platform/linux-dpdk/include/odp/api/packet.h       | 60 +--------------
 .../linux-dpdk/include/odp/api/packet_inlines.h    | 87 ++++++++++++++++++++++
 platform/linux-dpdk/m4/configure.m4                |  3 +-
 platform/linux-dpdk/odp_packet.c                   |  3 +
 8 files changed, 133 insertions(+), 57 deletions(-)
 create mode 100644 platform/linux-dpdk/include/odp/api/inlines.h.in
 create mode 100644 platform/linux-dpdk/include/odp/api/packet_inlines.h

Comments

Bill Fischofer May 30, 2016, 1:46 a.m. | #1
On Wed, May 25, 2016 at 9:13 AM, Zoltan Kiss <zoltan.kiss@linaro.org> wrote:

> In order to have proper packaging and dynamic linking we need a fixed ABI.
> For that, each function has to be in the library file. But that hurts
> performance a bit, as small accessor functions couldn't be inlined,
> despite it's a viable option with static linking.
> This patch lets the platform automatically decide what should be done.
> ./configure generates inlines.h based on whether --enable-shared=yes was
> added or not. If yes, it enables the INLINES macro.
> 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 INLINES defined, the accessors appear as static inline functions in the
> header, and the application can directly inline them.
>
> Signed-off-by: Zoltan Kiss <zoltan.kiss@linaro.org>
> ---
>  .gitignore                                         |  1 +
>  configure.ac                                       |  7 ++
>  platform/linux-dpdk/Makefile.am                    |  2 +
>  platform/linux-dpdk/include/odp/api/inlines.h.in   | 27 +++++++
>  platform/linux-dpdk/include/odp/api/packet.h       | 60 +--------------
>  .../linux-dpdk/include/odp/api/packet_inlines.h    | 87
> ++++++++++++++++++++++
>  platform/linux-dpdk/m4/configure.m4                |  3 +-
>  platform/linux-dpdk/odp_packet.c                   |  3 +
>  8 files changed, 133 insertions(+), 57 deletions(-)
>  create mode 100644 platform/linux-dpdk/include/odp/api/inlines.h.in
>  create mode 100644 platform/linux-dpdk/include/odp/api/packet_inlines.h
>
> diff --git a/.gitignore b/.gitignore
> index 2b1da19..4040e2b 100644
> --- a/.gitignore
> +++ b/.gitignore
> @@ -40,5 +40,6 @@ ltmain.sh
>  m4/*.m4
>  missing
>  pkgconfig/libodp*.pc
> +platform/linux-dpdk/include/odp/api/inlines.h
>  tags
>  test-driver
> diff --git a/configure.ac b/configure.ac
> index 99772a1..199a6e2 100644
> --- a/configure.ac
> +++ b/configure.ac
> @@ -141,6 +141,13 @@ AM_CONDITIONAL([test_helper], [test x$test_helper =
> xyes ])
>  AM_CONDITIONAL([HAVE_DOXYGEN], [test "x${DOXYGEN}" = "xdoxygen"])
>  AM_CONDITIONAL([user_guide], [test "x${user_guides}" = "xyes" ])
>  AM_CONDITIONAL([HAVE_MSCGEN], [test "x${MSCGEN}" = "xmscgen"])
> +if test x$enable_shared != xyes;
> +then
> +    INLINES="INLINES"
> +else
> +    INLINES="__NOTHING"
> +fi
> +AC_SUBST(INLINES)
>
>  ##########################################################################
>  # Setup doxygen documentation
> diff --git a/platform/linux-dpdk/Makefile.am
> b/platform/linux-dpdk/Makefile.am
> index eadaf63..49ad877 100644
> --- a/platform/linux-dpdk/Makefile.am
> +++ b/platform/linux-dpdk/Makefile.am
> @@ -47,8 +47,10 @@ odpapiinclude_HEADERS = \
>                   $(srcdir)/include/odp/api/hash.h \
>                   $(srcdir)/include/odp/api/hints.h \
>                   $(srcdir)/include/odp/api/init.h \
> +                 $(srcdir)/include/odp/api/inlines.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/inlines.h.in
> b/platform/linux-dpdk/include/odp/api/inlines.h.in
> new file mode 100644
> index 0000000..1e8bac8
> --- /dev/null
> +++ b/platform/linux-dpdk/include/odp/api/inlines.h.in
> @@ -0,0 +1,27 @@
> +/* Copyright (c) 2016, Linaro Limited
> + * All rights reserved.
> + *
> + * SPDX-License-Identifier: BSD-3-Clause
> + */
> +
> +/**
> + * @file
> + *
> + * ODP packet inline functions
> + */
> +
> +#ifndef ODP_PLAT_INLINES_H_
> +#define ODP_PLAT_INLINES_H_
> +
> +#ifdef __cplusplus
> +extern "C" {
> +#endif
> +
> +#define @INLINES@
>

Why the @ symbols here? Also, as an ODP configuration option, shouldn't
this be something like ODP_INLINES to avoid potential name space issues?


> +
> +#ifdef __cplusplus
> +}
> +#endif
> +
> +
> +#endif /* ODP_PLAT_INLINES_H_ */
> diff --git a/platform/linux-dpdk/include/odp/api/packet.h
> b/platform/linux-dpdk/include/odp/api/packet.h
> index 7c0db32..0fb31e3 100644
> --- a/platform/linux-dpdk/include/odp/api/packet.h
> +++ b/platform/linux-dpdk/include/odp/api/packet.h
> @@ -27,62 +27,10 @@ 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;
> -}
> -
> +#include <odp/api/inlines.h>
> +#ifdef INLINES
>

ODP_INLINES ? (see earlier comment)


> +#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..569090b
> --- /dev/null
> +++ b/platform/linux-dpdk/include/odp/api/packet_inlines.h
> @@ -0,0 +1,87 @@
> +/* 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/m4/configure.m4
> b/platform/linux-dpdk/m4/configure.m4
> index fab0f6a..7c5ccbc 100644
> --- a/platform/linux-dpdk/m4/configure.m4
> +++ b/platform/linux-dpdk/m4/configure.m4
> @@ -47,4 +47,5 @@ m4_include([platform/linux-dpdk/m4/odp_openssl.m4])
>
>  AC_CONFIG_FILES([platform/linux-dpdk/Makefile
>                  platform/linux-dpdk/test/Makefile
> -                platform/linux-dpdk/test/pktio/Makefile])
> +                platform/linux-dpdk/test/pktio/Makefile
> +                platform/linux-dpdk/include/odp/api/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
>

Same comment as above.


> +#include <odp/api/packet_inlines.h>
> +#endif
>
>  odp_packet_t _odp_packet_from_buffer(odp_buffer_t buf)
>  {
> --
> 1.9.1
>
> _______________________________________________
> lng-odp-dpdk mailing list
> lng-odp-dpdk@lists.linaro.org
> https://lists.linaro.org/mailman/listinfo/lng-odp-dpdk
>
Zoltan Kiss June 7, 2016, 4:37 p.m. | #2
I've merged this patch as it was required for 1.10 to work on ODP-DPDK:

https://git.linaro.org/lng/odp-dpdk.git/commitdiff/968237b592a8162041a4edf0091575fd1390d647

The changes I made:
- packet_flags also handled
- now it called _ODP_INLINES

On 30/05/16 11:03, Zoltan Kiss wrote:
> Hi,
>
> On 30/05/16 02:46, Bill Fischofer wrote:
>>
>>     diff --git a/configure.ac <http://configure.ac> b/configure.ac
>>     <http://configure.ac>
>>     index 99772a1..199a6e2 100644
>>     --- a/configure.ac <http://configure.ac>
>>     +++ b/configure.ac <http://configure.ac>
>>     @@ -141,6 +141,13 @@ AM_CONDITIONAL([test_helper], [test
>>     x$test_helper = xyes ])
>>      AM_CONDITIONAL([HAVE_DOXYGEN], [test "x${DOXYGEN}" = "xdoxygen"])
>>      AM_CONDITIONAL([user_guide], [test "x${user_guides}" = "xyes" ])
>>      AM_CONDITIONAL([HAVE_MSCGEN], [test "x${MSCGEN}" = "xmscgen"])
>>     +if test x$enable_shared != xyes;
>>     +then
>>     +    INLINES="INLINES"
>>     +else
>>     +    INLINES="__NOTHING"
>>     +fi
>>     +AC_SUBST(INLINES)
>>
>>      ##########################################################################
>>      # Setup doxygen documentation
>>     diff --git a/platform/linux-dpdk/Makefile.am
>>     b/platform/linux-dpdk/Makefile.am
>>     index eadaf63..49ad877 100644
>>     --- a/platform/linux-dpdk/Makefile.am
>>     +++ b/platform/linux-dpdk/Makefile.am
>>     @@ -47,8 +47,10 @@ odpapiinclude_HEADERS = \
>>                       $(srcdir)/include/odp/api/hash.h \
>>                       $(srcdir)/include/odp/api/hints.h \
>>                       $(srcdir)/include/odp/api/init.h \
>>     +                 $(srcdir)/include/odp/api/inlines.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/inlines.h.in
>>     <http://inlines.h.in>
>>     b/platform/linux-dpdk/include/odp/api/inlines.h.in
>>     <http://inlines.h.in>
>>     new file mode 100644
>>     index 0000000..1e8bac8
>>     --- /dev/null
>>     +++ b/platform/linux-dpdk/include/odp/api/inlines.h.in
>>     <http://inlines.h.in>
>>     @@ -0,0 +1,27 @@
>>     +/* Copyright (c) 2016, Linaro Limited
>>     + * All rights reserved.
>>     + *
>>     + * SPDX-License-Identifier: BSD-3-Clause
>>     + */
>>     +
>>     +/**
>>     + * @file
>>     + *
>>     + * ODP packet inline functions
>>     + */
>>
>
> I just noticed, this comment is also wrong.
>
>>     +
>>     +#ifndef ODP_PLAT_INLINES_H_
>>     +#define ODP_PLAT_INLINES_H_
>>     +
>>     +#ifdef __cplusplus
>>     +extern "C" {
>>     +#endif
>>     +
>>     +#define @INLINES@
>>
>>
>> Why the @ symbols here? Also, as an ODP configuration option,
>> shouldn't this be something like ODP_INLINES to avoid potential name
>> space issues?
>
> AC_SUBST(INLINES) should replace @INLINES@ to the value determined in
> the configure script. That's why platform/linux-dpdk/m4/configure.m4
> includes this header file as well.
> Yes, we can prepend it with "ODP_", but I would add an underscore to the
> beginning, because this supposed to be an internal configuration
> variable, applications are not supposed to use it directly. But they
> include the generated inlines.h, so when they get to packet_inlines.h,
> they'll know whether they should inline the functions or not.
>
> Regards,
>
> Zoli
Maxim Uvarov June 8, 2016, 6:42 p.m. | #3
patch looks good for me.

One small note that you probably can remove ifdefs while including this:

+#ifndef INLINES
+#include <odp/api/packet_inlines.h>
+#endif


Because you already have ifdef inside packet_inlines.h

Maxim.


On 06/07/16 19:37, Zoltan Kiss wrote:
> I've merged this patch as it was required for 1.10 to work on ODP-DPDK:
>
> https://git.linaro.org/lng/odp-dpdk.git/commitdiff/968237b592a8162041a4edf0091575fd1390d647 
>
>
> The changes I made:
> - packet_flags also handled
> - now it called _ODP_INLINES
>
> On 30/05/16 11:03, Zoltan Kiss wrote:
>> Hi,
>>
>> On 30/05/16 02:46, Bill Fischofer wrote:
>>>
>>>     diff --git a/configure.ac <http://configure.ac> b/configure.ac
>>>     <http://configure.ac>
>>>     index 99772a1..199a6e2 100644
>>>     --- a/configure.ac <http://configure.ac>
>>>     +++ b/configure.ac <http://configure.ac>
>>>     @@ -141,6 +141,13 @@ AM_CONDITIONAL([test_helper], [test
>>>     x$test_helper = xyes ])
>>>      AM_CONDITIONAL([HAVE_DOXYGEN], [test "x${DOXYGEN}" = "xdoxygen"])
>>>      AM_CONDITIONAL([user_guide], [test "x${user_guides}" = "xyes" ])
>>>      AM_CONDITIONAL([HAVE_MSCGEN], [test "x${MSCGEN}" = "xmscgen"])
>>>     +if test x$enable_shared != xyes;
>>>     +then
>>>     +    INLINES="INLINES"
>>>     +else
>>>     +    INLINES="__NOTHING"
>>>     +fi
>>>     +AC_SUBST(INLINES)
>>>
>>> ##########################################################################
>>>      # Setup doxygen documentation
>>>     diff --git a/platform/linux-dpdk/Makefile.am
>>>     b/platform/linux-dpdk/Makefile.am
>>>     index eadaf63..49ad877 100644
>>>     --- a/platform/linux-dpdk/Makefile.am
>>>     +++ b/platform/linux-dpdk/Makefile.am
>>>     @@ -47,8 +47,10 @@ odpapiinclude_HEADERS = \
>>>                       $(srcdir)/include/odp/api/hash.h \
>>>                       $(srcdir)/include/odp/api/hints.h \
>>>                       $(srcdir)/include/odp/api/init.h \
>>>     +                 $(srcdir)/include/odp/api/inlines.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/inlines.h.in
>>>     <http://inlines.h.in>
>>>     b/platform/linux-dpdk/include/odp/api/inlines.h.in
>>>     <http://inlines.h.in>
>>>     new file mode 100644
>>>     index 0000000..1e8bac8
>>>     --- /dev/null
>>>     +++ b/platform/linux-dpdk/include/odp/api/inlines.h.in
>>>     <http://inlines.h.in>
>>>     @@ -0,0 +1,27 @@
>>>     +/* Copyright (c) 2016, Linaro Limited
>>>     + * All rights reserved.
>>>     + *
>>>     + * SPDX-License-Identifier: BSD-3-Clause
>>>     + */
>>>     +
>>>     +/**
>>>     + * @file
>>>     + *
>>>     + * ODP packet inline functions
>>>     + */
>>>
>>
>> I just noticed, this comment is also wrong.
>>
>>>     +
>>>     +#ifndef ODP_PLAT_INLINES_H_
>>>     +#define ODP_PLAT_INLINES_H_
>>>     +
>>>     +#ifdef __cplusplus
>>>     +extern "C" {
>>>     +#endif
>>>     +
>>>     +#define @INLINES@
>>>
>>>
>>> Why the @ symbols here? Also, as an ODP configuration option,
>>> shouldn't this be something like ODP_INLINES to avoid potential name
>>> space issues?
>>
>> AC_SUBST(INLINES) should replace @INLINES@ to the value determined in
>> the configure script. That's why platform/linux-dpdk/m4/configure.m4
>> includes this header file as well.
>> Yes, we can prepend it with "ODP_", but I would add an underscore to the
>> beginning, because this supposed to be an internal configuration
>> variable, applications are not supposed to use it directly. But they
>> include the generated inlines.h, so when they get to packet_inlines.h,
>> they'll know whether they should inline the functions or not.
>>
>> Regards,
>>
>> Zoli
> _______________________________________________
> lng-odp-dpdk mailing list
> lng-odp-dpdk@lists.linaro.org
> https://lists.linaro.org/mailman/listinfo/lng-odp-dpdk
Zoltan Kiss June 9, 2016, 4:39 p.m. | #4
Actually you can't do that: from packet.h you has to include 
packet_inlines.h if inlines enabled, so they appear for the application 
as inlines (_STATIC changes its meaning as well). If you don't have 
inlines, you include it from packet.c

Zoli

On 08/06/16 19:42, Maxim Uvarov wrote:
> patch looks good for me.
>
> One small note that you probably can remove ifdefs while including this:
>
> +#ifndef INLINES
> +#include <odp/api/packet_inlines.h>
> +#endif
>
>
> Because you already have ifdef inside packet_inlines.h
>
> Maxim.
>
>
> On 06/07/16 19:37, Zoltan Kiss wrote:
>> I've merged this patch as it was required for 1.10 to work on ODP-DPDK:
>>
>> https://git.linaro.org/lng/odp-dpdk.git/commitdiff/968237b592a8162041a4edf0091575fd1390d647
>>
>>
>> The changes I made:
>> - packet_flags also handled
>> - now it called _ODP_INLINES
>>
>> On 30/05/16 11:03, Zoltan Kiss wrote:
>>> Hi,
>>>
>>> On 30/05/16 02:46, Bill Fischofer wrote:
>>>>
>>>>     diff --git a/configure.ac <http://configure.ac> b/configure.ac
>>>>     <http://configure.ac>
>>>>     index 99772a1..199a6e2 100644
>>>>     --- a/configure.ac <http://configure.ac>
>>>>     +++ b/configure.ac <http://configure.ac>
>>>>     @@ -141,6 +141,13 @@ AM_CONDITIONAL([test_helper], [test
>>>>     x$test_helper = xyes ])
>>>>      AM_CONDITIONAL([HAVE_DOXYGEN], [test "x${DOXYGEN}" = "xdoxygen"])
>>>>      AM_CONDITIONAL([user_guide], [test "x${user_guides}" = "xyes" ])
>>>>      AM_CONDITIONAL([HAVE_MSCGEN], [test "x${MSCGEN}" = "xmscgen"])
>>>>     +if test x$enable_shared != xyes;
>>>>     +then
>>>>     +    INLINES="INLINES"
>>>>     +else
>>>>     +    INLINES="__NOTHING"
>>>>     +fi
>>>>     +AC_SUBST(INLINES)
>>>>
>>>> ##########################################################################
>>>>
>>>>      # Setup doxygen documentation
>>>>     diff --git a/platform/linux-dpdk/Makefile.am
>>>>     b/platform/linux-dpdk/Makefile.am
>>>>     index eadaf63..49ad877 100644
>>>>     --- a/platform/linux-dpdk/Makefile.am
>>>>     +++ b/platform/linux-dpdk/Makefile.am
>>>>     @@ -47,8 +47,10 @@ odpapiinclude_HEADERS = \
>>>>                       $(srcdir)/include/odp/api/hash.h \
>>>>                       $(srcdir)/include/odp/api/hints.h \
>>>>                       $(srcdir)/include/odp/api/init.h \
>>>>     +                 $(srcdir)/include/odp/api/inlines.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/inlines.h.in
>>>>     <http://inlines.h.in>
>>>>     b/platform/linux-dpdk/include/odp/api/inlines.h.in
>>>>     <http://inlines.h.in>
>>>>     new file mode 100644
>>>>     index 0000000..1e8bac8
>>>>     --- /dev/null
>>>>     +++ b/platform/linux-dpdk/include/odp/api/inlines.h.in
>>>>     <http://inlines.h.in>
>>>>     @@ -0,0 +1,27 @@
>>>>     +/* Copyright (c) 2016, Linaro Limited
>>>>     + * All rights reserved.
>>>>     + *
>>>>     + * SPDX-License-Identifier: BSD-3-Clause
>>>>     + */
>>>>     +
>>>>     +/**
>>>>     + * @file
>>>>     + *
>>>>     + * ODP packet inline functions
>>>>     + */
>>>>
>>>
>>> I just noticed, this comment is also wrong.
>>>
>>>>     +
>>>>     +#ifndef ODP_PLAT_INLINES_H_
>>>>     +#define ODP_PLAT_INLINES_H_
>>>>     +
>>>>     +#ifdef __cplusplus
>>>>     +extern "C" {
>>>>     +#endif
>>>>     +
>>>>     +#define @INLINES@
>>>>
>>>>
>>>> Why the @ symbols here? Also, as an ODP configuration option,
>>>> shouldn't this be something like ODP_INLINES to avoid potential name
>>>> space issues?
>>>
>>> AC_SUBST(INLINES) should replace @INLINES@ to the value determined in
>>> the configure script. That's why platform/linux-dpdk/m4/configure.m4
>>> includes this header file as well.
>>> Yes, we can prepend it with "ODP_", but I would add an underscore to the
>>> beginning, because this supposed to be an internal configuration
>>> variable, applications are not supposed to use it directly. But they
>>> include the generated inlines.h, so when they get to packet_inlines.h,
>>> they'll know whether they should inline the functions or not.
>>>
>>> Regards,
>>>
>>> Zoli
>> _______________________________________________
>> lng-odp-dpdk mailing list
>> lng-odp-dpdk@lists.linaro.org
>> https://lists.linaro.org/mailman/listinfo/lng-odp-dpdk
>
> _______________________________________________
> lng-odp-dpdk mailing list
> lng-odp-dpdk@lists.linaro.org
> https://lists.linaro.org/mailman/listinfo/lng-odp-dpdk

Patch hide | download patch | download mbox

diff --git a/.gitignore b/.gitignore
index 2b1da19..4040e2b 100644
--- a/.gitignore
+++ b/.gitignore
@@ -40,5 +40,6 @@  ltmain.sh
 m4/*.m4
 missing
 pkgconfig/libodp*.pc
+platform/linux-dpdk/include/odp/api/inlines.h
 tags
 test-driver
diff --git a/configure.ac b/configure.ac
index 99772a1..199a6e2 100644
--- a/configure.ac
+++ b/configure.ac
@@ -141,6 +141,13 @@  AM_CONDITIONAL([test_helper], [test x$test_helper = xyes ])
 AM_CONDITIONAL([HAVE_DOXYGEN], [test "x${DOXYGEN}" = "xdoxygen"])
 AM_CONDITIONAL([user_guide], [test "x${user_guides}" = "xyes" ])
 AM_CONDITIONAL([HAVE_MSCGEN], [test "x${MSCGEN}" = "xmscgen"])
+if test x$enable_shared != xyes;
+then
+    INLINES="INLINES"
+else
+    INLINES="__NOTHING"
+fi
+AC_SUBST(INLINES)
 
 ##########################################################################
 # Setup doxygen documentation
diff --git a/platform/linux-dpdk/Makefile.am b/platform/linux-dpdk/Makefile.am
index eadaf63..49ad877 100644
--- a/platform/linux-dpdk/Makefile.am
+++ b/platform/linux-dpdk/Makefile.am
@@ -47,8 +47,10 @@  odpapiinclude_HEADERS = \
 		  $(srcdir)/include/odp/api/hash.h \
 		  $(srcdir)/include/odp/api/hints.h \
 		  $(srcdir)/include/odp/api/init.h \
+		  $(srcdir)/include/odp/api/inlines.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/inlines.h.in b/platform/linux-dpdk/include/odp/api/inlines.h.in
new file mode 100644
index 0000000..1e8bac8
--- /dev/null
+++ b/platform/linux-dpdk/include/odp/api/inlines.h.in
@@ -0,0 +1,27 @@ 
+/* Copyright (c) 2016, Linaro Limited
+ * All rights reserved.
+ *
+ * SPDX-License-Identifier: BSD-3-Clause
+ */
+
+/**
+ * @file
+ *
+ * ODP packet inline functions
+ */
+
+#ifndef ODP_PLAT_INLINES_H_
+#define ODP_PLAT_INLINES_H_
+
+#ifdef __cplusplus
+extern "C" {
+#endif
+
+#define @INLINES@
+
+#ifdef __cplusplus
+}
+#endif
+
+
+#endif /* ODP_PLAT_INLINES_H_ */
diff --git a/platform/linux-dpdk/include/odp/api/packet.h b/platform/linux-dpdk/include/odp/api/packet.h
index 7c0db32..0fb31e3 100644
--- a/platform/linux-dpdk/include/odp/api/packet.h
+++ b/platform/linux-dpdk/include/odp/api/packet.h
@@ -27,62 +27,10 @@  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;
-}
-
+#include <odp/api/inlines.h>
+#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..569090b
--- /dev/null
+++ b/platform/linux-dpdk/include/odp/api/packet_inlines.h
@@ -0,0 +1,87 @@ 
+/* 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/m4/configure.m4 b/platform/linux-dpdk/m4/configure.m4
index fab0f6a..7c5ccbc 100644
--- a/platform/linux-dpdk/m4/configure.m4
+++ b/platform/linux-dpdk/m4/configure.m4
@@ -47,4 +47,5 @@  m4_include([platform/linux-dpdk/m4/odp_openssl.m4])
 
 AC_CONFIG_FILES([platform/linux-dpdk/Makefile
 		 platform/linux-dpdk/test/Makefile
-		 platform/linux-dpdk/test/pktio/Makefile])
+		 platform/linux-dpdk/test/pktio/Makefile
+		 platform/linux-dpdk/include/odp/api/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)
 {