| Message ID | 1476890991-4693-6-git-send-email-christophe.milard@linaro.org |
|---|---|
| State | New |
| Headers | show |
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-bounces@lists.linaro.org] On Behalf Of > Christophe Milard > Sent: Wednesday, October 19, 2016 6:30 PM > To: mike.holmes@linaro.org; bill.fischofer@linaro.org; lng- > odp@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.milard@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
On 21 October 2016 at 12:40, Savolainen, Petri (Nokia - FI/Espoo) <petri.savolainen@nokia-bell-labs.com> wrote: > 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. I dont agree with this: these are 3 different functions and the fact that their implementations affect the same api file does not make them a single patch: What we have now is: patch1: function 1 API patch2: function 1 implementation patch3: function 2 API patch4: function 2 implementation patch5: function 3 API patch6: function 3 implementation What you request is: patch1: function 1,2 and 3 API patch2: function 1 implementation patch3: function 2 implementation patch4: function 3 implementation I don't see how this would make things simpler. ...And you are actually giving a proof later on when asking for the LOCK flag to be removed...With the current splitting: just skip the related 2 patches. With your proposal: "unmerge" the changes. Having said that, if you keep asking for it, as I dont expect anyone to give any opinion, and I need to progress, I guess I'll have to change it. But at least you are given a change to think again :-) > > >> -----Original Message----- >> From: lng-odp [mailto:lng-odp-bounces@lists.linaro.org] On Behalf Of >> Christophe Milard >> Sent: Wednesday, October 19, 2016 6:30 PM >> To: mike.holmes@linaro.org; bill.fischofer@linaro.org; lng- >> odp@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... > OK: V4 >> >> Signed-off-by: Christophe Milard <christophe.milard@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)). > */ OK: V4 >> +#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. I think it is: Why would we not allow for ODP application to have large areas of " slow" memory (rarely hit web pages, control structure, whatever) Always locking memory has a cost as well. But you decides when comes to the API: Just say: "REMOVE!", and I remove :-) > > >> 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 > * > * / >> +#define ODP_SHM_EXPORT 0x10 OK: V4 > >> +/** >> + * get and reserve a block of shared memory, exported by another ODP >> instance > > Start document text with an upper case character. OK: V4 > >> + * >> + * @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. OK: V4 (though this becomes inconsistent with the rest of the file) > >> + * @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). OK: V4 > >> + * @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. Interresting comment: This is true when there is not virtual address. But when the system supports VA, the same memory block might be mapped at different addresses... When, sharing memory between different ODP instances, the uniqueness of the mapping address (in VA) cannot be guaranteed (because there is no commun ODP ancestor to reserve the Virtual address space). In other words, the VA addresses at which the shared memory block will be mapped will very probably be different in the 2 odp instances. So the alignment might differ. Having say that, on paged systems (read: on the linux-gen implementation), returned memory blocks as always page aligned, so the alignment is ignored (because always satisfied) when less than the page size. Alignements larger that the page size are supported and actually imply choosing the VA address so the alignment is satisfied. Once again, 2 ODP instances mapping the memory block in their own VA spaces may have different alignment, even if, as said, in linux-gen it is always aligned on page boundaries. So it make sense to me to keep this parameter. > > >> + * @param[in] flags Shared memory parameter flags (ODP_SHM_*). >> + * Default value is 0. > > Same thing here. Flags were already defined by the other instance. and same kind of answer: some flags will be relevant, some won't, depending whether they affect virtual memory properties or physical memory: ODP_SHM_SINGLE_VA makes sense (and guarantees that the VA address will be unique within the ODP instance only). ODP_SHM_LOCK does not: if it is locked in the first instance, it will be lock here as well. Some other flag may arise in the future sa well... Shall we really forbid all? > >> + * >> + * @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. Don't agree: shm handles are scoped at ODP instance. This call does return a new handle (as opposed to any lookup calls which returns an existing handle). An shm handle returned by odp_shm_reserve_exported() needs to be freed when done. Yes, the memory that the handles "refers to" happens to be shared, but a new handle (and related data) is created: seen from an application perspective, this is much closer to a reserve() than a lookup (which would return an existing handle with no extra need for free() ) Hope this make sense for you too. Christophe. > > odp_shm_t odp_shm_lookup_remote(odp_instance_t remote_inst, > const char *remote_name, > const char *local_name); > > > -Petri > >
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 */ +#define ODP_SHM_SINGLE_VA 0x4 /**< guarantee unique addr on all threads*/ /** * Shared memory block info
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) Signed-off-by: Christophe Milard <christophe.milard@linaro.org> --- include/odp/api/spec/shared_memory.h | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) -- 2.7.4