| Message ID | 1416360067-13278-2-git-send-email-bill.fischofer@linaro.org |
|---|---|
| State | New |
| Headers | show |
Hi, I think you could have squashed the two patches, it's easy to figure out that there are changes in test apps and changes in API files. The combined patches don't break the build though. See my comments below: On Wed, Nov 19, 2014 at 3:21 AM, Bill Fischofer <bill.fischofer@linaro.org> wrote: > Signed-off-by: Bill Fischofer <bill.fischofer@linaro.org> > --- > .../linux-generic/include/api/odp_buffer_pool.h | 111 +++++++++++++++++++-- > .../linux-generic/include/api/odp_platform_types.h | 9 ++ > .../linux-generic/include/api/odp_shared_memory.h | 10 +- > .../include/odp_buffer_pool_internal.h | 8 ++ > platform/linux-generic/odp_buffer_pool.c | 99 ++++++++++++++++-- > platform/linux-generic/odp_schedule.c | 20 ++-- > 6 files changed, 212 insertions(+), 45 deletions(-) > > diff --git a/platform/linux-generic/include/api/odp_buffer_pool.h b/platform/linux-generic/include/api/odp_buffer_pool.h > index 30b83e0..d87872b 100644 > --- a/platform/linux-generic/include/api/odp_buffer_pool.h > +++ b/platform/linux-generic/include/api/odp_buffer_pool.h > @@ -36,32 +36,121 @@ extern "C" { > #define ODP_BUFFER_POOL_INVALID 0 > > /** > + * Buffer pool parameters > + * > + * @param[in] buf_size Size of application data in each buffer > + * @param[in] buf_align Alignment requested for buffer. Specified > + * as a power-of-2 integer (e.g., 8, 16, 64). > + * @param[in] num_bufs Number of buffers that pool should contain > + * @param[in] buf_type Buffer type > + */ > +typedef struct odp_buffer_pool_param_t { > + size_t buf_size; /**< Application data size of each buffer */ > + size_t buf_align; /**< Requested buffer alignment */ > + uint32_t num_bufs; /**< Number of buffers in this pool */ > + int buf_type; /**< Buffer type */ > +} odp_buffer_pool_param_t; /**< Type of buffer pool parameter struct */ > + > +/** > * Create a buffer pool > * > - * @param name Name of the pool (max ODP_BUFFER_POOL_NAME_LEN - 1 chars) > - * @param base_addr Pool base address > - * @param size Pool size in bytes > - * @param buf_size Buffer size in bytes > - * @param buf_align Minimum buffer alignment > - * @param buf_type Buffer type > + * @param[in] name Name of the pool, max ODP_BUFFER_POOL_NAME_LEN-1 chars. > + * May be specified as NULL for anonymous pools. > + * > + * @param[in] shm The shared memory object in which to create the pool. > + * May be specified as ODP_SHM_NULL to request that the > + * storage needed for the pool be allocated by the API. > + * > + * @param[in] params Parameters controlling the creation of this buffer pool. > + * > + * @return Buffer pool handle or ODP_BUFFER_POOL_NULL if call failed. > * > - * @return Buffer pool handle > + * @note This routine is used to create a buffer pool. It take three nit: It takes three > + * arguments: the optional name of the pool to be created, an optional shared > + * memory handle, and a parameter struct that describes the pool to be > + * created. If a name is not specified the result is an anonymous pool that > + * cannot be referenced by odp_buffer_pool_lookup(). > + * > + * @note The shm parameter is used to specify whether the pool should be > + * created in an application-supplied shared memory object. If specified as nit: Two spaces after sentence > + * ODP_SHM_NULL, the implementation will allocate a shared memory object to > + * contain the pool. > + * > + * @note The parameter structure specifies the type of buffer to be contained Maybe 'type of buffers' is better, next piece is 'their number, size, and alignment' > + * in the pool as well as their number, size, and alignment. The specified > + * buf_align MUST be a power of two, and is the minimum alignment requested by > + * the caller. The buf_align parameter MAY be specified as 0 to request that nit: two spaces again > + * the implementation use a default alignment which MUST be a minimum of 8 > + * bytes. > */ > + > odp_buffer_pool_t odp_buffer_pool_create(const char *name, > - void *base_addr, uint64_t size, > - size_t buf_size, size_t buf_align, > - int buf_type); > + odp_shm_t shm, > + odp_buffer_pool_param_t *params); > > +/** > + * Destroy a buffer pool previously created by odp_buffer_pool_create() > + * > + * @param[in] pool Handle of the buffer pool to be destroyed > + * > + * @return 0 on Success, -1 on Failure. > + * > + * @note This routine destroys a previously created buffer pool. This call nit: two spaces > + * does not destroy any shared memory object passed to > + * odp_buffer_pool_create() used to store the buffer pool contents. The caller > + * takes responsibility for that. If no shared memory object was passed as nit: 2 x <SPACE> > + * part of the create call, then this routine will destroy any internal shared > + * memory objects associated with the buffer pool. Results are undefined if nit: you know the drill by now > + * an attempt is made to destroy a buffer pool that contains allocated or > + * otherwise active buffers. > + */ > +int odp_buffer_pool_destroy(odp_buffer_pool_t pool); > > /** > * Find a buffer pool by name > * > - * @param name Name of the pool > + * @param[in] name Name of the pool > * > * @return Buffer pool handle, or ODP_BUFFER_POOL_INVALID if not found. > + * > + * @note This routine cannot be used to look up an anonymous pool (one created > + * with no name). > */ > odp_buffer_pool_t odp_buffer_pool_lookup(const char *name); > > +/** > + * @param[out] name Pointer to the name of the buffer pool. NULL if this > + * pool is anonymous. > + * > + * @param[out] shm Handle of the shared memory object containing this pool, > + * if pool was created from an application supplied shared > + * memory area. Otherwise ODP_SHM_NULL. > + * > + * @param[out] params Copy of the odp_buffer_pool_param_t used to create this > + * pool. > + */ > +typedef struct odp_buffer_pool_info_t { > + const char *name; > + odp_buffer_pool_param_t params; > +} odp_buffer_pool_info_t; > + > +/** > + * Retrieve information about a buffer pool > + * > + * @param[in] pool Buffer pool handle > + * > + * @param[out] shm Recieves odp_shm_t supplied by caller at nit: Receives > + * pool creation, or ODP_SHM_NULL if the > + * pool is managed internally. > + * > + * @param[out] info Receives an odp_buffer_pool_info_t object > + * that describes the pool. > + * > + * @return 0 on success, -1 if info could not be retrieved. > + */ > + > +int odp_buffer_pool_info(odp_buffer_pool_t pool, odp_shm_t *shm, > + odp_buffer_pool_info_t *info); > > /** > * Print buffer pool info > diff --git a/platform/linux-generic/include/api/odp_platform_types.h b/platform/linux-generic/include/api/odp_platform_types.h > index 4db47d3..b9b3aea 100644 > --- a/platform/linux-generic/include/api/odp_platform_types.h > +++ b/platform/linux-generic/include/api/odp_platform_types.h > @@ -65,6 +65,15 @@ typedef uint32_t odp_pktio_t; > #define ODP_PKTIO_ANY ((odp_pktio_t)~0) > > /** > + * ODP shared memory block > + */ > +typedef uint32_t odp_shm_t; > + > +/** Invalid shared memory block */ > +#define ODP_SHM_INVALID 0 > +#define ODP_SHM_NULL ODP_SHM_INVALID /**< Synonym for buffer pool use */ > + > +/** > * @} > */ > > diff --git a/platform/linux-generic/include/api/odp_shared_memory.h b/platform/linux-generic/include/api/odp_shared_memory.h > index ff6f9a9..b681860 100644 > --- a/platform/linux-generic/include/api/odp_shared_memory.h > +++ b/platform/linux-generic/include/api/odp_shared_memory.h > @@ -20,6 +20,7 @@ extern "C" { > > > #include <odp_std_types.h> > +#include <odp_platform_types.h> > > /** @defgroup odp_shared_memory ODP SHARED MEMORY > * Operations on shared memory. > @@ -38,15 +39,6 @@ extern "C" { > #define ODP_SHM_PROC 0x2 /**< Share with external processes */ > > /** > - * ODP shared memory block > - */ > -typedef uint32_t odp_shm_t; > - > -/** Invalid shared memory block */ > -#define ODP_SHM_INVALID 0 This should be in a new patch, similar to the refactoring of implementation specific typedefs and defines that you sent for buffers, buffer pools, packets and packet_io. But I think it's ok to send it in this series if you don't want a separately tracked patch. > - > - > -/** > * Shared memory block info > */ > typedef struct odp_shm_info_t { > diff --git a/platform/linux-generic/include/odp_buffer_pool_internal.h b/platform/linux-generic/include/odp_buffer_pool_internal.h > index e0210bd..a1c0990 100644 > --- a/platform/linux-generic/include/odp_buffer_pool_internal.h > +++ b/platform/linux-generic/include/odp_buffer_pool_internal.h > @@ -56,6 +56,14 @@ struct pool_entry_s { > size_t buf_size; > size_t buf_offset; > uint64_t num_bufs; > + odp_shm_t pool_shm; > + union { > + uint32_t all; > + struct { > + uint32_t has_name:1; > + uint32_t user_supplied_shm:1; > + }; > + } flags; > void *pool_base_addr; > uint64_t pool_size; > size_t user_size; > diff --git a/platform/linux-generic/odp_buffer_pool.c b/platform/linux-generic/odp_buffer_pool.c > index a48d7d6..88497a9 100644 > --- a/platform/linux-generic/odp_buffer_pool.c > +++ b/platform/linux-generic/odp_buffer_pool.c > @@ -369,16 +369,19 @@ static void link_bufs(pool_entry_t *pool) > } > } > > - > odp_buffer_pool_t odp_buffer_pool_create(const char *name, > - void *base_addr, uint64_t size, > - size_t buf_size, size_t buf_align, > - int buf_type) > + odp_shm_t shm, > + odp_buffer_pool_param_t *params) > { > odp_buffer_pool_t pool_hdl = ODP_BUFFER_POOL_INVALID; > pool_entry_t *pool; > + void *base_addr; > + size_t base_size; > uint32_t i; > > + if (params == NULL) > + return ODP_BUFFER_POOL_INVALID; > + > for (i = 0; i < ODP_CONFIG_BUFFER_POOLS; i++) { > pool = get_pool_entry(i); > > @@ -386,15 +389,49 @@ odp_buffer_pool_t odp_buffer_pool_create(const char *name, > > if (pool->s.buf_base == 0) { > /* found free pool */ > + pool->s.flags.all = 0; > + > + if (name == NULL) { > + pool->s.name[0] = 0; > + } else { > + strncpy(pool->s.name, name, > + ODP_BUFFER_POOL_NAME_LEN - 1); > + pool->s.name[ODP_BUFFER_POOL_NAME_LEN - 1] = 0; > + pool->s.flags.has_name = 1; > + } > + > + pool->s.pool_shm = shm; This has to be moved further down, if shm is NULL you get a bug. > + > + if (shm == ODP_SHM_NULL) { > + base_size = > + ODP_PAGE_SIZE_ROUNDUP( > + params->buf_size * > + params->num_bufs); > + shm = odp_shm_reserve(pool->s.name, > + base_size, > + ODP_PAGE_SIZE, 0); > + if (shm == ODP_SHM_INVALID) { > + UNLOCK(&pool->s.lock); > + return ODP_BUFFER_INVALID; > + } > + } else { > + odp_shm_info_t info; > + if (odp_shm_info(shm, &info) != 0) { > + UNLOCK(&pool->s.lock); > + return ODP_BUFFER_POOL_INVALID; > + } > + base_size = info.size; > + pool->s.flags.user_supplied_shm = 1; > + } > + > + base_addr = odp_shm_addr(shm); > > - strncpy(pool->s.name, name, > - ODP_BUFFER_POOL_NAME_LEN - 1); > - pool->s.name[ODP_BUFFER_POOL_NAME_LEN - 1] = 0; > pool->s.pool_base_addr = base_addr; > - pool->s.pool_size = size; > - pool->s.user_size = buf_size; > - pool->s.user_align = buf_align; > - pool->s.buf_type = buf_type; > + pool->s.pool_size = base_size; > + pool->s.user_size = params->buf_size; > + pool->s.user_align = params->buf_align == 0 ? > + ODP_CACHE_LINE_SIZE : params->buf_align; > + pool->s.buf_type = params->buf_type; > > link_bufs(pool); > > @@ -431,6 +468,46 @@ odp_buffer_pool_t odp_buffer_pool_lookup(const char *name) > return ODP_BUFFER_POOL_INVALID; > } > > +int odp_buffer_pool_info(odp_buffer_pool_t pool_hdl, > + odp_shm_t *shm, > + odp_buffer_pool_info_t *info) > +{ > + uint32_t pool_id = pool_handle_to_index(pool_hdl); > + pool_entry_t *pool = get_pool_entry(pool_id); > + > + if (pool == NULL || info == NULL) > + return -1; > + > + *shm = pool->s.flags.user_supplied_shm ? > + pool->s.pool_shm : ODP_SHM_NULL; > + info->name = pool->s.name; > + info->params.buf_size = pool->s.buf_size; > + info->params.buf_align = pool->s.user_align; > + info->params.num_bufs = pool->s.num_bufs; > + info->params.buf_type = pool->s.buf_type; > + > + return 0; > +} > + > +int odp_buffer_pool_destroy(odp_buffer_pool_t pool_hdl) > +{ > + uint32_t pool_id = pool_handle_to_index(pool_hdl); > + pool_entry_t *pool = get_pool_entry(pool_id); > + > + if (pool == NULL) > + return -1; > + > + LOCK(&pool->s.lock); > + > + if (!pool->s.flags.user_supplied_shm) > + odp_shm_free(pool->s.pool_shm); > + > + pool->s.buf_base = 0; > + UNLOCK(&pool->s.lock); > + > + return 0; > +} > + > > odp_buffer_t odp_buffer_alloc(odp_buffer_pool_t pool_hdl) > { > diff --git a/platform/linux-generic/odp_schedule.c b/platform/linux-generic/odp_schedule.c > index 1bf819b..85e249f 100644 > --- a/platform/linux-generic/odp_schedule.c > +++ b/platform/linux-generic/odp_schedule.c > @@ -83,8 +83,8 @@ int odp_schedule_init_global(void) > { > odp_shm_t shm; > odp_buffer_pool_t pool; > - void *pool_base; > int i, j; > + odp_buffer_pool_param_t params; > > ODP_DBG("Schedule init ... "); > > @@ -99,20 +99,12 @@ int odp_schedule_init_global(void) > return -1; > } > > - shm = odp_shm_reserve("odp_sched_pool", > - SCHED_POOL_SIZE, ODP_CACHE_LINE_SIZE, 0); > + params.buf_size = sizeof(queue_desc_t); > + params.buf_align = ODP_CACHE_LINE_SIZE; > + params.num_bufs = SCHED_POOL_SIZE/sizeof(queue_desc_t); > + params.buf_type = ODP_BUFFER_TYPE_RAW; > > - pool_base = odp_shm_addr(shm); > - > - if (pool_base == NULL) { > - ODP_ERR("Schedule init: Shm reserve failed.\n"); > - return -1; > - } > - > - pool = odp_buffer_pool_create("odp_sched_pool", pool_base, > - SCHED_POOL_SIZE, sizeof(queue_desc_t), > - ODP_CACHE_LINE_SIZE, > - ODP_BUFFER_TYPE_RAW); > + pool = odp_buffer_pool_create("odp_sched_pool", ODP_SHM_NULL, ¶ms); > > if (pool == ODP_BUFFER_POOL_INVALID) { > ODP_ERR("Schedule init: Pool create failed.\n"); > -- > 1.8.3.2 > > > _______________________________________________ > lng-odp mailing list > lng-odp@lists.linaro.org > http://lists.linaro.org/mailman/listinfo/lng-odp
On the two spaces. If we want to enforce this rule (which is contrary to normal English writing style), we need up update checkpatch. The patch submission criteria is that patches are checkpatch-clean. Now we want to add additional criteria on top of this. That's awkward. Can checkpatch be updated? If not, I suggest we drop this nit picking. Bill On Wed, Nov 19, 2014 at 5:02 AM, Ciprian Barbu <ciprian.barbu@linaro.org> wrote: > Hi, > > I think you could have squashed the two patches, it's easy to figure > out that there are changes in test apps and changes in API files. The > combined patches don't break the build though. > > See my comments below: > > On Wed, Nov 19, 2014 at 3:21 AM, Bill Fischofer > <bill.fischofer@linaro.org> wrote: > > Signed-off-by: Bill Fischofer <bill.fischofer@linaro.org> > > --- > > .../linux-generic/include/api/odp_buffer_pool.h | 111 > +++++++++++++++++++-- > > .../linux-generic/include/api/odp_platform_types.h | 9 ++ > > .../linux-generic/include/api/odp_shared_memory.h | 10 +- > > .../include/odp_buffer_pool_internal.h | 8 ++ > > platform/linux-generic/odp_buffer_pool.c | 99 > ++++++++++++++++-- > > platform/linux-generic/odp_schedule.c | 20 ++-- > > 6 files changed, 212 insertions(+), 45 deletions(-) > > > > diff --git a/platform/linux-generic/include/api/odp_buffer_pool.h > b/platform/linux-generic/include/api/odp_buffer_pool.h > > index 30b83e0..d87872b 100644 > > --- a/platform/linux-generic/include/api/odp_buffer_pool.h > > +++ b/platform/linux-generic/include/api/odp_buffer_pool.h > > @@ -36,32 +36,121 @@ extern "C" { > > #define ODP_BUFFER_POOL_INVALID 0 > > > > /** > > + * Buffer pool parameters > > + * > > + * @param[in] buf_size Size of application data in each buffer > > + * @param[in] buf_align Alignment requested for buffer. Specified > > + * as a power-of-2 integer (e.g., 8, 16, 64). > > + * @param[in] num_bufs Number of buffers that pool should contain > > + * @param[in] buf_type Buffer type > > + */ > > +typedef struct odp_buffer_pool_param_t { > > + size_t buf_size; /**< Application data size of each > buffer */ > > + size_t buf_align; /**< Requested buffer alignment */ > > + uint32_t num_bufs; /**< Number of buffers in this pool > */ > > + int buf_type; /**< Buffer type */ > > +} odp_buffer_pool_param_t; /**< Type of buffer pool parameter > struct */ > > + > > +/** > > * Create a buffer pool > > * > > - * @param name Name of the pool (max ODP_BUFFER_POOL_NAME_LEN - 1 > chars) > > - * @param base_addr Pool base address > > - * @param size Pool size in bytes > > - * @param buf_size Buffer size in bytes > > - * @param buf_align Minimum buffer alignment > > - * @param buf_type Buffer type > > + * @param[in] name Name of the pool, max ODP_BUFFER_POOL_NAME_LEN-1 > chars. > > + * May be specified as NULL for anonymous pools. > > + * > > + * @param[in] shm The shared memory object in which to create the > pool. > > + * May be specified as ODP_SHM_NULL to request that > the > > + * storage needed for the pool be allocated by the > API. > > + * > > + * @param[in] params Parameters controlling the creation of this > buffer pool. > > + * > > + * @return Buffer pool handle or ODP_BUFFER_POOL_NULL if call failed. > > * > > - * @return Buffer pool handle > > + * @note This routine is used to create a buffer pool. It take three > > nit: It takes three > > > + * arguments: the optional name of the pool to be created, an optional > shared > > + * memory handle, and a parameter struct that describes the pool to be > > + * created. If a name is not specified the result is an anonymous pool > that > > + * cannot be referenced by odp_buffer_pool_lookup(). > > + * > > + * @note The shm parameter is used to specify whether the pool should be > > + * created in an application-supplied shared memory object. If > specified as > > nit: Two spaces after sentence > > > + * ODP_SHM_NULL, the implementation will allocate a shared memory > object to > > + * contain the pool. > > + * > > + * @note The parameter structure specifies the type of buffer to be > contained > > Maybe 'type of buffers' is better, next piece is 'their number, size, > and alignment' > > > + * in the pool as well as their number, size, and alignment. The > specified > > + * buf_align MUST be a power of two, and is the minimum alignment > requested by > > + * the caller. The buf_align parameter MAY be specified as 0 to > request that > > nit: two spaces again > > > + * the implementation use a default alignment which MUST be a minimum > of 8 > > + * bytes. > > */ > > + > > odp_buffer_pool_t odp_buffer_pool_create(const char *name, > > - void *base_addr, uint64_t size, > > - size_t buf_size, size_t > buf_align, > > - int buf_type); > > + odp_shm_t shm, > > + odp_buffer_pool_param_t > *params); > > > > +/** > > + * Destroy a buffer pool previously created by odp_buffer_pool_create() > > + * > > + * @param[in] pool Handle of the buffer pool to be destroyed > > + * > > + * @return 0 on Success, -1 on Failure. > > + * > > + * @note This routine destroys a previously created buffer pool. This > call > > nit: two spaces > > > + * does not destroy any shared memory object passed to > > + * odp_buffer_pool_create() used to store the buffer pool contents. The > caller > > + * takes responsibility for that. If no shared memory object was > passed as > > nit: 2 x <SPACE> > > > + * part of the create call, then this routine will destroy any internal > shared > > + * memory objects associated with the buffer pool. Results are > undefined if > > nit: you know the drill by now > > > + * an attempt is made to destroy a buffer pool that contains allocated > or > > + * otherwise active buffers. > > + */ > > +int odp_buffer_pool_destroy(odp_buffer_pool_t pool); > > > > /** > > * Find a buffer pool by name > > * > > - * @param name Name of the pool > > + * @param[in] name Name of the pool > > * > > * @return Buffer pool handle, or ODP_BUFFER_POOL_INVALID if not found. > > + * > > + * @note This routine cannot be used to look up an anonymous pool (one > created > > + * with no name). > > */ > > odp_buffer_pool_t odp_buffer_pool_lookup(const char *name); > > > > +/** > > + * @param[out] name Pointer to the name of the buffer pool. NULL if > this > > + * pool is anonymous. > > + * > > + * @param[out] shm Handle of the shared memory object containing > this pool, > > + * if pool was created from an application supplied > shared > > + * memory area. Otherwise ODP_SHM_NULL. > > + * > > + * @param[out] params Copy of the odp_buffer_pool_param_t used to > create this > > + * pool. > > + */ > > +typedef struct odp_buffer_pool_info_t { > > + const char *name; > > + odp_buffer_pool_param_t params; > > +} odp_buffer_pool_info_t; > > + > > +/** > > + * Retrieve information about a buffer pool > > + * > > + * @param[in] pool Buffer pool handle > > + * > > + * @param[out] shm Recieves odp_shm_t supplied by caller at > > nit: Receives > > > + * pool creation, or ODP_SHM_NULL if the > > + * pool is managed internally. > > + * > > + * @param[out] info Receives an odp_buffer_pool_info_t object > > + * that describes the pool. > > + * > > + * @return 0 on success, -1 if info could not be retrieved. > > + */ > > + > > +int odp_buffer_pool_info(odp_buffer_pool_t pool, odp_shm_t *shm, > > + odp_buffer_pool_info_t *info); > > > > /** > > * Print buffer pool info > > diff --git a/platform/linux-generic/include/api/odp_platform_types.h > b/platform/linux-generic/include/api/odp_platform_types.h > > index 4db47d3..b9b3aea 100644 > > --- a/platform/linux-generic/include/api/odp_platform_types.h > > +++ b/platform/linux-generic/include/api/odp_platform_types.h > > @@ -65,6 +65,15 @@ typedef uint32_t odp_pktio_t; > > #define ODP_PKTIO_ANY ((odp_pktio_t)~0) > > > > /** > > + * ODP shared memory block > > + */ > > +typedef uint32_t odp_shm_t; > > + > > +/** Invalid shared memory block */ > > +#define ODP_SHM_INVALID 0 > > +#define ODP_SHM_NULL ODP_SHM_INVALID /**< Synonym for buffer pool use */ > > + > > +/** > > * @} > > */ > > > > diff --git a/platform/linux-generic/include/api/odp_shared_memory.h > b/platform/linux-generic/include/api/odp_shared_memory.h > > index ff6f9a9..b681860 100644 > > --- a/platform/linux-generic/include/api/odp_shared_memory.h > > +++ b/platform/linux-generic/include/api/odp_shared_memory.h > > @@ -20,6 +20,7 @@ extern "C" { > > > > > > #include <odp_std_types.h> > > +#include <odp_platform_types.h> > > > > /** @defgroup odp_shared_memory ODP SHARED MEMORY > > * Operations on shared memory. > > @@ -38,15 +39,6 @@ extern "C" { > > #define ODP_SHM_PROC 0x2 /**< Share with external processes */ > > > > /** > > - * ODP shared memory block > > - */ > > -typedef uint32_t odp_shm_t; > > - > > -/** Invalid shared memory block */ > > -#define ODP_SHM_INVALID 0 > > This should be in a new patch, similar to the refactoring of > implementation specific typedefs and defines that you sent for > buffers, buffer pools, packets and packet_io. But I think it's ok to > send it in this series if you don't want a separately tracked patch. > > > - > > - > > -/** > > * Shared memory block info > > */ > > typedef struct odp_shm_info_t { > > diff --git a/platform/linux-generic/include/odp_buffer_pool_internal.h > b/platform/linux-generic/include/odp_buffer_pool_internal.h > > index e0210bd..a1c0990 100644 > > --- a/platform/linux-generic/include/odp_buffer_pool_internal.h > > +++ b/platform/linux-generic/include/odp_buffer_pool_internal.h > > @@ -56,6 +56,14 @@ struct pool_entry_s { > > size_t buf_size; > > size_t buf_offset; > > uint64_t num_bufs; > > + odp_shm_t pool_shm; > > + union { > > + uint32_t all; > > + struct { > > + uint32_t has_name:1; > > + uint32_t user_supplied_shm:1; > > + }; > > + } flags; > > void *pool_base_addr; > > uint64_t pool_size; > > size_t user_size; > > diff --git a/platform/linux-generic/odp_buffer_pool.c > b/platform/linux-generic/odp_buffer_pool.c > > index a48d7d6..88497a9 100644 > > --- a/platform/linux-generic/odp_buffer_pool.c > > +++ b/platform/linux-generic/odp_buffer_pool.c > > @@ -369,16 +369,19 @@ static void link_bufs(pool_entry_t *pool) > > } > > } > > > > - > > odp_buffer_pool_t odp_buffer_pool_create(const char *name, > > - void *base_addr, uint64_t size, > > - size_t buf_size, size_t > buf_align, > > - int buf_type) > > + odp_shm_t shm, > > + odp_buffer_pool_param_t *params) > > { > > odp_buffer_pool_t pool_hdl = ODP_BUFFER_POOL_INVALID; > > pool_entry_t *pool; > > + void *base_addr; > > + size_t base_size; > > uint32_t i; > > > > + if (params == NULL) > > + return ODP_BUFFER_POOL_INVALID; > > + > > for (i = 0; i < ODP_CONFIG_BUFFER_POOLS; i++) { > > pool = get_pool_entry(i); > > > > @@ -386,15 +389,49 @@ odp_buffer_pool_t odp_buffer_pool_create(const > char *name, > > > > if (pool->s.buf_base == 0) { > > /* found free pool */ > > + pool->s.flags.all = 0; > > + > > + if (name == NULL) { > > + pool->s.name[0] = 0; > > + } else { > > + strncpy(pool->s.name, name, > > + ODP_BUFFER_POOL_NAME_LEN - 1); > > + pool->s.name[ODP_BUFFER_POOL_NAME_LEN - > 1] = 0; > > + pool->s.flags.has_name = 1; > > + } > > + > > + pool->s.pool_shm = shm; > > This has to be moved further down, if shm is NULL you get a bug. > > > + > > + if (shm == ODP_SHM_NULL) { > > + base_size = > > + ODP_PAGE_SIZE_ROUNDUP( > > + params->buf_size * > > + params->num_bufs); > > + shm = odp_shm_reserve(pool->s.name, > > + base_size, > > + ODP_PAGE_SIZE, 0); > > + if (shm == ODP_SHM_INVALID) { > > + UNLOCK(&pool->s.lock); > > + return ODP_BUFFER_INVALID; > > + } > > + } else { > > + odp_shm_info_t info; > > + if (odp_shm_info(shm, &info) != 0) { > > + UNLOCK(&pool->s.lock); > > + return ODP_BUFFER_POOL_INVALID; > > + } > > + base_size = info.size; > > + pool->s.flags.user_supplied_shm = 1; > > + } > > + > > + base_addr = odp_shm_addr(shm); > > > > - strncpy(pool->s.name, name, > > - ODP_BUFFER_POOL_NAME_LEN - 1); > > - pool->s.name[ODP_BUFFER_POOL_NAME_LEN - 1] = 0; > > pool->s.pool_base_addr = base_addr; > > - pool->s.pool_size = size; > > - pool->s.user_size = buf_size; > > - pool->s.user_align = buf_align; > > - pool->s.buf_type = buf_type; > > + pool->s.pool_size = base_size; > > + pool->s.user_size = params->buf_size; > > + pool->s.user_align = params->buf_align == 0 ? > > + ODP_CACHE_LINE_SIZE : params->buf_align; > > + pool->s.buf_type = params->buf_type; > > > > link_bufs(pool); > > > > @@ -431,6 +468,46 @@ odp_buffer_pool_t odp_buffer_pool_lookup(const char > *name) > > return ODP_BUFFER_POOL_INVALID; > > } > > > > +int odp_buffer_pool_info(odp_buffer_pool_t pool_hdl, > > + odp_shm_t *shm, > > + odp_buffer_pool_info_t *info) > > +{ > > + uint32_t pool_id = pool_handle_to_index(pool_hdl); > > + pool_entry_t *pool = get_pool_entry(pool_id); > > + > > + if (pool == NULL || info == NULL) > > + return -1; > > + > > + *shm = pool->s.flags.user_supplied_shm ? > > + pool->s.pool_shm : ODP_SHM_NULL; > > + info->name = pool->s.name; > > + info->params.buf_size = pool->s.buf_size; > > + info->params.buf_align = pool->s.user_align; > > + info->params.num_bufs = pool->s.num_bufs; > > + info->params.buf_type = pool->s.buf_type; > > + > > + return 0; > > +} > > + > > +int odp_buffer_pool_destroy(odp_buffer_pool_t pool_hdl) > > +{ > > + uint32_t pool_id = pool_handle_to_index(pool_hdl); > > + pool_entry_t *pool = get_pool_entry(pool_id); > > + > > + if (pool == NULL) > > + return -1; > > + > > + LOCK(&pool->s.lock); > > + > > + if (!pool->s.flags.user_supplied_shm) > > + odp_shm_free(pool->s.pool_shm); > > + > > + pool->s.buf_base = 0; > > + UNLOCK(&pool->s.lock); > > + > > + return 0; > > +} > > + > > > > odp_buffer_t odp_buffer_alloc(odp_buffer_pool_t pool_hdl) > > { > > diff --git a/platform/linux-generic/odp_schedule.c > b/platform/linux-generic/odp_schedule.c > > index 1bf819b..85e249f 100644 > > --- a/platform/linux-generic/odp_schedule.c > > +++ b/platform/linux-generic/odp_schedule.c > > @@ -83,8 +83,8 @@ int odp_schedule_init_global(void) > > { > > odp_shm_t shm; > > odp_buffer_pool_t pool; > > - void *pool_base; > > int i, j; > > + odp_buffer_pool_param_t params; > > > > ODP_DBG("Schedule init ... "); > > > > @@ -99,20 +99,12 @@ int odp_schedule_init_global(void) > > return -1; > > } > > > > - shm = odp_shm_reserve("odp_sched_pool", > > - SCHED_POOL_SIZE, ODP_CACHE_LINE_SIZE, 0); > > + params.buf_size = sizeof(queue_desc_t); > > + params.buf_align = ODP_CACHE_LINE_SIZE; > > + params.num_bufs = SCHED_POOL_SIZE/sizeof(queue_desc_t); > > + params.buf_type = ODP_BUFFER_TYPE_RAW; > > > > - pool_base = odp_shm_addr(shm); > > - > > - if (pool_base == NULL) { > > - ODP_ERR("Schedule init: Shm reserve failed.\n"); > > - return -1; > > - } > > - > > - pool = odp_buffer_pool_create("odp_sched_pool", pool_base, > > - SCHED_POOL_SIZE, > sizeof(queue_desc_t), > > - ODP_CACHE_LINE_SIZE, > > - ODP_BUFFER_TYPE_RAW); > > + pool = odp_buffer_pool_create("odp_sched_pool", ODP_SHM_NULL, > ¶ms); > > > > if (pool == ODP_BUFFER_POOL_INVALID) { > > ODP_ERR("Schedule init: Pool create failed.\n"); > > -- > > 1.8.3.2 > > > > > > _______________________________________________ > > lng-odp mailing list > > lng-odp@lists.linaro.org > > http://lists.linaro.org/mailman/listinfo/lng-odp >
On 19 November 2014 07:13, Bill Fischofer <bill.fischofer@linaro.org> wrote: > On the two spaces. If we want to enforce this rule (which is contrary to > normal English writing style), we need up update checkpatch. The patch > submission criteria is that patches are checkpatch-clean. Now we want to > add additional criteria on top of this. That's awkward. > > Can checkpatch be updated? If not, I suggest we drop this nit picking. > Checkpatch is only intended to find the basic problems with style so I am not surprised we have additional ones championed by various people. As we mature the rules will become very clear in our community but even then I doubt we can make a tool that replaces the human eye completely. However we do have the check_odp tool suite which is maturing, patches for it are welcome. The suite exceeds the capabilities of checkpatch and it would be a good place to add a grep to look of double spaces after a period in a comment block. On the double space after a period I think most of our code has adopted it and thus it is our de facto standard. It is easy to comply with using global replace. > > Bill > > On Wed, Nov 19, 2014 at 5:02 AM, Ciprian Barbu <ciprian.barbu@linaro.org> > wrote: > >> Hi, >> >> I think you could have squashed the two patches, it's easy to figure >> out that there are changes in test apps and changes in API files. The >> combined patches don't break the build though. >> >> See my comments below: >> >> On Wed, Nov 19, 2014 at 3:21 AM, Bill Fischofer >> <bill.fischofer@linaro.org> wrote: >> > Signed-off-by: Bill Fischofer <bill.fischofer@linaro.org> >> > --- >> > .../linux-generic/include/api/odp_buffer_pool.h | 111 >> +++++++++++++++++++-- >> > .../linux-generic/include/api/odp_platform_types.h | 9 ++ >> > .../linux-generic/include/api/odp_shared_memory.h | 10 +- >> > .../include/odp_buffer_pool_internal.h | 8 ++ >> > platform/linux-generic/odp_buffer_pool.c | 99 >> ++++++++++++++++-- >> > platform/linux-generic/odp_schedule.c | 20 ++-- >> > 6 files changed, 212 insertions(+), 45 deletions(-) >> > >> > diff --git a/platform/linux-generic/include/api/odp_buffer_pool.h >> b/platform/linux-generic/include/api/odp_buffer_pool.h >> > index 30b83e0..d87872b 100644 >> > --- a/platform/linux-generic/include/api/odp_buffer_pool.h >> > +++ b/platform/linux-generic/include/api/odp_buffer_pool.h >> > @@ -36,32 +36,121 @@ extern "C" { >> > #define ODP_BUFFER_POOL_INVALID 0 >> > >> > /** >> > + * Buffer pool parameters >> > + * >> > + * @param[in] buf_size Size of application data in each buffer >> > + * @param[in] buf_align Alignment requested for buffer. Specified >> > + * as a power-of-2 integer (e.g., 8, 16, 64). >> > + * @param[in] num_bufs Number of buffers that pool should contain >> > + * @param[in] buf_type Buffer type >> > + */ >> > +typedef struct odp_buffer_pool_param_t { >> > + size_t buf_size; /**< Application data size of each >> buffer */ >> > + size_t buf_align; /**< Requested buffer alignment */ >> > + uint32_t num_bufs; /**< Number of buffers in this pool >> */ >> > + int buf_type; /**< Buffer type */ >> > +} odp_buffer_pool_param_t; /**< Type of buffer pool parameter >> struct */ >> > + >> > +/** >> > * Create a buffer pool >> > * >> > - * @param name Name of the pool (max ODP_BUFFER_POOL_NAME_LEN - 1 >> chars) >> > - * @param base_addr Pool base address >> > - * @param size Pool size in bytes >> > - * @param buf_size Buffer size in bytes >> > - * @param buf_align Minimum buffer alignment >> > - * @param buf_type Buffer type >> > + * @param[in] name Name of the pool, max >> ODP_BUFFER_POOL_NAME_LEN-1 chars. >> > + * May be specified as NULL for anonymous pools. >> > + * >> > + * @param[in] shm The shared memory object in which to create the >> pool. >> > + * May be specified as ODP_SHM_NULL to request >> that the >> > + * storage needed for the pool be allocated by the >> API. >> > + * >> > + * @param[in] params Parameters controlling the creation of this >> buffer pool. >> > + * >> > + * @return Buffer pool handle or ODP_BUFFER_POOL_NULL if call failed. >> > * >> > - * @return Buffer pool handle >> > + * @note This routine is used to create a buffer pool. It take three >> >> nit: It takes three >> >> > + * arguments: the optional name of the pool to be created, an optional >> shared >> > + * memory handle, and a parameter struct that describes the pool to be >> > + * created. If a name is not specified the result is an anonymous >> pool that >> > + * cannot be referenced by odp_buffer_pool_lookup(). >> > + * >> > + * @note The shm parameter is used to specify whether the pool should >> be >> > + * created in an application-supplied shared memory object. If >> specified as >> >> nit: Two spaces after sentence >> >> > + * ODP_SHM_NULL, the implementation will allocate a shared memory >> object to >> > + * contain the pool. >> > + * >> > + * @note The parameter structure specifies the type of buffer to be >> contained >> >> Maybe 'type of buffers' is better, next piece is 'their number, size, >> and alignment' >> >> > + * in the pool as well as their number, size, and alignment. The >> specified >> > + * buf_align MUST be a power of two, and is the minimum alignment >> requested by >> > + * the caller. The buf_align parameter MAY be specified as 0 to >> request that >> >> nit: two spaces again >> >> > + * the implementation use a default alignment which MUST be a minimum >> of 8 >> > + * bytes. >> > */ >> > + >> > odp_buffer_pool_t odp_buffer_pool_create(const char *name, >> > - void *base_addr, uint64_t size, >> > - size_t buf_size, size_t >> buf_align, >> > - int buf_type); >> > + odp_shm_t shm, >> > + odp_buffer_pool_param_t >> *params); >> > >> > +/** >> > + * Destroy a buffer pool previously created by odp_buffer_pool_create() >> > + * >> > + * @param[in] pool Handle of the buffer pool to be destroyed >> > + * >> > + * @return 0 on Success, -1 on Failure. >> > + * >> > + * @note This routine destroys a previously created buffer pool. This >> call >> >> nit: two spaces >> >> > + * does not destroy any shared memory object passed to >> > + * odp_buffer_pool_create() used to store the buffer pool contents. >> The caller >> > + * takes responsibility for that. If no shared memory object was >> passed as >> >> nit: 2 x <SPACE> >> >> > + * part of the create call, then this routine will destroy any >> internal shared >> > + * memory objects associated with the buffer pool. Results are >> undefined if >> >> nit: you know the drill by now >> >> > + * an attempt is made to destroy a buffer pool that contains allocated >> or >> > + * otherwise active buffers. >> > + */ >> > +int odp_buffer_pool_destroy(odp_buffer_pool_t pool); >> > >> > /** >> > * Find a buffer pool by name >> > * >> > - * @param name Name of the pool >> > + * @param[in] name Name of the pool >> > * >> > * @return Buffer pool handle, or ODP_BUFFER_POOL_INVALID if not found. >> > + * >> > + * @note This routine cannot be used to look up an anonymous pool (one >> created >> > + * with no name). >> > */ >> > odp_buffer_pool_t odp_buffer_pool_lookup(const char *name); >> > >> > +/** >> > + * @param[out] name Pointer to the name of the buffer pool. NULL if >> this >> > + * pool is anonymous. >> > + * >> > + * @param[out] shm Handle of the shared memory object containing >> this pool, >> > + * if pool was created from an application supplied >> shared >> > + * memory area. Otherwise ODP_SHM_NULL. >> > + * >> > + * @param[out] params Copy of the odp_buffer_pool_param_t used to >> create this >> > + * pool. >> > + */ >> > +typedef struct odp_buffer_pool_info_t { >> > + const char *name; >> > + odp_buffer_pool_param_t params; >> > +} odp_buffer_pool_info_t; >> > + >> > +/** >> > + * Retrieve information about a buffer pool >> > + * >> > + * @param[in] pool Buffer pool handle >> > + * >> > + * @param[out] shm Recieves odp_shm_t supplied by caller at >> >> nit: Receives >> >> > + * pool creation, or ODP_SHM_NULL if the >> > + * pool is managed internally. >> > + * >> > + * @param[out] info Receives an odp_buffer_pool_info_t object >> > + * that describes the pool. >> > + * >> > + * @return 0 on success, -1 if info could not be retrieved. >> > + */ >> > + >> > +int odp_buffer_pool_info(odp_buffer_pool_t pool, odp_shm_t *shm, >> > + odp_buffer_pool_info_t *info); >> > >> > /** >> > * Print buffer pool info >> > diff --git a/platform/linux-generic/include/api/odp_platform_types.h >> b/platform/linux-generic/include/api/odp_platform_types.h >> > index 4db47d3..b9b3aea 100644 >> > --- a/platform/linux-generic/include/api/odp_platform_types.h >> > +++ b/platform/linux-generic/include/api/odp_platform_types.h >> > @@ -65,6 +65,15 @@ typedef uint32_t odp_pktio_t; >> > #define ODP_PKTIO_ANY ((odp_pktio_t)~0) >> > >> > /** >> > + * ODP shared memory block >> > + */ >> > +typedef uint32_t odp_shm_t; >> > + >> > +/** Invalid shared memory block */ >> > +#define ODP_SHM_INVALID 0 >> > +#define ODP_SHM_NULL ODP_SHM_INVALID /**< Synonym for buffer pool use >> */ >> > + >> > +/** >> > * @} >> > */ >> > >> > diff --git a/platform/linux-generic/include/api/odp_shared_memory.h >> b/platform/linux-generic/include/api/odp_shared_memory.h >> > index ff6f9a9..b681860 100644 >> > --- a/platform/linux-generic/include/api/odp_shared_memory.h >> > +++ b/platform/linux-generic/include/api/odp_shared_memory.h >> > @@ -20,6 +20,7 @@ extern "C" { >> > >> > >> > #include <odp_std_types.h> >> > +#include <odp_platform_types.h> >> > >> > /** @defgroup odp_shared_memory ODP SHARED MEMORY >> > * Operations on shared memory. >> > @@ -38,15 +39,6 @@ extern "C" { >> > #define ODP_SHM_PROC 0x2 /**< Share with external processes */ >> > >> > /** >> > - * ODP shared memory block >> > - */ >> > -typedef uint32_t odp_shm_t; >> > - >> > -/** Invalid shared memory block */ >> > -#define ODP_SHM_INVALID 0 >> >> This should be in a new patch, similar to the refactoring of >> implementation specific typedefs and defines that you sent for >> buffers, buffer pools, packets and packet_io. But I think it's ok to >> send it in this series if you don't want a separately tracked patch. >> >> > - >> > - >> > -/** >> > * Shared memory block info >> > */ >> > typedef struct odp_shm_info_t { >> > diff --git a/platform/linux-generic/include/odp_buffer_pool_internal.h >> b/platform/linux-generic/include/odp_buffer_pool_internal.h >> > index e0210bd..a1c0990 100644 >> > --- a/platform/linux-generic/include/odp_buffer_pool_internal.h >> > +++ b/platform/linux-generic/include/odp_buffer_pool_internal.h >> > @@ -56,6 +56,14 @@ struct pool_entry_s { >> > size_t buf_size; >> > size_t buf_offset; >> > uint64_t num_bufs; >> > + odp_shm_t pool_shm; >> > + union { >> > + uint32_t all; >> > + struct { >> > + uint32_t has_name:1; >> > + uint32_t user_supplied_shm:1; >> > + }; >> > + } flags; >> > void *pool_base_addr; >> > uint64_t pool_size; >> > size_t user_size; >> > diff --git a/platform/linux-generic/odp_buffer_pool.c >> b/platform/linux-generic/odp_buffer_pool.c >> > index a48d7d6..88497a9 100644 >> > --- a/platform/linux-generic/odp_buffer_pool.c >> > +++ b/platform/linux-generic/odp_buffer_pool.c >> > @@ -369,16 +369,19 @@ static void link_bufs(pool_entry_t *pool) >> > } >> > } >> > >> > - >> > odp_buffer_pool_t odp_buffer_pool_create(const char *name, >> > - void *base_addr, uint64_t size, >> > - size_t buf_size, size_t >> buf_align, >> > - int buf_type) >> > + odp_shm_t shm, >> > + odp_buffer_pool_param_t >> *params) >> > { >> > odp_buffer_pool_t pool_hdl = ODP_BUFFER_POOL_INVALID; >> > pool_entry_t *pool; >> > + void *base_addr; >> > + size_t base_size; >> > uint32_t i; >> > >> > + if (params == NULL) >> > + return ODP_BUFFER_POOL_INVALID; >> > + >> > for (i = 0; i < ODP_CONFIG_BUFFER_POOLS; i++) { >> > pool = get_pool_entry(i); >> > >> > @@ -386,15 +389,49 @@ odp_buffer_pool_t odp_buffer_pool_create(const >> char *name, >> > >> > if (pool->s.buf_base == 0) { >> > /* found free pool */ >> > + pool->s.flags.all = 0; >> > + >> > + if (name == NULL) { >> > + pool->s.name[0] = 0; >> > + } else { >> > + strncpy(pool->s.name, name, >> > + ODP_BUFFER_POOL_NAME_LEN - 1); >> > + pool->s.name[ODP_BUFFER_POOL_NAME_LEN >> - 1] = 0; >> > + pool->s.flags.has_name = 1; >> > + } >> > + >> > + pool->s.pool_shm = shm; >> >> This has to be moved further down, if shm is NULL you get a bug. >> >> > + >> > + if (shm == ODP_SHM_NULL) { >> > + base_size = >> > + ODP_PAGE_SIZE_ROUNDUP( >> > + params->buf_size * >> > + params->num_bufs); >> > + shm = odp_shm_reserve(pool->s.name, >> > + base_size, >> > + ODP_PAGE_SIZE, 0); >> > + if (shm == ODP_SHM_INVALID) { >> > + UNLOCK(&pool->s.lock); >> > + return ODP_BUFFER_INVALID; >> > + } >> > + } else { >> > + odp_shm_info_t info; >> > + if (odp_shm_info(shm, &info) != 0) { >> > + UNLOCK(&pool->s.lock); >> > + return ODP_BUFFER_POOL_INVALID; >> > + } >> > + base_size = info.size; >> > + pool->s.flags.user_supplied_shm = 1; >> > + } >> > + >> > + base_addr = odp_shm_addr(shm); >> > >> > - strncpy(pool->s.name, name, >> > - ODP_BUFFER_POOL_NAME_LEN - 1); >> > - pool->s.name[ODP_BUFFER_POOL_NAME_LEN - 1] = 0; >> > pool->s.pool_base_addr = base_addr; >> > - pool->s.pool_size = size; >> > - pool->s.user_size = buf_size; >> > - pool->s.user_align = buf_align; >> > - pool->s.buf_type = buf_type; >> > + pool->s.pool_size = base_size; >> > + pool->s.user_size = params->buf_size; >> > + pool->s.user_align = params->buf_align == 0 >> ? >> > + ODP_CACHE_LINE_SIZE : params->buf_align; >> > + pool->s.buf_type = params->buf_type; >> > >> > link_bufs(pool); >> > >> > @@ -431,6 +468,46 @@ odp_buffer_pool_t odp_buffer_pool_lookup(const >> char *name) >> > return ODP_BUFFER_POOL_INVALID; >> > } >> > >> > +int odp_buffer_pool_info(odp_buffer_pool_t pool_hdl, >> > + odp_shm_t *shm, >> > + odp_buffer_pool_info_t *info) >> > +{ >> > + uint32_t pool_id = pool_handle_to_index(pool_hdl); >> > + pool_entry_t *pool = get_pool_entry(pool_id); >> > + >> > + if (pool == NULL || info == NULL) >> > + return -1; >> > + >> > + *shm = pool->s.flags.user_supplied_shm ? >> > + pool->s.pool_shm : ODP_SHM_NULL; >> > + info->name = pool->s.name; >> > + info->params.buf_size = pool->s.buf_size; >> > + info->params.buf_align = pool->s.user_align; >> > + info->params.num_bufs = pool->s.num_bufs; >> > + info->params.buf_type = pool->s.buf_type; >> > + >> > + return 0; >> > +} >> > + >> > +int odp_buffer_pool_destroy(odp_buffer_pool_t pool_hdl) >> > +{ >> > + uint32_t pool_id = pool_handle_to_index(pool_hdl); >> > + pool_entry_t *pool = get_pool_entry(pool_id); >> > + >> > + if (pool == NULL) >> > + return -1; >> > + >> > + LOCK(&pool->s.lock); >> > + >> > + if (!pool->s.flags.user_supplied_shm) >> > + odp_shm_free(pool->s.pool_shm); >> > + >> > + pool->s.buf_base = 0; >> > + UNLOCK(&pool->s.lock); >> > + >> > + return 0; >> > +} >> > + >> > >> > odp_buffer_t odp_buffer_alloc(odp_buffer_pool_t pool_hdl) >> > { >> > diff --git a/platform/linux-generic/odp_schedule.c >> b/platform/linux-generic/odp_schedule.c >> > index 1bf819b..85e249f 100644 >> > --- a/platform/linux-generic/odp_schedule.c >> > +++ b/platform/linux-generic/odp_schedule.c >> > @@ -83,8 +83,8 @@ int odp_schedule_init_global(void) >> > { >> > odp_shm_t shm; >> > odp_buffer_pool_t pool; >> > - void *pool_base; >> > int i, j; >> > + odp_buffer_pool_param_t params; >> > >> > ODP_DBG("Schedule init ... "); >> > >> > @@ -99,20 +99,12 @@ int odp_schedule_init_global(void) >> > return -1; >> > } >> > >> > - shm = odp_shm_reserve("odp_sched_pool", >> > - SCHED_POOL_SIZE, ODP_CACHE_LINE_SIZE, 0); >> > + params.buf_size = sizeof(queue_desc_t); >> > + params.buf_align = ODP_CACHE_LINE_SIZE; >> > + params.num_bufs = SCHED_POOL_SIZE/sizeof(queue_desc_t); >> > + params.buf_type = ODP_BUFFER_TYPE_RAW; >> > >> > - pool_base = odp_shm_addr(shm); >> > - >> > - if (pool_base == NULL) { >> > - ODP_ERR("Schedule init: Shm reserve failed.\n"); >> > - return -1; >> > - } >> > - >> > - pool = odp_buffer_pool_create("odp_sched_pool", pool_base, >> > - SCHED_POOL_SIZE, >> sizeof(queue_desc_t), >> > - ODP_CACHE_LINE_SIZE, >> > - ODP_BUFFER_TYPE_RAW); >> > + pool = odp_buffer_pool_create("odp_sched_pool", ODP_SHM_NULL, >> ¶ms); >> > >> > if (pool == ODP_BUFFER_POOL_INVALID) { >> > ODP_ERR("Schedule init: Pool create failed.\n"); >> > -- >> > 1.8.3.2 >> > >> > >> > _______________________________________________ >> > lng-odp mailing list >> > lng-odp@lists.linaro.org >> > http://lists.linaro.org/mailman/listinfo/lng-odp >> > > > _______________________________________________ > lng-odp mailing list > lng-odp@lists.linaro.org > http://lists.linaro.org/mailman/listinfo/lng-odp > >
> -----Original Message----- > From: lng-odp-bounces@lists.linaro.org [mailto:lng-odp- > bounces@lists.linaro.org] On Behalf Of ext Bill Fischofer > Sent: Wednesday, November 19, 2014 3:21 AM > To: lng-odp@lists.linaro.org > Subject: [lng-odp] [PATCH 2/2] Changes for v1.0 buffer pool APIs > > Signed-off-by: Bill Fischofer <bill.fischofer@linaro.org> > --- > .../linux-generic/include/api/odp_buffer_pool.h | 111 > +++++++++++++++++++-- > .../linux-generic/include/api/odp_platform_types.h | 9 ++ > .../linux-generic/include/api/odp_shared_memory.h | 10 +- > .../include/odp_buffer_pool_internal.h | 8 ++ > platform/linux-generic/odp_buffer_pool.c | 99 > ++++++++++++++++-- > platform/linux-generic/odp_schedule.c | 20 ++-- > 6 files changed, 212 insertions(+), 45 deletions(-) > > diff --git a/platform/linux-generic/include/api/odp_buffer_pool.h > b/platform/linux-generic/include/api/odp_buffer_pool.h > index 30b83e0..d87872b 100644 > --- a/platform/linux-generic/include/api/odp_buffer_pool.h > +++ b/platform/linux-generic/include/api/odp_buffer_pool.h > @@ -36,32 +36,121 @@ extern "C" { > #define ODP_BUFFER_POOL_INVALID 0 > > /** > + * Buffer pool parameters > + * > + * @param[in] buf_size Size of application data in each buffer "Buffer size in bytes" == maximum number of bytes app will store in each buffer > + * @param[in] buf_align Alignment requested for buffer. Specified > + * as a power-of-2 integer (e.g., 8, 16, 64). "Minimum buffer alignment in bytes. Valid values are power of twos. Use 0 for the default alignment. Default alignment is a multiple of 8." > + * @param[in] num_bufs Number of buffers that pool should contain "Number of buffers in the pool" > + * @param[in] buf_type Buffer type > + */ > +typedef struct odp_buffer_pool_param_t { > + size_t buf_size; /**< Application data size of each > buffer */ > + size_t buf_align; /**< Requested buffer alignment */ > + uint32_t num_bufs; /**< Number of buffers in this pool */ > + int buf_type; /**< Buffer type */ > +} odp_buffer_pool_param_t; /**< Type of buffer pool parameter > struct */ Parameters are documented twice - remove extra copy. > + > +/** > * Create a buffer pool > * > - * @param name Name of the pool (max ODP_BUFFER_POOL_NAME_LEN - 1 > chars) > - * @param base_addr Pool base address > - * @param size Pool size in bytes > - * @param buf_size Buffer size in bytes > - * @param buf_align Minimum buffer alignment > - * @param buf_type Buffer type > + * @param[in] name Name of the pool, max ODP_BUFFER_POOL_NAME_LEN-1 > chars. > + * May be specified as NULL for anonymous pools. > + * > + * @param[in] shm The shared memory object in which to create the > pool. > + * May be specified as ODP_SHM_NULL to request that > the > + * storage needed for the pool be allocated by the > API. "Use ODP_SHM_NULL to reserve default memory type for the buffer type" ... or something similar > + * > + * @param[in] params Parameters controlling the creation of this buffer > pool. "Buffer pool parameters" > + * > + * @return Buffer pool handle or ODP_BUFFER_POOL_NULL if call failed or ODP_BUFFER_POOL_INVALID . > * > - * @return Buffer pool handle > + * @note This routine is used to create a buffer pool. It take three > + * arguments: the optional name of the pool to be created, an optional > shared > + * memory handle, and a parameter struct that describes the pool to be > + * created. If a name is not specified the result is an anonymous pool > that > + * cannot be referenced by odp_buffer_pool_lookup(). > + * > + * @note The shm parameter is used to specify whether the pool should be > + * created in an application-supplied shared memory object. If specified > as > + * ODP_SHM_NULL, the implementation will allocate a shared memory object > to > + * contain the pool. "Will allocate default memory for the pool" Compress text above, remove @note's and move as the "long" description under the "short" description (== first line after /**). Avoid repeating specification, but tell purpose of the function. > + * > + * @note The parameter structure specifies the type of buffer to be > contained > + * in the pool as well as their number, size, and alignment. The > specified > + * buf_align MUST be a power of two, and is the minimum alignment > requested by > + * the caller. The buf_align parameter MAY be specified as 0 to request > that > + * the implementation use a default alignment which MUST be a minimum of > 8 > + * bytes. > */ This text above can be removed (repeats the argument spec). > + > odp_buffer_pool_t odp_buffer_pool_create(const char *name, > - void *base_addr, uint64_t size, > - size_t buf_size, size_t buf_align, > - int buf_type); > + odp_shm_t shm, > + odp_buffer_pool_param_t *params); > -Petri
diff --git a/platform/linux-generic/include/api/odp_buffer_pool.h b/platform/linux-generic/include/api/odp_buffer_pool.h index 30b83e0..d87872b 100644 --- a/platform/linux-generic/include/api/odp_buffer_pool.h +++ b/platform/linux-generic/include/api/odp_buffer_pool.h @@ -36,32 +36,121 @@ extern "C" { #define ODP_BUFFER_POOL_INVALID 0 /** + * Buffer pool parameters + * + * @param[in] buf_size Size of application data in each buffer + * @param[in] buf_align Alignment requested for buffer. Specified + * as a power-of-2 integer (e.g., 8, 16, 64). + * @param[in] num_bufs Number of buffers that pool should contain + * @param[in] buf_type Buffer type + */ +typedef struct odp_buffer_pool_param_t { + size_t buf_size; /**< Application data size of each buffer */ + size_t buf_align; /**< Requested buffer alignment */ + uint32_t num_bufs; /**< Number of buffers in this pool */ + int buf_type; /**< Buffer type */ +} odp_buffer_pool_param_t; /**< Type of buffer pool parameter struct */ + +/** * Create a buffer pool * - * @param name Name of the pool (max ODP_BUFFER_POOL_NAME_LEN - 1 chars) - * @param base_addr Pool base address - * @param size Pool size in bytes - * @param buf_size Buffer size in bytes - * @param buf_align Minimum buffer alignment - * @param buf_type Buffer type + * @param[in] name Name of the pool, max ODP_BUFFER_POOL_NAME_LEN-1 chars. + * May be specified as NULL for anonymous pools. + * + * @param[in] shm The shared memory object in which to create the pool. + * May be specified as ODP_SHM_NULL to request that the + * storage needed for the pool be allocated by the API. + * + * @param[in] params Parameters controlling the creation of this buffer pool. + * + * @return Buffer pool handle or ODP_BUFFER_POOL_NULL if call failed. * - * @return Buffer pool handle + * @note This routine is used to create a buffer pool. It take three + * arguments: the optional name of the pool to be created, an optional shared + * memory handle, and a parameter struct that describes the pool to be + * created. If a name is not specified the result is an anonymous pool that + * cannot be referenced by odp_buffer_pool_lookup(). + * + * @note The shm parameter is used to specify whether the pool should be + * created in an application-supplied shared memory object. If specified as + * ODP_SHM_NULL, the implementation will allocate a shared memory object to + * contain the pool. + * + * @note The parameter structure specifies the type of buffer to be contained + * in the pool as well as their number, size, and alignment. The specified + * buf_align MUST be a power of two, and is the minimum alignment requested by + * the caller. The buf_align parameter MAY be specified as 0 to request that + * the implementation use a default alignment which MUST be a minimum of 8 + * bytes. */ + odp_buffer_pool_t odp_buffer_pool_create(const char *name, - void *base_addr, uint64_t size, - size_t buf_size, size_t buf_align, - int buf_type); + odp_shm_t shm, + odp_buffer_pool_param_t *params); +/** + * Destroy a buffer pool previously created by odp_buffer_pool_create() + * + * @param[in] pool Handle of the buffer pool to be destroyed + * + * @return 0 on Success, -1 on Failure. + * + * @note This routine destroys a previously created buffer pool. This call + * does not destroy any shared memory object passed to + * odp_buffer_pool_create() used to store the buffer pool contents. The caller + * takes responsibility for that. If no shared memory object was passed as + * part of the create call, then this routine will destroy any internal shared + * memory objects associated with the buffer pool. Results are undefined if + * an attempt is made to destroy a buffer pool that contains allocated or + * otherwise active buffers. + */ +int odp_buffer_pool_destroy(odp_buffer_pool_t pool); /** * Find a buffer pool by name * - * @param name Name of the pool + * @param[in] name Name of the pool * * @return Buffer pool handle, or ODP_BUFFER_POOL_INVALID if not found. + * + * @note This routine cannot be used to look up an anonymous pool (one created + * with no name). */ odp_buffer_pool_t odp_buffer_pool_lookup(const char *name); +/** + * @param[out] name Pointer to the name of the buffer pool. NULL if this + * pool is anonymous. + * + * @param[out] shm Handle of the shared memory object containing this pool, + * if pool was created from an application supplied shared + * memory area. Otherwise ODP_SHM_NULL. + * + * @param[out] params Copy of the odp_buffer_pool_param_t used to create this + * pool. + */ +typedef struct odp_buffer_pool_info_t { + const char *name; + odp_buffer_pool_param_t params; +} odp_buffer_pool_info_t; + +/** + * Retrieve information about a buffer pool + * + * @param[in] pool Buffer pool handle + * + * @param[out] shm Recieves odp_shm_t supplied by caller at + * pool creation, or ODP_SHM_NULL if the + * pool is managed internally. + * + * @param[out] info Receives an odp_buffer_pool_info_t object + * that describes the pool. + * + * @return 0 on success, -1 if info could not be retrieved. + */ + +int odp_buffer_pool_info(odp_buffer_pool_t pool, odp_shm_t *shm, + odp_buffer_pool_info_t *info); /** * Print buffer pool info diff --git a/platform/linux-generic/include/api/odp_platform_types.h b/platform/linux-generic/include/api/odp_platform_types.h index 4db47d3..b9b3aea 100644 --- a/platform/linux-generic/include/api/odp_platform_types.h +++ b/platform/linux-generic/include/api/odp_platform_types.h @@ -65,6 +65,15 @@ typedef uint32_t odp_pktio_t; #define ODP_PKTIO_ANY ((odp_pktio_t)~0) /** + * ODP shared memory block + */ +typedef uint32_t odp_shm_t; + +/** Invalid shared memory block */ +#define ODP_SHM_INVALID 0 +#define ODP_SHM_NULL ODP_SHM_INVALID /**< Synonym for buffer pool use */ + +/** * @} */ diff --git a/platform/linux-generic/include/api/odp_shared_memory.h b/platform/linux-generic/include/api/odp_shared_memory.h index ff6f9a9..b681860 100644 --- a/platform/linux-generic/include/api/odp_shared_memory.h +++ b/platform/linux-generic/include/api/odp_shared_memory.h @@ -20,6 +20,7 @@ extern "C" { #include <odp_std_types.h> +#include <odp_platform_types.h> /** @defgroup odp_shared_memory ODP SHARED MEMORY * Operations on shared memory. @@ -38,15 +39,6 @@ extern "C" { #define ODP_SHM_PROC 0x2 /**< Share with external processes */ /** - * ODP shared memory block - */ -typedef uint32_t odp_shm_t; - -/** Invalid shared memory block */ -#define ODP_SHM_INVALID 0 - - -/** * Shared memory block info */ typedef struct odp_shm_info_t { diff --git a/platform/linux-generic/include/odp_buffer_pool_internal.h b/platform/linux-generic/include/odp_buffer_pool_internal.h index e0210bd..a1c0990 100644 --- a/platform/linux-generic/include/odp_buffer_pool_internal.h +++ b/platform/linux-generic/include/odp_buffer_pool_internal.h @@ -56,6 +56,14 @@ struct pool_entry_s { size_t buf_size; size_t buf_offset; uint64_t num_bufs; + odp_shm_t pool_shm; + union { + uint32_t all; + struct { + uint32_t has_name:1; + uint32_t user_supplied_shm:1; + }; + } flags; void *pool_base_addr; uint64_t pool_size; size_t user_size; diff --git a/platform/linux-generic/odp_buffer_pool.c b/platform/linux-generic/odp_buffer_pool.c index a48d7d6..88497a9 100644 --- a/platform/linux-generic/odp_buffer_pool.c +++ b/platform/linux-generic/odp_buffer_pool.c @@ -369,16 +369,19 @@ static void link_bufs(pool_entry_t *pool) } } - odp_buffer_pool_t odp_buffer_pool_create(const char *name, - void *base_addr, uint64_t size, - size_t buf_size, size_t buf_align, - int buf_type) + odp_shm_t shm, + odp_buffer_pool_param_t *params) { odp_buffer_pool_t pool_hdl = ODP_BUFFER_POOL_INVALID; pool_entry_t *pool; + void *base_addr; + size_t base_size; uint32_t i; + if (params == NULL) + return ODP_BUFFER_POOL_INVALID; + for (i = 0; i < ODP_CONFIG_BUFFER_POOLS; i++) { pool = get_pool_entry(i); @@ -386,15 +389,49 @@ odp_buffer_pool_t odp_buffer_pool_create(const char *name, if (pool->s.buf_base == 0) { /* found free pool */ + pool->s.flags.all = 0; + + if (name == NULL) { + pool->s.name[0] = 0; + } else { + strncpy(pool->s.name, name, + ODP_BUFFER_POOL_NAME_LEN - 1); + pool->s.name[ODP_BUFFER_POOL_NAME_LEN - 1] = 0; + pool->s.flags.has_name = 1; + } + + pool->s.pool_shm = shm; + + if (shm == ODP_SHM_NULL) { + base_size = + ODP_PAGE_SIZE_ROUNDUP( + params->buf_size * + params->num_bufs); + shm = odp_shm_reserve(pool->s.name, + base_size, + ODP_PAGE_SIZE, 0); + if (shm == ODP_SHM_INVALID) { + UNLOCK(&pool->s.lock); + return ODP_BUFFER_INVALID; + } + } else { + odp_shm_info_t info; + if (odp_shm_info(shm, &info) != 0) { + UNLOCK(&pool->s.lock); + return ODP_BUFFER_POOL_INVALID; + } + base_size = info.size; + pool->s.flags.user_supplied_shm = 1; + } + + base_addr = odp_shm_addr(shm); - strncpy(pool->s.name, name, - ODP_BUFFER_POOL_NAME_LEN - 1); - pool->s.name[ODP_BUFFER_POOL_NAME_LEN - 1] = 0; pool->s.pool_base_addr = base_addr; - pool->s.pool_size = size; - pool->s.user_size = buf_size; - pool->s.user_align = buf_align; - pool->s.buf_type = buf_type; + pool->s.pool_size = base_size; + pool->s.user_size = params->buf_size; + pool->s.user_align = params->buf_align == 0 ? + ODP_CACHE_LINE_SIZE : params->buf_align; + pool->s.buf_type = params->buf_type; link_bufs(pool); @@ -431,6 +468,46 @@ odp_buffer_pool_t odp_buffer_pool_lookup(const char *name) return ODP_BUFFER_POOL_INVALID; } +int odp_buffer_pool_info(odp_buffer_pool_t pool_hdl, + odp_shm_t *shm, + odp_buffer_pool_info_t *info) +{ + uint32_t pool_id = pool_handle_to_index(pool_hdl); + pool_entry_t *pool = get_pool_entry(pool_id); + + if (pool == NULL || info == NULL) + return -1; + + *shm = pool->s.flags.user_supplied_shm ? + pool->s.pool_shm : ODP_SHM_NULL; + info->name = pool->s.name; + info->params.buf_size = pool->s.buf_size; + info->params.buf_align = pool->s.user_align; + info->params.num_bufs = pool->s.num_bufs; + info->params.buf_type = pool->s.buf_type; + + return 0; +} + +int odp_buffer_pool_destroy(odp_buffer_pool_t pool_hdl) +{ + uint32_t pool_id = pool_handle_to_index(pool_hdl); + pool_entry_t *pool = get_pool_entry(pool_id); + + if (pool == NULL) + return -1; + + LOCK(&pool->s.lock); + + if (!pool->s.flags.user_supplied_shm) + odp_shm_free(pool->s.pool_shm); + + pool->s.buf_base = 0; + UNLOCK(&pool->s.lock); + + return 0; +} + odp_buffer_t odp_buffer_alloc(odp_buffer_pool_t pool_hdl) { diff --git a/platform/linux-generic/odp_schedule.c b/platform/linux-generic/odp_schedule.c index 1bf819b..85e249f 100644 --- a/platform/linux-generic/odp_schedule.c +++ b/platform/linux-generic/odp_schedule.c @@ -83,8 +83,8 @@ int odp_schedule_init_global(void) { odp_shm_t shm; odp_buffer_pool_t pool; - void *pool_base; int i, j; + odp_buffer_pool_param_t params; ODP_DBG("Schedule init ... "); @@ -99,20 +99,12 @@ int odp_schedule_init_global(void) return -1; } - shm = odp_shm_reserve("odp_sched_pool", - SCHED_POOL_SIZE, ODP_CACHE_LINE_SIZE, 0); + params.buf_size = sizeof(queue_desc_t); + params.buf_align = ODP_CACHE_LINE_SIZE; + params.num_bufs = SCHED_POOL_SIZE/sizeof(queue_desc_t); + params.buf_type = ODP_BUFFER_TYPE_RAW; - pool_base = odp_shm_addr(shm); - - if (pool_base == NULL) { - ODP_ERR("Schedule init: Shm reserve failed.\n"); - return -1; - } - - pool = odp_buffer_pool_create("odp_sched_pool", pool_base, - SCHED_POOL_SIZE, sizeof(queue_desc_t), - ODP_CACHE_LINE_SIZE, - ODP_BUFFER_TYPE_RAW); + pool = odp_buffer_pool_create("odp_sched_pool", ODP_SHM_NULL, ¶ms); if (pool == ODP_BUFFER_POOL_INVALID) { ODP_ERR("Schedule init: Pool create failed.\n");
Signed-off-by: Bill Fischofer <bill.fischofer@linaro.org> --- .../linux-generic/include/api/odp_buffer_pool.h | 111 +++++++++++++++++++-- .../linux-generic/include/api/odp_platform_types.h | 9 ++ .../linux-generic/include/api/odp_shared_memory.h | 10 +- .../include/odp_buffer_pool_internal.h | 8 ++ platform/linux-generic/odp_buffer_pool.c | 99 ++++++++++++++++-- platform/linux-generic/odp_schedule.c | 20 ++-- 6 files changed, 212 insertions(+), 45 deletions(-)