[ofa-general] Re: [PATCHv4] opensm/lash: Set minimum VL for LASH to use

[Sasha Khapyorsky]

Hi Hal,

On 16:51 Tue 21 Jul     , Hal Rosenstock wrote:
> diff --git a/opensm/opensm/osm_ucast_lash.c b/opensm/opensm/osm_ucast_lash.c
> index 12b5e34..b785551 100644
> --- a/opensm/opensm/osm_ucast_lash.c
> +++ b/opensm/opensm/osm_ucast_lash.c
> @@ -486,6 +486,7 @@ static void balance_virtual_lanes(lash_t * p_lash, unsigned lanes_needed)
>  	int next_switch2, output_link2;
>  	int stop = 0, cycle_found;
>  	int cycle_found2;
> +	unsigned start_vl = p_lash->p_osm->subn.opt.lash_start_vl;
>  
>  	max_filled_lane = 0;
>  	min_filled_lane = lanes_needed - 1;
> @@ -572,8 +573,8 @@ static void balance_virtual_lanes(lash_t * p_lash, unsigned lanes_needed)
>  			virtual_location[dest][src][max_filled_lane] = 0;
>  			virtual_location[src][dest][min_filled_lane] = 1;
>  			virtual_location[dest][src][min_filled_lane] = 1;
> -			p_lash->switches[src]->routing_table[dest].lane = min_filled_lane;
> -			p_lash->switches[dest]->routing_table[src].lane = min_filled_lane;
> +			p_lash->switches[src]->routing_table[dest].lane = min_filled_lane + start_vl;
> +			p_lash->switches[dest]->routing_table[src].lane = min_filled_lane + start_vl;
>  		}
>  
>  		if (trials == 0)
> @@ -804,6 +805,7 @@ static int lash_core(lash_t * p_lash)
>  	int cycle_found2 = 0;
>  	int status = 0;
>  	int *switch_bitmap = NULL;	/* Bitmap to check if we have processed this pair */
> +	unsigned start_vl = p_lash->p_osm->subn.opt.lash_start_vl;
>  
>  	OSM_LOG_ENTER(p_log);
>  
> @@ -838,7 +840,9 @@ static int lash_core(lash_t * p_lash)
>  	}
>  
>  	for (i = 0; i < num_switches; i++) {
> -		for (dest_switch = 0; dest_switch < num_switches; dest_switch++)
> +		for (dest_switch = 0; dest_switch < num_switches; dest_switch++) {
> +			if (lanes_needed > p_lash->vl_min)
> +				goto Error_Not_Enough_Lanes;

Hmm, such check is already performed in place where lanes_needed is
increased (below). Why do we need another one?

>  			if (dest_switch != i && switch_bitmap[i * num_switches + dest_switch] == 0) {
>  				v_lane = 0;
>  				stop = 0;
> @@ -902,8 +906,8 @@ static int lash_core(lash_t * p_lash)
>  					}
>  				}
>  
> -				switches[i]->routing_table[dest_switch].lane = v_lane;
> -				switches[dest_switch]->routing_table[i].lane = v_lane;
> +				switches[i]->routing_table[dest_switch].lane = v_lane + start_vl;
> +				switches[dest_switch]->routing_table[i].lane = v_lane + start_vl;
>  
>  				if (cycle_found == 1 || cycle_found2 == 1) {
>  					if (++lanes_needed > p_lash->vl_min)

This one.

> @@ -926,16 +930,20 @@ static int lash_core(lash_t * p_lash)
>  				switch_bitmap[i * num_switches + dest_switch] = 1;
>  				switch_bitmap[dest_switch * num_switches + i] = 1;
>  			}
> +		}
>  	}
>  
> -	OSM_LOG(p_log, OSM_LOG_INFO,
> -		"Lanes needed: %d, Balancing\n", lanes_needed);
> +	if (lanes_needed > p_lash->vl_min)
> +		goto Error_Not_Enough_Lanes;

The same question, why do we need yet another lanes_needed > vl_min
check?
>  
>  	for (i = 0; i < lanes_needed; i++) {
>  		OSM_LOG(p_log, OSM_LOG_INFO, "Lanes in layer %d: %d\n",
>  			i, p_lash->num_mst_in_lane[i]);
>  	}
>  
> +	OSM_LOG(p_log, OSM_LOG_INFO,
> +		"Lanes needed: %d, Balancing\n", lanes_needed);
> +
>  	balance_virtual_lanes(p_lash, lanes_needed);
>  
>  	for (i = 0; i < lanes_needed; i++) {
> @@ -948,8 +956,9 @@ static int lash_core(lash_t * p_lash)
>  Error_Not_Enough_Lanes:
>  	status = -1;
>  	OSM_LOG(p_log, OSM_LOG_ERROR, "ERR 4D02: "
> -		"Lane requirements (%d) exceed available lanes (%d)\n",
> -		lanes_needed, p_lash->vl_min);
> +		"Lane requirements (%d) exceed available lanes (%d)"
> +		" with starting lane (%d)\n",
> +		lanes_needed, p_lash->vl_min, start_vl);
>  Exit:
>  	if (switch_bitmap)
>  		free(switch_bitmap);
> @@ -1177,10 +1186,11 @@ static int discover_network_properties(lash_t * p_lash)
>  	if (vl_min > 15)
>  		vl_min = 15;
>  
> -	p_lash->vl_min = vl_min;
> +	p_lash->vl_min = vl_min - p_lash->p_osm->subn.opt.lash_start_vl;

vl_min =< lash_start_vl could be a legal case and lash->vl_min has
unsigned type. You will get an invalid value in this case. I suppose
that check should be here with LASH termination if no VLs are available.

Sasha