[ofa-general] Re: [PATCH] osm: QoS: selecting PathRecord according to QoS policy

Yevgeny Kliteynik kliteyn at dev.mellanox.co.il
Wed Sep 5 07:59:30 PDT 2007


Sasha Khapyorsky wrote:
> Hi Yevgeny,
> 
> On 10:53 Wed 05 Sep     , Yevgeny Kliteynik wrote:
>>>>  	ib_net16_t dest_lid;
>>>> +	uint8_t i;
>>>> +	uint8_t vl;
>>>> +	ib_slvl_table_t *p_slvl_tbl = NULL;
>>>> +	boolean_t valid_sls[IB_MAX_NUM_VLS];
>>> Use here uint16_t sl_mask instead of array - flow will be simpler.
>>  No, it won't.
>>  It will save three lines in the end when checking whether there is
>>  a valid sl that doesn't lead to VL15,
> 
> It saves loop, not just three lines :)
> 
>> but it will compilcate a bit
>>  rest of the related code, because I still need to read port's SL2VL
>>  table values one by one and mark them in the array (or bitmap) one
>>  by one.
> 
> Right, but since (!sl_mask) check is cheap you are able to stop PR
> generation at the moment when no valid SLs exist. Just look at the
> patch (against recent PR code):
> 
> 
> diff --git a/opensm/opensm/osm_sa_path_record.c b/opensm/opensm/osm_sa_path_record.c
> index edfa15f..1c6532b 100644
> --- a/opensm/opensm/osm_sa_path_record.c
> +++ b/opensm/opensm/osm_sa_path_record.c
> @@ -253,16 +253,12 @@ __osm_pr_rcv_get_path_parms(IN osm_pr_rcv_t * const p_rcv,
>  	uint8_t in_port_num;
>  	ib_net16_t dest_lid;
>  	uint8_t i;
> -	uint8_t vl;
>  	ib_slvl_table_t *p_slvl_tbl = NULL;
> -	boolean_t valid_sls[IB_MAX_NUM_VLS];
> -	boolean_t sl2vl_valid_path;
> -	uint8_t first_valid_sl;
> +	uint16_t sl_mask = 0xffff;
>  	osm_qos_level_t *p_qos_level = NULL;
>  
>  	OSM_LOG_ENTER(p_rcv->p_log, __osm_pr_rcv_get_path_parms);
>  
> -	memset(valid_sls, TRUE, IB_MAX_NUM_VLS);
>  	dest_lid = cl_hton16(dest_lid_ho);
>  
>  	p_dest_physp = p_dest_port->p_physp;
> @@ -328,12 +324,18 @@ __osm_pr_rcv_get_path_parms(IN osm_pr_rcv_t * const p_rcv,
>  			p_slvl_tbl = osm_physp_get_slvl_tbl(p_physp, 0);
>  
>  		/* update valid SLs that still exist on this route */
> -		for (i = 0; i < IB_MAX_NUM_VLS; i++) {
> -			if (valid_sls[i]) {
> -				vl = ib_slvl_table_get(p_slvl_tbl, i);
> -				if (vl == IB_DROP_VL)
> -					valid_sls[i] = FALSE;
> -			}
> +		for (i = 0; i < IB_MAX_NUM_VLS; i++)
> +			if (sl_mask & (1 << i) &&
> +			    ib_slvl_table_get(p_slvl_tbl, i) == IB_DROP_VL)
> +				sl_mask &= ~(1 << i);
> +
> +		if (!sl_mask) {
> +			if (osm_log_is_active(p_rcv->p_log, OSM_LOG_DEBUG))
> +				osm_log(p_rcv->p_log, OSM_LOG_DEBUG,
> +					"__osm_pr_rcv_get_path_parms: "
> +					"All the SLs lead to VL15 on this path\n");
> +			status = IB_NOT_FOUND;
> +			goto Exit;
>  		}
>  	}
>  
> @@ -456,12 +458,18 @@ __osm_pr_rcv_get_path_parms(IN osm_pr_rcv_t * const p_rcv,
>  			 * Check SL2VL table of the switch and update valid SLs
>  			 */
>  			p_slvl_tbl = osm_physp_get_slvl_tbl(p_physp, in_port_num);
> -			for (i = 0; i < IB_MAX_NUM_VLS; i++) {
> -				if (valid_sls[i]) {
> -					vl = ib_slvl_table_get(p_slvl_tbl, i);
> -					if (vl == IB_DROP_VL)
> -						valid_sls[i] = FALSE;
> -				}
> +			for (i = 0; i < IB_MAX_NUM_VLS; i++)
> +				if (sl_mask & (1 << i) &&
> +				    ib_slvl_table_get(p_slvl_tbl, i) == IB_DROP_VL)
> +					sl_mask &= ~(1 << i);
> +			if (!sl_mask) {
> +				if (osm_log_is_active(p_rcv->p_log, OSM_LOG_DEBUG))
> +					osm_log(p_rcv->p_log, OSM_LOG_DEBUG,
> +						"__osm_pr_rcv_get_path_parms: "
> +						"All the SLs lead to VL15 "
> +						"on this path\n");
> +				status = IB_NOT_FOUND;
> +				goto Exit;
>  			}
>  		}
>  	}
> @@ -483,31 +491,6 @@ __osm_pr_rcv_get_path_parms(IN osm_pr_rcv_t * const p_rcv,
>  			"Path min MTU = %u, min rate = %u\n",
>  			mtu, rate);
>  
> -	if (!p_rcv->p_subn->opt.no_qos) {
> -		/*
> -		 * check whether there is some SL
> -		 * that won't lead to VL15 eventually
> -		 */
> -		sl2vl_valid_path = FALSE;
> -		for (i = 0; i < IB_MAX_NUM_VLS; i++) {
> -			if (valid_sls[i]) {
> -				sl2vl_valid_path = TRUE;
> -				first_valid_sl = i;
> -				break;
> -			}
> -		}
> -
> -		if (!sl2vl_valid_path) {
> -			if (osm_log_is_active(p_rcv->p_log, OSM_LOG_DEBUG)) {
> -				osm_log(p_rcv->p_log, OSM_LOG_DEBUG,
> -					"__osm_pr_rcv_get_path_parms: "
> -					"All the SLs lead to VL15 on this path\n");
> -			}
> -			status = IB_NOT_FOUND;
> -			goto Exit;
> -		}
> -	}
> -
>  	if (!p_rcv->p_subn->opt.no_qos && p_rcv->p_subn->p_qos_policy) {
>  		/* Get QoS Level object according to the path request */
>  		osm_qos_policy_get_qos_level_by_pr(p_rcv->p_subn->p_qos_policy,
> @@ -542,11 +525,11 @@ __osm_pr_rcv_get_path_parms(IN osm_pr_rcv_t * const p_rcv,
>  			pkt_life = p_qos_level->pkt_life;
>  
>  		if (p_qos_level->sl_set) {
> -			if (!valid_sls[p_qos_level->sl]) {
> +			sl = p_qos_level->sl;
> +			if (!(sl_mask & ( 1 << sl))) {
>  				status = IB_NOT_FOUND;
>  				goto Exit;
>  			}
> -			sl = p_qos_level->sl;
>  		}
>  
>  		if (osm_log_is_active(p_rcv->p_log, OSM_LOG_DEBUG))
> @@ -830,12 +813,16 @@ __osm_pr_rcv_get_path_parms(IN osm_pr_rcv_t * const p_rcv,
>  		sl = osm_get_lash_sl(p_rcv->p_subn->p_osm,
>  				     p_src_port, p_dest_port);
>  	} else if (!p_rcv->p_subn->opt.no_qos) {
> -		sl = first_valid_sl;
> +		for (i = 0; i < IB_MAX_NUM_VLS; i++)
> +			if (sl_mask&(1 << i)) {
> +				sl = i;
> +				break;
> +			}
>  	}
>  	else
>  		sl = OSM_DEFAULT_SL;
>  
> -	if (!p_rcv->p_subn->opt.no_qos && !valid_sls[sl]) {
> +	if (!p_rcv->p_subn->opt.no_qos && !(sl_mask & (1 << sl))) {
>  		osm_log(p_rcv->p_log, OSM_LOG_ERROR,
>  			"__osm_pr_rcv_get_path_parms: ERR 1F23: "
>  			"Selected SL (%u) leads to VL15\n", p_prtn->sl);
> 

Thanks, I'll look into this and I'll repost the patch.

>>>> +	/*
>>>> +	 * set Pkey for this path record request
>>>> +	 */
>>>> +
>>>> +	if ((comp_mask & IB_PR_COMPMASK_RAWTRAFFIC) &&
>>>> +	    (cl_ntoh32(p_pr->hop_flow_raw) & (1 << 31)))
>>> No extra () was needed - this generates confused diff lines.
>>  No sure what you mean here by "confused diff lines".
> 
> I mean those extra lines in the patch where the only differences are
> formatting or cosmetic stuff like extra braces. If you have a reason to
> make such changes just send it as separate patch.
> 
>>  I agree that the extra () are not *needed*, but isn't
>>
>>  	if ((comp_mask & IB_PR_COMPMASK_RAWTRAFFIC) &&
>>  	    (cl_ntoh32(p_pr->hop_flow_raw) & (1 << 31)))
>>
>>  is more readable than
>>
>>  	if (comp_mask & IB_PR_COMPMASK_RAWTRAFFIC &&
>>  	    cl_ntoh32(p_pr->hop_flow_raw) & 1 << 31)
>>
>>  ?
> 
> No. It requires 2+ seconds to make sure that some braces are just
> "extra" ones.
> 
> BTW the second is incorrect - should  be (1 << 31), those '()' were
> needed.

No, they weren't :)

http://www.cppreference.com/operator_precedence.html

-- Yevgeny

> Sasha
> 




More information about the general mailing list