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

[Eitan Zahavi]

Hi Sasha,

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.

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.

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.

Please see the next section as an example for a struct header.
> +
>  /****s* OpenSM: OpenSM/osm_opensm_t
>  * NAME
>  *	osm_opensm_t
> @@ -116,7 +128,7 @@ typedef struct _osm_opensm_t
>    osm_log_t			log;
>    cl_dispatcher_t	disp;
>    cl_plock_t		lock;
> -  updn_t         *p_updn_ucast_routing;
> +  struct osm_routing_engine routing_engine;
>    osm_stats_t		stats;
>  } osm_opensm_t;
>  /*
> @@ -153,6 +165,9 @@ typedef struct _osm_opensm_t
>  *	lock
>  *		Shared lock guarding most OpenSM structures.
>  *
> +*	routing_engine
> +*		Routing engine, will be initialized then used
> +*
>  *	stats
>  *		Open SM statistics block
>  *
> diff --git a/osm/opensm/osm_ucast_mgr.c b/osm/opensm/osm_ucast_mgr.c
> index cac7f9b..0c0d635 100644
> --- a/osm/opensm/osm_ucast_mgr.c
> +++ b/osm/opensm/osm_ucast_mgr.c
> @@ -62,6 +62,7 @@ #include <opensm/osm_node.h>
>  #include <opensm/osm_switch.h>
>  #include <opensm/osm_helper.h>
>  #include <opensm/osm_msgdef.h>
> +#include <opensm/osm_opensm.h>
>  
>  #define LINE_LENGTH 256
>  
> @@ -269,7 +270,7 @@ osm_ucast_mgr_dump_ucast_routes(
>        strcat( p_mgr->p_report_buf, "yes" );
>      else
>      {
> -      if (p_mgr->p_subn->opt.pfn_ui_ucast_fdb_assign) {
> +      if (p_mgr->p_subn->p_osm->routing_engine.ucast_fdb_assign) {
>          ui_ucast_fdb_assign_func_defined = TRUE;
>        } else {
>          ui_ucast_fdb_assign_func_defined = FALSE;
> @@ -708,7 +709,7 @@ __osm_ucast_mgr_process_port(
>    node_guid = osm_node_get_node_guid(osm_switch_get_node_ptr( p_sw ) );
>  
>    /* Flag to mark whether or not a ui ucast fdb assign function was given */
> -  if (p_mgr->p_subn->opt.pfn_ui_ucast_fdb_assign)
> +  if (p_mgr->p_subn->p_osm->routing_engine.ucast_fdb_assign)
>      ui_ucast_fdb_assign_func_defined = TRUE;
>    else
>      ui_ucast_fdb_assign_func_defined = FALSE;
> @@ -753,7 +754,7 @@ __osm_ucast_mgr_process_port(
>  
>        /* Up/Down routing can cause unreachable routes between some 
>           switches so we do not report that as an error in that case */
> -      if (!p_mgr->p_subn->opt.updn_activate)
> +      if (!p_mgr->p_subn->p_osm->routing_engine.ucast_fdb_assign)
>        {
>          osm_log( p_mgr->p_log, OSM_LOG_ERROR,
>                   "__osm_ucast_mgr_process_port: ERR 3A08: "
> @@ -973,6 +974,18 @@ __osm_ucast_mgr_process_tbl(
>  /**********************************************************************
>   **********************************************************************/
>  static void
> +__osm_ucast_mgr_set_table_cb(
> +  IN cl_map_item_t* const  p_map_item,
> +  IN void* context )
> +{
> +  osm_switch_t* const p_sw = (osm_switch_t*)p_map_item;
> +  osm_ucast_mgr_t* const p_mgr = (osm_ucast_mgr_t*)context;
> +  __osm_ucast_mgr_set_table( p_mgr, p_sw );
> +}
> +
> +/**********************************************************************
> + **********************************************************************/
> +static void
>  __osm_ucast_mgr_process_neighbors(
>    IN cl_map_item_t* const  p_map_item,
>    IN void* context )
> @@ -1058,12 +1071,14 @@ osm_ucast_mgr_process(
>  {
>    uint32_t i;
>    uint32_t iteration_max;
> +  struct osm_routing_engine *p_routing_eng;
>    osm_signal_t signal;
>    cl_qmap_t *p_sw_guid_tbl;
>  
>    OSM_LOG_ENTER( p_mgr->p_log, osm_ucast_mgr_process );
>  
>    p_sw_guid_tbl = &p_mgr->p_subn->sw_guid_tbl;
> +  p_routing_eng = &p_mgr->p_subn->p_osm->routing_engine;
>  
>    CL_PLOCK_EXCL_ACQUIRE( p_mgr->p_lock );
>  
> @@ -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.

>      /*
>        This is the place where we can load pre-defined routes
>        into the switches fwd_tbl structures.