[ofa-general] Re: [PATCH] opensm: routing chaining

[Sasha Khapyorsky]

Hi Al,

Thanks for the comments. Answers are below.

On 15:17 Mon 29 Sep     , Al Chu wrote:
> > diff --git a/opensm/opensm/osm_opensm.c b/opensm/opensm/osm_opensm.c
> > index d17fed3..4970d0c 100644
> > --- a/opensm/opensm/osm_opensm.c
> > +++ b/opensm/opensm/osm_opensm.c
> > @@ -61,24 +61,23 @@
> >  
> >  struct routing_engine_module {
> >  	const char *name;
> > -	int (*setup) (osm_opensm_t * p_osm);
> > +	int (*setup) (struct osm_routing_engine *, osm_opensm_t *);
> >  };
> >  
> > -extern int osm_ucast_updn_setup(osm_opensm_t * p_osm);
> > -extern int osm_ucast_file_setup(osm_opensm_t * p_osm);
> > -extern int osm_ucast_ftree_setup(osm_opensm_t * p_osm);
> > -extern int osm_ucast_lash_setup(osm_opensm_t * p_osm);
> > -
> > -static int osm_ucast_null_setup(osm_opensm_t * p_osm);
> > +extern int osm_ucast_minhop_setup(struct osm_routing_engine *, osm_opensm_t *);
> > +extern int osm_ucast_updn_setup(struct osm_routing_engine *, osm_opensm_t *);
> > +extern int osm_ucast_file_setup(struct osm_routing_engine *, osm_opensm_t *);
> > +extern int osm_ucast_ftree_setup(struct osm_routing_engine *, osm_opensm_t *);
> > +extern int osm_ucast_lash_setup(struct osm_routing_engine *, osm_opensm_t *);
> > +extern int osm_ucast_dor_setup(struct osm_routing_engine *, osm_opensm_t *);
> >  
> >  const static struct routing_engine_module routing_modules[] = {
> > -	{"null", osm_ucast_null_setup},
> 
> Not sure how much legacy opensm.opts files are out there, but I kept the
> "null" routing engine in there just for safety.  Is it ok to remove?

I think it is good time to remove - we changed OpenSM configuration and
opensm,opts is not used anymore. Also even in case when "null" (or any
other unknown name) will be used minhop routing engine will be used
instead (as default).

> > -	{"minhop", osm_ucast_null_setup},
> > +	{"minhop", osm_ucast_minhop_setup},
> >  	{"updn", osm_ucast_updn_setup},
> >  	{"file", osm_ucast_file_setup},
> >  	{"ftree", osm_ucast_ftree_setup},
> >  	{"lash", osm_ucast_lash_setup},
> > -	{"dor", osm_ucast_null_setup},
> > +	{"dor", osm_ucast_dor_setup},
> >  	{NULL, NULL}
> >  };
> >  

> > @@ -861,3 +871,28 @@ Exit:
> >  	OSM_LOG_EXIT(p_mgr->p_log);
> >  	return (signal);
> >  }
> > +
> > +static int ucast_build_lid_matrices(void *context)
> > +{
> > +	return osm_ucast_mgr_build_lid_matrices(context);
> > +}
> > +
> > +static int ucast_build_lfts(void *context)
> > +{
> > +	return ucast_mgr_build_lfts(context);
> > +}
> > +
> > +int osm_ucast_minhop_setup(struct osm_routing_engine *r, osm_opensm_t *osm)
> > +{
> > +	r->context = &osm->sm.ucast_mgr;
> > +	r->build_lid_matrices = ucast_build_lid_matrices;
> > +	r->ucast_build_fwd_tables = ucast_build_lfts;
> > +	return 0;
> > +}
> > +
> > +int osm_ucast_dor_setup(struct osm_routing_engine *r, osm_opensm_t *osm)
> > +{
> > +	osm_ucast_minhop_setup(r, osm);
> > +	osm->sm.ucast_mgr.is_dor = 1;
> 
> If dor is listed in the routing chain, all other algorithms that may
> fall-through into minhop's build_lfts callback (minhop, updn, file),
> will be affected by the is_dor flag.  Is this intended?

It is just bug (wanted to handle this, but forgot... :( ).

> If we don't want to abstract it for this round, perhaps we could stick
> the "is_dor" flag set/unset into ucast_mgr_route() so that is_dor is set
> only when dor is being routed.

I think we may define ucast_build_fwd_tables method for dor, something
like:

static int ucast_dor_build_lfts(void *context)
{
	osm_ucast_mgr_t *mgr = context;
	int ret;

	mgr->is_dor = 1;
	ret = ucast_mgr_build_lfts(mgr);
	mgr->is_dor = 0;

	return ret;
}

I will fix this.

> The patch looks fine as whole.

Thanks!

Sasha