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

[Eli Dorfman (Voltaire)]

Sasha Khapyorsky wrote:
> Hi Eli,
> 
> On 15:37 Thu 19 Mar     , Eli Dorfman (Voltaire) wrote:
>> setup routing engine when in use and delete when fail
>>
>> setup routing engine and allocate resources 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/include/opensm/osm_opensm.h |    6 ++++++
>>  opensm/opensm/osm_opensm.c         |   10 ++--------
>>  opensm/opensm/osm_ucast_mgr.c      |   17 +++++++++++++++++
>>  3 files changed, 25 insertions(+), 8 deletions(-)
>>
>> diff --git a/opensm/include/opensm/osm_opensm.h b/opensm/include/opensm/osm_opensm.h
>> index c121be4..8d1b276 100644
>> --- a/opensm/include/opensm/osm_opensm.h
>> +++ b/opensm/include/opensm/osm_opensm.h
>> @@ -109,6 +109,9 @@ typedef enum _osm_routing_engine_type {
>>  } osm_routing_engine_type_t;
>>  /***********/
>>  
>> +struct osm_routing_engine;
>> +struct osm_opensm;
>> +
>>  /****s* OpenSM: OpenSM/osm_routing_engine
>>  * NAME
>>  *	struct osm_routing_engine
>> @@ -122,6 +125,7 @@ typedef enum _osm_routing_engine_type {
>>  struct osm_routing_engine {
>>  	const char *name;
>>  	void *context;
>> +	int (*setup) (struct osm_routing_engine *re, struct osm_opensm *p_osm);
>>  	int (*build_lid_matrices) (void *context);
>>  	int (*ucast_build_fwd_tables) (void *context);
>>  	void (*ucast_dump_tables) (void *context);
>> @@ -523,5 +527,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);
>> +
> 
> This function is not implemented in this patch. Please move it to
> related patch.
> 
>>  END_C_DECLS
>>  #endif				/* _OSM_OPENSM_H_ */
>> diff --git a/opensm/opensm/osm_opensm.c b/opensm/opensm/osm_opensm.c
>> index 50d1349..9739122 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 = m->setup;
> 
> Ok, only 'setup' callback is initialized here. That is fine. But later
> in destroy_routing_engines() for all routing engines delete() method is
> called unconditionally, which obviously should crash OpenSM.
> 
> It is still be broken IMO.

No.
routing_engine->setup is called by osm_ucast_mgr_process()
and if routing algorithm fails then delete is called (and is already set).
I already tested this patch and opensm does not crash.

Eli