[openib-general] [PATCH] osm: PathRecord prefer 1K MTU for MT23108 devices

Sasha Khapyorsky sashak at voltaire.com
Fri Sep 15 17:04:32 PDT 2006


On 19:06 Fri 15 Sep     , Hal Rosenstock wrote:
> Hi Sasha,
> 
> On Fri, 2006-09-15 at 18:17, Sasha Khapyorsky wrote:
> > Hi Eitan,
> > 
> > Some comments about the patch.
> > 
> > On 14:45 Fri 15 Sep     , Eitan Zahavi wrote:
> > > Hi Hal
> > > 
> > > The following patch solves an issue with OpenSM preferring largest MTU 
> > > for PathRecord/MultiPathRecord for paths going to or from MT23108 (Tavor)
> > > devices instead of using a 1K MTU which is best for this device.
> > > 
> > > Since this is a device specific quirk I have added a configuration option
> > > named enable_quirks which is FALSE by default to enable this functionality.
> > > 
> > > To summarize the functionality change:
> > > 1. Added enable_quirks option 
> > > 2. If enable_quirks is FALSE do nothing
> > 
> > I see those quirks are SA specific. Then should this option be called
> > 'enable_sa_quirks' instead?
> 
> Not sure what the right "granularity" is for this. Would all quirks be
> enabled at once or would this end up being a pick and choose ?

Of course this matters how we define this (so I was asking). Right now I
see that this is used for SA.

Sasha

> 
> > > 3. If a specific MTU is requested (either =2K or >1K) do nothing
> > > 4. If either source port or destination port is a Tavor device  
> > > 	MTU is limited to 1K (can be further reduced by path traversal) 
> > > 
> > > Target is both trunk and OFED 1.1
> > > 
> > > Thanks
> > > 
> > > Eitan
> > > 
> > > Signed-off-by:  Eitan Zahavi <eitan at mellanox.co.il>
> > > 
> > > Index: include/opensm/osm_subnet.h
> > > ===================================================================
> > > --- include/opensm/osm_subnet.h	(revision 9493)
> > > +++ include/opensm/osm_subnet.h	(working copy)
> > > @@ -286,6 +286,7 @@ typedef struct _osm_subn_opt
> > >    osm_qos_options_t        qos_sw0_options;
> > >    osm_qos_options_t        qos_swe_options;
> > >    osm_qos_options_t        qos_rtr_options;
> > > +  boolean_t                enable_quirks;
> > >  } osm_subn_opt_t;
> > >  /*
> > >  * FIELDS
> > > @@ -469,6 +470,10 @@ typedef struct _osm_subn_opt
> > >  *	qos_rtr_options
> > >  *		QoS options for router ports
> > >  *
> > > +*  enable_quirks
> > > +*     Enable high risk new features and not fully qualified 
> > > +*     hardware specific work arounds
> > > +*
> > >  * SEE ALSO
> > >  *	Subnet object
> > >  *********/
> > > Index: include/opensm/osm_base.h
> > > ===================================================================
> > > --- include/opensm/osm_base.h	(revision 9493)
> > > +++ include/opensm/osm_base.h	(working copy)
> > > @@ -778,6 +778,34 @@ typedef enum _osm_mcast_req_type
> > >  #define MAX_UPDN_GUID_FILE_LINE_LENGTH 120
> > >  /**********/
> > >  
> > > +/****s* OpenSM: Base/VendorOUIs
> > > +* NAME
> > > +*	VendorOUIs
> > > +*
> > > +* DESCRIPTION
> > > +*	Known device vendor ID and GUID OUIs
> > > +*
> > > +* SYNOPSIS
> > > +*/
> > > +#define OSM_VENDOR_ID_INTEL         0x00D0B7
> > > +#define OSM_VENDOR_ID_MELLANOX      0x0002C9
> > > +#define OSM_VENDOR_ID_REDSWITCH     0x000617
> > > +#define OSM_VENDOR_ID_SILVERSTORM   0x00066A
> > > +#define OSM_VENDOR_ID_TOPSPIN       0x0005AD
> > > +#define OSM_VENDOR_ID_FUJITSU       0x00E000
> > > +#define OSM_VENDOR_ID_FUJITSU2      0x000B5D
> > > +#define OSM_VENDOR_ID_VOLTAIRE      0x0008F1
> > > +#define OSM_VENDOR_ID_YOTTAYOTTA    0x000453
> > > +#define OSM_VENDOR_ID_PATHSCALE     0x001175
> > > +#define OSM_VENDOR_ID_IBM           0x000255
> > > +#define OSM_VENDOR_ID_DIVERGENET    0x00084E
> > > +#define OSM_VENDOR_ID_FLEXTRONICS   0x000B8C
> > > +#define OSM_VENDOR_ID_AGILENT       0x0030D3
> > > +#define OSM_VENDOR_ID_OBSIDIAN      0x001777
> > > +#define OSM_VENDOR_ID_BAYMICRO      0x000BC1
> > > +#define OSM_VENDOR_ID_LSILOGIC      0x00A0B8
> > > +/**********/
> > > +
> > >  END_C_DECLS
> > >  
> > >  #endif	/* _OSM_BASE_H_ */
> > > Index: opensm/osm_sa_multipath_record.c
> > > ===================================================================
> > > --- opensm/osm_sa_multipath_record.c	(revision 9493)
> > > +++ opensm/osm_sa_multipath_record.c	(working copy)
> > > @@ -150,6 +150,75 @@ osm_mpr_rcv_init(
> > >  
> > >  /**********************************************************************
> > >   **********************************************************************/
> > > +static inline boolean_t
> > > +__osm_sa_multipath_rec_is_tavor_port(
> > > +	IN const osm_port_t*     const p_port)
> > > +{
> > > +	osm_node_t const* p_node;
> > > +	ib_net32_t vend_id;
> > > +
> > > +	p_node = osm_port_get_parent_node( p_port );
> > > +	vend_id = ib_node_info_get_vendor_id( &p_node->node_info );
> > > +	
> > > +	return( (p_node->node_info.device_id == CL_HTON16(23108)) &&
> > > +			  ((vend_id == CL_HTON32(OSM_VENDOR_ID_MELLANOX)) || 
> > > +				(vend_id == CL_HTON32(OSM_VENDOR_ID_TOPSPIN)) || 
> > > +				(vend_id == CL_HTON32(OSM_VENDOR_ID_SILVERSTORM)) || 
> > > +				(vend_id == CL_HTON32(OSM_VENDOR_ID_VOLTAIRE))));
> > > +}
> > > +
> > > +/**********************************************************************
> > > + **********************************************************************/
> > > +boolean_t
> > > + __osm_sa_multipath_rec_apply_tavor_mtu_limit(
> > > +  IN const ib_multipath_rec_t*  const p_mpr,
> > > +  IN const osm_port_t*          const p_src_port,
> > > +  IN const osm_port_t*          const p_dest_port,
> > > +  IN const ib_net64_t           comp_mask)
> > > +{
> > > +	uint8_t   required_mtu;
> > > +	
> > > +	/* only if one of the ports is a Tavor device */
> > > +	if (! __osm_sa_multipath_rec_is_tavor_port(p_src_port) && 
> > > +		 ! __osm_sa_multipath_rec_is_tavor_port(p_dest_port) )
> > > +		return( FALSE );
> > > +	
> > > +	/*
> > > +	  we can apply the patch if either:
> > > +	  1. No MTU required
> > > +	  2. Required MTU < 
> > > +	  3. Required MTU = 1K or 512 or 256
> > > +	  4. Required MTU > 256 or 512
> > > +	*/
> > > +	required_mtu = ib_multipath_rec_mtu( p_mpr );
> > > +	if ( ( comp_mask & IB_PR_COMPMASK_MTUSELEC ) &&
> > > +		  ( comp_mask & IB_PR_COMPMASK_MTU ) )
> > 
> > Should here be IB_MPR_COMPMASK_* instead of IB_PR_COMPMASK_*?
> 
> Good catch.
> 
> > > +	{
> > > +		switch( ib_multipath_rec_mtu_sel( p_mpr ) )
> > > +		{
> > > +		case 0:    /* must be greater than */
> > > +		case 2:    /* exact match */
> > > +			if( IB_MTU_LEN_1024 < required_mtu )
> > > +				return(FALSE);
> > > +			break;
> > > +
> > > +		case 1:    /* must be less than */
> > > +		case 3:    /* largest available */
> > > +			/* can't be disqualified by this one */
> > > +			break;
> > > +			
> > > +		default:
> > > +			/* if we're here, there's a bug in ib_path_rec_mtu_sel() */
> > > +			CL_ASSERT( FALSE );
> > > +			break;
> > > +		}
> > > +	}
> > > +
> > > +	return(TRUE);
> > > +}
> > > +
> > > +/**********************************************************************
> > > + **********************************************************************/
> > >  static ib_api_status_t
> > >  __osm_mpr_rcv_get_path_parms(
> > >    IN osm_mpr_rcv_t*		const p_rcv,
> > > @@ -195,6 +264,23 @@ __osm_mpr_rcv_get_path_parms(
> > >    mtu = ib_port_info_get_mtu_cap( p_pi );
> > >    rate = ib_port_info_compute_rate( p_pi );
> > >  
> > > +  /* 
> > > +	  Mellanox Tavor device performance is better using 1K MTU.
> > > +	  If required MTU and MTU selector are such that 1K is OK 
> > > +	  and one of the ends of the path is Tavor we override the
> > > +	  port MTU with 1K.
> > > +  */
> > > +  if ( p_rcv->p_subn->opt.enable_quirks &&
> > > +		 __osm_sa_multipath_rec_apply_tavor_mtu_limit(
> > > +			 p_mpr, p_src_port, p_dest_port, comp_mask) )
> > > +	  if (mtu > IB_MTU_LEN_1024) 
> > > +	  {
> > > +		  mtu = IB_MTU_LEN_1024;
> > > +  		  osm_log( p_rcv->p_log, OSM_LOG_DEBUG,
> > > +					  "__osm_mpr_rcv_get_path_parms: "
> > > +					  "Optimized Path MTU to 1K for Mellanox Tavor device\n");
> > > +	  }
> > > +
> > 
> > This part is pure hardcode, isn't it? Could this be at least isolated in
> > single call 'osm_*_do_quirks()' or like thin?
> 
> Perhaps. This can be worked on the trunk.
> 
> > >    if ( comp_mask & IB_MPR_COMPMASK_RAWTRAFFIC &&
> > >         cl_ntoh32( p_mpr->hop_flow_raw ) & ( 1<<31 ) )
> > >      required_pkey = osm_physp_find_common_pkey( p_physp, p_dest_physp );
> > > Index: opensm/osm_subnet.c
> > > ===================================================================
> > > --- opensm/osm_subnet.c	(revision 9493)
> > > +++ opensm/osm_subnet.c	(working copy)
> > > @@ -494,6 +494,7 @@ osm_subn_set_default_opt(
> > >    p_opt->ucast_dump_file = NULL;
> > >    p_opt->updn_guid_file = NULL;
> > >    p_opt->exit_on_fatal = TRUE;
> > > +  p_opt->enable_quirks = FALSE;
> > >    subn_set_default_qos_options(&p_opt->qos_options);
> > >    subn_set_default_qos_options(&p_opt->qos_ca_options);
> > >    subn_set_default_qos_options(&p_opt->qos_sw0_options);
> > > @@ -979,6 +980,10 @@ osm_subn_parse_conf_file(
> > >        subn_parse_qos_options("qos_rtr",
> > >          p_key, p_val, &p_opts->qos_rtr_options);
> > >  
> > > +      __osm_subn_opts_unpack_boolean(
> > > +        "enable_quirks",
> > > +        p_key, p_val, &p_opts->enable_quirks);
> > > +
> > >      }
> > >    }
> > >    fclose(opts_file);
> > > @@ -1179,11 +1184,15 @@ osm_subn_write_conf_file(
> > >      "force_log_flush %s\n\n"
> > >      "# Log file to be used\n"
> > >      "log_file %s\n\n"
> > > +	 "# Limit the the size of the log file. If overrun log is restarted\n"
> > >      "log_max_size %lu\n\n"
> > > +	 "# If TRUE will accumulate the log over multiple OpenSM sessions\n"
> > >      "accum_log_file %s\n\n"
> > >      "# The directory to hold the file OpenSM dumps\n"
> > >      "dump_files_dir %s\n\n"
> > > -    "# If TRUE if OpenSM should disable multicast support\n"
> > > +	 "# If TRUE enables new high risk options and hardware specific quirks\n"
> > > +	 "enable_quirks %s\n\n"
> > > +    "# If TRUE OpenSM should disable multicast support\n"  
> > >      "no_multicast_option %s\n\n"
> > >      "# No multicast routing is performed if TRUE\n"
> > >      "disable_multicast %s\n\n"
> > > @@ -1195,6 +1204,7 @@ osm_subn_write_conf_file(
> > >      p_opts->log_max_size,
> > >      p_opts->accum_log_file ? "TRUE" : "FALSE",
> > >      p_opts->dump_files_dir,
> > > +    p_opts->enable_quirks ? "TRUE" : "FALSE",
> > >      p_opts->no_multicast_option ? "TRUE" : "FALSE",
> > >      p_opts->disable_multicast ? "TRUE" : "FALSE",
> > >      p_opts->exit_on_fatal ? "TRUE" : "FALSE"
> > > Index: opensm/osm_helper.c
> > > ===================================================================
> > > --- opensm/osm_helper.c	(revision 9493)
> > > +++ opensm/osm_helper.c	(working copy)
> > > @@ -2289,24 +2289,6 @@ osm_get_node_type_str_fixed_width(
> > >    return( __osm_node_type_str_fixed_width[node_type] );
> > >  }
> > >  
> > > -#define OSM_VENDOR_ID_INTEL         0x00D0B7
> > > -#define OSM_VENDOR_ID_MELLANOX      0x0002C9
> > > -#define OSM_VENDOR_ID_REDSWITCH     0x000617
> > > -#define OSM_VENDOR_ID_SILVERSTORM     0x00066A
> > > -#define OSM_VENDOR_ID_TOPSPIN    0x0005AD
> > > -#define OSM_VENDOR_ID_FUJITSU    0x00E000
> > > -#define OSM_VENDOR_ID_FUJITSU2      0x000B5D
> > > -#define OSM_VENDOR_ID_VOLTAIRE   0x0008F1
> > > -#define OSM_VENDOR_ID_YOTTAYOTTA    0x000453
> > > -#define OSM_VENDOR_ID_PATHSCALE     0x001175
> > > -#define OSM_VENDOR_ID_IBM           0x000255
> > > -#define OSM_VENDOR_ID_DIVERGENET    0x00084E
> > > -#define OSM_VENDOR_ID_FLEXTRONICS   0x000B8C
> > > -#define OSM_VENDOR_ID_AGILENT       0x0030D3
> > > -#define OSM_VENDOR_ID_OBSIDIAN      0x001777
> > > -#define OSM_VENDOR_ID_BAYMICRO      0x000BC1
> > > -#define OSM_VENDOR_ID_LSILOGIC      0x00A0B8
> > > -
> > >  /**********************************************************************
> > >   **********************************************************************/
> > >  const char*
> > > Index: opensm/osm_sa_path_record.c
> > > ===================================================================
> > > --- opensm/osm_sa_path_record.c	(revision 9493)
> > > +++ opensm/osm_sa_path_record.c	(working copy)
> > > @@ -57,6 +57,7 @@
> > >  #include <complib/cl_passivelock.h>
> > >  #include <complib/cl_debug.h>
> > >  #include <complib/cl_qlist.h>
> > > +#include <opensm/osm_base.h>
> > >  #include <opensm/osm_sa_path_record.h>
> > >  #include <opensm/osm_port.h>
> > >  #include <opensm/osm_node.h>
> > > @@ -150,6 +151,75 @@ osm_pr_rcv_init(
> > >  
> > >  /**********************************************************************
> > >   **********************************************************************/
> > > +static inline boolean_t
> > > +__osm_sa_path_rec_is_tavor_port(
> > > +	IN const osm_port_t*     const p_port)
> > > +{
> > > +	osm_node_t const* p_node;
> > > +	ib_net32_t vend_id;
> > > +
> > > +	p_node = osm_port_get_parent_node( p_port );
> > > +	vend_id = ib_node_info_get_vendor_id( &p_node->node_info );
> > > +	
> > > +	return( (p_node->node_info.device_id == CL_HTON16(23108)) &&
> > > +			  ((vend_id == CL_HTON32(OSM_VENDOR_ID_MELLANOX)) || 
> > > +				(vend_id == CL_HTON32(OSM_VENDOR_ID_TOPSPIN)) || 
> > > +				(vend_id == CL_HTON32(OSM_VENDOR_ID_SILVERSTORM)) || 
> > > +				(vend_id == CL_HTON32(OSM_VENDOR_ID_VOLTAIRE))));
> > > +}
> > > +
> > > +/**********************************************************************
> > > + **********************************************************************/
> > > +static boolean_t
> > > + __osm_sa_path_rec_apply_tavor_mtu_limit(
> > > +  IN const ib_path_rec_t*  const p_pr,
> > > +  IN const osm_port_t*     const p_src_port,
> > > +  IN const osm_port_t*     const p_dest_port,
> > > +  IN const ib_net64_t      comp_mask)
> > > +{
> > > +	uint8_t   required_mtu;
> > > +	
> > > +	/* only if one of the ports is a Tavor device */
> > > +	if (! __osm_sa_path_rec_is_tavor_port(p_src_port) && 
> > > +		 ! __osm_sa_path_rec_is_tavor_port(p_dest_port) )
> > > +		return( FALSE );
> > > +	
> > > +	/*
> > > +	  we can apply the patch if either:
> > > +	  1. No MTU required
> > > +	  2. Required MTU < 
> > > +	  3. Required MTU = 1K or 512 or 256
> > > +	  4. Required MTU > 256 or 512
> > > +	*/
> > > +	required_mtu = ib_path_rec_mtu( p_pr );
> > > +	if ( ( comp_mask & IB_PR_COMPMASK_MTUSELEC ) &&
> > > +		  ( comp_mask & IB_PR_COMPMASK_MTU ) )
> > > +	{
> > > +		switch( ib_path_rec_mtu_sel( p_pr ) )
> > > +		{
> > > +		case 0:    /* must be greater than */
> > > +		case 2:    /* exact match */
> > > +			if( IB_MTU_LEN_1024 < required_mtu )
> > > +				return(FALSE);
> > > +			break;
> > > +
> > > +		case 1:    /* must be less than */
> > > +		case 3:    /* largest available */
> > > +			/* can't be disqualified by this one */
> > > +			break;
> > > +			
> > > +		default:
> > > +			/* if we're here, there's a bug in ib_path_rec_mtu_sel() */
> > > +			CL_ASSERT( FALSE );
> > > +			break;
> > > +		}
> > > +	}
> > > +
> > > +	return(TRUE);
> > > +}
> > > +
> > > +/**********************************************************************
> > > + **********************************************************************/
> > >  static ib_api_status_t
> > >  __osm_pr_rcv_get_path_parms(
> > >    IN osm_pr_rcv_t*         const p_rcv,
> > > @@ -191,6 +261,23 @@ __osm_pr_rcv_get_path_parms(
> > >    mtu = ib_port_info_get_mtu_cap( p_pi );
> > >    rate = ib_port_info_compute_rate( p_pi );
> > >  
> > > +  /* 
> > > +	  Mellanox Tavor device performance is better using 1K MTU.
> > > +	  If required MTU and MTU selector are such that 1K is OK 
> > > +	  and one of the ends of the path is Tavor we override the
> > > +	  port MTU with 1K.
> > > +  */
> > > +  if (  p_rcv->p_subn->opt.enable_quirks &&
> > > +		  __osm_sa_path_rec_apply_tavor_mtu_limit(
> > > +			  p_pr, p_src_port, p_dest_port, comp_mask) )
> > > +	  if (mtu > IB_MTU_LEN_1024) 
> > > +	  {
> > > +		  mtu = IB_MTU_LEN_1024;
> > > +		  osm_log( p_rcv->p_log, OSM_LOG_DEBUG,
> > > +					  "__osm_pr_rcv_get_path_parms: "
> > > +					  "Optimized Path MTU to 1K for Mellanox Tavor device\n");
> > > +	  }
> > > +
> > 
> > The same is here (about hardcodes).
> > 
> > Also I see that tavor specific functions are pretty similar for PR and
> > MPR cases. Why not to share this in something like osm_sa_quirks.c?
> 
> I think we can work on this on the trunk and see if there is an OFED 1.1
> opening.
> 
> -- Hal
> 
> > Sasha
> > 
> > >    /*
> > >      Walk the subnet object from source to destination,
> > >      tracking the most restrictive rate and mtu values along the way...
> > > @@ -444,10 +531,10 @@ __osm_pr_rcv_get_path_parms(
> > >    */
> > >  
> > >    /* we silently ignore cases where only the MTU selector is defined */
> > > +  required_mtu = ib_path_rec_mtu( p_pr );
> > >    if ( ( comp_mask & IB_PR_COMPMASK_MTUSELEC ) &&
> > >         ( comp_mask & IB_PR_COMPMASK_MTU ) )
> > >    {
> > > -    required_mtu = ib_path_rec_mtu( p_pr );
> > >      switch( ib_path_rec_mtu_sel( p_pr ) )
> > >      {
> > >      case 0:    /* must be greater than */
> > > 
> > > 
> > > _______________________________________________
> > > openib-general mailing list
> > > openib-general at openib.org
> > > http://openib.org/mailman/listinfo/openib-general
> > > 
> > > To unsubscribe, please visit http://openib.org/mailman/listinfo/openib-general
> > > 
> 




More information about the general mailing list