On 11:40 Fri 30 Oct , Hal Rosenstock wrote:
> >> @@ -150,7 +151,7 @@ static ib_api_status_t vlarb_update(osm_sm_t * sm,
> >> osm_physp_t * p,
> >>
> >> static ib_api_status_t sl2vl_update_table(osm_sm_t * sm, osm_physp_t * p,
> >> uint8_t in_port, uint8_t out_port,
> >> - unsigned force_update,
> >> + unsigned optimize, unsigned
> >> force_update,
> >> const ib_slvl_table_t *
> >> sl2vl_table)
> >> {
> >> osm_madw_context_t context;
> >> @@ -177,10 +178,18 @@ static ib_api_status_t sl2vl_update_table(osm_sm_t *
> >> sm, osm_physp_t * p,
> >> !memcmp(p_tbl, &tbl, sizeof(tbl)))
> >> return IB_SUCCESS;
> >>
> >> + /* both input port and output port wildcarded */
> >> + if (optimize && (in_port != 1 || out_port != 1))
> >> + return IB_SUCCESS;
> >> +
> >
> > This function (sl2vl_update_table()) is called for each in and out ports
> > combination. I think that instead of calling it N * N times and
> > filtering out the case 'in == out == 1' you just need it once somewhere
> > at higher layer.
>
> I was trying to minimize the changes to the current code flow. It can
> be done the way you propose but that will cause larger changes. Are
> you sure about this direction ?
I'm sure that that we don't need a bunch of empty loops. Also I think
that doing this on an appropriate layer will not complicate things more.
> >> @@ -72,7 +73,9 @@ void osm_slvl_rcv_process(IN void *context, IN void
> >> *p_data)
> >> osm_slvl_context_t *p_context;
> >> ib_net64_t port_guid;
> >> ib_net64_t node_guid;
> >> - uint8_t out_port_num, in_port_num;
> >> + uint32_t attr_mod;
> >> + uint8_t out_port_num, in_port_num, startinport, startoutport,
> >> + endinport, endoutport;
> >>
> >> CL_ASSERT(sm);
> >>
> >> @@ -111,6 +114,9 @@ void osm_slvl_rcv_process(IN void *context, IN void
> >> *p_data)
> >> (uint8_t) cl_ntoh32(p_smp->attr_mod & 0xFF000000);
> >> in_port_num =
> >> (uint8_t) cl_ntoh32((p_smp->attr_mod & 0x00FF0000) << 8);
> >> + attr_mod = cl_ntoh32(p_smp->attr_mod);
> >> + if (attr_mod & 0x30000)
> >> + goto opt_sl2vl;
> >> p_physp = osm_node_get_physp_ptr(p_node, out_port_num);
> >> } else {
> >> p_physp = p_port->p_physp;
> >> @@ -123,7 +129,7 @@ void osm_slvl_rcv_process(IN void *context, IN void
> >> *p_data)
> >> all we want is to update the subnet.
> >> */
> >> OSM_LOG(sm->p_log, OSM_LOG_VERBOSE,
> >> - "Got SLtoVL get response in_port_num %u out_port_num %u with
> >> "
> >> + "Received SLtoVL GetResp in_port_num %u out_port_num %u with
> >> "
> >> "GUID 0x%" PRIx64 " for parent node GUID 0x%" PRIx64 ", TID
> >> 0x%"
> >> PRIx64 "\n", in_port_num, out_port_num, cl_ntoh64(port_guid),
> >> cl_ntoh64(node_guid), cl_ntoh64(p_smp->trans_id));
> >> @@ -142,6 +148,39 @@ void osm_slvl_rcv_process(IN void *context, IN void
> >> *p_data)
> >> out_port_num, p_slvl_tbl, OSM_LOG_DEBUG);
> >>
> >> osm_physp_set_slvl_tbl(p_physp, p_slvl_tbl, in_port_num);
> >> + goto Exit;
> >> +
> >> +opt_sl2vl:
> >
> > I think that you need to use functions here.
>
> Where is here ? To what are you referring ?
When your function becomes to form of:
if (cond1)
goto do1;
do_else();
goto exit;
do1:
do_1();
exit:
return;
I'm suggesting to improve flows, for example by using functions (or
another way if applicable and achieves the simplification goal).
>
> >> + OSM_LOG(sm->p_log, OSM_LOG_VERBOSE,
> >> + "Got optimized SLtoVL get response in_port_num %u
> >> out_port_num "
> >> + "%u with GUID 0x%" PRIx64 " for parent node GUID 0x%" PRIx64
> >> + ", TID 0x%" PRIx64 "\n", in_port_num, out_port_num,
> >> + cl_ntoh64(port_guid), cl_ntoh64(node_guid),
> >> + cl_ntoh64(p_smp->trans_id));
> >> +
> >> + osm_dump_slvl_map_table(sm->p_log, port_guid, in_port_num,
> >> + out_port_num, p_slvl_tbl, OSM_LOG_DEBUG);
> >> +
> >> + if (attr_mod & 0x10000) {
> >> + startoutport =
> >> ib_switch_info_is_enhanced_port0(&p_node->sw->switch_info) ? 0 : 1;
> >> + endoutport = osm_node_get_num_physp(p_node);
> >> + } else
> >> + endoutport = startoutport = out_port_num;
> >> + if (attr_mod & 0x20000) {
> >> + startinport =
> >> ib_switch_info_is_enhanced_port0(&p_node->sw->switch_info) ? 0 : 1;
> >> + endinport = osm_node_get_num_physp(p_node);
> >> + } else
> >> + endinport = startinport = in_port_num;
> >> +
> >> + for (out_port_num = startoutport; out_port_num < endoutport;
> >> + out_port_num++) {
> >> + p_physp = osm_node_get_physp_ptr(p_node, out_port_num);
> >> + if (!p_physp)
> >
> > When could this be NULL?
>
> It was just defensive. Should it be removed ?
I wasn't sure, so asked. If it is "just defensive" please remove it.
Sasha
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
