On 14 August 2014 14:49, Victor Kamensky <[email protected]> wrote:
> Mike, > > Please see inline > > On 13 August 2014 03:13, Mike Holmes <[email protected]> wrote: > > Signed-off-by: Mike Holmes <[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)) { > > 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. > > 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); > > } > > 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) > > { > > 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] > > 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
