[v2,0/6] lib/ring: templates to support custom element size

Message ID 20190906190510.11146-1-honnappa.nagarahalli@arm.com
Headers show
Series
  • lib/ring: templates to support custom element size
Related show

Message

Honnappa Nagarahalli Sept. 6, 2019, 7:05 p.m.
The current rte_ring hard-codes the type of the ring element to 'void *',
hence the size of the element is hard-coded to 32b/64b. Since the ring
element type is not an input to rte_ring APIs, it results in couple
of issues:

1) If an application requires to store an element which is not 64b, it
   needs to write its own ring APIs similar to rte_event_ring APIs. This
   creates additional burden on the programmers, who end up making
   work-arounds and often waste memory.
2) If there are multiple libraries that store elements of the same
   type, currently they would have to write their own rte_ring APIs. This
   results in code duplication.

This patch consists of several parts:
1) New APIs to support configurable ring element size
   These will help reduce code duplication in the templates. I think these
   can be made internal (do not expose to DPDK applications, but expose to
   DPDK libraries), feedback needed.

2) rte_ring templates
   The templates provide an easy way to add new APIs for different ring
   element types/sizes which can be used by multiple libraries. These
   also allow for creating APIs to store elements of custom types
   (for ex: a structure)

   The template needs 4 parameters:
   a) RTE_RING_TMPLT_API_SUFFIX - This is used as a suffix to the
      rte_ring APIs.
      For ex: if RTE_RING_TMPLT_API_SUFFIX is '32b', the API name will be
      rte_ring_create_32b
   b) RTE_RING_TMPLT_ELEM_SIZE - Size of the ring element in bytes.
      For ex: sizeof(uint32_t)
   c) RTE_RING_TMPLT_ELEM_TYPE - Type of the ring element.
      For ex: uint32_t. If a common ring library does not use a standard
      data type, it should create its own type by defining a structure
      with standard data type. For ex: for an elment size of 96b, one
      could define a structure

      struct s_96b {
          uint32_t a[3];
      }
      The common library can use this structure to define
      RTE_RING_TMPLT_ELEM_TYPE.

      The application using this common ring library should define its
      element type as a union with the above structure.

      union app_element_type {
          struct s_96b v;
          struct app_element {
              uint16_t a;
              uint16_t b;
              uint32_t c;
              uint32_t d;
          }
      }
   d) RTE_RING_TMPLT_EXPERIMENTAL - Indicates if the new APIs being defined
      are experimental. Should be set to empty to remove the experimental
      tag.

   The ring library consists of some APIs that are defined as inline
   functions and some APIs that are non-inline functions. The non-inline
   functions are in rte_ring_template.c. However, this file needs to be
   included in other .c files. Any feedback on how to handle this is
   appreciated.

   Note that the templates help create the APIs that are dependent on the
   element size (for ex: rte_ring_create, enqueue/dequeue etc). Other APIs
   that do NOT depend on the element size do not need to be part of the
   template (for ex: rte_ring_dump, rte_ring_count, rte_ring_free_count
   etc).

3) APIs for 32b ring element size
   This uses the templates to create APIs to enqueue/dequeue elements of
   size 32b.

4) rte_hash libray is changed to use 32b ring APIs
   The 32b APIs are used in rte_hash library to store the free slot index
   and free bucket index.

5) Event Dev changed to use ring templates
   Event Dev defines its own 128b ring APIs using the templates. This helps
   in keeping the 'struct rte_event' as is. If Event Dev has to use generic
   128b ring APIs, it requires 'struct rte_event' to change to
   'union rte_event' to include a generic data type such as '__int128_t'.
   This breaks the API compatibility and results in large number of
   changes.
   With this change, the event rings are stored on rte_ring's tailq.
   Event Dev specific ring list is NOT available. IMO, this does not have
   any impact to the user.

This patch results in following checkpatch issue:
WARNING:UNSPECIFIED_INT: Prefer 'unsigned int' to bare use of 'unsigned'

However, this patch is following the rules in the existing code. Please
let me know if this needs to be fixed.

v2
 - Change Event Ring implementation to use ring templates
   (Jerin, Pavan)

Honnappa Nagarahalli (6):
  lib/ring: apis to support configurable element size
  lib/ring: add template to support different element sizes
  tools/checkpatch: relax constraints on __rte_experimental
  lib/ring: add ring APIs to support 32b ring elements
  lib/hash: use ring with 32b element size to save memory
  lib/eventdev: use ring templates for event rings

 devtools/checkpatches.sh                  |  11 +-
 lib/librte_eventdev/Makefile              |   2 +
 lib/librte_eventdev/meson.build           |   2 +
 lib/librte_eventdev/rte_event_ring.c      | 146 +---------
 lib/librte_eventdev/rte_event_ring.h      |  41 +--
 lib/librte_eventdev/rte_event_ring_128b.c |  19 ++
 lib/librte_eventdev/rte_event_ring_128b.h |  44 +++
 lib/librte_hash/rte_cuckoo_hash.c         |  55 ++--
 lib/librte_hash/rte_cuckoo_hash.h         |   2 +-
 lib/librte_ring/Makefile                  |   9 +-
 lib/librte_ring/meson.build               |  11 +-
 lib/librte_ring/rte_ring.c                |  34 ++-
 lib/librte_ring/rte_ring.h                |  72 +++++
 lib/librte_ring/rte_ring_32.c             |  19 ++
 lib/librte_ring/rte_ring_32.h             |  36 +++
 lib/librte_ring/rte_ring_template.c       |  46 +++
 lib/librte_ring/rte_ring_template.h       | 330 ++++++++++++++++++++++
 lib/librte_ring/rte_ring_version.map      |   4 +
 18 files changed, 660 insertions(+), 223 deletions(-)
 create mode 100644 lib/librte_eventdev/rte_event_ring_128b.c
 create mode 100644 lib/librte_eventdev/rte_event_ring_128b.h
 create mode 100644 lib/librte_ring/rte_ring_32.c
 create mode 100644 lib/librte_ring/rte_ring_32.h
 create mode 100644 lib/librte_ring/rte_ring_template.c
 create mode 100644 lib/librte_ring/rte_ring_template.h

-- 
2.17.1

Comments

Aaron Conole Sept. 9, 2019, 1:04 p.m. | #1
Honnappa Nagarahalli <honnappa.nagarahalli@arm.com> writes:

> The current rte_ring hard-codes the type of the ring element to 'void *',

> hence the size of the element is hard-coded to 32b/64b. Since the ring

> element type is not an input to rte_ring APIs, it results in couple

> of issues:

>

> 1) If an application requires to store an element which is not 64b, it

>    needs to write its own ring APIs similar to rte_event_ring APIs. This

>    creates additional burden on the programmers, who end up making

>    work-arounds and often waste memory.

> 2) If there are multiple libraries that store elements of the same

>    type, currently they would have to write their own rte_ring APIs. This

>    results in code duplication.

>

> This patch consists of several parts:

> 1) New APIs to support configurable ring element size

>    These will help reduce code duplication in the templates. I think these

>    can be made internal (do not expose to DPDK applications, but expose to

>    DPDK libraries), feedback needed.

>

> 2) rte_ring templates

>    The templates provide an easy way to add new APIs for different ring

>    element types/sizes which can be used by multiple libraries. These

>    also allow for creating APIs to store elements of custom types

>    (for ex: a structure)

>

>    The template needs 4 parameters:

>    a) RTE_RING_TMPLT_API_SUFFIX - This is used as a suffix to the

>       rte_ring APIs.

>       For ex: if RTE_RING_TMPLT_API_SUFFIX is '32b', the API name will be

>       rte_ring_create_32b

>    b) RTE_RING_TMPLT_ELEM_SIZE - Size of the ring element in bytes.

>       For ex: sizeof(uint32_t)

>    c) RTE_RING_TMPLT_ELEM_TYPE - Type of the ring element.

>       For ex: uint32_t. If a common ring library does not use a standard

>       data type, it should create its own type by defining a structure

>       with standard data type. For ex: for an elment size of 96b, one

>       could define a structure

>

>       struct s_96b {

>           uint32_t a[3];

>       }

>       The common library can use this structure to define

>       RTE_RING_TMPLT_ELEM_TYPE.

>

>       The application using this common ring library should define its

>       element type as a union with the above structure.

>

>       union app_element_type {

>           struct s_96b v;

>           struct app_element {

>               uint16_t a;

>               uint16_t b;

>               uint32_t c;

>               uint32_t d;

>           }

>       }

>    d) RTE_RING_TMPLT_EXPERIMENTAL - Indicates if the new APIs being defined

>       are experimental. Should be set to empty to remove the experimental

>       tag.

>

>    The ring library consists of some APIs that are defined as inline

>    functions and some APIs that are non-inline functions. The non-inline

>    functions are in rte_ring_template.c. However, this file needs to be

>    included in other .c files. Any feedback on how to handle this is

>    appreciated.

>

>    Note that the templates help create the APIs that are dependent on the

>    element size (for ex: rte_ring_create, enqueue/dequeue etc). Other APIs

>    that do NOT depend on the element size do not need to be part of the

>    template (for ex: rte_ring_dump, rte_ring_count, rte_ring_free_count

>    etc).

>

> 3) APIs for 32b ring element size

>    This uses the templates to create APIs to enqueue/dequeue elements of

>    size 32b.

>

> 4) rte_hash libray is changed to use 32b ring APIs

>    The 32b APIs are used in rte_hash library to store the free slot index

>    and free bucket index.

>

> 5) Event Dev changed to use ring templates

>    Event Dev defines its own 128b ring APIs using the templates. This helps

>    in keeping the 'struct rte_event' as is. If Event Dev has to use generic

>    128b ring APIs, it requires 'struct rte_event' to change to

>    'union rte_event' to include a generic data type such as '__int128_t'.

>    This breaks the API compatibility and results in large number of

>    changes.

>    With this change, the event rings are stored on rte_ring's tailq.

>    Event Dev specific ring list is NOT available. IMO, this does not have

>    any impact to the user.

>

> This patch results in following checkpatch issue:

> WARNING:UNSPECIFIED_INT: Prefer 'unsigned int' to bare use of 'unsigned'

>

> However, this patch is following the rules in the existing code. Please

> let me know if this needs to be fixed.

>

> v2

>  - Change Event Ring implementation to use ring templates

>    (Jerin, Pavan)


Since you'll likely be spinning a v3 (to switch off the macroization),
this series seems to have some unit test failures:

   24/82 DPDK:fast-tests / event_ring_autotest   FAIL     0.12 s (exit status 255 or signal 127 SIGinvalid)
   --- command ---
   DPDK_TEST='event_ring_autotest' /home/travis/build/ovsrobot/dpdk/build/app/test/dpdk-test -l 0-1 --file-prefix=event_ring_autotest
   --- stdout ---
   EAL: Probing VFIO support...
   APP: HPET is not enabled, using TSC as default timer
   RTE>>event_ring_autotest
   RING: Requested number of elements is invalid, must be power of 2, and do not exceed the limit 2147483647
   Test detected odd count
   Test detected NULL ring lookup
   RING: Requested number of elements is invalid, must be power of 2, and do not exceed the limit 2147483647
   RING: Requested number of elements is invalid, must be power of 2, and do not exceed the limit 2147483647
   Error, status after enqueue is unexpected
   Test Failed
   RTE>>
   --- stderr ---
   EAL: Detected 2 lcore(s)
   EAL: Detected 1 NUMA nodes
   EAL: Multi-process socket /var/run/dpdk/event_ring_autotest/mp_socket
   EAL: Selected IOVA mode 'PA'
   EAL: No available hugepages reported in hugepages-1048576kB
   -------

Please double check.  Seems to only happen with clang/llvm.

> Honnappa Nagarahalli (6):

>   lib/ring: apis to support configurable element size

>   lib/ring: add template to support different element sizes

>   tools/checkpatch: relax constraints on __rte_experimental

>   lib/ring: add ring APIs to support 32b ring elements

>   lib/hash: use ring with 32b element size to save memory

>   lib/eventdev: use ring templates for event rings

>

>  devtools/checkpatches.sh                  |  11 +-

>  lib/librte_eventdev/Makefile              |   2 +

>  lib/librte_eventdev/meson.build           |   2 +

>  lib/librte_eventdev/rte_event_ring.c      | 146 +---------

>  lib/librte_eventdev/rte_event_ring.h      |  41 +--

>  lib/librte_eventdev/rte_event_ring_128b.c |  19 ++

>  lib/librte_eventdev/rte_event_ring_128b.h |  44 +++

>  lib/librte_hash/rte_cuckoo_hash.c         |  55 ++--

>  lib/librte_hash/rte_cuckoo_hash.h         |   2 +-

>  lib/librte_ring/Makefile                  |   9 +-

>  lib/librte_ring/meson.build               |  11 +-

>  lib/librte_ring/rte_ring.c                |  34 ++-

>  lib/librte_ring/rte_ring.h                |  72 +++++

>  lib/librte_ring/rte_ring_32.c             |  19 ++

>  lib/librte_ring/rte_ring_32.h             |  36 +++

>  lib/librte_ring/rte_ring_template.c       |  46 +++

>  lib/librte_ring/rte_ring_template.h       | 330 ++++++++++++++++++++++

>  lib/librte_ring/rte_ring_version.map      |   4 +

>  18 files changed, 660 insertions(+), 223 deletions(-)

>  create mode 100644 lib/librte_eventdev/rte_event_ring_128b.c

>  create mode 100644 lib/librte_eventdev/rte_event_ring_128b.h

>  create mode 100644 lib/librte_ring/rte_ring_32.c

>  create mode 100644 lib/librte_ring/rte_ring_32.h

>  create mode 100644 lib/librte_ring/rte_ring_template.c

>  create mode 100644 lib/librte_ring/rte_ring_template.h
David Marchand Oct. 7, 2019, 1:49 p.m. | #2
On Fri, Sep 6, 2019 at 9:05 PM Honnappa Nagarahalli
<honnappa.nagarahalli@arm.com> wrote:
>

> The current rte_ring hard-codes the type of the ring element to 'void *',

> hence the size of the element is hard-coded to 32b/64b. Since the ring

> element type is not an input to rte_ring APIs, it results in couple

> of issues:

>

> 1) If an application requires to store an element which is not 64b, it

>    needs to write its own ring APIs similar to rte_event_ring APIs. This

>    creates additional burden on the programmers, who end up making

>    work-arounds and often waste memory.

> 2) If there are multiple libraries that store elements of the same

>    type, currently they would have to write their own rte_ring APIs. This

>    results in code duplication.

>

> This patch consists of several parts:

> 1) New APIs to support configurable ring element size

>    These will help reduce code duplication in the templates. I think these

>    can be made internal (do not expose to DPDK applications, but expose to

>    DPDK libraries), feedback needed.

>

> 2) rte_ring templates

>    The templates provide an easy way to add new APIs for different ring

>    element types/sizes which can be used by multiple libraries. These

>    also allow for creating APIs to store elements of custom types

>    (for ex: a structure)

>

>    The template needs 4 parameters:

>    a) RTE_RING_TMPLT_API_SUFFIX - This is used as a suffix to the

>       rte_ring APIs.

>       For ex: if RTE_RING_TMPLT_API_SUFFIX is '32b', the API name will be

>       rte_ring_create_32b

>    b) RTE_RING_TMPLT_ELEM_SIZE - Size of the ring element in bytes.

>       For ex: sizeof(uint32_t)

>    c) RTE_RING_TMPLT_ELEM_TYPE - Type of the ring element.

>       For ex: uint32_t. If a common ring library does not use a standard

>       data type, it should create its own type by defining a structure

>       with standard data type. For ex: for an elment size of 96b, one

>       could define a structure

>

>       struct s_96b {

>           uint32_t a[3];

>       }

>       The common library can use this structure to define

>       RTE_RING_TMPLT_ELEM_TYPE.

>

>       The application using this common ring library should define its

>       element type as a union with the above structure.

>

>       union app_element_type {

>           struct s_96b v;

>           struct app_element {

>               uint16_t a;

>               uint16_t b;

>               uint32_t c;

>               uint32_t d;

>           }

>       }

>    d) RTE_RING_TMPLT_EXPERIMENTAL - Indicates if the new APIs being defined

>       are experimental. Should be set to empty to remove the experimental

>       tag.

>

>    The ring library consists of some APIs that are defined as inline

>    functions and some APIs that are non-inline functions. The non-inline

>    functions are in rte_ring_template.c. However, this file needs to be

>    included in other .c files. Any feedback on how to handle this is

>    appreciated.

>

>    Note that the templates help create the APIs that are dependent on the

>    element size (for ex: rte_ring_create, enqueue/dequeue etc). Other APIs

>    that do NOT depend on the element size do not need to be part of the

>    template (for ex: rte_ring_dump, rte_ring_count, rte_ring_free_count

>    etc).

>

> 3) APIs for 32b ring element size

>    This uses the templates to create APIs to enqueue/dequeue elements of

>    size 32b.

>

> 4) rte_hash libray is changed to use 32b ring APIs

>    The 32b APIs are used in rte_hash library to store the free slot index

>    and free bucket index.

>

> 5) Event Dev changed to use ring templates

>    Event Dev defines its own 128b ring APIs using the templates. This helps

>    in keeping the 'struct rte_event' as is. If Event Dev has to use generic

>    128b ring APIs, it requires 'struct rte_event' to change to

>    'union rte_event' to include a generic data type such as '__int128_t'.

>    This breaks the API compatibility and results in large number of

>    changes.

>    With this change, the event rings are stored on rte_ring's tailq.

>    Event Dev specific ring list is NOT available. IMO, this does not have

>    any impact to the user.

>

> This patch results in following checkpatch issue:

> WARNING:UNSPECIFIED_INT: Prefer 'unsigned int' to bare use of 'unsigned'

>

> However, this patch is following the rules in the existing code. Please

> let me know if this needs to be fixed.

>

> v2

>  - Change Event Ring implementation to use ring templates

>    (Jerin, Pavan)


I expect a v3 on this series:
- Bruce/Stephen were not happy with using macros,
- Aaron caught test issues,
- from my side, if patch 3 still applies after your changes, I prefer
we drop this patch on the check script, we can live with these
warnings,


Thanks.

-- 
David Marchand
Honnappa Nagarahalli Oct. 9, 2019, 2:47 a.m. | #3
The current rte_ring hard-codes the type of the ring element to 'void *',
hence the size of the element is hard-coded to 32b/64b. Since the ring
element type is not an input to rte_ring APIs, it results in couple
of issues:

1) If an application requires to store an element which is not 64b, it
   needs to write its own ring APIs similar to rte_event_ring APIs. This
   creates additional burden on the programmers, who end up making
   work-arounds and often waste memory.
2) If there are multiple libraries that store elements of the same
   type, currently they would have to write their own rte_ring APIs. This
   results in code duplication.

This patch adds new APIs to support configurable ring element size.
The APIs support custom element sizes by allowing to define the ring
element to be a multiple of 32b.

The aim is to achieve same performance as the existing ring
implementation. The patch adds same performance tests that are run
for existing APIs. This allows for performance comparison.

I also tested with memcpy. x86 shows significant improvements on bulk
and burst tests. On the Arm platform, I used, there is a drop of
4% to 6% in few tests. May be this is something that we can explore
later.

Note that this version skips changes to other libraries as I would
like to get an agreement on the implementation from the community.
They will be added once there is agreement on the rte_ring changes.

v4
 - Few fixes after more performance testing

v3
 - Removed macro-fest and used inline functions
   (Stephen, Bruce)

v2
 - Change Event Ring implementation to use ring templates
   (Jerin, Pavan)

Honnappa Nagarahalli (2):
  lib/ring: apis to support configurable element size
  test/ring: add test cases for configurable element size ring

 app/test/Makefile                    |   1 +
 app/test/meson.build                 |   1 +
 app/test/test_ring_perf_elem.c       | 419 ++++++++++++
 lib/librte_ring/Makefile             |   3 +-
 lib/librte_ring/meson.build          |   3 +
 lib/librte_ring/rte_ring.c           |  45 +-
 lib/librte_ring/rte_ring.h           |   1 +
 lib/librte_ring/rte_ring_elem.h      | 946 +++++++++++++++++++++++++++
 lib/librte_ring/rte_ring_version.map |   2 +
 9 files changed, 1412 insertions(+), 9 deletions(-)
 create mode 100644 app/test/test_ring_perf_elem.c
 create mode 100644 lib/librte_ring/rte_ring_elem.h

-- 
2.17.1