[ofa-general] Re: [PATCH] osm: bug in dumping opensm.fdbs

[Yevgeny Kliteynik]

Sasha Khapyorsky wrote:
> On 00:28 Sat 07 Jul     , Yevgeny Kliteynik wrote:
>>  Hi Sasha,
>>
>>  Sasha Khapyorsky wrote:
>>> Hi Yevgeny,
>>> On 10:43 Thu 05 Jul     , Yevgeny Kliteynik wrote:
>>>>  Hi Hal,
>>>>
>>>>  opensm.fdbs dump function adaptation to the recent changes in min hop 
>>>> tables
>>>>  broke fat-tree routing (or any other future routing that may not use the  
>>>> same
>>>>  min hop tables creation functions).
>>> Could you please explain how this dump function break the routing for
>>> fat-tree? Thanks.
>>  Example:
>>    - We're dumping table for switch SW_A, and the target is CA.
>>    - To get to CA from SW_A, there are at leas two options:
>>        1. SW_A->...->SW_X->...->SW_B->CA
>>        2. SW_A->...->SW_Y->...->SW_B->CA
>>    - Fat-tree may chose to go through SW_X when routing from SW_A to CA,
>>      and through SW_Y when routing from SW_A to SW_B, hence it might chose
>>      different ports on SW_A
> 
> Ok, so your are refering incorrect dumping info, and not the routing
> itself?

Yes, sorry if I wasn't clear on this.

>>  In the recent optimization for MinHop and Up/Dn, min hop tables creation is 
>>  done
>>  only for switches,
> 
> BTW is such optimization is suitable for fat-tree engine?

No. In fact, fat-tree doesn't use these tables for routing - it creates them
as a routing by-product w/o any additional complexity.

-- Yevgeny

> Sasha
> 
>> and in order to go from SW_A to CA the algorithm checks 
>>  which
>>  switch is connected to CA (SW_B in this case), and choses the port on SW_A 
>>  that
>>  routes to SW_B, hence routes to SW_B and CA have to be the same (except for 
>>  the
>>  last SW_B->CA hop).
>>
>>  -- Yevgeny
>>
>>> Sasha
>>>>  -- Yevgeny
>>>>
>>>>  Signed-off-by: Yevgeny Kliteynik <kliteyn at dev.mellanox.co.il>
>>>>  ---
>>>>   opensm/opensm/osm_ucast_mgr.c |   33 ++++++++++++++++++++++++---------
>>>>   1 files changed, 24 insertions(+), 9 deletions(-)
>>>>
>>>>  diff --git a/opensm/opensm/osm_ucast_mgr.c 
>>>> b/opensm/opensm/osm_ucast_mgr.c
>>>>  index 5bcb655..cab272e 100644
>>>>  --- a/opensm/opensm/osm_ucast_mgr.c
>>>>  +++ b/opensm/opensm/osm_ucast_mgr.c
>>>>  @@ -242,6 +242,7 @@ __osm_ucast_mgr_dump_path_distribution(
>>>>
>>>>   /**********************************************************************
>>>>    **********************************************************************/
>>>>  +
>>>>   static void
>>>>   __osm_ucast_mgr_dump_ucast_routes(
>>>>     IN cl_map_item_t *p_map_item,
>>>>  @@ -255,6 +256,7 @@ __osm_ucast_mgr_dump_ucast_routes(
>>>>     uint8_t                  best_port;
>>>>     uint16_t                 max_lid_ho;
>>>>     uint16_t                 lid_ho, base_lid;
>>>>  +  boolean_t                direct_route_exists = FALSE;
>>>>     osm_switch_t* p_sw = (osm_switch_t *)p_map_item;
>>>>     osm_ucast_mgr_t* p_mgr = ((struct ucast_mgr_dump_context 
>>>> *)cxt)->p_mgr;
>>>>     FILE *file = ((struct ucast_mgr_dump_context *)cxt)->file;
>>>>  @@ -300,22 +302,35 @@ __osm_ucast_mgr_dump_ucast_routes(
>>>>       */
>>>>       if( p_port->p_node->sw )
>>>>       {
>>>>  +      /* Target LID is switch.
>>>>  +         Get its base lid and check hop count for this base LID only.*/
>>>>         base_lid = osm_node_get_base_lid(p_port->p_node, 0);
>>>>         base_lid = cl_ntoh16(base_lid);
>>>>         num_hops = osm_switch_get_hop_count( p_sw, base_lid, port_num );
>>>>       }
>>>>       else
>>>>       {
>>>>  -      osm_physp_t *p_physp = p_port->p_physp;
>>>>  -      if( !p_physp || !p_physp->p_remote_physp ||
>>>>  -          !p_physp->p_remote_physp->p_node->sw )
>>>>  -        num_hops = OSM_NO_PATH;
>>>>  +      /* Target LID is not switch (CA or router).
>>>>  +         Check if we have route to this target from current switch.*/
>>>>  +      num_hops = osm_switch_get_hop_count( p_sw, lid_ho, port_num );
>>>>  +      if (num_hops != OSM_NO_PATH)
>>>>  +      {
>>>>  +          direct_route_exists = TRUE;
>>>>  +          base_lid = lid_ho;
>>>>  +      }
>>>>         else
>>>>         {
>>>>  -        base_lid = 
>>>> osm_node_get_base_lid(p_physp->p_remote_physp->p_node,  0);
>>>>  -        base_lid = cl_ntoh16(base_lid);
>>>>  -        num_hops = p_physp->p_remote_physp->p_node->sw == p_sw ?
>>>>  -                   0 : osm_switch_get_hop_count( p_sw, base_lid, 
>>>> port_num  );
>>>>  +        osm_physp_t *p_physp = p_port->p_physp;
>>>>  +        if( !p_physp || !p_physp->p_remote_physp ||
>>>>  +            !p_physp->p_remote_physp->p_node->sw )
>>>>  +          num_hops = OSM_NO_PATH;
>>>>  +        else
>>>>  +        {
>>>>  +          base_lid = 
>>>> osm_node_get_base_lid(p_physp->p_remote_physp->p_node,  0);
>>>>  +          base_lid = cl_ntoh16(base_lid);
>>>>  +          num_hops = p_physp->p_remote_physp->p_node->sw == p_sw ?
>>>>  +                     0 : osm_switch_get_hop_count( p_sw, base_lid, 
>>>> port_num  );
>>>>  +        }
>>>>         }
>>>>       }
>>>>
>>>>  @@ -326,7 +341,7 @@ __osm_ucast_mgr_dump_ucast_routes(
>>>>       }
>>>>
>>>>       best_hops = osm_switch_get_least_hops( p_sw, base_lid );
>>>>  -    if (!p_port->p_node->sw)
>>>>  +    if (!p_port->p_node->sw && !direct_route_exists)
>>>>       {
>>>>         best_hops++;
>>>>         num_hops++;
>>>>  --  1.5.1.4
>>>>
>>>>
>