[ofa-general] Re: [PATCH] opensm: Convert mgrp_mlid_tbl into array

Hal Rosenstock hrosenstock at xsigo.com
Tue Jun 24 15:19:39 PDT 2008


Hi Sasha,

On Wed, 2008-06-25 at 00:58 +0300, Sasha Khapyorsky wrote:
> Hi Hal,
> 
> On 04:46 Sun 22 Jun     , Hal Rosenstock wrote:
> > opensm: Convert mgrp_mlid_tbl into array
> 
> Comments are below.
> 
> > 
> > Signed-off-by: Hal Rosenstock <hal at xsigo.com>
> > 
> > diff --git a/opensm/include/opensm/osm_subnet.h b/opensm/include/opensm/osm_subnet.h
> > index ef9996c..75be84a 100644
> > --- a/opensm/include/opensm/osm_subnet.h
> > +++ b/opensm/include/opensm/osm_subnet.h
> > @@ -2,6 +2,7 @@
> >   * Copyright (c) 2004-2007 Voltaire, Inc. All rights reserved.
> >   * Copyright (c) 2002-2005 Mellanox Technologies LTD. All rights reserved.
> >   * Copyright (c) 1996-2003 Intel Corporation. All rights reserved.
> > + * Copyright (c) 2008 Xsigo Systems Inc.  All rights reserved.
> >   *
> >   * This software is available to you under a choice of one of two
> >   * licenses.  You may choose to be licensed under the terms of the GNU
> > @@ -446,6 +447,10 @@ typedef struct osm_subn_opt {
> >  *	Subnet object
> >  *********/
> >  
> > +struct _osm_mgrp_mlid_tbl {
> > +	void *mgroup[IB_LID_MCAST_END_HO - IB_LID_MCAST_START_HO + 1];
> > +};
> > +
> 
> Why this wrapper struct is needed?

It's just for clarity (self documenting). Does it need to be removed ?

> >  /****s* OpenSM: Subnet/osm_subn_t
> >  * NAME
> >  *	osm_subn_t
> > @@ -467,7 +472,6 @@ typedef struct osm_subn {
> >  	cl_qmap_t rtr_guid_tbl;
> >  	cl_qlist_t prefix_routes_list;
> >  	cl_qmap_t prtn_pkey_tbl;
> > -	cl_qmap_t mgrp_mlid_tbl;
> >  	cl_qmap_t sm_guid_tbl;
> >  	cl_qlist_t sa_sr_list;
> >  	cl_qlist_t sa_infr_list;
> > @@ -490,6 +494,7 @@ typedef struct osm_subn {
> >  	boolean_t first_time_master_sweep;
> >  	boolean_t coming_out_of_standby;
> >  	unsigned need_update;
> > +	struct _osm_mgrp_mlid_tbl mgrp_mlid_tbl;
> >  } osm_subn_t;
> >  /*
> >  * FIELDS
> > @@ -513,10 +518,6 @@ typedef struct osm_subn {
> >  *		Container of pointers to all Partition objects in the subnet.
> >  *		Indexed by P_KEY.
> >  *
> > -*	mgrp_mlid_tbl
> > -*		Container of pointers to all Multicast Group objects in the subnet.
> > -*		Indexed by MLID.
> > -*
> >  *	sm_guid_tbl
> >  *		Container of pointers to SM objects representing other SMs
> >  *		on the subnet.
> > @@ -615,6 +616,10 @@ typedef struct osm_subn {
> >  *     This flag should be on during first non-master heavy (including
> >  *     pre-master discovery stage)
> >  *
> > +*	mgrp_mlid_tbl
> > +*		Array of pointers to all Multicast Group objects in the subnet.
> > +*		Indexed by MLID offset from base MLID.
> > +*
> >  * SEE ALSO
> >  *	Subnet object
> >  *********/
> > diff --git a/opensm/opensm/osm_drop_mgr.c b/opensm/opensm/osm_drop_mgr.c
> > index e2f5828..44ee8eb 100644
> > --- a/opensm/opensm/osm_drop_mgr.c
> > +++ b/opensm/opensm/osm_drop_mgr.c
> > @@ -2,6 +2,7 @@
> >   * Copyright (c) 2004-2007 Voltaire, Inc. All rights reserved.
> >   * Copyright (c) 2002-2005 Mellanox Technologies LTD. All rights reserved.
> >   * Copyright (c) 1996-2003 Intel Corporation. All rights reserved.
> > + * Copyright (c) 2008 Xsigo Systems Inc.  All rights reserved.
> >   *
> >   * This software is available to you under a choice of one of two
> >   * licenses.  You may choose to be licensed under the terms of the GNU
> > @@ -163,6 +164,7 @@ static void __osm_drop_mgr_remove_port(osm_sm_t * sm, IN osm_port_t * p_port)
> >  	ib_gid_t port_gid;
> >  	ib_mad_notice_attr_t notice;
> >  	ib_api_status_t status;
> > +	int index;
> 
> Here and below, 'int i' looks much more friendly as loop index variable.

OK.

> >  	OSM_LOG_ENTER(sm->p_log);
> >  
> > @@ -206,14 +208,15 @@ static void __osm_drop_mgr_remove_port(osm_sm_t * sm, IN osm_port_t * p_port)
> >  
> >  	p_mcm = (osm_mcm_info_t *) cl_qlist_remove_head(&p_port->mcm_list);
> >  	while (p_mcm != (osm_mcm_info_t *) cl_qlist_end(&p_port->mcm_list)) {
> > -		p_mgrp =
> > -		    (osm_mgrp_t *) cl_qmap_get(&sm->p_subn->mgrp_mlid_tbl,
> > -					       p_mcm->mlid);
> > -		if (p_mgrp !=
> > -		    (osm_mgrp_t *) cl_qmap_end(&sm->p_subn->mgrp_mlid_tbl)) {
> > -			osm_mgrp_remove_port(sm->p_subn, sm->p_log,
> > -					     p_mgrp, p_port->guid);
> > -			osm_mcm_info_delete((osm_mcm_info_t *) p_mcm);
> > +		for (index = 0;
> > +		     index <= sm->p_subn->max_multicast_lid_ho - IB_LID_MCAST_START_HO;
> > +		     index++) {
> > +			p_mgrp = sm->p_subn->mgrp_mlid_tbl.mgroup[index];
> > +			if (p_mgrp) {
> > +				osm_mgrp_remove_port(sm->p_subn, sm->p_log,
> > +						     p_mgrp, p_port->guid);
> > +				osm_mcm_info_delete((osm_mcm_info_t *) p_mcm);
> > +			}
> 
> Here you have p_mcm->mlid which is index of multicast group where this
> port is member + IB_LID_MCAST_START_HO. So you don't need to loop other
> all groups in the array. Actually there only one line require changing:
> 
> 	p_mgrp = sm->p_subn->mgrp_mlid_tbl.mgroup[p_mcm->mlid -
> 			IB_LID_MCAST_START_HO]
> 	if (p_mgrp) {
> 		osm_mgrp_remove_port(sm->p_subn, sm->p_log, p_mgrp,
> 				     p_port->guid);
> 		osm_mcm_info_delete((osm_mcm_info_t *) p_mcm);
> 	}

Yes, that's much better.

> >  		}
> >  		p_mcm =
> >  		    (osm_mcm_info_t *) cl_qlist_remove_head(&p_port->mcm_list);
> > diff --git a/opensm/opensm/osm_mcast_mgr.c b/opensm/opensm/osm_mcast_mgr.c
> > index d7a0f56..15a585e 100644
> > --- a/opensm/opensm/osm_mcast_mgr.c
> > +++ b/opensm/opensm/osm_mcast_mgr.c
> > @@ -2,6 +2,7 @@
> >   * Copyright (c) 2004-2007 Voltaire, Inc. All rights reserved.
> >   * Copyright (c) 2002-2006 Mellanox Technologies LTD. All rights reserved.
> >   * Copyright (c) 1996-2003 Intel Corporation. All rights reserved.
> > + * Copyright (c) 2008 Xsigo Systems Inc.  All rights reserved.
> >   *
> >   * This software is available to you under a choice of one of two
> >   * licenses.  You may choose to be licensed under the terms of the GNU
> > @@ -1132,6 +1133,8 @@ mcast_mgr_process_mgrp(osm_sm_t * sm,
> >  		       IN ib_net64_t port_guid)
> >  {
> >  	ib_api_status_t status;
> > +	osm_mgrp_t *p_mgroup;
> > +	int index;
> >  
> >  	OSM_LOG_ENTER(sm->p_log);
> >  
> > @@ -1154,8 +1157,15 @@ mcast_mgr_process_mgrp(osm_sm_t * sm,
> >  		/* Send a Report to any InformInfo registered for
> >  		   Trap 67 : MCGroup delete */
> >  		osm_mgrp_send_delete_notice(sm->p_subn, sm->p_log, p_mgrp);
> > -		cl_qmap_remove_item(&sm->p_subn->mgrp_mlid_tbl,
> > -				    (cl_map_item_t *) p_mgrp);
> > +		for (index = 0;
> > +		     index <= sm->p_subn->max_multicast_lid_ho - IB_LID_MCAST_START_HO;
> > +		     index++) {
> > +			p_mgroup = sm->p_subn->mgrp_mlid_tbl.mgroup[index];
> > +			if (p_mgroup == p_mgrp) {
> > +				sm->p_subn->mgrp_mlid_tbl.mgroup[index] = NULL;
> > +				break;
> > +			}
> > +		}
> 
> The same is here.

Ditto.

> >  		osm_mgrp_delete(p_mgrp);
> >  	}
> >  
> > @@ -1171,16 +1181,14 @@ osm_signal_t osm_mcast_mgr_process(osm_sm_t * sm)
> >  	osm_signal_t signal;
> >  	osm_switch_t *p_sw;
> >  	cl_qmap_t *p_sw_tbl;
> > -	cl_qmap_t *p_mcast_tbl;
> >  	cl_qlist_t *p_list = &sm->mgrp_list;
> >  	osm_mgrp_t *p_mgrp;
> > -	osm_mgrp_t *p_next_mgrp;
> >  	boolean_t pending_transactions = FALSE;
> > +	int index;
> >  
> >  	OSM_LOG_ENTER(sm->p_log);
> >  
> >  	p_sw_tbl = &sm->p_subn->sw_guid_tbl;
> > -	p_mcast_tbl = &sm->p_subn->mgrp_mlid_tbl;
> >  	/*
> >  	   While holding the lock, iterate over all the established
> >  	   multicast groups, servicing each in turn.
> > @@ -1189,17 +1197,19 @@ osm_signal_t osm_mcast_mgr_process(osm_sm_t * sm)
> >  	 */
> >  	CL_PLOCK_EXCL_ACQUIRE(sm->p_lock);
> >  
> > -	p_mgrp = (osm_mgrp_t *) cl_qmap_head(p_mcast_tbl);
> > -	while (p_mgrp != (osm_mgrp_t *) cl_qmap_end(p_mcast_tbl)) {
> > +	for (index = 0;
> > +	     index <= sm->p_subn->max_multicast_lid_ho - IB_LID_MCAST_START_HO;
> > +	     index++) {
> >  		/*
> >  		   We reached here due to some change that caused a heavy sweep
> >  		   of the subnet. Not due to a specific multicast request.
> >  		   So the request type is subnet_change and the port guid is 0.
> >  		 */
> > -		p_next_mgrp = (osm_mgrp_t *) cl_qmap_next(&p_mgrp->map_item);
> > -		mcast_mgr_process_mgrp(sm, p_mgrp,
> > -				       OSM_MCAST_REQ_TYPE_SUBNET_CHANGE, 0);
> > -		p_mgrp = p_next_mgrp;
> > +		p_mgrp = sm->p_subn->mgrp_mlid_tbl.mgroup[index];
> > +		if (p_mgrp)
> > +			mcast_mgr_process_mgrp(sm, p_mgrp,
> > +					       OSM_MCAST_REQ_TYPE_SUBNET_CHANGE,
> > +					       0); 
> >  	}
> >  
> >  	/*
> > @@ -1233,13 +1243,7 @@ osm_signal_t osm_mcast_mgr_process(osm_sm_t * sm)
> >  static
> >  osm_mgrp_t *__get_mgrp_by_mlid(osm_sm_t * sm, IN ib_net16_t const mlid)
> >  {
> > -	cl_map_item_t *map_item;
> > -
> > -	map_item = cl_qmap_get(&sm->p_subn->mgrp_mlid_tbl, mlid);
> > -	if (map_item == cl_qmap_end(&sm->p_subn->mgrp_mlid_tbl)) {
> > -		return NULL;
> > -	}
> > -	return (osm_mgrp_t *) map_item;
> > +	return(sm->p_subn->mgrp_mlid_tbl.mgroup[cl_ntoh16(mlid) - IB_LID_MCAST_START_HO]);
> >  }
> 
> Same function exists in osm_sa_mcmember_record.c. It is probably good
> time to consolidate it in something like osm_get_mgrp_by_mlid() (it
> could be placed in osm_subnet.c where another osm_get_*_by() hosted).

Sure; is it OK for that to be an incremental patch on top of this to
follow shortly ?

> >  
> >  /**********************************************************************
> > diff --git a/opensm/opensm/osm_qos_policy.c b/opensm/opensm/osm_qos_policy.c
> > index f1d4e54..5a8dd3b 100644
> > --- a/opensm/opensm/osm_qos_policy.c
> > +++ b/opensm/opensm/osm_qos_policy.c
> > @@ -2,6 +2,7 @@
> >   * Copyright (c) 2004-2006 Voltaire, Inc. All rights reserved.
> >   * Copyright (c) 2002-2005 Mellanox Technologies LTD. All rights reserved.
> >   * Copyright (c) 1996-2003 Intel Corporation. All rights reserved.
> > + * Copyright (c) 2008 Xsigo Systems Inc.  All rights reserved.
> >   *
> >   * This software is available to you under a choice of one of two
> >   * licenses.  You may choose to be licensed under the terms of the GNU
> > @@ -792,11 +793,8 @@ static void __qos_policy_validate_pkey(
> >  	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)) {
> > +	p_mgrp = p_qos_policy->p_subn->mgrp_mlid_tbl.mgroup[cl_ntoh16(p_prtn->mlid) - IB_LID_MCAST_START_HO];
> > +	if (!p_mgrp) {
> >  		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",
> > diff --git a/opensm/opensm/osm_sa.c b/opensm/opensm/osm_sa.c
> > index adf4931..4695bc0 100644
> > --- a/opensm/opensm/osm_sa.c
> > +++ b/opensm/opensm/osm_sa.c
> > @@ -2,6 +2,7 @@
> >   * Copyright (c) 2004-2007 Voltaire, Inc. All rights reserved.
> >   * Copyright (c) 2002-2005 Mellanox Technologies LTD. All rights reserved.
> >   * Copyright (c) 1996-2003 Intel Corporation. All rights reserved.
> > + * Copyright (c) 2008 Xsigo Systems Inc.  All rights reserved.
> >   *
> >   * This software is available to you under a choice of one of two
> >   * licenses.  You may choose to be licensed under the terms of the GNU
> > @@ -558,12 +559,11 @@ static void mcast_mgr_dump_one_port(cl_map_item_t * p_map_item, void *cxt)
> >  		p_mcm_port->scope_state, p_mcm_port->proxy_join);
> >  }
> >  
> > -static void sa_dump_one_mgrp(cl_map_item_t * p_map_item, void *cxt)
> > +static void sa_dump_one_mgrp(osm_mgrp_t *p_mgrp, void *cxt)
> >  {
> >  	struct opensm_dump_context dump_context;
> >  	osm_opensm_t *p_osm = ((struct opensm_dump_context *)cxt)->p_osm;
> >  	FILE *file = ((struct opensm_dump_context *)cxt)->file;
> > -	osm_mgrp_t *p_mgrp = (osm_mgrp_t *) p_map_item;
> >  
> >  	fprintf(file, "MC Group 0x%04x %s:"
> >  		" mgid=0x%016" PRIx64 ":0x%016" PRIx64
> > @@ -700,13 +700,20 @@ static void sa_dump_one_service(cl_list_item_t * p_list_item, void *cxt)
> >  static void sa_dump_all_sa(osm_opensm_t * p_osm, FILE * file)
> >  {
> >  	struct opensm_dump_context dump_context;
> > +	osm_mgrp_t *p_mgrp;
> > +	int index;
> >  
> >  	dump_context.p_osm = p_osm;
> >  	dump_context.file = file;
> >  	OSM_LOG(&p_osm->log, OSM_LOG_DEBUG, "Dump multicast:\n");
> >  	cl_plock_acquire(&p_osm->lock);
> > -	cl_qmap_apply_func(&p_osm->subn.mgrp_mlid_tbl,
> > -			   sa_dump_one_mgrp, &dump_context);
> > +	for (index = 0;
> > +	     index <= p_osm->subn.max_multicast_lid_ho - IB_LID_MCAST_START_HO;
> > +	     index++) {
> > +		p_mgrp = p_osm->subn.mgrp_mlid_tbl.mgroup[index];
> > +		if (p_mgrp)
> > +			sa_dump_one_mgrp(p_mgrp, &dump_context);
> > +	}
> >  	OSM_LOG(&p_osm->log, OSM_LOG_DEBUG, "Dump inform:\n");
> >  	cl_qlist_apply_func(&p_osm->subn.sa_infr_list,
> >  			    sa_dump_one_inform, &dump_context);
> > @@ -729,14 +736,12 @@ static osm_mgrp_t *load_mcgroup(osm_opensm_t * p_osm, ib_net16_t mlid,
> >  				unsigned well_known)
> >  {
> >  	ib_net64_t comp_mask;
> > -	cl_map_item_t *p_next;
> >  	osm_mgrp_t *p_mgrp;
> >  
> >  	cl_plock_excl_acquire(&p_osm->lock);
> >  
> > -	if ((p_next = cl_qmap_get(&p_osm->subn.mgrp_mlid_tbl, mlid)) !=
> > -	    cl_qmap_end(&p_osm->subn.mgrp_mlid_tbl)) {
> > -		p_mgrp = (osm_mgrp_t *) p_next;
> > +	p_mgrp = p_osm->subn.mgrp_mlid_tbl.mgroup[cl_ntoh16(mlid) - IB_LID_MCAST_START_HO];
> > +	if (p_mgrp) {
> >  		if (!memcmp(&p_mgrp->mcmember_rec.mgid, &p_mcm_rec->mgid,
> >  			    sizeof(ib_gid_t))) {
> >  			OSM_LOG(&p_osm->log, OSM_LOG_DEBUG,
> > diff --git a/opensm/opensm/osm_sa_mcmember_record.c b/opensm/opensm/osm_sa_mcmember_record.c
> > index 46c87c7..7049fc0 100644
> > --- a/opensm/opensm/osm_sa_mcmember_record.c
> > +++ b/opensm/opensm/osm_sa_mcmember_record.c
> > @@ -2,6 +2,7 @@
> >   * Copyright (c) 2004-2007 Voltaire, Inc. All rights reserved.
> >   * Copyright (c) 2002-2005 Mellanox Technologies LTD. All rights reserved.
> >   * Copyright (c) 1996-2003 Intel Corporation. All rights reserved.
> > + * Copyright (c) 2008 Xsigo Systems Inc.  All rights reserved.
> >   *
> >   * This software is available to you under a choice of one of two
> >   * licenses.  You may choose to be licensed under the terms of the GNU
> > @@ -96,13 +97,7 @@ typedef struct osm_sa_mcmr_search_ctxt {
> >  static osm_mgrp_t *__get_mgrp_by_mlid(IN osm_sa_t * sa,
> >  				      IN ib_net16_t const mlid)
> >  {
> > -	cl_map_item_t *map_item;
> > -
> > -	map_item = cl_qmap_get(&sa->p_subn->mgrp_mlid_tbl, mlid);
> > -	if (map_item == cl_qmap_end(&sa->p_subn->mgrp_mlid_tbl)) {
> > -		return NULL;
> > -	}
> > -	return (osm_mgrp_t *) map_item;
> > +	return(sa->p_subn->mgrp_mlid_tbl.mgroup[cl_ntoh16(mlid) - IB_LID_MCAST_START_HO]);
> >  }
> >  
> >  /*********************************************************************
> > @@ -154,16 +149,20 @@ __get_new_mlid(IN osm_sa_t * sa, IN ib_net16_t requested_mlid)
> >  
> >  	if (requested_mlid && cl_ntoh16(requested_mlid) >= IB_LID_MCAST_START_HO
> >  	    && cl_ntoh16(requested_mlid) <= p_subn->max_multicast_lid_ho
> > -	    && cl_qmap_get(&p_subn->mgrp_mlid_tbl,
> > -			   requested_mlid) ==
> > -	    cl_qmap_end(&p_subn->mgrp_mlid_tbl)) {
> > +	    && !p_subn->mgrp_mlid_tbl.mgroup[cl_ntoh16(requested_mlid) - IB_LID_MCAST_START_HO]) { 
> >  		mlid = cl_ntoh16(requested_mlid);
> >  		goto Exit;
> >  	}
> >  
> >  	/* If MCGroups table is empty, first return the min mlid */
> > -	p_mgrp = (osm_mgrp_t *) cl_qmap_head(&p_subn->mgrp_mlid_tbl);
> > -	if (p_mgrp == (osm_mgrp_t *) cl_qmap_end(&p_subn->mgrp_mlid_tbl)) {
> > +	max_num_mlids = sa->p_subn->max_multicast_lid_ho -
> > +			IB_LID_MCAST_START_HO + 1;
> > +	for (idx = 0; idx < max_num_mlids; idx++) {
> > +		p_mgrp = sa->p_subn->mgrp_mlid_tbl.mgroup[idx];
> > +		if (p_mgrp)
> > +			break;
> > +	}
> > +	if (!p_mgrp) {
> >  		mlid = IB_LID_MCAST_START_HO;
> >  		OSM_LOG(sa->p_log, OSM_LOG_VERBOSE,
> >  			"No multicast groups found using minimal mlid:0x%04X\n",
> > @@ -171,9 +170,6 @@ __get_new_mlid(IN osm_sa_t * sa, IN ib_net16_t requested_mlid)
> >  		goto Exit;
> >  	}
> >  
> > -	max_num_mlids =
> > -	    sa->p_subn->max_multicast_lid_ho - IB_LID_MCAST_START_HO + 1;
> > -
> >  	/* track all used mlids in the array (by mlid index) */
> >  	used_mlids_array = (uint8_t *) malloc(sizeof(uint8_t) * max_num_mlids);
> >  	if (!used_mlids_array)
> > @@ -181,9 +177,10 @@ __get_new_mlid(IN osm_sa_t * sa, IN ib_net16_t requested_mlid)
> >  	memset(used_mlids_array, 0, sizeof(uint8_t) * max_num_mlids);
> >  
> >  	/* scan all available multicast groups in the DB and fill in the table */
> > -	while (p_mgrp != (osm_mgrp_t *) cl_qmap_end(&p_subn->mgrp_mlid_tbl)) {
> > +	for (idx = 0; idx < max_num_mlids; idx++) {
> > +		p_mgrp = sa->p_subn->mgrp_mlid_tbl.mgroup[idx];
> >  		/* ignore mgrps marked for deletion */
> > -		if (p_mgrp->to_be_deleted == FALSE) {
> > +		if (p_mgrp && p_mgrp->to_be_deleted == FALSE) {
> >  			OSM_LOG(sa->p_log, OSM_LOG_DEBUG,
> >  				"Found mgrp with lid:0x%X MGID: 0x%016" PRIx64
> >  				" : " "0x%016" PRIx64 "\n",
> > @@ -202,11 +199,9 @@ __get_new_mlid(IN osm_sa_t * sa, IN ib_net16_t requested_mlid)
> >  					cl_ntoh16(p_mgrp->mlid),
> >  					max_num_mlids + IB_LID_MCAST_START_HO);
> >  			} else {
> > -				used_mlids_array[cl_ntoh16(p_mgrp->mlid) -
> > -						 IB_LID_MCAST_START_HO] = 1;
> > +				used_mlids_array[idx] = 1;
> >  			}
> >  		}
> > -		p_mgrp = (osm_mgrp_t *) cl_qmap_next(&p_mgrp->map_item);
> >  	}
> >  
> >  	/* Find "mlid holes" in the mgrp table */
> 
> There were some dirty tricks when qmap was used and finally they did
> an additional mlid array (that with malloc()) for free mlid searching.
> But now you already have such array, what is the reason to copy this and
> to run additional loop?

It can be eliminated in an incremental patch. OK ?

> > @@ -247,8 +242,7 @@ __cleanup_mgrp(IN osm_sa_t * sa, IN ib_net16_t const mlid)
> >  	   not a well known group */
> >  	if (p_mgrp && cl_is_qmap_empty(&p_mgrp->mcm_port_tbl) &&
> >  	    p_mgrp->well_known == FALSE) {
> > -		cl_qmap_remove_item(&sa->p_subn->mgrp_mlid_tbl,
> > -				    (cl_map_item_t *) p_mgrp);
> > +		sa->p_subn->mgrp_mlid_tbl.mgroup[cl_ntoh16(mlid) - IB_LID_MCAST_START_HO] = NULL;
> >  		osm_mgrp_delete(p_mgrp);
> >  	}
> >  }
> > @@ -1033,21 +1027,17 @@ osm_mcmr_rcv_create_new_mgrp(IN osm_sa_t * sa,
> >  	/* since we might have an old group by that mlid
> >  	   one whose deletion was delayed for an idle time
> >  	   we need to deallocate it first */
> > -	p_prev_mgrp =
> > -	    (osm_mgrp_t *) cl_qmap_get(&sa->p_subn->mgrp_mlid_tbl, mlid);
> > -	if (p_prev_mgrp !=
> > -	    (osm_mgrp_t *) cl_qmap_end(&sa->p_subn->mgrp_mlid_tbl)) {
> > +	p_prev_mgrp = sa->p_subn->mgrp_mlid_tbl.mgroup[cl_ntoh16(mlid) - IB_LID_MCAST_START_HO];
> > +	if (p_prev_mgrp) {
> >  		OSM_LOG(sa->p_log, OSM_LOG_DEBUG,
> >  			"Found previous group for mlid:0x%04x - "
> >  			"Destroying it first\n",
> >  			cl_ntoh16(mlid));
> > -		cl_qmap_remove_item(&sa->p_subn->mgrp_mlid_tbl,
> > -				    (cl_map_item_t *) p_prev_mgrp);
> > +		sa->p_subn->mgrp_mlid_tbl.mgroup[cl_ntoh16(mlid) - IB_LID_MCAST_START_HO] = NULL;
> >  		osm_mgrp_delete(p_prev_mgrp);
> >  	}
> >  
> > -	cl_qmap_insert(&sa->p_subn->mgrp_mlid_tbl,
> > -		       mlid, &(*pp_mgrp)->map_item);
> > +	sa->p_subn->mgrp_mlid_tbl.mgroup[cl_ntoh16(mlid) - IB_LID_MCAST_START_HO] = *pp_mgrp;
> >  
> >  	/* Send a Report to any InformInfo registered for
> >  	   Trap 66: MCGroup create */
> > @@ -1069,9 +1059,8 @@ typedef struct osm_sa_pr_mcmr_search_ctxt {
> >  /**********************************************************************
> >   *********************************************************************/
> >  static void
> > -__search_mgrp_by_mgid(IN cl_map_item_t * const p_map_item, IN void *context)
> > +__search_mgrp_by_mgid(IN osm_mgrp_t * const p_mgrp, IN void *context)
> >  {
> > -	osm_mgrp_t *p_mgrp = (osm_mgrp_t *) p_map_item;
> >  	osm_sa_pr_mcmr_search_ctxt_t *p_ctxt =
> >  	    (osm_sa_pr_mcmr_search_ctxt_t *) context;
> >  	const ib_gid_t *p_recvd_mgid;
> > @@ -1135,23 +1124,30 @@ __search_mgrp_by_mgid(IN cl_map_item_t * const p_map_item, IN void *context)
> >   **********************************************************************/
> >  ib_api_status_t
> >  osm_get_mgrp_by_mgid(IN osm_sa_t *sa,
> > -		   IN ib_gid_t *p_mgid,
> > -		   OUT osm_mgrp_t **pp_mgrp)
> > +		     IN ib_gid_t *p_mgid,
> > +		     OUT osm_mgrp_t **pp_mgrp)
> >  {
> >  	osm_sa_pr_mcmr_search_ctxt_t mcmr_search_context;
> > +	osm_mgrp_t *p_mgrp;
> > +	int index;
> >  
> >  	mcmr_search_context.p_mgid = p_mgid;
> >  	mcmr_search_context.sa = sa;
> >  	mcmr_search_context.p_mgrp = NULL;
> >  
> > -	cl_qmap_apply_func(&sa->p_subn->mgrp_mlid_tbl,
> > -			   __search_mgrp_by_mgid, &mcmr_search_context);
> > -
> > -	if (mcmr_search_context.p_mgrp == NULL)
> > -		return IB_NOT_FOUND;
> > -
> > -	*pp_mgrp = mcmr_search_context.p_mgrp;
> > -	return IB_SUCCESS;
> > +	for (index = 0;
> > +	     index <= sa->p_subn->max_multicast_lid_ho - IB_LID_MCAST_START_HO;
> > +	     index++) {
> > +		p_mgrp = sa->p_subn->mgrp_mlid_tbl.mgroup[index];
> > +		if (p_mgrp) {
> > +			__search_mgrp_by_mgid(p_mgrp, &mcmr_search_context);
> > +			if (mcmr_search_context.p_mgrp) {
> > +				*pp_mgrp = mcmr_search_context.p_mgrp;
> > +				return IB_SUCCESS;
> > +			}
> > +		}
> > +	}
> > +	return IB_NOT_FOUND;
> >  }
> >  
> >  /**********************************************************************
> > @@ -1619,10 +1615,9 @@ Exit:
> >   Match the given mgrp to the requested mcmr
> >  **********************************************************************/
> >  static void
> > -__osm_sa_mcm_by_comp_mask_cb(IN cl_map_item_t * const p_map_item,
> > +__osm_sa_mcm_by_comp_mask_cb(IN osm_mgrp_t * const p_mgrp,
> >  			     IN void *context)
> >  {
> > -	const osm_mgrp_t *const p_mgrp = (osm_mgrp_t *) p_map_item;
> >  	osm_sa_mcmr_search_ctxt_t *const p_ctxt =
> >  	    (osm_sa_mcmr_search_ctxt_t *) context;
> >  	osm_sa_t *sa = p_ctxt->sa;
> > @@ -1811,6 +1806,8 @@ __osm_mcmr_query_mgrp(IN osm_sa_t * sa,
> >  	ib_net64_t comp_mask;
> >  	osm_physp_t *p_req_physp;
> >  	boolean_t trusted_req;
> > +	osm_mgrp_t *p_mgrp;
> > +	int index;
> >  
> >  	OSM_LOG_ENTER(sa->p_log);
> >  
> > @@ -1846,8 +1843,14 @@ __osm_mcmr_query_mgrp(IN osm_sa_t * sa,
> >  	CL_PLOCK_ACQUIRE(sa->p_lock);
> >  
> >  	/* simply go over all MCGs and match */
> > -	cl_qmap_apply_func(&sa->p_subn->mgrp_mlid_tbl,
> > -			   __osm_sa_mcm_by_comp_mask_cb, &context);
> > +	for (index = 0;
> > +	     index <= sa->p_subn->max_multicast_lid_ho - IB_LID_MCAST_START_HO;
> > +	     index++) {
> > +		p_mgrp = sa->p_subn->mgrp_mlid_tbl.mgroup[index];
> > +		if (p_mgrp) {
> > +			__osm_sa_mcm_by_comp_mask_cb(p_mgrp, &context);
> > +		}
> > +	}
> >  
> >  	CL_PLOCK_RELEASE(sa->p_lock);
> >  
> > diff --git a/opensm/opensm/osm_sa_path_record.c b/opensm/opensm/osm_sa_path_record.c
> > index 2a9155c..fb42afb 100644
> > --- a/opensm/opensm/osm_sa_path_record.c
> > +++ b/opensm/opensm/osm_sa_path_record.c
> > @@ -2,6 +2,7 @@
> >   * Copyright (c) 2004-2007 Voltaire, Inc. All rights reserved.
> >   * Copyright (c) 2002-2006 Mellanox Technologies LTD. All rights reserved.
> >   * Copyright (c) 1996-2003 Intel Corporation. All rights reserved.
> > + * Copyright (c) 2008 Xsigo Systems Inc.  All rights reserved.
> >   *
> >   * This software is available to you under a choice of one of two
> >   * licenses.  You may choose to be licensed under the terms of the GNU
> > @@ -1472,14 +1473,7 @@ __osm_pr_rcv_process_pair(IN osm_sa_t * sa,
> >  static osm_mgrp_t *__get_mgrp_by_mlid(IN osm_sa_t * sa,
> >  				      IN ib_net16_t const mlid)
> >  {
> > -	cl_map_item_t *map_item;
> > -
> > -	map_item = cl_qmap_get(&sa->p_subn->mgrp_mlid_tbl, mlid);
> > -
> > -	if (map_item == cl_qmap_end(&sa->p_subn->mgrp_mlid_tbl))
> > -		return NULL;
> > -
> > -	return (osm_mgrp_t *) map_item;
> > +        return(sa->p_subn->mgrp_mlid_tbl.mgroup[cl_ntoh16(mlid) - IB_LID_MCAST_START_HO]);
> >  }
> 
> Hmm, I thought there are two instances of __get_mgrp_by_mlid(), this one
> os third.

Yes, there were/are three instances of this code.

> >  /**********************************************************************
> > diff --git a/opensm/opensm/osm_sm.c b/opensm/opensm/osm_sm.c
> > index 7393ead..c6526e7 100644
> > --- a/opensm/opensm/osm_sm.c
> > +++ b/opensm/opensm/osm_sm.c
> > @@ -2,6 +2,7 @@
> >   * Copyright (c) 2004-2007 Voltaire, Inc. All rights reserved.
> >   * Copyright (c) 2002-2005 Mellanox Technologies LTD. All rights reserved.
> >   * Copyright (c) 1996-2003 Intel Corporation. All rights reserved.
> > + * Copyright (c) 2008 Xsigo Systems Inc.  All rights reserved.
> >   *
> >   * This software is available to you under a choice of one of two
> >   * licenses.  You may choose to be licensed under the terms of the GNU
> > @@ -498,7 +499,6 @@ osm_sm_mcgrp_join(IN osm_sm_t * const p_sm,
> >  {
> >  	osm_mgrp_t *p_mgrp;
> >  	osm_port_t *p_port;
> > -	cl_qmap_t *p_tbl;
> >  	ib_api_status_t status = IB_SUCCESS;
> >  	osm_mcm_info_t *p_mcm;
> >  
> > @@ -525,9 +525,8 @@ osm_sm_mcgrp_join(IN osm_sm_t * const p_sm,
> >  	/*
> >  	 * If this multicast group does not already exist, create it.
> >  	 */
> > -	p_tbl = &p_sm->p_subn->mgrp_mlid_tbl;
> > -	p_mgrp = (osm_mgrp_t *) cl_qmap_get(p_tbl, mlid);
> > -	if (p_mgrp == (osm_mgrp_t *) cl_qmap_end(p_tbl)) {
> > +	p_mgrp = p_sm->p_subn->mgrp_mlid_tbl.mgroup[cl_ntoh16(mlid) - IB_LID_MCAST_START_HO];
> > +	if (!p_mgrp) {
> >  		OSM_LOG(p_sm->p_log, OSM_LOG_VERBOSE,
> >  			"Creating group, MLID 0x%X\n", cl_ntoh16(mlid));
> >  
> > @@ -540,7 +539,7 @@ osm_sm_mcgrp_join(IN osm_sm_t * const p_sm,
> >  			goto Exit;
> >  		}
> >  
> > -		cl_qmap_insert(p_tbl, mlid, &p_mgrp->map_item);
> > +		p_sm->p_subn->mgrp_mlid_tbl.mgroup[cl_ntoh16(mlid) - IB_LID_MCAST_START_HO] = p_mgrp;
> >  	} else {
> >  		/*
> >  		 * The group already exists.  If the port is not a
> > @@ -603,7 +602,6 @@ osm_sm_mcgrp_leave(IN osm_sm_t * const p_sm,
> >  {
> >  	osm_mgrp_t *p_mgrp;
> >  	osm_port_t *p_port;
> > -	cl_qmap_t *p_tbl;
> >  	ib_api_status_t status = IB_SUCCESS;
> >  
> >  	OSM_LOG_ENTER(p_sm->p_log);
> > @@ -630,9 +628,8 @@ osm_sm_mcgrp_leave(IN osm_sm_t * const p_sm,
> >  	/*
> >  	 * Get the multicast group object for this group.
> >  	 */
> > -	p_tbl = &p_sm->p_subn->mgrp_mlid_tbl;
> > -	p_mgrp = (osm_mgrp_t *) cl_qmap_get(p_tbl, mlid);
> > -	if (p_mgrp == (osm_mgrp_t *) cl_qmap_end(p_tbl)) {
> > +	p_mgrp = p_sm->p_subn->mgrp_mlid_tbl.mgroup[cl_ntoh16(mlid) - IB_LID_MCAST_START_HO];
> > +	if (!p_mgrp) {
> >  		CL_PLOCK_RELEASE(p_sm->p_lock);
> >  		OSM_LOG(p_sm->p_log, OSM_LOG_ERROR, "ERR 2E08: "
> >  			"No multicast group for MLID 0x%X\n", cl_ntoh16(mlid));
> > diff --git a/opensm/opensm/osm_subnet.c b/opensm/opensm/osm_subnet.c
> > index 812b022..3e1e3b2 100644
> > --- a/opensm/opensm/osm_subnet.c
> > +++ b/opensm/opensm/osm_subnet.c
> > @@ -2,6 +2,7 @@
> >   * Copyright (c) 2004-2007 Voltaire, Inc. All rights reserved.
> >   * Copyright (c) 2002-2005 Mellanox Technologies LTD. All rights reserved.
> >   * Copyright (c) 1996-2003 Intel Corporation. All rights reserved.
> > + * Copyright (c) 2008 Xsigo Systems Inc.  All rights reserved.
> >   *
> >   * This software is available to you under a choice of one of two
> >   * licenses.  You may choose to be licensed under the terms of the GNU
> > @@ -73,6 +74,8 @@ static const char null_str[] = "(null)";
> >   **********************************************************************/
> >  void osm_subn_construct(IN osm_subn_t * const p_subn)
> >  {
> > +	int index;
> > +
> >  	memset(p_subn, 0, sizeof(*p_subn));
> >  	cl_ptr_vector_construct(&p_subn->port_lid_tbl);
> >  	cl_qmap_init(&p_subn->sw_guid_tbl);
> > @@ -84,19 +87,23 @@ void osm_subn_construct(IN osm_subn_t * const p_subn)
> >  	cl_qlist_init(&p_subn->prefix_routes_list);
> >  	cl_qmap_init(&p_subn->rtr_guid_tbl);
> >  	cl_qmap_init(&p_subn->prtn_pkey_tbl);
> > -	cl_qmap_init(&p_subn->mgrp_mlid_tbl);
> > +	for (index = 0;
> > +	     index <= p_subn->max_multicast_lid_ho - IB_LID_MCAST_START_HO;
> > +	     index++)
> > +		p_subn->mgrp_mlid_tbl.mgroup[index] = NULL;
> 
> Not needed, memset() is already here (line 3 in this function).

OK.

-- Hal

> Sasha
> 
> >  }
> >  
> >  /**********************************************************************
> >   **********************************************************************/
> >  void osm_subn_destroy(IN osm_subn_t * const p_subn)
> >  {
> > +	int index;
> >  	osm_node_t *p_node, *p_next_node;
> >  	osm_port_t *p_port, *p_next_port;
> >  	osm_switch_t *p_sw, *p_next_sw;
> >  	osm_remote_sm_t *p_rsm, *p_next_rsm;
> >  	osm_prtn_t *p_prtn, *p_next_prtn;
> > -	osm_mgrp_t *p_mgrp, *p_next_mgrp;
> > +	osm_mgrp_t *p_mgrp;
> >  	osm_infr_t *p_infr, *p_next_infr;
> >  
> >  	/* it might be a good idea to de-allocate all known objects */
> > @@ -139,12 +146,13 @@ void osm_subn_destroy(IN osm_subn_t * const p_subn)
> >  		osm_prtn_delete(&p_prtn);
> >  	}
> >  
> > -	p_next_mgrp = (osm_mgrp_t *) cl_qmap_head(&p_subn->mgrp_mlid_tbl);
> > -	while (p_next_mgrp !=
> > -	       (osm_mgrp_t *) cl_qmap_end(&p_subn->mgrp_mlid_tbl)) {
> > -		p_mgrp = p_next_mgrp;
> > -		p_next_mgrp = (osm_mgrp_t *) cl_qmap_next(&p_mgrp->map_item);
> > -		osm_mgrp_delete(p_mgrp);
> > +	for (index = 0;
> > +	     index <= p_subn->max_multicast_lid_ho - IB_LID_MCAST_START_HO;
> > +	     index++) {
> > +		p_mgrp = p_subn->mgrp_mlid_tbl.mgroup[index];
> > +		if (p_mgrp)
> > +			osm_mgrp_delete(p_mgrp);
> > +		p_subn->mgrp_mlid_tbl.mgroup[index] = NULL;
> >  	}
> >  
> >  	p_next_infr = (osm_infr_t *) cl_qlist_head(&p_subn->sa_infr_list);
> > 
> > 
> _______________________________________________
> general mailing list
> general at lists.openfabrics.org
> http://lists.openfabrics.org/cgi-bin/mailman/listinfo/general
> 
> To unsubscribe, please visit http://openib.org/mailman/listinfo/openib-general




More information about the general mailing list