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

[Sasha Khapyorsky]

Hi Hal,

On 09:12 Tue 18 Nov     , Hal Rosenstock wrote:
> Sasha,
>
> The following patch (attached this time:-) removes the SM from the 
> sm_guid_table when IsSM is not present. 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>
> 
> diff --git a/opensm/opensm/osm_port_info_rcv.c b/opensm/opensm/osm_port_info_rcv.c
> index 47eb457..97ec5b3 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,7 +191,8 @@ __osm_pi_rcv_process_endport(IN osm_sm_t * sm,
>  						"Failure requesting SMInfo (%s)\n",
>  						ib_get_err_str(status));
>  			}
> -		}
> +		} else
> +			cl_qmap_remove(p_sm_tbl, port_guid);

Isn't it should be freed too? Something like:

	p_sm = cl_qmap_remove(p_sm_tbl, port_guid);
	free(p_sm);

Sasha

>  	}
>  
>  	OSM_LOG_EXIT(sm->p_log);