| Message ID | 1482998237-36552-3-git-send-email-christophe.milard@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
> -----Original Message----- > From: lng-odp [mailto:lng-odp-bounces@lists.linaro.org] On Behalf Of > Christophe Milard > Sent: Thursday, December 29, 2016 9:57 AM > To: mike.holmes@linaro.org; bill.fischofer@linaro.org; yi.he@linaro.org; > forrest.shi@linaro.org; lng-odp@lists.linaro.org > 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 <christophe.milard@linaro.org> > --- > 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. > + 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 ... > + > + /** Maximum alloc size application will request from pool (bytes)*/ Same here. > + 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". > + > +/** > + * 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. > + * > + * @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 > + * @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 > + * @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 > + * > + * @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. > + */ > +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 > + * > + * @return Non-zero value if pool inconsistency is detected 0 on success <0 on failure > + * > + * @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
On 29 December 2016 at 10:19, Savolainen, Petri (Nokia - FI/Espoo) <petri.savolainen@nokia-bell-labs.com> wrote: > > >> -----Original Message----- >> From: lng-odp [mailto:lng-odp-bounces@lists.linaro.org] On Behalf Of >> Christophe Milard >> Sent: Thursday, December 29, 2016 9:57 AM >> To: mike.holmes@linaro.org; bill.fischofer@linaro.org; yi.he@linaro.org; >> forrest.shi@linaro.org; lng-odp@lists.linaro.org >> 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 <christophe.milard@linaro.org> >> --- >> 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.
> > > >> + 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"? I think user is OK. The main point is that application never uses/sees this interface. > > > >> + * > >> + * @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. if (foo()) is OK. Interface does not define any positive values for success either. Specification of <0 allows -1, -2, etc for failure return codes. If interface would change to have more success codes than the 0, caller would likely need to check those and change the if statement anyway. -Petri
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)*/ + uint64_t pool_size; + + /** Minimum alloc size application will request from pool (bytes)*/ + uint64_t min_alloc; + + /** Maximum alloc size application will request from pool (bytes)*/ + 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); + +/** + * 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 + * + * @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 + * @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 + * @param addr pointer to a previously allocated memory + * (as returned by a previous cal to odpdrv_shm_pool_alloc) + * + * @return A pointer to the allocated memory + * @retval NULL on error. + */ +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 + * + * @return Non-zero value if pool inconsistency is detected + * + * @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); +/** * @} */ diff --git a/platform/linux-generic/include/odp/drv/plat/shm_types.h b/platform/linux-generic/include/odp/drv/plat/shm_types.h index c48eeca..50a0837 100644 --- a/platform/linux-generic/include/odp/drv/plat/shm_types.h +++ b/platform/linux-generic/include/odp/drv/plat/shm_types.h @@ -35,6 +35,9 @@ static inline uint64_t odpdrv_shm_to_u64(odpdrv_shm_t hdl) return _odpdrv_pri(hdl); } +typedef ODPDRV_HANDLE_T(odpdrv_shm_pool_t); + +#define ODPDRV_SHM_POOL_INVALID _odpdrv_cast_scalar(odpdrv_shm_pool_t, NULL) /** * @} */
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 <christophe.milard@linaro.org> --- include/odp/drv/spec/shm.h | 97 ++++++++++++++++++++++ .../linux-generic/include/odp/drv/plat/shm_types.h | 3 + 2 files changed, 100 insertions(+) -- 2.7.4