[openib-general] [PATCH 2/4] Modular routing engine (unicast only yet).

[Sasha Khapyorsky]

Hi Eitan,

On 14:55 Tue 13 Jun     , Eitan Zahavi wrote:
> 
> As provided in my previous patch 1/4 comments
> I think the callbacks should also have an entry for the MinHop stage (maybe
> this is the ucast_build_fwd_tables?) I have some algorithms in mind that 
> will
> skip that stage all-together.

We may add new callback when it will be useful.

> Also it might make sense for each routing engine to provide its own "dump"
> routine such that each could support difference file format if needed.

Why we may want dump format per routing engine? Even if we are, you may
put it into routing engine specific code.

> 
> Rest of the comments are inline
> 
> EZ
> 
> Sasha Khapyorsky wrote:
> >
> >diff --git a/osm/include/opensm/osm_opensm.h 
> >b/osm/include/opensm/osm_opensm.h
> >index 3235ad4..3e6e120 100644
> >--- a/osm/include/opensm/osm_opensm.h
> >+++ b/osm/include/opensm/osm_opensm.h
> >@@ -92,6 +92,18 @@ BEGIN_C_DECLS
> > *
> > *********/
> > 
> >+/*
> >+ * routing engine structure - yet limited by ucast_fdb_assign and
> >+ *      ucast_build_fwd_tables (multicast callbacks may be added later)
> >+ */
> >+struct osm_routing_engine {
> >+	const char *name;
> >+	void *context;
> >+	int (*ucast_build_fwd_tables)(void *context);
> >+	int (*ucast_fdb_assign)(void *context);
> >+	void (*delete)(void *context);
> >+};
> It would be nice if you added a standard header to this struct.
> It is not clear to me what ucast_build_fwd_tables and
> ucast_fdb_assign are mapping to.

Ok, will add.

BTW, seems OpenSM declarations were used for generation manuals or other
docs. Do you know are those

 /****h*
 /****s*
 /****f*

in use anymore? And with what is the tool?

> Please see the next section as an example for a struct header.
> >+
> > /****s* OpenSM: OpenSM/osm_opensm_t
> > * NAME
> > *	osm_opensm_t

> >@@ -1129,6 +1144,14 @@ osm_ucast_mgr_process(
> >              i
> >              );
> > 
> >+    if (p_routing_eng->ucast_build_fwd_tables &&
> >+        p_routing_eng->ucast_build_fwd_tables(p_routing_eng->context) == 
> >0)
> >+    {
> >+      cl_qmap_apply_func( p_sw_guid_tbl,
> >+                          __osm_ucast_mgr_set_table_cb, p_mgr );
> >+    } /* fallback on the regular path in case of failures */
> >+    else
> >+    {
> Please explain why this step is needed and why if the routing engine 
> function is
> returning 0 you still invoke the standard __osm_ucast_mgr_set_table_cb.

->ucast_build_fwd_tables() creates fwd tables and
__osm_ucast_mgr_set_table_cb() upload them on the switches. In case of
->ucast_build_fwd_tables() fatal failure (when return status is != 0),
tables uploading will be skipped and flow will continue with default
routing code.

Thanks for the comments.
Sasha