| Message ID | 1409069025-60872-1-git-send-email-mike.holmes@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
On Tue, Aug 26, 2014 at 05:03:45PM +0100, Mike Holmes wrote: > 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 > > 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 | 26 ++++++-- > 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, 68 insertions(+), 138 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/platform/linux-generic/include/api/odp_init.h b/platform/linux-generic/include/api/odp_init.h > index 490324a..4acaea4 100644 > --- a/platform/linux-generic/include/api/odp_init.h > +++ b/platform/linux-generic/include/api/odp_init.h > @@ -23,23 +23,41 @@ extern "C" { > #include <odp_std_types.h> > > > +/** > + * ODP initialisation > + * Data that is required to initialize the ODP > + * application, such as specifying a logging callback, the log level etc. > + */ Line wrapping is odd. Isn't this initialising ODP with application specific data, rather than initialising the application? > +typedef struct odp_global_init { > +} odp_global_init_t; > > +/** > + * ODP platform initialization > + * Platform specific data. ODP does nothing with > + * this data. The data will be used by ODP, it'll just be a platform specific part of ODP. I am thinking from an application perspective where ODP is one blob of functionality rather than some layer in between the application and an SDK. It's worth noting somewhere, probably here, that use of this structure is not portable and what an application should do to cope with that. Is the application expected to just know which platform it's running on? or do we need an API that returns a platform identifier? > + */ > +typedef struct odp_global_platform_init { > +} odp_global_platform_init_t; > > /** > - * Perform global ODP initalisation. > + * 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 ODP > + * @param[in] platform_params Those parameters that are passed > + * without interpretation to the implementation. Isn't the implementation itself part of ODP? Also I think it's better to stick to using the term platform rather than mixing the terms platform and implementation (they mean the same thing in this context.. right?) How about.. params ODP initialisation parameters, common across all platforms platform_params Platform specific initialisation parameters
On 08/26/2014 07:03 PM, Mike Holmes wrote: > 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. Hi Mike I doubt it is worth to reuse odp_init_global() from linux-generic. Initialization won't always follow init steps in the same order as linux-generic. Also you will have to reuse odp_init_local() and add all steps there, because you don't know which part of initialization need to be called for each thread.
On 08/27/2014 03:16 PM, Taras Kondratiuk wrote: > On 08/26/2014 07:03 PM, Mike Holmes wrote: >> 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. > > Hi Mike > > I doubt it is worth to reuse odp_init_global() from linux-generic. > Initialization won't always follow init steps in the same order as > linux-generic. Also you will have to reuse odp_init_local() and add all > steps there, because you don't know which part of initialization need > to be called for each thread. By making a generic odp_init_local() KS2 specific one is removed, so ti_em_rh_init_local() is not called any more.
I think the consensus was to fix up Stuarts comments and apply this, bearing in mind that Taras will have to completely replace global_init for his platform, and that may be true for others so only dpdk will benefit from the refactoring for now. The goal for apply this patch as is being to allow DPDK folks to get their parameters passed correctly via global init. On 27 August 2014 10:39, Taras Kondratiuk <taras.kondratiuk@linaro.org> wrote: > On 08/27/2014 03:16 PM, Taras Kondratiuk wrote: > >> On 08/26/2014 07:03 PM, Mike Holmes wrote: >> >>> 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. >>> >> >> Hi Mike >> >> I doubt it is worth to reuse odp_init_global() from linux-generic. >> Initialization won't always follow init steps in the same order as >> linux-generic. Also you will have to reuse odp_init_local() and add all >> steps there, because you don't know which part of initialization need >> to be called for each thread. >> > > By making a generic odp_init_local() KS2 specific one is removed, so > ti_em_rh_init_local() is not called any more. >
On Aug 28, 2014, at 12:10 PM, Mike Holmes <mike.holmes@linaro.org> wrote: > I think the consensus was to fix up Stuarts comments and apply this, bearing in mind that Taras will have to completely replace global_init for his platform, and that may be true for others so only dpdk will benefit from the refactoring for now. > After the patch is applied and Taras has updated his odp_init_global() routine I would like to understand why Taras needed to rewrite his odp_init_global() if possible. I would like to at least understand why we can not have a common odp_init_global() routine for all platforms. > The goal for apply this patch as is being to allow DPDK folks to get their parameters passed correctly via global init. > > > On 27 August 2014 10:39, Taras Kondratiuk <taras.kondratiuk@linaro.org> wrote: > On 08/27/2014 03:16 PM, Taras Kondratiuk wrote: > On 08/26/2014 07:03 PM, Mike Holmes wrote: > 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. > > Hi Mike > > I doubt it is worth to reuse odp_init_global() from linux-generic. > Initialization won't always follow init steps in the same order as > linux-generic. Also you will have to reuse odp_init_local() and add all > steps there, because you don't know which part of initialization need > to be called for each thread. > > By making a generic odp_init_local() KS2 specific one is removed, so ti_em_rh_init_local() is not called any more. > > > > -- > 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
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 dbe0e5b..1af8c2b 100644 --- a/example/timer/odp_timer_test.c +++ b/example/timer/odp_timer_test.c @@ -210,7 +210,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) { - 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..4acaea4 100644 --- a/platform/linux-generic/include/api/odp_init.h +++ b/platform/linux-generic/include/api/odp_init.h @@ -23,23 +23,41 @@ extern "C" { #include <odp_std_types.h> +/** + * ODP initialisation + * Data that is required to initialize the ODP + * application, such as specifying a logging callback, the log level etc. + */ +typedef struct odp_global_init { +} odp_global_init_t; +/** + * ODP platform initialization + * Platform specific data. ODP does nothing with + * this data. + */ +typedef struct odp_global_platform_init { +} odp_global_platform_init_t; /** - * Perform global ODP initalisation. + * 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 ODP + * @param[in] platform_params Those parameters that are passed + * without interpretation to the implementation. * @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 + * * 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)) { + 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; +} diff --git a/platform/linux-keystone2/Makefile.am b/platform/linux-keystone2/Makefile.am index 8e5fbb3..6e3cc00 100644 --- a/platform/linux-keystone2/Makefile.am +++ b/platform/linux-keystone2/Makefile.am @@ -70,12 +70,13 @@ __LIB__libodp_la_SOURCES = \ odp_buffer.c \ odp_buffer_pool.c \ ../linux-generic/odp_coremask.c \ - odp_init.c \ + ../linux-generic/odp_init.c \ ../linux-generic/odp_linux.c \ odp_packet.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-keystone2/odp_init.c b/platform/linux-keystone2/odp_platform.c similarity index 72% rename from platform/linux-keystone2/odp_init.c rename to platform/linux-keystone2/odp_platform.c index f832551..4659b8a 100644 --- a/platform/linux-keystone2/odp_init.c +++ b/platform/linux-keystone2/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 @@ -15,17 +15,19 @@ #include <odp_packet_internal.h> /* - * Make region_configs[] global, because hw_config is saved in - * ti_em_rh_init_global() and it references region_configs[]. - */ + * * Make region_configs[] global, because hw_config is saved in + * * ti_em_rh_init_global() and it references region_configs[]. + * */ static ti_em_osal_hw_region_config_t region_configs[TI_ODP_REGION_NUM]; -static int ti_init_hw_config(void) +int odp_init_platform(odp_global_platform_init_t *platform_params ODP_UNUSED) { ti_em_rh_hw_config_t hw_config; ti_em_osal_hw_region_config_t *reg_config; memset(&hw_config, 0, sizeof(ti_em_rh_hw_config_t)); + ti_em_osal_core_init_global(); + /* Set ODP initialization parameters */ hw_config.private_free_queue_idx = MY_EM_PRIVATE_FREE_QUEUE_IDX; hw_config.hw_queue_base_idx = MY_EM_SCHED_QUEUE_IDX; @@ -89,65 +91,7 @@ static int ti_init_hw_config(void) return 0; } - -int odp_init_global(void) +int odp_crypto_init_global(void) { - odp_thread_init_global(); - - odp_system_info_init(); - - ti_em_osal_core_init_global(); - ti_init_hw_config(); - - 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); - - ti_em_rh_init_local(); - - 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/test/api_test/odp_common.c b/test/api_test/odp_common.c index 89ebd2d..fc583c0 100644 --- a/test/api_test/odp_common.c +++ b/test/api_test/odp_common.c @@ -56,7 +56,7 @@ int odp_test_global_init(void) { memset(thread_tbl, 0, sizeof(thread_tbl)); - if (odp_init_global()) { + if (odp_init_global(NULL, NULL)) { ODP_ERR("ODP global init failed.\n"); return -1; }
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 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 | 26 ++++++-- 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, 68 insertions(+), 138 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%)