linux-dpdk: shm: use native dpdk memzones

Message ID 1493986571-1306-1-git-send-email-matias.elo@nokia.com
State New
Headers show

Commit Message

Matias Elo May 5, 2017, 12:16 p.m.
Replace odp-linux shared memory implementation with native DPDK memzones.

Signed-off-by: Matias Elo <matias.elo@nokia.com>
---
Note:
- Shared memory validation test will fail if the hugepage size used by the
  wrapper-script.sh is not same as OS default hugepage size. The validation
  test has to modified to fix this.

 platform/linux-dpdk/Makefile.am                |  10 +-
 platform/linux-dpdk/include/odp_shm_internal.h |  32 ++
 platform/linux-dpdk/odp_init.c                 |  31 +-
 platform/linux-dpdk/odp_shared_memory.c        | 405 +++++++++++++++++++++++++
 4 files changed, 449 insertions(+), 29 deletions(-)
 create mode 100644 platform/linux-dpdk/include/odp_shm_internal.h
 create mode 100644 platform/linux-dpdk/odp_shared_memory.c

Comments

Balakrishna Garapati May 17, 2017, 3:23 p.m. | #1
On 5 May 2017 at 14:16, Matias Elo <matias.elo@nokia.com> wrote:

> Replace odp-linux shared memory implementation with native DPDK memzones.
>
> Signed-off-by: Matias Elo <matias.elo@nokia.com>
> ---
> Note:
> - Shared memory validation test will fail if the hugepage size used by the
>   wrapper-script.sh is not same as OS default hugepage size. The validation
>   test has to modified to fix this.
>
>  platform/linux-dpdk/Makefile.am                |  10 +-
>  platform/linux-dpdk/include/odp_shm_internal.h |  32 ++
>  platform/linux-dpdk/odp_init.c                 |  31 +-
>  platform/linux-dpdk/odp_shared_memory.c        | 405
> +++++++++++++++++++++++++
>  4 files changed, 449 insertions(+), 29 deletions(-)
>  create mode 100644 platform/linux-dpdk/include/odp_shm_internal.h
>  create mode 100644 platform/linux-dpdk/odp_shared_memory.c
>
> diff --git a/platform/linux-dpdk/Makefile.am b/platform/linux-dpdk/
> Makefile.am
> index 08c2c46..ac3086d 100644
> --- a/platform/linux-dpdk/Makefile.am
> +++ b/platform/linux-dpdk/Makefile.am
> @@ -119,9 +119,6 @@ odpdrvinclude_HEADERS = \
>                   $(srcdir)/include/odp/drv/compiler.h
>
>  noinst_HEADERS = \
> -                 ${top_srcdir}/platform/linux-generic/include/_fdserver_internal.h
> \
> -                 ${top_srcdir}/platform/linux-generic/include/_ishm_internal.h
> \
> -                 ${top_srcdir}/platform/linux-generic/include/_ishmphy_internal.h
> \
>                   ${top_srcdir}/platform/linux-generic/include/odp_align_internal.h
> \
>                   ${top_srcdir}/platform/linux-generic/include/odp_atomic_internal.h
> \
>                   ${srcdir}/include/odp_buffer_inlines.h \
> @@ -152,7 +149,7 @@ noinst_HEADERS = \
>                   ${top_srcdir}/platform/linux-generic/include/odp_ring_internal.h
> \
>                   ${top_srcdir}/platform/linux-
> generic/include/odp_schedule_if.h \
>                   ${top_srcdir}/platform/linux-generic/include/odp_sorted_list_internal.h
> \
> -                 ${top_srcdir}/platform/linux-generic/include/odp_shm_internal.h
> \
> +                 ${srcdir}/include/odp_shm_internal.h \
>                   ${srcdir}/include/odp_time_internal.h \
>                   ${top_srcdir}/platform/linux-generic/include/odp_timer_internal.h
> \
>                   ${top_srcdir}/platform/linux-generic/include/odp_timer_wheel_internal.h
> \
> @@ -165,9 +162,6 @@ noinst_HEADERS = \
>                   ${srcdir}/Makefile.inc
>
>  __LIB__libodp_dpdk_la_SOURCES = \
> -                          ../linux-generic/_fdserver.c \
> -                          ../linux-generic/_ishm.c \
> -                          ../linux-generic/_ishmphy.c \
>                            ../linux-generic/odp_atomic.c \
>                            ../linux-generic/odp_barrier.c \
>                            ../linux-generic/odp_bitmap.c \
> @@ -197,7 +191,7 @@ __LIB__libodp_dpdk_la_SOURCES = \
>                            ../linux-generic/odp_schedule.c \
>                            ../linux-generic/odp_schedule_if.c \
>                            ../linux-generic/odp_schedule_iquery.c \
> -                          ../linux-generic/odp_shared_memory.c \
> +                          odp_shared_memory.c \
>                            ../linux-generic/odp_sorted_list.c \
>                            ../linux-generic/odp_spinlock.c \
>                            ../linux-generic/odp_spinlock_recursive.c \
> diff --git a/platform/linux-dpdk/include/odp_shm_internal.h
> b/platform/linux-dpdk/include/odp_shm_internal.h
> new file mode 100644
> index 0000000..03adc14
> --- /dev/null
> +++ b/platform/linux-dpdk/include/odp_shm_internal.h
> @@ -0,0 +1,32 @@
> +/* Copyright (c) 2017, Linaro Limited
> + * All rights reserved.
> + *
> + * SPDX-License-Identifier:     BSD-3-Clause
> + */
> +
> +/**
> + * @file
> + *
> + * ODP shared memory - implementation internal
> + */
> +
> +#ifndef ODP_SHM_INTERNAL_H_
> +#define ODP_SHM_INTERNAL_H_
> +
> +#ifdef __cplusplus
> +extern "C" {
> +#endif
> +
> +int shm_init_global(void);
>
better to replace it with "_odp" prefix as they are internal and are called
from odp_init. Same for the remaining.

> +
> +int shm_init_local(void);
> +
> +int shm_term_global(void);
> +
> +int shm_term_local(void);
> +
> +#ifdef __cplusplus
> +}
> +#endif
> +
> +#endif
> diff --git a/platform/linux-dpdk/odp_init.c b/platform/linux-dpdk/odp_
> init.c
> index 6cea393..c31ff44 100644
> --- a/platform/linux-dpdk/odp_init.c
> +++ b/platform/linux-dpdk/odp_init.c
> @@ -12,6 +12,7 @@
>  #include <unistd.h>
>  #include <odp_internal.h>
>  #include <odp_schedule_if.h>
> +#include <odp_shm_internal.h>
>  #include <string.h>
>  #include <stdio.h>
>  #include <linux/limits.h>
> @@ -238,14 +239,8 @@ int odp_init_global(odp_instance_t *instance,
>                 cleanup_files(hpdir, odp_global_data.main_pid);
>         stage = SYSINFO_INIT;
>
> -       if (_odp_fdserver_init_global()) {
> -               ODP_ERR("ODP fdserver init failed.\n");
> -               goto init_failed;
> -       }
> -       stage = FDSERVER_INIT;
> -
> -       if (_odp_ishm_init_global()) {
> -               ODP_ERR("ODP ishm init failed.\n");
> +       if (shm_init_global()) {
> +               ODP_ERR("ODP shm init failed.\n");
>                 goto init_failed;
>         }
>         stage = ISHM_INIT;
> @@ -405,19 +400,13 @@ int _odp_term_global(enum init_stage stage)
>                 /* Fall through */
>
>         case ISHM_INIT:
> -               if (_odp_ishm_term_global()) {
> -                       ODP_ERR("ODP ishm term failed.\n");
> +               if (shm_term_global()) {
> +                       ODP_ERR("ODP shm term failed.\n");
>                         rc = -1;
>                 }
>                 /* Fall through */
> -
> +       /* Needed to prevent compiler warning */
>         case FDSERVER_INIT:
> -               if (_odp_fdserver_term_global()) {
> -                       ODP_ERR("ODP fdserver term failed.\n");
> -                       rc = -1;
> -               }
> -               /* Fall through */
> -
>         case SYSINFO_INIT:
>                 if (odp_system_info_term()) {
>                         ODP_ERR("ODP system info term failed.\n");
> @@ -455,8 +444,8 @@ int odp_init_local(odp_instance_t instance,
> odp_thread_type_t thr_type)
>                 goto init_fail;
>         }
>
> -       if (_odp_ishm_init_local()) {
> -               ODP_ERR("ODP ishm local init failed.\n");
> +       if (shm_init_local()) {
> +               ODP_ERR("ODP shm local init failed.\n");
>                 goto init_fail;
>         }
>         stage = ISHM_INIT;
> @@ -531,8 +520,8 @@ int _odp_term_local(enum init_stage stage)
>                 /* Fall through */
>
>         case ISHM_INIT:
> -               if (_odp_ishm_term_local()) {
> -                       ODP_ERR("ODP ishm local term failed.\n");
> +               if (shm_term_local()) {
> +                       ODP_ERR("ODP shm local term failed.\n");
>                         rc = -1;
>                 }
>                 /* Fall through */
> diff --git a/platform/linux-dpdk/odp_shared_memory.c
> b/platform/linux-dpdk/odp_shared_memory.c
> new file mode 100644
> index 0000000..9f3d03f
> --- /dev/null
> +++ b/platform/linux-dpdk/odp_shared_memory.c
> @@ -0,0 +1,405 @@
> +/* Copyright (c) 2017, Linaro Limited
> + * All rights reserved.
> + *
> + * SPDX-License-Identifier:     BSD-3-Clause
> + */
> +
> +#include <odp_align_internal.h>
> +#include <odp_config_internal.h>
> +#include <odp/api/debug.h>
> +#include <odp_debug_internal.h>
> +#include <odp/api/std_types.h>
> +#include <odp/api/shared_memory.h>
> +#include <odp/api/spinlock.h>
> +#include <odp/api/plat/strong_types.h>
> +#include <_ishm_internal.h>
> +#include <odp_shm_internal.h>
> +#include <string.h>
> +#include <sys/mman.h>
> +#include <sys/syscall.h>
> +#include <unistd.h>
> +#include <inttypes.h>
> +
> +#include <rte_lcore.h>
> +#include <rte_memzone.h>
> +
> +#define SHM_MAX_ALIGN (0x80000000)
> +#define SHM_BLOCK_NAME "%" PRIu64 "-%d-%s"
> +
> +ODP_STATIC_ASSERT(ODP_CONFIG_SHM_BLOCKS >= ODP_CONFIG_POOLS,
> +                 "ODP_CONFIG_SHM_BLOCKS < ODP_CONFIG_POOLS");
> +
> +ODP_STATIC_ASSERT(ODP_SHM_NAME_LEN >= RTE_MEMZONE_NAMESIZE,
> +                 "ODP_SHM_NAME_LEN < RTE_MEMZONE_NAMESIZE");
> +
> +typedef enum {
> +       SHM_TYPE_LOCAL = 0,
> +       SHM_TYPE_REMOTE
> +} shm_type_t;
> +
> +/**
> + * Memory block descriptor
> + */
> +typedef struct {
> +       /* Memory block type */
> +       shm_type_t type;
> +        /* Memory block name */
> +       char name[ODP_SHM_NAME_LEN];
> +       /* Block creation flags */
> +       uint32_t flags;
> +       /* DPDK memzone */
> +       const struct rte_memzone *mz;
> +} shm_block_t;
> +
> +/**
> + * Table of blocks describing allocated shared memory. This table is
> visible to
> + * every ODP thread (linux process or pthreads). It is allocated shared
> at odp
> + * init time and is therefore inherited by all.
> + */
> +typedef struct {
> +       odp_spinlock_t  lock;
> +       shm_block_t block[ODP_CONFIG_SHM_BLOCKS];
> +} shm_table_t;
> +
> +static shm_table_t *shm_tbl;
> +
> +/**
> + * Check if DPDK memzone name has been used already
> + */
> +static int mz_name_used(const char *name)
> +{
> +       int i;
>
replace "i" with some meaning full name ex: idx as used in
"find_free_block". Same applies to other palces.

> +
> +       for (i = 0; i < ODP_CONFIG_SHM_BLOCKS; i++) {
> +               if (shm_tbl->block[i].mz &&
> +                   strncmp(name, shm_tbl->block[i].mz->name,
> +                           RTE_MEMZONE_NAMESIZE) == 0)
> +                       return 1;
> +       }
> +       return 0;
> +}
> +
> +/**
> + * Convert ODP shm name into unique DPDK memzone name
> + */
> +static void name_to_mz_name(const char *name, char *mz_name)
> +{
> +       int i = 0;
> +
> +       /* Use pid and counter to make name unique */
> +       do {
> +               snprintf(mz_name, RTE_MEMZONE_NAMESIZE - 1, SHM_BLOCK_NAME,
> +                        (odp_instance_t)odp_global_data.main_pid, i++,
> name);
> +               mz_name[RTE_MEMZONE_NAMESIZE - 1] = 0;
> +       } while (mz_name_used(mz_name));
> +}
> +
> +static int find_free_block(void)
> +{
> +       int idx;
> +
> +       for (idx = 0; idx < ODP_CONFIG_SHM_BLOCKS; idx++) {
> +               if (shm_tbl->block[idx].mz == NULL)
> +                       return idx;
> +       }
> +       return -1;
> +}
> +
> +static inline uint32_t from_handle(odp_shm_t shm)
>
The name of the function looks bit odd when reading. May be to something
like "handle_to_idx" makes more understandable.

> +{
> +       return _odp_typeval(shm) - 1;
> +}
> +
> +static inline odp_shm_t to_handle(uint32_t idx)
>
the name looks ok here, but adding a prefix "idx_" makes more readable.

> +{
> +       return _odp_cast_scalar(odp_shm_t, idx + 1);
> +}
> +
> +static inline int valid_handle(odp_shm_t shm)
> +{
> +       int idx = from_handle(shm);
> +
> +       if (idx < 0 || idx >= ODP_CONFIG_SHM_BLOCKS ||
> +           shm_tbl->block[idx].mz == NULL) {
> +               ODP_ERR("Invalid odp_shm_t handle: %" PRIu64 "\n",
> +                       odp_shm_to_u64(shm));
> +               return 0;
>
in many places it returns -1 as a failure code & 0 for success return. Good
to maintain one format.

> +       }
> +       return 1;
> +}
> +
> +int shm_init_global(void)
> +{
> +       void *addr;
> +
> +       if ((getpid() != odp_global_data.main_pid) ||
> +           (syscall(SYS_gettid) != getpid()))
> +               ODP_ERR("shm_init_global() must be performed by the main "
> +                       "ODP process!\n.");
> +
> +       /* Allocate space for the internal shared mem block table */
> +       addr = mmap(NULL, sizeof(shm_table_t), PROT_READ | PROT_WRITE,
> +                   MAP_SHARED | MAP_ANONYMOUS, -1, 0);
> +       if (addr == MAP_FAILED) {
> +               ODP_ERR("Unable to mmap the shm block table\n");
> +               return -1;
> +       }
> +
> +       shm_tbl = addr;
> +       memset(shm_tbl, 0, sizeof(shm_table_t));
> +
> +       odp_spinlock_init(&shm_tbl->lock);
> +
> +       return 0;
> +}
> +
> +int shm_init_local(void)
> +{
> +       return 0;
> +}
> +
> +int shm_term_global(void)
> +{
> +       shm_block_t *block;
> +       int i;
>
replace "i" with some other name ex: idx.

> +
> +       if ((getpid() != odp_global_data.main_pid) ||
> +           (syscall(SYS_gettid) != getpid()))
> +               ODP_ERR("shm_term_global() must be performed by the main "
> +                       "ODP process!\n.");
> +
> +       /* Cleanup possibly non freed memory (and complain a bit) */
> +       for (i = 0; i < ODP_CONFIG_SHM_BLOCKS; i++) {
> +               block = &shm_tbl->block[i];
> +               if (block->mz) {
> +                       ODP_ERR("block '%s' was never freed (cleaning
> up...)\n",
> +                               block->name);
> +                       rte_memzone_free(block->mz);
> +               }
> +       }
> +       /* Free the shared memory block table */
> +       if (munmap(shm_tbl, sizeof(shm_table_t)) < 0) {
> +               ODP_ERR("Unable to munmap the shm block table\n");
> +               return -1;
> +       }
> +       return 0;
> +}
> +
> +int shm_term_local(void)
> +{
> +       return 0;
> +}
> +
> +int odp_shm_capability(odp_shm_capability_t *capa)
> +{
> +       memset(capa, 0, sizeof(odp_shm_capability_t));
> +
> +       capa->max_blocks = ODP_CONFIG_SHM_BLOCKS;
> +       capa->max_size = 0;
> +       capa->max_align = SHM_MAX_ALIGN;
> +
> +       return 0;
> +}
> +
> +odp_shm_t odp_shm_reserve(const char *name, uint64_t size, uint64_t align,
> +                         uint32_t flags)
> +{
> +       shm_block_t *block;
> +       const struct rte_memzone *mz;
> +       char mz_name[RTE_MEMZONE_NAMESIZE];
> +       uint32_t mz_flags = RTE_MEMZONE_1GB | RTE_MEMZONE_SIZE_HINT_ONLY;
> +       int idx;
> +
> +       if (align > SHM_MAX_ALIGN) {
> +               ODP_ERR("Align too large: %" PRIu64 "\n", align);
> +               return ODP_SHM_INVALID;
> +       }
> +
> +       /* DPDK requires alignment to be power of two */
> +       if (!rte_is_power_of_2(align))
> +               align = ROUNDUP_POWER2_U32(align);
> +
> +       odp_spinlock_lock(&shm_tbl->lock);
> +
> +       idx = find_free_block();
> +       if (idx < 0) {
> +               odp_spinlock_unlock(&shm_tbl->lock);
> +               ODP_ERR("No free SHM blocks left\n");
> +               return ODP_SHM_INVALID;
> +       }
> +       block = &shm_tbl->block[idx];
> +
> +       /* DPDK requires unique memzone names */
> +       name_to_mz_name(name, mz_name);
> +       mz = rte_memzone_reserve_aligned(mz_name, size, rte_socket_id(),
> +                                        mz_flags, align);
> +       if (mz == NULL) {
> +               odp_spinlock_unlock(&shm_tbl->lock);
> +               ODP_ERR("Reserving DPDK memzone failed\n");
> +               return ODP_SHM_INVALID;
> +       }
> +
> +       block->mz = mz;
> +       snprintf(block->name, ODP_SHM_NAME_LEN - 1, "%s", name);
> +       block->name[ODP_SHM_NAME_LEN - 1] = 0;
> +       block->type = SHM_TYPE_LOCAL;
> +       /* Flags are not yet used anywhere */
> +       block->flags = flags;
> +
> +       odp_spinlock_unlock(&shm_tbl->lock);
> +
> +       return to_handle(idx);
> +}
> +
> +odp_shm_t odp_shm_import(const char *remote_name, odp_instance_t odp_inst,
> +                        const char *local_name)
> +{
> +       shm_block_t *block;
> +       const struct rte_memzone *mz;
> +       char mz_name[RTE_MEMZONE_NAMESIZE];
> +       int idx;
> +
> +       snprintf(mz_name, RTE_MEMZONE_NAMESIZE - 1, SHM_BLOCK_NAME,
> odp_inst, 0,
> +                remote_name);
>
 not sure if I understood this, how do we know if the other odp instance
created the mz_name with this exact format with "0" as counter ?.

> +       mz_name[RTE_MEMZONE_NAMESIZE - 1] = 0;
> +
> +       mz = rte_memzone_lookup(mz_name);
> +       if (mz == NULL) {
> +               ODP_ERR("Unable to find remote SHM block: %s\n",
> remote_name);
> +               return ODP_SHM_INVALID;
> +       }
> +
> +       odp_spinlock_lock(&shm_tbl->lock);
> +
> +       idx = find_free_block();
> +       if (idx < 0) {
> +               odp_spinlock_unlock(&shm_tbl->lock);
> +               ODP_ERR("No free SHM blocks left\n");
> +               return ODP_SHM_INVALID;
> +       }
> +       block = &shm_tbl->block[idx];
> +
> +       block->mz = mz;
> +       snprintf(block->name, ODP_SHM_NAME_LEN - 1, "%s", local_name);
> +       block->name[ODP_SHM_NAME_LEN - 1] = 0;
> +       block->type = SHM_TYPE_REMOTE;
> +       block->flags = 0;
> +
> +       odp_spinlock_unlock(&shm_tbl->lock);
> +
> +       return to_handle(idx);
> +}
> +
> +int odp_shm_free(odp_shm_t shm)
> +{
> +       shm_block_t *block;
> +       int ret = 0;
> +
> +       odp_spinlock_lock(&shm_tbl->lock);
> +
> +       if (!valid_handle(shm)) {
> +               odp_spinlock_unlock(&shm_tbl->lock);
> +               return -1;
> +       }
> +
> +       block = &shm_tbl->block[from_handle(shm)];
> +
> +       /* Only the creator of memzone can free it */
> +       if (block->type == SHM_TYPE_LOCAL)
> +               ret = rte_memzone_free(block->mz);

since we are not checking the "ret", may be we should check the if we are
passing valid block->mz ?

> +
> +       block->mz = NULL;
>
shouldn't the whole "block" also be freed from shm_tbl ?

> +
> +       odp_spinlock_unlock(&shm_tbl->lock);
> +
> +       return ret;
> +}
> +
> +odp_shm_t odp_shm_lookup(const char *name)
> +{
> +       int i;
>
replace "i" with some other name.

> +
> +       odp_spinlock_lock(&shm_tbl->lock);
> +
> +       for (i = 0; i < ODP_CONFIG_SHM_BLOCKS; i++) {
> +               if (shm_tbl->block[i].mz &&
> +                   strncmp(name, shm_tbl->block[i].name,
> +                           ODP_SHM_NAME_LEN) == 0) {
> +                       odp_spinlock_unlock(&shm_tbl->lock);
> +                       return to_handle(i);
> +               }
> +       }
> +
> +       odp_spinlock_unlock(&shm_tbl->lock);
> +
> +       return ODP_SHM_INVALID;
> +}
> +
> +void *odp_shm_addr(odp_shm_t shm)
> +{
> +       void *addr;
> +
> +       odp_spinlock_lock(&shm_tbl->lock);
> +
> +       if (!valid_handle(shm)) {
> +               odp_spinlock_unlock(&shm_tbl->lock);
> +               return NULL;
> +       }
> +
> +       addr = shm_tbl->block[from_handle(shm)].mz->addr;
> +
> +       odp_spinlock_unlock(&shm_tbl->lock);
> +
> +       return addr;
> +}
> +
> +int odp_shm_info(odp_shm_t shm, odp_shm_info_t *info)
> +{
> +       shm_block_t *block;
> +       int idx = from_handle(shm);
> +
> +       odp_spinlock_lock(&shm_tbl->lock);
> +
> +       if (!valid_handle(shm)) {
> +               odp_spinlock_unlock(&shm_tbl->lock);
> +               return -1;
> +       }
> +
> +       block = &shm_tbl->block[idx];
> +
> +       info->name = block->name;
> +       info->addr = block->mz->addr;
> +       info->size = block->mz->len;
> +       info->page_size = block->mz->hugepage_sz;
> +       info->flags = block->mz->flags;
>
"flags"  param  was set to "0" (block->flags) during shm_reserve and here
it's returning dpdk mz->flags. Not sure if this is what odp application is
expecting.

/Krishna

> +
> +       odp_spinlock_unlock(&shm_tbl->lock);
> +
> +       return 0;
> +}
> +
> +void odp_shm_print_all(void)
> +{
> +       shm_block_t *block;
> +       int i;
> +
> +       odp_spinlock_lock(&shm_tbl->lock);
> +
> +       printf("\nShared memory blocks\n--------------------\n");
> +
> +       for (i = 0; i < ODP_CONFIG_SHM_BLOCKS; i++) {
> +               block = &shm_tbl->block[i];
> +               if (block->mz == NULL)
> +                       continue;
> +               printf("  %s: addr: %p, len: %" PRIu64 " page size: "
> +                      "%" PRIu64 "\n", block->name, block->mz->addr,
> +                      block->mz->len, block->mz->hugepage_sz);
> +       }
> +
> +       odp_spinlock_unlock(&shm_tbl->lock);
> +}
> +
> +uint64_t odp_shm_to_u64(odp_shm_t hdl)
> +{
> +       return _odp_pri(hdl);
> +}
> --
> 2.7.4
>
> _______________________________________________
> lng-odp-dpdk mailing list
> lng-odp-dpdk@lists.linaro.org
> https://lists.linaro.org/mailman/listinfo/lng-odp-dpdk
>
Matias Elo May 18, 2017, 7:47 a.m. | #2
Thanks for the review. Comments inline.

-Matias

> +int shm_init_global(void);
> better to replace it with "_odp" prefix as they are internal and are called from odp_init. Same for the remaining.

OK

> 
> +
> +/**
> + * Check if DPDK memzone name has been used already
> + */
> +static int mz_name_used(const char *name)
> +{
> +       int i;
> replace "i" with some meaning full name ex: idx as used in "find_free_block". Same applies to other palces.

OK

> +
> +static inline uint32_t from_handle(odp_shm_t shm)
> The name of the function looks bit odd when reading. May be to something like "handle_to_idx" makes more understandable.
> +
> +static inline odp_shm_t to_handle(uint32_t idx)
> the name looks ok here, but adding a prefix "idx_" makes more readable.

OK

> +static inline int valid_handle(odp_shm_t shm)
> +{
> +       int idx = from_handle(shm);
> +
> +       if (idx < 0 || idx >= ODP_CONFIG_SHM_BLOCKS ||
> +           shm_tbl->block[idx].mz == NULL) {
> +               ODP_ERR("Invalid odp_shm_t handle: %" PRIu64 "\n",
> +                       odp_shm_to_u64(shm));
> +               return 0;
> in many places it returns -1 as a failure code & 0 for success return. Good to maintain one format.  

This function acts in the same manner as e.g. odp_buffer_is_valid() and uses the same return values.
I could rename the function to handle_is_valid() to make it clearer.

> 
> +
> +int shm_term_global(void)
> +{
> +       shm_block_t *block;
> +       int i;
> replace "i" with some other name ex: idx. 

OK

> +
> +odp_shm_t odp_shm_import(const char *remote_name, odp_instance_t odp_inst,
> +                        const char *local_name)
> +{
> +       shm_block_t *block;
> +       const struct rte_memzone *mz;
> +       char mz_name[RTE_MEMZONE_NAMESIZE];
> +       int idx;
> +
> +       snprintf(mz_name, RTE_MEMZONE_NAMESIZE - 1, SHM_BLOCK_NAME, odp_inst, 0,
> +                remote_name);
>  not sure if I understood this, how do we know if the other odp instance created the mz_name with this exact format with "0" as counter ?.

What makes this problematic is that the odp api doesn't require shm names to be unique. If the application uses the same name multiple times there is no way to reference a specific shm block in odp_shm_import(). By using 0 here, the shm name references always to the first shm block allocated with that particular name. 

> +       block = &shm_tbl->block[from_handle(shm)];
> +
> +       /* Only the creator of memzone can free it */
> +       if (block->type == SHM_TYPE_LOCAL)
> +               ret = rte_memzone_free(block->mz);
> since we are not checking the "ret", may be we should check the if we are passing valid block->mz ? 

valid_handle() function called above checks this already.

> +
> +       block->mz = NULL;
> shouldn't the whole "block" also be freed from shm_tbl ?

I'm using the 'mz' member of shm_block_t to keep track if the block is reserved/free. I'll add
a comment about this.

> +
> +odp_shm_t odp_shm_lookup(const char *name)
> +{
> +       int i;
> replace "i" with some other name. 

OK

> 
> +
> +       info->name = block->name;
> +       info->addr = block->mz->addr;
> +       info->size = block->mz->len;
> +       info->page_size = block->mz->hugepage_sz;
> +       info->flags = block->mz->flags;
> "flags"  param  was set to "0" (block->flags) during shm_reserve and here it's returning dpdk mz->flags. Not sure if this is what odp application is expecting.


Good catch, will fix.

Patch hide | download patch | download mbox

diff --git a/platform/linux-dpdk/Makefile.am b/platform/linux-dpdk/Makefile.am
index 08c2c46..ac3086d 100644
--- a/platform/linux-dpdk/Makefile.am
+++ b/platform/linux-dpdk/Makefile.am
@@ -119,9 +119,6 @@  odpdrvinclude_HEADERS = \
 		  $(srcdir)/include/odp/drv/compiler.h
 
 noinst_HEADERS = \
-		  ${top_srcdir}/platform/linux-generic/include/_fdserver_internal.h \
-		  ${top_srcdir}/platform/linux-generic/include/_ishm_internal.h \
-		  ${top_srcdir}/platform/linux-generic/include/_ishmphy_internal.h \
 		  ${top_srcdir}/platform/linux-generic/include/odp_align_internal.h \
 		  ${top_srcdir}/platform/linux-generic/include/odp_atomic_internal.h \
 		  ${srcdir}/include/odp_buffer_inlines.h \
@@ -152,7 +149,7 @@  noinst_HEADERS = \
 		  ${top_srcdir}/platform/linux-generic/include/odp_ring_internal.h \
 		  ${top_srcdir}/platform/linux-generic/include/odp_schedule_if.h \
 		  ${top_srcdir}/platform/linux-generic/include/odp_sorted_list_internal.h \
-		  ${top_srcdir}/platform/linux-generic/include/odp_shm_internal.h \
+		  ${srcdir}/include/odp_shm_internal.h \
 		  ${srcdir}/include/odp_time_internal.h \
 		  ${top_srcdir}/platform/linux-generic/include/odp_timer_internal.h \
 		  ${top_srcdir}/platform/linux-generic/include/odp_timer_wheel_internal.h \
@@ -165,9 +162,6 @@  noinst_HEADERS = \
 		  ${srcdir}/Makefile.inc
 
 __LIB__libodp_dpdk_la_SOURCES = \
-			   ../linux-generic/_fdserver.c \
-			   ../linux-generic/_ishm.c \
-			   ../linux-generic/_ishmphy.c \
 			   ../linux-generic/odp_atomic.c \
 			   ../linux-generic/odp_barrier.c \
 			   ../linux-generic/odp_bitmap.c \
@@ -197,7 +191,7 @@  __LIB__libodp_dpdk_la_SOURCES = \
 			   ../linux-generic/odp_schedule.c \
 			   ../linux-generic/odp_schedule_if.c \
 			   ../linux-generic/odp_schedule_iquery.c \
-			   ../linux-generic/odp_shared_memory.c \
+			   odp_shared_memory.c \
 			   ../linux-generic/odp_sorted_list.c \
 			   ../linux-generic/odp_spinlock.c \
 			   ../linux-generic/odp_spinlock_recursive.c \
diff --git a/platform/linux-dpdk/include/odp_shm_internal.h b/platform/linux-dpdk/include/odp_shm_internal.h
new file mode 100644
index 0000000..03adc14
--- /dev/null
+++ b/platform/linux-dpdk/include/odp_shm_internal.h
@@ -0,0 +1,32 @@ 
+/* Copyright (c) 2017, Linaro Limited
+ * All rights reserved.
+ *
+ * SPDX-License-Identifier:     BSD-3-Clause
+ */
+
+/**
+ * @file
+ *
+ * ODP shared memory - implementation internal
+ */
+
+#ifndef ODP_SHM_INTERNAL_H_
+#define ODP_SHM_INTERNAL_H_
+
+#ifdef __cplusplus
+extern "C" {
+#endif
+
+int shm_init_global(void);
+
+int shm_init_local(void);
+
+int shm_term_global(void);
+
+int shm_term_local(void);
+
+#ifdef __cplusplus
+}
+#endif
+
+#endif
diff --git a/platform/linux-dpdk/odp_init.c b/platform/linux-dpdk/odp_init.c
index 6cea393..c31ff44 100644
--- a/platform/linux-dpdk/odp_init.c
+++ b/platform/linux-dpdk/odp_init.c
@@ -12,6 +12,7 @@ 
 #include <unistd.h>
 #include <odp_internal.h>
 #include <odp_schedule_if.h>
+#include <odp_shm_internal.h>
 #include <string.h>
 #include <stdio.h>
 #include <linux/limits.h>
@@ -238,14 +239,8 @@  int odp_init_global(odp_instance_t *instance,
 		cleanup_files(hpdir, odp_global_data.main_pid);
 	stage = SYSINFO_INIT;
 
-	if (_odp_fdserver_init_global()) {
-		ODP_ERR("ODP fdserver init failed.\n");
-		goto init_failed;
-	}
-	stage = FDSERVER_INIT;
-
-	if (_odp_ishm_init_global()) {
-		ODP_ERR("ODP ishm init failed.\n");
+	if (shm_init_global()) {
+		ODP_ERR("ODP shm init failed.\n");
 		goto init_failed;
 	}
 	stage = ISHM_INIT;
@@ -405,19 +400,13 @@  int _odp_term_global(enum init_stage stage)
 		/* Fall through */
 
 	case ISHM_INIT:
-		if (_odp_ishm_term_global()) {
-			ODP_ERR("ODP ishm term failed.\n");
+		if (shm_term_global()) {
+			ODP_ERR("ODP shm term failed.\n");
 			rc = -1;
 		}
 		/* Fall through */
-
+	/* Needed to prevent compiler warning */
 	case FDSERVER_INIT:
-		if (_odp_fdserver_term_global()) {
-			ODP_ERR("ODP fdserver term failed.\n");
-			rc = -1;
-		}
-		/* Fall through */
-
 	case SYSINFO_INIT:
 		if (odp_system_info_term()) {
 			ODP_ERR("ODP system info term failed.\n");
@@ -455,8 +444,8 @@  int odp_init_local(odp_instance_t instance, odp_thread_type_t thr_type)
 		goto init_fail;
 	}
 
-	if (_odp_ishm_init_local()) {
-		ODP_ERR("ODP ishm local init failed.\n");
+	if (shm_init_local()) {
+		ODP_ERR("ODP shm local init failed.\n");
 		goto init_fail;
 	}
 	stage = ISHM_INIT;
@@ -531,8 +520,8 @@  int _odp_term_local(enum init_stage stage)
 		/* Fall through */
 
 	case ISHM_INIT:
-		if (_odp_ishm_term_local()) {
-			ODP_ERR("ODP ishm local term failed.\n");
+		if (shm_term_local()) {
+			ODP_ERR("ODP shm local term failed.\n");
 			rc = -1;
 		}
 		/* Fall through */
diff --git a/platform/linux-dpdk/odp_shared_memory.c b/platform/linux-dpdk/odp_shared_memory.c
new file mode 100644
index 0000000..9f3d03f
--- /dev/null
+++ b/platform/linux-dpdk/odp_shared_memory.c
@@ -0,0 +1,405 @@ 
+/* Copyright (c) 2017, Linaro Limited
+ * All rights reserved.
+ *
+ * SPDX-License-Identifier:     BSD-3-Clause
+ */
+
+#include <odp_align_internal.h>
+#include <odp_config_internal.h>
+#include <odp/api/debug.h>
+#include <odp_debug_internal.h>
+#include <odp/api/std_types.h>
+#include <odp/api/shared_memory.h>
+#include <odp/api/spinlock.h>
+#include <odp/api/plat/strong_types.h>
+#include <_ishm_internal.h>
+#include <odp_shm_internal.h>
+#include <string.h>
+#include <sys/mman.h>
+#include <sys/syscall.h>
+#include <unistd.h>
+#include <inttypes.h>
+
+#include <rte_lcore.h>
+#include <rte_memzone.h>
+
+#define SHM_MAX_ALIGN (0x80000000)
+#define SHM_BLOCK_NAME "%" PRIu64 "-%d-%s"
+
+ODP_STATIC_ASSERT(ODP_CONFIG_SHM_BLOCKS >= ODP_CONFIG_POOLS,
+		  "ODP_CONFIG_SHM_BLOCKS < ODP_CONFIG_POOLS");
+
+ODP_STATIC_ASSERT(ODP_SHM_NAME_LEN >= RTE_MEMZONE_NAMESIZE,
+		  "ODP_SHM_NAME_LEN < RTE_MEMZONE_NAMESIZE");
+
+typedef enum {
+	SHM_TYPE_LOCAL = 0,
+	SHM_TYPE_REMOTE
+} shm_type_t;
+
+/**
+ * Memory block descriptor
+ */
+typedef struct {
+	/* Memory block type */
+	shm_type_t type;
+	 /* Memory block name */
+	char name[ODP_SHM_NAME_LEN];
+	/* Block creation flags */
+	uint32_t flags;
+	/* DPDK memzone */
+	const struct rte_memzone *mz;
+} shm_block_t;
+
+/**
+ * Table of blocks describing allocated shared memory. This table is visible to
+ * every ODP thread (linux process or pthreads). It is allocated shared at odp
+ * init time and is therefore inherited by all.
+ */
+typedef struct {
+	odp_spinlock_t  lock;
+	shm_block_t block[ODP_CONFIG_SHM_BLOCKS];
+} shm_table_t;
+
+static shm_table_t *shm_tbl;
+
+/**
+ * Check if DPDK memzone name has been used already
+ */
+static int mz_name_used(const char *name)
+{
+	int i;
+
+	for (i = 0; i < ODP_CONFIG_SHM_BLOCKS; i++) {
+		if (shm_tbl->block[i].mz &&
+		    strncmp(name, shm_tbl->block[i].mz->name,
+			    RTE_MEMZONE_NAMESIZE) == 0)
+			return 1;
+	}
+	return 0;
+}
+
+/**
+ * Convert ODP shm name into unique DPDK memzone name
+ */
+static void name_to_mz_name(const char *name, char *mz_name)
+{
+	int i = 0;
+
+	/* Use pid and counter to make name unique */
+	do {
+		snprintf(mz_name, RTE_MEMZONE_NAMESIZE - 1, SHM_BLOCK_NAME,
+			 (odp_instance_t)odp_global_data.main_pid, i++, name);
+		mz_name[RTE_MEMZONE_NAMESIZE - 1] = 0;
+	} while (mz_name_used(mz_name));
+}
+
+static int find_free_block(void)
+{
+	int idx;
+
+	for (idx = 0; idx < ODP_CONFIG_SHM_BLOCKS; idx++) {
+		if (shm_tbl->block[idx].mz == NULL)
+			return idx;
+	}
+	return -1;
+}
+
+static inline uint32_t from_handle(odp_shm_t shm)
+{
+	return _odp_typeval(shm) - 1;
+}
+
+static inline odp_shm_t to_handle(uint32_t idx)
+{
+	return _odp_cast_scalar(odp_shm_t, idx + 1);
+}
+
+static inline int valid_handle(odp_shm_t shm)
+{
+	int idx = from_handle(shm);
+
+	if (idx < 0 || idx >= ODP_CONFIG_SHM_BLOCKS ||
+	    shm_tbl->block[idx].mz == NULL) {
+		ODP_ERR("Invalid odp_shm_t handle: %" PRIu64 "\n",
+			odp_shm_to_u64(shm));
+		return 0;
+	}
+	return 1;
+}
+
+int shm_init_global(void)
+{
+	void *addr;
+
+	if ((getpid() != odp_global_data.main_pid) ||
+	    (syscall(SYS_gettid) != getpid()))
+		ODP_ERR("shm_init_global() must be performed by the main "
+			"ODP process!\n.");
+
+	/* Allocate space for the internal shared mem block table */
+	addr = mmap(NULL, sizeof(shm_table_t), PROT_READ | PROT_WRITE,
+		    MAP_SHARED | MAP_ANONYMOUS, -1, 0);
+	if (addr == MAP_FAILED) {
+		ODP_ERR("Unable to mmap the shm block table\n");
+		return -1;
+	}
+
+	shm_tbl = addr;
+	memset(shm_tbl, 0, sizeof(shm_table_t));
+
+	odp_spinlock_init(&shm_tbl->lock);
+
+	return 0;
+}
+
+int shm_init_local(void)
+{
+	return 0;
+}
+
+int shm_term_global(void)
+{
+	shm_block_t *block;
+	int i;
+
+	if ((getpid() != odp_global_data.main_pid) ||
+	    (syscall(SYS_gettid) != getpid()))
+		ODP_ERR("shm_term_global() must be performed by the main "
+			"ODP process!\n.");
+
+	/* Cleanup possibly non freed memory (and complain a bit) */
+	for (i = 0; i < ODP_CONFIG_SHM_BLOCKS; i++) {
+		block = &shm_tbl->block[i];
+		if (block->mz) {
+			ODP_ERR("block '%s' was never freed (cleaning up...)\n",
+				block->name);
+			rte_memzone_free(block->mz);
+		}
+	}
+	/* Free the shared memory block table */
+	if (munmap(shm_tbl, sizeof(shm_table_t)) < 0) {
+		ODP_ERR("Unable to munmap the shm block table\n");
+		return -1;
+	}
+	return 0;
+}
+
+int shm_term_local(void)
+{
+	return 0;
+}
+
+int odp_shm_capability(odp_shm_capability_t *capa)
+{
+	memset(capa, 0, sizeof(odp_shm_capability_t));
+
+	capa->max_blocks = ODP_CONFIG_SHM_BLOCKS;
+	capa->max_size = 0;
+	capa->max_align = SHM_MAX_ALIGN;
+
+	return 0;
+}
+
+odp_shm_t odp_shm_reserve(const char *name, uint64_t size, uint64_t align,
+			  uint32_t flags)
+{
+	shm_block_t *block;
+	const struct rte_memzone *mz;
+	char mz_name[RTE_MEMZONE_NAMESIZE];
+	uint32_t mz_flags = RTE_MEMZONE_1GB | RTE_MEMZONE_SIZE_HINT_ONLY;
+	int idx;
+
+	if (align > SHM_MAX_ALIGN) {
+		ODP_ERR("Align too large: %" PRIu64 "\n", align);
+		return ODP_SHM_INVALID;
+	}
+
+	/* DPDK requires alignment to be power of two */
+	if (!rte_is_power_of_2(align))
+		align = ROUNDUP_POWER2_U32(align);
+
+	odp_spinlock_lock(&shm_tbl->lock);
+
+	idx = find_free_block();
+	if (idx < 0) {
+		odp_spinlock_unlock(&shm_tbl->lock);
+		ODP_ERR("No free SHM blocks left\n");
+		return ODP_SHM_INVALID;
+	}
+	block = &shm_tbl->block[idx];
+
+	/* DPDK requires unique memzone names */
+	name_to_mz_name(name, mz_name);
+	mz = rte_memzone_reserve_aligned(mz_name, size, rte_socket_id(),
+					 mz_flags, align);
+	if (mz == NULL) {
+		odp_spinlock_unlock(&shm_tbl->lock);
+		ODP_ERR("Reserving DPDK memzone failed\n");
+		return ODP_SHM_INVALID;
+	}
+
+	block->mz = mz;
+	snprintf(block->name, ODP_SHM_NAME_LEN - 1, "%s", name);
+	block->name[ODP_SHM_NAME_LEN - 1] = 0;
+	block->type = SHM_TYPE_LOCAL;
+	/* Flags are not yet used anywhere */
+	block->flags = flags;
+
+	odp_spinlock_unlock(&shm_tbl->lock);
+
+	return to_handle(idx);
+}
+
+odp_shm_t odp_shm_import(const char *remote_name, odp_instance_t odp_inst,
+			 const char *local_name)
+{
+	shm_block_t *block;
+	const struct rte_memzone *mz;
+	char mz_name[RTE_MEMZONE_NAMESIZE];
+	int idx;
+
+	snprintf(mz_name, RTE_MEMZONE_NAMESIZE - 1, SHM_BLOCK_NAME, odp_inst, 0,
+		 remote_name);
+	mz_name[RTE_MEMZONE_NAMESIZE - 1] = 0;
+
+	mz = rte_memzone_lookup(mz_name);
+	if (mz == NULL) {
+		ODP_ERR("Unable to find remote SHM block: %s\n", remote_name);
+		return ODP_SHM_INVALID;
+	}
+
+	odp_spinlock_lock(&shm_tbl->lock);
+
+	idx = find_free_block();
+	if (idx < 0) {
+		odp_spinlock_unlock(&shm_tbl->lock);
+		ODP_ERR("No free SHM blocks left\n");
+		return ODP_SHM_INVALID;
+	}
+	block = &shm_tbl->block[idx];
+
+	block->mz = mz;
+	snprintf(block->name, ODP_SHM_NAME_LEN - 1, "%s", local_name);
+	block->name[ODP_SHM_NAME_LEN - 1] = 0;
+	block->type = SHM_TYPE_REMOTE;
+	block->flags = 0;
+
+	odp_spinlock_unlock(&shm_tbl->lock);
+
+	return to_handle(idx);
+}
+
+int odp_shm_free(odp_shm_t shm)
+{
+	shm_block_t *block;
+	int ret = 0;
+
+	odp_spinlock_lock(&shm_tbl->lock);
+
+	if (!valid_handle(shm)) {
+		odp_spinlock_unlock(&shm_tbl->lock);
+		return -1;
+	}
+
+	block = &shm_tbl->block[from_handle(shm)];
+
+	/* Only the creator of memzone can free it */
+	if (block->type == SHM_TYPE_LOCAL)
+		ret = rte_memzone_free(block->mz);
+
+	block->mz = NULL;
+
+	odp_spinlock_unlock(&shm_tbl->lock);
+
+	return ret;
+}
+
+odp_shm_t odp_shm_lookup(const char *name)
+{
+	int i;
+
+	odp_spinlock_lock(&shm_tbl->lock);
+
+	for (i = 0; i < ODP_CONFIG_SHM_BLOCKS; i++) {
+		if (shm_tbl->block[i].mz &&
+		    strncmp(name, shm_tbl->block[i].name,
+			    ODP_SHM_NAME_LEN) == 0) {
+			odp_spinlock_unlock(&shm_tbl->lock);
+			return to_handle(i);
+		}
+	}
+
+	odp_spinlock_unlock(&shm_tbl->lock);
+
+	return ODP_SHM_INVALID;
+}
+
+void *odp_shm_addr(odp_shm_t shm)
+{
+	void *addr;
+
+	odp_spinlock_lock(&shm_tbl->lock);
+
+	if (!valid_handle(shm)) {
+		odp_spinlock_unlock(&shm_tbl->lock);
+		return NULL;
+	}
+
+	addr = shm_tbl->block[from_handle(shm)].mz->addr;
+
+	odp_spinlock_unlock(&shm_tbl->lock);
+
+	return addr;
+}
+
+int odp_shm_info(odp_shm_t shm, odp_shm_info_t *info)
+{
+	shm_block_t *block;
+	int idx = from_handle(shm);
+
+	odp_spinlock_lock(&shm_tbl->lock);
+
+	if (!valid_handle(shm)) {
+		odp_spinlock_unlock(&shm_tbl->lock);
+		return -1;
+	}
+
+	block = &shm_tbl->block[idx];
+
+	info->name = block->name;
+	info->addr = block->mz->addr;
+	info->size = block->mz->len;
+	info->page_size = block->mz->hugepage_sz;
+	info->flags = block->mz->flags;
+
+	odp_spinlock_unlock(&shm_tbl->lock);
+
+	return 0;
+}
+
+void odp_shm_print_all(void)
+{
+	shm_block_t *block;
+	int i;
+
+	odp_spinlock_lock(&shm_tbl->lock);
+
+	printf("\nShared memory blocks\n--------------------\n");
+
+	for (i = 0; i < ODP_CONFIG_SHM_BLOCKS; i++) {
+		block = &shm_tbl->block[i];
+		if (block->mz == NULL)
+			continue;
+		printf("  %s: addr: %p, len: %" PRIu64 " page size: "
+		       "%" PRIu64 "\n", block->name, block->mz->addr,
+		       block->mz->len, block->mz->hugepage_sz);
+	}
+
+	odp_spinlock_unlock(&shm_tbl->lock);
+}
+
+uint64_t odp_shm_to_u64(odp_shm_t hdl)
+{
+	return _odp_pri(hdl);
+}