On 19 November 2014 07:13, Bill Fischofer <[email protected]> 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 <[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 > > -- *Mike Holmes* Linaro Sr Technical Manager LNG - ODP
_______________________________________________ lng-odp mailing list [email protected] http://lists.linaro.org/mailman/listinfo/lng-odp
