[ofa-general] Re: [PATCH 2/4 v2] multicast multiplexing - implementation of newinfrastructure

[Sasha Khapyorsky]

On 13:10 Mon 20 Jul     , Slava Strebkov wrote:
> 
> > +ib_net16_t osm_mgrp_holder_get_mlid_by_mgid(IN osm_sa_t * sa,
> > +					    IN const ib_gid_t * const
> p_mgid);
> 
> Why *sa and not *subn? And why should it be in osm_mcm_info.h (which
> just describes *port*'s mcm related list)?
> -This may be moved to another file, or perhaps it's better to create a
> new file for this stuff?

I don't think that we need new file (we have enough already), move it to
osm_subnet.c (where all osm_get_*_by_*() are sitting) or
osm_multicast.c.
 
> > +	osm_mgrp_holder_prepare_common_mgid(p_mgid, &common_mgid);
> > +	for (i = 0; i <= sa->p_subn->max_mcast_lid_ho -
> IB_LID_MCAST_START_HO;
> > +	     i++) {
> > +		if (sa->p_subn->mgroup_holders[i]) {
> > +			p_mgrp_holder =
> > +			    (osm_mgrp_holder_t *)
> sa->p_subn->mgroup_holders[i];
> > +			if (!memcmp
> > +			    (&p_mgrp_holder->common_mgid, &common_mgid,
> > +			     sizeof(ib_gid_t))) {
> > +				char gid_str[INET6_ADDRSTRLEN];
> > +				OSM_LOG(sa->p_log, OSM_LOG_DEBUG,
> > +					"Found holder  0x%X for MGID
> %s\n",
> > +					cl_ntoh16(p_mgrp_holder->mlid),
> > +					inet_ntop(AF_INET6, p_mgid->raw,
> > +						  gid_str,
> sizeof(gid_str)));
> > +				OSM_LOG_EXIT(sa->p_log);
> > +				return p_mgrp_holder->mlid;
> > +			}
> > +		}
> > +	}
> 
> Hmm, why do we need linear search her? What was a purpose of storing
> mgrps in fleximap?
> -Each fleximap contains mgroups with same MLID.
> Linear search compares "common" mgid only.

So why to not use common (per subnet) fleximap where mgrp will be mapped
by mgid ("real" one)? I think it will simplify your data model and
speedup most common searches.

> > +void osm_mgrp_holder_add_mgrp(osm_mgrp_holder_t * p_mgrp_holder,
> > +			      osm_mgrp_t * p_mgrp, osm_log_t * p_log)
> > +{
> > +	cl_fmap_item_t *p_fitem;
> > +	char gid_str[INET6_ADDRSTRLEN];
> > +
> > +	OSM_LOG_ENTER(p_log);
> > +
> > +	if (p_mgrp_holder->to_be_deleted) {
> > +		/* this is re-used mgrp_holder, need to update
> common_mgid */
> > +
> osm_mgrp_holder_prepare_common_mgid(&p_mgrp->mcmember_rec.mgid,
> > +						    &p_mgrp_holder->
> > +						    common_mgid);
> > +	}
> > +	p_fitem = cl_fmap_insert(&p_mgrp_holder->mgrp_map,
> > +				 &p_mgrp->mcmember_rec.mgid,
> > +				 &p_mgrp->mgid_item);
> 
> Ok, starting to understand (hopefully :)) - you have fleximap only per
> mgrp_holder. Why? Wouldn't it better to have single (per subnet) mgrp
> fleximap? I guess this will cover all common search cases and will
> simplify a data model dramatically.
> - I don't think so. In case of single fleximap search will always take
> logN.
> Having fleximap divided into groups per MLID will give logN in a worst
> case.

logN + MLID detection (which is slow). No?

Another downside of proposed data model is its compression algorithm
awareness - it assumes MGID is compressed using signature. As I said
somewhere in previous comment it would be really nice to have many MGIDs
per MLID data transparent to used compression algorithm (it can change
in a future, be configurable, etc.).

And also I suppose that using common fleximap will require less code and
it will not be overcomplicated as it is purposed now.

> Also, keeping mgroups per MLID simplifies queries like saquery -m <MLID>

Had nothing against this.

> Also (unrelated directly to this patch series). I have some local
> cleanup/simplification changes in multicast stuff, in particular it
> eliminates the need in all those 'to_be_deleted', 'tree_id', etc. flags.
> I think this could help you with simpler implementation.  
> Sasha
> -Your changes are already in git?

In my local branch, I didn't publish it yet.

Sasha