[ofa-general] [PATCH] opensm: sweep component processors return status value

[Sasha Khapyorsky]

Hi Hal,

On 13:47 Mon 22 Jun     , Hal Rosenstock wrote:
> 
> Some possible simplifications and some other comments below.

Thanks for the comments.

> > @@ -453,20 +451,20 @@ static osm_signal_t link_mgr_process_node(osm_sm_t * sm, IN osm_node_t * p_node,
> > ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ??p_physp->port_num,
> > ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ??ib_get_port_state_str(current_state));
> > ?? ?? ?? ?? ?? ?? ?? ??else if (link_mgr_set_physp_pi(sm, p_physp, link_state))
> > - ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? signal = OSM_SIGNAL_DONE_PENDING;
> > + ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ret = -1;
> 
> Simpler as:
> else ret = link_mgr_set_physp_pi() ?

This function is called in loop for each node port:

	for (i = 0; i < num_physp; i++)

, so if link_mgr_set_physp_pi() will fail for one or more ports the
return status will remain -1.

In your simplification it can be overwritten to '0' by following
successful runs.

> > @@ -476,14 +474,12 @@ osm_signal_t osm_link_mgr_process(osm_sm_t * sm, IN const uint8_t link_state)
> >
> > ?? ?? ?? ??for (p_node = (osm_node_t *) cl_qmap_head(p_node_guid_tbl);
> > ?? ?? ?? ?? ?? ?? p_node != (osm_node_t *) cl_qmap_end(p_node_guid_tbl);
> > - ?? ?? ?? ?? ?? ??p_node = (osm_node_t *) cl_qmap_next(&p_node->map_item)) {
> > - ?? ?? ?? ?? ?? ?? ?? if (link_mgr_process_node(sm, p_node, link_state) ==
> > - ?? ?? ?? ?? ?? ?? ?? ?? ?? OSM_SIGNAL_DONE_PENDING)
> > - ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? signal = OSM_SIGNAL_DONE_PENDING;
> > - ?? ?? ?? }
> > + ?? ?? ?? ?? ?? ??p_node = (osm_node_t *) cl_qmap_next(&p_node->map_item))
> > + ?? ?? ?? ?? ?? ?? ?? if (link_mgr_process_node(sm, p_node, link_state))
> > + ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ret = -1;
> 
> Simpler as:
> ret = link_mgr_process_node() ?

Same as above, but in this case the loop is for each node.

> > @@ -1142,9 +1139,8 @@ osm_signal_t osm_mcast_mgr_process(osm_sm_t * sm)
> > ?? ?? ?? ?? */
> > ?? ?? ?? ??p_sw = (osm_switch_t *) cl_qmap_head(p_sw_tbl);
> > ?? ?? ?? ??while (p_sw != (osm_switch_t *) cl_qmap_end(p_sw_tbl)) {
> > - ?? ?? ?? ?? ?? ?? ?? signal = mcast_mgr_set_tbl(sm, p_sw);
> > - ?? ?? ?? ?? ?? ?? ?? if (signal == OSM_SIGNAL_DONE_PENDING)
> > - ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? pending_transactions = TRUE;
> > + ?? ?? ?? ?? ?? ?? ?? if (mcast_mgr_set_tbl(sm, p_sw))
> > + ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ret = -1;
> 
> Simpler as:
> ret = mcast_mgr_set_tbl() ?

Same as above.

> > @@ -396,21 +394,20 @@ pkey_mgr_update_peer_port(osm_log_t * p_log, osm_sm_t * sm,
> > ?? ?? ?? ??uint16_t num_of_blocks;
> > ?? ?? ?? ??uint16_t peer_max_blocks;
> > ?? ?? ?? ??ib_api_status_t status = IB_SUCCESS;
> > - ?? ?? ?? boolean_t ret_val = FALSE;
> > - ?? ?? ?? boolean_t port_info_set = FALSE;
> > ?? ?? ?? ??ib_pkey_table_t empty_block;
> > + ?? ?? ?? int ret = 0;
> >
> > ?? ?? ?? ??memset(&empty_block, 0, sizeof(ib_pkey_table_t));
> >
> > ?? ?? ?? ??p_physp = p_port->p_physp;
> > ?? ?? ?? ??if (!p_physp)
> > - ?? ?? ?? ?? ?? ?? ?? return FALSE;
> > + ?? ?? ?? ?? ?? ?? ?? return -1;
> > ?? ?? ?? ??peer = osm_physp_get_remote(p_physp);
> > ?? ?? ?? ??if (!peer)
> > - ?? ?? ?? ?? ?? ?? ?? return FALSE;
> > + ?? ?? ?? ?? ?? ?? ?? return -1;
> > ?? ?? ?? ??p_node = osm_physp_get_node_ptr(peer);
> > ?? ?? ?? ??if (!p_node->sw || !p_node->sw->switch_info.enforce_cap)
> > - ?? ?? ?? ?? ?? ?? ?? return FALSE;
> > + ?? ?? ?? ?? ?? ?? ?? return 0;
> 
> Why are the FALSEs above changed inconsistently ?

It is because previously this function returned an indication that
something was sent or not, regardless to the reason - errors or not.
Now it returns the status of success including the case when no MADs
sending is needed.

> > @@ -424,13 +421,14 @@ pkey_mgr_update_peer_port(osm_log_t * p_log, osm_sm_t * sm,
> > ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ??cl_ntoh64(osm_node_get_node_guid(p_node)),
> > ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ??osm_physp_get_port_num(peer));
> > ?? ?? ?? ?? ?? ?? ?? ??enforce = FALSE;
> > + ?? ?? ?? ?? ?? ?? ?? ret = -1;
> > ?? ?? ?? ??}
> >
> > ?? ?? ?? ??if (pkey_mgr_enforce_partition(p_log, sm, peer, enforce))
> > - ?? ?? ?? ?? ?? ?? ?? port_info_set = TRUE;
> > + ?? ?? ?? ?? ?? ?? ?? ret = -1;
> 
> Simpler as:
> ret = pkey_mgr_enforce_partition() ?

Similar to a loop cases, such statement may overwrite ret value if it
indicates previous errors.

> > @@ -512,17 +508,17 @@ osm_signal_t osm_pkey_mgr_process(IN osm_opensm_t * p_osm)
> > ?? ?? ?? ?? ?? ?? ?? ??p_port = (osm_port_t *) p_next;
> > ?? ?? ?? ?? ?? ?? ?? ??p_next = cl_qmap_next(p_next);
> > ?? ?? ?? ?? ?? ?? ?? ??if (pkey_mgr_update_port(&p_osm->log, &p_osm->sm, p_port))
> > - ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? signal = OSM_SIGNAL_DONE_PENDING;
> > + ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ret = -1;
> 
> Simpler as:
> ret = pkey_mgr_update_port() ?

This is a loop case.

> 
> > ?? ?? ?? ?? ?? ?? ?? ??if ((osm_node_get_type(p_port->p_node) != IB_NODE_TYPE_SWITCH)
> > ?? ?? ?? ?? ?? ?? ?? ?? ?? ??&& pkey_mgr_update_peer_port(&p_osm->log, &p_osm->sm,
> > ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? &p_osm->subn, p_port,
> > ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? !p_osm->subn.opt.
> > ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? no_partition_enforcement))
> > - ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? signal = OSM_SIGNAL_DONE_PENDING;
> > + ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ret = -1;
> 
> Simpler as:
> if (osm_node_get_type())
> ret = pkey_mgr_update_peer_port() ?

Ditto.

> > @@ -305,10 +305,10 @@ osm_signal_t osm_qos_setup(osm_opensm_t * p_osm)
> > ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ??continue;
> > ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ??force_update = p_physp->need_update ||
> > ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ??p_osm->subn.need_update;
> > - ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? status =
> > - ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? qos_physp_setup(&p_osm->log, &p_osm->sm,
> > + ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? if (qos_physp_setup(&p_osm->log, &p_osm->sm,
> > ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ??p_port, p_physp, i,
> > - ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? force_update, &swe_config);
> > + ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? force_update, &swe_config))
> > + ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ret = -1;
> 
> Simpler as:
> ret = qos_physp_setup() ?
> 
> > ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ??}
> > ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ??/* skip base port 0 */
> > ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ??if (!ib_switch_info_is_enhanced_port0
> > @@ -326,14 +326,15 @@ osm_signal_t osm_qos_setup(osm_opensm_t * p_osm)
> > ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ??continue;
> >
> > ?? ?? ?? ?? ?? ?? ?? ??force_update = p_physp->need_update || p_osm->subn.need_update;
> > - ?? ?? ?? ?? ?? ?? ?? status = qos_physp_setup(&p_osm->log, &p_osm->sm, p_port,
> > - ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ??p_physp, 0, force_update, cfg);
> > + ?? ?? ?? ?? ?? ?? ?? if (qos_physp_setup(&p_osm->log, &p_osm->sm, p_port, p_physp,
> > + ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? 0, force_update, cfg))
> > + ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ret = -1;
> 
> Simpler as:
> ret = qos_physp_setup() ?

Same as above - it is under loop.

Sasha