[v4] Add-global_init-paramiters

Message ID	1409250358-14680-1-git-send-email-mike.holmes@linaro.org
State	Superseded
Headers	show Return-Path: <patchwork-forward+bncBCT4HJVWSENBBUPI7WPQKGQECVCMRXA@linaro.org> Received-SPF: pass (google.com: domain of patch+caf_=patchwork-forward=linaro.org@linaro.org designates 209.85.220.175 as permitted sender) client-ip=209.85.220.175; Received-SPF: none (google.com: lng-odp-bounces@lists.linaro.org does not designate permitted sender hosts) client-ip=54.225.227.206; From: Mike Holmes <mike.holmes@linaro.org> To: lng-odp@lists.linaro.org Date: Thu, 28 Aug 2014 14:25:58 -0400 Message-Id: <1409250358-14680-1-git-send-email-mike.holmes@linaro.org> Subject: [lng-odp] [PATCH v4] Add-global_init-paramiters Precedence: list MIME-Version: 1.0 Errors-To: lng-odp-bounces@lists.linaro.org Sender: lng-odp-bounces@lists.linaro.org Mailing-list: list patchwork-forward@linaro.org; contact patchwork-forward+owners@linaro.org Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit

Mike Holmes Aug. 28, 2014, 6:25 p.m. UTC

Signed-off-by: Mike Holmes <mike.holmes@linaro.org>
---

v2:
Factor out odp_platform_init so that odp_global_init is only defined
for linux_generic and resued.

v3:
Use -M to simplify the diff
Fix missing doxygen comments

v4:
Improve the documentation.

 example/generator/odp_generator.c                  |  2 +-
 example/l2fwd/odp_l2fwd.c                          |  2 +-
 example/odp_example/odp_example.c                  |  2 +-
 example/packet/odp_pktio.c                         |  2 +-
 example/timer/odp_timer_test.c                     |  2 +-
 platform/linux-dpdk/Makefile.am                    |  3 +-
 platform/linux-dpdk/{odp_init.c => odp_platform.c} | 63 +------------------
 platform/linux-generic/Makefile.am                 |  1 +
 platform/linux-generic/include/api/odp_init.h      | 34 +++++++---
 platform/linux-generic/include/odp_internal.h      |  3 +
 platform/linux-generic/odp_init.c                  |  9 ++-
 platform/linux-generic/odp_platform.c              | 14 +++++
 platform/linux-keystone2/Makefile.am               |  3 +-
 .../linux-keystone2/{odp_init.c => odp_platform.c} | 72 +++-------------------
 test/api_test/odp_common.c                         |  2 +-
 15 files changed, 72 insertions(+), 142 deletions(-)
 rename platform/linux-dpdk/{odp_init.c => odp_platform.c} (50%)
 create mode 100644 platform/linux-generic/odp_platform.c
 rename platform/linux-keystone2/{odp_init.c => odp_platform.c} (72%)

Savolainen, Petri (NSN - FI/Espoo) Aug. 29, 2014, 7:59 a.m. UTC | #1

> -----Original Message-----
> From: lng-odp-bounces@lists.linaro.org [mailto:lng-odp-
> bounces@lists.linaro.org] On Behalf Of ext Mike Holmes
> Sent: Thursday, August 28, 2014 9:26 PM
> To: lng-odp@lists.linaro.org
> Subject: [lng-odp] [PATCH v4] Add-global_init-paramiters
> 
> Signed-off-by: Mike Holmes <mike.holmes@linaro.org>
> ---
> 
> v2:
> Factor out odp_platform_init so that odp_global_init is only defined
> for linux_generic and resued.
> 
> v3:
> Use -M to simplify the diff
> Fix missing doxygen comments
> 
> v4:
> Improve the documentation.
> 
>  example/generator/odp_generator.c                  |  2 +-
>  example/l2fwd/odp_l2fwd.c                          |  2 +-
>  example/odp_example/odp_example.c                  |  2 +-
>  example/packet/odp_pktio.c                         |  2 +-
>  example/timer/odp_timer_test.c                     |  2 +-
>  platform/linux-dpdk/Makefile.am                    |  3 +-
>  platform/linux-dpdk/{odp_init.c => odp_platform.c} | 63 +----------------
> --
>  platform/linux-generic/Makefile.am                 |  1 +
>  platform/linux-generic/include/api/odp_init.h      | 34 +++++++---
>  platform/linux-generic/include/odp_internal.h      |  3 +
>  platform/linux-generic/odp_init.c                  |  9 ++-
>  platform/linux-generic/odp_platform.c              | 14 +++++
>  platform/linux-keystone2/Makefile.am               |  3 +-
>  .../linux-keystone2/{odp_init.c => odp_platform.c} | 72 +++--------------
> -----
>  test/api_test/odp_common.c                         |  2 +-
>  15 files changed, 72 insertions(+), 142 deletions(-)
>  rename platform/linux-dpdk/{odp_init.c => odp_platform.c} (50%)
>  create mode 100644 platform/linux-generic/odp_platform.c
>  rename platform/linux-keystone2/{odp_init.c => odp_platform.c} (72%)
> 
> diff --git a/example/generator/odp_generator.c
> b/example/generator/odp_generator.c
> index 9fa9b37..ba7aa68 100644
> --- a/example/generator/odp_generator.c
> +++ b/example/generator/odp_generator.c
> @@ -525,7 +525,7 @@ int main(int argc, char *argv[])
>  	int core_count;
> 
>  	/* Init ODP before calling anything else */
> -	if (odp_init_global()) {
> +	if (odp_init_global(NULL, NULL)) {
>  		ODP_ERR("Error: ODP global init failed.\n");
>  		exit(EXIT_FAILURE);
>  	}
> diff --git a/example/l2fwd/odp_l2fwd.c b/example/l2fwd/odp_l2fwd.c
> index d74449a..f8d6729 100644
> --- a/example/l2fwd/odp_l2fwd.c
> +++ b/example/l2fwd/odp_l2fwd.c
> @@ -323,7 +323,7 @@ int main(int argc, char *argv[])
>  	odp_pktio_t pktio;
> 
>  	/* Init ODP before calling anything else */
> -	if (odp_init_global()) {
> +	if (odp_init_global(NULL, NULL)) {
>  		ODP_ERR("Error: ODP global init failed.\n");
>  		exit(EXIT_FAILURE);
>  	}
> diff --git a/example/odp_example/odp_example.c
> b/example/odp_example/odp_example.c
> index f0bdf29..3d38ac7 100644
> --- a/example/odp_example/odp_example.c
> +++ b/example/odp_example/odp_example.c
> @@ -951,7 +951,7 @@ int main(int argc, char *argv[])
> 
>  	memset(thread_tbl, 0, sizeof(thread_tbl));
> 
> -	if (odp_init_global()) {
> +	if (odp_init_global(NULL, NULL)) {
>  		printf("ODP global init failed.\n");
>  		return -1;
>  	}
> diff --git a/example/packet/odp_pktio.c b/example/packet/odp_pktio.c
> index f247bd0..f3543a0 100644
> --- a/example/packet/odp_pktio.c
> +++ b/example/packet/odp_pktio.c
> @@ -311,7 +311,7 @@ int main(int argc, char *argv[])
>  	int core_count;
> 
>  	/* Init ODP before calling anything else */
> -	if (odp_init_global()) {
> +	if (odp_init_global(NULL, NULL)) {
>  		ODP_ERR("Error: ODP global init failed.\n");
>  		exit(EXIT_FAILURE);
>  	}
> diff --git a/example/timer/odp_timer_test.c
> b/example/timer/odp_timer_test.c
> index bf1d7df..abf90aa 100644
> --- a/example/timer/odp_timer_test.c
> +++ b/example/timer/odp_timer_test.c
> @@ -260,7 +260,7 @@ int main(int argc, char *argv[])
> 
>  	memset(thread_tbl, 0, sizeof(thread_tbl));
> 
> -	if (odp_init_global()) {
> +	if (odp_init_global(NULL, NULL)) {
>  		printf("ODP global init failed.\n");
>  		return -1;
>  	}
> diff --git a/platform/linux-dpdk/Makefile.am b/platform/linux-
> dpdk/Makefile.am
> index ff49b7d..70fa549 100644
> --- a/platform/linux-dpdk/Makefile.am
> +++ b/platform/linux-dpdk/Makefile.am
> @@ -67,13 +67,14 @@ __LIB__libodp_la_SOURCES = \
>  			   odp_buffer.c \
>  			   odp_buffer_pool.c \
>  			   ../linux-generic/odp_coremask.c \
> -			   odp_init.c \
> +			   ../linux-generic/odp_init.c \
>  			   odp_linux.c \
>  			   odp_packet.c \
>  			   odp_packet_dpdk.c \
>  			   ../linux-generic/odp_packet_flags.c \
>  			   odp_packet_io.c \
>  			   ../linux-generic/odp_packet_socket.c \
> +			   odp_platform.c \
>  			   odp_queue.c \
>  			   ../linux-generic/odp_ring.c \
>  			   ../linux-generic/odp_rwlock.c \
> diff --git a/platform/linux-dpdk/odp_init.c b/platform/linux-
> dpdk/odp_platform.c
> similarity index 50%
> rename from platform/linux-dpdk/odp_init.c
> rename to platform/linux-dpdk/odp_platform.c
> index ecc2066..5a054fd 100644
> --- a/platform/linux-dpdk/odp_init.c
> +++ b/platform/linux-dpdk/odp_platform.c
> @@ -1,4 +1,4 @@
> -/* Copyright (c) 2013, Linaro Limited
> +/* Copyright (c) 2014, Linaro Limited
>   * All rights reserved.
>   *
>   * SPDX-License-Identifier:     BSD-3-Clause
> @@ -9,7 +9,7 @@
>  #include <odp_debug.h>
>  #include <odp_packet_dpdk.h>
> 
> -int odp_init_dpdk(void)
> +int odp_init_platform(odp_global_platform_init_t *platform_params
> ODP_UNUSED)
>  {
>  	int test_argc = 5;
>  	char *test_argv[6];
> @@ -50,64 +50,7 @@ int odp_init_dpdk(void)
>  	return 0;
>  }
> 
> -int odp_init_global(void)
> +int odp_crypto_init_global(void)

Is crypto init now in odp_platform.c? It should be in odp_crypto.c or similar... 

>  {
> -	odp_thread_init_global();
> -
> -	odp_system_info_init();
> -
> -	if (odp_init_dpdk()) {
> -		ODP_ERR("ODP dpdk init failed.\n");
> -		return -1;
> -	}
> -
> -	if (odp_shm_init_global()) {
> -		ODP_ERR("ODP shm init failed.\n");
> -		return -1;
> -	}
> -
> -	if (odp_buffer_pool_init_global()) {
> -		ODP_ERR("ODP buffer pool init failed.\n");
> -		return -1;
> -	}
> -
> -	if (odp_queue_init_global()) {
> -		ODP_ERR("ODP queue init failed.\n");
> -		return -1;
> -	}
> -
> -	if (odp_schedule_init_global()) {
> -		ODP_ERR("ODP schedule init failed.\n");
> -		return -1;
> -	}
> -
> -	if (odp_pktio_init_global()) {
> -		ODP_ERR("ODP packet io init failed.\n");
> -		return -1;
> -	}
> -
> -	if (odp_timer_init_global()) {
> -		ODP_ERR("ODP timer init failed.\n");
> -		return -1;
> -	}
> -
> -	return 0;
> -}
> -
> -
> -int odp_init_local(int thr_id)
> -{
> -	odp_thread_init_local(thr_id);
> -
> -	if (odp_pktio_init_local()) {
> -		ODP_ERR("ODP packet io local init failed.\n");
> -		return -1;
> -	}
> -
> -	if (odp_schedule_init_local()) {
> -		ODP_ERR("ODP schedule local init failed.\n");
> -		return -1;
> -	}
> -
>  	return 0;
>  }
> diff --git a/platform/linux-generic/Makefile.am b/platform/linux-
> generic/Makefile.am
> index f4dfdc1..c4aae9d 100644
> --- a/platform/linux-generic/Makefile.am
> +++ b/platform/linux-generic/Makefile.am
> @@ -62,6 +62,7 @@ __LIB__libodp_la_SOURCES = \
>  			   odp_packet_flags.c \
>  			   odp_packet_io.c \
>  			   odp_packet_socket.c \
> +			   odp_platform.c \
>  			   odp_queue.c \
>  			   odp_ring.c \
>  			   odp_rwlock.c \
> diff --git a/platform/linux-generic/include/api/odp_init.h
> b/platform/linux-generic/include/api/odp_init.h
> index 490324a..ece4547 100644
> --- a/platform/linux-generic/include/api/odp_init.h
> +++ b/platform/linux-generic/include/api/odp_init.h
> @@ -22,24 +22,42 @@ extern "C" {
> 
>  #include <odp_std_types.h>
> 
> -
> -
> +/** ODP  initialisation data.
> + * Data  that is required to initialize the ODP API with the
> + * application specific data such as specifying a logging callback, the
> log
> + * level etc.
> + */
> +typedef struct odp_global_init {

The struct name should be "struct odp_global_init_t", "struct odp_global_init_s" or "struct odp_global_init_".

The name without postfix (odp_global_init) should be reserved for variable names.

> +} odp_global_init_t;

In short, this could be named odp_init_t. It's the main init struct there will be.
If there will be a local init struct it could be then odp_init_local_t...

> +
> +/** ODP platform initialization data.
> + * @note ODP API does nothing with this data. It is the underlying
> + * implementation requires it and any data passed here is not portable.
> + * It is required that the application takes care of identifying and
> + * passing any required platform specific data.
> + */
> +typedef struct odp_global_platform_init {

Same thing, rename struct.

> +} odp_global_platform_init_t;

Same thing, odp_platform_init_t ??


> 
>  /**
> - * Perform global ODP initalisation.
> - *
> - * This function must be called once before calling
> - * any other ODP API functions.
> + * Perform global ODP initialisation.
>   *
> + * This function must be called once before calling any other ODP API
> + * functions.
> + * @param[in] params Those parameters that are interpreted by the ODP API

"Those parameters that are common to all ODP implementations. Specified in this API"

API does not interpret, implementation does...

> + * @param[in] platform_params Those parameters that are passed without
> + * interpretation by the ODP API to the implementation.

" ... implementation specific parameter. Specified outside of ODP API"

>   * @return 0 if successful
>   */
> -int odp_init_global(void);
> +int odp_init_global(odp_global_init_t *params,
> +		    odp_global_platform_init_t *platform_params);
> 
> 
>  /**
> - * Perform thread local ODP initalisation.
> + * Perform thread local ODP initialisation.
>   *
>   * All threads must call this function before calling
> + *

Extra line feed?

>   * any other ODP API functions.
>   * @param thr_id Thread id
>   * @return 0 if successful
> diff --git a/platform/linux-generic/include/odp_internal.h
> b/platform/linux-generic/include/odp_internal.h
> index aa79493..6c71fc9 100644
> --- a/platform/linux-generic/include/odp_internal.h
> +++ b/platform/linux-generic/include/odp_internal.h
> @@ -4,6 +4,7 @@
>   * SPDX-License-Identifier:     BSD-3-Clause
>   */
> 
> +#include <odp_init.h>
> 
>  /**
>   * @file
> @@ -42,6 +43,8 @@ int odp_schedule_init_local(void);
>  int odp_timer_init_global(void);
>  int odp_timer_disarm_all(void);
> 
> +int odp_init_platform(odp_global_platform_init_t *platform_params);
> +
>  #ifdef __cplusplus
>  }
>  #endif
> diff --git a/platform/linux-generic/odp_init.c b/platform/linux-
> generic/odp_init.c
> index 5b7e192..f595def 100644
> --- a/platform/linux-generic/odp_init.c
> +++ b/platform/linux-generic/odp_init.c
> @@ -8,13 +8,18 @@
>  #include <odp_internal.h>
>  #include <odp_debug.h>
> 
> -
> -int odp_init_global(void)
> +int odp_init_global(odp_global_init_t *params  ODP_UNUSED,
> +		    odp_global_platform_init_t *platform_params ODP_UNUSED)
>  {
>  	odp_thread_init_global();
> 
>  	odp_system_info_init();
> 
> +	if (odp_init_platform(platform_params)) {

platform_params is used here, so remove ODP_UNUSED from argument list.

On the other hand, it would be better to remove odp_init_platform() 
altogether since does not do anything. It's better to add it when actually used.
The order of init calls is very important, e.g. now you could not allocate shared
memory in odp_init_platform() because it's executed before shm init...

- Petri

> +		ODP_ERR("ODP platform init failed.\n");
> +		return -1;
> +	}
> +
>  	if (odp_shm_init_global()) {
>  		ODP_ERR("ODP shm init failed.\n");
>  		return -1;
> diff --git a/platform/linux-generic/odp_platform.c b/platform/linux-
> generic/odp_platform.c
> new file mode 100644
> index 0000000..a9efa7f
> --- /dev/null
> +++ b/platform/linux-generic/odp_platform.c
> @@ -0,0 +1,14 @@
> +/* Copyright (c) 2014, Linaro Limited
> + * All rights reserved.
> + *
> + * SPDX-License-Identifier:     BSD-3-Clause
> + */
> +
> +#include <odp_init.h>
> +#include <odp_internal.h>
> +#include <odp_debug.h>
> +
> +int odp_init_platform(odp_global_platform_init_t *platform_params
> ODP_UNUSED)
> +{
> +	return 0;
> +}

Wiles, Roger Keith Aug. 29, 2014, 3:07 p.m. UTC | #2

On Aug 29, 2014, at 2:59 AM, Savolainen, Petri (NSN - FI/Espoo) <petri.savolainen@nsn.com> wrote:

> 
> 
>> -----Original Message-----
>> From: lng-odp-bounces@lists.linaro.org [mailto:lng-odp-
>> bounces@lists.linaro.org] On Behalf Of ext Mike Holmes
>> Sent: Thursday, August 28, 2014 9:26 PM
>> To: lng-odp@lists.linaro.org
>> Subject: [lng-odp] [PATCH v4] Add-global_init-paramiters
>> 
>> Signed-off-by: Mike Holmes <mike.holmes@linaro.org>
>> ---
>> 
>> v2:
>> Factor out odp_platform_init so that odp_global_init is only defined
>> for linux_generic and resued.
>> 
>> v3:
>> Use -M to simplify the diff
>> Fix missing doxygen comments
>> 
>> v4:
>> Improve the documentation.
>> 
>> example/generator/odp_generator.c                  |  2 +-
>> example/l2fwd/odp_l2fwd.c                          |  2 +-
>> example/odp_example/odp_example.c                  |  2 +-
>> example/packet/odp_pktio.c                         |  2 +-
>> example/timer/odp_timer_test.c                     |  2 +-
>> platform/linux-dpdk/Makefile.am                    |  3 +-
>> platform/linux-dpdk/{odp_init.c => odp_platform.c} | 63 +----------------
>> --
>> platform/linux-generic/Makefile.am                 |  1 +
>> platform/linux-generic/include/api/odp_init.h      | 34 +++++++---
>> platform/linux-generic/include/odp_internal.h      |  3 +
>> platform/linux-generic/odp_init.c                  |  9 ++-
>> platform/linux-generic/odp_platform.c              | 14 +++++
>> platform/linux-keystone2/Makefile.am               |  3 +-
>> .../linux-keystone2/{odp_init.c => odp_platform.c} | 72 +++--------------
>> -----
>> test/api_test/odp_common.c                         |  2 +-
>> 15 files changed, 72 insertions(+), 142 deletions(-)
>> rename platform/linux-dpdk/{odp_init.c => odp_platform.c} (50%)
>> create mode 100644 platform/linux-generic/odp_platform.c
>> rename platform/linux-keystone2/{odp_init.c => odp_platform.c} (72%)
>> 
>> diff --git a/example/generator/odp_generator.c
>> b/example/generator/odp_generator.c
>> index 9fa9b37..ba7aa68 100644
>> --- a/example/generator/odp_generator.c
>> +++ b/example/generator/odp_generator.c
>> @@ -525,7 +525,7 @@ int main(int argc, char *argv[])
>> 	int core_count;
>> 
>> 	/* Init ODP before calling anything else */
>> -	if (odp_init_global()) {
>> +	if (odp_init_global(NULL, NULL)) {
>> 		ODP_ERR("Error: ODP global init failed.\n");
>> 		exit(EXIT_FAILURE);
>> 	}
>> diff --git a/example/l2fwd/odp_l2fwd.c b/example/l2fwd/odp_l2fwd.c
>> index d74449a..f8d6729 100644
>> --- a/example/l2fwd/odp_l2fwd.c
>> +++ b/example/l2fwd/odp_l2fwd.c
>> @@ -323,7 +323,7 @@ int main(int argc, char *argv[])
>> 	odp_pktio_t pktio;
>> 
>> 	/* Init ODP before calling anything else */
>> -	if (odp_init_global()) {
>> +	if (odp_init_global(NULL, NULL)) {
>> 		ODP_ERR("Error: ODP global init failed.\n");
>> 		exit(EXIT_FAILURE);
>> 	}
>> diff --git a/example/odp_example/odp_example.c
>> b/example/odp_example/odp_example.c
>> index f0bdf29..3d38ac7 100644
>> --- a/example/odp_example/odp_example.c
>> +++ b/example/odp_example/odp_example.c
>> @@ -951,7 +951,7 @@ int main(int argc, char *argv[])
>> 
>> 	memset(thread_tbl, 0, sizeof(thread_tbl));
>> 
>> -	if (odp_init_global()) {
>> +	if (odp_init_global(NULL, NULL)) {
>> 		printf("ODP global init failed.\n");
>> 		return -1;
>> 	}
>> diff --git a/example/packet/odp_pktio.c b/example/packet/odp_pktio.c
>> index f247bd0..f3543a0 100644
>> --- a/example/packet/odp_pktio.c
>> +++ b/example/packet/odp_pktio.c
>> @@ -311,7 +311,7 @@ int main(int argc, char *argv[])
>> 	int core_count;
>> 
>> 	/* Init ODP before calling anything else */
>> -	if (odp_init_global()) {
>> +	if (odp_init_global(NULL, NULL)) {
>> 		ODP_ERR("Error: ODP global init failed.\n");
>> 		exit(EXIT_FAILURE);
>> 	}
>> diff --git a/example/timer/odp_timer_test.c
>> b/example/timer/odp_timer_test.c
>> index bf1d7df..abf90aa 100644
>> --- a/example/timer/odp_timer_test.c
>> +++ b/example/timer/odp_timer_test.c
>> @@ -260,7 +260,7 @@ int main(int argc, char *argv[])
>> 
>> 	memset(thread_tbl, 0, sizeof(thread_tbl));
>> 
>> -	if (odp_init_global()) {
>> +	if (odp_init_global(NULL, NULL)) {
>> 		printf("ODP global init failed.\n");
>> 		return -1;
>> 	}
>> diff --git a/platform/linux-dpdk/Makefile.am b/platform/linux-
>> dpdk/Makefile.am
>> index ff49b7d..70fa549 100644
>> --- a/platform/linux-dpdk/Makefile.am
>> +++ b/platform/linux-dpdk/Makefile.am
>> @@ -67,13 +67,14 @@ __LIB__libodp_la_SOURCES = \
>> 			   odp_buffer.c \
>> 			   odp_buffer_pool.c \
>> 			   ../linux-generic/odp_coremask.c \
>> -			   odp_init.c \
>> +			   ../linux-generic/odp_init.c \
>> 			   odp_linux.c \
>> 			   odp_packet.c \
>> 			   odp_packet_dpdk.c \
>> 			   ../linux-generic/odp_packet_flags.c \
>> 			   odp_packet_io.c \
>> 			   ../linux-generic/odp_packet_socket.c \
>> +			   odp_platform.c \
>> 			   odp_queue.c \
>> 			   ../linux-generic/odp_ring.c \
>> 			   ../linux-generic/odp_rwlock.c \
>> diff --git a/platform/linux-dpdk/odp_init.c b/platform/linux-
>> dpdk/odp_platform.c
>> similarity index 50%
>> rename from platform/linux-dpdk/odp_init.c
>> rename to platform/linux-dpdk/odp_platform.c
>> index ecc2066..5a054fd 100644
>> --- a/platform/linux-dpdk/odp_init.c
>> +++ b/platform/linux-dpdk/odp_platform.c
>> @@ -1,4 +1,4 @@
>> -/* Copyright (c) 2013, Linaro Limited
>> +/* Copyright (c) 2014, Linaro Limited
>>  * All rights reserved.
>>  *
>>  * SPDX-License-Identifier:     BSD-3-Clause
>> @@ -9,7 +9,7 @@
>> #include <odp_debug.h>
>> #include <odp_packet_dpdk.h>
>> 
>> -int odp_init_dpdk(void)
>> +int odp_init_platform(odp_global_platform_init_t *platform_params
>> ODP_UNUSED)
>> {
>> 	int test_argc = 5;
>> 	char *test_argv[6];
>> @@ -50,64 +50,7 @@ int odp_init_dpdk(void)
>> 	return 0;
>> }
>> 
>> -int odp_init_global(void)
>> +int odp_crypto_init_global(void)
> 
> Is crypto init now in odp_platform.c? It should be in odp_crypto.c or similar... 
> 
>> {
>> -	odp_thread_init_global();
>> -
>> -	odp_system_info_init();
>> -
>> -	if (odp_init_dpdk()) {
>> -		ODP_ERR("ODP dpdk init failed.\n");
>> -		return -1;
>> -	}
>> -
>> -	if (odp_shm_init_global()) {
>> -		ODP_ERR("ODP shm init failed.\n");
>> -		return -1;
>> -	}
>> -
>> -	if (odp_buffer_pool_init_global()) {
>> -		ODP_ERR("ODP buffer pool init failed.\n");
>> -		return -1;
>> -	}
>> -
>> -	if (odp_queue_init_global()) {
>> -		ODP_ERR("ODP queue init failed.\n");
>> -		return -1;
>> -	}
>> -
>> -	if (odp_schedule_init_global()) {
>> -		ODP_ERR("ODP schedule init failed.\n");
>> -		return -1;
>> -	}
>> -
>> -	if (odp_pktio_init_global()) {
>> -		ODP_ERR("ODP packet io init failed.\n");
>> -		return -1;
>> -	}
>> -
>> -	if (odp_timer_init_global()) {
>> -		ODP_ERR("ODP timer init failed.\n");
>> -		return -1;
>> -	}
>> -
>> -	return 0;
>> -}
>> -
>> -
>> -int odp_init_local(int thr_id)
>> -{
>> -	odp_thread_init_local(thr_id);
>> -
>> -	if (odp_pktio_init_local()) {
>> -		ODP_ERR("ODP packet io local init failed.\n");
>> -		return -1;
>> -	}
>> -
>> -	if (odp_schedule_init_local()) {
>> -		ODP_ERR("ODP schedule local init failed.\n");
>> -		return -1;
>> -	}
>> -
>> 	return 0;
>> }
>> diff --git a/platform/linux-generic/Makefile.am b/platform/linux-
>> generic/Makefile.am
>> index f4dfdc1..c4aae9d 100644
>> --- a/platform/linux-generic/Makefile.am
>> +++ b/platform/linux-generic/Makefile.am
>> @@ -62,6 +62,7 @@ __LIB__libodp_la_SOURCES = \
>> 			   odp_packet_flags.c \
>> 			   odp_packet_io.c \
>> 			   odp_packet_socket.c \
>> +			   odp_platform.c \
>> 			   odp_queue.c \
>> 			   odp_ring.c \
>> 			   odp_rwlock.c \
>> diff --git a/platform/linux-generic/include/api/odp_init.h
>> b/platform/linux-generic/include/api/odp_init.h
>> index 490324a..ece4547 100644
>> --- a/platform/linux-generic/include/api/odp_init.h
>> +++ b/platform/linux-generic/include/api/odp_init.h
>> @@ -22,24 +22,42 @@ extern "C" {
>> 
>> #include <odp_std_types.h>
>> 
>> -
>> -
>> +/** ODP  initialisation data.
>> + * Data  that is required to initialize the ODP API with the
>> + * application specific data such as specifying a logging callback, the
>> log
>> + * level etc.
>> + */
>> +typedef struct odp_global_init {
> 
> The struct name should be "struct odp_global_init_t", "struct odp_global_init_s" or "struct odp_global_init_".
> 
> The name without postfix (odp_global_init) should be reserved for variable names.

I would use the 'struct odp_init_s {‘ adding the _s postfix and shorting the name to the suggestions below. 
> 
>> +} odp_global_init_t;
> 
> In short, this could be named odp_init_t. It's the main init struct there will be.
> If there will be a local init struct it could be then odp_init_local_t...
> 
>> +
>> +/** ODP platform initialization data.
>> + * @note ODP API does nothing with this data. It is the underlying
>> + * implementation requires it and any data passed here is not portable.
>> + * It is required that the application takes care of identifying and
>> + * passing any required platform specific data.
>> + */
>> +typedef struct odp_global_platform_init {
> 
> Same thing, rename struct.
> 
>> +} odp_global_platform_init_t;
> 
> Same thing, odp_platform_init_t ??
> 
> 
>> 
>> /**
>> - * Perform global ODP initalisation.
>> - *
>> - * This function must be called once before calling
>> - * any other ODP API functions.
>> + * Perform global ODP initialisation.
>>  *
>> + * This function must be called once before calling any other ODP API
>> + * functions.
>> + * @param[in] params Those parameters that are interpreted by the ODP API
> 
> "Those parameters that are common to all ODP implementations. Specified in this API"
> 
> API does not interpret, implementation does...
> 
>> + * @param[in] platform_params Those parameters that are passed without
>> + * interpretation by the ODP API to the implementation.
> 
> " ... implementation specific parameter. Specified outside of ODP API"
> 
>>  * @return 0 if successful
>>  */
>> -int odp_init_global(void);
>> +int odp_init_global(odp_global_init_t *params,
>> +		    odp_global_platform_init_t *platform_params);
>> 
>> 
>> /**
>> - * Perform thread local ODP initalisation.
>> + * Perform thread local ODP initialisation.
>>  *
>>  * All threads must call this function before calling
>> + *
> 
> Extra line feed?
> 
>>  * any other ODP API functions.
>>  * @param thr_id Thread id
>>  * @return 0 if successful
>> diff --git a/platform/linux-generic/include/odp_internal.h
>> b/platform/linux-generic/include/odp_internal.h
>> index aa79493..6c71fc9 100644
>> --- a/platform/linux-generic/include/odp_internal.h
>> +++ b/platform/linux-generic/include/odp_internal.h
>> @@ -4,6 +4,7 @@
>>  * SPDX-License-Identifier:     BSD-3-Clause
>>  */
>> 
>> +#include <odp_init.h>
>> 
>> /**
>>  * @file
>> @@ -42,6 +43,8 @@ int odp_schedule_init_local(void);
>> int odp_timer_init_global(void);
>> int odp_timer_disarm_all(void);
>> 
>> +int odp_init_platform(odp_global_platform_init_t *platform_params);
>> +
>> #ifdef __cplusplus
>> }
>> #endif
>> diff --git a/platform/linux-generic/odp_init.c b/platform/linux-
>> generic/odp_init.c
>> index 5b7e192..f595def 100644
>> --- a/platform/linux-generic/odp_init.c
>> +++ b/platform/linux-generic/odp_init.c
>> @@ -8,13 +8,18 @@
>> #include <odp_internal.h>
>> #include <odp_debug.h>
>> 
>> -
>> -int odp_init_global(void)
>> +int odp_init_global(odp_global_init_t *params  ODP_UNUSED,
>> +		    odp_global_platform_init_t *platform_params ODP_UNUSED)
>> {
>> 	odp_thread_init_global();
>> 
>> 	odp_system_info_init();
>> 
>> +	if (odp_init_platform(platform_params)) {
> 
> platform_params is used here, so remove ODP_UNUSED from argument list.
> 
> On the other hand, it would be better to remove odp_init_platform() 
> altogether since does not do anything. It's better to add it when actually used.
> The order of init calls is very important, e.g. now you could not allocate shared
> memory in odp_init_platform() because it's executed before shm init…

I disagree here, we should keep the odp_init_platform() as we do not know what will happen in the future and to make the platforms follow the same pattern.
> 
> - Petri
> 
>> +		ODP_ERR("ODP platform init failed.\n");
>> +		return -1;
>> +	}
>> +
>> 	if (odp_shm_init_global()) {
>> 		ODP_ERR("ODP shm init failed.\n");
>> 		return -1;
>> diff --git a/platform/linux-generic/odp_platform.c b/platform/linux-
>> generic/odp_platform.c
>> new file mode 100644
>> index 0000000..a9efa7f
>> --- /dev/null
>> +++ b/platform/linux-generic/odp_platform.c
>> @@ -0,0 +1,14 @@
>> +/* Copyright (c) 2014, Linaro Limited
>> + * All rights reserved.
>> + *
>> + * SPDX-License-Identifier:     BSD-3-Clause
>> + */
>> +
>> +#include <odp_init.h>
>> +#include <odp_internal.h>
>> +#include <odp_debug.h>
>> +
>> +int odp_init_platform(odp_global_platform_init_t *platform_params
>> ODP_UNUSED)
>> +{
>> +	return 0;
>> +}
> 
> 
> 
> 
> 
> _______________________________________________
> lng-odp mailing list
> lng-odp@lists.linaro.org
> http://lists.linaro.org/mailman/listinfo/lng-odp

Keith Wiles, Principal Technologist with CTO office, Wind River mobile 972-213-5533

Mike Holmes Aug. 29, 2014, 5:05 p.m. UTC | #3

On 29 August 2014 03:59, Savolainen, Petri (NSN - FI/Espoo) <
petri.savolainen@nsn.com> wrote:

>
>
> > -----Original Message-----
> > From: lng-odp-bounces@lists.linaro.org [mailto:lng-odp-
> > bounces@lists.linaro.org] On Behalf Of ext Mike Holmes
> > Sent: Thursday, August 28, 2014 9:26 PM
> > To: lng-odp@lists.linaro.org
> > Subject: [lng-odp] [PATCH v4] Add-global_init-paramiters
> >
> > Signed-off-by: Mike Holmes <mike.holmes@linaro.org>
> > ---
> >
> > v2:
> > Factor out odp_platform_init so that odp_global_init is only defined
> > for linux_generic and resued.
> >
> > v3:
> > Use -M to simplify the diff
> > Fix missing doxygen comments
> >
> > v4:
> > Improve the documentation.
> >
> >  example/generator/odp_generator.c                  |  2 +-
> >  example/l2fwd/odp_l2fwd.c                          |  2 +-
> >  example/odp_example/odp_example.c                  |  2 +-
> >  example/packet/odp_pktio.c                         |  2 +-
> >  example/timer/odp_timer_test.c                     |  2 +-
> >  platform/linux-dpdk/Makefile.am                    |  3 +-
> >  platform/linux-dpdk/{odp_init.c => odp_platform.c} | 63
> +----------------
> > --
> >  platform/linux-generic/Makefile.am                 |  1 +
> >  platform/linux-generic/include/api/odp_init.h      | 34 +++++++---
> >  platform/linux-generic/include/odp_internal.h      |  3 +
> >  platform/linux-generic/odp_init.c                  |  9 ++-
> >  platform/linux-generic/odp_platform.c              | 14 +++++
> >  platform/linux-keystone2/Makefile.am               |  3 +-
> >  .../linux-keystone2/{odp_init.c => odp_platform.c} | 72
> +++--------------
> > -----
> >  test/api_test/odp_common.c                         |  2 +-
> >  15 files changed, 72 insertions(+), 142 deletions(-)
> >  rename platform/linux-dpdk/{odp_init.c => odp_platform.c} (50%)
> >  create mode 100644 platform/linux-generic/odp_platform.c
> >  rename platform/linux-keystone2/{odp_init.c => odp_platform.c} (72%)
> >
> > diff --git a/example/generator/odp_generator.c
> > b/example/generator/odp_generator.c
> > index 9fa9b37..ba7aa68 100644
> > --- a/example/generator/odp_generator.c
> > +++ b/example/generator/odp_generator.c
> > @@ -525,7 +525,7 @@ int main(int argc, char *argv[])
> >       int core_count;
> >
> >       /* Init ODP before calling anything else */
> > -     if (odp_init_global()) {
> > +     if (odp_init_global(NULL, NULL)) {
> >               ODP_ERR("Error: ODP global init failed.\n");
> >               exit(EXIT_FAILURE);
> >       }
> > diff --git a/example/l2fwd/odp_l2fwd.c b/example/l2fwd/odp_l2fwd.c
> > index d74449a..f8d6729 100644
> > --- a/example/l2fwd/odp_l2fwd.c
> > +++ b/example/l2fwd/odp_l2fwd.c
> > @@ -323,7 +323,7 @@ int main(int argc, char *argv[])
> >       odp_pktio_t pktio;
> >
> >       /* Init ODP before calling anything else */
> > -     if (odp_init_global()) {
> > +     if (odp_init_global(NULL, NULL)) {
> >               ODP_ERR("Error: ODP global init failed.\n");
> >               exit(EXIT_FAILURE);
> >       }
> > diff --git a/example/odp_example/odp_example.c
> > b/example/odp_example/odp_example.c
> > index f0bdf29..3d38ac7 100644
> > --- a/example/odp_example/odp_example.c
> > +++ b/example/odp_example/odp_example.c
> > @@ -951,7 +951,7 @@ int main(int argc, char *argv[])
> >
> >       memset(thread_tbl, 0, sizeof(thread_tbl));
> >
> > -     if (odp_init_global()) {
> > +     if (odp_init_global(NULL, NULL)) {
> >               printf("ODP global init failed.\n");
> >               return -1;
> >       }
> > diff --git a/example/packet/odp_pktio.c b/example/packet/odp_pktio.c
> > index f247bd0..f3543a0 100644
> > --- a/example/packet/odp_pktio.c
> > +++ b/example/packet/odp_pktio.c
> > @@ -311,7 +311,7 @@ int main(int argc, char *argv[])
> >       int core_count;
> >
> >       /* Init ODP before calling anything else */
> > -     if (odp_init_global()) {
> > +     if (odp_init_global(NULL, NULL)) {
> >               ODP_ERR("Error: ODP global init failed.\n");
> >               exit(EXIT_FAILURE);
> >       }
> > diff --git a/example/timer/odp_timer_test.c
> > b/example/timer/odp_timer_test.c
> > index bf1d7df..abf90aa 100644
> > --- a/example/timer/odp_timer_test.c
> > +++ b/example/timer/odp_timer_test.c
> > @@ -260,7 +260,7 @@ int main(int argc, char *argv[])
> >
> >       memset(thread_tbl, 0, sizeof(thread_tbl));
> >
> > -     if (odp_init_global()) {
> > +     if (odp_init_global(NULL, NULL)) {
> >               printf("ODP global init failed.\n");
> >               return -1;
> >       }
> > diff --git a/platform/linux-dpdk/Makefile.am b/platform/linux-
> > dpdk/Makefile.am
> > index ff49b7d..70fa549 100644
> > --- a/platform/linux-dpdk/Makefile.am
> > +++ b/platform/linux-dpdk/Makefile.am
> > @@ -67,13 +67,14 @@ __LIB__libodp_la_SOURCES = \
> >                          odp_buffer.c \
> >                          odp_buffer_pool.c \
> >                          ../linux-generic/odp_coremask.c \
> > -                        odp_init.c \
> > +                        ../linux-generic/odp_init.c \
> >                          odp_linux.c \
> >                          odp_packet.c \
> >                          odp_packet_dpdk.c \
> >                          ../linux-generic/odp_packet_flags.c \
> >                          odp_packet_io.c \
> >                          ../linux-generic/odp_packet_socket.c \
> > +                        odp_platform.c \
> >                          odp_queue.c \
> >                          ../linux-generic/odp_ring.c \
> >                          ../linux-generic/odp_rwlock.c \
> > diff --git a/platform/linux-dpdk/odp_init.c b/platform/linux-
> > dpdk/odp_platform.c
> > similarity index 50%
> > rename from platform/linux-dpdk/odp_init.c
> > rename to platform/linux-dpdk/odp_platform.c
> > index ecc2066..5a054fd 100644
> > --- a/platform/linux-dpdk/odp_init.c
> > +++ b/platform/linux-dpdk/odp_platform.c
> > @@ -1,4 +1,4 @@
> > -/* Copyright (c) 2013, Linaro Limited
> > +/* Copyright (c) 2014, Linaro Limited
> >   * All rights reserved.
> >   *
> >   * SPDX-License-Identifier:     BSD-3-Clause
> > @@ -9,7 +9,7 @@
> >  #include <odp_debug.h>
> >  #include <odp_packet_dpdk.h>
> >
> > -int odp_init_dpdk(void)
> > +int odp_init_platform(odp_global_platform_init_t *platform_params
> > ODP_UNUSED)
> >  {
> >       int test_argc = 5;
> >       char *test_argv[6];
> > @@ -50,64 +50,7 @@ int odp_init_dpdk(void)
> >       return 0;
> >  }
> >
> > -int odp_init_global(void)
> > +int odp_crypto_init_global(void)
>
> Is crypto init now in odp_platform.c? It should be in odp_crypto.c or
> similar...
>
no problem
I can make a dummy   odp_crypto.c for ks2 and point DPDK at the linux
generic. I was trying to avoid
connecting internals.

>
> >  {
> > -     odp_thread_init_global();
> > -
> > -     odp_system_info_init();
> > -
> > -     if (odp_init_dpdk()) {
> > -             ODP_ERR("ODP dpdk init failed.\n");
> > -             return -1;
> > -     }
> > -
> > -     if (odp_shm_init_global()) {
> > -             ODP_ERR("ODP shm init failed.\n");
> > -             return -1;
> > -     }
> > -
> > -     if (odp_buffer_pool_init_global()) {
> > -             ODP_ERR("ODP buffer pool init failed.\n");
> > -             return -1;
> > -     }
> > -
> > -     if (odp_queue_init_global()) {
> > -             ODP_ERR("ODP queue init failed.\n");
> > -             return -1;
> > -     }
> > -
> > -     if (odp_schedule_init_global()) {
> > -             ODP_ERR("ODP schedule init failed.\n");
> > -             return -1;
> > -     }
> > -
> > -     if (odp_pktio_init_global()) {
> > -             ODP_ERR("ODP packet io init failed.\n");
> > -             return -1;
> > -     }
> > -
> > -     if (odp_timer_init_global()) {
> > -             ODP_ERR("ODP timer init failed.\n");
> > -             return -1;
> > -     }
> > -
> > -     return 0;
> > -}
> > -
> > -
> > -int odp_init_local(int thr_id)
> > -{
> > -     odp_thread_init_local(thr_id);
> > -
> > -     if (odp_pktio_init_local()) {
> > -             ODP_ERR("ODP packet io local init failed.\n");
> > -             return -1;
> > -     }
> > -
> > -     if (odp_schedule_init_local()) {
> > -             ODP_ERR("ODP schedule local init failed.\n");
> > -             return -1;
> > -     }
> > -
> >       return 0;
> >  }
> > diff --git a/platform/linux-generic/Makefile.am b/platform/linux-
> > generic/Makefile.am
> > index f4dfdc1..c4aae9d 100644
> > --- a/platform/linux-generic/Makefile.am
> > +++ b/platform/linux-generic/Makefile.am
> > @@ -62,6 +62,7 @@ __LIB__libodp_la_SOURCES = \
> >                          odp_packet_flags.c \
> >                          odp_packet_io.c \
> >                          odp_packet_socket.c \
> > +                        odp_platform.c \
> >                          odp_queue.c \
> >                          odp_ring.c \
> >                          odp_rwlock.c \
> > diff --git a/platform/linux-generic/include/api/odp_init.h
> > b/platform/linux-generic/include/api/odp_init.h
> > index 490324a..ece4547 100644
> > --- a/platform/linux-generic/include/api/odp_init.h
> > +++ b/platform/linux-generic/include/api/odp_init.h
> > @@ -22,24 +22,42 @@ extern "C" {
> >
> >  #include <odp_std_types.h>
> >
> > -
> > -
> > +/** ODP  initialisation data.
> > + * Data  that is required to initialize the ODP API with the
> > + * application specific data such as specifying a logging callback, the
> > log
> > + * level etc.
> > + */
> > +typedef struct odp_global_init {
>
> The struct name should be "struct odp_global_init_t", "struct
> odp_global_init_s" or "struct odp_global_init_".
>
> The name without postfix (odp_global_init) should be reserved for variable
> names.
> no probelm
>
I will go with odp_init_s



> > +} odp_global_init_t;
>
> In short, this could be named odp_init_t. It's the main init struct there
> will be.
> If there will be a local init struct it could be then odp_init_local_t...
>
> > +
> > +/** ODP platform initialization data.
> > + * @note ODP API does nothing with this data. It is the underlying
> > + * implementation requires it and any data passed here is not portable.
> > + * It is required that the application takes care of identifying and
> > + * passing any required platform specific data.
> > + */
> > +typedef struct odp_global_platform_init {
>
> Same thing, rename struct.
>
no problem
odp_init_platform_s

>
> > +} odp_platform_init_t;
>
> Same thing, odp_platform_init_t ??
>
>
> >
> >  /**
> > - * Perform global ODP initalisation.
> > - *
> > - * This function must be called once before calling
> > - * any other ODP API functions.
> > + * Perform global ODP initialisation.
> >   *
> > + * This function must be called once before calling any other ODP API
> > + * functions.
> > + * @param[in] params Those parameters that are interpreted by the ODP
> API
>
> "Those parameters that are common to all ODP implementations. Specified in
> this API"
>
> API does not interpret, implementation does...
>

Ok, I am struggling with names

how about "platform SDK" for the underlying code such as DPDK, so that the
platform has an API
implementation -  which is actually implemented in terms of a platform SDK

We need to distinguish that implementations should not use the platform
parameters unless the underlying
framework requires it, not that any implementation is free to just add
local stuff - or thats how it feels to me at least.

>
> > + * @param[in] platform_params Those parameters that are passed without
> > + * interpretation by the ODP API to the implementation.
>
> " ... implementation specific parameter. Specified outside of ODP API"
>
> >   * @return 0 if successful
> >   */
> > -int odp_init_global(void);
> > +int odp_init_global(odp_global_init_t *params,
> > +                 odp_global_platform_init_t *platform_params);
> >
> >
> >  /**
> > - * Perform thread local ODP initalisation.
> > + * Perform thread local ODP initialisation.
> >   *
> >   * All threads must call this function before calling
> > + *
>
> Extra line feed?
>
Will fix

>
> >   * any other ODP API functions.
> >   * @param thr_id Thread id
> >   * @return 0 if successful
> > diff --git a/platform/linux-generic/include/odp_internal.h
> > b/platform/linux-generic/include/odp_internal.h
> > index aa79493..6c71fc9 100644
> > --- a/platform/linux-generic/include/odp_internal.h
> > +++ b/platform/linux-generic/include/odp_internal.h
> > @@ -4,6 +4,7 @@
> >   * SPDX-License-Identifier:     BSD-3-Clause
> >   */
> >
> > +#include <odp_init.h>
> >
> >  /**
> >   * @file
> > @@ -42,6 +43,8 @@ int odp_schedule_init_local(void);
> >  int odp_timer_init_global(void);
> >  int odp_timer_disarm_all(void);
> >
> > +int odp_init_platform(odp_global_platform_init_t *platform_params);
> > +
> >  #ifdef __cplusplus
> >  }
> >  #endif
> > diff --git a/platform/linux-generic/odp_init.c b/platform/linux-
> > generic/odp_init.c
> > index 5b7e192..f595def 100644
> > --- a/platform/linux-generic/odp_init.c
> > +++ b/platform/linux-generic/odp_init.c
> > @@ -8,13 +8,18 @@
> >  #include <odp_internal.h>
> >  #include <odp_debug.h>
> >
> > -
> > -int odp_init_global(void)
> > +int odp_init_global(odp_global_init_t *params  ODP_UNUSED,
> > +                 odp_global_platform_init_t *platform_params ODP_UNUSED)
> >  {
> >       odp_thread_init_global();
> >
> >       odp_system_info_init();
> >
> > +     if (odp_init_platform(platform_params)) {
>
> platform_params is used here, so remove ODP_UNUSED from argument list.
>
> On the other hand, it would be better to remove odp_init_platform()
> altogether since does not do anything. It's better to add it when actually
> used.
> The order of init calls is very important, e.g. now you could not allocate
> shared
> memory in odp_init_platform() because it's executed before shm init...
>
 odp_init_platform() does do something for DPDK / KS2 and we need a place
holder because
we want this to be shared between implementations don't we ?
This is the stuff that must happen before any ODP init starts to satisfy
the platform SDK

>
> - Petri
>
> > +             ODP_ERR("ODP platform init failed.\n");
> > +             return -1;
> > +     }
> > +
> >       if (odp_shm_init_global()) {
> >               ODP_ERR("ODP shm init failed.\n");
> >               return -1;
> > diff --git a/platform/linux-generic/odp_platform.c b/platform/linux-
> > generic/odp_platform.c
> > new file mode 100644
> > index 0000000..a9efa7f
> > --- /dev/null
> > +++ b/platform/linux-generic/odp_platform.c
> > @@ -0,0 +1,14 @@
> > +/* Copyright (c) 2014, Linaro Limited
> > + * All rights reserved.
> > + *
> > + * SPDX-License-Identifier:     BSD-3-Clause
> > + */
> > +
> > +#include <odp_init.h>
> > +#include <odp_internal.h>
> > +#include <odp_debug.h>
> > +
> > +int odp_init_platform(odp_global_platform_init_t *platform_params
> > ODP_UNUSED)
> > +{
> > +     return 0;
> > +}
>
>
>
>
>

Bill Fischofer Aug. 29, 2014, 5:10 p.m. UTC | #4

We've been using odp_<name>_t for typedef stuff, taking advantage that of
the fact that struct tags occupy a different namespace.  So

typedef struct odp_<name>_t {...} odp_<name>_t;

is perfectly valid and unambiguous C and avoids additional namespace
clutter.

odp_<name>_e has been suggested for ODP enums.

Bill


On Fri, Aug 29, 2014 at 12:05 PM, Mike Holmes <mike.holmes@linaro.org>
wrote:

>
>
>
> On 29 August 2014 03:59, Savolainen, Petri (NSN - FI/Espoo) <
> petri.savolainen@nsn.com> wrote:
>
>>
>>
>> > -----Original Message-----
>> > From: lng-odp-bounces@lists.linaro.org [mailto:lng-odp-
>> > bounces@lists.linaro.org] On Behalf Of ext Mike Holmes
>> > Sent: Thursday, August 28, 2014 9:26 PM
>> > To: lng-odp@lists.linaro.org
>> > Subject: [lng-odp] [PATCH v4] Add-global_init-paramiters
>> >
>> > Signed-off-by: Mike Holmes <mike.holmes@linaro.org>
>> > ---
>> >
>> > v2:
>> > Factor out odp_platform_init so that odp_global_init is only defined
>> > for linux_generic and resued.
>> >
>> > v3:
>> > Use -M to simplify the diff
>> > Fix missing doxygen comments
>> >
>> > v4:
>> > Improve the documentation.
>> >
>> >  example/generator/odp_generator.c                  |  2 +-
>> >  example/l2fwd/odp_l2fwd.c                          |  2 +-
>> >  example/odp_example/odp_example.c                  |  2 +-
>> >  example/packet/odp_pktio.c                         |  2 +-
>> >  example/timer/odp_timer_test.c                     |  2 +-
>> >  platform/linux-dpdk/Makefile.am                    |  3 +-
>> >  platform/linux-dpdk/{odp_init.c => odp_platform.c} | 63
>> +----------------
>> > --
>> >  platform/linux-generic/Makefile.am                 |  1 +
>> >  platform/linux-generic/include/api/odp_init.h      | 34 +++++++---
>> >  platform/linux-generic/include/odp_internal.h      |  3 +
>> >  platform/linux-generic/odp_init.c                  |  9 ++-
>> >  platform/linux-generic/odp_platform.c              | 14 +++++
>> >  platform/linux-keystone2/Makefile.am               |  3 +-
>> >  .../linux-keystone2/{odp_init.c => odp_platform.c} | 72
>> +++--------------
>> > -----
>> >  test/api_test/odp_common.c                         |  2 +-
>> >  15 files changed, 72 insertions(+), 142 deletions(-)
>> >  rename platform/linux-dpdk/{odp_init.c => odp_platform.c} (50%)
>> >  create mode 100644 platform/linux-generic/odp_platform.c
>> >  rename platform/linux-keystone2/{odp_init.c => odp_platform.c} (72%)
>> >
>> > diff --git a/example/generator/odp_generator.c
>> > b/example/generator/odp_generator.c
>> > index 9fa9b37..ba7aa68 100644
>> > --- a/example/generator/odp_generator.c
>> > +++ b/example/generator/odp_generator.c
>> > @@ -525,7 +525,7 @@ int main(int argc, char *argv[])
>> >       int core_count;
>> >
>> >       /* Init ODP before calling anything else */
>> > -     if (odp_init_global()) {
>> > +     if (odp_init_global(NULL, NULL)) {
>> >               ODP_ERR("Error: ODP global init failed.\n");
>> >               exit(EXIT_FAILURE);
>> >       }
>> > diff --git a/example/l2fwd/odp_l2fwd.c b/example/l2fwd/odp_l2fwd.c
>> > index d74449a..f8d6729 100644
>> > --- a/example/l2fwd/odp_l2fwd.c
>> > +++ b/example/l2fwd/odp_l2fwd.c
>> > @@ -323,7 +323,7 @@ int main(int argc, char *argv[])
>> >       odp_pktio_t pktio;
>> >
>> >       /* Init ODP before calling anything else */
>> > -     if (odp_init_global()) {
>> > +     if (odp_init_global(NULL, NULL)) {
>> >               ODP_ERR("Error: ODP global init failed.\n");
>> >               exit(EXIT_FAILURE);
>> >       }
>> > diff --git a/example/odp_example/odp_example.c
>> > b/example/odp_example/odp_example.c
>> > index f0bdf29..3d38ac7 100644
>> > --- a/example/odp_example/odp_example.c
>> > +++ b/example/odp_example/odp_example.c
>> > @@ -951,7 +951,7 @@ int main(int argc, char *argv[])
>> >
>> >       memset(thread_tbl, 0, sizeof(thread_tbl));
>> >
>> > -     if (odp_init_global()) {
>> > +     if (odp_init_global(NULL, NULL)) {
>> >               printf("ODP global init failed.\n");
>> >               return -1;
>> >       }
>> > diff --git a/example/packet/odp_pktio.c b/example/packet/odp_pktio.c
>> > index f247bd0..f3543a0 100644
>> > --- a/example/packet/odp_pktio.c
>> > +++ b/example/packet/odp_pktio.c
>> > @@ -311,7 +311,7 @@ int main(int argc, char *argv[])
>> >       int core_count;
>> >
>> >       /* Init ODP before calling anything else */
>> > -     if (odp_init_global()) {
>> > +     if (odp_init_global(NULL, NULL)) {
>> >               ODP_ERR("Error: ODP global init failed.\n");
>> >               exit(EXIT_FAILURE);
>> >       }
>> > diff --git a/example/timer/odp_timer_test.c
>> > b/example/timer/odp_timer_test.c
>> > index bf1d7df..abf90aa 100644
>> > --- a/example/timer/odp_timer_test.c
>> > +++ b/example/timer/odp_timer_test.c
>> > @@ -260,7 +260,7 @@ int main(int argc, char *argv[])
>> >
>> >       memset(thread_tbl, 0, sizeof(thread_tbl));
>> >
>> > -     if (odp_init_global()) {
>> > +     if (odp_init_global(NULL, NULL)) {
>> >               printf("ODP global init failed.\n");
>> >               return -1;
>> >       }
>> > diff --git a/platform/linux-dpdk/Makefile.am b/platform/linux-
>> > dpdk/Makefile.am
>> > index ff49b7d..70fa549 100644
>> > --- a/platform/linux-dpdk/Makefile.am
>> > +++ b/platform/linux-dpdk/Makefile.am
>> > @@ -67,13 +67,14 @@ __LIB__libodp_la_SOURCES = \
>> >                          odp_buffer.c \
>> >                          odp_buffer_pool.c \
>> >                          ../linux-generic/odp_coremask.c \
>> > -                        odp_init.c \
>> > +                        ../linux-generic/odp_init.c \
>> >                          odp_linux.c \
>> >                          odp_packet.c \
>> >                          odp_packet_dpdk.c \
>> >                          ../linux-generic/odp_packet_flags.c \
>> >                          odp_packet_io.c \
>> >                          ../linux-generic/odp_packet_socket.c \
>> > +                        odp_platform.c \
>> >                          odp_queue.c \
>> >                          ../linux-generic/odp_ring.c \
>> >                          ../linux-generic/odp_rwlock.c \
>> > diff --git a/platform/linux-dpdk/odp_init.c b/platform/linux-
>> > dpdk/odp_platform.c
>> > similarity index 50%
>> > rename from platform/linux-dpdk/odp_init.c
>> > rename to platform/linux-dpdk/odp_platform.c
>> > index ecc2066..5a054fd 100644
>> > --- a/platform/linux-dpdk/odp_init.c
>> > +++ b/platform/linux-dpdk/odp_platform.c
>> > @@ -1,4 +1,4 @@
>> > -/* Copyright (c) 2013, Linaro Limited
>> > +/* Copyright (c) 2014, Linaro Limited
>> >   * All rights reserved.
>> >   *
>> >   * SPDX-License-Identifier:     BSD-3-Clause
>> > @@ -9,7 +9,7 @@
>> >  #include <odp_debug.h>
>> >  #include <odp_packet_dpdk.h>
>> >
>> > -int odp_init_dpdk(void)
>> > +int odp_init_platform(odp_global_platform_init_t *platform_params
>> > ODP_UNUSED)
>> >  {
>> >       int test_argc = 5;
>> >       char *test_argv[6];
>> > @@ -50,64 +50,7 @@ int odp_init_dpdk(void)
>> >       return 0;
>> >  }
>> >
>> > -int odp_init_global(void)
>> > +int odp_crypto_init_global(void)
>>
>> Is crypto init now in odp_platform.c? It should be in odp_crypto.c or
>> similar...
>>
> no problem
> I can make a dummy   odp_crypto.c for ks2 and point DPDK at the linux
> generic. I was trying to avoid
> connecting internals.
>
>>
>> >  {
>> > -     odp_thread_init_global();
>> > -
>> > -     odp_system_info_init();
>> > -
>> > -     if (odp_init_dpdk()) {
>> > -             ODP_ERR("ODP dpdk init failed.\n");
>> > -             return -1;
>> > -     }
>> > -
>> > -     if (odp_shm_init_global()) {
>> > -             ODP_ERR("ODP shm init failed.\n");
>> > -             return -1;
>> > -     }
>> > -
>> > -     if (odp_buffer_pool_init_global()) {
>> > -             ODP_ERR("ODP buffer pool init failed.\n");
>> > -             return -1;
>> > -     }
>> > -
>> > -     if (odp_queue_init_global()) {
>> > -             ODP_ERR("ODP queue init failed.\n");
>> > -             return -1;
>> > -     }
>> > -
>> > -     if (odp_schedule_init_global()) {
>> > -             ODP_ERR("ODP schedule init failed.\n");
>> > -             return -1;
>> > -     }
>> > -
>> > -     if (odp_pktio_init_global()) {
>> > -             ODP_ERR("ODP packet io init failed.\n");
>> > -             return -1;
>> > -     }
>> > -
>> > -     if (odp_timer_init_global()) {
>> > -             ODP_ERR("ODP timer init failed.\n");
>> > -             return -1;
>> > -     }
>> > -
>> > -     return 0;
>> > -}
>> > -
>> > -
>> > -int odp_init_local(int thr_id)
>> > -{
>> > -     odp_thread_init_local(thr_id);
>> > -
>> > -     if (odp_pktio_init_local()) {
>> > -             ODP_ERR("ODP packet io local init failed.\n");
>> > -             return -1;
>> > -     }
>> > -
>> > -     if (odp_schedule_init_local()) {
>> > -             ODP_ERR("ODP schedule local init failed.\n");
>> > -             return -1;
>> > -     }
>> > -
>> >       return 0;
>> >  }
>> > diff --git a/platform/linux-generic/Makefile.am b/platform/linux-
>> > generic/Makefile.am
>> > index f4dfdc1..c4aae9d 100644
>> > --- a/platform/linux-generic/Makefile.am
>> > +++ b/platform/linux-generic/Makefile.am
>> > @@ -62,6 +62,7 @@ __LIB__libodp_la_SOURCES = \
>> >                          odp_packet_flags.c \
>> >                          odp_packet_io.c \
>> >                          odp_packet_socket.c \
>> > +                        odp_platform.c \
>> >                          odp_queue.c \
>> >                          odp_ring.c \
>> >                          odp_rwlock.c \
>> > diff --git a/platform/linux-generic/include/api/odp_init.h
>> > b/platform/linux-generic/include/api/odp_init.h
>> > index 490324a..ece4547 100644
>> > --- a/platform/linux-generic/include/api/odp_init.h
>> > +++ b/platform/linux-generic/include/api/odp_init.h
>> > @@ -22,24 +22,42 @@ extern "C" {
>> >
>> >  #include <odp_std_types.h>
>> >
>> > -
>> > -
>> > +/** ODP  initialisation data.
>> > + * Data  that is required to initialize the ODP API with the
>> > + * application specific data such as specifying a logging callback, the
>> > log
>> > + * level etc.
>> > + */
>> > +typedef struct odp_global_init {
>>
>> The struct name should be "struct odp_global_init_t", "struct
>> odp_global_init_s" or "struct odp_global_init_".
>>
>> The name without postfix (odp_global_init) should be reserved for
>> variable names.
>> no probelm
>>
> I will go with odp_init_s
>
>
>
>> > +} odp_global_init_t;
>>
>> In short, this could be named odp_init_t. It's the main init struct there
>> will be.
>> If there will be a local init struct it could be then odp_init_local_t...
>>
>> > +
>> > +/** ODP platform initialization data.
>> > + * @note ODP API does nothing with this data. It is the underlying
>> > + * implementation requires it and any data passed here is not portable.
>> > + * It is required that the application takes care of identifying and
>> > + * passing any required platform specific data.
>> > + */
>> > +typedef struct odp_global_platform_init {
>>
>> Same thing, rename struct.
>>
> no problem
> odp_init_platform_s
>
>>
>> > +} odp_platform_init_t;
>>
>>
>> Same thing, odp_platform_init_t ??
>>
>>
>> >
>> >  /**
>> > - * Perform global ODP initalisation.
>> > - *
>> > - * This function must be called once before calling
>> > - * any other ODP API functions.
>> > + * Perform global ODP initialisation.
>> >   *
>> > + * This function must be called once before calling any other ODP API
>> > + * functions.
>> > + * @param[in] params Those parameters that are interpreted by the ODP
>> API
>>
>> "Those parameters that are common to all ODP implementations. Specified
>> in this API"
>>
>> API does not interpret, implementation does...
>>
>
> Ok, I am struggling with names
>
> how about "platform SDK" for the underlying code such as DPDK, so that the
> platform has an API
> implementation -  which is actually implemented in terms of a platform SDK
>
> We need to distinguish that implementations should not use the platform
> parameters unless the underlying
> framework requires it, not that any implementation is free to just add
> local stuff - or thats how it feels to me at least.
>
>>
>> > + * @param[in] platform_params Those parameters that are passed without
>> > + * interpretation by the ODP API to the implementation.
>>
>> " ... implementation specific parameter. Specified outside of ODP API"
>>
>> >   * @return 0 if successful
>> >   */
>> > -int odp_init_global(void);
>> > +int odp_init_global(odp_global_init_t *params,
>> > +                 odp_global_platform_init_t *platform_params);
>> >
>> >
>> >  /**
>> > - * Perform thread local ODP initalisation.
>> > + * Perform thread local ODP initialisation.
>> >   *
>> >   * All threads must call this function before calling
>> > + *
>>
>> Extra line feed?
>>
> Will fix
>
>>
>> >   * any other ODP API functions.
>> >   * @param thr_id Thread id
>> >   * @return 0 if successful
>> > diff --git a/platform/linux-generic/include/odp_internal.h
>> > b/platform/linux-generic/include/odp_internal.h
>> > index aa79493..6c71fc9 100644
>> > --- a/platform/linux-generic/include/odp_internal.h
>> > +++ b/platform/linux-generic/include/odp_internal.h
>> > @@ -4,6 +4,7 @@
>> >   * SPDX-License-Identifier:     BSD-3-Clause
>> >   */
>> >
>> > +#include <odp_init.h>
>> >
>> >  /**
>> >   * @file
>> > @@ -42,6 +43,8 @@ int odp_schedule_init_local(void);
>> >  int odp_timer_init_global(void);
>> >  int odp_timer_disarm_all(void);
>> >
>> > +int odp_init_platform(odp_global_platform_init_t *platform_params);
>> > +
>> >  #ifdef __cplusplus
>> >  }
>> >  #endif
>> > diff --git a/platform/linux-generic/odp_init.c b/platform/linux-
>> > generic/odp_init.c
>> > index 5b7e192..f595def 100644
>> > --- a/platform/linux-generic/odp_init.c
>> > +++ b/platform/linux-generic/odp_init.c
>> > @@ -8,13 +8,18 @@
>> >  #include <odp_internal.h>
>> >  #include <odp_debug.h>
>> >
>> > -
>> > -int odp_init_global(void)
>> > +int odp_init_global(odp_global_init_t *params  ODP_UNUSED,
>> > +                 odp_global_platform_init_t *platform_params
>> ODP_UNUSED)
>> >  {
>> >       odp_thread_init_global();
>> >
>> >       odp_system_info_init();
>> >
>> > +     if (odp_init_platform(platform_params)) {
>>
>> platform_params is used here, so remove ODP_UNUSED from argument list.
>>
>> On the other hand, it would be better to remove odp_init_platform()
>> altogether since does not do anything. It's better to add it when
>> actually used.
>> The order of init calls is very important, e.g. now you could not
>> allocate shared
>> memory in odp_init_platform() because it's executed before shm init...
>>
>  odp_init_platform() does do something for DPDK / KS2 and we need a place
> holder because
> we want this to be shared between implementations don't we ?
> This is the stuff that must happen before any ODP init starts to satisfy
> the platform SDK
>
>>
>> - Petri
>>
>> > +             ODP_ERR("ODP platform init failed.\n");
>> > +             return -1;
>> > +     }
>> > +
>> >       if (odp_shm_init_global()) {
>> >               ODP_ERR("ODP shm init failed.\n");
>> >               return -1;
>> > diff --git a/platform/linux-generic/odp_platform.c b/platform/linux-
>> > generic/odp_platform.c
>> > new file mode 100644
>> > index 0000000..a9efa7f
>> > --- /dev/null
>> > +++ b/platform/linux-generic/odp_platform.c
>> > @@ -0,0 +1,14 @@
>> > +/* Copyright (c) 2014, Linaro Limited
>> > + * All rights reserved.
>> > + *
>> > + * SPDX-License-Identifier:     BSD-3-Clause
>> > + */
>> > +
>> > +#include <odp_init.h>
>> > +#include <odp_internal.h>
>> > +#include <odp_debug.h>
>> > +
>> > +int odp_init_platform(odp_global_platform_init_t *platform_params
>> > ODP_UNUSED)
>> > +{
>> > +     return 0;
>> > +}
>>
>>
>>
>>
>>
>
>
> --
> *Mike Holmes*
> Linaro Technical Manager / Lead
> LNG - ODP
>
> _______________________________________________
> lng-odp mailing list
> lng-odp@lists.linaro.org
> http://lists.linaro.org/mailman/listinfo/lng-odp
>
>

Mike Holmes Aug. 29, 2014, 5:19 p.m. UTC | #5

Ok, I need a better feel for consensus here or it will flip flop :), as
this impacts all structs we really need a final answer

typedef struct odp_<name>_t {...} odp_<name>_t;
OR
typedef struct odp_<name>_s {...} odp_<name>_t;
OR
typedef struct {...} odp_<name>_t;

I prefer the latter, why even name it unless you need to reference the
name inside the struct being typedefed ?




On 29 August 2014 13:10, Bill Fischofer <bill.fischofer@linaro.org> wrote:

> We've been using odp_<name>_t for typedef stuff, taking advantage that of
> the fact that struct tags occupy a different namespace.  So
>
> typedef struct odp_<name>_t {...} odp_<name>_t;
>
> is perfectly valid and unambiguous C and avoids additional namespace
> clutter.
>
> odp_<name>_e has been suggested for ODP enums.
>
> Bill
>
>
> On Fri, Aug 29, 2014 at 12:05 PM, Mike Holmes <mike.holmes@linaro.org>
> wrote:
>
>>
>>
>>
>> On 29 August 2014 03:59, Savolainen, Petri (NSN - FI/Espoo) <
>> petri.savolainen@nsn.com> wrote:
>>
>>>
>>>
>>> > -----Original Message-----
>>> > From: lng-odp-bounces@lists.linaro.org [mailto:lng-odp-
>>> > bounces@lists.linaro.org] On Behalf Of ext Mike Holmes
>>> > Sent: Thursday, August 28, 2014 9:26 PM
>>> > To: lng-odp@lists.linaro.org
>>> > Subject: [lng-odp] [PATCH v4] Add-global_init-paramiters
>>> >
>>> > Signed-off-by: Mike Holmes <mike.holmes@linaro.org>
>>> > ---
>>> >
>>> > v2:
>>> > Factor out odp_platform_init so that odp_global_init is only defined
>>> > for linux_generic and resued.
>>> >
>>> > v3:
>>> > Use -M to simplify the diff
>>> > Fix missing doxygen comments
>>> >
>>> > v4:
>>> > Improve the documentation.
>>> >
>>> >  example/generator/odp_generator.c                  |  2 +-
>>> >  example/l2fwd/odp_l2fwd.c                          |  2 +-
>>> >  example/odp_example/odp_example.c                  |  2 +-
>>> >  example/packet/odp_pktio.c                         |  2 +-
>>> >  example/timer/odp_timer_test.c                     |  2 +-
>>> >  platform/linux-dpdk/Makefile.am                    |  3 +-
>>> >  platform/linux-dpdk/{odp_init.c => odp_platform.c} | 63
>>> +----------------
>>> > --
>>> >  platform/linux-generic/Makefile.am                 |  1 +
>>> >  platform/linux-generic/include/api/odp_init.h      | 34 +++++++---
>>> >  platform/linux-generic/include/odp_internal.h      |  3 +
>>> >  platform/linux-generic/odp_init.c                  |  9 ++-
>>> >  platform/linux-generic/odp_platform.c              | 14 +++++
>>> >  platform/linux-keystone2/Makefile.am               |  3 +-
>>> >  .../linux-keystone2/{odp_init.c => odp_platform.c} | 72
>>> +++--------------
>>> > -----
>>> >  test/api_test/odp_common.c                         |  2 +-
>>> >  15 files changed, 72 insertions(+), 142 deletions(-)
>>> >  rename platform/linux-dpdk/{odp_init.c => odp_platform.c} (50%)
>>> >  create mode 100644 platform/linux-generic/odp_platform.c
>>> >  rename platform/linux-keystone2/{odp_init.c => odp_platform.c} (72%)
>>> >
>>> > diff --git a/example/generator/odp_generator.c
>>> > b/example/generator/odp_generator.c
>>> > index 9fa9b37..ba7aa68 100644
>>> > --- a/example/generator/odp_generator.c
>>> > +++ b/example/generator/odp_generator.c
>>> > @@ -525,7 +525,7 @@ int main(int argc, char *argv[])
>>> >       int core_count;
>>> >
>>> >       /* Init ODP before calling anything else */
>>> > -     if (odp_init_global()) {
>>> > +     if (odp_init_global(NULL, NULL)) {
>>> >               ODP_ERR("Error: ODP global init failed.\n");
>>> >               exit(EXIT_FAILURE);
>>> >       }
>>> > diff --git a/example/l2fwd/odp_l2fwd.c b/example/l2fwd/odp_l2fwd.c
>>> > index d74449a..f8d6729 100644
>>> > --- a/example/l2fwd/odp_l2fwd.c
>>> > +++ b/example/l2fwd/odp_l2fwd.c
>>> > @@ -323,7 +323,7 @@ int main(int argc, char *argv[])
>>> >       odp_pktio_t pktio;
>>> >
>>> >       /* Init ODP before calling anything else */
>>> > -     if (odp_init_global()) {
>>> > +     if (odp_init_global(NULL, NULL)) {
>>> >               ODP_ERR("Error: ODP global init failed.\n");
>>> >               exit(EXIT_FAILURE);
>>> >       }
>>> > diff --git a/example/odp_example/odp_example.c
>>> > b/example/odp_example/odp_example.c
>>> > index f0bdf29..3d38ac7 100644
>>> > --- a/example/odp_example/odp_example.c
>>> > +++ b/example/odp_example/odp_example.c
>>> > @@ -951,7 +951,7 @@ int main(int argc, char *argv[])
>>> >
>>> >       memset(thread_tbl, 0, sizeof(thread_tbl));
>>> >
>>> > -     if (odp_init_global()) {
>>> > +     if (odp_init_global(NULL, NULL)) {
>>> >               printf("ODP global init failed.\n");
>>> >               return -1;
>>> >       }
>>> > diff --git a/example/packet/odp_pktio.c b/example/packet/odp_pktio.c
>>> > index f247bd0..f3543a0 100644
>>> > --- a/example/packet/odp_pktio.c
>>> > +++ b/example/packet/odp_pktio.c
>>> > @@ -311,7 +311,7 @@ int main(int argc, char *argv[])
>>> >       int core_count;
>>> >
>>> >       /* Init ODP before calling anything else */
>>> > -     if (odp_init_global()) {
>>> > +     if (odp_init_global(NULL, NULL)) {
>>> >               ODP_ERR("Error: ODP global init failed.\n");
>>> >               exit(EXIT_FAILURE);
>>> >       }
>>> > diff --git a/example/timer/odp_timer_test.c
>>> > b/example/timer/odp_timer_test.c
>>> > index bf1d7df..abf90aa 100644
>>> > --- a/example/timer/odp_timer_test.c
>>> > +++ b/example/timer/odp_timer_test.c
>>> > @@ -260,7 +260,7 @@ int main(int argc, char *argv[])
>>> >
>>> >       memset(thread_tbl, 0, sizeof(thread_tbl));
>>> >
>>> > -     if (odp_init_global()) {
>>> > +     if (odp_init_global(NULL, NULL)) {
>>> >               printf("ODP global init failed.\n");
>>> >               return -1;
>>> >       }
>>> > diff --git a/platform/linux-dpdk/Makefile.am b/platform/linux-
>>> > dpdk/Makefile.am
>>> > index ff49b7d..70fa549 100644
>>> > --- a/platform/linux-dpdk/Makefile.am
>>> > +++ b/platform/linux-dpdk/Makefile.am
>>> > @@ -67,13 +67,14 @@ __LIB__libodp_la_SOURCES = \
>>> >                          odp_buffer.c \
>>> >                          odp_buffer_pool.c \
>>> >                          ../linux-generic/odp_coremask.c \
>>> > -                        odp_init.c \
>>> > +                        ../linux-generic/odp_init.c \
>>> >                          odp_linux.c \
>>> >                          odp_packet.c \
>>> >                          odp_packet_dpdk.c \
>>> >                          ../linux-generic/odp_packet_flags.c \
>>> >                          odp_packet_io.c \
>>> >                          ../linux-generic/odp_packet_socket.c \
>>> > +                        odp_platform.c \
>>> >                          odp_queue.c \
>>> >                          ../linux-generic/odp_ring.c \
>>> >                          ../linux-generic/odp_rwlock.c \
>>> > diff --git a/platform/linux-dpdk/odp_init.c b/platform/linux-
>>> > dpdk/odp_platform.c
>>> > similarity index 50%
>>> > rename from platform/linux-dpdk/odp_init.c
>>> > rename to platform/linux-dpdk/odp_platform.c
>>> > index ecc2066..5a054fd 100644
>>> > --- a/platform/linux-dpdk/odp_init.c
>>> > +++ b/platform/linux-dpdk/odp_platform.c
>>> > @@ -1,4 +1,4 @@
>>> > -/* Copyright (c) 2013, Linaro Limited
>>> > +/* Copyright (c) 2014, Linaro Limited
>>> >   * All rights reserved.
>>> >   *
>>> >   * SPDX-License-Identifier:     BSD-3-Clause
>>> > @@ -9,7 +9,7 @@
>>> >  #include <odp_debug.h>
>>> >  #include <odp_packet_dpdk.h>
>>> >
>>> > -int odp_init_dpdk(void)
>>> > +int odp_init_platform(odp_global_platform_init_t *platform_params
>>> > ODP_UNUSED)
>>> >  {
>>> >       int test_argc = 5;
>>> >       char *test_argv[6];
>>> > @@ -50,64 +50,7 @@ int odp_init_dpdk(void)
>>> >       return 0;
>>> >  }
>>> >
>>> > -int odp_init_global(void)
>>> > +int odp_crypto_init_global(void)
>>>
>>> Is crypto init now in odp_platform.c? It should be in odp_crypto.c or
>>> similar...
>>>
>> no problem
>> I can make a dummy   odp_crypto.c for ks2 and point DPDK at the linux
>> generic. I was trying to avoid
>> connecting internals.
>>
>>>
>>> >  {
>>> > -     odp_thread_init_global();
>>> > -
>>> > -     odp_system_info_init();
>>> > -
>>> > -     if (odp_init_dpdk()) {
>>> > -             ODP_ERR("ODP dpdk init failed.\n");
>>> > -             return -1;
>>> > -     }
>>> > -
>>> > -     if (odp_shm_init_global()) {
>>> > -             ODP_ERR("ODP shm init failed.\n");
>>> > -             return -1;
>>> > -     }
>>> > -
>>> > -     if (odp_buffer_pool_init_global()) {
>>> > -             ODP_ERR("ODP buffer pool init failed.\n");
>>> > -             return -1;
>>> > -     }
>>> > -
>>> > -     if (odp_queue_init_global()) {
>>> > -             ODP_ERR("ODP queue init failed.\n");
>>> > -             return -1;
>>> > -     }
>>> > -
>>> > -     if (odp_schedule_init_global()) {
>>> > -             ODP_ERR("ODP schedule init failed.\n");
>>> > -             return -1;
>>> > -     }
>>> > -
>>> > -     if (odp_pktio_init_global()) {
>>> > -             ODP_ERR("ODP packet io init failed.\n");
>>> > -             return -1;
>>> > -     }
>>> > -
>>> > -     if (odp_timer_init_global()) {
>>> > -             ODP_ERR("ODP timer init failed.\n");
>>> > -             return -1;
>>> > -     }
>>> > -
>>> > -     return 0;
>>> > -}
>>> > -
>>> > -
>>> > -int odp_init_local(int thr_id)
>>> > -{
>>> > -     odp_thread_init_local(thr_id);
>>> > -
>>> > -     if (odp_pktio_init_local()) {
>>> > -             ODP_ERR("ODP packet io local init failed.\n");
>>> > -             return -1;
>>> > -     }
>>> > -
>>> > -     if (odp_schedule_init_local()) {
>>> > -             ODP_ERR("ODP schedule local init failed.\n");
>>> > -             return -1;
>>> > -     }
>>> > -
>>> >       return 0;
>>> >  }
>>> > diff --git a/platform/linux-generic/Makefile.am b/platform/linux-
>>> > generic/Makefile.am
>>> > index f4dfdc1..c4aae9d 100644
>>> > --- a/platform/linux-generic/Makefile.am
>>> > +++ b/platform/linux-generic/Makefile.am
>>> > @@ -62,6 +62,7 @@ __LIB__libodp_la_SOURCES = \
>>> >                          odp_packet_flags.c \
>>> >                          odp_packet_io.c \
>>> >                          odp_packet_socket.c \
>>> > +                        odp_platform.c \
>>> >                          odp_queue.c \
>>> >                          odp_ring.c \
>>> >                          odp_rwlock.c \
>>> > diff --git a/platform/linux-generic/include/api/odp_init.h
>>> > b/platform/linux-generic/include/api/odp_init.h
>>> > index 490324a..ece4547 100644
>>> > --- a/platform/linux-generic/include/api/odp_init.h
>>> > +++ b/platform/linux-generic/include/api/odp_init.h
>>> > @@ -22,24 +22,42 @@ extern "C" {
>>> >
>>> >  #include <odp_std_types.h>
>>> >
>>> > -
>>> > -
>>> > +/** ODP  initialisation data.
>>> > + * Data  that is required to initialize the ODP API with the
>>> > + * application specific data such as specifying a logging callback,
>>> the
>>> > log
>>> > + * level etc.
>>> > + */
>>> > +typedef struct odp_global_init {
>>>
>>> The struct name should be "struct odp_global_init_t", "struct
>>> odp_global_init_s" or "struct odp_global_init_".
>>>
>>> The name without postfix (odp_global_init) should be reserved for
>>> variable names.
>>> no probelm
>>>
>> I will go with odp_init_s
>>
>>
>>
>>> > +} odp_global_init_t;
>>>
>>> In short, this could be named odp_init_t. It's the main init struct
>>> there will be.
>>> If there will be a local init struct it could be then odp_init_local_t...
>>>
>>> > +
>>> > +/** ODP platform initialization data.
>>> > + * @note ODP API does nothing with this data. It is the underlying
>>> > + * implementation requires it and any data passed here is not
>>> portable.
>>> > + * It is required that the application takes care of identifying and
>>> > + * passing any required platform specific data.
>>> > + */
>>> > +typedef struct odp_global_platform_init {
>>>
>>> Same thing, rename struct.
>>>
>> no problem
>> odp_init_platform_s
>>
>>>
>>> > +} odp_platform_init_t;
>>>
>>>
>>> Same thing, odp_platform_init_t ??
>>>
>>>
>>> >
>>> >  /**
>>> > - * Perform global ODP initalisation.
>>> > - *
>>> > - * This function must be called once before calling
>>> > - * any other ODP API functions.
>>> > + * Perform global ODP initialisation.
>>> >   *
>>> > + * This function must be called once before calling any other ODP API
>>> > + * functions.
>>> > + * @param[in] params Those parameters that are interpreted by the ODP
>>> API
>>>
>>> "Those parameters that are common to all ODP implementations. Specified
>>> in this API"
>>>
>>> API does not interpret, implementation does...
>>>
>>
>> Ok, I am struggling with names
>>
>> how about "platform SDK" for the underlying code such as DPDK, so that
>> the platform has an API
>> implementation -  which is actually implemented in terms of a platform SDK
>>
>> We need to distinguish that implementations should not use the platform
>> parameters unless the underlying
>> framework requires it, not that any implementation is free to just add
>> local stuff - or thats how it feels to me at least.
>>
>>>
>>> > + * @param[in] platform_params Those parameters that are passed without
>>> > + * interpretation by the ODP API to the implementation.
>>>
>>> " ... implementation specific parameter. Specified outside of ODP API"
>>>
>>> >   * @return 0 if successful
>>> >   */
>>> > -int odp_init_global(void);
>>> > +int odp_init_global(odp_global_init_t *params,
>>> > +                 odp_global_platform_init_t *platform_params);
>>> >
>>> >
>>> >  /**
>>> > - * Perform thread local ODP initalisation.
>>> > + * Perform thread local ODP initialisation.
>>> >   *
>>> >   * All threads must call this function before calling
>>> > + *
>>>
>>> Extra line feed?
>>>
>> Will fix
>>
>>>
>>> >   * any other ODP API functions.
>>> >   * @param thr_id Thread id
>>> >   * @return 0 if successful
>>> > diff --git a/platform/linux-generic/include/odp_internal.h
>>> > b/platform/linux-generic/include/odp_internal.h
>>> > index aa79493..6c71fc9 100644
>>> > --- a/platform/linux-generic/include/odp_internal.h
>>> > +++ b/platform/linux-generic/include/odp_internal.h
>>> > @@ -4,6 +4,7 @@
>>> >   * SPDX-License-Identifier:     BSD-3-Clause
>>> >   */
>>> >
>>> > +#include <odp_init.h>
>>> >
>>> >  /**
>>> >   * @file
>>> > @@ -42,6 +43,8 @@ int odp_schedule_init_local(void);
>>> >  int odp_timer_init_global(void);
>>> >  int odp_timer_disarm_all(void);
>>> >
>>> > +int odp_init_platform(odp_global_platform_init_t *platform_params);
>>> > +
>>> >  #ifdef __cplusplus
>>> >  }
>>> >  #endif
>>> > diff --git a/platform/linux-generic/odp_init.c b/platform/linux-
>>> > generic/odp_init.c
>>> > index 5b7e192..f595def 100644
>>> > --- a/platform/linux-generic/odp_init.c
>>> > +++ b/platform/linux-generic/odp_init.c
>>> > @@ -8,13 +8,18 @@
>>> >  #include <odp_internal.h>
>>> >  #include <odp_debug.h>
>>> >
>>> > -
>>> > -int odp_init_global(void)
>>> > +int odp_init_global(odp_global_init_t *params  ODP_UNUSED,
>>> > +                 odp_global_platform_init_t *platform_params
>>> ODP_UNUSED)
>>> >  {
>>> >       odp_thread_init_global();
>>> >
>>> >       odp_system_info_init();
>>> >
>>> > +     if (odp_init_platform(platform_params)) {
>>>
>>> platform_params is used here, so remove ODP_UNUSED from argument list.
>>>
>>> On the other hand, it would be better to remove odp_init_platform()
>>> altogether since does not do anything. It's better to add it when
>>> actually used.
>>> The order of init calls is very important, e.g. now you could not
>>> allocate shared
>>> memory in odp_init_platform() because it's executed before shm init...
>>>
>>  odp_init_platform() does do something for DPDK / KS2 and we need a place
>> holder because
>> we want this to be shared between implementations don't we ?
>> This is the stuff that must happen before any ODP init starts to satisfy
>> the platform SDK
>>
>>>
>>> - Petri
>>>
>>> > +             ODP_ERR("ODP platform init failed.\n");
>>> > +             return -1;
>>> > +     }
>>> > +
>>> >       if (odp_shm_init_global()) {
>>> >               ODP_ERR("ODP shm init failed.\n");
>>> >               return -1;
>>> > diff --git a/platform/linux-generic/odp_platform.c b/platform/linux-
>>> > generic/odp_platform.c
>>> > new file mode 100644
>>> > index 0000000..a9efa7f
>>> > --- /dev/null
>>> > +++ b/platform/linux-generic/odp_platform.c
>>> > @@ -0,0 +1,14 @@
>>> > +/* Copyright (c) 2014, Linaro Limited
>>> > + * All rights reserved.
>>> > + *
>>> > + * SPDX-License-Identifier:     BSD-3-Clause
>>> > + */
>>> > +
>>> > +#include <odp_init.h>
>>> > +#include <odp_internal.h>
>>> > +#include <odp_debug.h>
>>> > +
>>> > +int odp_init_platform(odp_global_platform_init_t *platform_params
>>> > ODP_UNUSED)
>>> > +{
>>> > +     return 0;
>>> > +}
>>>
>>>
>>>
>>>
>>>
>>
>>
>> --
>> *Mike Holmes*
>> Linaro Technical Manager / Lead
>> LNG - ODP
>>
>> _______________________________________________
>> lng-odp mailing list
>> lng-odp@lists.linaro.org
>> http://lists.linaro.org/mailman/listinfo/lng-odp
>>
>>
>

Bill Fischofer Aug. 29, 2014, 5:22 p.m. UTC | #6

I'm happy with either 1 or 3.  And yes, the purpose of the struct tag is to
permit self-reference.  Since it's not intended to be used by anyone except
the typedef itself it can be at the implementer's discretion.  If you have
a self-referencing struct then use it, otherwise you can either omit it or
include it to avoid the need to add it later should you extend things to
include a self reference.


On Fri, Aug 29, 2014 at 12:19 PM, Mike Holmes <mike.holmes@linaro.org>
wrote:

> Ok, I need a better feel for consensus here or it will flip flop :), as
> this impacts all structs we really need a final answer
>
> typedef struct odp_<name>_t {...} odp_<name>_t;
> OR
> typedef struct odp_<name>_s {...} odp_<name>_t;
> OR
> typedef struct {...} odp_<name>_t;
>
> I prefer the latter, why even name it unless you need to reference the
> name inside the struct being typedefed ?
>
>
>
>
> On 29 August 2014 13:10, Bill Fischofer <bill.fischofer@linaro.org> wrote:
>
>> We've been using odp_<name>_t for typedef stuff, taking advantage that of
>> the fact that struct tags occupy a different namespace.  So
>>
>> typedef struct odp_<name>_t {...} odp_<name>_t;
>>
>> is perfectly valid and unambiguous C and avoids additional namespace
>> clutter.
>>
>> odp_<name>_e has been suggested for ODP enums.
>>
>> Bill
>>
>>
>> On Fri, Aug 29, 2014 at 12:05 PM, Mike Holmes <mike.holmes@linaro.org>
>> wrote:
>>
>>>
>>>
>>>
>>> On 29 August 2014 03:59, Savolainen, Petri (NSN - FI/Espoo) <
>>> petri.savolainen@nsn.com> wrote:
>>>
>>>>
>>>>
>>>> > -----Original Message-----
>>>> > From: lng-odp-bounces@lists.linaro.org [mailto:lng-odp-
>>>> > bounces@lists.linaro.org] On Behalf Of ext Mike Holmes
>>>> > Sent: Thursday, August 28, 2014 9:26 PM
>>>> > To: lng-odp@lists.linaro.org
>>>> > Subject: [lng-odp] [PATCH v4] Add-global_init-paramiters
>>>> >
>>>> > Signed-off-by: Mike Holmes <mike.holmes@linaro.org>
>>>> > ---
>>>> >
>>>> > v2:
>>>> > Factor out odp_platform_init so that odp_global_init is only defined
>>>> > for linux_generic and resued.
>>>> >
>>>> > v3:
>>>> > Use -M to simplify the diff
>>>> > Fix missing doxygen comments
>>>> >
>>>> > v4:
>>>> > Improve the documentation.
>>>> >
>>>> >  example/generator/odp_generator.c                  |  2 +-
>>>> >  example/l2fwd/odp_l2fwd.c                          |  2 +-
>>>> >  example/odp_example/odp_example.c                  |  2 +-
>>>> >  example/packet/odp_pktio.c                         |  2 +-
>>>> >  example/timer/odp_timer_test.c                     |  2 +-
>>>> >  platform/linux-dpdk/Makefile.am                    |  3 +-
>>>> >  platform/linux-dpdk/{odp_init.c => odp_platform.c} | 63
>>>> +----------------
>>>> > --
>>>> >  platform/linux-generic/Makefile.am                 |  1 +
>>>> >  platform/linux-generic/include/api/odp_init.h      | 34 +++++++---
>>>> >  platform/linux-generic/include/odp_internal.h      |  3 +
>>>> >  platform/linux-generic/odp_init.c                  |  9 ++-
>>>> >  platform/linux-generic/odp_platform.c              | 14 +++++
>>>> >  platform/linux-keystone2/Makefile.am               |  3 +-
>>>> >  .../linux-keystone2/{odp_init.c => odp_platform.c} | 72
>>>> +++--------------
>>>> > -----
>>>> >  test/api_test/odp_common.c                         |  2 +-
>>>> >  15 files changed, 72 insertions(+), 142 deletions(-)
>>>> >  rename platform/linux-dpdk/{odp_init.c => odp_platform.c} (50%)
>>>> >  create mode 100644 platform/linux-generic/odp_platform.c
>>>> >  rename platform/linux-keystone2/{odp_init.c => odp_platform.c} (72%)
>>>> >
>>>> > diff --git a/example/generator/odp_generator.c
>>>> > b/example/generator/odp_generator.c
>>>> > index 9fa9b37..ba7aa68 100644
>>>> > --- a/example/generator/odp_generator.c
>>>> > +++ b/example/generator/odp_generator.c
>>>> > @@ -525,7 +525,7 @@ int main(int argc, char *argv[])
>>>> >       int core_count;
>>>> >
>>>> >       /* Init ODP before calling anything else */
>>>> > -     if (odp_init_global()) {
>>>> > +     if (odp_init_global(NULL, NULL)) {
>>>> >               ODP_ERR("Error: ODP global init failed.\n");
>>>> >               exit(EXIT_FAILURE);
>>>> >       }
>>>> > diff --git a/example/l2fwd/odp_l2fwd.c b/example/l2fwd/odp_l2fwd.c
>>>> > index d74449a..f8d6729 100644
>>>> > --- a/example/l2fwd/odp_l2fwd.c
>>>> > +++ b/example/l2fwd/odp_l2fwd.c
>>>> > @@ -323,7 +323,7 @@ int main(int argc, char *argv[])
>>>> >       odp_pktio_t pktio;
>>>> >
>>>> >       /* Init ODP before calling anything else */
>>>> > -     if (odp_init_global()) {
>>>> > +     if (odp_init_global(NULL, NULL)) {
>>>> >               ODP_ERR("Error: ODP global init failed.\n");
>>>> >               exit(EXIT_FAILURE);
>>>> >       }
>>>> > diff --git a/example/odp_example/odp_example.c
>>>> > b/example/odp_example/odp_example.c
>>>> > index f0bdf29..3d38ac7 100644
>>>> > --- a/example/odp_example/odp_example.c
>>>> > +++ b/example/odp_example/odp_example.c
>>>> > @@ -951,7 +951,7 @@ int main(int argc, char *argv[])
>>>> >
>>>> >       memset(thread_tbl, 0, sizeof(thread_tbl));
>>>> >
>>>> > -     if (odp_init_global()) {
>>>> > +     if (odp_init_global(NULL, NULL)) {
>>>> >               printf("ODP global init failed.\n");
>>>> >               return -1;
>>>> >       }
>>>> > diff --git a/example/packet/odp_pktio.c b/example/packet/odp_pktio.c
>>>> > index f247bd0..f3543a0 100644
>>>> > --- a/example/packet/odp_pktio.c
>>>> > +++ b/example/packet/odp_pktio.c
>>>> > @@ -311,7 +311,7 @@ int main(int argc, char *argv[])
>>>> >       int core_count;
>>>> >
>>>> >       /* Init ODP before calling anything else */
>>>> > -     if (odp_init_global()) {
>>>> > +     if (odp_init_global(NULL, NULL)) {
>>>> >               ODP_ERR("Error: ODP global init failed.\n");
>>>> >               exit(EXIT_FAILURE);
>>>> >       }
>>>> > diff --git a/example/timer/odp_timer_test.c
>>>> > b/example/timer/odp_timer_test.c
>>>> > index bf1d7df..abf90aa 100644
>>>> > --- a/example/timer/odp_timer_test.c
>>>> > +++ b/example/timer/odp_timer_test.c
>>>> > @@ -260,7 +260,7 @@ int main(int argc, char *argv[])
>>>> >
>>>> >       memset(thread_tbl, 0, sizeof(thread_tbl));
>>>> >
>>>> > -     if (odp_init_global()) {
>>>> > +     if (odp_init_global(NULL, NULL)) {
>>>> >               printf("ODP global init failed.\n");
>>>> >               return -1;
>>>> >       }
>>>> > diff --git a/platform/linux-dpdk/Makefile.am b/platform/linux-
>>>> > dpdk/Makefile.am
>>>> > index ff49b7d..70fa549 100644
>>>> > --- a/platform/linux-dpdk/Makefile.am
>>>> > +++ b/platform/linux-dpdk/Makefile.am
>>>> > @@ -67,13 +67,14 @@ __LIB__libodp_la_SOURCES = \
>>>> >                          odp_buffer.c \
>>>> >                          odp_buffer_pool.c \
>>>> >                          ../linux-generic/odp_coremask.c \
>>>> > -                        odp_init.c \
>>>> > +                        ../linux-generic/odp_init.c \
>>>> >                          odp_linux.c \
>>>> >                          odp_packet.c \
>>>> >                          odp_packet_dpdk.c \
>>>> >                          ../linux-generic/odp_packet_flags.c \
>>>> >                          odp_packet_io.c \
>>>> >                          ../linux-generic/odp_packet_socket.c \
>>>> > +                        odp_platform.c \
>>>> >                          odp_queue.c \
>>>> >                          ../linux-generic/odp_ring.c \
>>>> >                          ../linux-generic/odp_rwlock.c \
>>>> > diff --git a/platform/linux-dpdk/odp_init.c b/platform/linux-
>>>> > dpdk/odp_platform.c
>>>> > similarity index 50%
>>>> > rename from platform/linux-dpdk/odp_init.c
>>>> > rename to platform/linux-dpdk/odp_platform.c
>>>> > index ecc2066..5a054fd 100644
>>>> > --- a/platform/linux-dpdk/odp_init.c
>>>> > +++ b/platform/linux-dpdk/odp_platform.c
>>>> > @@ -1,4 +1,4 @@
>>>> > -/* Copyright (c) 2013, Linaro Limited
>>>> > +/* Copyright (c) 2014, Linaro Limited
>>>> >   * All rights reserved.
>>>> >   *
>>>> >   * SPDX-License-Identifier:     BSD-3-Clause
>>>> > @@ -9,7 +9,7 @@
>>>> >  #include <odp_debug.h>
>>>> >  #include <odp_packet_dpdk.h>
>>>> >
>>>> > -int odp_init_dpdk(void)
>>>> > +int odp_init_platform(odp_global_platform_init_t *platform_params
>>>> > ODP_UNUSED)
>>>> >  {
>>>> >       int test_argc = 5;
>>>> >       char *test_argv[6];
>>>> > @@ -50,64 +50,7 @@ int odp_init_dpdk(void)
>>>> >       return 0;
>>>> >  }
>>>> >
>>>> > -int odp_init_global(void)
>>>> > +int odp_crypto_init_global(void)
>>>>
>>>> Is crypto init now in odp_platform.c? It should be in odp_crypto.c or
>>>> similar...
>>>>
>>> no problem
>>> I can make a dummy   odp_crypto.c for ks2 and point DPDK at the linux
>>> generic. I was trying to avoid
>>> connecting internals.
>>>
>>>>
>>>> >  {
>>>> > -     odp_thread_init_global();
>>>> > -
>>>> > -     odp_system_info_init();
>>>> > -
>>>> > -     if (odp_init_dpdk()) {
>>>> > -             ODP_ERR("ODP dpdk init failed.\n");
>>>> > -             return -1;
>>>> > -     }
>>>> > -
>>>> > -     if (odp_shm_init_global()) {
>>>> > -             ODP_ERR("ODP shm init failed.\n");
>>>> > -             return -1;
>>>> > -     }
>>>> > -
>>>> > -     if (odp_buffer_pool_init_global()) {
>>>> > -             ODP_ERR("ODP buffer pool init failed.\n");
>>>> > -             return -1;
>>>> > -     }
>>>> > -
>>>> > -     if (odp_queue_init_global()) {
>>>> > -             ODP_ERR("ODP queue init failed.\n");
>>>> > -             return -1;
>>>> > -     }
>>>> > -
>>>> > -     if (odp_schedule_init_global()) {
>>>> > -             ODP_ERR("ODP schedule init failed.\n");
>>>> > -             return -1;
>>>> > -     }
>>>> > -
>>>> > -     if (odp_pktio_init_global()) {
>>>> > -             ODP_ERR("ODP packet io init failed.\n");
>>>> > -             return -1;
>>>> > -     }
>>>> > -
>>>> > -     if (odp_timer_init_global()) {
>>>> > -             ODP_ERR("ODP timer init failed.\n");
>>>> > -             return -1;
>>>> > -     }
>>>> > -
>>>> > -     return 0;
>>>> > -}
>>>> > -
>>>> > -
>>>> > -int odp_init_local(int thr_id)
>>>> > -{
>>>> > -     odp_thread_init_local(thr_id);
>>>> > -
>>>> > -     if (odp_pktio_init_local()) {
>>>> > -             ODP_ERR("ODP packet io local init failed.\n");
>>>> > -             return -1;
>>>> > -     }
>>>> > -
>>>> > -     if (odp_schedule_init_local()) {
>>>> > -             ODP_ERR("ODP schedule local init failed.\n");
>>>> > -             return -1;
>>>> > -     }
>>>> > -
>>>> >       return 0;
>>>> >  }
>>>> > diff --git a/platform/linux-generic/Makefile.am b/platform/linux-
>>>> > generic/Makefile.am
>>>> > index f4dfdc1..c4aae9d 100644
>>>> > --- a/platform/linux-generic/Makefile.am
>>>> > +++ b/platform/linux-generic/Makefile.am
>>>> > @@ -62,6 +62,7 @@ __LIB__libodp_la_SOURCES = \
>>>> >                          odp_packet_flags.c \
>>>> >                          odp_packet_io.c \
>>>> >                          odp_packet_socket.c \
>>>> > +                        odp_platform.c \
>>>> >                          odp_queue.c \
>>>> >                          odp_ring.c \
>>>> >                          odp_rwlock.c \
>>>> > diff --git a/platform/linux-generic/include/api/odp_init.h
>>>> > b/platform/linux-generic/include/api/odp_init.h
>>>> > index 490324a..ece4547 100644
>>>> > --- a/platform/linux-generic/include/api/odp_init.h
>>>> > +++ b/platform/linux-generic/include/api/odp_init.h
>>>> > @@ -22,24 +22,42 @@ extern "C" {
>>>> >
>>>> >  #include <odp_std_types.h>
>>>> >
>>>> > -
>>>> > -
>>>> > +/** ODP  initialisation data.
>>>> > + * Data  that is required to initialize the ODP API with the
>>>> > + * application specific data such as specifying a logging callback,
>>>> the
>>>> > log
>>>> > + * level etc.
>>>> > + */
>>>> > +typedef struct odp_global_init {
>>>>
>>>> The struct name should be "struct odp_global_init_t", "struct
>>>> odp_global_init_s" or "struct odp_global_init_".
>>>>
>>>> The name without postfix (odp_global_init) should be reserved for
>>>> variable names.
>>>> no probelm
>>>>
>>> I will go with odp_init_s
>>>
>>>
>>>
>>>> > +} odp_global_init_t;
>>>>
>>>> In short, this could be named odp_init_t. It's the main init struct
>>>> there will be.
>>>> If there will be a local init struct it could be then
>>>> odp_init_local_t...
>>>>
>>>> > +
>>>> > +/** ODP platform initialization data.
>>>> > + * @note ODP API does nothing with this data. It is the underlying
>>>> > + * implementation requires it and any data passed here is not
>>>> portable.
>>>> > + * It is required that the application takes care of identifying and
>>>> > + * passing any required platform specific data.
>>>> > + */
>>>> > +typedef struct odp_global_platform_init {
>>>>
>>>> Same thing, rename struct.
>>>>
>>> no problem
>>> odp_init_platform_s
>>>
>>>>
>>>> > +} odp_platform_init_t;
>>>>
>>>>
>>>> Same thing, odp_platform_init_t ??
>>>>
>>>>
>>>> >
>>>> >  /**
>>>> > - * Perform global ODP initalisation.
>>>> > - *
>>>> > - * This function must be called once before calling
>>>> > - * any other ODP API functions.
>>>> > + * Perform global ODP initialisation.
>>>> >   *
>>>> > + * This function must be called once before calling any other ODP API
>>>> > + * functions.
>>>> > + * @param[in] params Those parameters that are interpreted by the
>>>> ODP API
>>>>
>>>> "Those parameters that are common to all ODP implementations. Specified
>>>> in this API"
>>>>
>>>> API does not interpret, implementation does...
>>>>
>>>
>>> Ok, I am struggling with names
>>>
>>> how about "platform SDK" for the underlying code such as DPDK, so that
>>> the platform has an API
>>> implementation -  which is actually implemented in terms of a platform
>>> SDK
>>>
>>> We need to distinguish that implementations should not use the platform
>>> parameters unless the underlying
>>> framework requires it, not that any implementation is free to just add
>>> local stuff - or thats how it feels to me at least.
>>>
>>>>
>>>> > + * @param[in] platform_params Those parameters that are passed
>>>> without
>>>> > + * interpretation by the ODP API to the implementation.
>>>>
>>>> " ... implementation specific parameter. Specified outside of ODP API"
>>>>
>>>> >   * @return 0 if successful
>>>> >   */
>>>> > -int odp_init_global(void);
>>>> > +int odp_init_global(odp_global_init_t *params,
>>>> > +                 odp_global_platform_init_t *platform_params);
>>>> >
>>>> >
>>>> >  /**
>>>> > - * Perform thread local ODP initalisation.
>>>> > + * Perform thread local ODP initialisation.
>>>> >   *
>>>> >   * All threads must call this function before calling
>>>> > + *
>>>>
>>>> Extra line feed?
>>>>
>>> Will fix
>>>
>>>>
>>>> >   * any other ODP API functions.
>>>> >   * @param thr_id Thread id
>>>> >   * @return 0 if successful
>>>> > diff --git a/platform/linux-generic/include/odp_internal.h
>>>> > b/platform/linux-generic/include/odp_internal.h
>>>> > index aa79493..6c71fc9 100644
>>>> > --- a/platform/linux-generic/include/odp_internal.h
>>>> > +++ b/platform/linux-generic/include/odp_internal.h
>>>> > @@ -4,6 +4,7 @@
>>>> >   * SPDX-License-Identifier:     BSD-3-Clause
>>>> >   */
>>>> >
>>>> > +#include <odp_init.h>
>>>> >
>>>> >  /**
>>>> >   * @file
>>>> > @@ -42,6 +43,8 @@ int odp_schedule_init_local(void);
>>>> >  int odp_timer_init_global(void);
>>>> >  int odp_timer_disarm_all(void);
>>>> >
>>>> > +int odp_init_platform(odp_global_platform_init_t *platform_params);
>>>> > +
>>>> >  #ifdef __cplusplus
>>>> >  }
>>>> >  #endif
>>>> > diff --git a/platform/linux-generic/odp_init.c b/platform/linux-
>>>> > generic/odp_init.c
>>>> > index 5b7e192..f595def 100644
>>>> > --- a/platform/linux-generic/odp_init.c
>>>> > +++ b/platform/linux-generic/odp_init.c
>>>> > @@ -8,13 +8,18 @@
>>>> >  #include <odp_internal.h>
>>>> >  #include <odp_debug.h>
>>>> >
>>>> > -
>>>> > -int odp_init_global(void)
>>>> > +int odp_init_global(odp_global_init_t *params  ODP_UNUSED,
>>>> > +                 odp_global_platform_init_t *platform_params
>>>> ODP_UNUSED)
>>>> >  {
>>>> >       odp_thread_init_global();
>>>> >
>>>> >       odp_system_info_init();
>>>> >
>>>> > +     if (odp_init_platform(platform_params)) {
>>>>
>>>> platform_params is used here, so remove ODP_UNUSED from argument list.
>>>>
>>>> On the other hand, it would be better to remove odp_init_platform()
>>>> altogether since does not do anything. It's better to add it when
>>>> actually used.
>>>> The order of init calls is very important, e.g. now you could not
>>>> allocate shared
>>>> memory in odp_init_platform() because it's executed before shm init...
>>>>
>>>  odp_init_platform() does do something for DPDK / KS2 and we need a
>>> place holder because
>>> we want this to be shared between implementations don't we ?
>>> This is the stuff that must happen before any ODP init starts to satisfy
>>> the platform SDK
>>>
>>>>
>>>> - Petri
>>>>
>>>> > +             ODP_ERR("ODP platform init failed.\n");
>>>> > +             return -1;
>>>> > +     }
>>>> > +
>>>> >       if (odp_shm_init_global()) {
>>>> >               ODP_ERR("ODP shm init failed.\n");
>>>> >               return -1;
>>>> > diff --git a/platform/linux-generic/odp_platform.c b/platform/linux-
>>>> > generic/odp_platform.c
>>>> > new file mode 100644
>>>> > index 0000000..a9efa7f
>>>> > --- /dev/null
>>>> > +++ b/platform/linux-generic/odp_platform.c
>>>> > @@ -0,0 +1,14 @@
>>>> > +/* Copyright (c) 2014, Linaro Limited
>>>> > + * All rights reserved.
>>>> > + *
>>>> > + * SPDX-License-Identifier:     BSD-3-Clause
>>>> > + */
>>>> > +
>>>> > +#include <odp_init.h>
>>>> > +#include <odp_internal.h>
>>>> > +#include <odp_debug.h>
>>>> > +
>>>> > +int odp_init_platform(odp_global_platform_init_t *platform_params
>>>> > ODP_UNUSED)
>>>> > +{
>>>> > +     return 0;
>>>> > +}
>>>>
>>>>
>>>>
>>>>
>>>>
>>>
>>>
>>> --
>>> *Mike Holmes*
>>> Linaro Technical Manager / Lead
>>> LNG - ODP
>>>
>>> _______________________________________________
>>> lng-odp mailing list
>>> lng-odp@lists.linaro.org
>>> http://lists.linaro.org/mailman/listinfo/lng-odp
>>>
>>>
>>
>
>
> --
> *Mike Holmes*
> Linaro Technical Manager / Lead
> LNG - ODP
>

Bill Mills Aug. 29, 2014, 5:28 p.m. UTC | #7

OK, Yes I never say anything on this list but ...

On 08/29/2014 01:19 PM, Mike Holmes wrote:
> Ok, I need a better feel for consensus here or it will flip flop :), as
> this impacts all structs we really need a final answer
>
> typedef struct odp_<name>_t {...} odp_<name>_t;
> OR
> typedef struct odp_<name>_s {...} odp_<name>_t;
> OR
> typedef struct {...} odp_<name>_t;
>
> I prefer the latter, why even name it unless you need to reference the
> name inside the struct being typedefed ?
>

I wrote/collected our internal coding conventions and we insisted on a
name.  The reason (at least at that time) was some debuggers showed the 
structure name but not the typedef name.

We used struct odp_<name>_tag {...} to emphasize that it should not be
used in the code.  In retrospect I don't like that but either the _s or
_t above are OK.

>
>
>
> On 29 August 2014 13:10, Bill Fischofer <bill.fischofer@linaro.org
> <mailto:bill.fischofer@linaro.org>> wrote:
>
>     We've been using odp_<name>_t for typedef stuff, taking advantage
>     that of the fact that struct tags occupy a different namespace.  So
>
>     typedef struct odp_<name>_t {...} odp_<name>_t;
>
>     is perfectly valid and unambiguous C and avoids additional namespace
>     clutter.
>
>     odp_<name>_e has been suggested for ODP enums.
>
>     Bill
>
>
>     On Fri, Aug 29, 2014 at 12:05 PM, Mike Holmes
>     <mike.holmes@linaro.org <mailto:mike.holmes@linaro.org>> wrote:
>
>
>
>
>         On 29 August 2014 03:59, Savolainen, Petri (NSN - FI/Espoo)
>         <petri.savolainen@nsn.com <mailto:petri.savolainen@nsn.com>> wrote:
>
>
>
>              > -----Original Message-----
>              > From: lng-odp-bounces@lists.linaro.org
>             <mailto:lng-odp-bounces@lists.linaro.org> [mailto:lng-odp-
>             <mailto:lng-odp->
>              > bounces@lists.linaro.org
>             <mailto:bounces@lists.linaro.org>] On Behalf Of ext Mike Holmes
>              > Sent: Thursday, August 28, 2014 9:26 PM
>              > To: lng-odp@lists.linaro.org
>             <mailto:lng-odp@lists.linaro.org>
>              > Subject: [lng-odp] [PATCH v4] Add-global_init-paramiters
>              >
>              > Signed-off-by: Mike Holmes <mike.holmes@linaro.org
>             <mailto:mike.holmes@linaro.org>>
>              > ---
>              >
>              > v2:
>              > Factor out odp_platform_init so that odp_global_init is
>             only defined
>              > for linux_generic and resued.
>              >
>              > v3:
>              > Use -M to simplify the diff
>              > Fix missing doxygen comments
>              >
>              > v4:
>              > Improve the documentation.
>              >
>              >  example/generator/odp_generator.c                  |  2 +-
>              >  example/l2fwd/odp_l2fwd.c                          |  2 +-
>              >  example/odp_example/odp_example.c                  |  2 +-
>              >  example/packet/odp_pktio.c                         |  2 +-
>              >  example/timer/odp_timer_test.c                     |  2 +-
>              >  platform/linux-dpdk/Makefile.am                    |  3 +-
>              >  platform/linux-dpdk/{odp_init.c => odp_platform.c} | 63
>             +----------------
>              > --
>              >  platform/linux-generic/Makefile.am                 |  1 +
>              >  platform/linux-generic/include/api/odp_init.h      | 34
>             +++++++---
>              >  platform/linux-generic/include/odp_internal.h      |  3 +
>              >  platform/linux-generic/odp_init.c                  |  9 ++-
>              >  platform/linux-generic/odp_platform.c              | 14
>             +++++
>              >  platform/linux-keystone2/Makefile.am               |  3 +-
>              >  .../linux-keystone2/{odp_init.c => odp_platform.c} | 72
>             +++--------------
>              > -----
>              >  test/api_test/odp_common.c                         |  2 +-
>              >  15 files changed, 72 insertions(+), 142 deletions(-)
>              >  rename platform/linux-dpdk/{odp_init.c =>
>             odp_platform.c} (50%)
>              >  create mode 100644 platform/linux-generic/odp_platform.c
>              >  rename platform/linux-keystone2/{odp_init.c =>
>             odp_platform.c} (72%)
>              >
>              > diff --git a/example/generator/odp_generator.c
>              > b/example/generator/odp_generator.c
>              > index 9fa9b37..ba7aa68 100644
>              > --- a/example/generator/odp_generator.c
>              > +++ b/example/generator/odp_generator.c
>              > @@ -525,7 +525,7 @@ int main(int argc, char *argv[])
>              >       int core_count;
>              >
>              >       /* Init ODP before calling anything else */
>              > -     if (odp_init_global()) {
>              > +     if (odp_init_global(NULL, NULL)) {
>              >               ODP_ERR("Error: ODP global init failed.\n");
>              >               exit(EXIT_FAILURE);
>              >       }
>              > diff --git a/example/l2fwd/odp_l2fwd.c
>             b/example/l2fwd/odp_l2fwd.c
>              > index d74449a..f8d6729 100644
>              > --- a/example/l2fwd/odp_l2fwd.c
>              > +++ b/example/l2fwd/odp_l2fwd.c
>              > @@ -323,7 +323,7 @@ int main(int argc, char *argv[])
>              >       odp_pktio_t pktio;
>              >
>              >       /* Init ODP before calling anything else */
>              > -     if (odp_init_global()) {
>              > +     if (odp_init_global(NULL, NULL)) {
>              >               ODP_ERR("Error: ODP global init failed.\n");
>              >               exit(EXIT_FAILURE);
>              >       }
>              > diff --git a/example/odp_example/odp_example.c
>              > b/example/odp_example/odp_example.c
>              > index f0bdf29..3d38ac7 100644
>              > --- a/example/odp_example/odp_example.c
>              > +++ b/example/odp_example/odp_example.c
>              > @@ -951,7 +951,7 @@ int main(int argc, char *argv[])
>              >
>              >       memset(thread_tbl, 0, sizeof(thread_tbl));
>              >
>              > -     if (odp_init_global()) {
>              > +     if (odp_init_global(NULL, NULL)) {
>              >               printf("ODP global init failed.\n");
>              >               return -1;
>              >       }
>              > diff --git a/example/packet/odp_pktio.c
>             b/example/packet/odp_pktio.c
>              > index f247bd0..f3543a0 100644
>              > --- a/example/packet/odp_pktio.c
>              > +++ b/example/packet/odp_pktio.c
>              > @@ -311,7 +311,7 @@ int main(int argc, char *argv[])
>              >       int core_count;
>              >
>              >       /* Init ODP before calling anything else */
>              > -     if (odp_init_global()) {
>              > +     if (odp_init_global(NULL, NULL)) {
>              >               ODP_ERR("Error: ODP global init failed.\n");
>              >               exit(EXIT_FAILURE);
>              >       }
>              > diff --git a/example/timer/odp_timer_test.c
>              > b/example/timer/odp_timer_test.c
>              > index bf1d7df..abf90aa 100644
>              > --- a/example/timer/odp_timer_test.c
>              > +++ b/example/timer/odp_timer_test.c
>              > @@ -260,7 +260,7 @@ int main(int argc, char *argv[])
>              >
>              >       memset(thread_tbl, 0, sizeof(thread_tbl));
>              >
>              > -     if (odp_init_global()) {
>              > +     if (odp_init_global(NULL, NULL)) {
>              >               printf("ODP global init failed.\n");
>              >               return -1;
>              >       }
>              > diff --git a/platform/linux-dpdk/Makefile.am
>             b/platform/linux-
>              > dpdk/Makefile.am
>              > index ff49b7d..70fa549 100644
>              > --- a/platform/linux-dpdk/Makefile.am
>              > +++ b/platform/linux-dpdk/Makefile.am
>              > @@ -67,13 +67,14 @@ __LIB__libodp_la_SOURCES = \
>              >                          odp_buffer.c \
>              >                          odp_buffer_pool.c \
>              >                          ../linux-generic/odp_coremask.c \
>              > -                        odp_init.c \
>              > +                        ../linux-generic/odp_init.c \
>              >                          odp_linux.c \
>              >                          odp_packet.c \
>              >                          odp_packet_dpdk.c \
>              >
>             ../linux-generic/odp_packet_flags.c \
>              >                          odp_packet_io.c \
>              >
>             ../linux-generic/odp_packet_socket.c \
>              > +                        odp_platform.c \
>              >                          odp_queue.c \
>              >                          ../linux-generic/odp_ring.c \
>              >                          ../linux-generic/odp_rwlock.c \
>              > diff --git a/platform/linux-dpdk/odp_init.c b/platform/linux-
>              > dpdk/odp_platform.c
>              > similarity index 50%
>              > rename from platform/linux-dpdk/odp_init.c
>              > rename to platform/linux-dpdk/odp_platform.c
>              > index ecc2066..5a054fd 100644
>              > --- a/platform/linux-dpdk/odp_init.c
>              > +++ b/platform/linux-dpdk/odp_platform.c
>              > @@ -1,4 +1,4 @@
>              > -/* Copyright (c) 2013, Linaro Limited
>              > +/* Copyright (c) 2014, Linaro Limited
>              >   * All rights reserved.
>              >   *
>              >   * SPDX-License-Identifier:     BSD-3-Clause
>              > @@ -9,7 +9,7 @@
>              >  #include <odp_debug.h>
>              >  #include <odp_packet_dpdk.h>
>              >
>              > -int odp_init_dpdk(void)
>              > +int odp_init_platform(odp_global_platform_init_t
>             *platform_params
>              > ODP_UNUSED)
>              >  {
>              >       int test_argc = 5;
>              >       char *test_argv[6];
>              > @@ -50,64 +50,7 @@ int odp_init_dpdk(void)
>              >       return 0;
>              >  }
>              >
>              > -int odp_init_global(void)
>              > +int odp_crypto_init_global(void)
>
>             Is crypto init now in odp_platform.c? It should be in
>             odp_crypto.c or similar...
>
>         no problem
>         I can make a dummy   odp_crypto.c for ks2 and point DPDK at the
>         linux generic. I was trying to avoid
>         connecting internals.
>
>
>              >  {
>              > -     odp_thread_init_global();
>              > -
>              > -     odp_system_info_init();
>              > -
>              > -     if (odp_init_dpdk()) {
>              > -             ODP_ERR("ODP dpdk init failed.\n");
>              > -             return -1;
>              > -     }
>              > -
>              > -     if (odp_shm_init_global()) {
>              > -             ODP_ERR("ODP shm init failed.\n");
>              > -             return -1;
>              > -     }
>              > -
>              > -     if (odp_buffer_pool_init_global()) {
>              > -             ODP_ERR("ODP buffer pool init failed.\n");
>              > -             return -1;
>              > -     }
>              > -
>              > -     if (odp_queue_init_global()) {
>              > -             ODP_ERR("ODP queue init failed.\n");
>              > -             return -1;
>              > -     }
>              > -
>              > -     if (odp_schedule_init_global()) {
>              > -             ODP_ERR("ODP schedule init failed.\n");
>              > -             return -1;
>              > -     }
>              > -
>              > -     if (odp_pktio_init_global()) {
>              > -             ODP_ERR("ODP packet io init failed.\n");
>              > -             return -1;
>              > -     }
>              > -
>              > -     if (odp_timer_init_global()) {
>              > -             ODP_ERR("ODP timer init failed.\n");
>              > -             return -1;
>              > -     }
>              > -
>              > -     return 0;
>              > -}
>              > -
>              > -
>              > -int odp_init_local(int thr_id)
>              > -{
>              > -     odp_thread_init_local(thr_id);
>              > -
>              > -     if (odp_pktio_init_local()) {
>              > -             ODP_ERR("ODP packet io local init failed.\n");
>              > -             return -1;
>              > -     }
>              > -
>              > -     if (odp_schedule_init_local()) {
>              > -             ODP_ERR("ODP schedule local init failed.\n");
>              > -             return -1;
>              > -     }
>              > -
>              >       return 0;
>              >  }
>              > diff --git a/platform/linux-generic/Makefile.am
>             b/platform/linux-
>              > generic/Makefile.am
>              > index f4dfdc1..c4aae9d 100644
>              > --- a/platform/linux-generic/Makefile.am
>              > +++ b/platform/linux-generic/Makefile.am
>              > @@ -62,6 +62,7 @@ __LIB__libodp_la_SOURCES = \
>              >                          odp_packet_flags.c \
>              >                          odp_packet_io.c \
>              >                          odp_packet_socket.c \
>              > +                        odp_platform.c \
>              >                          odp_queue.c \
>              >                          odp_ring.c \
>              >                          odp_rwlock.c \
>              > diff --git a/platform/linux-generic/include/api/odp_init.h
>              > b/platform/linux-generic/include/api/odp_init.h
>              > index 490324a..ece4547 100644
>              > --- a/platform/linux-generic/include/api/odp_init.h
>              > +++ b/platform/linux-generic/include/api/odp_init.h
>              > @@ -22,24 +22,42 @@ extern "C" {
>              >
>              >  #include <odp_std_types.h>
>              >
>              > -
>              > -
>              > +/** ODP  initialisation data.
>              > + * Data  that is required to initialize the ODP API with the
>              > + * application specific data such as specifying a
>             logging callback, the
>              > log
>              > + * level etc.
>              > + */
>              > +typedef struct odp_global_init {
>
>             The struct name should be "struct odp_global_init_t",
>             "struct odp_global_init_s" or "struct odp_global_init_".
>
>             The name without postfix (odp_global_init) should be
>             reserved for variable names.
>             no probelm
>
>         I will go with odp_init_s
>
>              > +} odp_global_init_t;
>
>             In short, this could be named odp_init_t. It's the main init
>             struct there will be.
>             If there will be a local init struct it could be then
>             odp_init_local_t...
>
>              > +
>              > +/** ODP platform initialization data.
>              > + * @note ODP API does nothing with this data. It is the
>             underlying
>              > + * implementation requires it and any data passed here
>             is not portable.
>              > + * It is required that the application takes care of
>             identifying and
>              > + * passing any required platform specific data.
>              > + */
>              > +typedef struct odp_global_platform_init {
>
>             Same thing, rename struct.
>
>         no problem
>         odp_init_platform_s
>
>
>              > +} odp_platform_init_t;
>
>
>             Same thing, odp_platform_init_t ??
>
>
>              >
>              >  /**
>              > - * Perform global ODP initalisation.
>              > - *
>              > - * This function must be called once before calling
>              > - * any other ODP API functions.
>              > + * Perform global ODP initialisation.
>              >   *
>              > + * This function must be called once before calling any
>             other ODP API
>              > + * functions.
>              > + * @param[in] params Those parameters that are
>             interpreted by the ODP API
>
>             "Those parameters that are common to all ODP
>             implementations. Specified in this API"
>
>             API does not interpret, implementation does...
>
>
>         Ok, I am struggling with names
>
>         how about "platform SDK" for the underlying code such as DPDK,
>         so that the platform has an API
>         implementation -  which is actually implemented in terms of a
>         platform SDK
>
>         We need to distinguish that implementations should not use the
>         platform parameters unless the underlying
>         framework requires it, not that any implementation is free to
>         just add local stuff - or thats how it feels to me at least.
>
>
>              > + * @param[in] platform_params Those parameters that are
>             passed without
>              > + * interpretation by the ODP API to the implementation.
>
>             " ... implementation specific parameter. Specified outside
>             of ODP API"
>
>              >   * @return 0 if successful
>              >   */
>              > -int odp_init_global(void);
>              > +int odp_init_global(odp_global_init_t *params,
>              > +                 odp_global_platform_init_t
>             *platform_params);
>              >
>              >
>              >  /**
>              > - * Perform thread local ODP initalisation.
>              > + * Perform thread local ODP initialisation.
>              >   *
>              >   * All threads must call this function before calling
>              > + *
>
>             Extra line feed?
>
>         Will fix
>
>
>              >   * any other ODP API functions.
>              >   * @param thr_id Thread id
>              >   * @return 0 if successful
>              > diff --git a/platform/linux-generic/include/odp_internal.h
>              > b/platform/linux-generic/include/odp_internal.h
>              > index aa79493..6c71fc9 100644
>              > --- a/platform/linux-generic/include/odp_internal.h
>              > +++ b/platform/linux-generic/include/odp_internal.h
>              > @@ -4,6 +4,7 @@
>              >   * SPDX-License-Identifier:     BSD-3-Clause
>              >   */
>              >
>              > +#include <odp_init.h>
>              >
>              >  /**
>              >   * @file
>              > @@ -42,6 +43,8 @@ int odp_schedule_init_local(void);
>              >  int odp_timer_init_global(void);
>              >  int odp_timer_disarm_all(void);
>              >
>              > +int odp_init_platform(odp_global_platform_init_t
>             *platform_params);
>              > +
>              >  #ifdef __cplusplus
>              >  }
>              >  #endif
>              > diff --git a/platform/linux-generic/odp_init.c
>             b/platform/linux-
>              > generic/odp_init.c
>              > index 5b7e192..f595def 100644
>              > --- a/platform/linux-generic/odp_init.c
>              > +++ b/platform/linux-generic/odp_init.c
>              > @@ -8,13 +8,18 @@
>              >  #include <odp_internal.h>
>              >  #include <odp_debug.h>
>              >
>              > -
>              > -int odp_init_global(void)
>              > +int odp_init_global(odp_global_init_t *params  ODP_UNUSED,
>              > +                 odp_global_platform_init_t
>             *platform_params ODP_UNUSED)
>              >  {
>              >       odp_thread_init_global();
>              >
>              >       odp_system_info_init();
>              >
>              > +     if (odp_init_platform(platform_params)) {
>
>             platform_params is used here, so remove ODP_UNUSED from
>             argument list.
>
>             On the other hand, it would be better to remove
>             odp_init_platform()
>             altogether since does not do anything. It's better to add it
>             when actually used.
>             The order of init calls is very important, e.g. now you
>             could not allocate shared
>             memory in odp_init_platform() because it's executed before
>             shm init...
>
>           odp_init_platform() does do something for DPDK / KS2 and we
>         need a place holder because
>         we want this to be shared between implementations don't we ?
>         This is the stuff that must happen before any ODP init starts to
>         satisfy the platform SDK
>
>
>             - Petri
>
>              > +             ODP_ERR("ODP platform init failed.\n");
>              > +             return -1;
>              > +     }
>              > +
>              >       if (odp_shm_init_global()) {
>              >               ODP_ERR("ODP shm init failed.\n");
>              >               return -1;
>              > diff --git a/platform/linux-generic/odp_platform.c
>             b/platform/linux-
>              > generic/odp_platform.c
>              > new file mode 100644
>              > index 0000000..a9efa7f
>              > --- /dev/null
>              > +++ b/platform/linux-generic/odp_platform.c
>              > @@ -0,0 +1,14 @@
>              > +/* Copyright (c) 2014, Linaro Limited
>              > + * All rights reserved.
>              > + *
>              > + * SPDX-License-Identifier:     BSD-3-Clause
>              > + */
>              > +
>              > +#include <odp_init.h>
>              > +#include <odp_internal.h>
>              > +#include <odp_debug.h>
>              > +
>              > +int odp_init_platform(odp_global_platform_init_t
>             *platform_params
>              > ODP_UNUSED)
>              > +{
>              > +     return 0;
>              > +}
>
>
>
>
>
>
>
>         --
>         *Mike Holmes*
>         Linaro Technical Manager / Lead
>         LNG - ODP
>
>         _______________________________________________
>         lng-odp mailing list
>         lng-odp@lists.linaro.org <mailto:lng-odp@lists.linaro.org>
>         http://lists.linaro.org/mailman/listinfo/lng-odp
>
>
>
>
>
> --
> *Mike Holmes*
> Linaro Technical Manager / Lead
> LNG - ODP
>
>
> _______________________________________________
> lng-odp mailing list
> lng-odp@lists.linaro.org
> http://lists.linaro.org/mailman/listinfo/lng-odp
>
>
>
> To unsubscribe from this group and stop receiving emails from it, send an email to lng-sc+unsubscribe@linaro.org.
>

Mike Holmes Aug. 29, 2014, 5:34 p.m. UTC | #8

The common set is option 1, which is what we have in may places, I dont
like that we make it possible to reference the struct without the typedef,
but unless we have further dissent we have a winner.I think.

Mike




On 29 August 2014 13:28, William Mills <wmills@ti.com> wrote:

> OK, Yes I never say anything on this list but ...
>
>
> On 08/29/2014 01:19 PM, Mike Holmes wrote:
>
>> Ok, I need a better feel for consensus here or it will flip flop :), as
>> this impacts all structs we really need a final answer
>>
>> typedef struct odp_<name>_t {...} odp_<name>_t;
>> OR
>> typedef struct odp_<name>_s {...} odp_<name>_t;
>> OR
>> typedef struct {...} odp_<name>_t;
>>
>> I prefer the latter, why even name it unless you need to reference the
>> name inside the struct being typedefed ?
>>
>>
> I wrote/collected our internal coding conventions and we insisted on a
> name.  The reason (at least at that time) was some debuggers showed the
> structure name but not the typedef name.
>
> We used struct odp_<name>_tag {...} to emphasize that it should not be
> used in the code.  In retrospect I don't like that but either the _s or
> _t above are OK.
>
>
>>
>>
>> On 29 August 2014 13:10, Bill Fischofer <bill.fischofer@linaro.org
>> <mailto:bill.fischofer@linaro.org>> wrote:
>>
>>     We've been using odp_<name>_t for typedef stuff, taking advantage
>>     that of the fact that struct tags occupy a different namespace.  So
>>
>>     typedef struct odp_<name>_t {...} odp_<name>_t;
>>
>>     is perfectly valid and unambiguous C and avoids additional namespace
>>     clutter.
>>
>>     odp_<name>_e has been suggested for ODP enums.
>>
>>     Bill
>>
>>
>>     On Fri, Aug 29, 2014 at 12:05 PM, Mike Holmes
>>     <mike.holmes@linaro.org <mailto:mike.holmes@linaro.org>> wrote:
>>
>>
>>
>>
>>         On 29 August 2014 03:59, Savolainen, Petri (NSN - FI/Espoo)
>>         <petri.savolainen@nsn.com <mailto:petri.savolainen@nsn.com>>
>> wrote:
>>
>>
>>
>>              > -----Original Message-----
>>              > From: lng-odp-bounces@lists.linaro.org
>>             <mailto:lng-odp-bounces@lists.linaro.org> [mailto:lng-odp-
>>             <mailto:lng-odp->
>>              > bounces@lists.linaro.org
>>             <mailto:bounces@lists.linaro.org>] On Behalf Of ext Mike
>> Holmes
>>              > Sent: Thursday, August 28, 2014 9:26 PM
>>              > To: lng-odp@lists.linaro.org
>>             <mailto:lng-odp@lists.linaro.org>
>>              > Subject: [lng-odp] [PATCH v4] Add-global_init-paramiters
>>              >
>>              > Signed-off-by: Mike Holmes <mike.holmes@linaro.org
>>             <mailto:mike.holmes@linaro.org>>
>>
>>              > ---
>>              >
>>              > v2:
>>              > Factor out odp_platform_init so that odp_global_init is
>>             only defined
>>              > for linux_generic and resued.
>>              >
>>              > v3:
>>              > Use -M to simplify the diff
>>              > Fix missing doxygen comments
>>              >
>>              > v4:
>>              > Improve the documentation.
>>              >
>>              >  example/generator/odp_generator.c                  |  2
>> +-
>>              >  example/l2fwd/odp_l2fwd.c                          |  2 +-
>>              >  example/odp_example/odp_example.c                  |  2
>> +-
>>              >  example/packet/odp_pktio.c                         |  2 +-
>>              >  example/timer/odp_timer_test.c                     |  2 +-
>>              >  platform/linux-dpdk/Makefile.am                    |  3
>> +-
>>              >  platform/linux-dpdk/{odp_init.c => odp_platform.c} | 63
>>             +----------------
>>              > --
>>              >  platform/linux-generic/Makefile.am                 |  1 +
>>              >  platform/linux-generic/include/api/odp_init.h      | 34
>>             +++++++---
>>              >  platform/linux-generic/include/odp_internal.h      |  3 +
>>              >  platform/linux-generic/odp_init.c                  |  9
>> ++-
>>              >  platform/linux-generic/odp_platform.c              | 14
>>             +++++
>>              >  platform/linux-keystone2/Makefile.am               |  3
>> +-
>>              >  .../linux-keystone2/{odp_init.c => odp_platform.c} | 72
>>             +++--------------
>>              > -----
>>              >  test/api_test/odp_common.c                         |  2 +-
>>              >  15 files changed, 72 insertions(+), 142 deletions(-)
>>              >  rename platform/linux-dpdk/{odp_init.c =>
>>             odp_platform.c} (50%)
>>              >  create mode 100644 platform/linux-generic/odp_platform.c
>>              >  rename platform/linux-keystone2/{odp_init.c =>
>>             odp_platform.c} (72%)
>>              >
>>              > diff --git a/example/generator/odp_generator.c
>>              > b/example/generator/odp_generator.c
>>              > index 9fa9b37..ba7aa68 100644
>>              > --- a/example/generator/odp_generator.c
>>              > +++ b/example/generator/odp_generator.c
>>              > @@ -525,7 +525,7 @@ int main(int argc, char *argv[])
>>              >       int core_count;
>>              >
>>              >       /* Init ODP before calling anything else */
>>              > -     if (odp_init_global()) {
>>              > +     if (odp_init_global(NULL, NULL)) {
>>              >               ODP_ERR("Error: ODP global init failed.\n");
>>              >               exit(EXIT_FAILURE);
>>              >       }
>>              > diff --git a/example/l2fwd/odp_l2fwd.c
>>             b/example/l2fwd/odp_l2fwd.c
>>              > index d74449a..f8d6729 100644
>>              > --- a/example/l2fwd/odp_l2fwd.c
>>              > +++ b/example/l2fwd/odp_l2fwd.c
>>              > @@ -323,7 +323,7 @@ int main(int argc, char *argv[])
>>              >       odp_pktio_t pktio;
>>              >
>>              >       /* Init ODP before calling anything else */
>>              > -     if (odp_init_global()) {
>>              > +     if (odp_init_global(NULL, NULL)) {
>>              >               ODP_ERR("Error: ODP global init failed.\n");
>>              >               exit(EXIT_FAILURE);
>>              >       }
>>              > diff --git a/example/odp_example/odp_example.c
>>              > b/example/odp_example/odp_example.c
>>              > index f0bdf29..3d38ac7 100644
>>              > --- a/example/odp_example/odp_example.c
>>              > +++ b/example/odp_example/odp_example.c
>>              > @@ -951,7 +951,7 @@ int main(int argc, char *argv[])
>>              >
>>              >       memset(thread_tbl, 0, sizeof(thread_tbl));
>>              >
>>              > -     if (odp_init_global()) {
>>              > +     if (odp_init_global(NULL, NULL)) {
>>              >               printf("ODP global init failed.\n");
>>              >               return -1;
>>              >       }
>>              > diff --git a/example/packet/odp_pktio.c
>>             b/example/packet/odp_pktio.c
>>              > index f247bd0..f3543a0 100644
>>              > --- a/example/packet/odp_pktio.c
>>              > +++ b/example/packet/odp_pktio.c
>>              > @@ -311,7 +311,7 @@ int main(int argc, char *argv[])
>>              >       int core_count;
>>              >
>>              >       /* Init ODP before calling anything else */
>>              > -     if (odp_init_global()) {
>>              > +     if (odp_init_global(NULL, NULL)) {
>>              >               ODP_ERR("Error: ODP global init failed.\n");
>>              >               exit(EXIT_FAILURE);
>>              >       }
>>              > diff --git a/example/timer/odp_timer_test.c
>>              > b/example/timer/odp_timer_test.c
>>              > index bf1d7df..abf90aa 100644
>>              > --- a/example/timer/odp_timer_test.c
>>              > +++ b/example/timer/odp_timer_test.c
>>              > @@ -260,7 +260,7 @@ int main(int argc, char *argv[])
>>              >
>>              >       memset(thread_tbl, 0, sizeof(thread_tbl));
>>              >
>>              > -     if (odp_init_global()) {
>>              > +     if (odp_init_global(NULL, NULL)) {
>>              >               printf("ODP global init failed.\n");
>>              >               return -1;
>>              >       }
>>              > diff --git a/platform/linux-dpdk/Makefile.am
>>             b/platform/linux-
>>              > dpdk/Makefile.am
>>              > index ff49b7d..70fa549 100644
>>              > --- a/platform/linux-dpdk/Makefile.am
>>              > +++ b/platform/linux-dpdk/Makefile.am
>>              > @@ -67,13 +67,14 @@ __LIB__libodp_la_SOURCES = \
>>              >                          odp_buffer.c \
>>              >                          odp_buffer_pool.c \
>>              >                          ../linux-generic/odp_coremask.c \
>>              > -                        odp_init.c \
>>              > +                        ../linux-generic/odp_init.c \
>>              >                          odp_linux.c \
>>              >                          odp_packet.c \
>>              >                          odp_packet_dpdk.c \
>>              >
>>             ../linux-generic/odp_packet_flags.c \
>>              >                          odp_packet_io.c \
>>              >
>>             ../linux-generic/odp_packet_socket.c \
>>              > +                        odp_platform.c \
>>              >                          odp_queue.c \
>>              >                          ../linux-generic/odp_ring.c \
>>              >                          ../linux-generic/odp_rwlock.c \
>>              > diff --git a/platform/linux-dpdk/odp_init.c
>> b/platform/linux-
>>              > dpdk/odp_platform.c
>>              > similarity index 50%
>>              > rename from platform/linux-dpdk/odp_init.c
>>              > rename to platform/linux-dpdk/odp_platform.c
>>              > index ecc2066..5a054fd 100644
>>              > --- a/platform/linux-dpdk/odp_init.c
>>              > +++ b/platform/linux-dpdk/odp_platform.c
>>              > @@ -1,4 +1,4 @@
>>              > -/* Copyright (c) 2013, Linaro Limited
>>              > +/* Copyright (c) 2014, Linaro Limited
>>              >   * All rights reserved.
>>              >   *
>>              >   * SPDX-License-Identifier:     BSD-3-Clause
>>              > @@ -9,7 +9,7 @@
>>              >  #include <odp_debug.h>
>>              >  #include <odp_packet_dpdk.h>
>>              >
>>              > -int odp_init_dpdk(void)
>>              > +int odp_init_platform(odp_global_platform_init_t
>>             *platform_params
>>              > ODP_UNUSED)
>>              >  {
>>              >       int test_argc = 5;
>>              >       char *test_argv[6];
>>              > @@ -50,64 +50,7 @@ int odp_init_dpdk(void)
>>              >       return 0;
>>              >  }
>>              >
>>              > -int odp_init_global(void)
>>              > +int odp_crypto_init_global(void)
>>
>>             Is crypto init now in odp_platform.c? It should be in
>>             odp_crypto.c or similar...
>>
>>         no problem
>>         I can make a dummy   odp_crypto.c for ks2 and point DPDK at the
>>         linux generic. I was trying to avoid
>>         connecting internals.
>>
>>
>>              >  {
>>              > -     odp_thread_init_global();
>>              > -
>>              > -     odp_system_info_init();
>>              > -
>>              > -     if (odp_init_dpdk()) {
>>              > -             ODP_ERR("ODP dpdk init failed.\n");
>>              > -             return -1;
>>              > -     }
>>              > -
>>              > -     if (odp_shm_init_global()) {
>>              > -             ODP_ERR("ODP shm init failed.\n");
>>              > -             return -1;
>>              > -     }
>>              > -
>>              > -     if (odp_buffer_pool_init_global()) {
>>              > -             ODP_ERR("ODP buffer pool init failed.\n");
>>              > -             return -1;
>>              > -     }
>>              > -
>>              > -     if (odp_queue_init_global()) {
>>              > -             ODP_ERR("ODP queue init failed.\n");
>>              > -             return -1;
>>              > -     }
>>              > -
>>              > -     if (odp_schedule_init_global()) {
>>              > -             ODP_ERR("ODP schedule init failed.\n");
>>              > -             return -1;
>>              > -     }
>>              > -
>>              > -     if (odp_pktio_init_global()) {
>>              > -             ODP_ERR("ODP packet io init failed.\n");
>>              > -             return -1;
>>              > -     }
>>              > -
>>              > -     if (odp_timer_init_global()) {
>>              > -             ODP_ERR("ODP timer init failed.\n");
>>              > -             return -1;
>>              > -     }
>>              > -
>>              > -     return 0;
>>              > -}
>>              > -
>>              > -
>>              > -int odp_init_local(int thr_id)
>>              > -{
>>              > -     odp_thread_init_local(thr_id);
>>              > -
>>              > -     if (odp_pktio_init_local()) {
>>              > -             ODP_ERR("ODP packet io local init
>> failed.\n");
>>              > -             return -1;
>>              > -     }
>>              > -
>>              > -     if (odp_schedule_init_local()) {
>>              > -             ODP_ERR("ODP schedule local init failed.\n");
>>              > -             return -1;
>>              > -     }
>>              > -
>>              >       return 0;
>>              >  }
>>              > diff --git a/platform/linux-generic/Makefile.am
>>             b/platform/linux-
>>              > generic/Makefile.am
>>              > index f4dfdc1..c4aae9d 100644
>>              > --- a/platform/linux-generic/Makefile.am
>>              > +++ b/platform/linux-generic/Makefile.am
>>              > @@ -62,6 +62,7 @@ __LIB__libodp_la_SOURCES = \
>>              >                          odp_packet_flags.c \
>>              >                          odp_packet_io.c \
>>              >                          odp_packet_socket.c \
>>              > +                        odp_platform.c \
>>              >                          odp_queue.c \
>>              >                          odp_ring.c \
>>              >                          odp_rwlock.c \
>>              > diff --git a/platform/linux-generic/include/api/odp_init.h
>>              > b/platform/linux-generic/include/api/odp_init.h
>>              > index 490324a..ece4547 100644
>>              > --- a/platform/linux-generic/include/api/odp_init.h
>>              > +++ b/platform/linux-generic/include/api/odp_init.h
>>              > @@ -22,24 +22,42 @@ extern "C" {
>>              >
>>              >  #include <odp_std_types.h>
>>              >
>>              > -
>>              > -
>>              > +/** ODP  initialisation data.
>>              > + * Data  that is required to initialize the ODP API with
>> the
>>              > + * application specific data such as specifying a
>>             logging callback, the
>>              > log
>>              > + * level etc.
>>              > + */
>>              > +typedef struct odp_global_init {
>>
>>             The struct name should be "struct odp_global_init_t",
>>             "struct odp_global_init_s" or "struct odp_global_init_".
>>
>>             The name without postfix (odp_global_init) should be
>>             reserved for variable names.
>>             no probelm
>>
>>         I will go with odp_init_s
>>
>>              > +} odp_global_init_t;
>>
>>             In short, this could be named odp_init_t. It's the main init
>>             struct there will be.
>>             If there will be a local init struct it could be then
>>             odp_init_local_t...
>>
>>              > +
>>              > +/** ODP platform initialization data.
>>              > + * @note ODP API does nothing with this data. It is the
>>             underlying
>>              > + * implementation requires it and any data passed here
>>             is not portable.
>>              > + * It is required that the application takes care of
>>             identifying and
>>              > + * passing any required platform specific data.
>>              > + */
>>              > +typedef struct odp_global_platform_init {
>>
>>             Same thing, rename struct.
>>
>>         no problem
>>         odp_init_platform_s
>>
>>
>>              > +} odp_platform_init_t;
>>
>>
>>             Same thing, odp_platform_init_t ??
>>
>>
>>              >
>>              >  /**
>>              > - * Perform global ODP initalisation.
>>              > - *
>>              > - * This function must be called once before calling
>>              > - * any other ODP API functions.
>>              > + * Perform global ODP initialisation.
>>              >   *
>>              > + * This function must be called once before calling any
>>             other ODP API
>>              > + * functions.
>>              > + * @param[in] params Those parameters that are
>>             interpreted by the ODP API
>>
>>             "Those parameters that are common to all ODP
>>             implementations. Specified in this API"
>>
>>             API does not interpret, implementation does...
>>
>>
>>         Ok, I am struggling with names
>>
>>         how about "platform SDK" for the underlying code such as DPDK,
>>         so that the platform has an API
>>         implementation -  which is actually implemented in terms of a
>>         platform SDK
>>
>>         We need to distinguish that implementations should not use the
>>         platform parameters unless the underlying
>>         framework requires it, not that any implementation is free to
>>         just add local stuff - or thats how it feels to me at least.
>>
>>
>>              > + * @param[in] platform_params Those parameters that are
>>             passed without
>>              > + * interpretation by the ODP API to the implementation.
>>
>>             " ... implementation specific parameter. Specified outside
>>             of ODP API"
>>
>>              >   * @return 0 if successful
>>              >   */
>>              > -int odp_init_global(void);
>>              > +int odp_init_global(odp_global_init_t *params,
>>              > +                 odp_global_platform_init_t
>>             *platform_params);
>>              >
>>              >
>>              >  /**
>>              > - * Perform thread local ODP initalisation.
>>              > + * Perform thread local ODP initialisation.
>>              >   *
>>              >   * All threads must call this function before calling
>>              > + *
>>
>>             Extra line feed?
>>
>>         Will fix
>>
>>
>>              >   * any other ODP API functions.
>>              >   * @param thr_id Thread id
>>              >   * @return 0 if successful
>>              > diff --git a/platform/linux-generic/include/odp_internal.h
>>              > b/platform/linux-generic/include/odp_internal.h
>>              > index aa79493..6c71fc9 100644
>>              > --- a/platform/linux-generic/include/odp_internal.h
>>              > +++ b/platform/linux-generic/include/odp_internal.h
>>              > @@ -4,6 +4,7 @@
>>              >   * SPDX-License-Identifier:     BSD-3-Clause
>>              >   */
>>              >
>>              > +#include <odp_init.h>
>>              >
>>              >  /**
>>              >   * @file
>>              > @@ -42,6 +43,8 @@ int odp_schedule_init_local(void);
>>              >  int odp_timer_init_global(void);
>>              >  int odp_timer_disarm_all(void);
>>              >
>>              > +int odp_init_platform(odp_global_platform_init_t
>>             *platform_params);
>>              > +
>>              >  #ifdef __cplusplus
>>              >  }
>>              >  #endif
>>              > diff --git a/platform/linux-generic/odp_init.c
>>             b/platform/linux-
>>              > generic/odp_init.c
>>              > index 5b7e192..f595def 100644
>>              > --- a/platform/linux-generic/odp_init.c
>>              > +++ b/platform/linux-generic/odp_init.c
>>              > @@ -8,13 +8,18 @@
>>              >  #include <odp_internal.h>
>>              >  #include <odp_debug.h>
>>              >
>>              > -
>>              > -int odp_init_global(void)
>>              > +int odp_init_global(odp_global_init_t *params
>> ODP_UNUSED,
>>              > +                 odp_global_platform_init_t
>>             *platform_params ODP_UNUSED)
>>              >  {
>>              >       odp_thread_init_global();
>>              >
>>              >       odp_system_info_init();
>>              >
>>              > +     if (odp_init_platform(platform_params)) {
>>
>>             platform_params is used here, so remove ODP_UNUSED from
>>             argument list.
>>
>>             On the other hand, it would be better to remove
>>             odp_init_platform()
>>             altogether since does not do anything. It's better to add it
>>             when actually used.
>>             The order of init calls is very important, e.g. now you
>>             could not allocate shared
>>             memory in odp_init_platform() because it's executed before
>>             shm init...
>>
>>           odp_init_platform() does do something for DPDK / KS2 and we
>>         need a place holder because
>>         we want this to be shared between implementations don't we ?
>>         This is the stuff that must happen before any ODP init starts to
>>         satisfy the platform SDK
>>
>>
>>             - Petri
>>
>>              > +             ODP_ERR("ODP platform init failed.\n");
>>              > +             return -1;
>>              > +     }
>>              > +
>>              >       if (odp_shm_init_global()) {
>>              >               ODP_ERR("ODP shm init failed.\n");
>>              >               return -1;
>>              > diff --git a/platform/linux-generic/odp_platform.c
>>             b/platform/linux-
>>              > generic/odp_platform.c
>>              > new file mode 100644
>>              > index 0000000..a9efa7f
>>              > --- /dev/null
>>              > +++ b/platform/linux-generic/odp_platform.c
>>              > @@ -0,0 +1,14 @@
>>              > +/* Copyright (c) 2014, Linaro Limited
>>              > + * All rights reserved.
>>              > + *
>>              > + * SPDX-License-Identifier:     BSD-3-Clause
>>              > + */
>>              > +
>>              > +#include <odp_init.h>
>>              > +#include <odp_internal.h>
>>              > +#include <odp_debug.h>
>>              > +
>>              > +int odp_init_platform(odp_global_platform_init_t
>>             *platform_params
>>              > ODP_UNUSED)
>>              > +{
>>              > +     return 0;
>>              > +}
>>
>>
>>
>>
>>
>>
>>
>>         --
>>         *Mike Holmes*
>>
>>         Linaro Technical Manager / Lead
>>         LNG - ODP
>>
>>         _______________________________________________
>>         lng-odp mailing list
>>         lng-odp@lists.linaro.org <mailto:lng-odp@lists.linaro.org>
>>         http://lists.linaro.org/mailman/listinfo/lng-odp
>>
>>
>>
>>
>>
>> --
>> *Mike Holmes*
>>
>> Linaro Technical Manager / Lead
>> LNG - ODP
>>
>>
>> _______________________________________________
>> lng-odp mailing list
>> lng-odp@lists.linaro.org
>> http://lists.linaro.org/mailman/listinfo/lng-odp
>>
>>
>>
>> To unsubscribe from this group and stop receiving emails from it, send an
>> email to lng-sc+unsubscribe@linaro.org.
>>
>>

Bill Fischofer Aug. 29, 2014, 5:42 p.m. UTC | #9

Bill Mill's point about debuggers would seem to tip the argument back to
option 1, which is what we have for the most part.



On Fri, Aug 29, 2014 at 12:34 PM, Mike Holmes <mike.holmes@linaro.org>
wrote:

> The common set is option 1, which is what we have in may places, I dont
> like that we make it possible to reference the struct without the typedef,
> but unless we have further dissent we have a winner.I think.
>
> Mike
>
>
>
>
> On 29 August 2014 13:28, William Mills <wmills@ti.com> wrote:
>
>> OK, Yes I never say anything on this list but ...
>>
>>
>> On 08/29/2014 01:19 PM, Mike Holmes wrote:
>>
>>> Ok, I need a better feel for consensus here or it will flip flop :), as
>>> this impacts all structs we really need a final answer
>>>
>>> typedef struct odp_<name>_t {...} odp_<name>_t;
>>> OR
>>> typedef struct odp_<name>_s {...} odp_<name>_t;
>>> OR
>>> typedef struct {...} odp_<name>_t;
>>>
>>> I prefer the latter, why even name it unless you need to reference the
>>> name inside the struct being typedefed ?
>>>
>>>
>> I wrote/collected our internal coding conventions and we insisted on a
>> name.  The reason (at least at that time) was some debuggers showed the
>> structure name but not the typedef name.
>>
>> We used struct odp_<name>_tag {...} to emphasize that it should not be
>> used in the code.  In retrospect I don't like that but either the _s or
>> _t above are OK.
>>
>>
>>>
>>>
>>> On 29 August 2014 13:10, Bill Fischofer <bill.fischofer@linaro.org
>>> <mailto:bill.fischofer@linaro.org>> wrote:
>>>
>>>     We've been using odp_<name>_t for typedef stuff, taking advantage
>>>     that of the fact that struct tags occupy a different namespace.  So
>>>
>>>     typedef struct odp_<name>_t {...} odp_<name>_t;
>>>
>>>     is perfectly valid and unambiguous C and avoids additional namespace
>>>     clutter.
>>>
>>>     odp_<name>_e has been suggested for ODP enums.
>>>
>>>     Bill
>>>
>>>
>>>     On Fri, Aug 29, 2014 at 12:05 PM, Mike Holmes
>>>     <mike.holmes@linaro.org <mailto:mike.holmes@linaro.org>> wrote:
>>>
>>>
>>>
>>>
>>>         On 29 August 2014 03:59, Savolainen, Petri (NSN - FI/Espoo)
>>>         <petri.savolainen@nsn.com <mailto:petri.savolainen@nsn.com>>
>>> wrote:
>>>
>>>
>>>
>>>              > -----Original Message-----
>>>              > From: lng-odp-bounces@lists.linaro.org
>>>             <mailto:lng-odp-bounces@lists.linaro.org> [mailto:lng-odp-
>>>             <mailto:lng-odp->
>>>              > bounces@lists.linaro.org
>>>             <mailto:bounces@lists.linaro.org>] On Behalf Of ext Mike
>>> Holmes
>>>              > Sent: Thursday, August 28, 2014 9:26 PM
>>>              > To: lng-odp@lists.linaro.org
>>>             <mailto:lng-odp@lists.linaro.org>
>>>              > Subject: [lng-odp] [PATCH v4] Add-global_init-paramiters
>>>              >
>>>              > Signed-off-by: Mike Holmes <mike.holmes@linaro.org
>>>             <mailto:mike.holmes@linaro.org>>
>>>
>>>              > ---
>>>              >
>>>              > v2:
>>>              > Factor out odp_platform_init so that odp_global_init is
>>>             only defined
>>>              > for linux_generic and resued.
>>>              >
>>>              > v3:
>>>              > Use -M to simplify the diff
>>>              > Fix missing doxygen comments
>>>              >
>>>              > v4:
>>>              > Improve the documentation.
>>>              >
>>>              >  example/generator/odp_generator.c                  |  2
>>> +-
>>>              >  example/l2fwd/odp_l2fwd.c                          |  2
>>> +-
>>>              >  example/odp_example/odp_example.c                  |  2
>>> +-
>>>              >  example/packet/odp_pktio.c                         |  2
>>> +-
>>>              >  example/timer/odp_timer_test.c                     |  2
>>> +-
>>>              >  platform/linux-dpdk/Makefile.am                    |  3
>>> +-
>>>              >  platform/linux-dpdk/{odp_init.c => odp_platform.c} | 63
>>>             +----------------
>>>              > --
>>>              >  platform/linux-generic/Makefile.am                 |  1
>>> +
>>>              >  platform/linux-generic/include/api/odp_init.h      | 34
>>>             +++++++---
>>>              >  platform/linux-generic/include/odp_internal.h      |  3
>>> +
>>>              >  platform/linux-generic/odp_init.c                  |  9
>>> ++-
>>>              >  platform/linux-generic/odp_platform.c              | 14
>>>             +++++
>>>              >  platform/linux-keystone2/Makefile.am               |  3
>>> +-
>>>              >  .../linux-keystone2/{odp_init.c => odp_platform.c} | 72
>>>             +++--------------
>>>              > -----
>>>              >  test/api_test/odp_common.c                         |  2
>>> +-
>>>              >  15 files changed, 72 insertions(+), 142 deletions(-)
>>>              >  rename platform/linux-dpdk/{odp_init.c =>
>>>             odp_platform.c} (50%)
>>>              >  create mode 100644 platform/linux-generic/odp_platform.c
>>>              >  rename platform/linux-keystone2/{odp_init.c =>
>>>             odp_platform.c} (72%)
>>>              >
>>>              > diff --git a/example/generator/odp_generator.c
>>>              > b/example/generator/odp_generator.c
>>>              > index 9fa9b37..ba7aa68 100644
>>>              > --- a/example/generator/odp_generator.c
>>>              > +++ b/example/generator/odp_generator.c
>>>              > @@ -525,7 +525,7 @@ int main(int argc, char *argv[])
>>>              >       int core_count;
>>>              >
>>>              >       /* Init ODP before calling anything else */
>>>              > -     if (odp_init_global()) {
>>>              > +     if (odp_init_global(NULL, NULL)) {
>>>              >               ODP_ERR("Error: ODP global init failed.\n");
>>>              >               exit(EXIT_FAILURE);
>>>              >       }
>>>              > diff --git a/example/l2fwd/odp_l2fwd.c
>>>             b/example/l2fwd/odp_l2fwd.c
>>>              > index d74449a..f8d6729 100644
>>>              > --- a/example/l2fwd/odp_l2fwd.c
>>>              > +++ b/example/l2fwd/odp_l2fwd.c
>>>              > @@ -323,7 +323,7 @@ int main(int argc, char *argv[])
>>>              >       odp_pktio_t pktio;
>>>              >
>>>              >       /* Init ODP before calling anything else */
>>>              > -     if (odp_init_global()) {
>>>              > +     if (odp_init_global(NULL, NULL)) {
>>>              >               ODP_ERR("Error: ODP global init failed.\n");
>>>              >               exit(EXIT_FAILURE);
>>>              >       }
>>>              > diff --git a/example/odp_example/odp_example.c
>>>              > b/example/odp_example/odp_example.c
>>>              > index f0bdf29..3d38ac7 100644
>>>              > --- a/example/odp_example/odp_example.c
>>>              > +++ b/example/odp_example/odp_example.c
>>>              > @@ -951,7 +951,7 @@ int main(int argc, char *argv[])
>>>              >
>>>              >       memset(thread_tbl, 0, sizeof(thread_tbl));
>>>              >
>>>              > -     if (odp_init_global()) {
>>>              > +     if (odp_init_global(NULL, NULL)) {
>>>              >               printf("ODP global init failed.\n");
>>>              >               return -1;
>>>              >       }
>>>              > diff --git a/example/packet/odp_pktio.c
>>>             b/example/packet/odp_pktio.c
>>>              > index f247bd0..f3543a0 100644
>>>              > --- a/example/packet/odp_pktio.c
>>>              > +++ b/example/packet/odp_pktio.c
>>>              > @@ -311,7 +311,7 @@ int main(int argc, char *argv[])
>>>              >       int core_count;
>>>              >
>>>              >       /* Init ODP before calling anything else */
>>>              > -     if (odp_init_global()) {
>>>              > +     if (odp_init_global(NULL, NULL)) {
>>>              >               ODP_ERR("Error: ODP global init failed.\n");
>>>              >               exit(EXIT_FAILURE);
>>>              >       }
>>>              > diff --git a/example/timer/odp_timer_test.c
>>>              > b/example/timer/odp_timer_test.c
>>>              > index bf1d7df..abf90aa 100644
>>>              > --- a/example/timer/odp_timer_test.c
>>>              > +++ b/example/timer/odp_timer_test.c
>>>              > @@ -260,7 +260,7 @@ int main(int argc, char *argv[])
>>>              >
>>>              >       memset(thread_tbl, 0, sizeof(thread_tbl));
>>>              >
>>>              > -     if (odp_init_global()) {
>>>              > +     if (odp_init_global(NULL, NULL)) {
>>>              >               printf("ODP global init failed.\n");
>>>              >               return -1;
>>>              >       }
>>>              > diff --git a/platform/linux-dpdk/Makefile.am
>>>             b/platform/linux-
>>>              > dpdk/Makefile.am
>>>              > index ff49b7d..70fa549 100644
>>>              > --- a/platform/linux-dpdk/Makefile.am
>>>              > +++ b/platform/linux-dpdk/Makefile.am
>>>              > @@ -67,13 +67,14 @@ __LIB__libodp_la_SOURCES = \
>>>              >                          odp_buffer.c \
>>>              >                          odp_buffer_pool.c \
>>>              >                          ../linux-generic/odp_coremask.c
>>> \
>>>              > -                        odp_init.c \
>>>              > +                        ../linux-generic/odp_init.c \
>>>              >                          odp_linux.c \
>>>              >                          odp_packet.c \
>>>              >                          odp_packet_dpdk.c \
>>>              >
>>>             ../linux-generic/odp_packet_flags.c \
>>>              >                          odp_packet_io.c \
>>>              >
>>>             ../linux-generic/odp_packet_socket.c \
>>>              > +                        odp_platform.c \
>>>              >                          odp_queue.c \
>>>              >                          ../linux-generic/odp_ring.c \
>>>              >                          ../linux-generic/odp_rwlock.c \
>>>              > diff --git a/platform/linux-dpdk/odp_init.c
>>> b/platform/linux-
>>>              > dpdk/odp_platform.c
>>>              > similarity index 50%
>>>              > rename from platform/linux-dpdk/odp_init.c
>>>              > rename to platform/linux-dpdk/odp_platform.c
>>>              > index ecc2066..5a054fd 100644
>>>              > --- a/platform/linux-dpdk/odp_init.c
>>>              > +++ b/platform/linux-dpdk/odp_platform.c
>>>              > @@ -1,4 +1,4 @@
>>>              > -/* Copyright (c) 2013, Linaro Limited
>>>              > +/* Copyright (c) 2014, Linaro Limited
>>>              >   * All rights reserved.
>>>              >   *
>>>              >   * SPDX-License-Identifier:     BSD-3-Clause
>>>              > @@ -9,7 +9,7 @@
>>>              >  #include <odp_debug.h>
>>>              >  #include <odp_packet_dpdk.h>
>>>              >
>>>              > -int odp_init_dpdk(void)
>>>              > +int odp_init_platform(odp_global_platform_init_t
>>>             *platform_params
>>>              > ODP_UNUSED)
>>>              >  {
>>>              >       int test_argc = 5;
>>>              >       char *test_argv[6];
>>>              > @@ -50,64 +50,7 @@ int odp_init_dpdk(void)
>>>              >       return 0;
>>>              >  }
>>>              >
>>>              > -int odp_init_global(void)
>>>              > +int odp_crypto_init_global(void)
>>>
>>>             Is crypto init now in odp_platform.c? It should be in
>>>             odp_crypto.c or similar...
>>>
>>>         no problem
>>>         I can make a dummy   odp_crypto.c for ks2 and point DPDK at the
>>>         linux generic. I was trying to avoid
>>>         connecting internals.
>>>
>>>
>>>              >  {
>>>              > -     odp_thread_init_global();
>>>              > -
>>>              > -     odp_system_info_init();
>>>              > -
>>>              > -     if (odp_init_dpdk()) {
>>>              > -             ODP_ERR("ODP dpdk init failed.\n");
>>>              > -             return -1;
>>>              > -     }
>>>              > -
>>>              > -     if (odp_shm_init_global()) {
>>>              > -             ODP_ERR("ODP shm init failed.\n");
>>>              > -             return -1;
>>>              > -     }
>>>              > -
>>>              > -     if (odp_buffer_pool_init_global()) {
>>>              > -             ODP_ERR("ODP buffer pool init failed.\n");
>>>              > -             return -1;
>>>              > -     }
>>>              > -
>>>              > -     if (odp_queue_init_global()) {
>>>              > -             ODP_ERR("ODP queue init failed.\n");
>>>              > -             return -1;
>>>              > -     }
>>>              > -
>>>              > -     if (odp_schedule_init_global()) {
>>>              > -             ODP_ERR("ODP schedule init failed.\n");
>>>              > -             return -1;
>>>              > -     }
>>>              > -
>>>              > -     if (odp_pktio_init_global()) {
>>>              > -             ODP_ERR("ODP packet io init failed.\n");
>>>              > -             return -1;
>>>              > -     }
>>>              > -
>>>              > -     if (odp_timer_init_global()) {
>>>              > -             ODP_ERR("ODP timer init failed.\n");
>>>              > -             return -1;
>>>              > -     }
>>>              > -
>>>              > -     return 0;
>>>              > -}
>>>              > -
>>>              > -
>>>              > -int odp_init_local(int thr_id)
>>>              > -{
>>>              > -     odp_thread_init_local(thr_id);
>>>              > -
>>>              > -     if (odp_pktio_init_local()) {
>>>              > -             ODP_ERR("ODP packet io local init
>>> failed.\n");
>>>              > -             return -1;
>>>              > -     }
>>>              > -
>>>              > -     if (odp_schedule_init_local()) {
>>>              > -             ODP_ERR("ODP schedule local init
>>> failed.\n");
>>>              > -             return -1;
>>>              > -     }
>>>              > -
>>>              >       return 0;
>>>              >  }
>>>              > diff --git a/platform/linux-generic/Makefile.am
>>>             b/platform/linux-
>>>              > generic/Makefile.am
>>>              > index f4dfdc1..c4aae9d 100644
>>>              > --- a/platform/linux-generic/Makefile.am
>>>              > +++ b/platform/linux-generic/Makefile.am
>>>              > @@ -62,6 +62,7 @@ __LIB__libodp_la_SOURCES = \
>>>              >                          odp_packet_flags.c \
>>>              >                          odp_packet_io.c \
>>>              >                          odp_packet_socket.c \
>>>              > +                        odp_platform.c \
>>>              >                          odp_queue.c \
>>>              >                          odp_ring.c \
>>>              >                          odp_rwlock.c \
>>>              > diff --git a/platform/linux-generic/
>>> include/api/odp_init.h
>>>              > b/platform/linux-generic/include/api/odp_init.h
>>>              > index 490324a..ece4547 100644
>>>              > --- a/platform/linux-generic/include/api/odp_init.h
>>>              > +++ b/platform/linux-generic/include/api/odp_init.h
>>>              > @@ -22,24 +22,42 @@ extern "C" {
>>>              >
>>>              >  #include <odp_std_types.h>
>>>              >
>>>              > -
>>>              > -
>>>              > +/** ODP  initialisation data.
>>>              > + * Data  that is required to initialize the ODP API with
>>> the
>>>              > + * application specific data such as specifying a
>>>             logging callback, the
>>>              > log
>>>              > + * level etc.
>>>              > + */
>>>              > +typedef struct odp_global_init {
>>>
>>>             The struct name should be "struct odp_global_init_t",
>>>             "struct odp_global_init_s" or "struct odp_global_init_".
>>>
>>>             The name without postfix (odp_global_init) should be
>>>             reserved for variable names.
>>>             no probelm
>>>
>>>         I will go with odp_init_s
>>>
>>>              > +} odp_global_init_t;
>>>
>>>             In short, this could be named odp_init_t. It's the main init
>>>             struct there will be.
>>>             If there will be a local init struct it could be then
>>>             odp_init_local_t...
>>>
>>>              > +
>>>              > +/** ODP platform initialization data.
>>>              > + * @note ODP API does nothing with this data. It is the
>>>             underlying
>>>              > + * implementation requires it and any data passed here
>>>             is not portable.
>>>              > + * It is required that the application takes care of
>>>             identifying and
>>>              > + * passing any required platform specific data.
>>>              > + */
>>>              > +typedef struct odp_global_platform_init {
>>>
>>>             Same thing, rename struct.
>>>
>>>         no problem
>>>         odp_init_platform_s
>>>
>>>
>>>              > +} odp_platform_init_t;
>>>
>>>
>>>             Same thing, odp_platform_init_t ??
>>>
>>>
>>>              >
>>>              >  /**
>>>              > - * Perform global ODP initalisation.
>>>              > - *
>>>              > - * This function must be called once before calling
>>>              > - * any other ODP API functions.
>>>              > + * Perform global ODP initialisation.
>>>              >   *
>>>              > + * This function must be called once before calling any
>>>             other ODP API
>>>              > + * functions.
>>>              > + * @param[in] params Those parameters that are
>>>             interpreted by the ODP API
>>>
>>>             "Those parameters that are common to all ODP
>>>             implementations. Specified in this API"
>>>
>>>             API does not interpret, implementation does...
>>>
>>>
>>>         Ok, I am struggling with names
>>>
>>>         how about "platform SDK" for the underlying code such as DPDK,
>>>         so that the platform has an API
>>>         implementation -  which is actually implemented in terms of a
>>>         platform SDK
>>>
>>>         We need to distinguish that implementations should not use the
>>>         platform parameters unless the underlying
>>>         framework requires it, not that any implementation is free to
>>>         just add local stuff - or thats how it feels to me at least.
>>>
>>>
>>>              > + * @param[in] platform_params Those parameters that are
>>>             passed without
>>>              > + * interpretation by the ODP API to the implementation.
>>>
>>>             " ... implementation specific parameter. Specified outside
>>>             of ODP API"
>>>
>>>              >   * @return 0 if successful
>>>              >   */
>>>              > -int odp_init_global(void);
>>>              > +int odp_init_global(odp_global_init_t *params,
>>>              > +                 odp_global_platform_init_t
>>>             *platform_params);
>>>              >
>>>              >
>>>              >  /**
>>>              > - * Perform thread local ODP initalisation.
>>>              > + * Perform thread local ODP initialisation.
>>>              >   *
>>>              >   * All threads must call this function before calling
>>>              > + *
>>>
>>>             Extra line feed?
>>>
>>>         Will fix
>>>
>>>
>>>              >   * any other ODP API functions.
>>>              >   * @param thr_id Thread id
>>>              >   * @return 0 if successful
>>>              > diff --git a/platform/linux-generic/
>>> include/odp_internal.h
>>>              > b/platform/linux-generic/include/odp_internal.h
>>>              > index aa79493..6c71fc9 100644
>>>              > --- a/platform/linux-generic/include/odp_internal.h
>>>              > +++ b/platform/linux-generic/include/odp_internal.h
>>>              > @@ -4,6 +4,7 @@
>>>              >   * SPDX-License-Identifier:     BSD-3-Clause
>>>              >   */
>>>              >
>>>              > +#include <odp_init.h>
>>>              >
>>>              >  /**
>>>              >   * @file
>>>              > @@ -42,6 +43,8 @@ int odp_schedule_init_local(void);
>>>              >  int odp_timer_init_global(void);
>>>              >  int odp_timer_disarm_all(void);
>>>              >
>>>              > +int odp_init_platform(odp_global_platform_init_t
>>>             *platform_params);
>>>              > +
>>>              >  #ifdef __cplusplus
>>>              >  }
>>>              >  #endif
>>>              > diff --git a/platform/linux-generic/odp_init.c
>>>             b/platform/linux-
>>>              > generic/odp_init.c
>>>              > index 5b7e192..f595def 100644
>>>              > --- a/platform/linux-generic/odp_init.c
>>>              > +++ b/platform/linux-generic/odp_init.c
>>>              > @@ -8,13 +8,18 @@
>>>              >  #include <odp_internal.h>
>>>              >  #include <odp_debug.h>
>>>              >
>>>              > -
>>>              > -int odp_init_global(void)
>>>              > +int odp_init_global(odp_global_init_t *params
>>> ODP_UNUSED,
>>>              > +                 odp_global_platform_init_t
>>>             *platform_params ODP_UNUSED)
>>>              >  {
>>>              >       odp_thread_init_global();
>>>              >
>>>              >       odp_system_info_init();
>>>              >
>>>              > +     if (odp_init_platform(platform_params)) {
>>>
>>>             platform_params is used here, so remove ODP_UNUSED from
>>>             argument list.
>>>
>>>             On the other hand, it would be better to remove
>>>             odp_init_platform()
>>>             altogether since does not do anything. It's better to add it
>>>             when actually used.
>>>             The order of init calls is very important, e.g. now you
>>>             could not allocate shared
>>>             memory in odp_init_platform() because it's executed before
>>>             shm init...
>>>
>>>           odp_init_platform() does do something for DPDK / KS2 and we
>>>         need a place holder because
>>>         we want this to be shared between implementations don't we ?
>>>         This is the stuff that must happen before any ODP init starts to
>>>         satisfy the platform SDK
>>>
>>>
>>>             - Petri
>>>
>>>              > +             ODP_ERR("ODP platform init failed.\n");
>>>              > +             return -1;
>>>              > +     }
>>>              > +
>>>              >       if (odp_shm_init_global()) {
>>>              >               ODP_ERR("ODP shm init failed.\n");
>>>              >               return -1;
>>>              > diff --git a/platform/linux-generic/odp_platform.c
>>>             b/platform/linux-
>>>              > generic/odp_platform.c
>>>              > new file mode 100644
>>>              > index 0000000..a9efa7f
>>>              > --- /dev/null
>>>              > +++ b/platform/linux-generic/odp_platform.c
>>>              > @@ -0,0 +1,14 @@
>>>              > +/* Copyright (c) 2014, Linaro Limited
>>>              > + * All rights reserved.
>>>              > + *
>>>              > + * SPDX-License-Identifier:     BSD-3-Clause
>>>              > + */
>>>              > +
>>>              > +#include <odp_init.h>
>>>              > +#include <odp_internal.h>
>>>              > +#include <odp_debug.h>
>>>              > +
>>>              > +int odp_init_platform(odp_global_platform_init_t
>>>             *platform_params
>>>              > ODP_UNUSED)
>>>              > +{
>>>              > +     return 0;
>>>              > +}
>>>
>>>
>>>
>>>
>>>
>>>
>>>
>>>         --
>>>         *Mike Holmes*
>>>
>>>         Linaro Technical Manager / Lead
>>>         LNG - ODP
>>>
>>>         _______________________________________________
>>>         lng-odp mailing list
>>>         lng-odp@lists.linaro.org <mailto:lng-odp@lists.linaro.org>
>>>         http://lists.linaro.org/mailman/listinfo/lng-odp
>>>
>>>
>>>
>>>
>>>
>>> --
>>> *Mike Holmes*
>>>
>>> Linaro Technical Manager / Lead
>>> LNG - ODP
>>>
>>>
>>> _______________________________________________
>>> lng-odp mailing list
>>> lng-odp@lists.linaro.org
>>> http://lists.linaro.org/mailman/listinfo/lng-odp
>>>
>>>
>>>
>>> To unsubscribe from this group and stop receiving emails from it, send
>>> an email to lng-sc+unsubscribe@linaro.org.
>>>
>>>
>
>
> --
> *Mike Holmes*
> Linaro Technical Manager / Lead
> LNG - ODP
>

Wiles, Roger Keith Aug. 29, 2014, 5:53 p.m. UTC | #10

I do like having the _s as it is more readable IMO. What happens when you have a self reference in another structure.

#1
typedef struct odp_<name>_t {
	struct odp_<name>_t	* name;
} odp_<name>_t;

OR #2

typedef struct odp_<name>_s {
	struct odp_<name>_s	* name;
} odp_<name>_t;

OR #3

typedef struct odp_<name> {
	struct odp_<name>		* name;
} odp_<name>_t;

I feel like I am getting my eyes check is one better or two better or one better or three better :-)

I vote for #2 as it is the cleanest and most readable, we should not rely on the fact that structure name and typedef name are in different name spaces.

On Aug 29, 2014, at 12:34 PM, Mike Holmes <mike.holmes@linaro.org> wrote:

> The common set is option 1, which is what we have in may places, I dont like that we make it possible to reference the struct without the typedef, but unless we have further dissent we have a winner.I think.
> 
> Mike
> 
> 
> 
> 
> On 29 August 2014 13:28, William Mills <wmills@ti.com> wrote:
> OK, Yes I never say anything on this list but ...
> 
> 
> On 08/29/2014 01:19 PM, Mike Holmes wrote:
> Ok, I need a better feel for consensus here or it will flip flop :), as
> this impacts all structs we really need a final answer
> 
> typedef struct odp_<name>_t {...} odp_<name>_t;
> OR
> typedef struct odp_<name>_s {...} odp_<name>_t;
> OR
> typedef struct {...} odp_<name>_t;
> 
> I prefer the latter, why even name it unless you need to reference the
> name inside the struct being typedefed ?
> 
> 
> I wrote/collected our internal coding conventions and we insisted on a
> name.  The reason (at least at that time) was some debuggers showed the structure name but not the typedef name.
> 
> We used struct odp_<name>_tag {...} to emphasize that it should not be
> used in the code.  In retrospect I don't like that but either the _s or
> _t above are OK.
> 
> 
> 
> 
> On 29 August 2014 13:10, Bill Fischofer <bill.fischofer@linaro.org
> <mailto:bill.fischofer@linaro.org>> wrote:
> 
>     We've been using odp_<name>_t for typedef stuff, taking advantage
>     that of the fact that struct tags occupy a different namespace.  So
> 
>     typedef struct odp_<name>_t {...} odp_<name>_t;
> 
>     is perfectly valid and unambiguous C and avoids additional namespace
>     clutter.
> 
>     odp_<name>_e has been suggested for ODP enums.
> 
>     Bill
> 
> 
>     On Fri, Aug 29, 2014 at 12:05 PM, Mike Holmes
>     <mike.holmes@linaro.org <mailto:mike.holmes@linaro.org>> wrote:
> 
> 
> 
> 
>         On 29 August 2014 03:59, Savolainen, Petri (NSN - FI/Espoo)
>         <petri.savolainen@nsn.com <mailto:petri.savolainen@nsn.com>> wrote:
> 
> 
> 
>              > -----Original Message-----
>              > From: lng-odp-bounces@lists.linaro.org
>             <mailto:lng-odp-bounces@lists.linaro.org> [mailto:lng-odp-
>             <mailto:lng-odp->
>              > bounces@lists.linaro.org
>             <mailto:bounces@lists.linaro.org>] On Behalf Of ext Mike Holmes
>              > Sent: Thursday, August 28, 2014 9:26 PM
>              > To: lng-odp@lists.linaro.org
>             <mailto:lng-odp@lists.linaro.org>
>              > Subject: [lng-odp] [PATCH v4] Add-global_init-paramiters
>              >
>              > Signed-off-by: Mike Holmes <mike.holmes@linaro.org
>             <mailto:mike.holmes@linaro.org>>
> 
>              > ---
>              >
>              > v2:
>              > Factor out odp_platform_init so that odp_global_init is
>             only defined
>              > for linux_generic and resued.
>              >
>              > v3:
>              > Use -M to simplify the diff
>              > Fix missing doxygen comments
>              >
>              > v4:
>              > Improve the documentation.
>              >
>              >  example/generator/odp_generator.c                  |  2 +-
>              >  example/l2fwd/odp_l2fwd.c                          |  2 +-
>              >  example/odp_example/odp_example.c                  |  2 +-
>              >  example/packet/odp_pktio.c                         |  2 +-
>              >  example/timer/odp_timer_test.c                     |  2 +-
>              >  platform/linux-dpdk/Makefile.am                    |  3 +-
>              >  platform/linux-dpdk/{odp_init.c => odp_platform.c} | 63
>             +----------------
>              > --
>              >  platform/linux-generic/Makefile.am                 |  1 +
>              >  platform/linux-generic/include/api/odp_init.h      | 34
>             +++++++---
>              >  platform/linux-generic/include/odp_internal.h      |  3 +
>              >  platform/linux-generic/odp_init.c                  |  9 ++-
>              >  platform/linux-generic/odp_platform.c              | 14
>             +++++
>              >  platform/linux-keystone2/Makefile.am               |  3 +-
>              >  .../linux-keystone2/{odp_init.c => odp_platform.c} | 72
>             +++--------------
>              > -----
>              >  test/api_test/odp_common.c                         |  2 +-
>              >  15 files changed, 72 insertions(+), 142 deletions(-)
>              >  rename platform/linux-dpdk/{odp_init.c =>
>             odp_platform.c} (50%)
>              >  create mode 100644 platform/linux-generic/odp_platform.c
>              >  rename platform/linux-keystone2/{odp_init.c =>
>             odp_platform.c} (72%)
>              >
>              > diff --git a/example/generator/odp_generator.c
>              > b/example/generator/odp_generator.c
>              > index 9fa9b37..ba7aa68 100644
>              > --- a/example/generator/odp_generator.c
>              > +++ b/example/generator/odp_generator.c
>              > @@ -525,7 +525,7 @@ int main(int argc, char *argv[])
>              >       int core_count;
>              >
>              >       /* Init ODP before calling anything else */
>              > -     if (odp_init_global()) {
>              > +     if (odp_init_global(NULL, NULL)) {
>              >               ODP_ERR("Error: ODP global init failed.\n");
>              >               exit(EXIT_FAILURE);
>              >       }
>              > diff --git a/example/l2fwd/odp_l2fwd.c
>             b/example/l2fwd/odp_l2fwd.c
>              > index d74449a..f8d6729 100644
>              > --- a/example/l2fwd/odp_l2fwd.c
>              > +++ b/example/l2fwd/odp_l2fwd.c
>              > @@ -323,7 +323,7 @@ int main(int argc, char *argv[])
>              >       odp_pktio_t pktio;
>              >
>              >       /* Init ODP before calling anything else */
>              > -     if (odp_init_global()) {
>              > +     if (odp_init_global(NULL, NULL)) {
>              >               ODP_ERR("Error: ODP global init failed.\n");
>              >               exit(EXIT_FAILURE);
>              >       }
>              > diff --git a/example/odp_example/odp_example.c
>              > b/example/odp_example/odp_example.c
>              > index f0bdf29..3d38ac7 100644
>              > --- a/example/odp_example/odp_example.c
>              > +++ b/example/odp_example/odp_example.c
>              > @@ -951,7 +951,7 @@ int main(int argc, char *argv[])
>              >
>              >       memset(thread_tbl, 0, sizeof(thread_tbl));
>              >
>              > -     if (odp_init_global()) {
>              > +     if (odp_init_global(NULL, NULL)) {
>              >               printf("ODP global init failed.\n");
>              >               return -1;
>              >       }
>              > diff --git a/example/packet/odp_pktio.c
>             b/example/packet/odp_pktio.c
>              > index f247bd0..f3543a0 100644
>              > --- a/example/packet/odp_pktio.c
>              > +++ b/example/packet/odp_pktio.c
>              > @@ -311,7 +311,7 @@ int main(int argc, char *argv[])
>              >       int core_count;
>              >
>              >       /* Init ODP before calling anything else */
>              > -     if (odp_init_global()) {
>              > +     if (odp_init_global(NULL, NULL)) {
>              >               ODP_ERR("Error: ODP global init failed.\n");
>              >               exit(EXIT_FAILURE);
>              >       }
>              > diff --git a/example/timer/odp_timer_test.c
>              > b/example/timer/odp_timer_test.c
>              > index bf1d7df..abf90aa 100644
>              > --- a/example/timer/odp_timer_test.c
>              > +++ b/example/timer/odp_timer_test.c
>              > @@ -260,7 +260,7 @@ int main(int argc, char *argv[])
>              >
>              >       memset(thread_tbl, 0, sizeof(thread_tbl));
>              >
>              > -     if (odp_init_global()) {
>              > +     if (odp_init_global(NULL, NULL)) {
>              >               printf("ODP global init failed.\n");
>              >               return -1;
>              >       }
>              > diff --git a/platform/linux-dpdk/Makefile.am
>             b/platform/linux-
>              > dpdk/Makefile.am
>              > index ff49b7d..70fa549 100644
>              > --- a/platform/linux-dpdk/Makefile.am
>              > +++ b/platform/linux-dpdk/Makefile.am
>              > @@ -67,13 +67,14 @@ __LIB__libodp_la_SOURCES = \
>              >                          odp_buffer.c \
>              >                          odp_buffer_pool.c \
>              >                          ../linux-generic/odp_coremask.c \
>              > -                        odp_init.c \
>              > +                        ../linux-generic/odp_init.c \
>              >                          odp_linux.c \
>              >                          odp_packet.c \
>              >                          odp_packet_dpdk.c \
>              >
>             ../linux-generic/odp_packet_flags.c \
>              >                          odp_packet_io.c \
>              >
>             ../linux-generic/odp_packet_socket.c \
>              > +                        odp_platform.c \
>              >                          odp_queue.c \
>              >                          ../linux-generic/odp_ring.c \
>              >                          ../linux-generic/odp_rwlock.c \
>              > diff --git a/platform/linux-dpdk/odp_init.c b/platform/linux-
>              > dpdk/odp_platform.c
>              > similarity index 50%
>              > rename from platform/linux-dpdk/odp_init.c
>              > rename to platform/linux-dpdk/odp_platform.c
>              > index ecc2066..5a054fd 100644
>              > --- a/platform/linux-dpdk/odp_init.c
>              > +++ b/platform/linux-dpdk/odp_platform.c
>              > @@ -1,4 +1,4 @@
>              > -/* Copyright (c) 2013, Linaro Limited
>              > +/* Copyright (c) 2014, Linaro Limited
>              >   * All rights reserved.
>              >   *
>              >   * SPDX-License-Identifier:     BSD-3-Clause
>              > @@ -9,7 +9,7 @@
>              >  #include <odp_debug.h>
>              >  #include <odp_packet_dpdk.h>
>              >
>              > -int odp_init_dpdk(void)
>              > +int odp_init_platform(odp_global_platform_init_t
>             *platform_params
>              > ODP_UNUSED)
>              >  {
>              >       int test_argc = 5;
>              >       char *test_argv[6];
>              > @@ -50,64 +50,7 @@ int odp_init_dpdk(void)
>              >       return 0;
>              >  }
>              >
>              > -int odp_init_global(void)
>              > +int odp_crypto_init_global(void)
> 
>             Is crypto init now in odp_platform.c? It should be in
>             odp_crypto.c or similar...
> 
>         no problem
>         I can make a dummy   odp_crypto.c for ks2 and point DPDK at the
>         linux generic. I was trying to avoid
>         connecting internals.
> 
> 
>              >  {
>              > -     odp_thread_init_global();
>              > -
>              > -     odp_system_info_init();
>              > -
>              > -     if (odp_init_dpdk()) {
>              > -             ODP_ERR("ODP dpdk init failed.\n");
>              > -             return -1;
>              > -     }
>              > -
>              > -     if (odp_shm_init_global()) {
>              > -             ODP_ERR("ODP shm init failed.\n");
>              > -             return -1;
>              > -     }
>              > -
>              > -     if (odp_buffer_pool_init_global()) {
>              > -             ODP_ERR("ODP buffer pool init failed.\n");
>              > -             return -1;
>              > -     }
>              > -
>              > -     if (odp_queue_init_global()) {
>              > -             ODP_ERR("ODP queue init failed.\n");
>              > -             return -1;
>              > -     }
>              > -
>              > -     if (odp_schedule_init_global()) {
>              > -             ODP_ERR("ODP schedule init failed.\n");
>              > -             return -1;
>              > -     }
>              > -
>              > -     if (odp_pktio_init_global()) {
>              > -             ODP_ERR("ODP packet io init failed.\n");
>              > -             return -1;
>              > -     }
>              > -
>              > -     if (odp_timer_init_global()) {
>              > -             ODP_ERR("ODP timer init failed.\n");
>              > -             return -1;
>              > -     }
>              > -
>              > -     return 0;
>              > -}
>              > -
>              > -
>              > -int odp_init_local(int thr_id)
>              > -{
>              > -     odp_thread_init_local(thr_id);
>              > -
>              > -     if (odp_pktio_init_local()) {
>              > -             ODP_ERR("ODP packet io local init failed.\n");
>              > -             return -1;
>              > -     }
>              > -
>              > -     if (odp_schedule_init_local()) {
>              > -             ODP_ERR("ODP schedule local init failed.\n");
>              > -             return -1;
>              > -     }
>              > -
>              >       return 0;
>              >  }
>              > diff --git a/platform/linux-generic/Makefile.am
>             b/platform/linux-
>              > generic/Makefile.am
>              > index f4dfdc1..c4aae9d 100644
>              > --- a/platform/linux-generic/Makefile.am
>              > +++ b/platform/linux-generic/Makefile.am
>              > @@ -62,6 +62,7 @@ __LIB__libodp_la_SOURCES = \
>              >                          odp_packet_flags.c \
>              >                          odp_packet_io.c \
>              >                          odp_packet_socket.c \
>              > +                        odp_platform.c \
>              >                          odp_queue.c \
>              >                          odp_ring.c \
>              >                          odp_rwlock.c \
>              > diff --git a/platform/linux-generic/include/api/odp_init.h
>              > b/platform/linux-generic/include/api/odp_init.h
>              > index 490324a..ece4547 100644
>              > --- a/platform/linux-generic/include/api/odp_init.h
>              > +++ b/platform/linux-generic/include/api/odp_init.h
>              > @@ -22,24 +22,42 @@ extern "C" {
>              >
>              >  #include <odp_std_types.h>
>              >
>              > -
>              > -
>              > +/** ODP  initialisation data.
>              > + * Data  that is required to initialize the ODP API with the
>              > + * application specific data such as specifying a
>             logging callback, the
>              > log
>              > + * level etc.
>              > + */
>              > +typedef struct odp_global_init {
> 
>             The struct name should be "struct odp_global_init_t",
>             "struct odp_global_init_s" or "struct odp_global_init_".
> 
>             The name without postfix (odp_global_init) should be
>             reserved for variable names.
>             no probelm
> 
>         I will go with odp_init_s
> 
>              > +} odp_global_init_t;
> 
>             In short, this could be named odp_init_t. It's the main init
>             struct there will be.
>             If there will be a local init struct it could be then
>             odp_init_local_t...
> 
>              > +
>              > +/** ODP platform initialization data.
>              > + * @note ODP API does nothing with this data. It is the
>             underlying
>              > + * implementation requires it and any data passed here
>             is not portable.
>              > + * It is required that the application takes care of
>             identifying and
>              > + * passing any required platform specific data.
>              > + */
>              > +typedef struct odp_global_platform_init {
> 
>             Same thing, rename struct.
> 
>         no problem
>         odp_init_platform_s
> 
> 
>              > +} odp_platform_init_t;
> 
> 
>             Same thing, odp_platform_init_t ??
> 
> 
>              >
>              >  /**
>              > - * Perform global ODP initalisation.
>              > - *
>              > - * This function must be called once before calling
>              > - * any other ODP API functions.
>              > + * Perform global ODP initialisation.
>              >   *
>              > + * This function must be called once before calling any
>             other ODP API
>              > + * functions.
>              > + * @param[in] params Those parameters that are
>             interpreted by the ODP API
> 
>             "Those parameters that are common to all ODP
>             implementations. Specified in this API"
> 
>             API does not interpret, implementation does...
> 
> 
>         Ok, I am struggling with names
> 
>         how about "platform SDK" for the underlying code such as DPDK,
>         so that the platform has an API
>         implementation -  which is actually implemented in terms of a
>         platform SDK
> 
>         We need to distinguish that implementations should not use the
>         platform parameters unless the underlying
>         framework requires it, not that any implementation is free to
>         just add local stuff - or thats how it feels to me at least.
> 
> 
>              > + * @param[in] platform_params Those parameters that are
>             passed without
>              > + * interpretation by the ODP API to the implementation.
> 
>             " ... implementation specific parameter. Specified outside
>             of ODP API"
> 
>              >   * @return 0 if successful
>              >   */
>              > -int odp_init_global(void);
>              > +int odp_init_global(odp_global_init_t *params,
>              > +                 odp_global_platform_init_t
>             *platform_params);
>              >
>              >
>              >  /**
>              > - * Perform thread local ODP initalisation.
>              > + * Perform thread local ODP initialisation.
>              >   *
>              >   * All threads must call this function before calling
>              > + *
> 
>             Extra line feed?
> 
>         Will fix
> 
> 
>              >   * any other ODP API functions.
>              >   * @param thr_id Thread id
>              >   * @return 0 if successful
>              > diff --git a/platform/linux-generic/include/odp_internal.h
>              > b/platform/linux-generic/include/odp_internal.h
>              > index aa79493..6c71fc9 100644
>              > --- a/platform/linux-generic/include/odp_internal.h
>              > +++ b/platform/linux-generic/include/odp_internal.h
>              > @@ -4,6 +4,7 @@
>              >   * SPDX-License-Identifier:     BSD-3-Clause
>              >   */
>              >
>              > +#include <odp_init.h>
>              >
>              >  /**
>              >   * @file
>              > @@ -42,6 +43,8 @@ int odp_schedule_init_local(void);
>              >  int odp_timer_init_global(void);
>              >  int odp_timer_disarm_all(void);
>              >
>              > +int odp_init_platform(odp_global_platform_init_t
>             *platform_params);
>              > +
>              >  #ifdef __cplusplus
>              >  }
>              >  #endif
>              > diff --git a/platform/linux-generic/odp_init.c
>             b/platform/linux-
>              > generic/odp_init.c
>              > index 5b7e192..f595def 100644
>              > --- a/platform/linux-generic/odp_init.c
>              > +++ b/platform/linux-generic/odp_init.c
>              > @@ -8,13 +8,18 @@
>              >  #include <odp_internal.h>
>              >  #include <odp_debug.h>
>              >
>              > -
>              > -int odp_init_global(void)
>              > +int odp_init_global(odp_global_init_t *params  ODP_UNUSED,
>              > +                 odp_global_platform_init_t
>             *platform_params ODP_UNUSED)
>              >  {
>              >       odp_thread_init_global();
>              >
>              >       odp_system_info_init();
>              >
>              > +     if (odp_init_platform(platform_params)) {
> 
>             platform_params is used here, so remove ODP_UNUSED from
>             argument list.
> 
>             On the other hand, it would be better to remove
>             odp_init_platform()
>             altogether since does not do anything. It's better to add it
>             when actually used.
>             The order of init calls is very important, e.g. now you
>             could not allocate shared
>             memory in odp_init_platform() because it's executed before
>             shm init...
> 
>           odp_init_platform() does do something for DPDK / KS2 and we
>         need a place holder because
>         we want this to be shared between implementations don't we ?
>         This is the stuff that must happen before any ODP init starts to
>         satisfy the platform SDK
> 
> 
>             - Petri
> 
>              > +             ODP_ERR("ODP platform init failed.\n");
>              > +             return -1;
>              > +     }
>              > +
>              >       if (odp_shm_init_global()) {
>              >               ODP_ERR("ODP shm init failed.\n");
>              >               return -1;
>              > diff --git a/platform/linux-generic/odp_platform.c
>             b/platform/linux-
>              > generic/odp_platform.c
>              > new file mode 100644
>              > index 0000000..a9efa7f
>              > --- /dev/null
>              > +++ b/platform/linux-generic/odp_platform.c
>              > @@ -0,0 +1,14 @@
>              > +/* Copyright (c) 2014, Linaro Limited
>              > + * All rights reserved.
>              > + *
>              > + * SPDX-License-Identifier:     BSD-3-Clause
>              > + */
>              > +
>              > +#include <odp_init.h>
>              > +#include <odp_internal.h>
>              > +#include <odp_debug.h>
>              > +
>              > +int odp_init_platform(odp_global_platform_init_t
>             *platform_params
>              > ODP_UNUSED)
>              > +{
>              > +     return 0;
>              > +}
> 
> 
> 
> 
> 
> 
> 
>         --
>         *Mike Holmes*
> 
>         Linaro Technical Manager / Lead
>         LNG - ODP
> 
>         _______________________________________________
>         lng-odp mailing list
>         lng-odp@lists.linaro.org <mailto:lng-odp@lists.linaro.org>
>         http://lists.linaro.org/mailman/listinfo/lng-odp
> 
> 
> 
> 
> 
> --
> *Mike Holmes*
> 
> Linaro Technical Manager / Lead
> LNG - ODP
> 
> 
> _______________________________________________
> lng-odp mailing list
> lng-odp@lists.linaro.org
> http://lists.linaro.org/mailman/listinfo/lng-odp
> 
> 
> 
> To unsubscribe from this group and stop receiving emails from it, send an email to lng-sc+unsubscribe@linaro.org.
> 
> 
> 
> 
> -- 
> Mike Holmes 
> Linaro Technical Manager / Lead 
> LNG - ODP
> _______________________________________________
> lng-odp mailing list
> lng-odp@lists.linaro.org
> http://lists.linaro.org/mailman/listinfo/lng-odp

Keith Wiles, Principal Technologist with CTO office, Wind River mobile 972-213-5533

Savolainen, Petri (NSN - FI/Espoo) Sept. 1, 2014, 8:07 a.m. UTC | #11

> diff --git a/platform/linux-generic/odp_init.c b/platform/linux-

> generic/odp_init.c

> index 5b7e192..f595def 100644

> --- a/platform/linux-generic/odp_init.c

> +++ b/platform/linux-generic/odp_init.c

> @@ -8,13 +8,18 @@

>  #include <odp_internal.h>

>  #include <odp_debug.h>

>

> -

> -int odp_init_global(void)

> +int odp_init_global(odp_global_init_t *params  ODP_UNUSED,

> +                 odp_global_platform_init_t *platform_params ODP_UNUSED)

>  {

>       odp_thread_init_global();

>

>       odp_system_info_init();

>

> +     if (odp_init_platform(platform_params)) {

> platform_params is used here, so remove ODP_UNUSED from argument list.


>> On the other hand, it would be better to remove odp_init_platform()

>> altogether since does not do anything. It's better to add it when actually used.

>> The order of init calls is very important, e.g. now you could not allocate shared

>> memory in odp_init_platform() because it's executed before shm init...

> odp_init_platform() does do something for DPDK / KS2 and we need a place holder because

> we want this to be shared between implementations don't we ?

> This is the stuff that must happen before any ODP init starts to satisfy the platform SDK


Internals of odp_init_global() is totally platform independent - it cannot be shared in general. It's HW/OS/ODP implementation dependent what needs to be initialized in global init, and in which order (e.g. HW spinlocks before shm, shm before barriers, etc.).

So, if odp_init_platform() is empty for linux-generic, it's better to leave it out. Also it may not be even one call, the platform_params maybe passed all over the init calls.


-Petri

Wiles, Roger Keith Sept. 1, 2014, 8:26 a.m. UTC | #12

On Sep 1, 2014, at 3:07 AM, Savolainen, Petri (NSN - FI/Espoo) <petri.savolainen@nsn.com> wrote:

>> diff --git a/platform/linux-generic/odp_init.c b/platform/linux-
>> generic/odp_init.c
>> index 5b7e192..f595def 100644
>> --- a/platform/linux-generic/odp_init.c
>> +++ b/platform/linux-generic/odp_init.c
>> @@ -8,13 +8,18 @@
>>   #include <odp_internal.h>
>>   #include <odp_debug.h>
>> 
>> -
>> -int odp_init_global(void)
>> +int odp_init_global(odp_global_init_t *params  ODP_UNUSED,
>> +                 odp_global_platform_init_t *platform_params ODP_UNUSED)
>>   {
>>        odp_thread_init_global();
>> 
>>        odp_system_info_init();
>> 
>> +     if (odp_init_platform(platform_params)) {
>> platform_params is used here, so remove ODP_UNUSED from argument list.
> 
>>> On the other hand, it would be better to remove odp_init_platform()
>>> altogether since does not do anything. It's better to add it when actually used.
>>> The order of init calls is very important, e.g. now you could not allocate shared
>>> memory in odp_init_platform() because it's executed before shm init...
>>  odp_init_platform() does do something for DPDK / KS2 and we need a place holder because
>> we want this to be shared between implementations don't we ?
>> This is the stuff that must happen before any ODP init starts to satisfy the platform SDK
> 
> Internals of odp_init_global() is totally platform independent - it cannot be shared in general. It's HW/OS/ODP implementation dependent what needs to be initialized in global init, and in which order (e.g. HW spinlocks before shm, shm before barriers, etc.).

All of the platform dependent inits should be in the odp_init_platform() and the calls from odp_init() should be generic for all Linux platforms. The odp_init_platform() should be close to the bottom of the odp_init() routine as no routine called before the platform init should require any thing from the platform.

If someone can point out why odp_init() is not generic with specific examples that would help as I do not see any of the above examples as being platform specific. Why would sum, spin locks and barriers (which are linux features) need to be in the platform.
> 
> So, if odp_init_platform() is empty for linux-generic, it's better to leave it out. Also it may not be even one call, the platform_params maybe passed all over the init calls.

We need the place holder for the function even if it is empty, besides it does not hurt anything to have the function. It maybe we need to add something to the linux-generic at some point.
> 
> 
> -Petri
> 
> 
> _______________________________________________
> lng-odp mailing list
> lng-odp@lists.linaro.org
> http://lists.linaro.org/mailman/listinfo/lng-odp

Keith Wiles, Principal Technologist with CTO office, Wind River mobile 972-213-5533

Taras Kondratiuk Sept. 1, 2014, 8:54 a.m. UTC | #13

On 08/29/2014 08:05 PM, Mike Holmes wrote:
>      > diff --git a/platform/linux-dpdk/odp_init.c b/platform/linux-
>      > dpdk/odp_platform.c
>      > similarity index 50%
>      > rename from platform/linux-dpdk/odp_init.c
>      > rename to platform/linux-dpdk/odp_platform.c
>      > index ecc2066..5a054fd 100644
>      > --- a/platform/linux-dpdk/odp_init.c
>      > +++ b/platform/linux-dpdk/odp_platform.c
>      > @@ -1,4 +1,4 @@
>      > -/* Copyright (c) 2013, Linaro Limited
>      > +/* Copyright (c) 2014, Linaro Limited
>      >   * All rights reserved.
>      >   *
>      >   * SPDX-License-Identifier:     BSD-3-Clause
>      > @@ -9,7 +9,7 @@
>      >  #include <odp_debug.h>
>      >  #include <odp_packet_dpdk.h>
>      >
>      > -int odp_init_dpdk(void)
>      > +int odp_init_platform(odp_global_platform_init_t *platform_params
>      > ODP_UNUSED)
>      >  {
>      >       int test_argc = 5;
>      >       char *test_argv[6];
>      > @@ -50,64 +50,7 @@ int odp_init_dpdk(void)
>      >       return 0;
>      >  }
>      >
>      > -int odp_init_global(void)
>      > +int odp_crypto_init_global(void)
>
>     Is crypto init now in odp_platform.c? It should be in odp_crypto.c
>     or similar...
>
> no problem
> I can make a dummy   odp_crypto.c for ks2 and point DPDK at the linux
> generic. I was trying to avoid
> connecting internals.

No need to make a dummy file. Just point to linux-generic.
I'll replace it with accelerated implementation.

Taras Kondratiuk Sept. 1, 2014, 9:40 a.m. UTC | #14

On 09/01/2014 11:26 AM, Wiles, Roger Keith wrote:
>
> On Sep 1, 2014, at 3:07 AM, Savolainen, Petri (NSN - FI/Espoo) <petri.savolainen@nsn.com> wrote:
>
>>> diff --git a/platform/linux-generic/odp_init.c b/platform/linux-
>>> generic/odp_init.c
>>> index 5b7e192..f595def 100644
>>> --- a/platform/linux-generic/odp_init.c
>>> +++ b/platform/linux-generic/odp_init.c
>>> @@ -8,13 +8,18 @@
>>>    #include <odp_internal.h>
>>>    #include <odp_debug.h>
>>>
>>> -
>>> -int odp_init_global(void)
>>> +int odp_init_global(odp_global_init_t *params  ODP_UNUSED,
>>> +                 odp_global_platform_init_t *platform_params ODP_UNUSED)
>>>    {
>>>         odp_thread_init_global();
>>>
>>>         odp_system_info_init();
>>>
>>> +     if (odp_init_platform(platform_params)) {
>>> platform_params is used here, so remove ODP_UNUSED from argument list.
>>
>>>> On the other hand, it would be better to remove odp_init_platform()
>>>> altogether since does not do anything. It's better to add it when actually used.
>>>> The order of init calls is very important, e.g. now you could not allocate shared
>>>> memory in odp_init_platform() because it's executed before shm init...
>>>   odp_init_platform() does do something for DPDK / KS2 and we need a place holder because
>>> we want this to be shared between implementations don't we ?
>>> This is the stuff that must happen before any ODP init starts to satisfy the platform SDK
>>
>> Internals of odp_init_global() is totally platform independent - it cannot be shared in general. It's HW/OS/ODP implementation dependent what needs to be initialized in global init, and in which order (e.g. HW spinlocks before shm, shm before barriers, etc.).
>
> All of the platform dependent inits should be in the odp_init_platform() and the calls from odp_init() should be generic for all Linux platforms. The odp_init_platform() should be close to the bottom of the odp_init() routine as no routine called before the platform init should require any thing from the platform.

Assumption that all init functions don't need platform to be
initialized is not true for a current code. Sure it can be changed in a
way you assume, but what's the profit to do so? Why to constrain
implementation?

> If someone can point out why odp_init() is not generic with specific examples that would help as I do not see any of the above examples as being platform specific. Why would sum, spin locks and barriers (which are linux features) need to be in the platform.
>>
>> So, if odp_init_platform() is empty for linux-generic, it's better to leave it out. Also it may not be even one call, the platform_params maybe passed all over the init calls.
>
> We need the place holder for the function even if it is empty, besides it does not hurt anything to have the function. It maybe we need to add something to the linux-generic at some point.
>>
>>
>> -Petri
>>
>>
>> _______________________________________________
>> lng-odp mailing list
>> lng-odp@lists.linaro.org
>> http://lists.linaro.org/mailman/listinfo/lng-odp
>
> Keith Wiles, Principal Technologist with CTO office, Wind River mobile 972-213-5533
>
>
> _______________________________________________
> lng-odp mailing list
> lng-odp@lists.linaro.org
> http://lists.linaro.org/mailman/listinfo/lng-odp
>

Savolainen, Petri (NSN - FI/Espoo) Sept. 1, 2014, 10:01 a.m. UTC | #15

> -----Original Message-----
> From: ext Wiles, Roger Keith [mailto:keith.wiles@windriver.com]
> Sent: Monday, September 01, 2014 11:27 AM
> To: Savolainen, Petri (NSN - FI/Espoo)
> Cc: ext Mike Holmes; lng-odp@lists.linaro.org
> Subject: Re: [lng-odp] [PATCH v4] Add-global_init-paramiters
> 
> 
> On Sep 1, 2014, at 3:07 AM, Savolainen, Petri (NSN - FI/Espoo)
> <petri.savolainen@nsn.com> wrote:
> 
> >> diff --git a/platform/linux-generic/odp_init.c b/platform/linux-
> >> generic/odp_init.c
> >> index 5b7e192..f595def 100644
> >> --- a/platform/linux-generic/odp_init.c
> >> +++ b/platform/linux-generic/odp_init.c
> >> @@ -8,13 +8,18 @@
> >>   #include <odp_internal.h>
> >>   #include <odp_debug.h>
> >>
> >> -
> >> -int odp_init_global(void)
> >> +int odp_init_global(odp_global_init_t *params  ODP_UNUSED,
> >> +                 odp_global_platform_init_t *platform_params
> ODP_UNUSED)
> >>   {
> >>        odp_thread_init_global();
> >>
> >>        odp_system_info_init();
> >>
> >> +     if (odp_init_platform(platform_params)) {
> >> platform_params is used here, so remove ODP_UNUSED from argument list.
> >
> >>> On the other hand, it would be better to remove odp_init_platform()
> >>> altogether since does not do anything. It's better to add it when
> actually used.
> >>> The order of init calls is very important, e.g. now you could not
> allocate shared
> >>> memory in odp_init_platform() because it's executed before shm init...
> >>  odp_init_platform() does do something for DPDK / KS2 and we need a
> place holder because
> >> we want this to be shared between implementations don't we ?
> >> This is the stuff that must happen before any ODP init starts to
> satisfy the platform SDK
> >
> > Internals of odp_init_global() is totally platform independent - it
> cannot be shared in general. It's HW/OS/ODP implementation dependent what
> needs to be initialized in global init, and in which order (e.g. HW
> spinlocks before shm, shm before barriers, etc.).
> 
> All of the platform dependent inits should be in the odp_init_platform()
> and the calls from odp_init() should be generic for all Linux platforms.
> The odp_init_platform() should be close to the bottom of the odp_init()
> routine as no routine called before the platform init should require any
> thing from the platform.
> 
> If someone can point out why odp_init() is not generic with specific
> examples that would help as I do not see any of the above examples as
> being platform specific. Why would sum, spin locks and barriers (which are
> linux features) need to be in the platform.

The init order depends on implementation internal dependencies. E.g. if a queue implementation uses pools, pools have to be initialized before queues (and pools cannot use queues for their implementation) -  BUT if a pool implementation uses queues, queues have to be init before pools, etc. There's no point to standardize implementation internals, here or elsewhere.

Although an implementation may run on Linux, e.g a spinlock may be implement with SW, HW queues or more specific HW...


-Petri

Bill Fischofer Sept. 1, 2014, 8:55 p.m. UTC | #16

Let's discuss these scenarios during tomorrow's ODP calls.  Could the
various camps put together a brief summary of how they see this organized?
 That would be helpful in framing the discussion.

Thanks.

Bill


On Mon, Sep 1, 2014 at 5:01 AM, Savolainen, Petri (NSN - FI/Espoo) <
petri.savolainen@nsn.com> wrote:

>
>
> > -----Original Message-----
> > From: ext Wiles, Roger Keith [mailto:keith.wiles@windriver.com]
> > Sent: Monday, September 01, 2014 11:27 AM
> > To: Savolainen, Petri (NSN - FI/Espoo)
> > Cc: ext Mike Holmes; lng-odp@lists.linaro.org
> > Subject: Re: [lng-odp] [PATCH v4] Add-global_init-paramiters
> >
> >
> > On Sep 1, 2014, at 3:07 AM, Savolainen, Petri (NSN - FI/Espoo)
> > <petri.savolainen@nsn.com> wrote:
> >
> > >> diff --git a/platform/linux-generic/odp_init.c b/platform/linux-
> > >> generic/odp_init.c
> > >> index 5b7e192..f595def 100644
> > >> --- a/platform/linux-generic/odp_init.c
> > >> +++ b/platform/linux-generic/odp_init.c
> > >> @@ -8,13 +8,18 @@
> > >>   #include <odp_internal.h>
> > >>   #include <odp_debug.h>
> > >>
> > >> -
> > >> -int odp_init_global(void)
> > >> +int odp_init_global(odp_global_init_t *params  ODP_UNUSED,
> > >> +                 odp_global_platform_init_t *platform_params
> > ODP_UNUSED)
> > >>   {
> > >>        odp_thread_init_global();
> > >>
> > >>        odp_system_info_init();
> > >>
> > >> +     if (odp_init_platform(platform_params)) {
> > >> platform_params is used here, so remove ODP_UNUSED from argument list.
> > >
> > >>> On the other hand, it would be better to remove odp_init_platform()
> > >>> altogether since does not do anything. It's better to add it when
> > actually used.
> > >>> The order of init calls is very important, e.g. now you could not
> > allocate shared
> > >>> memory in odp_init_platform() because it's executed before shm
> init...
> > >>  odp_init_platform() does do something for DPDK / KS2 and we need a
> > place holder because
> > >> we want this to be shared between implementations don't we ?
> > >> This is the stuff that must happen before any ODP init starts to
> > satisfy the platform SDK
> > >
> > > Internals of odp_init_global() is totally platform independent - it
> > cannot be shared in general. It's HW/OS/ODP implementation dependent what
> > needs to be initialized in global init, and in which order (e.g. HW
> > spinlocks before shm, shm before barriers, etc.).
> >
> > All of the platform dependent inits should be in the odp_init_platform()
> > and the calls from odp_init() should be generic for all Linux platforms.
> > The odp_init_platform() should be close to the bottom of the odp_init()
> > routine as no routine called before the platform init should require any
> > thing from the platform.
> >
> > If someone can point out why odp_init() is not generic with specific
> > examples that would help as I do not see any of the above examples as
> > being platform specific. Why would sum, spin locks and barriers (which
> are
> > linux features) need to be in the platform.
>
> The init order depends on implementation internal dependencies. E.g. if a
> queue implementation uses pools, pools have to be initialized before queues
> (and pools cannot use queues for their implementation) -  BUT if a pool
> implementation uses queues, queues have to be init before pools, etc.
> There's no point to standardize implementation internals, here or elsewhere.
>
> Although an implementation may run on Linux, e.g a spinlock may be
> implement with SW, HW queues or more specific HW...
>
>
> -Petri
>
>
> _______________________________________________
> lng-odp mailing list
> lng-odp@lists.linaro.org
> http://lists.linaro.org/mailman/listinfo/lng-odp
>

Santosh Shukla Sept. 2, 2014, 2:08 p.m. UTC | #17

Mike,
I am finding it difficult to use this patch for my use-case where
l2fwd passing  "coremask" param to linux-dpdk layer.  My
implementation takes coremask value from "l2fwd command line argument
-c" and processing of command line argument happens after
odp_global_init().

odp_global_init is first api to call in any application and it does
essential service init like shared mem, its lock etc.. thereafter
argument processing happens so which mean passing
odp_xxx_platform_init_t from odp_global_init wont really help..
What do you thin?

Thanks.

On 2 September 2014 02:25, Bill Fischofer <bill.fischofer@linaro.org> wrote:
> Let's discuss these scenarios during tomorrow's ODP calls.  Could the
> various camps put together a brief summary of how they see this organized?
> That would be helpful in framing the discussion.
>
> Thanks.
>
> Bill
>
>
> On Mon, Sep 1, 2014 at 5:01 AM, Savolainen, Petri (NSN - FI/Espoo)
> <petri.savolainen@nsn.com> wrote:
>>
>>
>>
>> > -----Original Message-----
>> > From: ext Wiles, Roger Keith [mailto:keith.wiles@windriver.com]
>> > Sent: Monday, September 01, 2014 11:27 AM
>> > To: Savolainen, Petri (NSN - FI/Espoo)
>> > Cc: ext Mike Holmes; lng-odp@lists.linaro.org
>> > Subject: Re: [lng-odp] [PATCH v4] Add-global_init-paramiters
>> >
>> >
>> > On Sep 1, 2014, at 3:07 AM, Savolainen, Petri (NSN - FI/Espoo)
>> > <petri.savolainen@nsn.com> wrote:
>> >
>> > >> diff --git a/platform/linux-generic/odp_init.c b/platform/linux-
>> > >> generic/odp_init.c
>> > >> index 5b7e192..f595def 100644
>> > >> --- a/platform/linux-generic/odp_init.c
>> > >> +++ b/platform/linux-generic/odp_init.c
>> > >> @@ -8,13 +8,18 @@
>> > >>   #include <odp_internal.h>
>> > >>   #include <odp_debug.h>
>> > >>
>> > >> -
>> > >> -int odp_init_global(void)
>> > >> +int odp_init_global(odp_global_init_t *params  ODP_UNUSED,
>> > >> +                 odp_global_platform_init_t *platform_params
>> > ODP_UNUSED)
>> > >>   {
>> > >>        odp_thread_init_global();
>> > >>
>> > >>        odp_system_info_init();
>> > >>
>> > >> +     if (odp_init_platform(platform_params)) {
>> > >> platform_params is used here, so remove ODP_UNUSED from argument
>> > >> list.
>> > >
>> > >>> On the other hand, it would be better to remove odp_init_platform()
>> > >>> altogether since does not do anything. It's better to add it when
>> > actually used.
>> > >>> The order of init calls is very important, e.g. now you could not
>> > allocate shared
>> > >>> memory in odp_init_platform() because it's executed before shm
>> > >>> init...
>> > >>  odp_init_platform() does do something for DPDK / KS2 and we need a
>> > place holder because
>> > >> we want this to be shared between implementations don't we ?
>> > >> This is the stuff that must happen before any ODP init starts to
>> > satisfy the platform SDK
>> > >
>> > > Internals of odp_init_global() is totally platform independent - it
>> > cannot be shared in general. It's HW/OS/ODP implementation dependent
>> > what
>> > needs to be initialized in global init, and in which order (e.g. HW
>> > spinlocks before shm, shm before barriers, etc.).
>> >
>> > All of the platform dependent inits should be in the odp_init_platform()
>> > and the calls from odp_init() should be generic for all Linux platforms.
>> > The odp_init_platform() should be close to the bottom of the odp_init()
>> > routine as no routine called before the platform init should require any
>> > thing from the platform.
>> >
>> > If someone can point out why odp_init() is not generic with specific
>> > examples that would help as I do not see any of the above examples as
>> > being platform specific. Why would sum, spin locks and barriers (which
>> > are
>> > linux features) need to be in the platform.
>>
>> The init order depends on implementation internal dependencies. E.g. if a
>> queue implementation uses pools, pools have to be initialized before queues
>> (and pools cannot use queues for their implementation) -  BUT if a pool
>> implementation uses queues, queues have to be init before pools, etc.
>> There's no point to standardize implementation internals, here or elsewhere.
>>
>> Although an implementation may run on Linux, e.g a spinlock may be
>> implement with SW, HW queues or more specific HW...
>>
>>
>> -Petri
>>
>>
>> _______________________________________________
>> lng-odp mailing list
>> lng-odp@lists.linaro.org
>> http://lists.linaro.org/mailman/listinfo/lng-odp
>
>
>
> _______________________________________________
> lng-odp mailing list
> lng-odp@lists.linaro.org
> http://lists.linaro.org/mailman/listinfo/lng-odp
>

[v4] Add-global_init-paramiters

Commit Message

Comments

Patch