[ofa-general] Re: [PATCH 2/2] opensm routing engine update

[Sasha Khapyorsky]

On 17:49 Thu 26 Feb     , Eli Dorfman (Voltaire) wrote:
> 
> diff --git a/opensm/opensm/osm_opensm.c b/opensm/opensm/osm_opensm.c
> index a2620d5..6ab28be 100644
> --- a/opensm/opensm/osm_opensm.c
> +++ b/opensm/opensm/osm_opensm.c
> @@ -230,6 +230,15 @@ static void destroy_routing_engines(struct osm_routing_engine **re)
>  	*re = NULL;
>  }
>  
> +void update_routing_engines(osm_opensm_t *osm, const char *engine_names)
> +{
> +	/* cleanup prev routing engine list and replace with current list */
> +	destroy_routing_engines(&osm->prev_routing_engine_list);
> +	osm->prev_routing_engine_list = osm->routing_engine_list;
> +	osm->routing_engine_list = NULL;
> +	setup_routing_engines(osm, engine_names);
> +}
> +

If you are going to setup/delete routing engine right before/after use
(previous patch series) why do you need to keep whole list as previous?
Could you just drop uninitialized REs?

> diff --git a/opensm/opensm/osm_subnet.c b/opensm/opensm/osm_subnet.c
> index b3100a4..1ba5c91 100644
> --- a/opensm/opensm/osm_subnet.c
> +++ b/opensm/opensm/osm_subnet.c
> @@ -151,6 +151,14 @@ static void opts_setup_sm_priority(osm_subn_t *p_subn, void *p_val)
>  	osm_set_sm_priority(p_sm, sm_priority);
>  }
>  
> +static void opts_setup_routing_engine(osm_subn_t *p_subn, void *p_val)
> +{
> +	osm_opensm_t *p_osm = p_subn->p_osm;
> +	char *engines = (char *) p_val;

Casting is not needed. Actually you don't need any intermediate
variables here. Just something like this:

	osm_update_routing_engines(p_subn->p_osm, p_val);

should work fine.

> +
> +	update_routing_engines(p_osm, engines);
> +}
> +
>  static void opts_parse_net64(IN osm_subn_t *p_subn, IN char *p_key,
>  			     IN char *p_val_str, void *p_v1, void *p_v2,
>  			     void (*pfn)(osm_subn_t *, void *))

> diff --git a/opensm/opensm/osm_ucast_mgr.c b/opensm/opensm/osm_ucast_mgr.c
> index 7175926..cda9f34 100644
> --- a/opensm/opensm/osm_ucast_mgr.c
> +++ b/opensm/opensm/osm_ucast_mgr.c
> @@ -879,7 +879,7 @@ static int ucast_mgr_route(struct osm_routing_engine *r, osm_opensm_t *osm)
>  int osm_ucast_mgr_process(IN osm_ucast_mgr_t * const p_mgr)
>  {
>  	osm_opensm_t *p_osm;
> -	struct osm_routing_engine *p_routing_eng;
> +	struct osm_routing_engine *p_routing_eng, *r;
>  	cl_qmap_t *p_sw_guid_tbl;
>  
>  	OSM_LOG_ENTER(p_mgr->p_log);
> @@ -896,6 +896,26 @@ 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;
>  
> +	/* find used routing engine in previous list */
> +	r = p_osm->prev_routing_engine_list;
> +	while (r) {
> +		if (p_osm->routing_engine_used == 
> +			osm_routing_engine_type(r->name))
> +		{
> +			p_routing_eng = p_osm->routing_engine_list;
> +			while (p_routing_eng) {
> +				if (p_osm->routing_engine_used == 
> +					osm_routing_engine_type(p_routing_eng->name)) {
> +					memcpy(p_routing_eng, r, sizeof(*p_routing_eng));
> +					break;
> +				}
> +				p_routing_eng = p_routing_eng->next;
> +			}
> +			break;
> +		}
> +		r = r->next;
> +	}
> +

I think that I'm able to understand what you are trying to achieve here.
But could this be done less tricky (I'm fine to have more complicated
code in osm_update_routing_engine() function instead)?

For example osm_update_routing_engine() could save currently used RE as
to_be_deleted_re field (in case when it was deleted) and drop it at end
of the sweep.

Sasha