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 <[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]>
> 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
>>
>>
>
--
*Mike Holmes*
Linaro Technical Manager / Lead
LNG - ODP
_______________________________________________
lng-odp mailing list
[email protected]
http://lists.linaro.org/mailman/listinfo/lng-odp
