On 29 December 2016 at 10:19, Savolainen, Petri (Nokia - FI/Espoo)
<[email protected]> wrote:
>
>
>> -----Original Message-----
>> From: lng-odp [mailto:[email protected]] On Behalf Of
>> Christophe Milard
>> Sent: Thursday, December 29, 2016 9:57 AM
>> To: [email protected]; [email protected]; [email protected];
>> [email protected]; [email protected]
>> Subject: [lng-odp] [API-NEXT PATCHv2 2/6] drv: adding odpdrv_shm_pool
>> functions
>>
>> Adding functions to create and destroy memory pools (from which memory
>> can be allocated and freed) are added.
>> These functions enable the usage of small memory amount (compared to
>> drvshm_reserve() whose granularity is the page size).
>> The usage of this pool guatantees that allocated memory is sharable
>> between ODP threads. (using malloc would not work when ODP threads
>> are linux processes).
>>
>> Signed-off-by: Christophe Milard <[email protected]>
>> ---
>> include/odp/drv/spec/shm.h | 97
>> ++++++++++++++++++++++
>> .../linux-generic/include/odp/drv/plat/shm_types.h | 3 +
>> 2 files changed, 100 insertions(+)
>>
>> diff --git a/include/odp/drv/spec/shm.h b/include/odp/drv/spec/shm.h
>> index ef64f5d..20bbfd2 100644
>> --- a/include/odp/drv/spec/shm.h
>> +++ b/include/odp/drv/spec/shm.h
>> @@ -220,6 +220,103 @@ int odpdrv_shm_print_all(const char *title);
>> uint64_t odpdrv_shm_to_u64(odpdrv_shm_t hdl);
>>
>> /**
>> + * drv shm pool parameters
>> + * Used to communicate pool creation options.
>> + */
>> +typedef struct {
>> + /** sum of all (simultaneous) allocs (bytes)*/
>
> Capital 'S' for clean doxygen doc.
OK => V3
>
>> + uint64_t pool_size;
>> +
>> + /** Minimum alloc size application will request from pool (bytes)*/
>> + uint64_t min_alloc;
>
> Since this is a driver interface: ... size *driver* will request ...
It is not limited to drivers: other driver elements such as
enumerators, devios... will also be "clients" of this interface. Would
you accept "user" rather than "application"?
>
>> +
>> + /** Maximum alloc size application will request from pool (bytes)*/
>
> Same here.
same answer as above
>
>> + uint64_t max_alloc;
>> +} odpdrv_shm_pool_param_t;
>> +
>> +/**
>> + * @typedef odpdrv_shm_pool_t
>> + * odpdrv shared memory pool
>> + */
>> +
>> +/**
>> + * Create a memory pool
>> + *
>> + * This routine is used to create a memory pool. The use of pool name is
>> + * optional.
>> + * Unique names are not required. However, odpdrv_shm_pool_lookup()
>> + * returns only a single matching pool.
>> + *
>> + * @param name Name of the pool or NULL.
>> + * @param params Pool parameters.
>> + *
>> + * @return Handle of the created drv shm memory pool
>> + * @retval ODPDRV_SHM_POOL_INVALID Pool could not be created
>> + */
>> +odpdrv_shm_pool_t odpdrv_shm_pool_create(const char *pool_name,
>> + odpdrv_shm_pool_param_t *params);
>
> We are have chosen to use "xxx_param_t *param", not the plural "params".
OK => V3
>
>> +
>> +/**
>> + * Destroy a pool previously created by odpdrv_shm_pool_create()
>> + *
>> + * @param pool Handle of the pool to be destroyed
>> + *
>> + * @retval 0 Success
>> + * @retval -1 Failure
>
> We are using <0 for failure, implementation usually returns -1 but API is
> more portable/extendable with <0.
OK => V3
>
>
>> + *
>> + * @note This routine destroys a previously created pool, and will
>> destroy any
>> + * internal shared memory objects associated with the pool. Results are
>> + * undefined if an attempt is made to destroy a pool that contains
>> allocated
>> + * or otherwise active allocations.
>> + */
>> +int odpdrv_shm_pool_destroy(odpdrv_shm_pool_t pool);
>> +
>> +/**
>> + * Find a memory pool by name
>> + *
>> + * @param name Name of the pool
>> + *
>> + * @return Handle of the first matching pool
>> + * @retval ODPDRV_SHM_POOL_INVALID Pool could not be found
>> + */
>> +odpdrv_shm_pool_t odpdrv_shm_pool_lookup(const char *name);
>> +
>> +/**
>> + * Allocate memory from a memory pool
>> + *
>> + * @param pool Memory Pool handle
>
> Lower case 'p' for pool
OK=>V3
>
>> + * @param size Number of bytes to allocate (bytes)
>> + *
>> + * @return A pointer to the allocated memory
>> + * @retval NULL on error.
>> + */
>> +void *odpdrv_shm_pool_alloc(odpdrv_shm_pool_t pool, uint64_t size);
>> +
>> +/**
>> + * Free memory back to a memory pool
>> + *
>> + * @param pool Memory Pool handle
>
> Lower case 'p' for pool
OK => V3
>
>> + * @param addr pointer to a previously allocated memory
>> + * (as returned by a previous cal to odpdrv_shm_pool_alloc)
>
> Upper case 'P' for pointer.
>
> Typo: cal -> call
OK => V3 for both.
>
>
>
>> + *
>> + * @return A pointer to the allocated memory
>> + * @retval NULL on error.
>
> No return value. Remove these. Doesn't 'make doxygen-doc' run doxygen on this
> file, or didn't it warn about @return on a void function.
I must have missed it, of course. These should not be there. Thanks. =>V3
>
>
>> + */
>> +void odpdrv_shm_pool_free(odpdrv_shm_pool_t pool, void *addr);
>> +
>> +/**
>> + * Print memory pool info
>> + *
>> + * @param title A string to be printed as a title (e.g. location)
>> + * @param pool Memory Pool handle
>
> Lower case 'p' for pool
OK=> V3
>
>> + *
>> + * @return Non-zero value if pool inconsistency is detected
>
> 0 on success
> <0 on failure
OK=> V3
A side comment there: If we say 0 on success and <0 on failure,
do you regard the statement "if (odpdrv_shm_pool_print()) {error}" as valid?
Or do you always require "if (odpdrv_shm_pool_print() < 0) {error}" ?
The first statement would react on positive values, and nothing is
technically said about them...
I will change it as you want anyway.
>
>> + *
>> + * @note This routine writes implementation-defined information about the
>> + * specified pool to the ODP log. The intended use is for debugging.
>> + */
>> +int odpdrv_shm_pool_print(const char *title, odpdrv_shm_pool_t pool);
>> +/**
>> * @}
>> */
>
>
> -Petri
Thanks,
Christophe.
