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]>
wrote:
>
>
>
> On 29 August 2014 03:59, Savolainen, Petri (NSN - FI/Espoo) <
> [email protected]> wrote:
>
>>
>>
>> > -----Original Message-----
>> > From: [email protected] [mailto:lng-odp-
>> > [email protected]] On Behalf Of ext Mike Holmes
>> > Sent: Thursday, August 28, 2014 9:26 PM
>> > To: [email protected]
>> > Subject: [lng-odp] [PATCH v4] Add-global_init-paramiters
>> >
>> > Signed-off-by: Mike Holmes <[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]
> http://lists.linaro.org/mailman/listinfo/lng-odp
>
>
_______________________________________________
lng-odp mailing list
[email protected]
http://lists.linaro.org/mailman/listinfo/lng-odp
