[openib-general] Re: A couple of questions about osm_lid_mgr.c::__osm_lid_mgr_set_physp_pi

[Eitan Zahavi]

Hal Rosenstock wrote:
> Hi,
> 
> I have a couple of questions about
> osm_lid_mgr.c::__osm_lid_mgr_set_physp_pi.
> 
> There is the following code:
> 
>     if ( (mtu != ib_port_info_get_mtu_cap( p_old_pi )) ||
>          (op_vls !=  ib_port_info_get_op_vls(p_old_pi)))
>     {
>       if( osm_log_is_active( p_mgr->p_log, OSM_LOG_DEBUG ) )
>       {
>         osm_log( p_mgr->p_log, OSM_LOG_DEBUG,
>                  "__osm_lid_mgr_set_physp_pi: "
>                  "Sending Link Down due to op_vls or mtu change.
> MTU:%u,%u VL_CAP:%u,%u\n",
>                  mtu, ib_port_info_get_mtu_cap( p_old_pi ),
>                  op_vls, ib_port_info_get_op_vls(p_old_pi)
>                  );
>       }
>       ib_port_info_set_port_state( p_pi, IB_LINK_DOWN );
> 
> This seems a little inconsistent to me. It seems like NeighborMTU would
> be the equivalent of OperationalVLs, rather than MTUCap (which is RO).
Yes I think we should have checked the NeighborMTU and not the MTUCap.

> Also, why does changing the MTU require that the link be taken down ?
The behavior of the link when a neighbor MTU is changes is not very well defined.
So the best way to handle that is to force it down.

> 
> I also noticed a nit in the same function:
> 
>   p_pi->m_key_lease_period = p_mgr->p_subn->opt.m_key_lease_period;
>   /* Check to see if the value we are setting is different than
>      the value in the port_info. If it is - turn on send_set flag */
>   if (cl_memcmp( &p_pi->m_key_lease_period,
> &p_old_pi->m_key_lease_period,
>                  sizeof(p_pi->m_key_lease_period) ))
>     send_set = TRUE;
> 
> Should that be only when the Mkey is non 0 ?
Well, I know the lease is not relevant when MKey = 0. But for code clarity I
propose to ignore that fact. The effect is only when someone set lease period but MKey = 0
which IMO does not make any sense anyway.
> 
> -- Hal
>