Hi, I'll copy all API related comments into this response. Also I'd suggest that the next version has all three API patches merged into one, since those touch the same feature and lines in the file. Single patch is easier to review, find later in commit log and handle during merge, etc. Now all three patches conflict if you change any one of them.
> -----Original Message----- > From: lng-odp [mailto:lng-odp-boun...@lists.linaro.org] On Behalf Of > Christophe Milard > Sent: Wednesday, October 19, 2016 6:30 PM > To: mike.hol...@linaro.org; bill.fischo...@linaro.org; lng- > o...@lists.linaro.org > Subject: [lng-odp] [API-NEXT PATCHv3 05/16] api: shm: add flag to > guarantee address unicity on all ODP threads > > The ODP_SHM_SINGLE_VA flag is created: when set (at odp_shm_reserve()), > this flag guarantees that all ODP threads sharing this memory > block will see the block at the same address (regadless of ODP > thread type -pthread vs process- or fork time) This explanation would be good to move into the actual API spec under... > > Signed-off-by: Christophe Milard <christophe.mil...@linaro.org> > --- > include/odp/api/spec/shared_memory.h | 7 +++---- > 1 file changed, 3 insertions(+), 4 deletions(-) > > diff --git a/include/odp/api/spec/shared_memory.h > b/include/odp/api/spec/shared_memory.h > index 8c76807..fefb5d6 100644 > --- a/include/odp/api/spec/shared_memory.h > +++ b/include/odp/api/spec/shared_memory.h > @@ -45,10 +45,9 @@ extern "C" { > /* > * Shared memory flags > */ > - > -/* Share level */ > -#define ODP_SHM_SW_ONLY 0x1 /**< Application SW only, no HW access */ > -#define ODP_SHM_PROC 0x2 /**< Share with external processes */ > +#define ODP_SHM_SW_ONLY 0x1 /**< Application SW only, no HW > access */ > +#define ODP_SHM_PROC 0x2 /**< Share with external processes > */ ... something like this: /** * Single virtual address * * When set, this flag guarantees that all ODP threads sharing this * memory block will see the block at the same address - regardless * of ODP thread type (e.g. pthread vs. process (or fork process time)). */ > +#define ODP_SHM_SINGLE_VA 0x4 > > /** > * Shared memory block info > -- > 2.7.4 > +#define ODP_SHM_LOCK 0x8 /**< prevent swapping this memory */ Is this really needed? I think you can assume that a data plane application never wants that memory (allocated from ODP) can be swapped to HDD. It would just ruin application performance and latency totally. So, let's leave the flag out and implementation will prevent swapping whenever possible. > The flag ODP_SHM_EXPORT is added: when passed at odp_shm_reserve() time > the memory block becomes visible to other ODP instances. > The function odp_shm_reserve_exported() is added: this function enables to > reserve block of memories exported by other ODP instances (using the > ODP_SHM_EXPORT flag). Again part of this git log text should be moved to the API spec. For example: /** * When set, the memory block becomes visible to other ODP instances * through odp_shm_reserve_exported(). * / > +#define ODP_SHM_EXPORT 0x10 > +/** > + * get and reserve a block of shared memory, exported by another ODP > instance Start document text with an upper case character. > + * > + * @param[in] remote_name Name of the block, in the remote ODP instance [in] is not needed, since far majority of parameters are [in]. We need to highlight only the exceptional [out] or [in, out] parameters. > + * @param[in] odp_inst Remote ODP instance, as returned by > odp_init_global() > + * @param[in] local_name Name given to the block, in the local ODP > instance This name can be optional (== NULL). > + * @param[in] align Block alignment in bytes When the block is already created (by the other instance), you would not be able to change the align (or size). Why this param is here? I'd just remove it. > + * @param[in] flags Shared memory parameter flags (ODP_SHM_*). > + * Default value is 0. Same thing here. Flags were already defined by the other instance. > + * > + * @return A new handle to the block if it is found (must be freed when > done). > + * @retval ODP_SHM_INVALID on failure > + */ > +odp_shm_t odp_shm_reserve_exported(const char *remote_name, > + odp_instance_t odp_inst, > + const char *local_name, > + uint64_t align, uint32_t flags); The name of the function should not include term "reserve", since this call does not reserve anything from the system, but returns a handle to an already existing memory block. odp_shm_t odp_shm_lookup_remote(odp_instance_t remote_inst, const char *remote_name, const char *local_name); -Petri
