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

[Sasha Khapyorsky]

On 13:58 Sun 23 Nov     , Yevgeny Kliteynik wrote:
> 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?

Maybe, but I'm not sure - this is more for bug#1401 materials :),
basically I would expect subnet_initialization_error flag setup when LFT
Set fails.

>
>> +		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?

Yes.

Sasha

>
> -- 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++) {
>