Re: [PATCH] opensm: Add initial support for optimized SLtoVLMappingTable programming

Re: [PATCH] opensm: Add initial support for optimized SLtoVLMappingTable programming

[Sasha Khapyorsky]

On 09:18 Tue 04 Aug     , Hal Rosenstock wrote:
> 

Implementation description would be very useful.

What does "initial support" mean?

> Signed-off-by: Hal Rosenstock <hal.rosenstock-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> ---
> diff --git a/opensm/include/opensm/osm_subnet.h b/opensm/include/opensm/osm_subnet.h
> index 6c20de8..8443763 100644
> --- a/opensm/include/opensm/osm_subnet.h
> +++ b/opensm/include/opensm/osm_subnet.h
> @@ -4,6 +4,7 @@
>   * Copyright (c) 1996-2003 Intel Corporation. All rights reserved.
>   * Copyright (c) 2008 Xsigo Systems Inc.  All rights reserved.
>   * Copyright (c) 2009 System Fabric Works, Inc. All rights reserved.
> + * Copyright (c) 2009 HNR Consulting. All rights reserved.
>   *
>   * This software is available to you under a choice of one of two
>   * licenses.  You may choose to be licensed under the terms of the GNU
> @@ -204,6 +205,7 @@ typedef struct osm_subn_opt {
>  	boolean_t daemon;
>  	boolean_t sm_inactive;
>  	boolean_t babbling_port_policy;
> +	boolean_t use_optimized_slvl;
>  	osm_qos_options_t qos_options;
>  	osm_qos_options_t qos_ca_options;
>  	osm_qos_options_t qos_sw0_options;
> @@ -428,6 +430,10 @@ typedef struct osm_subn_opt {
>  *	babbling_port_policy
>  *		OpenSM will enforce its "babbling" port policy.
>  *
> +*	use_optimized_slvl
> +*		Use optimized SLtoVLMappingTable programming if
> +*		device indicates it supports this.
> +*
>  *	perfmgr
>  *		Enable or disable the performance manager
>  *
> diff --git a/opensm/opensm/osm_qos.c b/opensm/opensm/osm_qos.c
> index e3dfb58..592e082 100644
> --- a/opensm/opensm/osm_qos.c
> +++ b/opensm/opensm/osm_qos.c
> @@ -1,5 +1,6 @@
>  /*
>   * Copyright (c) 2006-2008 Voltaire, Inc. All rights reserved.
> + * Copyright (c) 2009 HNR Consulting. All rights reserved.
>   *
>   * This software is available to you under a choice of one of two
>   * licenses.  You may choose to be licensed under the terms of the GNU
> @@ -150,7 +151,7 @@ static ib_api_status_t vlarb_update(osm_sm_t * sm, osm_physp_t * p,
>  
>  static ib_api_status_t sl2vl_update_table(osm_sm_t * sm, osm_physp_t * p,
>  					  uint8_t in_port, uint8_t out_port,
> -					  unsigned force_update,
> +					  unsigned optimize, unsigned force_update,
>  					  const ib_slvl_table_t * sl2vl_table)
>  {
>  	osm_madw_context_t context;
> @@ -177,10 +178,18 @@ static ib_api_status_t sl2vl_update_table(osm_sm_t * sm, osm_physp_t * p,
>  	    !memcmp(p_tbl, &tbl, sizeof(tbl)))
>  		return IB_SUCCESS;
>  
> +	/* both input port and output port wildcarded */
> +	if (optimize && (in_port != 1 || out_port != 1))
> +		return IB_SUCCESS;
> +

This function (sl2vl_update_table()) is called for each in and out ports
combination. I think that instead of calling it N * N times and
filtering out the case 'in == out == 1' you just need it once somewhere
at higher layer.

>  	context.slvl_context.node_guid = osm_node_get_node_guid(p_node);
>  	context.slvl_context.port_guid = osm_physp_get_port_guid(p);
>  	context.slvl_context.set_method = TRUE;
> -	attr_mod = in_port << 8 | out_port;
> +	if (optimize)
> +		/* both input port and output port wildcarded */
> +		attr_mod = 0x30000;
> +	else
> +		attr_mod = in_port << 8 | out_port;
>  	return osm_req_set(sm, osm_physp_get_dr_path_ptr(p),
>  			   (uint8_t *) & tbl, sizeof(tbl),
>  			   IB_MAD_ATTR_SLVL_TABLE, cl_hton32(attr_mod),
> @@ -189,14 +198,17 @@ static ib_api_status_t sl2vl_update_table(osm_sm_t * sm, osm_physp_t * p,
>  
>  static ib_api_status_t sl2vl_update(osm_sm_t * sm, osm_port_t * p_port,
>  				    osm_physp_t * p, uint8_t port_num,
> -				    unsigned force_update,
> +				    unsigned optimize, unsigned force_update,
>  				    const struct qos_config *qcfg)
>  {
>  	ib_api_status_t status;
>  	uint8_t i, num_ports;
>  	osm_physp_t *p_physp;
> +	osm_node_t *p_node;
> +	unsigned optimizesl2vl = 0;
>  
> -	if (osm_node_get_type(osm_physp_get_node_ptr(p)) == IB_NODE_TYPE_SWITCH) {
> +	p_node = osm_physp_get_node_ptr(p);
> +	if (osm_node_get_type(p_node) == IB_NODE_TYPE_SWITCH) {
>  		if (ib_port_info_get_vl_cap(&p->port_info) == 1) {
>  			/* Check port 0's capability mask */
>  			p_physp = p_port->p_physp;
> @@ -205,7 +217,8 @@ static ib_api_status_t sl2vl_update(osm_sm_t * sm, osm_port_t * p_port,
>  			     capability_mask & IB_PORT_CAP_HAS_SL_MAP))
>  				return IB_SUCCESS;
>  		}
> -		num_ports = osm_node_get_num_physp(osm_physp_get_node_ptr(p));
> +		num_ports = osm_node_get_num_physp(p_node);
> +		optimizesl2vl = ib_switch_info_get_opt_sl2vlmapping(&p_node->sw->switch_info) & optimize;

It should be once per switch, not for each switch port.

>  	} else {
>  		if (!(p->port_info.capability_mask & IB_PORT_CAP_HAS_SL_MAP))
>  			return IB_SUCCESS;
> @@ -213,8 +226,8 @@ static ib_api_status_t sl2vl_update(osm_sm_t * sm, osm_port_t * p_port,
>  	}
>  
>  	for (i = 0; i < num_ports; i++) {
> -		status = sl2vl_update_table(sm, p, i, port_num, force_update,
> -					    &qcfg->sl2vl);
> +		status = sl2vl_update_table(sm, p, i, port_num, optimizesl2vl,
> +					    force_update, &qcfg->sl2vl);
>  		if (status != IB_SUCCESS)
>  			return status;
>  	}
> @@ -224,7 +237,8 @@ static ib_api_status_t sl2vl_update(osm_sm_t * sm, osm_port_t * p_port,
>  
>  static int qos_physp_setup(osm_log_t * p_log, osm_sm_t * sm,
>  			   osm_port_t * p_port, osm_physp_t * p,
> -			   uint8_t port_num, unsigned force_update,
> +			   uint8_t port_num, unsigned optimize,
> +			   unsigned force_update,
>  			   const struct qos_config *qcfg)
>  {
>  	ib_api_status_t status;
> @@ -245,7 +259,8 @@ static int qos_physp_setup(osm_log_t * p_log, osm_sm_t * sm,
>  	}
>  
>  	/* setup SL2VL tables */
> -	status = sl2vl_update(sm, p_port, p, port_num, force_update, qcfg);
> +	status = sl2vl_update(sm, p_port, p, port_num, optimize, force_update,
> +			      qcfg);
>  	if (status != IB_SUCCESS) {
>  		OSM_LOG(p_log, OSM_LOG_ERROR, "ERR 6203 : "
>  			"failed to update SL2VLMapping tables "
> @@ -307,6 +322,7 @@ int osm_qos_setup(osm_opensm_t * p_osm)
>  				    p_osm->subn.need_update;
>  				if (qos_physp_setup(&p_osm->log, &p_osm->sm,
>  						    p_port, p_physp, i,
> +						    p_osm->subn.opt.use_optimized_slvl,
>  						    force_update, &swe_config))
>  					ret = -1;
>  			}
> @@ -327,7 +343,7 @@ int osm_qos_setup(osm_opensm_t * p_osm)
>  
>  		force_update = p_physp->need_update || p_osm->subn.need_update;
>  		if (qos_physp_setup(&p_osm->log, &p_osm->sm, p_port, p_physp,
> -				    0, force_update, cfg))
> +				    0, 0, force_update, cfg))
>  			ret = -1;
>  	}
>  
> diff --git a/opensm/opensm/osm_slvl_map_rcv.c b/opensm/opensm/osm_slvl_map_rcv.c
> index 9c37442..67c71bd 100644
> --- a/opensm/opensm/osm_slvl_map_rcv.c
> +++ b/opensm/opensm/osm_slvl_map_rcv.c
> @@ -2,6 +2,7 @@
>   * Copyright (c) 2004-2008 Voltaire, Inc. All rights reserved.
>   * Copyright (c) 2002-2005 Mellanox Technologies LTD. All rights reserved.
>   * Copyright (c) 1996-2003 Intel Corporation. All rights reserved.
> + * Copyright (c) 2009 HNR Consulting. All rights reserved.
>   *
>   * This software is available to you under a choice of one of two
>   * licenses.  You may choose to be licensed under the terms of the GNU
> @@ -72,7 +73,9 @@ void osm_slvl_rcv_process(IN void *context, IN void *p_data)
>  	osm_slvl_context_t *p_context;
>  	ib_net64_t port_guid;
>  	ib_net64_t node_guid;
> -	uint8_t out_port_num, in_port_num;
> +	uint32_t attr_mod;
> +	uint8_t out_port_num, in_port_num, startinport, startoutport,
> +		endinport, endoutport;
>  
>  	CL_ASSERT(sm);
>  
> @@ -111,6 +114,9 @@ void osm_slvl_rcv_process(IN void *context, IN void *p_data)
>  		    (uint8_t) cl_ntoh32(p_smp->attr_mod & 0xFF000000);
>  		in_port_num =
>  		    (uint8_t) cl_ntoh32((p_smp->attr_mod & 0x00FF0000) << 8);
> +		attr_mod = cl_ntoh32(p_smp->attr_mod);
> +		if (attr_mod & 0x30000)
> +			goto opt_sl2vl;
>  		p_physp = osm_node_get_physp_ptr(p_node, out_port_num);
>  	} else {
>  		p_physp = p_port->p_physp;
> @@ -123,7 +129,7 @@ void osm_slvl_rcv_process(IN void *context, IN void *p_data)
>  	   all we want is to update the subnet.
>  	 */
>  	OSM_LOG(sm->p_log, OSM_LOG_VERBOSE,
> -		"Got SLtoVL get response in_port_num %u out_port_num %u with "
> +		"Received SLtoVL GetResp in_port_num %u out_port_num %u with "
>  		"GUID 0x%" PRIx64 " for parent node GUID 0x%" PRIx64 ", TID 0x%"
>  		PRIx64 "\n", in_port_num, out_port_num, cl_ntoh64(port_guid),
>  		cl_ntoh64(node_guid), cl_ntoh64(p_smp->trans_id));
> @@ -142,6 +148,39 @@ void osm_slvl_rcv_process(IN void *context, IN void *p_data)
>  				out_port_num, p_slvl_tbl, OSM_LOG_DEBUG);
>  
>  	osm_physp_set_slvl_tbl(p_physp, p_slvl_tbl, in_port_num);
> +	goto Exit;
> +
> +opt_sl2vl:

I think that you need to use functions here.

> +	OSM_LOG(sm->p_log, OSM_LOG_VERBOSE,
> +		"Got optimized SLtoVL get response in_port_num %u out_port_num "
> +		"%u with GUID 0x%" PRIx64 " for parent node GUID 0x%" PRIx64
> +		", TID 0x%" PRIx64 "\n", in_port_num, out_port_num,
> +		cl_ntoh64(port_guid), cl_ntoh64(node_guid),
> +		cl_ntoh64(p_smp->trans_id));
> +
> +	osm_dump_slvl_map_table(sm->p_log, port_guid, in_port_num,
> +				out_port_num, p_slvl_tbl, OSM_LOG_DEBUG);
> +
> +	if (attr_mod & 0x10000) {
> +		startoutport = ib_switch_info_is_enhanced_port0(&p_node->sw->switch_info) ? 0 : 1;
> +		endoutport = osm_node_get_num_physp(p_node);
> +	} else
> +		endoutport = startoutport = out_port_num;
> +	if (attr_mod & 0x20000) {
> +		startinport = ib_switch_info_is_enhanced_port0(&p_node->sw->switch_info) ? 0 : 1;
> +		endinport = osm_node_get_num_physp(p_node);
> +	} else
> +		endinport = startinport = in_port_num;
> +
> +	for (out_port_num = startoutport; out_port_num < endoutport;
> +	     out_port_num++) {
> +		p_physp = osm_node_get_physp_ptr(p_node, out_port_num);
> +		if (!p_physp)

When could this be NULL?

Sasha

> +			continue;
> +		for (in_port_num = startinport; in_port_num < endinport;
> +		     in_port_num++)
> +			osm_physp_set_slvl_tbl(p_physp, p_slvl_tbl, in_port_num);
> +	}
>  
>  Exit:
>  	cl_plock_release(sm->p_lock);
> diff --git a/opensm/opensm/osm_subnet.c b/opensm/opensm/osm_subnet.c
> index 0d11811..540165a 100644
> --- a/opensm/opensm/osm_subnet.c
> +++ b/opensm/opensm/osm_subnet.c
> @@ -4,6 +4,7 @@
>   * Copyright (c) 1996-2003 Intel Corporation. All rights reserved.
>   * Copyright (c) 2008 Xsigo Systems Inc.  All rights reserved.
>   * Copyright (c) 2009 System Fabric Works, Inc. All rights reserved.
> + * Copyright (c) 2009 HNR Consulting. All rights reserved.
>   *
>   * This software is available to you under a choice of one of two
>   * licenses.  You may choose to be licensed under the terms of the GNU
> @@ -352,6 +353,7 @@ static const opt_rec_t opt_tbl[] = {
>  	{ "daemon", OPT_OFFSET(daemon), opts_parse_boolean, NULL, 0 },
>  	{ "sm_inactive", OPT_OFFSET(sm_inactive), opts_parse_boolean, NULL, 1 },
>  	{ "babbling_port_policy", OPT_OFFSET(babbling_port_policy), opts_parse_boolean, NULL, 1 },
> +	{"use_optimized_slvl", OPT_OFFSET(use_optimized_slvl), opts_parse_boolean, NULL, 1 },
>  #ifdef ENABLE_OSM_PERF_MGR
>  	{ "perfmgr", OPT_OFFSET(perfmgr), opts_parse_boolean, NULL, 0 },
>  	{ "perfmgr_redir", OPT_OFFSET(perfmgr_redir), opts_parse_boolean, NULL, 0 },
> @@ -715,6 +717,7 @@ void osm_subn_set_default_opt(IN osm_subn_opt_t * const p_opt)
>  	p_opt->daemon = FALSE;
>  	p_opt->sm_inactive = FALSE;
>  	p_opt->babbling_port_policy = FALSE;
> +	p_opt->use_optimized_slvl = FALSE;
>  #ifdef ENABLE_OSM_PERF_MGR
>  	p_opt->perfmgr = FALSE;
>  	p_opt->perfmgr_redir = TRUE;
> @@ -1501,10 +1504,13 @@ int osm_subn_output_conf(FILE *out, IN osm_subn_opt_t *const p_opts)
>  		"# SM Inactive\n"
>  		"sm_inactive %s\n\n"
>  		"# Babbling Port Policy\n"
> -		"babbling_port_policy %s\n\n",
> +		"babbling_port_policy %s\n\n"
> +		"# Use Optimized SLtoVLMapping programming if supported by device\n"
> +		"use_optimized_slvl %s\n\n",
>  		p_opts->daemon ? "TRUE" : "FALSE",
>  		p_opts->sm_inactive ? "TRUE" : "FALSE",
> -		p_opts->babbling_port_policy ? "TRUE" : "FALSE");
> +		p_opts->babbling_port_policy ? "TRUE" : "FALSE",
> +		p_opts->use_optimized_slvl ? "TRUE" : "FALSE");
>  
>  #ifdef ENABLE_OSM_PERF_MGR
>  	fprintf(out,
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] opensm: Add initial support for optimized SLtoVLMappingTable programming

[Hal Rosenstock]

On Thu, Oct 29, 2009 at 10:23 PM, Sasha Khapyorsky <sashak-smomgflXvOZWk0Htik3J/w@public.gmane.org> wrote:
> On 09:18 Tue 04 Aug     , Hal Rosenstock wrote:
>>
>
> Implementation description would be very useful.

> What does "initial support" mean?

It means there's more to come in terms of using
OptimizedSLtoVLMappingProgramming. This is the simplest
use/introduction of this optional feature.

>> Signed-off-by: Hal Rosenstock <hal.rosenstock-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
>> ---
>> diff --git a/opensm/include/opensm/osm_subnet.h b/opensm/include/opensm/osm_subnet.h
>> index 6c20de8..8443763 100644
>> --- a/opensm/include/opensm/osm_subnet.h
>> +++ b/opensm/include/opensm/osm_subnet.h
>> @@ -4,6 +4,7 @@
>>   * Copyright (c) 1996-2003 Intel Corporation. All rights reserved.
>>   * Copyright (c) 2008 Xsigo Systems Inc.  All rights reserved.
>>   * Copyright (c) 2009 System Fabric Works, Inc. All rights reserved.
>> + * Copyright (c) 2009 HNR Consulting. All rights reserved.
>>   *
>>   * This software is available to you under a choice of one of two
>>   * licenses.  You may choose to be licensed under the terms of the GNU
>> @@ -204,6 +205,7 @@ typedef struct osm_subn_opt {
>>       boolean_t daemon;
>>       boolean_t sm_inactive;
>>       boolean_t babbling_port_policy;
>> +     boolean_t use_optimized_slvl;
>>       osm_qos_options_t qos_options;
>>       osm_qos_options_t qos_ca_options;
>>       osm_qos_options_t qos_sw0_options;
>> @@ -428,6 +430,10 @@ typedef struct osm_subn_opt {
>>  *    babbling_port_policy
>>  *            OpenSM will enforce its "babbling" port policy.
>>  *
>> +*    use_optimized_slvl
>> +*            Use optimized SLtoVLMappingTable programming if
>> +*            device indicates it supports this.
>> +*
>>  *    perfmgr
>>  *            Enable or disable the performance manager
>>  *
>> diff --git a/opensm/opensm/osm_qos.c b/opensm/opensm/osm_qos.c
>> index e3dfb58..592e082 100644
>> --- a/opensm/opensm/osm_qos.c
>> +++ b/opensm/opensm/osm_qos.c
>> @@ -1,5 +1,6 @@
>>  /*
>>   * Copyright (c) 2006-2008 Voltaire, Inc. All rights reserved.
>> + * Copyright (c) 2009 HNR Consulting. All rights reserved.
>>   *
>>   * This software is available to you under a choice of one of two
>>   * licenses.  You may choose to be licensed under the terms of the GNU
>> @@ -150,7 +151,7 @@ static ib_api_status_t vlarb_update(osm_sm_t * sm, osm_physp_t * p,
>>
>>  static ib_api_status_t sl2vl_update_table(osm_sm_t * sm, osm_physp_t * p,
>>                                         uint8_t in_port, uint8_t out_port,
>> -                                       unsigned force_update,
>> +                                       unsigned optimize, unsigned force_update,
>>                                         const ib_slvl_table_t * sl2vl_table)
>>  {
>>       osm_madw_context_t context;
>> @@ -177,10 +178,18 @@ static ib_api_status_t sl2vl_update_table(osm_sm_t * sm, osm_physp_t * p,
>>           !memcmp(p_tbl, &tbl, sizeof(tbl)))
>>               return IB_SUCCESS;
>>
>> +     /* both input port and output port wildcarded */
>> +     if (optimize && (in_port != 1 || out_port != 1))
>> +             return IB_SUCCESS;
>> +
>
> This function (sl2vl_update_table()) is called for each in and out ports
> combination. I think that instead of calling it N * N times and
> filtering out the case 'in == out == 1' you just need it once somewhere
> at higher layer.

I was trying to minimize the changes to the current code flow. It can
be done the way you propose but that will cause larger changes. Are
you sure about this direction ?

>>       context.slvl_context.node_guid = osm_node_get_node_guid(p_node);
>>       context.slvl_context.port_guid = osm_physp_get_port_guid(p);
>>       context.slvl_context.set_method = TRUE;
>> -     attr_mod = in_port << 8 | out_port;
>> +     if (optimize)
>> +             /* both input port and output port wildcarded */
>> +             attr_mod = 0x30000;
>> +     else
>> +             attr_mod = in_port << 8 | out_port;
>>       return osm_req_set(sm, osm_physp_get_dr_path_ptr(p),
>>                          (uint8_t *) & tbl, sizeof(tbl),
>>                          IB_MAD_ATTR_SLVL_TABLE, cl_hton32(attr_mod),
>> @@ -189,14 +198,17 @@ static ib_api_status_t sl2vl_update_table(osm_sm_t * sm, osm_physp_t * p,
>>
>>  static ib_api_status_t sl2vl_update(osm_sm_t * sm, osm_port_t * p_port,
>>                                   osm_physp_t * p, uint8_t port_num,
>> -                                 unsigned force_update,
>> +                                 unsigned optimize, unsigned force_update,
>>                                   const struct qos_config *qcfg)
>>  {
>>       ib_api_status_t status;
>>       uint8_t i, num_ports;
>>       osm_physp_t *p_physp;
>> +     osm_node_t *p_node;
>> +     unsigned optimizesl2vl = 0;
>>
>> -     if (osm_node_get_type(osm_physp_get_node_ptr(p)) == IB_NODE_TYPE_SWITCH) {
>> +     p_node = osm_physp_get_node_ptr(p);
>> +     if (osm_node_get_type(p_node) == IB_NODE_TYPE_SWITCH) {
>>               if (ib_port_info_get_vl_cap(&p->port_info) == 1) {
>>                       /* Check port 0's capability mask */
>>                       p_physp = p_port->p_physp;
>> @@ -205,7 +217,8 @@ static ib_api_status_t sl2vl_update(osm_sm_t * sm, osm_port_t * p_port,
>>                            capability_mask & IB_PORT_CAP_HAS_SL_MAP))
>>                               return IB_SUCCESS;
>>               }
>> -             num_ports = osm_node_get_num_physp(osm_physp_get_node_ptr(p));
>> +             num_ports = osm_node_get_num_physp(p_node);
>> +             optimizesl2vl = ib_switch_info_get_opt_sl2vlmapping(&p_node->sw->switch_info) & optimize;
>
> It should be once per switch, not for each switch port.

Sure; if (still) needed here (might depend on the approach) is a
tradeoff of a simple local operation v. an additional passed in
parameter.

>>       } else {
>>               if (!(p->port_info.capability_mask & IB_PORT_CAP_HAS_SL_MAP))
>>                       return IB_SUCCESS;
>> @@ -213,8 +226,8 @@ static ib_api_status_t sl2vl_update(osm_sm_t * sm, osm_port_t * p_port,
>>       }
>>
>>       for (i = 0; i < num_ports; i++) {
>> -             status = sl2vl_update_table(sm, p, i, port_num, force_update,
>> -                                         &qcfg->sl2vl);
>> +             status = sl2vl_update_table(sm, p, i, port_num, optimizesl2vl,
>> +                                         force_update, &qcfg->sl2vl);
>>               if (status != IB_SUCCESS)
>>                       return status;
>>       }
>> @@ -224,7 +237,8 @@ static ib_api_status_t sl2vl_update(osm_sm_t * sm, osm_port_t * p_port,
>>
>>  static int qos_physp_setup(osm_log_t * p_log, osm_sm_t * sm,
>>                          osm_port_t * p_port, osm_physp_t * p,
>> -                        uint8_t port_num, unsigned force_update,
>> +                        uint8_t port_num, unsigned optimize,
>> +                        unsigned force_update,
>>                          const struct qos_config *qcfg)
>>  {
>>       ib_api_status_t status;
>> @@ -245,7 +259,8 @@ static int qos_physp_setup(osm_log_t * p_log, osm_sm_t * sm,
>>       }
>>
>>       /* setup SL2VL tables */
>> -     status = sl2vl_update(sm, p_port, p, port_num, force_update, qcfg);
>> +     status = sl2vl_update(sm, p_port, p, port_num, optimize, force_update,
>> +                           qcfg);
>>       if (status != IB_SUCCESS) {
>>               OSM_LOG(p_log, OSM_LOG_ERROR, "ERR 6203 : "
>>                       "failed to update SL2VLMapping tables "
>> @@ -307,6 +322,7 @@ int osm_qos_setup(osm_opensm_t * p_osm)
>>                                   p_osm->subn.need_update;
>>                               if (qos_physp_setup(&p_osm->log, &p_osm->sm,
>>                                                   p_port, p_physp, i,
>> +                                                 p_osm->subn.opt.use_optimized_slvl,
>>                                                   force_update, &swe_config))
>>                                       ret = -1;
>>                       }
>> @@ -327,7 +343,7 @@ int osm_qos_setup(osm_opensm_t * p_osm)
>>
>>               force_update = p_physp->need_update || p_osm->subn.need_update;
>>               if (qos_physp_setup(&p_osm->log, &p_osm->sm, p_port, p_physp,
>> -                                 0, force_update, cfg))
>> +                                 0, 0, force_update, cfg))
>>                       ret = -1;
>>       }
>>
>> diff --git a/opensm/opensm/osm_slvl_map_rcv.c b/opensm/opensm/osm_slvl_map_rcv.c
>> index 9c37442..67c71bd 100644
>> --- a/opensm/opensm/osm_slvl_map_rcv.c
>> +++ b/opensm/opensm/osm_slvl_map_rcv.c
>> @@ -2,6 +2,7 @@
>>   * Copyright (c) 2004-2008 Voltaire, Inc. All rights reserved.
>>   * Copyright (c) 2002-2005 Mellanox Technologies LTD. All rights reserved.
>>   * Copyright (c) 1996-2003 Intel Corporation. All rights reserved.
>> + * Copyright (c) 2009 HNR Consulting. All rights reserved.
>>   *
>>   * This software is available to you under a choice of one of two
>>   * licenses.  You may choose to be licensed under the terms of the GNU
>> @@ -72,7 +73,9 @@ void osm_slvl_rcv_process(IN void *context, IN void *p_data)
>>       osm_slvl_context_t *p_context;
>>       ib_net64_t port_guid;
>>       ib_net64_t node_guid;
>> -     uint8_t out_port_num, in_port_num;
>> +     uint32_t attr_mod;
>> +     uint8_t out_port_num, in_port_num, startinport, startoutport,
>> +             endinport, endoutport;
>>
>>       CL_ASSERT(sm);
>>
>> @@ -111,6 +114,9 @@ void osm_slvl_rcv_process(IN void *context, IN void *p_data)
>>                   (uint8_t) cl_ntoh32(p_smp->attr_mod & 0xFF000000);
>>               in_port_num =
>>                   (uint8_t) cl_ntoh32((p_smp->attr_mod & 0x00FF0000) << 8);
>> +             attr_mod = cl_ntoh32(p_smp->attr_mod);
>> +             if (attr_mod & 0x30000)
>> +                     goto opt_sl2vl;
>>               p_physp = osm_node_get_physp_ptr(p_node, out_port_num);
>>       } else {
>>               p_physp = p_port->p_physp;
>> @@ -123,7 +129,7 @@ void osm_slvl_rcv_process(IN void *context, IN void *p_data)
>>          all we want is to update the subnet.
>>        */
>>       OSM_LOG(sm->p_log, OSM_LOG_VERBOSE,
>> -             "Got SLtoVL get response in_port_num %u out_port_num %u with "
>> +             "Received SLtoVL GetResp in_port_num %u out_port_num %u with "
>>               "GUID 0x%" PRIx64 " for parent node GUID 0x%" PRIx64 ", TID 0x%"
>>               PRIx64 "\n", in_port_num, out_port_num, cl_ntoh64(port_guid),
>>               cl_ntoh64(node_guid), cl_ntoh64(p_smp->trans_id));
>> @@ -142,6 +148,39 @@ void osm_slvl_rcv_process(IN void *context, IN void *p_data)
>>                               out_port_num, p_slvl_tbl, OSM_LOG_DEBUG);
>>
>>       osm_physp_set_slvl_tbl(p_physp, p_slvl_tbl, in_port_num);
>> +     goto Exit;
>> +
>> +opt_sl2vl:
>
> I think that you need to use functions here.

Where is here ? To what are you referring ?

>> +     OSM_LOG(sm->p_log, OSM_LOG_VERBOSE,
>> +             "Got optimized SLtoVL get response in_port_num %u out_port_num "
>> +             "%u with GUID 0x%" PRIx64 " for parent node GUID 0x%" PRIx64
>> +             ", TID 0x%" PRIx64 "\n", in_port_num, out_port_num,
>> +             cl_ntoh64(port_guid), cl_ntoh64(node_guid),
>> +             cl_ntoh64(p_smp->trans_id));
>> +
>> +     osm_dump_slvl_map_table(sm->p_log, port_guid, in_port_num,
>> +                             out_port_num, p_slvl_tbl, OSM_LOG_DEBUG);
>> +
>> +     if (attr_mod & 0x10000) {
>> +             startoutport = ib_switch_info_is_enhanced_port0(&p_node->sw->switch_info) ? 0 : 1;
>> +             endoutport = osm_node_get_num_physp(p_node);
>> +     } else
>> +             endoutport = startoutport = out_port_num;
>> +     if (attr_mod & 0x20000) {
>> +             startinport = ib_switch_info_is_enhanced_port0(&p_node->sw->switch_info) ? 0 : 1;
>> +             endinport = osm_node_get_num_physp(p_node);
>> +     } else
>> +             endinport = startinport = in_port_num;
>> +
>> +     for (out_port_num = startoutport; out_port_num < endoutport;
>> +          out_port_num++) {
>> +             p_physp = osm_node_get_physp_ptr(p_node, out_port_num);
>> +             if (!p_physp)
>
> When could this be NULL?

It was just defensive. Should it be removed ?

-- Hal

> Sasha
>
>> +                     continue;
>> +             for (in_port_num = startinport; in_port_num < endinport;
>> +                  in_port_num++)
>> +                     osm_physp_set_slvl_tbl(p_physp, p_slvl_tbl, in_port_num);
>> +     }
>>
>>  Exit:
>>       cl_plock_release(sm->p_lock);
>> diff --git a/opensm/opensm/osm_subnet.c b/opensm/opensm/osm_subnet.c
>> index 0d11811..540165a 100644
>> --- a/opensm/opensm/osm_subnet.c
>> +++ b/opensm/opensm/osm_subnet.c
>> @@ -4,6 +4,7 @@
>>   * Copyright (c) 1996-2003 Intel Corporation. All rights reserved.
>>   * Copyright (c) 2008 Xsigo Systems Inc.  All rights reserved.
>>   * Copyright (c) 2009 System Fabric Works, Inc. All rights reserved.
>> + * Copyright (c) 2009 HNR Consulting. All rights reserved.
>>   *
>>   * This software is available to you under a choice of one of two
>>   * licenses.  You may choose to be licensed under the terms of the GNU
>> @@ -352,6 +353,7 @@ static const opt_rec_t opt_tbl[] = {
>>       { "daemon", OPT_OFFSET(daemon), opts_parse_boolean, NULL, 0 },
>>       { "sm_inactive", OPT_OFFSET(sm_inactive), opts_parse_boolean, NULL, 1 },
>>       { "babbling_port_policy", OPT_OFFSET(babbling_port_policy), opts_parse_boolean, NULL, 1 },
>> +     {"use_optimized_slvl", OPT_OFFSET(use_optimized_slvl), opts_parse_boolean, NULL, 1 },
>>  #ifdef ENABLE_OSM_PERF_MGR
>>       { "perfmgr", OPT_OFFSET(perfmgr), opts_parse_boolean, NULL, 0 },
>>       { "perfmgr_redir", OPT_OFFSET(perfmgr_redir), opts_parse_boolean, NULL, 0 },
>> @@ -715,6 +717,7 @@ void osm_subn_set_default_opt(IN osm_subn_opt_t * const p_opt)
>>       p_opt->daemon = FALSE;
>>       p_opt->sm_inactive = FALSE;
>>       p_opt->babbling_port_policy = FALSE;
>> +     p_opt->use_optimized_slvl = FALSE;
>>  #ifdef ENABLE_OSM_PERF_MGR
>>       p_opt->perfmgr = FALSE;
>>       p_opt->perfmgr_redir = TRUE;
>> @@ -1501,10 +1504,13 @@ int osm_subn_output_conf(FILE *out, IN osm_subn_opt_t *const p_opts)
>>               "# SM Inactive\n"
>>               "sm_inactive %s\n\n"
>>               "# Babbling Port Policy\n"
>> -             "babbling_port_policy %s\n\n",
>> +             "babbling_port_policy %s\n\n"
>> +             "# Use Optimized SLtoVLMapping programming if supported by device\n"
>> +             "use_optimized_slvl %s\n\n",
>>               p_opts->daemon ? "TRUE" : "FALSE",
>>               p_opts->sm_inactive ? "TRUE" : "FALSE",
>> -             p_opts->babbling_port_policy ? "TRUE" : "FALSE");
>> +             p_opts->babbling_port_policy ? "TRUE" : "FALSE",
>> +             p_opts->use_optimized_slvl ? "TRUE" : "FALSE");
>>
>>  #ifdef ENABLE_OSM_PERF_MGR
>>       fprintf(out,
>>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
> the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] opensm: Add initial support for optimized SLtoVLMappingTable programming

[Or Gerlitz]

Hal Rosenstock wrote:
> On Thu, Oct 29, 2009 at 10:23 PM, Sasha Khapyorsky <sashak-smomgflXvOZWk0Htik3J/w@public.gmane.org> wrote:
>> Implementation description would be very useful. What does "initial support" mean?
> It means there's more to come in terms of using OptimizedSLtoVLMappingProgramming. This is the simplest use/introduction of this optional feature.
You can just send people to reads specs, your change log should explain 
what the patch is about, if this is a big change to opensm, maybe even 
RFC it will a detailed writeup

Or.
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] opensm: Add initial support for optimized SLtoVLMappingTable programming

[Sasha Khapyorsky]

On 11:40 Fri 30 Oct     , Hal Rosenstock wrote:
> >> @@ -150,7 +151,7 @@ static ib_api_status_t vlarb_update(osm_sm_t * sm, osm_physp_t * p,
> >>
> >>  static ib_api_status_t sl2vl_update_table(osm_sm_t * sm, osm_physp_t * p,
> >>                                         uint8_t in_port, uint8_t out_port,
> >> -                                       unsigned force_update,
> >> +                                       unsigned optimize, unsigned force_update,
> >>                                         const ib_slvl_table_t * sl2vl_table)
> >>  {
> >>       osm_madw_context_t context;
> >> @@ -177,10 +178,18 @@ static ib_api_status_t sl2vl_update_table(osm_sm_t * sm, osm_physp_t * p,
> >>           !memcmp(p_tbl, &tbl, sizeof(tbl)))
> >>               return IB_SUCCESS;
> >>
> >> +     /* both input port and output port wildcarded */
> >> +     if (optimize && (in_port != 1 || out_port != 1))
> >> +             return IB_SUCCESS;
> >> +
> >
> > This function (sl2vl_update_table()) is called for each in and out ports
> > combination. I think that instead of calling it N * N times and
> > filtering out the case 'in == out == 1' you just need it once somewhere
> > at higher layer.
> 
> I was trying to minimize the changes to the current code flow. It can
> be done the way you propose but that will cause larger changes. Are
> you sure about this direction ?

I'm sure that that we don't need a bunch of empty loops. Also I think
that doing this on an appropriate layer will not complicate things more.

> >> @@ -72,7 +73,9 @@ void osm_slvl_rcv_process(IN void *context, IN void *p_data)
> >>       osm_slvl_context_t *p_context;
> >>       ib_net64_t port_guid;
> >>       ib_net64_t node_guid;
> >> -     uint8_t out_port_num, in_port_num;
> >> +     uint32_t attr_mod;
> >> +     uint8_t out_port_num, in_port_num, startinport, startoutport,
> >> +             endinport, endoutport;
> >>
> >>       CL_ASSERT(sm);
> >>
> >> @@ -111,6 +114,9 @@ void osm_slvl_rcv_process(IN void *context, IN void *p_data)
> >>                   (uint8_t) cl_ntoh32(p_smp->attr_mod & 0xFF000000);
> >>               in_port_num =
> >>                   (uint8_t) cl_ntoh32((p_smp->attr_mod & 0x00FF0000) << 8);
> >> +             attr_mod = cl_ntoh32(p_smp->attr_mod);
> >> +             if (attr_mod & 0x30000)
> >> +                     goto opt_sl2vl;
> >>               p_physp = osm_node_get_physp_ptr(p_node, out_port_num);
> >>       } else {
> >>               p_physp = p_port->p_physp;
> >> @@ -123,7 +129,7 @@ void osm_slvl_rcv_process(IN void *context, IN void *p_data)
> >>          all we want is to update the subnet.
> >>        */
> >>       OSM_LOG(sm->p_log, OSM_LOG_VERBOSE,
> >> -             "Got SLtoVL get response in_port_num %u out_port_num %u with "
> >> +             "Received SLtoVL GetResp in_port_num %u out_port_num %u with "
> >>               "GUID 0x%" PRIx64 " for parent node GUID 0x%" PRIx64 ", TID 0x%"
> >>               PRIx64 "\n", in_port_num, out_port_num, cl_ntoh64(port_guid),
> >>               cl_ntoh64(node_guid), cl_ntoh64(p_smp->trans_id));
> >> @@ -142,6 +148,39 @@ void osm_slvl_rcv_process(IN void *context, IN void *p_data)
> >>                               out_port_num, p_slvl_tbl, OSM_LOG_DEBUG);
> >>
> >>       osm_physp_set_slvl_tbl(p_physp, p_slvl_tbl, in_port_num);
> >> +     goto Exit;
> >> +
> >> +opt_sl2vl:
> >
> > I think that you need to use functions here.
> 
> Where is here ? To what are you referring ?

When your function becomes to form of:

	if (cond1)
		goto do1;
	
	do_else();

	goto exit;

do1:
	do_1();

exit:
	return;

I'm suggesting to improve flows, for example by using functions (or
another way if applicable and achieves the simplification goal).

> 
> >> +     OSM_LOG(sm->p_log, OSM_LOG_VERBOSE,
> >> +             "Got optimized SLtoVL get response in_port_num %u out_port_num "
> >> +             "%u with GUID 0x%" PRIx64 " for parent node GUID 0x%" PRIx64
> >> +             ", TID 0x%" PRIx64 "\n", in_port_num, out_port_num,
> >> +             cl_ntoh64(port_guid), cl_ntoh64(node_guid),
> >> +             cl_ntoh64(p_smp->trans_id));
> >> +
> >> +     osm_dump_slvl_map_table(sm->p_log, port_guid, in_port_num,
> >> +                             out_port_num, p_slvl_tbl, OSM_LOG_DEBUG);
> >> +
> >> +     if (attr_mod & 0x10000) {
> >> +             startoutport = ib_switch_info_is_enhanced_port0(&p_node->sw->switch_info) ? 0 : 1;
> >> +             endoutport = osm_node_get_num_physp(p_node);
> >> +     } else
> >> +             endoutport = startoutport = out_port_num;
> >> +     if (attr_mod & 0x20000) {
> >> +             startinport = ib_switch_info_is_enhanced_port0(&p_node->sw->switch_info) ? 0 : 1;
> >> +             endinport = osm_node_get_num_physp(p_node);
> >> +     } else
> >> +             endinport = startinport = in_port_num;
> >> +
> >> +     for (out_port_num = startoutport; out_port_num < endoutport;
> >> +          out_port_num++) {
> >> +             p_physp = osm_node_get_physp_ptr(p_node, out_port_num);
> >> +             if (!p_physp)
> >
> > When could this be NULL?
> 
> It was just defensive. Should it be removed ?

I wasn't sure, so asked. If it is "just defensive" please remove it.

Sasha
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] opensm: Add initial support for optimized SLtoVLMappingTable programming

[Hal Rosenstock]

On Thu, Oct 29, 2009 at 9:23 PM, Sasha Khapyorsky <sashak-smomgflXvOZWk0Htik3J/w@public.gmane.org> wrote:
<snip...>

>> diff --git a/opensm/opensm/osm_slvl_map_rcv.c b/opensm/opensm/osm_slvl_map_rcv.c
>> index 9c37442..67c71bd 100644
>> --- a/opensm/opensm/osm_slvl_map_rcv.c
>> +++ b/opensm/opensm/osm_slvl_map_rcv.c
>> @@ -2,6 +2,7 @@
>>   * Copyright (c) 2004-2008 Voltaire, Inc. All rights reserved.
>>   * Copyright (c) 2002-2005 Mellanox Technologies LTD. All rights reserved.
>>   * Copyright (c) 1996-2003 Intel Corporation. All rights reserved.
>> + * Copyright (c) 2009 HNR Consulting. All rights reserved.
>>   *
>>   * This software is available to you under a choice of one of two
>>   * licenses.  You may choose to be licensed under the terms of the GNU
>> @@ -72,7 +73,9 @@ void osm_slvl_rcv_process(IN void *context, IN void *p_data)
>>       osm_slvl_context_t *p_context;
>>       ib_net64_t port_guid;
>>       ib_net64_t node_guid;
>> -     uint8_t out_port_num, in_port_num;
>> +     uint32_t attr_mod;
>> +     uint8_t out_port_num, in_port_num, startinport, startoutport,
>> +             endinport, endoutport;
>>
>>       CL_ASSERT(sm);
>>
>> @@ -111,6 +114,9 @@ void osm_slvl_rcv_process(IN void *context, IN void *p_data)
>>                   (uint8_t) cl_ntoh32(p_smp->attr_mod & 0xFF000000);
>>               in_port_num =
>>                   (uint8_t) cl_ntoh32((p_smp->attr_mod & 0x00FF0000) << 8);
>> +             attr_mod = cl_ntoh32(p_smp->attr_mod);
>> +             if (attr_mod & 0x30000)
>> +                     goto opt_sl2vl;
>>               p_physp = osm_node_get_physp_ptr(p_node, out_port_num);
>>       } else {
>>               p_physp = p_port->p_physp;
>> @@ -123,7 +129,7 @@ void osm_slvl_rcv_process(IN void *context, IN void *p_data)
>>          all we want is to update the subnet.
>>        */
>>       OSM_LOG(sm->p_log, OSM_LOG_VERBOSE,
>> -             "Got SLtoVL get response in_port_num %u out_port_num %u with "
>> +             "Received SLtoVL GetResp in_port_num %u out_port_num %u with "
>>               "GUID 0x%" PRIx64 " for parent node GUID 0x%" PRIx64 ", TID 0x%"
>>               PRIx64 "\n", in_port_num, out_port_num, cl_ntoh64(port_guid),
>>               cl_ntoh64(node_guid), cl_ntoh64(p_smp->trans_id));
>> @@ -142,6 +148,39 @@ void osm_slvl_rcv_process(IN void *context, IN void *p_data)
>>                               out_port_num, p_slvl_tbl, OSM_LOG_DEBUG);
>>
>>       osm_physp_set_slvl_tbl(p_physp, p_slvl_tbl, in_port_num);
>> +     goto Exit;
>> +
>> +opt_sl2vl:
>
> I think that you need to use functions here.
>
>> +     OSM_LOG(sm->p_log, OSM_LOG_VERBOSE,
>> +             "Got optimized SLtoVL get response in_port_num %u out_port_num "
>> +             "%u with GUID 0x%" PRIx64 " for parent node GUID 0x%" PRIx64
>> +             ", TID 0x%" PRIx64 "\n", in_port_num, out_port_num,
>> +             cl_ntoh64(port_guid), cl_ntoh64(node_guid),
>> +             cl_ntoh64(p_smp->trans_id));
>> +

Is this a macro/va args limit ?

Does the existing log message this was modeled after need to change
too ? It is virtually the same (maybe one less arg).

-- Hal

<snip...>
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html