[ofa-general] Re: [PATCH 1/2] opensm: replace switch's fwd_tbl with simple LFT

[Sasha Khapyorsky]

On 13:14 Wed 29 Oct     , Yevgeny Kliteynik wrote:
> Sasha Khapyorsky wrote:
>> Hi Yevgeny,
>> On 23:36 Thu 16 Oct     , Yevgeny Kliteynik wrote:
>>> Replace the unnecessarily complex switch's forwarding table
>>> implementation with a simple LFT that is implemented as plain
>>> uint8_t array.
>>>
>>> Signed-off-by: Yevgeny Kliteynik <kliteyn at dev.mellanox.co.il>
>>> ---
>> [snip...]
>>> diff --git a/opensm/opensm/osm_switch.c b/opensm/opensm/osm_switch.c
>>> index 9bf76e0..bdfc7d0 100644
>>> --- a/opensm/opensm/osm_switch.c
>>> +++ b/opensm/opensm/osm_switch.c
>>> @@ -97,9 +97,26 @@ osm_switch_init(IN osm_switch_t * const p_sw,
>>>  	p_sw->num_ports = num_ports;
>>>  	p_sw->need_update = 2;
>>>
>>> -	status = osm_fwd_tbl_init(&p_sw->fwd_tbl, p_si);
>>> -	if (status != IB_SUCCESS)
>>> +	/* Initiate the linear forwarding table */
>>> +
>>> +	if (!p_si->lin_cap) {
>>> +		/* This switch does not support linear forwarding tables */
>>> +		status = IB_UNSUPPORTED;
>>>  		goto Exit;
>>> +	}
>>> +
>>> +	/* The capacity reported by the switch includes LID 0,
>>> +	   so add 1 to the end of the range here for this assert. */
>>> +        CL_ASSERT(cl_ntoh16(p_si->lin_cap) <= IB_LID_UCAST_END_HO + 1);
>> Maybe there should be run-time check (not sure since lin_cap is not
>> really used in other places in the code), but not assertion - any bogus
>> data received from network should not crash OpenSM. I'm removing this.
>
> Do we care that the lin_cap of the switch claims to support more
> than IB_LID_UCAST_END_HO? Don't think so, so I agree - removing this.

It means buggy switch, we can drop a warning in run-time, but this is
not a reason for OpenSM to crash with CL_ASSERT().

>>> +
>>> +	p_sw->lft = malloc(IB_LID_UCAST_END_HO + 1);
>>> +	if (!p_sw->lft) {
>>> +		status = IB_INSUFFICIENT_MEMORY;
>>> +		goto Exit;
>>> +	}
>>> +
>>> +	/* 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) {
>>> @@ -138,7 +155,8 @@ void osm_switch_delete(IN OUT osm_switch_t ** const 
>>> pp_sw)
>>>
>>>  	osm_mcast_tbl_destroy(&p_sw->mcast_tbl);
>>>  	free(p_sw->p_prof);
>>> -	osm_fwd_tbl_destroy(&p_sw->fwd_tbl);
>>> +	if (p_sw->lft)
>>> +		free(p_sw->lft);
>>>  	if (p_sw->lft_buf)
>>>  		free(p_sw->lft_buf);
>>>  	if (p_sw->hops) {
>>> @@ -176,44 +194,36 @@ osm_switch_t *osm_switch_new(IN osm_node_t * const 
>>> p_node,
>>>  /**********************************************************************
>>>   **********************************************************************/
>>>  boolean_t
>>> -osm_switch_get_fwd_tbl_block(IN const osm_switch_t * const p_sw,
>>> -			     IN const uint32_t block_id,
>>> -			     OUT uint8_t * const p_block)
>>> +osm_switch_get_lft_block(IN const osm_switch_t * const p_sw,
>>> +			 IN const uint32_t block_id,
>>> +			 OUT uint8_t * const p_block)
>>>  {
>>>  	uint16_t base_lid_ho;
>>> -	uint16_t max_lid_ho;
>>> -	uint16_t lid_ho;
>>>  	uint16_t block_top_lid_ho;
>>> -	uint32_t lids_per_block;
>>> -	osm_fwd_tbl_t *p_tbl;
>>>  	boolean_t return_flag = FALSE;
>>>
>>>  	CL_ASSERT(p_sw);
>>>  	CL_ASSERT(p_block);
>>>
>>> -	p_tbl = osm_switch_get_fwd_tbl_ptr(p_sw);
>>> -	max_lid_ho = p_sw->max_lid_ho;
>>> -	lids_per_block = osm_fwd_tbl_get_lids_per_block(&p_sw->fwd_tbl);
>>> -	base_lid_ho = (uint16_t) (block_id * lids_per_block);
>>> +	base_lid_ho = (uint16_t) (block_id * IB_SMP_DATA_SIZE);
>>>
>>> -	if (base_lid_ho <= max_lid_ho) {
>>> +	if (base_lid_ho <= p_sw->max_lid_ho) {
>>>  		/* Initialize LIDs in block to invalid port number. */
>>>  		memset(p_block, OSM_NO_PATH, IB_SMP_DATA_SIZE);
>>>  		/*
>>>  		   Determine the range of LIDs we can return with this block.
>>>  		 */
>>>  		block_top_lid_ho =
>>> -		    (uint16_t) (base_lid_ho + lids_per_block - 1);
>>> -		if (block_top_lid_ho > max_lid_ho)
>>> -			block_top_lid_ho = max_lid_ho;
>>> +		    (uint16_t) (base_lid_ho + IB_SMP_DATA_SIZE - 1);
>>> +		if (block_top_lid_ho > p_sw->max_lid_ho)
>>> +			block_top_lid_ho = p_sw->max_lid_ho;
>>>
>>>  		/*
>>>  		   Configure the forwarding table with the routing
>>>  		   information for the specified block of LIDs.
>>>  		 */
>>> -		for (lid_ho = base_lid_ho; lid_ho <= block_top_lid_ho; lid_ho++)
>>> -			p_block[lid_ho - base_lid_ho] =
>>> -			    osm_fwd_tbl_get(p_tbl, lid_ho);
>>> +		memcpy(p_block, &(p_sw->lft[base_lid_ho]),
>>> +		       block_top_lid_ho - base_lid_ho + 1);
>> Hmm, why not just
>> 	memcpy(p_block, &p_sw->lft[base_lid_ho], 64);
>> ? And then no need initial memset()?
>
> Well, I can really simplify this whole function to
> something like this:
>
> boolean_t
> osm_switch_get_lft_block(IN const osm_switch_t * const p_sw,
> 			 IN const uint16_t block_id,
> 			 OUT uint8_t * const p_block)
> {
> 	uint16_t base_lid_ho = block_id * IB_SMP_DATA_SIZE;
> 	CL_ASSERT(p_sw);
> 	CL_ASSERT(p_block);
> 	if (base_lid_ho > p_sw->max_lid_ho)
> 		return FALSE;
> 	memcpy(p_block, &(p_sw->lft[base_lid_ho]), IB_SMP_DATA_SIZE);
> 	return TRUE;
> }

Good.

> Patch shortly.

Thanks.

Sasha