[API-NEXT,PATCHv3,05/16] api: shm: add flag to guarantee address unicity on all ODP threads

Message ID 1476890991-4693-6-git-send-email-christophe.milard@linaro.org
State New
Headers show

Commit Message

Christophe Milard Oct. 19, 2016, 3:29 p.m.
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

Comments

Savolainen, Petri (Nokia - FI/Espoo) Oct. 21, 2016, 10:40 a.m. | #1
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
Christophe Milard Oct. 24, 2016, 7:42 a.m. | #2
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

>

>

Patch

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