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

[Eli Dorfman (Voltaire)]

Sasha Khapyorsky wrote:
> 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?

No since i don't want the next pointer.

> 
>> +
>> +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?

yes, i should. the second patch fixes that.