[ofa-general] Re: [PATCH v2] opensm: support routing engine update

[Sasha Khapyorsky]

On 10:24 Mon 16 Mar     , Eli Dorfman (Voltaire) wrote:

<snip...>
> diff --git a/opensm/include/opensm/osm_opensm.h b/opensm/include/opensm/osm_opensm.h
> index c121be4..d297dd8 100644
> --- a/opensm/include/opensm/osm_opensm.h
> +++ b/opensm/include/opensm/osm_opensm.h
> @@ -122,6 +122,8 @@ typedef enum _osm_routing_engine_type {
>  struct osm_routing_engine {
>  	const char *name;
>  	void *context;
> +	int initialized;
> +	int (*setup) (void *re, void *p_osm);

In order to avoid ugly castings here you need to declare a real function
prototype:

int (*)(struct osm_routing_engine *, struct osm_opensm *);

For this you probably should put type forward declarations above those
lines:

struct osm_routing_engine;
struct osm_opensm;

>  	int (*build_lid_matrices) (void *context);
>  	int (*ucast_build_fwd_tables) (void *context);
>  	void (*ucast_dump_tables) (void *context);
> @@ -183,6 +185,7 @@ typedef struct osm_opensm {
>  	cl_dispatcher_t disp;
>  	cl_plock_t lock;
>  	struct osm_routing_engine *routing_engine_list;
> +	struct osm_routing_engine *last_routing_engine;
>  	osm_routing_engine_type_t routing_engine_used;
>  	osm_stats_t stats;
>  	osm_console_t console;
> @@ -523,5 +526,7 @@ extern volatile unsigned int osm_exit_flag;
>  *  Set to one to cause all threads to leave
>  *********/
>  
> +void osm_update_routing_engines(osm_opensm_t *osm, const char *engine_names);
> +
>  END_C_DECLS
>  #endif				/* _OSM_OPENSM_H_ */
> diff --git a/opensm/opensm/osm_opensm.c b/opensm/opensm/osm_opensm.c
> index cfe6474..7126658 100644
> --- a/opensm/opensm/osm_opensm.c
> +++ b/opensm/opensm/osm_opensm.c
> @@ -169,14 +169,7 @@ static void setup_routing_engine(osm_opensm_t *osm, const char *name)
>  			memset(re, 0, sizeof(struct osm_routing_engine));
>  
>  			re->name = m->name;
> -			if (m->setup(re, osm)) {
> -				OSM_LOG(&osm->log, OSM_LOG_VERBOSE,
> -					"setup of routing"
> -					" engine \'%s\' failed\n", name);
> -				return;
> -			}
> -			OSM_LOG(&osm->log, OSM_LOG_DEBUG,
> -				"\'%s\' routing engine set up\n", re->name);
> +			re->setup = (int (*)(void *, void *))m->setup;
>  			append_routing_engine(osm, re);
>  			return;
>  		}
> @@ -236,6 +229,55 @@ static void destroy_routing_engines(osm_opensm_t *osm)
>  			r->delete(r->context);
>  		free(r);
>  	}
> +	osm->routing_engine_list = NULL;
> +}
> +
> +static void update_routing_engine(
> +	struct osm_routing_engine *cur,
> +	struct osm_routing_engine *last)
> +{
> +	cur->context = last->context;
> +	cur->initialized = last->initialized;
> +	cur->setup = last->setup;
> +	cur->build_lid_matrices = last->build_lid_matrices;
> +	cur->ucast_build_fwd_tables = last->ucast_build_fwd_tables;
> +	cur->ucast_dump_tables = last->ucast_dump_tables;
> +	cur->delete = last->delete;
> +}

This is memcpy(), isn't it?

> +
> +void osm_update_routing_engines(osm_opensm_t *osm, const char *engine_names)
> +{
> +	struct osm_routing_engine *r, *l;
> +
> +	/* find used routing engine and save as last */
> +	l = r = osm->routing_engine_list;
> +	if (r && osm->routing_engine_used == osm_routing_engine_type(r->name)) {
> +		osm->last_routing_engine = r;
> +		osm->routing_engine_list = r->next;
> +	}
> +	else while ((r = r->next)) {
> +		if (osm->routing_engine_used == 
> +			osm_routing_engine_type(r->name)) {
> +			osm->last_routing_engine = r;
> +			l->next = r->next;
> +			break;
> +		}
> +		l = r;
> +	}
> +	/* cleanup prev routing engine list and replace with current list */
> +	destroy_routing_engines(osm);
> +	setup_routing_engines(osm, engine_names);
> +
> +	/* check if last routing engine exist in new list and update callbacks */
> +	r = osm->routing_engine_list;
> +	while (r) {
> +		if (osm->routing_engine_used == 
> +			osm_routing_engine_type(r->name)) {
> +			update_routing_engine(r, osm->last_routing_engine);
> +			break;
> +		}
> +		r = r->next;
> +	}
>  }
>  
>  /**********************************************************************
> diff --git a/opensm/opensm/osm_ucast_mgr.c b/opensm/opensm/osm_ucast_mgr.c
> index fe0a446..abae978 100644
> --- a/opensm/opensm/osm_ucast_mgr.c
> +++ b/opensm/opensm/osm_ucast_mgr.c
> @@ -970,8 +970,25 @@ int osm_ucast_mgr_process(IN osm_ucast_mgr_t * const p_mgr)
>  
>  	p_osm->routing_engine_used = OSM_ROUTING_ENGINE_TYPE_NONE;
>  	while (p_routing_eng) {
> +		if (!p_routing_eng->initialized && 
> +			p_routing_eng->setup(p_routing_eng, p_osm)) {
> +			OSM_LOG(p_mgr->p_log, OSM_LOG_ERROR,
> +					"ERR 3A0F: setup of routing engine \'%s\' failed\n", 
> +					p_routing_eng->name);
> +			p_routing_eng = p_routing_eng->next;
> +			continue;
> +		}
> +		OSM_LOG(p_mgr->p_log, OSM_LOG_INFO,
> +			"\'%s\' routing engine set up\n", p_routing_eng->name);
> +		p_routing_eng->initialized = 1;
> +
>  		if (!ucast_mgr_route(p_routing_eng, p_osm))
>  			break;
> +
> +		/* delete unused routing engine */
> +		if (p_routing_eng->delete)
> +			p_routing_eng->delete(p_routing_eng->context);
> +

Now it can be deleted twice - here and in destroy_routing_engines()?
Also shouldn't you drop initialized flag?

Sasha