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 <[email protected]
<mailto:[email protected]>> 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
<[email protected] <mailto:[email protected]>> wrote:
On 29 August 2014 03:59, Savolainen, Petri (NSN - FI/Espoo)
<[email protected] <mailto:[email protected]>> wrote:
> -----Original Message-----
> From: [email protected]
<mailto:[email protected]> [mailto:lng-odp-
<mailto:lng-odp->
> [email protected]
<mailto:[email protected]>] On Behalf Of ext Mike Holmes
> Sent: Thursday, August 28, 2014 9:26 PM
> To: [email protected]
<mailto:[email protected]>
> Subject: [lng-odp] [PATCH v4] Add-global_init-paramiters
>
> Signed-off-by: Mike Holmes <[email protected]
<mailto:[email protected]>>
> ---
>
> 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
[email protected] <mailto:[email protected]>
http://lists.linaro.org/mailman/listinfo/lng-odp
--
*Mike Holmes*
Linaro Technical Manager / Lead
LNG - ODP
_______________________________________________
lng-odp mailing list
[email protected]
http://lists.linaro.org/mailman/listinfo/lng-odp
To unsubscribe from this group and stop receiving emails from it, send an email
to [email protected].
_______________________________________________
lng-odp mailing list
[email protected]
http://lists.linaro.org/mailman/listinfo/lng-odp
