On 4 September 2014 10:18, Santosh Shukla <[email protected]> wrote:
> On 3 September 2014 15:48, 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: Tuesday, September 02, 2014 9:06 PM
>>> To: [email protected]
>>> Subject: [lng-odp] [PATCH v5] 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.
>>>
>>> v5:
>>> rename typedef struct names
>>>
>>> This touches all platforms to get crypto working with generic init, DPDK
>>> should
>>> be using generic crypto but obviously KS2 will replace this.
>>> A platform may replace global init entirely but this patch starts out
>>> assuming
>>> that the sub module inits can be called from a central global_init as a
>>> starting
>>> point.
>>>
>>> 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 | 4 +-
>>> platform/linux-dpdk/{odp_init.c => odp_platform.c} | 66 +----------------
>>> --
>>> platform/linux-generic/Makefile.am | 1 +
>>> platform/linux-generic/include/api/odp.h | 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 | 4 +-
>>> .../linux-keystone2/{odp_init.c => odp_platform.c} | 74 ++---------------
>>> -----
>>> test/api_test/odp_common.c | 2 +-
>>> 16 files changed, 73 insertions(+), 149 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..48fc9d7 100644
>>> --- a/platform/linux-dpdk/Makefile.am
>>> +++ b/platform/linux-dpdk/Makefile.am
>>> @@ -67,13 +67,15 @@ __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 \
>>> + ../linux-generic/odp_crypto.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..3162c05 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];
>>> @@ -49,65 +49,3 @@ int odp_init_dpdk(void)
>>>
>>> return 0;
>>> }
>>> -
>>> -int odp_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.h b/platform/linux-
>>> generic/include/api/odp.h
>>> index 0ee3faf..99e2ae0 100644
>>> --- a/platform/linux-generic/include/api/odp.h
>>> +++ b/platform/linux-generic/include/api/odp.h
>>> @@ -48,6 +48,7 @@ extern "C" {
>>> #include <odp_packet.h>
>>> #include <odp_packet_flags.h>
>>> #include <odp_packet_io.h>
>>> +#include <odp_crypto.h>
>>>
>>> #ifdef __cplusplus
>>> }
>>> diff --git a/platform/linux-generic/include/api/odp_init.h
>>> b/platform/linux-generic/include/api/odp_init.h
>>> index 490324a..8cdce46 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_t {
>>> +} odp_global_init_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_t {
>>> +} odp_global_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
>>> + * @param[in] platform_params Those parameters that are passed without
>>> + * interpretation by the ODP API 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
>>> + *
>>
>>
>> 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 -> remove ODP_UNUSED
>>
>> ...OR actually you can remove odp_init_platform() since it's doing nothing.
>> For the same reason,
>> there are no calls to e.g. odp_barrier_init(), odp_coremask_init(),
>> odp_spinlock_init() either - those do not need global initialization (in
>> linux-generic implementation). Also if some linux-generic platform init data
>> would be added to odp_global_platform_init_t, it would be consumed by
>> individual init functions e.g. odp_schedule_init_global(platform_params),
>> etc.
>
> (I thought, I sent this reply to list, But it went only to petri (: - )
>
> Petri, I agree what you pointed out.
>
> However, By separating/removing odp_init_platform () from
> odp_global_init() call will lead to segfault for other platform
> specific resource init.
>
> For example, I tried removing odp_platfrom_init() and call separately
> from application layer, got this
>
> (gdb) bt
> #0 0x0000000000443e3a in memzone_reserve_aligned_thread_unsafe ()
> #1 0x0000000000444250 in rte_memzone_reserve_aligned ()
> #2 0x000000000044ceff in rte_ring_create ()
> #3 0x000000000042986e in rte_mempool_xmem_create ()
> #4 0x0000000000429c8b in rte_mempool_create ()
> #5 0x0000000000403963 in odp_buffer_pool_create
> (name=name@entry=0x46bba2 "odp_sched_pool", base_addr=<optimized out>,
> size=size@entry=262144,
> buf_size=buf_size@entry=4, buf_align=buf_align@entry=64,
> buf_type=buf_type@entry=1) at odp_buffer_pool.c:113
> #6 0x0000000000406931 in odp_schedule_init_global () at
> ../linux-generic/odp_schedule.c:103
> #7 0x000000000040445b in odp_init_global (params=params@entry=0x0,
> platform_params=platform_params@entry=0x0) at
> ../linux-generic/odp_init.c:38
> #8 0x0000000000402940 in main (argc=7, argv=0x7fffffffe338) at
> odp_l2fwd.c:341
>
>
> My concern :
>
> - odp_global_init (arg1, agr2), agr2 : is platform centric, Most
> likely to receive data from cmdline. cmdline argument processing
> happens after odp resource init, So in that case arg2 is redundant.
> IIUC, intent of this patch to provide method by which application can
> send command from application layer to
> odp-platform-centric-generric-layer. I believe this patch NOT
> addressing that requirement.
Conti..
Root cause of my concern is not entirely specific to this patch. Its
more related to odp api initialization calling sequence in
application. From early days, we used to follow below step in
application -
odp_global_init() -> which does almost everything for platform initialization.
odp_shm_reserve() -> used for argument processing then
odp_create_thread etc...
Now we found a need to introduce platform specific init hints, coming
from application. Therefore order of odp_xxx_init api calling sequence
should change too.
Something like.
Application "X"
main ()
{
- First do argument processing. For that, call odp_shm_init() in
application and remove it from odp_global_init OR application has to
allocate memory(bad idea ... I guess).
- Then call odp_global/platform_init(argX, argY); where arg -X :
global param, Y : platform centric stuff.
... Rest stuff as it is.
}
Any idea? thoughts? Does above make sense. Thanks.
>>
>> This is the platform specific init ... there's not need for
>> odp_init_platform().
>>
>> -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;
>>> +}
>>> http://lists.linaro.org/mailman/listinfo/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
