[ofa-general] Re: [PATCH] opensm: allow multiple scopes in a partition

[Sasha Khapyorsky]

Hi Rolf,

On 17:00 Wed 28 Nov     , Rolf Manderscheid wrote:
> Hi Sasha,
> 
> This patch allows multiple scopes to be configured for a partition.
> This allows ipoib interfaces with different scopes to coexist in a
> partition.  The partition configuration file can now have multiple
> scope=N flags and they all take effect (instead of just the last one).
> 
> Signed-off-by: Rolf Manderscheid <rvm at obsidianresearch.com>

The idea looks good for me. Some comments about the patch are below.

> 
> --
> 
> diff --git a/opensm/man/opensm.8 b/opensm/man/opensm.8
> index efd6ff0..c51f386 100644
> --- a/opensm/man/opensm.8
> +++ b/opensm/man/opensm.8
> @@ -366,7 +366,8 @@ Currently recognized flags are:
>   sl=<val>    - specifies SL for this IPoIB MC group
>                 (default is 0)
>   scope=<val> - specifies scope for this IPoIB MC group
> -               (default is 2 (link local))
> +               (default is 2 (link local)).  Multiple scope settings
> +               are permitted for a partition.
>  
>  Note that values for rate, mtu, and scope should be specified as
>  defined in the IBTA specification (for example, mtu=4 for 2048).
> diff --git a/opensm/opensm/osm_prtn_config.c b/opensm/opensm/osm_prtn_config.c
> index 1253031..646bf2a 100644
> --- a/opensm/opensm/osm_prtn_config.c
> +++ b/opensm/opensm/osm_prtn_config.c
> @@ -68,7 +68,7 @@ struct part_conf {
>  	osm_log_t *p_log;
>  	osm_subn_t *p_subn;
>  	osm_prtn_t *p_prtn;
> -	unsigned is_ipoib, mtu, rate, sl, scope;
> +	unsigned is_ipoib, mtu, rate, sl, scope_mask;
>  	boolean_t full;
>  };
>  
> @@ -89,6 +89,7 @@ static int partition_create(unsigned lineno, struct part_conf *conf,
>  			    char *name, char *id, char *flag, char *flag_val)
>  {
>  	uint16_t pkey;
> +	unsigned int scope;
>  
>  	if (!id && name && isdigit(*name)) {
>  		id = name;
> @@ -119,12 +120,26 @@ static int partition_create(unsigned lineno, struct part_conf *conf,
>  	}
>  	conf->p_prtn->sl = (uint8_t) conf->sl;
>  
> -	if (conf->is_ipoib)
> +	if (! conf->is_ipoib)

No need a blank after '!'.

> +		return 0;
> +
> +	if (! conf->scope_mask) {

Ditto.

>  		osm_prtn_add_mcgroup(conf->p_log, conf->p_subn, conf->p_prtn,
>  				     (uint8_t) conf->rate,
>  				     (uint8_t) conf->mtu,
> -				     (uint8_t) conf->scope);
> +				     0);
> +		return 0;
> +	}
> +
> +	for (scope = 0; scope < 16; scope++) {
> +		if (((1<<scope) & conf->scope_mask) == 0)
> +			continue;
>  
> +		osm_prtn_add_mcgroup(conf->p_log, conf->p_subn, conf->p_prtn,
> +				     (uint8_t) conf->rate,
> +				     (uint8_t) conf->mtu,
> +				     (uint8_t) scope);
> +	}
>  	return 0;
>  }
>  
> @@ -147,11 +162,13 @@ static int partition_add_flag(unsigned lineno, struct part_conf *conf,
>  				"flag \'rate\' requires valid value"
>  				" - skipped\n", lineno);
>  	} else if (!strncmp(flag, "scope", len)) {
> -		if (!val || (conf->scope = strtoul(val, NULL, 0)) == 0)
> +		unsigned int scope;
> +		if (!val || (scope = strtoul(val, NULL, 0)) == 0 || scope > 0xF)
>  			osm_log(conf->p_log, OSM_LOG_VERBOSE,
>  				"PARSE WARN: line %d: "
>  				"flag \'scope\' requires valid value"
>  				" - skipped\n", lineno);
> +		conf->scope_mask |= (1<<scope);

In case when val is NULL scope will be non-initialized and
conf->scope_mask will get a wrong value. And in case of other errors
too...

Sasha

>  	} else if (!strncmp(flag, "sl", len)) {
>  		unsigned sl;
>  		char *end;