[ofa-general] Re: [PATCH 2/2] opensm: setup routing engine when in use and delete when fail

[Eli Dorfman (Voltaire)]

Sasha Khapyorsky wrote:
> On 17:36 Thu 26 Feb     , Eli Dorfman (Voltaire) wrote:
>>  setup routing engine when in use and delete when fail
>>  setup routing engine before use.
>>  delete resources when routing algorithm fails
>>  this will save allocation for routing algorithms that are not used.
>>
>> Signed-off-by: Eli Dorfman <elid at voltaire.com>
>> ---
>>  opensm/opensm/osm_opensm.c    |   20 ++++++--------------
>>  opensm/opensm/osm_ucast_mgr.c |   34 +++++++++++++++++++++++++++++++++-
>>  2 files changed, 39 insertions(+), 15 deletions(-)
>>
>> diff --git a/opensm/opensm/osm_opensm.c b/opensm/opensm/osm_opensm.c
>> index 7de2e5b..a2620d5 100644
>> --- a/opensm/opensm/osm_opensm.c
>> +++ b/opensm/opensm/osm_opensm.c
>> @@ -169,21 +169,14 @@ 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 = m->setup;
>>  			append_routing_engine(osm, re);
>>  			return;
>>  		}
>>  	}
>>  
>>  	OSM_LOG(&osm->log, OSM_LOG_ERROR,
>> -		"cannot find or setup routing engine \'%s\'", name);
>> +		"cannot find or setup routing engine \'%s\'\n", name);
> 
> This is needed fix, but unrelated to the patch.
> 
>>  }
>>  
>>  static void setup_routing_engines(osm_opensm_t *osm, const char *engine_names)
>> @@ -224,18 +217,17 @@ void osm_opensm_construct(IN osm_opensm_t * const p_osm)
>>  
>>  /**********************************************************************
>>   **********************************************************************/
>> -static void destroy_routing_engines(osm_opensm_t *osm)
>> +static void destroy_routing_engines(struct osm_routing_engine **re)
> 
> Any reason for function prototype change?
> 
>>  {
>>  	struct osm_routing_engine *r, *next;
>>  
>> -	next = osm->routing_engine_list;
>> +	next = *re;
>>  	while (next) {
>>  		r = next;
>>  		next = r->next;
>> -		if (r->delete)
>> -			r->delete(r->context);
> 
> What will happen with currently used routing engine? It will be never
> deleted, leak? right?

you are partially right - when opensm goes down it is not deleted.
there are two RE list (prev and current).
when routing engine list is updated we destroy prev used RE unless it is the 
same as currently used RE.
see also explanation below.

> 
>>  		free(r);
>>  	}
>> +	*re = NULL;
>>  }
>>  
>>  /**********************************************************************
>> @@ -289,7 +281,7 @@ void osm_opensm_destroy(IN osm_opensm_t * const p_osm)
>>  
>>  	/* do the destruction in reverse order as init */
>>  	destroy_plugins(p_osm);
>> -	destroy_routing_engines(p_osm);
>> +	destroy_routing_engines(&p_osm->routing_engine_list);
>>  	osm_sa_destroy(&p_osm->sa);
>>  	osm_sm_destroy(&p_osm->sm);
>>  #ifdef ENABLE_OSM_PERF_MGR
>> diff --git a/opensm/opensm/osm_ucast_mgr.c b/opensm/opensm/osm_ucast_mgr.c
>> index e404c91..7175926 100644
>> --- a/opensm/opensm/osm_ucast_mgr.c
>> +++ b/opensm/opensm/osm_ucast_mgr.c
>> @@ -886,7 +886,6 @@ int osm_ucast_mgr_process(IN osm_ucast_mgr_t * const p_mgr)
>>  
>>  	p_sw_guid_tbl = &p_mgr->p_subn->sw_guid_tbl;
>>  	p_osm = p_mgr->p_subn->p_osm;
>> -	p_routing_eng = p_osm->routing_engine_list;
>>  
>>  	CL_PLOCK_EXCL_ACQUIRE(p_mgr->p_lock);
>>  
>> @@ -897,10 +896,30 @@ int osm_ucast_mgr_process(IN osm_ucast_mgr_t * const p_mgr)
>>  	    ucast_mgr_setup_all_switches(p_mgr->p_subn) < 0)
>>  		goto Exit;
>>  
>> +	/* update the entry in active list */
>> +
>>  	p_osm->routing_engine_used = OSM_ROUTING_ENGINE_TYPE_NONE;
>> +	p_routing_eng = p_osm->routing_engine_list;
>>  	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_VERBOSE,
>> +				"setup of routing engine \'%s\' failed\n", 
>> +					p_routing_eng->name);
>                                 ^^^^^^^^
> 
> Keep indentation please.
> 
>> +			p_routing_eng = p_routing_eng->next;
>> +			continue;
>> +		}
>> +		OSM_LOG(p_mgr->p_log, OSM_LOG_DEBUG,
>> +			"\'%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);
>> +
>>  		p_routing_eng = p_routing_eng->next;
>>  	}
>>  
>> @@ -911,6 +930,19 @@ int osm_ucast_mgr_process(IN osm_ucast_mgr_t * const p_mgr)
>>  		p_osm->routing_engine_used = OSM_ROUTING_ENGINE_TYPE_MINHOP;
>>  	}
>>  
>> +	/* if for some reason different routing engine is used */
>> +	/* cleanup unused routing engine */
>> +	p_routing_eng = p_osm->routing_engine_list;
>> +	while (p_routing_eng) {
>> +		if (p_routing_eng->initialized &&
>> +			p_osm->routing_engine_used != 
>> +				osm_routing_engine_type(p_routing_eng->name) &&
>> +			p_routing_eng->delete) 
>> +			p_routing_eng->delete(p_routing_eng->context);
>> +
>> +		p_routing_eng = p_routing_eng->next;
>> +	}
>> +
> 
> How this section of code is useful?

This code is used to free memory allocated for RE that is not used anymore.
If for example we configured routing_engine=X,Z
and routing_engine_used was Z
Now if we modify routing_engine=Y,Z
and opensm succeeds to setup Y
Then we should free memory of RE(Z) that is not used now.

Eli