[ofa-general] Re: [PATCH v2] opensm: free lft_buf if it matches switch's lft

[Yevgeny Kliteynik]

Hi Sasha,

Sasha Khapyorsky wrote:
> Hi Yevgeny,
> 
> On 14:46 Thu 20 Nov     , Yevgeny Kliteynik wrote:
>> I can do something like the following patch, but I have
>> some strange feeling that I'm missing something...
> 
> I cannot see any errors here. But probably you can use simpler approach
> - just cleanup all switch's lft_buf separately after ucast_mgr is
> finished (including wait_for_pending_transactions()). Something like
> below (if it is fine for you I can just apply this patch).

In general, looks good. See below.

> BTW, what about to rename lft_buf to new_lft (to improve readability)?

Sure, why not.

> Sasha
> 
> 
> diff --git a/opensm/opensm/osm_state_mgr.c b/opensm/opensm/osm_state_mgr.c
> index 56212fe..c810106 100644
> --- a/opensm/opensm/osm_state_mgr.c
> +++ b/opensm/opensm/osm_state_mgr.c
> @@ -1001,6 +1001,23 @@ static void __osm_state_mgr_check_tbl_consistency(IN osm_sm_t * sm)
>  	OSM_LOG_EXIT(sm->p_log);
>  }
>  
> +static void cleanup_switch(cl_map_item_t *item, void *log)
> +{
> +	osm_switch_t *sw = (osm_switch_t *)item;
> +
> +	if (!sw->lft_buf)
> +		return;
> +	
> +	if (memcmp(sw->lft, sw->lft_buf, IB_LID_UCAST_END_HO + 1))

Should it turn on the p_subn->subnet_initialization_error flag?

> +		osm_log(log, OSM_LOG_ERROR, "ERR 331D: "
> +			"LFT of switch 0x%016" PRIx64 " is not up to date.\n",
> +			cl_ntoh64(sw->p_node->node_info.node_guid));
> +	else {
> +		free(sw->lft_buf);
> +		sw->lft_buf = NULL;
> +	}
> +}
> +
>  /**********************************************************************
>   **********************************************************************/
>  int wait_for_pending_transactions(osm_stats_t * stats)
> @@ -1254,6 +1271,9 @@ _repeat_discovery:
>  	if (wait_for_pending_transactions(&sm->p_subn->p_osm->stats))
>  		return;
>  
> +	/* cleanup switch lft buffers */
> +	cl_qmap_apply_func(&sm->p_subn->sw_guid_tbl, cleanup_switch, sm->p_log);
> +
>  	/* We are done setting all LFTs so clear the ignore existing.
>  	 * From now on, as long as we are still master, we want to
>  	 * take into account these lfts. */
> diff --git a/opensm/opensm/osm_switch.c b/opensm/opensm/osm_switch.c
> index 642dcd1..c446f4f 100644
> --- a/opensm/opensm/osm_switch.c
> +++ b/opensm/opensm/osm_switch.c
> @@ -114,13 +114,6 @@ osm_switch_init(IN osm_switch_t * const p_sw,
>  	/* Initialize the table to OSM_NO_PATH, which is "invalid port" */
>  	memset(p_sw->lft, OSM_NO_PATH, IB_LID_UCAST_END_HO + 1);
>  
> -	p_sw->lft_buf = malloc(IB_LID_UCAST_END_HO + 1);
> -	if (!p_sw->lft_buf) {
> -		status = IB_INSUFFICIENT_MEMORY;
> -		goto Exit;
> -	}
> -	memset(p_sw->lft_buf, OSM_NO_PATH, IB_LID_UCAST_END_HO + 1);
> -

This part is relevant even w/o the rest of the patch, right?

-- Yevgeny

>  	p_sw->p_prof = malloc(sizeof(*p_sw->p_prof) * num_ports);
>  	if (p_sw->p_prof == NULL) {
>  		status = IB_INSUFFICIENT_MEMORY;
> diff --git a/opensm/opensm/osm_ucast_mgr.c b/opensm/opensm/osm_ucast_mgr.c
> index 1409e15..3d47640 100644
> --- a/opensm/opensm/osm_ucast_mgr.c
> +++ b/opensm/opensm/osm_ucast_mgr.c
> @@ -397,13 +397,6 @@ int osm_ucast_mgr_set_fwd_table(IN osm_ucast_mgr_t * const p_mgr,
>  		goto Exit;
>  	}
>  
> -	if (!p_sw->need_update &&
> -	    !memcmp(p_sw->lft, p_sw->lft_buf, IB_LID_UCAST_END_HO + 1)) {
> -		free(p_sw->lft_buf);
> -		p_sw->lft_buf = NULL;
> -		goto Exit;
> -	}
> -
>  	for (block_id_ho = 0;
>  	     osm_switch_get_lft_block(p_sw, block_id_ho, block);
>  	     block_id_ho++) {
>