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

[Hal Rosenstock]

On Wed, 2008-06-25 at 01:36 +0300, Sasha Khapyorsky wrote:
> On 15:19 Tue 24 Jun     , Hal Rosenstock wrote:
> > > >  
> > > > +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 ?
> 
> Yes, please - this makes the code less readable (I spent some time in
> attempts to understand why it was done so).

OK.

> > > 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.
> 
> But check about possibly needed cl_ntoh16(p_mcm->mlid) - I don't remember
> how mcm stores mlid.

It's in net format so does need the conversion.

> > > 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 ?
> 
> Yes, if it is simpler for you. I would make osm_get_mgrp_by_mlid() as
> separate patch even before to array converting, but it is really minor.

Sure; it could be done before, during, or after this patch. I do prefer
to do it after.

> > > > @@ -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 ?
> 
> I would prefer to have this improved in v2 of the patch. This function
> was never ideal, but now it looks (and works) really bad.

It's no worse than it was; can't it wait a little ?

-- Hal

> Sasha
> _______________________________________________
> 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