[ofa-general] Re: [PATCHv3] opensm: Parallelize (Stripe) LFT sets across switches

[Sasha Khapyorsky]

Hi Hal,

On 07:08 Fri 07 Aug     , Hal Rosenstock wrote:
> 
> Signed-off-by: Hal Rosenstock <hal.rosenstock at gmail.com>

I'm applying this patch as it is, but have couple of comments below.
Actually I even prepared patches over those comment and will push it to
the list soon, please review.

> diff --git a/opensm/opensm/osm_ucast_mgr.c b/opensm/opensm/osm_ucast_mgr.c
> index 78a7031..e28752a 100644
> --- a/opensm/opensm/osm_ucast_mgr.c
> +++ b/opensm/opensm/osm_ucast_mgr.c

[snip...]

> @@ -516,6 +471,101 @@ static void ucast_mgr_process_tbl(IN cl_map_item_t * p_map_item,
>  	OSM_LOG_EXIT(p_mgr->p_log);
>  }
>  
> +static void ucast_mgr_process_top(IN cl_map_item_t * p_map_item,
> +				  IN void *context)
> +{
> +	osm_ucast_mgr_t *p_mgr = context;
> +	osm_switch_t *const p_sw = (osm_switch_t *) p_map_item;
> +
> +	set_fwd_tbl_top(p_mgr, p_sw);
> +}
> +
> +static boolean_t set_next_lft_block(IN osm_switch_t * p_sw, IN osm_sm_t * p_sm,
> +				    IN uint8_t * p_block,
> +				    IN osm_dr_path_t * p_path,
> +				    IN uint16_t block_id_ho,
> +				    IN osm_madw_context_t * p_context)
> +{
> +	ib_api_status_t status;
> +	boolean_t sts;
> +
> +	OSM_LOG_ENTER(p_sm->p_log);
> +
> +	for (;
> +	     (sts = osm_switch_get_lft_block(p_sw, block_id_ho, p_block));
> +	     block_id_ho++) {
> +		if (!p_sw->need_update && !p_sm->p_subn->need_update &&
> +		    !memcmp(p_block,
> +			    p_sw->new_lft + block_id_ho * IB_SMP_DATA_SIZE,
> +			    IB_SMP_DATA_SIZE))
> +			continue;

This function is called in loop with block number incremented. Inside it
loops by itself in looking for changed block, caller will repeat this
looping again and again. It would be really nice to avoid such useless
action. I prepared the patch, please review.

> @@ -940,6 +1025,9 @@ static int ucast_mgr_route(struct osm_routing_engine *r, osm_opensm_t * osm)
>  
>  	osm->routing_engine_used = osm_routing_engine_type(r->name);
>  
> +	if (r->ucast_build_fwd_tables)
> +		osm_ucast_mgr_set_fwd_table(&osm->sm.ucast_mgr);
> +

Any reason to not simplify (and unify) fwd table decision flow over
routing engines with and without ucast_build_fwd_tables method?

The patch to follow.

Sasha