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 <[email protected]> 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 > <[email protected]> wrote: > > Signed-off-by: Bill Fischofer <[email protected]> > > --- > > .../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 > > [email protected] > > http://lists.linaro.org/mailman/listinfo/lng-odp >
_______________________________________________ lng-odp mailing list [email protected] http://lists.linaro.org/mailman/listinfo/lng-odp
