On 23 February 2017 at 04:57, Bill Fischofer <bill.fischo...@linaro.org>
wrote:
>
>
> On Wed, Feb 22, 2017 at 6:55 AM, Christophe Milard <
> christophe.mil...@linaro.org> wrote:
>
>> The functions to register and probe enumerator classes are added.
>>
>> Signed-off-by: Christophe Milard <christophe.mil...@linaro.org>
>> ---
>> platform/linux-generic/Makefile.am | 1 +
>> platform/linux-generic/_modules.c | 4 +
>> platform/linux-generic/drv_driver.c | 212
>> ++++++++++++++++++++-
>> .../linux-generic/include/drv_driver_internal.h | 22 +++
>> platform/linux-generic/include/odp_internal.h | 5 +
>> platform/linux-generic/odp_init.c | 21 +-
>> 6 files changed, 260 insertions(+), 5 deletions(-)
>> create mode 100644 platform/linux-generic/include/drv_driver_internal.h
>>
>> diff --git a/platform/linux-generic/Makefile.am
>> b/platform/linux-generic/Makefile.am
>> index 66ff53d..b7d1b1a 100644
>> --- a/platform/linux-generic/Makefile.am
>> +++ b/platform/linux-generic/Makefile.am
>> @@ -133,6 +133,7 @@ noinst_HEADERS = \
>> ${srcdir}/include/_ishm_internal.h \
>> ${srcdir}/include/_ishmphy_internal.h \
>> ${srcdir}/include/_ishmpool_internal.h \
>> + ${srcdir}/include/drv_driver_internal.h\
>> ${srcdir}/include/odp_align_internal.h \
>> ${srcdir}/include/odp_atomic_internal.h \
>> ${srcdir}/include/odp_buffer_inlines.h \
>> diff --git a/platform/linux-generic/_modules.c
>> b/platform/linux-generic/_modules.c
>> index 6bb854e..b23c81f 100644
>> --- a/platform/linux-generic/_modules.c
>> +++ b/platform/linux-generic/_modules.c
>> @@ -9,6 +9,7 @@
>> #include <odp/api/std_types.h>
>> #include <odp/api/debug.h>
>> #include <odp_debug_internal.h>
>> +#include <drv_driver_internal.h>
>> #include <libconfig.h>
>> #include <dlfcn.h>
>>
>> @@ -40,6 +41,9 @@ static int load_modules(void)
>> ODP_DBG("module %s loaded.\n", module_name);
>> }
>>
>> + /* give a chance top the driver interface to probe for new
>> things: */
>>
>
> Garbled phrasing. I assume the intent here was "Give the top driver
> interface a chance to probe for new things" ?
>
>
>> + _odpdrv_driver_probe_drv_items();
>> +
>> return 0;
>> }
>>
>> diff --git a/platform/linux-generic/drv_driver.c
>> b/platform/linux-generic/drv_driver.c
>> index 529da48..50956a7 100644
>> --- a/platform/linux-generic/drv_driver.c
>> +++ b/platform/linux-generic/drv_driver.c
>> @@ -4,20 +4,131 @@
>> * SPDX-License-Identifier: BSD-3-Clause
>> */
>>
>> +#include <string.h>
>> +
>> #include <odp_config_internal.h>
>> +#include <_ishmpool_internal.h>
>>
>> #include <odp/api/std_types.h>
>> #include <odp/api/debug.h>
>> +#include <odp/api/rwlock_recursive.h>
>> #include <odp/drv/driver.h>
>> +#include <odp/drv/spec/driver.h>
>> #include <odp_debug_internal.h>
>> +#include <drv_driver_internal.h>
>> +
>> +static enum {UNDONE, IN_PROGRESS, DONE} init_global_status;
>> +
>> +/* pool from which different list elements are alocated: */
>> +#define ELT_POOL_SIZE (1 << 20) /* 1Mb */
>> +static _odp_ishm_pool_t *list_elt_pool;
>> +
>> +typedef struct _odpdrv_enumr_class_s _odpdrv_enumr_class_t;
>> +
>> +/* an enumerator class (list element) */
>> +struct _odpdrv_enumr_class_s {
>> + odpdrv_enumr_class_param_t param;
>> + int probed;
>> + _odp_ishm_pool_t *pool;
>> + struct _odpdrv_enumr_class_s *next;
>> +};
>> +
>> +/* the enumerator class list: */
>> +typedef struct _odpdrv_enumr_class_lst_t {
>> + odp_rwlock_recursive_t lock;
>> + _odpdrv_enumr_class_t *head;
>> +} _odpdrv_enumr_class_lst_t;
>> +static struct _odpdrv_enumr_class_lst_t enumr_class_lst;
>> +
>> +/* some driver elements (such as enumeraor classes, drivers, devio) may
>> + * register before init_global and init_local complete. Mutex will fail
>> + * in this cases but should be used later on.
>> + * These functions disable the usage of Mutex while it is global init
>> i.e.
>> + * while single threaded*/
>>
>
> I don't understand why these tests are needed. You've put the driver init
> stuff at the end of odp_init_global() so all the other machinery is
> available to you at this point. So these guards seem unnecessary.
>
Is this flag solving problem described as "driver probe time...", which to
cover both statically and dynamically linked modules' situation?
at the very beginning: statically linked plugin modules
__constructor__ (register) ...
odp_init_global():
_odpdrv_driver_init_global():
_odp_modules_init_global(): dynamically linked plugin modules
__constructor__ (register), and kick the probing ...
One possible alternative may be creating two lists:
registered_list: ring-buffer based, no system intialization required, all
plugin modules register by only adding themselves into this list but no
further operations. (malloc, shm, etc).
probed_list: lock protected, probing will actually walk through
registered_list and do initialization like shm allocate and then move them
into probed list.
In this way: eliminate this flag and also the probed flag in structure?
>
>
>> +static void enumr_class_list_read_lock(void)
>> +{
>> + if (init_global_status == DONE)
>> + odp_rwlock_recursive_read_lock(&enumr_class_lst.lock);
>> +}
>> +
>> +static void enumr_class_list_read_unlock(void)
>> +{
>> + if (init_global_status == DONE)
>> + odp_rwlock_recursive_read_unlock(&enumr_class_lst.lock);
>> +}
>> +
>> +static void enumr_class_list_write_lock(void)
>> +{
>> + if (init_global_status == DONE)
>> + odp_rwlock_recursive_write_lock(&enumr_class_lst.lock);
>> +}
>> +
>> +static void enumr_class_list_write_unlock(void)
>> +{
>> + if (init_global_status == DONE)
>> + odp_rwlock_recursive_write_unlock(&enumr_class_lst.lock);
>> +}
>> +
>>
>> odpdrv_enumr_class_t odpdrv_enumr_class_register(od
>> pdrv_enumr_class_param_t
>> *param)
>> {
>> - ODP_ERR("NOT Supported yet! Enumerator Class %s Registration!\n.",
>> - param->name);
>> + _odpdrv_enumr_class_t *enumr_c;
>>
>> - return ODPDRV_ENUMR_CLASS_INVALID;
>> + /* parse the list of already registered enumerator class to make
>> + * sure no enumerator with identical name already exists:
>> + */
>> + enumr_class_list_read_lock();
>> + enumr_c = enumr_class_lst.head;
>> + while (enumr_c) {
>> + if (strncmp(param->name, enumr_c->param.name,
>> + ODPDRV_NAME_SIZE) == 0) {
>> + ODP_ERR("enumerator class %s already exists!\n",
>> + param->name);
>> + enumr_class_list_read_unlock();
>> + return ODPDRV_ENUMR_CLASS_INVALID;
>> + }
>> + enumr_c = enumr_c->next;
>> + }
>> + enumr_class_list_read_unlock();
>> +
>> + /* allocate memory for the new enumerator class:
>> + * If init_global has not been done yet, then, we cannot allocate
>> + * from any _ishm pool (ishm has not even been initialised at this
>> + * stage...this happens when statically linked enumerator classes
>> + * register: their __constructor__ function is run before main()
>> + * is called). But any malloc performed here(before init_global)
>> + * will be inherited by any odpthreads (process or pthreads) as we
>> + * are still running in the ODP instantiation processes and all
>> + * other processes are guaranteed to be descendent of this one...
>> + * If init_global has been done, then we allocate from the _ishm
>> pool
>> + * to guarantee visibility from any ODP thread.
>> + */
>> +
>> + if (init_global_status == UNDONE) {
>> + enumr_c = malloc(sizeof(_odpdrv_enumr_class_t));
>> + if (!enumr_c)
>> + return ODPDRV_ENUMR_CLASS_INVALID;
>> + enumr_c->pool = NULL;
>> + } else {
>> + enumr_c = _odp_ishm_pool_alloc(list_elt_pool,
>> +
>> sizeof(_odpdrv_enumr_class_t));
>> + if (!enumr_c) {
>> + ODP_ERR("_odp_ishm_pool_alloc failed!\n");
>> + return ODPDRV_ENUMR_CLASS_INVALID;
>> + }
>> + enumr_c->pool = list_elt_pool;
>> + }
>> +
>> + /* save init parameters and insert enumerator class in list */
>> + enumr_c->param = *param;
>> + enumr_c->probed = 0;
>> + enumr_class_list_write_lock();
>> + enumr_c->next = enumr_class_lst.head;
>> + enumr_class_lst.head = enumr_c;
>> + enumr_class_list_write_unlock();
>> +
>> + return (odpdrv_enumr_class_t)enumr_c;
>> }
>>
>> odpdrv_enumr_t odpdrv_enumr_register(odpdrv_enumr_param_t *param)
>> @@ -57,8 +168,101 @@ odpdrv_driver_t
>> odpdrv_driver_register(odpdrv_driver_param_t
>> *param)
>> return ODPDRV_DRIVER_INVALID;
>> }
>>
>> +/* the following function is called each time probing is needed, i.e.
>> + * at init or after loading a new module as a module can be anything,
>> + * including enumerators or drivers */
>> +void _odpdrv_driver_probe_drv_items(void)
>> +{
>> + _odpdrv_enumr_class_t *enumr_c;
>> +
>> + /* probe unprobed enumerators: */
>> + enumr_class_list_read_lock();
>> + enumr_c = enumr_class_lst.head;
>>
>
> Where is enumr_class_lst.head initialized? You've initialized the lock
> part of this struct in _odpdrv_init_global() but not the head. So this may
> contain uninitialized / stale values from a previous ODP instance.
>
>
>> + while (enumr_c) {
>> + if (!enumr_c->probed) {
>> + enumr_c->param.probe();
>> + enumr_c->probed = 1;
>> + }
>> + enumr_c = enumr_c->next;
>> + }
>> + enumr_class_list_read_unlock();
>> +}
>> +
>> int odpdrv_print_all(void)
>> {
>> - ODP_ERR("odpdrv_print_all not Supported yet!\n.");
>> + _odpdrv_enumr_class_t *enumr_c;
>> +
>> + /* we cannot use ODP_DBG before ODP init... */
>> + if (init_global_status == UNDONE)
>> + return 0;
>> +
>> + ODP_DBG("ODP Driver status:\n");
>> +
>> + /* print the list of registered enumerator classes: */
>> + enumr_class_list_read_lock();
>> + enumr_c = enumr_class_lst.head;
>> + ODP_DBG("The following enumerator classes have been
>> registered:\n");
>> + while (enumr_c) {
>> + ODP_DBG(" class: %s\n", enumr_c->param.name);
>> + enumr_c = enumr_c->next;
>> + }
>> + enumr_class_list_read_unlock();
>> + return 0;
>> +}
>> +
>> +int _odpdrv_driver_init_global(void)
>> +{
>> + /* create a memory pool to for list elements: */
>> + list_elt_pool = _odp_ishm_pool_create(NULL, ELT_POOL_SIZE,
>> + 0, ELT_POOL_SIZE, 0);
>> +
>> + /* remember that init global is being done so the further list
>> allocs
>> + * are made from the list_elt_pool: */
>> + init_global_status = IN_PROGRESS;
>> +
>> + /* from now, we want to ensure mutex on the list: init lock: */
>> + odp_rwlock_recursive_init(&enumr_class_lst.lock);
>> +
>> + /* probe things... */
>> + _odpdrv_driver_probe_drv_items();
>> +
>> + return 0;
>> +}
>> +
>> +int _odpdrv_driver_init_local(void)
>> +{
>> + /* remember that init global is done, so list mutexes are used
>> from
>> + * now */
>> + init_global_status = DONE;
>>
>
> Not clear what this does. Also, having the init() function manipulate a
> global variable is not good form. The idea behind the init_local() calls is
> that these routines do thread-local initialization. Remember this routine
> may be called in parallel from multiple threads if the app is launching
> threads since each of them is going to be calling odp_init_local().
>
>
>> + return 0;
>> +}
>> +
>> +int _odpdrv_driver_term_global(void)
>> +{
>> + _odpdrv_enumr_class_t *enumr_c;
>> +
>> + if (init_global_status == UNDONE)
>> + return 0;
>>
>
> I'm not sure what this guard is protecting against. If an app calls
> odp_term_global() before it calls odp_init_global() that is an application
> programming error and we need not worry about such things.
>
>
>> +
>> + /* remove all enumerator classes which are registered: */
>> + enumr_class_list_write_lock();
>> + while (enumr_class_lst.head) {
>> + enumr_c = enumr_class_lst.head;
>> + if (enumr_c->param.remove) { /* run remove callback, if
>> any */
>> + if (enumr_c->param.remove())
>> + ODP_ERR("Enumerator class %s removal
>> failed.\n",
>> + enumr_c->param.name);
>> + }
>> + enumr_class_lst.head = enumr_c->next;
>> + if (enumr_c->pool)
>> + _odp_ishm_pool_free(list_elt_pool, enumr_c);
>> + else
>> + free(enumr_c);
>> + }
>> + enumr_class_list_write_unlock();
>> +
>> + /* destroy the list element pool: */
>> + _odp_ishm_pool_destroy(list_elt_pool);
>> +
>> return 0;
>> }
>> diff --git a/platform/linux-generic/include/drv_driver_internal.h
>> b/platform/linux-generic/include/drv_driver_internal.h
>> new file mode 100644
>> index 0000000..eb06c1b
>> --- /dev/null
>> +++ b/platform/linux-generic/include/drv_driver_internal.h
>> @@ -0,0 +1,22 @@
>> +/* Copyright (c) 2017, Linaro Limited
>> + * All rights reserved.
>> + *
>> + * SPDX-License-Identifier: BSD-3-Clause
>> + */
>> +
>> +#ifndef DRV_DRIVER_INTERNAL_H_
>> +#define DRV_DRIVER_INTERNAL_H_
>> +
>> +#ifdef __cplusplus
>> +extern "C" {
>> +#endif
>> +
>> +#include <sys/types.h>
>> +
>> +void _odpdrv_driver_probe_drv_items(void);
>> +
>> +#ifdef __cplusplus
>> +}
>> +#endif
>> +
>> +#endif
>> diff --git a/platform/linux-generic/include/odp_internal.h
>> b/platform/linux-generic/include/odp_internal.h
>> index 05c8a42..1760b56 100644
>> --- a/platform/linux-generic/include/odp_internal.h
>> +++ b/platform/linux-generic/include/odp_internal.h
>> @@ -71,6 +71,7 @@ enum init_stage {
>> CLASSIFICATION_INIT,
>> TRAFFIC_MNGR_INIT,
>> NAME_TABLE_INIT,
>> + DRIVER_INIT,
>> MODULES_INIT,
>> ALL_INIT /* All init stages completed */
>> };
>> @@ -130,6 +131,10 @@ int _odp_ishm_init_local(void);
>> int _odp_ishm_term_global(void);
>> int _odp_ishm_term_local(void);
>>
>> +int _odpdrv_driver_init_global(void);
>> +int _odpdrv_driver_init_local(void);
>> +int _odpdrv_driver_term_global(void);
>> +
>> int _odp_modules_init_global(void);
>>
>> int cpuinfo_parser(FILE *file, system_info_t *sysinfo);
>> diff --git a/platform/linux-generic/odp_init.c
>> b/platform/linux-generic/odp_init.c
>> index 685e02f..c59cc28 100644
>> --- a/platform/linux-generic/odp_init.c
>> +++ b/platform/linux-generic/odp_init.c
>> @@ -266,6 +266,12 @@ int odp_init_global(odp_instance_t *instance,
>> }
>> stage = NAME_TABLE_INIT;
>>
>> + if (_odpdrv_driver_init_global()) {
>> + ODP_ERR("ODP drivers init failed\n");
>> + goto init_failed;
>> + }
>> + stage = DRIVER_INIT;
>> +
>> if (_odp_modules_init_global()) {
>> ODP_ERR("ODP modules init failed\n");
>> goto init_failed;
>> @@ -296,6 +302,13 @@ int _odp_term_global(enum init_stage stage)
>> switch (stage) {
>> case ALL_INIT:
>> case MODULES_INIT:
>> + case DRIVER_INIT:
>> + if (_odpdrv_driver_term_global()) {
>> + ODP_ERR("driver term failed.\n");
>> + rc = -1;
>> + }
>> + /* Fall through */
>> +
>> case NAME_TABLE_INIT:
>> if (_odp_int_name_tbl_term_global()) {
>> ODP_ERR("Name table term failed.\n");
>> @@ -445,7 +458,13 @@ int odp_init_local(odp_instance_t instance,
>> odp_thread_type_t thr_type)
>> ODP_ERR("ODP schedule local init failed.\n");
>> goto init_fail;
>> }
>> - /* stage = SCHED_INIT; */
>> + stage = SCHED_INIT;
>> +
>> + if (_odpdrv_driver_init_local()) {
>> + ODP_ERR("ODP driver local init failed.\n");
>> + goto init_fail;
>> + }
>> + /* stage = DRIVER_INIT; */
>>
>> return 0;
>>
>> --
>> 2.7.4
>>
>>
>
