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

[Yevgeny Kliteynik]

Hi Sasha,

Sasha Khapyorsky wrote:
> 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.

NP.

>> +  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.

NP
 
>>   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.

Agree

>> -  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).

Right, good catch.
I'll issue a new patch.
Thanks.

-- Yevgeny

> The rest is fine.
> 
> Sasha
>