[ofa-general] Re: [PATCH] opensm/QoS: setting SL in the IPoIB MCast groups

[Yevgeny Kliteynik]

Sasha Khapyorsky wrote:
> Hi Yevgeny,
> 
> On 16:49 Mon 24 Mar     , Yevgeny Kliteynik wrote:
>> This is a reworked patch that sets SL in the IPoIB mcast groups
>> Note that I'm using functions here! :)
>>
>> Added mcast mlid to the partition structure.
>> This mlid is used later by the QoS manager for quick access
>> to the mcast group.
> 
> Basically looks fine for me. One question however.
> 
>> The patch is for master only.
>>
>> Signed-off-by: Yevgeny Kliteynik <kliteyn at dev.mellanox.co.il>
>> ---
>>  opensm/include/opensm/osm_partition.h |    5 ++
>>  opensm/opensm/osm_prtn.c              |    9 +++-
>>  opensm/opensm/osm_qos_policy.c        |   84 ++++++++++++++++++++++++++------
>>  3 files changed, 80 insertions(+), 18 deletions(-)
>>
>> diff --git a/opensm/include/opensm/osm_partition.h b/opensm/include/opensm/osm_partition.h
>> index 70da27d..326aeb6 100644
>> --- a/opensm/include/opensm/osm_partition.h
>> +++ b/opensm/include/opensm/osm_partition.h
>> @@ -97,6 +97,7 @@ BEGIN_C_DECLS
>>  typedef struct _osm_prtn {
>>  	cl_map_item_t map_item;
>>  	ib_net16_t pkey;
>> +	ib_net16_t mlid;
> 
> Why to not use 'osm_mgrp_t *mgr' instead of mlid and to not access mcast
> group directly w/out qmap search?

No special reason. Didn't want to think what would happen if the mcast
group was removed for some reason and the partition will still have the
pointer. If you really think that having a pointer instead of an mlid
will save some time (which is just a look up in the tree of mcast groups),
I can repost the patch.

-- Yevgeny

> Sasha
> 
>>  	uint8_t sl;
>>  	cl_map_t full_guid_tbl;
>>  	cl_map_t part_guid_tbl;
>> @@ -110,6 +111,10 @@ typedef struct _osm_prtn {
>>  *	pkey
>>  *		The IBA defined P_KEY of this Partition.
>>  *
>> +*	mlid
>> +*		The network ordered LID of the well known Multicast Group
>> +*		that was created for this partition.
>> +*
>>  *	sl
>>  *		The Service Level (SL) associated with this Partiton.
>>  *
>> diff --git a/opensm/opensm/osm_prtn.c b/opensm/opensm/osm_prtn.c
>> index 2d0b313..187cff6 100644
>> --- a/opensm/opensm/osm_prtn.c
>> +++ b/opensm/opensm/osm_prtn.c
>> @@ -230,8 +230,10 @@ ib_api_status_t osm_prtn_add_mcgroup(osm_log_t * p_log,
>>  		OSM_LOG(p_log, OSM_LOG_ERROR,
>>  			"Failed to create MC group with pkey 0x%04x\n",
>>  			cl_ntoh16(pkey));
>> -	if (p_mgrp)
>> +	if (p_mgrp) {
>>  		p_mgrp->well_known = TRUE;
>> +		p->mlid = p_mgrp->mlid;
>> +	}
>>
>>  	/* workaround for TS */
>>  	/* FIXME: remove this upon TS fixes */
>> @@ -243,8 +245,11 @@ ib_api_status_t osm_prtn_add_mcgroup(osm_log_t * p_log,
>>
>>  	status = osm_mcmr_rcv_find_or_create_new_mgrp(p_sa, comp_mask, &mc_rec,
>>  						      &p_mgrp);
>> -	if (p_mgrp)
>> +	if (p_mgrp) {
>>  		p_mgrp->well_known = TRUE;
>> +		if (!p->mlid)
>> +			p->mlid = p_mgrp->mlid;
>> +	}
>>
>>  	return status;
>>  }
>> diff --git a/opensm/opensm/osm_qos_policy.c b/opensm/opensm/osm_qos_policy.c
>> index aef1856..1c428e5 100644
>> --- a/opensm/opensm/osm_qos_policy.c
>> +++ b/opensm/opensm/osm_qos_policy.c
>> @@ -763,6 +763,69 @@ static osm_qos_port_group_t *__qos_policy_get_port_group_by_name(
>>  /***************************************************
>>   ***************************************************/
>>
>> +static void __qos_policy_validate_pkey(
>> +			osm_qos_policy_t * p_qos_policy,
>> +			osm_qos_match_rule_t * p_qos_match_rule,
>> +			osm_prtn_t * p_prtn)
>> +{
>> +	uint8_t sl;
>> +	uint32_t flow;
>> +	uint8_t hop;
>> +	osm_mgrp_t * p_mgrp;
>> +
>> +	if (!p_qos_policy || !p_qos_match_rule || !p_prtn)
>> +		return;
>> +
>> +	if (!p_qos_match_rule->p_qos_level->sl_set ||
>> +	    p_prtn->sl == p_qos_match_rule->p_qos_level->sl)
>> +		return;
>> +
>> +	/* overriding partition's SL */
>> +	OSM_LOG(&p_qos_policy->p_subn->p_osm->log, OSM_LOG_ERROR,
>> +		"ERR AC15: pkey 0x%04X in match rule - "
>> +		"overriding partition SL (%u) with QoS Level SL (%u)\n",
>> +		cl_ntoh16(p_prtn->pkey), p_prtn->sl,
>> +		p_qos_match_rule->p_qos_level->sl);
>> +	p_prtn->sl = p_qos_match_rule->p_qos_level->sl;
>> +
>> +
>> +	/* If this partition is an IPoIB partition, there should
>> +	   be a matching MCast group. Fix this group's SL too */
>> +
>> +	if (!p_prtn->mlid)
>> +		return;
>> +
>> +	p_mgrp = (osm_mgrp_t *) cl_qmap_get(
>> +		&p_qos_policy->p_subn->mgrp_mlid_tbl,
>> +		p_prtn->mlid);
>> +	if (p_mgrp == (osm_mgrp_t *)
>> +		cl_qmap_end(&p_qos_policy->p_subn->mgrp_mlid_tbl)) {
>> +		OSM_LOG(&p_qos_policy->p_subn->p_osm->log, OSM_LOG_ERROR,
>> +			"ERR AC16: MCast group for partition with "
>> +			"pkey 0x%04X not found\n",
>> +			cl_ntoh16(p_prtn->pkey));
>> +		return;
>> +	}
>> +
>> +	CL_ASSERT((cl_ntoh16(p_mgrp->mcmember_rec.pkey) & 0x7fff) ==
>> +		  (cl_ntoh16(p_prtn->pkey) & 0x7fff));
>> +
>> +	ib_member_get_sl_flow_hop(p_mgrp->mcmember_rec.sl_flow_hop,
>> +				  &sl, &flow, &hop);
>> +	if (sl != p_prtn->sl) {
>> +		OSM_LOG(&p_qos_policy->p_subn->p_osm->log, OSM_LOG_DEBUG,
>> +			"Updating MCGroup (MLID 0x%04x) SL to "
>> +			"match partition SL (%u)\n",
>> +			cl_hton16(p_mgrp->mcmember_rec.mlid),
>> +			p_prtn->sl);
>> +		p_mgrp->mcmember_rec.sl_flow_hop =
>> +			ib_member_set_sl_flow_hop(p_prtn->sl, flow, hop);
>> +	}
>> +}
>> +
>> +/***************************************************
>> + ***************************************************/
>> +
>>  int osm_qos_policy_validate(osm_qos_policy_t * p_qos_policy,
>>  			    osm_log_t *p_log)
>>  {
>> @@ -901,27 +964,16 @@ int osm_qos_policy_validate(osm_qos_policy_t * p_qos_policy,
>>  					&p_qos_policy->p_subn->prtn_pkey_tbl, pkey);
>>
>>  				if (p_prtn == (osm_prtn_t *)cl_qmap_end(
>> -					&p_qos_policy->p_subn->prtn_pkey_tbl)) {
>> +					&p_qos_policy->p_subn->prtn_pkey_tbl))
>>  					/* partition for this pkey not found */
>>  					OSM_LOG(p_log, OSM_LOG_ERROR, "ERR AC14: "
>>  						"pkey 0x%04X in match rule - "
>>  						"partition doesn't exist\n",
>>  						cl_ntoh16(pkey));
>> -					continue;
>> -				}
>> -
>> -				if (p_qos_match_rule->p_qos_level->sl_set &&
>> -                                    p_prtn->sl != p_qos_match_rule->p_qos_level->sl) {
>> -					/* overriding partition's SL */
>> -					OSM_LOG(p_log, OSM_LOG_ERROR, "ERR AC15: "
>> -						"pkey 0x%04X in match rule - "
>> -						"overriding partition SL (%u) "
>> -						"with QoS Level SL (%u)\n",
>> -						cl_ntoh16(pkey),
>> -						p_prtn->sl,
>> -						p_qos_match_rule->p_qos_level->sl);
>> -					p_prtn->sl = p_qos_match_rule->p_qos_level->sl;
>> -				}
>> +				else
>> +					__qos_policy_validate_pkey(p_qos_policy,
>> +							p_qos_match_rule,
>> +							p_prtn);
>>  			}
>>  		}
>>
>> -- 
>> 1.5.1.4
>>
>