[ofa-general] [PATCH] osm: up/dn optimization - improved ranking

[Sasha Khapyorsky]

Hi Yevgeny,

On 18:03 Wed 16 May     , Yevgeny Kliteynik wrote:
> Hi Hal,
> 
> This patch optimizes fabric ranking similar to the fat-tree ranking.
> All the root switches are marked with rank and added to the BFS list,
> and only then ranking of rest of the fabric begins.
> 
> Please apply to master. 
> 
> Signed-off-by: Yevgeny Kliteynik <kliteyn at dev.mellanox.co.il>
> ---

Basically looks good. However couple comments below.

> opensm/opensm/osm_ucast_updn.c |   66 
> +++++++++++++++++----------------------
> 1 files changed, 29 insertions(+), 37 deletions(-)
> 
> diff --git a/opensm/opensm/osm_ucast_updn.c b/opensm/opensm/osm_ucast_updn.c
> index 5cebd9b..9574216 100644
> --- a/opensm/opensm/osm_ucast_updn.c
> +++ b/opensm/opensm/osm_ucast_updn.c
> @@ -408,53 +408,49 @@ Exit :
> /*        rank is a SWITCH for BFS purpose */
> static int
> updn_subn_rank(
> -  IN uint64_t root_guid,
> -  IN uint8_t base_rank,
> +  IN uint32_t num_guids,

'num_guids' should not be fixed-size integer just compiler friendly
'unsigned' is fine.

> +  IN uint64_t* guid_list,
>   IN updn_t* p_updn )
> {
>   osm_switch_t *p_sw;
> -  uint32_t rank = base_rank;
>   osm_physp_t *p_physp, *p_remote_physp;
>   cl_qlist_t list;
>   cl_status_t did_cause_update;
>   struct updn_node *u, *remote_u;
>   uint8_t num_ports, port_num;
>   osm_log_t *p_log = &p_updn->p_osm->log;
> +  uint32_t idx = 0;

Ditto.

> 
>   OSM_LOG_ENTER( p_log, updn_subn_rank );
> +  cl_qlist_init(&list);
> 
> -  p_sw = osm_get_switch_by_guid(&p_updn->p_osm->subn, 
> cl_hton64(root_guid));
> -  if(!p_sw)
> -  {
> -    osm_log( p_log, OSM_LOG_ERROR,
> -             "updn_subn_rank: ERR AA05: "
> -             "Root switch GUID 0x%" PRIx64 " not found\n", root_guid );
> -    OSM_LOG_EXIT( p_log );
> -    return 1;
> -  }
> -
> -  osm_log( p_log, OSM_LOG_VERBOSE,
> -           "updn_subn_rank: "
> -           "Ranking starts from GUID 0x%" PRIx64 "\n", root_guid );
> -
> -  u = p_sw->priv;
> -  u->is_root = 1;
> +  /* Rank all the roots and add them to list */
> 
> -  /* Rank the first guid chosen anyway since it's the base rank */
> -  osm_log( p_log, OSM_LOG_DEBUG,
> -           "updn_subn_rank: "
> -           "Ranking port GUID 0x%" PRIx64 "\n", root_guid );
> +  for (idx = 0; idx < num_guids; idx++)
> +  {
> +    /* Apply the ranking for each guid given by user - bypass illegal ones 
> */
> +    p_sw = osm_get_switch_by_guid(&p_updn->p_osm->subn, 
> cl_hton64(guid_list[idx]));
> +    if(!p_sw)
> +    {
> +      osm_log( p_log, OSM_LOG_ERROR,
> +               "updn_subn_rank: ERR AA05: "
> +               "Root switch GUID 0x%" PRIx64 " not found\n", 
> guid_list[idx] );
> +      continue;
> +    }
> 
> -  __updn_update_rank(u, rank);
> +    u = p_sw->priv;
> +    u->is_root = 1;

Now when root switches are always ranked first 'is_root' field is not
needed anymore, (!u->rank) answers this.

> 
> -  cl_qlist_init(&list);
> -  cl_qlist_insert_tail(&list, &u->list);
> +    osm_log( p_log, OSM_LOG_DEBUG,
> +             "updn_subn_rank: "
> +             "Ranking root port GUID 0x%" PRIx64 "\n", guid_list[idx] );
> +    __updn_update_rank(u, 0);
> +    cl_qlist_insert_tail(&list, &u->list);
> +  }
> 
>   /* BFS the list till it's empty */
>   while (!cl_is_qlist_empty(&list))
>   {
> -    rank++;
> -
>     u = (struct updn_node *)cl_qlist_remove_head(&list);
>     /* Go over all remote nodes and rank them (if not already visited) */
>     p_sw = u->sw;
> @@ -483,7 +479,7 @@ updn_subn_rank(
>       {
>         remote_u = p_remote_physp->p_node->sw->priv;
>         port_guid = p_remote_physp->port_guid;
> -        did_cause_update = __updn_update_rank(remote_u, rank);
> +        did_cause_update = __updn_update_rank(remote_u, u->rank+1);
> 
>         osm_log( p_log, OSM_LOG_DEBUG,
>                  "updn_subn_rank: "
> @@ -500,8 +496,8 @@ updn_subn_rank(
>   /* Print Summary of ranking */
>   osm_log( p_log, OSM_LOG_VERBOSE,
>            "updn_subn_rank: "
> -           "Rank Info :\n\t Root Guid = 0x%" PRIx64 "\n\t Max Node Rank = 
> %d\n",
> -           root_guid, rank );
> +           "Subnet ranking completed. Max Node Rank = %d\n",
> +           remote_u->rank );

'remote_u' can be not initialized here. Another issue is that it can be
initialized but to remote switch which has lower than max rank (when
did_cause_update = 0).

The rest is fine.

Sasha