Comments inline. Keith Wiles, Principal Technologist with CTO office, Wind River mobile 972-213-5533
On Aug 14, 2014, at 2:11 PM, Mike Holmes <[email protected]<mailto:[email protected]>> wrote: On 14 August 2014 14:49, Victor Kamensky <[email protected]<mailto:[email protected]>> wrote: Mike, Please see inline On 13 August 2014 03:13, Mike Holmes <[email protected]<mailto:[email protected]>> wrote: > Signed-off-by: Mike Holmes > <[email protected]<mailto:[email protected]>> > --- > > This adds the two agreed upon init structure members, I left flags out but > the understanding is we add it along with its first agreed use case. > I wondered though if we need an init struct initializer function that > platforms can implement to set the defaults as they should be ? > Next steps would be to incorporate the logging callback and any debug level > control, presumably we will have another run time API that allows further > manipulation of the debug level, this would just be the initial state of that. > > > example/generator/odp_generator.c | 3 ++- > example/l2fwd/odp_l2fwd.c | 3 ++- > example/odp_example/odp_example.c | 3 ++- > example/packet/odp_pktio.c | 3 ++- > example/timer/odp_timer_test.c | 3 ++- > include/odp_init.h | 22 +++++++++++++++++----- > platform/linux-dpdk/odp_init.c | 2 +- > platform/linux-generic/odp_init.c | 2 +- > platform/linux-keystone2/odp_init.c | 2 +- > test/api_test/odp_common.c | 3 ++- > 10 files changed, 32 insertions(+), 14 deletions(-) > > diff --git a/example/generator/odp_generator.c > b/example/generator/odp_generator.c > index b10372e..893233f 100644 > --- a/example/generator/odp_generator.c > +++ b/example/generator/odp_generator.c > @@ -522,6 +522,7 @@ int main(int argc, char *argv[]) > { > odp_linux_pthread_t thread_tbl[MAX_WORKERS]; > odp_buffer_pool_t pool; > + odp_global_init_t init_data; > int thr_id; > int num_workers; > void *pool_base; > @@ -530,7 +531,7 @@ int main(int argc, char *argv[]) > int core_count; > > /* Init ODP before calling anything else */ > - if (odp_init_global()) { > + if (odp_init_global(init_data)) { Because init_data could be on the stack or allocated/freed memory by the developer it is assume odp_global_init() will extract out any data it needs to keep, correct? If we agree then add that comment to the function header. Normally structure is not passed by value, it could be big, so it should be passed by pointer: if (odp_init_global(&init_data)) { IMHO given that all current config parameters are options, normally I would allow NULL to be passed to odp_init_global with meaning "use defaults". I completely agree. I agree NULL is a valid argument. Plus this needs to be stated in the function header for doxygen. Also in this particular situation, the way I read above patch, it seems you have init_data allocated on stack of main - meaning it will have garbage in its content. It is wrong. odp_init_global should say that odp_global_init_t content should be memset to 0, fill in fields that current application understands leave remaining to 0. With such approach it would be possible to write code that will survive further addition to structure, i.e all new fields that application was not aware before will be 0. If you don't like memset rule one may provide odp_global_init_t initialization macro and implementation code decides how to initialize it properly. Thanks Victor, this was one of my big questions and the reason for pausing and calling this an RFC. I favored adding an init for the init struct because that is how I have done it before. Allowing for none zero defaults was useful, but I was not sure if it would be considered overkill, each platform would be at liberty to redefine the defaults in their implementation then. > 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 f89ea7a..12a6f53 100644 > --- a/example/l2fwd/odp_l2fwd.c > +++ b/example/l2fwd/odp_l2fwd.c > @@ -280,6 +280,7 @@ int main(int argc, char *argv[]) > { > odp_linux_pthread_t thread_tbl[MAX_WORKERS]; > odp_buffer_pool_t pool; > + odp_global_init_t init_data; > int thr_id; > void *pool_base; > int i; > @@ -288,7 +289,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(init_data)) { > 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 be96093..c82ef51 100644 > --- a/example/odp_example/odp_example.c > +++ b/example/odp_example/odp_example.c > @@ -917,6 +917,7 @@ static void parse_args(int argc, char *argv[], > test_args_t *args) > int main(int argc, char *argv[]) > { > odp_linux_pthread_t thread_tbl[MAX_WORKERS]; > + odp_global_init_t init_data; > test_args_t args; > int thr_id; > int num_workers; > @@ -934,7 +935,7 @@ int main(int argc, char *argv[]) > > memset(thread_tbl, 0, sizeof(thread_tbl)); > > - if (odp_init_global()) { > + if (odp_init_global(init_data)) { > printf("ODP global init failed.\n"); > return -1; > } > diff --git a/example/packet/odp_pktio.c b/example/packet/odp_pktio.c > index 247a28a..aa3e848 100644 > --- a/example/packet/odp_pktio.c > +++ b/example/packet/odp_pktio.c > @@ -277,6 +277,7 @@ int main(int argc, char *argv[]) > { > odp_linux_pthread_t thread_tbl[MAX_WORKERS]; > odp_buffer_pool_t pool; > + odp_global_init_t init_data; > int thr_id; > int num_workers; > void *pool_base; > @@ -285,7 +286,7 @@ int main(int argc, char *argv[]) > int core_count; > > /* Init ODP before calling anything else */ > - if (odp_init_global()) { > + if (odp_init_global(init_data)) { > ODP_ERR("Error: ODP global init failed.\n"); > exit(EXIT_FAILURE); > } In my example I converted the calls to odp_init_global() to a macro to reduce the amount of code to type and help eliminate someone not testing the return code. #define ODP_INIT_GLOBAL(_data) do { \ if ( odp_init_global(_data) ) { \ ODP_ERR(“Error: ODP global init failed!\n”); \ exit(EXIT_FAILURE); \ } \ } while ((0)) Note: the double ‘()’ are used for some older GCC compilers that complained about always being false (I think), but it did give a warning. > diff --git a/example/timer/odp_timer_test.c b/example/timer/odp_timer_test.c > index dbe0e5b..1fce60f 100644 > --- a/example/timer/odp_timer_test.c > +++ b/example/timer/odp_timer_test.c > @@ -193,6 +193,7 @@ static void parse_args(int argc, char *argv[], > test_args_t *args) > int main(int argc, char *argv[]) > { > odp_linux_pthread_t thread_tbl[MAX_WORKERS]; > + odp_global_init_t init_data; > test_args_t args; > int thr_id; > int num_workers; > @@ -210,7 +211,7 @@ int main(int argc, char *argv[]) > > memset(thread_tbl, 0, sizeof(thread_tbl)); > > - if (odp_init_global()) { > + if (odp_init_global(init_data)) { > printf("ODP global init failed.\n"); > return -1; > } > diff --git a/include/odp_init.h b/include/odp_init.h > index 490324a..520e6eb 100644 > --- a/include/odp_init.h > +++ b/include/odp_init.h > @@ -4,7 +4,6 @@ > * SPDX-License-Identifier: BSD-3-Clause > */ > > - > /** > * @file > * > @@ -23,23 +22,36 @@ extern "C" { > #include <odp_std_types.h> > > > - > +/** > + * ODP initialisation struct > + */ > +typedef struct odp_global_init{ > + /** Callback for logging replaces the default implementation if > + * not NULL > + */ > + int (*logging)(char *format,...); Normally it should be 'const char *format'. Agree What is 'int' return type is used for? Upon successful completion, the printf() function shall return the number of bytes transmitted. http://pubs.opengroup.org/onlinepubs/009695399/functions/fprintf.html We dont have to follow printf exactly, but felt like the correct thing to do. I think in ODP currently there are two macros used one for reporting errors and another debug one. I think it is good idea to provide two callbacks here. Or if we want to introduce error report value, enumeration defining those level is needed and another arument to the function. I had assumed that the FLAGS would be ERROR, WARNING, INFO, DBG etc, and they all went the same way to the logging. In addition the logging level can be set/enabled per module, i.e. independently for crypto, classification, DPI Ola had a suggestion that does exactly what you say, the actual statement in the ODP lib would take the arguments. But here I just wanted to define enough for global_init, and follow up with attaching it to the debugging. Also I don't think it should be ... api, it should be va_arg API. Because normally this function will be used as second layer after ... api. In ODP log function implementation will have ... API, that it would call callback if any was provided or call vprintf and such if default reporting is used. I.e something like this void (*custom_error) (const char *format, va_list va); Sure, we could also use void (*custom_error) (int level, const char *format, va_list va); where level is ERROR, WARNING etc And then allow the application to decide what to discard - the thought the other day was that this added overhead however. void odp_error(const char *format, ...) { va_list va; va_start(va, format); if (custom_error) { custom_error(format, va); } else { vprintf(format, va); } va_end(va); } This could be common, and it could check the level, or as above we pass that up to the application. void odp_log(int, level const char *format, ...) { va_list va; va_start(va, format); if ( this_module_log_level => level) if (custom_error) { custom_error(format, va); } else { vprintf(format, va); } va_end(va); } } If it would be ... api I am not sure how to call it. Thanks, Victor > + /** Platform specific data. ODP does nothing with > + * this pointer > + */ > + void *platform_implimentaion_data; > +} odp_global_init_t; > > /** > - * Perform global ODP initalisation. > + * Perform global ODP initialisation. > * > * This function must be called once before calling > * any other ODP API functions. > * > * @return 0 if successful > */ > -int odp_init_global(void); > +int odp_init_global(odp_global_init_t 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-dpdk/odp_init.c b/platform/linux-dpdk/odp_init.c > index ecc2066..97a4024 100644 > --- a/platform/linux-dpdk/odp_init.c > +++ b/platform/linux-dpdk/odp_init.c > @@ -50,7 +50,7 @@ int odp_init_dpdk(void) > return 0; > } > > -int odp_init_global(void) > +int odp_init_global(odp_global_init_t params ODP_UNUSED) Just to make sure need to change all odp_init_global() calls to use ‘odp_global_init_t * params’ adding the ‘*’. > { > odp_thread_init_global(); > > diff --git a/platform/linux-generic/odp_init.c > b/platform/linux-generic/odp_init.c > index d4c2eb8..db04903 100644 > --- a/platform/linux-generic/odp_init.c > +++ b/platform/linux-generic/odp_init.c > @@ -9,7 +9,7 @@ > #include <odp_debug.h> > > > -int odp_init_global(void) > +int odp_init_global(odp_global_init_t params ODP_UNUSED) > { > odp_thread_init_global(); > > diff --git a/platform/linux-keystone2/odp_init.c > b/platform/linux-keystone2/odp_init.c > index f832551..066dace 100644 > --- a/platform/linux-keystone2/odp_init.c > +++ b/platform/linux-keystone2/odp_init.c > @@ -90,7 +90,7 @@ static int ti_init_hw_config(void) > } > > > -int odp_init_global(void) > +int odp_init_global(odp_global_init_t init_data ODP_UNUSED) > { > odp_thread_init_global(); > > diff --git a/test/api_test/odp_common.c b/test/api_test/odp_common.c > index 89ebd2d..cfbef19 100644 > --- a/test/api_test/odp_common.c > +++ b/test/api_test/odp_common.c > @@ -54,9 +54,10 @@ void odp_print_system_info(void) > /** test init globals and call odp_init_global() */ > int odp_test_global_init(void) > { > + odp_global_init_t init_data; > memset(thread_tbl, 0, sizeof(thread_tbl)); > > - if (odp_init_global()) { > + if (odp_init_global(init_data)) { > ODP_ERR("ODP global init failed.\n"); > return -1; > } > -- > 1.9.1 > > > _______________________________________________ > 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]<mailto:[email protected]> http://lists.linaro.org/mailman/listinfo/lng-odp
_______________________________________________ lng-odp mailing list [email protected] http://lists.linaro.org/mailman/listinfo/lng-odp
