[ofa-general] Re: [PATCH] opensm/lash: Set minimum VL for LASH to use
Hal Rosenstock
hal.rosenstock at gmail.com
Mon Jul 6 08:53:59 PDT 2009
Hi Sasha,
On Tue, Jun 23, 2009 at 5:49 PM, Sasha Khapyorsky<sashak at voltaire.com> wrote:
> Hi Hal,
>
> On 10:14 Thu 18 Jun , Hal Rosenstock wrote:
>>
>> rather than starting from VL 0
>>
>> Signed-off-by: Robert Pearson <rpearson at systemfabricworks.com>
>> Signed-off-by: Hal Rosenstock <hal.rosenstock at gmail.com>
>
> See the comments below.
>
> <snip...>
>
>> diff --git a/opensm/opensm/osm_ucast_lash.c b/opensm/opensm/osm_ucast_lash.c
>> index 12b5e34..540214c 100644
>> --- a/opensm/opensm/osm_ucast_lash.c
>> +++ b/opensm/opensm/osm_ucast_lash.c
>> @@ -478,7 +478,7 @@ static void balance_virtual_lanes(lash_t * p_lash, unsigned lanes_needed)
>> cdg_vertex_t ****cdg_vertex_matrix = p_lash->cdg_vertex_matrix;
>> int *num_mst_in_lane = p_lash->num_mst_in_lane;
>> int ***virtual_location = p_lash->virtual_location;
>> - int min_filled_lane, max_filled_lane, trials;
>> + int min_filled_lane, max_filled_lane, trials, max_vl;
>> int old_min_filled_lane, old_max_filled_lane, new_num_min_lane,
>> new_num_max_lane;
>> unsigned int i, j;
>> @@ -486,9 +486,13 @@ 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;
>> + osm_subn_opt_t *opt = &p_lash->p_osm->subn.opt;
>
> In this function 'opt' is used only to get 'lash_start_vl', so wouldn't
> it be better to do something like:
>
> unsigned start_vl = &p_lash->p_osm->subn.opt.lash_start_vl;
> ?
>
> This is relevant for all other similar places in the patch.
>
>>
>> - max_filled_lane = 0;
>> - min_filled_lane = lanes_needed - 1;
>> + max_filled_lane = opt->lash_start_vl;
>> + max_vl = lanes_needed + opt->lash_start_vl;
>> + if (max_vl > IB_MAX_NUM_VLS)
>> + max_vl = IB_MAX_NUM_VLS;
>
> Isn't this case where LASH requires more VLs than available in IB?
>
>> + min_filled_lane = max_vl - 1;
>>
>> trials = num_mst_in_lane[max_filled_lane];
>> if (lanes_needed == 1)
>> @@ -590,7 +594,7 @@ static void balance_virtual_lanes(lash_t * p_lash, unsigned lanes_needed)
>> new_num_min_lane = MAX_INT;
>> new_num_max_lane = 0;
>>
>> - for (i = 0; i < lanes_needed; i++) {
>> + for (i = opt->lash_start_vl; i < max_vl; i++) {
>>
>> if (num_mst_in_lane[i] < new_num_min_lane) {
>> new_num_min_lane = num_mst_in_lane[i];
>> @@ -673,12 +677,18 @@ static void free_lash_structures(lash_t * p_lash)
>> {
>> unsigned int i, j, k;
>> unsigned num_switches = p_lash->num_switches;
>> + int max_vl;
>> osm_log_t *p_log = &p_lash->p_osm->log;
>> + osm_subn_opt_t *opt = &p_lash->p_osm->subn.opt;
>>
>> OSM_LOG_ENTER(p_log);
>>
>> + max_vl = opt->lash_start_vl + p_lash->vl_min;
>> + if (max_vl > IB_MAX_NUM_VLS)
>> + max_vl = IB_MAX_NUM_VLS;
>> +
>> /* free cdg_vertex_matrix */
>> - for (i = 0; i < p_lash->vl_min; i++) {
>> + for (i = opt->lash_start_vl; i < max_vl; i++) {
>> for (j = 0; j < num_switches; j++) {
>> for (k = 0; k < num_switches; k++)
>> if (p_lash->cdg_vertex_matrix[i][j][k])
>> @@ -715,13 +725,19 @@ static int init_lash_structures(lash_t * p_lash)
>> osm_log_t *p_log = &p_lash->p_osm->log;
>> int status = 0;
>> unsigned int i, j, k;
>> + int max_vl;
>> + osm_subn_opt_t *opt = &p_lash->p_osm->subn.opt;
>>
>> OSM_LOG_ENTER(p_log);
>>
>> + max_vl = vl_min + opt->lash_start_vl;
>
> I'm not following...
>
> vl_min is total number of VLs available in a fabric, lash_start_vl is
> VL number where LASH may start to use VLs, so VLs available for LASH are
> lash_start_vl..vl_min. Assuming so what does 'max_vl = vl_min +
> lash_start_vl' represent? It is always overflow in terms of available
> VLs, no?
>
>> + if (max_vl > IB_MAX_NUM_VLS)
>> + max_vl = IB_MAX_NUM_VLS;
>> +
>> /* initialise cdg_vertex_matrix[num_switches][num_switches][num_switches] */
>> p_lash->cdg_vertex_matrix =
>> - (cdg_vertex_t ****) malloc(vl_min * sizeof(cdg_vertex_t ****));
>> - for (i = 0; i < vl_min; i++) {
>> + (cdg_vertex_t ****) calloc(max_vl, sizeof(cdg_vertex_t ****));
>> + for (i = opt->lash_start_vl; i < max_vl; i++) {
>> p_lash->cdg_vertex_matrix[i] =
>> (cdg_vertex_t ***) malloc(num_switches *
>> sizeof(cdg_vertex_t ***));
>> @@ -730,7 +746,7 @@ static int init_lash_structures(lash_t * p_lash)
>> goto Exit_Mem_Error;
>> }
>>
>> - for (i = 0; i < vl_min; i++) {
>> + for (i = opt->lash_start_vl; i < max_vl; i++) {
>> for (j = 0; j < num_switches; j++) {
>> p_lash->cdg_vertex_matrix[i][j] =
>> (cdg_vertex_t **) malloc(num_switches *
>> @@ -763,7 +779,7 @@ static int init_lash_structures(lash_t * p_lash)
>> for (i = 0; i < num_switches; i++) {
>> for (j = 0; j < num_switches; j++) {
>> p_lash->virtual_location[i][j] =
>> - (int *)malloc(vl_min * sizeof(int *));
>> + (int *)calloc(max_vl, sizeof(int *));
>> if (p_lash->virtual_location[i][j] == NULL)
>> goto Exit_Mem_Error;
>> for (k = 0; k < vl_min; k++) {
>> @@ -804,10 +820,12 @@ 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 */
>> + int max_vl;
>> + osm_subn_opt_t *opt = &p_lash->p_osm->subn.opt;
>>
>> OSM_LOG_ENTER(p_log);
>>
>> - if (p_lash->p_osm->subn.opt.do_mesh_analysis && osm_do_mesh_analysis(p_lash)) {
>> + if (opt->do_mesh_analysis && osm_do_mesh_analysis(p_lash)) {
>> OSM_LOG(p_log, OSM_LOG_ERROR, "Mesh analysis failed\n");
>> goto Exit;
>> }
>> @@ -838,11 +856,14 @@ 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++) {
>> + max_vl = lanes_needed + opt->lash_start_vl;
>> + if (max_vl > IB_MAX_NUM_VLS)
>> + max_vl = IB_MAX_NUM_VLS;
>
> Shouldn't comparison be done against lsh->vl_min (number VLs available
> in a fabric)? And return an error in case if it is exceeded (which means
> LASH requires more VLs than available)?
>
>> if (dest_switch != i && switch_bitmap[i * num_switches + dest_switch] == 0) {
>> - v_lane = 0;
>> + v_lane = opt->lash_start_vl;
>> stop = 0;
>> - while (v_lane < lanes_needed && stop == 0) {
>> + while (v_lane < max_vl && stop == 0) {
>> generate_cdg_for_sp(p_lash, i, dest_switch, v_lane);
>> generate_cdg_for_sp(p_lash, dest_switch, i, v_lane);
>>
>> @@ -906,7 +927,9 @@ static int lash_core(lash_t * p_lash)
>> switches[dest_switch]->routing_table[i].lane = v_lane;
>>
>> if (cycle_found == 1 || cycle_found2 == 1) {
>> - if (++lanes_needed > p_lash->vl_min)
>> + lanes_needed++;
>> + if (lanes_needed > p_lash->vl_min ||
>> + opt->lash_start_vl + lanes_needed - 1 >= IB_DROP_VL)
>
> Shouldn't be just
>
> if (start_vl + lanes_needed > p_lash->vl_min)
>
> ?
>
>> goto Error_Not_Enough_Lanes;
>>
>> generate_cdg_for_sp(p_lash, i, dest_switch, v_lane);
>> @@ -926,19 +949,24 @@ 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);
>>
>> - for (i = 0; i < lanes_needed; i++) {
>> + max_vl = lanes_needed + opt->lash_start_vl;
>> + if (max_vl > IB_MAX_NUM_VLS)
>
> Shouldn't this be compared with p_lash->vl_min (which is maximum number
> of VLs available in a fabric) and not with IB_MAX_NUM_VLS?
>
>> + max_vl = IB_MAX_NUM_VLS;
>
> When max_vl is larger than number of available VLs (IOW lash needs more
> VLs than fabric has) shouldn't it fallback to another routing algorithm?
>
>> +
>> + for (i = opt->lash_start_vl; i < max_vl; i++) {
>> OSM_LOG(p_log, OSM_LOG_INFO, "Lanes in layer %d: %d\n",
>> i, p_lash->num_mst_in_lane[i]);
>> }
>>
>> balance_virtual_lanes(p_lash, lanes_needed);
>>
>> - for (i = 0; i < lanes_needed; i++) {
>> + for (i = opt->lash_start_vl; i < max_vl; i++) {
>> OSM_LOG(p_log, OSM_LOG_INFO, "Lanes in layer %d: %d\n",
>> i, p_lash->num_mst_in_lane[i]);
>> }
>> @@ -1271,6 +1299,7 @@ uint8_t osm_get_lash_sl(osm_opensm_t * p_osm, const osm_port_t * p_src_port,
>> unsigned dst_id;
>> unsigned src_id;
>> osm_switch_t *p_sw;
>> + osm_subn_opt_t *opt = &p_osm->subn.opt;
>>
>> if (p_osm->routing_engine_used != OSM_ROUTING_ENGINE_TYPE_LASH)
>> return OSM_DEFAULT_SL;
>> @@ -1286,7 +1315,7 @@ uint8_t osm_get_lash_sl(osm_opensm_t * p_osm, const osm_port_t * p_src_port,
>>
>> src_id = get_lash_id(p_sw);
>> if (src_id == dst_id)
>> - return OSM_DEFAULT_SL;
>> + return opt->lash_start_vl;
>
> Is this correct? As far as I understand this is SL for paths between
> CA ports connected to the same switch (which should be safe in sense of
> credit loops). Should lash be involved here?
Is there a reason not to use the LASH SL for this ? I think it's more
in the spirit of LASH to do this.
All other comments addressed in v2 of patch to come shortly.
-- Hal
> Sasha
> _______________________________________________
> general mailing list
> general at lists.openfabrics.org
> http://lists.openfabrics.org/cgi-bin/mailman/listinfo/general
>
> To unsubscribe, please visit http://openib.org/mailman/listinfo/openib-general
>
More information about the general
mailing list