[ofa-general] Re: [PATCHv2] opensm/osm_port_info_rcv.c: Remove SM from sm_guid_tbl when IsSM is not

[Sasha Khapyorsky]

Hi Hal,

On 11:55 Wed 19 Nov     , Hal Rosenstock wrote:
> Sasha,
>
> The following patch removes the SM from the sm_guid_table when IsSM is not 
> present.
> v2 of this fixes the memory leak you pointed out in the original version. 
> Compile tested only as I don't have an environment to recreate this 
> anymore.
>
> -- Hal
>

> opensm/osm_port_info_rcv.c: Remove SM from sm_guid_tbl when IsSM is 
> not present in PortInfo:CapabilityMask
> 
> SM should be removed from the sm_guid_tbl subsequent to a trap 144
> indicating the capability mask changed (and the new capabilities
> no longer include IsSM).
> 
> As a result of this, move clearing of SM state to be conditionalized on 
> IsSM present rather than regardless of whether IsSM is set.
> 
> Prior to this patch, the OpenSM log is spammed with error messages on
> SubnGets of SMInfo attribute.
> 
> Signed-off-by: Hal Rosenstock <hal.rosenstock at gmail.com>
> ---
> v2 fixes memory leak pointed out by Sasha.
> 
> diff --git a/opensm/opensm/osm_port_info_rcv.c b/opensm/opensm/osm_port_info_rcv.c
> index 47eb457..5988dc3 100644
> --- a/opensm/opensm/osm_port_info_rcv.c
> +++ b/opensm/opensm/osm_port_info_rcv.c
> @@ -149,17 +149,17 @@ __osm_pi_rcv_process_endport(IN osm_sm_t * sm,
>  			 */
>  			__osm_pi_rcv_set_sm(sm, p_physp);
>  	} else {
> -		/*
> -		   Before querying the SM - we want to make sure we clean its state, so
> -		   if the querying fails we recognize that this SM is not active.
> -		 */
>  		p_sm_tbl = &sm->p_subn->sm_guid_tbl;
> -		p_sm = (osm_remote_sm_t *) cl_qmap_get(p_sm_tbl, port_guid);
> -		if (p_sm != (osm_remote_sm_t *) cl_qmap_end(p_sm_tbl))
> -			/* clean it up */
> -			p_sm->smi.pri_state = 0xF0 & p_sm->smi.pri_state;
> -
>  		if (p_pi->capability_mask & IB_PORT_CAP_IS_SM) {
> +			/*
> +			 * Before querying the SM - we want to make sure we
> +			 * clean its state, so if the querying fails we
> +			 * recognize that this SM is not active.
> +			 */
> +			p_sm = (osm_remote_sm_t *) cl_qmap_get(p_sm_tbl, port_guid);
> +			if (p_sm != (osm_remote_sm_t *) cl_qmap_end(p_sm_tbl))
> +				/* clean it up */
> +				p_sm->smi.pri_state = 0xF0 & p_sm->smi.pri_state;
>  			if (sm->p_subn->opt.ignore_other_sm)
>  				OSM_LOG(sm->p_log, OSM_LOG_VERBOSE,
>  					"Ignoring SM on port 0x%" PRIx64 "\n",
> @@ -171,7 +171,8 @@ __osm_pi_rcv_process_endport(IN osm_sm_t * sm,
>  					cl_ntoh64(port_guid));
>  
>  				/*
> -				   This port indicates it's an SM and it's not our own port.
> +				   This port indicates it's an SM and
> +				   it's not our own port.
>  				   Acquire the SMInfo Attribute.
>  				 */
>  				memset(&context, 0, sizeof(context));
> @@ -190,6 +191,9 @@ __osm_pi_rcv_process_endport(IN osm_sm_t * sm,
>  						"Failure requesting SMInfo (%s)\n",
>  						ib_get_err_str(status));
>  			}
> +		} else {
> +			p_sm = (osm_remote_sm_t *) cl_qmap_remove(p_sm_tbl, port_guid);
> +			free(p_sm);

Sorry about my simplified example. Actually it should be:

	if (p_sm != (osm_remote_sm_t *) cl_qmap_end(p_sm_tbl))
		free(p_sm);

Since many ports may not have IsSM bit.

The patch is applied with this fix. Thanks.

Sasha