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
_______________________________________________
general mailing list
[email protected]
http://lists.openfabrics.org/cgi-bin/mailman/listinfo/general
To unsubscribe, please visit http://openib.org/mailman/listinfo/openib-general
