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

[Sasha Khapyorsky]

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).

> > 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.

> > 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.

> > > @@ -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.

Sasha