Re: [PATCH] opensm: Add initial support for optimized SLtoVLMappingTable programming

[Sasha Khapyorsky <sashak-smomgflXvOZWk0Htik3J/w@public.gmane.org>]

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 majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html