Hi Sasha, On Tue, Jun 23, 2009 at 5:49 PM, Sasha Khapyorsky<[email protected]> wrote: > Hi Hal, > > On 10:14 Thu 18 Jun , Hal Rosenstock wrote: >> >> rather than starting from VL 0 >> >> Signed-off-by: Robert Pearson <[email protected]> >> Signed-off-by: Hal Rosenstock <[email protected]> > > 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 > [email protected] > http://lists.openfabrics.org/cgi-bin/mailman/listinfo/general > > To unsubscribe, please visit http://openib.org/mailman/listinfo/openib-general > _______________________________________________ general mailing list [email protected] http://lists.openfabrics.org/cgi-bin/mailman/listinfo/general To unsubscribe, please visit http://openib.org/mailman/listinfo/openib-general
