[PATCH] opensm: Fix sl2vl configuration

[PATCH] opensm: Fix sl2vl configuration

[Eli Dorfman (Voltaire)]

Subject: [PATCH] Fix sl2vl configuration

For non-optimized sl2vl configuration in and out ports were reversed.
For optimal sl2vl added override of default ALL settting with port's
sl2vl when operational VL was other than the default port.

Signed-off-by: Eli Dorfman <elid-smomgflXvOZWk0Htik3J/w@public.gmane.org>
---
 opensm/opensm/osm_qos.c |   25 ++++++++++++++++++++-----
 1 files changed, 20 insertions(+), 5 deletions(-)

diff --git a/opensm/opensm/osm_qos.c b/opensm/opensm/osm_qos.c
index a571370..de0ae23 100644
--- a/opensm/opensm/osm_qos.c
+++ b/opensm/opensm/osm_qos.c
@@ -182,7 +182,7 @@ static ib_api_status_t sl2vl_update_table(osm_sm_t * sm, osm_physp_t * p,
 		tbl.raw_vl_by_sl[i] = (vl1 << 4) | vl2;
 	}
 
-	if (!force_update && (p_tbl = osm_physp_get_slvl_tbl(p, in_port)) &&
+	if (!force_update && in_port && (p_tbl = osm_physp_get_slvl_tbl(p, in_port)) &&
 	    !memcmp(p_tbl, &tbl, sizeof(tbl)))
 		return IB_SUCCESS;
 
@@ -209,6 +209,7 @@ static int qos_extports_setup(osm_sm_t * sm, osm_node_t *node,
 	unsigned num_ports = osm_node_get_num_physp(node);
 	int ret = 0;
 	unsigned i, j;
+	uint8_t op_vl1;
 
 	for (i = 1; i < num_ports; i++) {
 		p = osm_node_get_physp_ptr(node, i);
@@ -225,17 +226,31 @@ static int qos_extports_setup(osm_sm_t * sm, osm_node_t *node,
 	if (ib_switch_info_get_opt_sl2vlmapping(&node->sw->switch_info) &&
 	    sm->p_subn->opt.use_optimized_slvl) {
 		p = osm_node_get_physp_ptr(node, 1);
+		op_vl1 = ib_port_info_get_op_vls(&p->port_info);
 		force_update = p->need_update || sm->p_subn->need_update;
-		return sl2vl_update_table(sm, p, 1, 0x30000, force_update,
-					  &qcfg->sl2vl);
+		if (sl2vl_update_table(sm, p, 0, 0x30000, force_update,
+					&qcfg->sl2vl))
+			ret = -1;
+		/* overwrite default ALL configuration if port's
+		   op_vl is different */
+		for (i = 2; i < num_ports; i++) {
+			p = osm_node_get_physp_ptr(node, i);
+			if (ib_port_info_get_op_vls(&p->port_info) != op_vl1 && 
+			    sl2vl_update_table(sm, p, 0, 0x20000 | i, force_update,
+						&qcfg->sl2vl))
+				ret = -1;
+		}
+		return ret;
 	}
 
-	for (i = 0; i < num_ports; i++) {
+	/* non optimized sl2vl configuration */
+	i = ib_switch_info_is_enhanced_port0(&node->sw->switch_info) ? 0 : 1;
+	for (; i < num_ports; i++) {
 		p = osm_node_get_physp_ptr(node, i);
 		force_update = p->need_update || sm->p_subn->need_update;
 		j = ib_switch_info_is_enhanced_port0(&node->sw->switch_info) ? 0 : 1;
 		for (; j < num_ports; j++)
-			if (sl2vl_update_table(sm, p, i, i << 8 | j,
+			if (sl2vl_update_table(sm, p, j, j << 8 | i,
 					       force_update, &qcfg->sl2vl))
 				ret = -1;
 	}
-- 
1.5.5

--
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: Fix sl2vl configuration

[Jim Schutt]

Hi Eli,

On Wed, 2010-07-28 at 10:26 -0600, Eli Dorfman (Voltaire) wrote:
> Subject: [PATCH] Fix sl2vl configuration
> 
> For non-optimized sl2vl configuration in and out ports were reversed.

Nice catch.  I think this is also the correct fix for the problem
I was trying to fix with commit e1c253e893.

> For optimal sl2vl added override of default ALL settting with port's
> sl2vl when operational VL was other than the default port.
> 
> Signed-off-by: Eli Dorfman <elid-smomgflXvOZWk0Htik3J/w@public.gmane.org>
> ---
>  opensm/opensm/osm_qos.c |   25 ++++++++++++++++++++-----
>  1 files changed, 20 insertions(+), 5 deletions(-)
> 
> diff --git a/opensm/opensm/osm_qos.c b/opensm/opensm/osm_qos.c
> index a571370..de0ae23 100644
> --- a/opensm/opensm/osm_qos.c
> +++ b/opensm/opensm/osm_qos.c
> @@ -182,7 +182,7 @@ static ib_api_status_t sl2vl_update_table(osm_sm_t * sm, osm_physp_t * p,
>  		tbl.raw_vl_by_sl[i] = (vl1 << 4) | vl2;
>  	}
>  
> -	if (!force_update && (p_tbl = osm_physp_get_slvl_tbl(p, in_port)) &&
> +	if (!force_update && in_port && (p_tbl = osm_physp_get_slvl_tbl(p, in_port)) &&
>  	    !memcmp(p_tbl, &tbl, sizeof(tbl)))
>  		return IB_SUCCESS;


I'm confused.  Why do we want to always send the sl2vl update
if in_port is zero?


>  
> @@ -209,6 +209,7 @@ static int qos_extports_setup(osm_sm_t * sm, osm_node_t *node,
>  	unsigned num_ports = osm_node_get_num_physp(node);
>  	int ret = 0;
>  	unsigned i, j;
> +	uint8_t op_vl1;
>  
>  	for (i = 1; i < num_ports; i++) {
>  		p = osm_node_get_physp_ptr(node, i);
> @@ -225,17 +226,31 @@ static int qos_extports_setup(osm_sm_t * sm, osm_node_t *node,
>  	if (ib_switch_info_get_opt_sl2vlmapping(&node->sw->switch_info) &&
>  	    sm->p_subn->opt.use_optimized_slvl) {
>  		p = osm_node_get_physp_ptr(node, 1);
> +		op_vl1 = ib_port_info_get_op_vls(&p->port_info);
>  		force_update = p->need_update || sm->p_subn->need_update;
> -		return sl2vl_update_table(sm, p, 1, 0x30000, force_update,
> -					  &qcfg->sl2vl);
> +		if (sl2vl_update_table(sm, p, 0, 0x30000, force_update,
> +					&qcfg->sl2vl))
> +			ret = -1;
> +		/* overwrite default ALL configuration if port's
> +		   op_vl is different */
> +		for (i = 2; i < num_ports; i++) {
> +			p = osm_node_get_physp_ptr(node, i);
> +			if (ib_port_info_get_op_vls(&p->port_info) != op_vl1 && 
> +			    sl2vl_update_table(sm, p, 0, 0x20000 | i, force_update,
> +						&qcfg->sl2vl))
> +				ret = -1;
> +		}
> +		return ret;
>  	}
>  

I think below we only need to avoid port 0 when it is the output port,
and is not an enhanced port 0.

> -	for (i = 0; i < num_ports; i++) {
> +	/* non optimized sl2vl configuration */
> +	i = ib_switch_info_is_enhanced_port0(&node->sw->switch_info) ? 0 : 1;
> +	for (; i < num_ports; i++) {
>  		p = osm_node_get_physp_ptr(node, i);
>  		force_update = p->need_update || sm->p_subn->need_update;
>  		j = ib_switch_info_is_enhanced_port0(&node->sw->switch_info) ? 0 : 1;
>  		for (; j < num_ports; j++)
> -			if (sl2vl_update_table(sm, p, i, i << 8 | j,
> +			if (sl2vl_update_table(sm, p, j, j << 8 | i,
>  					       force_update, &qcfg->sl2vl))
>  				ret = -1;
>  	}


So I think the above should be:

-	for (i = 0; i < num_ports; i++) {
+	/* non optimized sl2vl configuration */
+	i = ib_switch_info_is_enhanced_port0(&node->sw->switch_info) ? 0 : 1;
+	for (; i < num_ports; i++) {
 		p = osm_node_get_physp_ptr(node, i);
 		force_update = p->need_update || sm->p_subn->need_update;
- 		j = ib_switch_info_is_enhanced_port0(&node->sw->switch_info) ? 0 : 1;
- 		for (; j < num_ports; j++)
-			if (sl2vl_update_table(sm, p, i, i << 8 | j,
+ 		for (j = 0; j < num_ports; j++)
+			if (sl2vl_update_table(sm, p, j, j << 8 | i,
 					       force_update, &qcfg->sl2vl))
 				ret = -1;
 	}

I've tested this version of your fix, and it also stops the 
messages logged as described in the commit log for e1c253e893.

Thanks -- Jim



--
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 v2] opensm: Fix sl2vl configuration

[Eli Dorfman (Voltaire)]

Fix the patch according to Jim's comments
Changed i,j to in,out to make code readable

For non-optimal sl2vl configuration in and out ports were reversed.
For optimal sl2vl added override of default ALL settting with port's
sl2vl when operational VL was other than the default port.

Signed-off-by: Eli Dorfman <elid-smomgflXvOZWk0Htik3J/w@public.gmane.org>
---
 opensm/opensm/osm_qos.c |   35 +++++++++++++++++++++++++----------
 1 files changed, 25 insertions(+), 10 deletions(-)

diff --git a/opensm/opensm/osm_qos.c b/opensm/opensm/osm_qos.c
index a571370..0ebcfdf 100644
--- a/opensm/opensm/osm_qos.c
+++ b/opensm/opensm/osm_qos.c
@@ -208,10 +208,11 @@ static int qos_extports_setup(osm_sm_t * sm, osm_node_t *node,
 	unsigned force_update;
 	unsigned num_ports = osm_node_get_num_physp(node);
 	int ret = 0;
-	unsigned i, j;
+	unsigned in, out;
+	uint8_t op_vl1;
 
-	for (i = 1; i < num_ports; i++) {
-		p = osm_node_get_physp_ptr(node, i);
+	for (out = 1; out < num_ports; out++) {
+		p = osm_node_get_physp_ptr(node, out);
 		force_update = p->need_update || sm->p_subn->need_update;
 		p->vl_high_limit = qcfg->vl_high_limit;
 		if (vlarb_update(sm, p, p->port_num, force_update, qcfg))
@@ -225,17 +226,31 @@ static int qos_extports_setup(osm_sm_t * sm, osm_node_t *node,
 	if (ib_switch_info_get_opt_sl2vlmapping(&node->sw->switch_info) &&
 	    sm->p_subn->opt.use_optimized_slvl) {
 		p = osm_node_get_physp_ptr(node, 1);
+		op_vl1 = ib_port_info_get_op_vls(&p->port_info);
 		force_update = p->need_update || sm->p_subn->need_update;
-		return sl2vl_update_table(sm, p, 1, 0x30000, force_update,
-					  &qcfg->sl2vl);
+		if (sl2vl_update_table(sm, p, 0, 0x30000, force_update,
+					&qcfg->sl2vl))
+			ret = -1;
+		/* overwrite default ALL configuration if port's
+		   op_vl is different */
+		for (out = 2; out < num_ports; out++) {
+			p = osm_node_get_physp_ptr(node, out);
+			if (ib_port_info_get_op_vls(&p->port_info) != op_vl1 && 
+			    sl2vl_update_table(sm, p, 0, 0x20000 | out, force_update,
+						&qcfg->sl2vl))
+				ret = -1;
+		}
+		return ret;
 	}
 
-	for (i = 0; i < num_ports; i++) {
-		p = osm_node_get_physp_ptr(node, i);
+	/* non optimized sl2vl configuration */
+	out = ib_switch_info_is_enhanced_port0(&node->sw->switch_info) ? 0 : 1;
+	for (; out < num_ports; out++) {
+		p = osm_node_get_physp_ptr(node, out);
 		force_update = p->need_update || sm->p_subn->need_update;
-		j = ib_switch_info_is_enhanced_port0(&node->sw->switch_info) ? 0 : 1;
-		for (; j < num_ports; j++)
-			if (sl2vl_update_table(sm, p, i, i << 8 | j,
+		/* go over all in ports */
+		for (in = 0; in < num_ports; in++)
+			if (sl2vl_update_table(sm, p, in, in << 8 | out,
 					       force_update, &qcfg->sl2vl))
 				ret = -1;
 	}
-- 
1.5.5

--
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: Fix sl2vl configuration

[Hal Rosenstock]

On Wed, Jul 28, 2010 at 12:26 PM, Eli Dorfman (Voltaire)
<dorfman.eli-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
>
> Subject: [PATCH] Fix sl2vl configuration
>
> For non-optimized sl2vl configuration in and out ports were reversed.

Are you sure these are reversed ? Any idea which commit introduced
this reversal of in and out ports ?

> For optimal sl2vl added override of default ALL settting with port's
> sl2vl when operational VL was other than the default port.

What is the motivation to override when the operational VLs is
different ? Why is that better than what is done currently ?

Is this really a policy issue ?

IMO there are two separate issues in this patch and they should be in
separate patches (for better git bisection).

Also, a couple of (possibly related) questions below.

> Signed-off-by: Eli Dorfman <elid-smomgflXvOZWk0Htik3J/w@public.gmane.org>
> ---
>  opensm/opensm/osm_qos.c |   25 ++++++++++++++++++++-----
>  1 files changed, 20 insertions(+), 5 deletions(-)
>
> diff --git a/opensm/opensm/osm_qos.c b/opensm/opensm/osm_qos.c
> index a571370..de0ae23 100644
> --- a/opensm/opensm/osm_qos.c
> +++ b/opensm/opensm/osm_qos.c
> @@ -182,7 +182,7 @@ static ib_api_status_t sl2vl_update_table(osm_sm_t * sm, osm_physp_t * p,
>                tbl.raw_vl_by_sl[i] = (vl1 << 4) | vl2;
>        }
>
> -       if (!force_update && (p_tbl = osm_physp_get_slvl_tbl(p, in_port)) &&
> +       if (!force_update && in_port && (p_tbl = osm_physp_get_slvl_tbl(p, in_port)) &&
>            !memcmp(p_tbl, &tbl, sizeof(tbl)))
>                return IB_SUCCESS;

Why exclude port 0 here ? Is it related to the change noted below ?

> @@ -209,6 +209,7 @@ static int qos_extports_setup(osm_sm_t * sm, osm_node_t *node,
>        unsigned num_ports = osm_node_get_num_physp(node);
>        int ret = 0;
>        unsigned i, j;
> +       uint8_t op_vl1;
>
>        for (i = 1; i < num_ports; i++) {
>                p = osm_node_get_physp_ptr(node, i);
> @@ -225,17 +226,31 @@ static int qos_extports_setup(osm_sm_t * sm, osm_node_t *node,
>        if (ib_switch_info_get_opt_sl2vlmapping(&node->sw->switch_info) &&
>            sm->p_subn->opt.use_optimized_slvl) {
>                p = osm_node_get_physp_ptr(node, 1);
> +               op_vl1 = ib_port_info_get_op_vls(&p->port_info);
>                force_update = p->need_update || sm->p_subn->need_update;
> -               return sl2vl_update_table(sm, p, 1, 0x30000, force_update,
> -                                         &qcfg->sl2vl);
> +               if (sl2vl_update_table(sm, p, 0, 0x30000, force_update,

Why is the third parameter (in_port) changed from 1 to 0 here ? Maybe
that's related to the change above for the skipping of port 0 in
sl2vl_update_table.

-- Hal

> +                                       &qcfg->sl2vl))
> +                       ret = -1;
> +               /* overwrite default ALL configuration if port's
> +                  op_vl is different */
> +               for (i = 2; i < num_ports; i++) {
> +                       p = osm_node_get_physp_ptr(node, i);
> +                       if (ib_port_info_get_op_vls(&p->port_info) != op_vl1 &&
> +                           sl2vl_update_table(sm, p, 0, 0x20000 | i, force_update,
> +                                               &qcfg->sl2vl))
> +                               ret = -1;
> +               }
> +               return ret;
>        }
>
> -       for (i = 0; i < num_ports; i++) {
> +       /* non optimized sl2vl configuration */
> +       i = ib_switch_info_is_enhanced_port0(&node->sw->switch_info) ? 0 : 1;
> +       for (; i < num_ports; i++) {
>                p = osm_node_get_physp_ptr(node, i);
>                force_update = p->need_update || sm->p_subn->need_update;
>                j = ib_switch_info_is_enhanced_port0(&node->sw->switch_info) ? 0 : 1;
>                for (; j < num_ports; j++)
> -                       if (sl2vl_update_table(sm, p, i, i << 8 | j,
> +                       if (sl2vl_update_table(sm, p, j, j << 8 | i,
>                                               force_update, &qcfg->sl2vl))
>                                ret = -1;
>        }
> --
> 1.5.5
>
> --
> 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: Fix sl2vl configuration

[Eli Dorfman]

Hi Hal,

On Mon, Aug 23, 2010 at 5:25 PM, Hal Rosenstock
<hal.rosenstock-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> On Wed, Jul 28, 2010 at 12:26 PM, Eli Dorfman (Voltaire)
> <dorfman.eli-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
>>
>> Subject: [PATCH] Fix sl2vl configuration
>>
>> For non-optimized sl2vl configuration in and out ports were reversed.
>
> Are you sure these are reversed ? Any idea which commit introduced
> this reversal of in and out ports ?
I'm sure they are reversed. This patch was also tested by Jim Schutt.
I didn't check which commit introduced this - why is it important?

>
>> For optimal sl2vl added override of default ALL settting with port's
>> sl2vl when operational VL was other than the default port.
>
> What is the motivation to override when the operational VLs is
> different ? Why is that better than what is done currently ?
The idea was to apply the default policy - set sl2vl modulo operational VL.
When applying ALL settings using port 1 we still want to override this
setting for ports with different operational VL.

>
> Is this really a policy issue ?
>
> IMO there are two separate issues in this patch and they should be in
> separate patches (for better git bisection).
Maybe, but I still think this patch fixes wrong setting for sl2vl
using optimized and non optimized methods.
I'm not sure this should be divided to 2 separate patches.

>
> Also, a couple of (possibly related) questions below.
It seems that you are referring to patch v1 which was modified
according to Jim's comments.
Please check the v2 patch .

Thanks,
Eli

>
>> Signed-off-by: Eli Dorfman <elid-smomgflXvOZWk0Htik3J/w@public.gmane.org>
>> ---
>>  opensm/opensm/osm_qos.c |   25 ++++++++++++++++++++-----
>>  1 files changed, 20 insertions(+), 5 deletions(-)
>>
>> diff --git a/opensm/opensm/osm_qos.c b/opensm/opensm/osm_qos.c
>> index a571370..de0ae23 100644
>> --- a/opensm/opensm/osm_qos.c
>> +++ b/opensm/opensm/osm_qos.c
>> @@ -182,7 +182,7 @@ static ib_api_status_t sl2vl_update_table(osm_sm_t * sm, osm_physp_t * p,
>>                tbl.raw_vl_by_sl[i] = (vl1 << 4) | vl2;
>>        }
>>
>> -       if (!force_update && (p_tbl = osm_physp_get_slvl_tbl(p, in_port)) &&
>> +       if (!force_update && in_port && (p_tbl = osm_physp_get_slvl_tbl(p, in_port)) &&
>>            !memcmp(p_tbl, &tbl, sizeof(tbl)))
>>                return IB_SUCCESS;
>
> Why exclude port 0 here ? Is it related to the change noted below ?
>
>> @@ -209,6 +209,7 @@ static int qos_extports_setup(osm_sm_t * sm, osm_node_t *node,
>>        unsigned num_ports = osm_node_get_num_physp(node);
>>        int ret = 0;
>>        unsigned i, j;
>> +       uint8_t op_vl1;
>>
>>        for (i = 1; i < num_ports; i++) {
>>                p = osm_node_get_physp_ptr(node, i);
>> @@ -225,17 +226,31 @@ static int qos_extports_setup(osm_sm_t * sm, osm_node_t *node,
>>        if (ib_switch_info_get_opt_sl2vlmapping(&node->sw->switch_info) &&
>>            sm->p_subn->opt.use_optimized_slvl) {
>>                p = osm_node_get_physp_ptr(node, 1);
>> +               op_vl1 = ib_port_info_get_op_vls(&p->port_info);
>>                force_update = p->need_update || sm->p_subn->need_update;
>> -               return sl2vl_update_table(sm, p, 1, 0x30000, force_update,
>> -                                         &qcfg->sl2vl);
>> +               if (sl2vl_update_table(sm, p, 0, 0x30000, force_update,
>
> Why is the third parameter (in_port) changed from 1 to 0 here ? Maybe
> that's related to the change above for the skipping of port 0 in
> sl2vl_update_table.
>
> -- Hal
>
>> +                                       &qcfg->sl2vl))
>> +                       ret = -1;
>> +               /* overwrite default ALL configuration if port's
>> +                  op_vl is different */
>> +               for (i = 2; i < num_ports; i++) {
>> +                       p = osm_node_get_physp_ptr(node, i);
>> +                       if (ib_port_info_get_op_vls(&p->port_info) != op_vl1 &&
>> +                           sl2vl_update_table(sm, p, 0, 0x20000 | i, force_update,
>> +                                               &qcfg->sl2vl))
>> +                               ret = -1;
>> +               }
>> +               return ret;
>>        }
>>
>> -       for (i = 0; i < num_ports; i++) {
>> +       /* non optimized sl2vl configuration */
>> +       i = ib_switch_info_is_enhanced_port0(&node->sw->switch_info) ? 0 : 1;
>> +       for (; i < num_ports; i++) {
>>                p = osm_node_get_physp_ptr(node, i);
>>                force_update = p->need_update || sm->p_subn->need_update;
>>                j = ib_switch_info_is_enhanced_port0(&node->sw->switch_info) ? 0 : 1;
>>                for (; j < num_ports; j++)
>> -                       if (sl2vl_update_table(sm, p, i, i << 8 | j,
>> +                       if (sl2vl_update_table(sm, p, j, j << 8 | i,
>>                                               force_update, &qcfg->sl2vl))
>>                                ret = -1;
>>        }
>> --
>> 1.5.5
>>
>> --
>> 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: Fix sl2vl configuration

[Hal Rosenstock]

Hi Eli,

On Mon, Aug 23, 2010 at 3:37 PM, Eli Dorfman <dorfman.eli-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> Hi Hal,
>
> On Mon, Aug 23, 2010 at 5:25 PM, Hal Rosenstock
> <hal.rosenstock-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
>> On Wed, Jul 28, 2010 at 12:26 PM, Eli Dorfman (Voltaire)
>> <dorfman.eli-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
>>>
>>> Subject: [PATCH] Fix sl2vl configuration
>>>
>>> For non-optimized sl2vl configuration in and out ports were reversed.
>>
>> Are you sure these are reversed ? Any idea which commit introduced
>> this reversal of in and out ports ?
> I'm sure they are reversed.

I looked at it some more and the ports look reversed to me too.

> This patch was also tested by Jim Schutt.

That only means it works in his environment rather than "correctness".

> I didn't check which commit introduced this - why is it important?

I'd like to understand which patch introduced the reversal. I don't
see it but might have missed it.  I think it's important to know which
versions are broken.

>>
>>> For optimal sl2vl added override of default ALL settting with port's
>>> sl2vl when operational VL was other than the default port.
>>
>> What is the motivation to override when the operational VLs is
>> different ? Why is that better than what is done currently ?
> The idea was to apply the default policy - set sl2vl modulo operational VL.
> When applying ALL settings using port 1 we still want to override this
> setting for ports with different operational VL.

What makes the default policy modulo operational VLs ?

>
>>
>> Is this really a policy issue ?
>>
>> IMO there are two separate issues in this patch and they should be in
>> separate patches (for better git bisection).
> Maybe, but I still think this patch fixes wrong setting for sl2vl
> using optimized and non optimized methods.
> I'm not sure this should be divided to 2 separate patches.

It's one thought per patch and to me this is two different thoughts.

>>
>> Also, a couple of (possibly related) questions below.
> It seems that you are referring to patch v1 which was modified
> according to Jim's comments.
> Please check the v2 patch .

I see my questions are moot in terms of the v2 patch.

-- Hal

> Thanks,
> Eli
>
>>
>>> Signed-off-by: Eli Dorfman <elid-smomgflXvOZWk0Htik3J/w@public.gmane.org>
>>> ---
>>>  opensm/opensm/osm_qos.c |   25 ++++++++++++++++++++-----
>>>  1 files changed, 20 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/opensm/opensm/osm_qos.c b/opensm/opensm/osm_qos.c
>>> index a571370..de0ae23 100644
>>> --- a/opensm/opensm/osm_qos.c
>>> +++ b/opensm/opensm/osm_qos.c
>>> @@ -182,7 +182,7 @@ static ib_api_status_t sl2vl_update_table(osm_sm_t * sm, osm_physp_t * p,
>>>                tbl.raw_vl_by_sl[i] = (vl1 << 4) | vl2;
>>>        }
>>>
>>> -       if (!force_update && (p_tbl = osm_physp_get_slvl_tbl(p, in_port)) &&
>>> +       if (!force_update && in_port && (p_tbl = osm_physp_get_slvl_tbl(p, in_port)) &&
>>>            !memcmp(p_tbl, &tbl, sizeof(tbl)))
>>>                return IB_SUCCESS;
>>
>> Why exclude port 0 here ? Is it related to the change noted below ?
>>
>>> @@ -209,6 +209,7 @@ static int qos_extports_setup(osm_sm_t * sm, osm_node_t *node,
>>>        unsigned num_ports = osm_node_get_num_physp(node);
>>>        int ret = 0;
>>>        unsigned i, j;
>>> +       uint8_t op_vl1;
>>>
>>>        for (i = 1; i < num_ports; i++) {
>>>                p = osm_node_get_physp_ptr(node, i);
>>> @@ -225,17 +226,31 @@ static int qos_extports_setup(osm_sm_t * sm, osm_node_t *node,
>>>        if (ib_switch_info_get_opt_sl2vlmapping(&node->sw->switch_info) &&
>>>            sm->p_subn->opt.use_optimized_slvl) {
>>>                p = osm_node_get_physp_ptr(node, 1);
>>> +               op_vl1 = ib_port_info_get_op_vls(&p->port_info);
>>>                force_update = p->need_update || sm->p_subn->need_update;
>>> -               return sl2vl_update_table(sm, p, 1, 0x30000, force_update,
>>> -                                         &qcfg->sl2vl);
>>> +               if (sl2vl_update_table(sm, p, 0, 0x30000, force_update,
>>
>> Why is the third parameter (in_port) changed from 1 to 0 here ? Maybe
>> that's related to the change above for the skipping of port 0 in
>> sl2vl_update_table.
>>
>> -- Hal
>>
>>> +                                       &qcfg->sl2vl))
>>> +                       ret = -1;
>>> +               /* overwrite default ALL configuration if port's
>>> +                  op_vl is different */
>>> +               for (i = 2; i < num_ports; i++) {
>>> +                       p = osm_node_get_physp_ptr(node, i);
>>> +                       if (ib_port_info_get_op_vls(&p->port_info) != op_vl1 &&
>>> +                           sl2vl_update_table(sm, p, 0, 0x20000 | i, force_update,
>>> +                                               &qcfg->sl2vl))
>>> +                               ret = -1;
>>> +               }
>>> +               return ret;
>>>        }
>>>
>>> -       for (i = 0; i < num_ports; i++) {
>>> +       /* non optimized sl2vl configuration */
>>> +       i = ib_switch_info_is_enhanced_port0(&node->sw->switch_info) ? 0 : 1;
>>> +       for (; i < num_ports; i++) {
>>>                p = osm_node_get_physp_ptr(node, i);
>>>                force_update = p->need_update || sm->p_subn->need_update;
>>>                j = ib_switch_info_is_enhanced_port0(&node->sw->switch_info) ? 0 : 1;
>>>                for (; j < num_ports; j++)
>>> -                       if (sl2vl_update_table(sm, p, i, i << 8 | j,
>>> +                       if (sl2vl_update_table(sm, p, j, j << 8 | i,
>>>                                               force_update, &qcfg->sl2vl))
>>>                                ret = -1;
>>>        }
>>> --
>>> 1.5.5
>>>
>>> --
>>> 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: Fix sl2vl configuration

[Eli Dorfman (Voltaire)]

Hal Rosenstock wrote:
> Hi Eli,
> 
> On Mon, Aug 23, 2010 at 3:37 PM, Eli Dorfman <dorfman.eli-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
>> Hi Hal,
>>
>> On Mon, Aug 23, 2010 at 5:25 PM, Hal Rosenstock
>> <hal.rosenstock-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
>>> On Wed, Jul 28, 2010 at 12:26 PM, Eli Dorfman (Voltaire)
>>> <dorfman.eli-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
>>>> Subject: [PATCH] Fix sl2vl configuration
>>>>
>>>> For non-optimized sl2vl configuration in and out ports were reversed.
>>> Are you sure these are reversed ? Any idea which commit introduced
>>> this reversal of in and out ports ?
>> I'm sure they are reversed.
> 
> I looked at it some more and the ports look reversed to me too.
> 
>> This patch was also tested by Jim Schutt.
> 
> That only means it works in his environment rather than "correctness".
It was tested in mine enviornment as well.
Usually I test the patch in my environment to verify its correctness (in addition to code review).
I assume you do the same.

Do you expect me to test the patch in your environment as well?


> 
>> I didn't check which commit introduced this - why is it important?
> 
> I'd like to understand which patch introduced the reversal. I don't
> see it but might have missed it.  I think it's important to know which
> versions are broken.

I think that the following commit added the bug:

commit 051a1dd514e63f3a971afad38e377932efca5e18
Author: Sasha Khapyorsky <sashak-smomgflXvOZWk0Htik3J/w@public.gmane.org>
Date:   Mon Jan 4 21:06:19 2010 +0200

    opensm/osm_qos.c: split switch external and end ports setup

    This splits QoS related port parameters setup over switch external ports
    and end ports. Such separation leaves us with simpler code and saves
    some repeated flows in case of switch external ports (actually required
    per switch and not per port).

    Another advantages: Optimized Sl2VL Mapping procedure can be implemented
    easier using this model. A low level port QoS related parameters setup
    infrastructure is ready for supporting per port QoS related configuration
    (which hopefully will be implemented some days).



> 
>>>> For optimal sl2vl added override of default ALL settting with port's
>>>> sl2vl when operational VL was other than the default port.
>>> What is the motivation to override when the operational VLs is
>>> different ? Why is that better than what is done currently ?
>> The idea was to apply the default policy - set sl2vl modulo operational VL.
>> When applying ALL settings using port 1 we still want to override this
>> setting for ports with different operational VL.
> 
> What makes the default policy modulo operational VLs ?
This is how its implemented in sl2vl_update_table()

        vl_mask = (1 << (ib_port_info_get_op_vls(&p->port_info) - 1)) - 1;

        for (i = 0; i < IB_MAX_NUM_VLS / 2; i++) {
                vl1 = sl2vl_table->raw_vl_by_sl[i] >> 4;
                vl2 = sl2vl_table->raw_vl_by_sl[i] & 0xf;
                if (vl1 != 15)
                        vl1 &= vl_mask;
                if (vl2 != 15)
                        vl2 &= vl_mask;

Default startup switches configuration uses the same policy.


> 
>>> Is this really a policy issue ?
>>>
>>> IMO there are two separate issues in this patch and they should be in
>>> separate patches (for better git bisection).
>> Maybe, but I still think this patch fixes wrong setting for sl2vl
>> using optimized and non optimized methods.
>> I'm not sure this should be divided to 2 separate patches.
> 
> It's one thought per patch and to me this is two different thoughts.
If this is the only issue, I can split this patch to 2 separate patches.

Eli
> 
>>> Also, a couple of (possibly related) questions below.
>> It seems that you are referring to patch v1 which was modified
>> according to Jim's comments.
>> Please check the v2 patch .
> 
> I see my questions are moot in terms of the v2 patch.
> 
> -- Hal
> 
>> Thanks,
>> Eli
>>
>>>> Signed-off-by: Eli Dorfman <elid-smomgflXvOZWk0Htik3J/w@public.gmane.org>
>>>> ---
>>>>  opensm/opensm/osm_qos.c |   25 ++++++++++++++++++++-----
>>>>  1 files changed, 20 insertions(+), 5 deletions(-)
>>>>
>>>> diff --git a/opensm/opensm/osm_qos.c b/opensm/opensm/osm_qos.c
>>>> index a571370..de0ae23 100644
>>>> --- a/opensm/opensm/osm_qos.c
>>>> +++ b/opensm/opensm/osm_qos.c
>>>> @@ -182,7 +182,7 @@ static ib_api_status_t sl2vl_update_table(osm_sm_t * sm, osm_physp_t * p,
>>>>                tbl.raw_vl_by_sl[i] = (vl1 << 4) | vl2;
>>>>        }
>>>>
>>>> -       if (!force_update && (p_tbl = osm_physp_get_slvl_tbl(p, in_port)) &&
>>>> +       if (!force_update && in_port && (p_tbl = osm_physp_get_slvl_tbl(p, in_port)) &&
>>>>            !memcmp(p_tbl, &tbl, sizeof(tbl)))
>>>>                return IB_SUCCESS;
>>> Why exclude port 0 here ? Is it related to the change noted below ?
>>>
>>>> @@ -209,6 +209,7 @@ static int qos_extports_setup(osm_sm_t * sm, osm_node_t *node,
>>>>        unsigned num_ports = osm_node_get_num_physp(node);
>>>>        int ret = 0;
>>>>        unsigned i, j;
>>>> +       uint8_t op_vl1;
>>>>
>>>>        for (i = 1; i < num_ports; i++) {
>>>>                p = osm_node_get_physp_ptr(node, i);
>>>> @@ -225,17 +226,31 @@ static int qos_extports_setup(osm_sm_t * sm, osm_node_t *node,
>>>>        if (ib_switch_info_get_opt_sl2vlmapping(&node->sw->switch_info) &&
>>>>            sm->p_subn->opt.use_optimized_slvl) {
>>>>                p = osm_node_get_physp_ptr(node, 1);
>>>> +               op_vl1 = ib_port_info_get_op_vls(&p->port_info);
>>>>                force_update = p->need_update || sm->p_subn->need_update;
>>>> -               return sl2vl_update_table(sm, p, 1, 0x30000, force_update,
>>>> -                                         &qcfg->sl2vl);
>>>> +               if (sl2vl_update_table(sm, p, 0, 0x30000, force_update,
>>> Why is the third parameter (in_port) changed from 1 to 0 here ? Maybe
>>> that's related to the change above for the skipping of port 0 in
>>> sl2vl_update_table.
>>>
>>> -- Hal
>>>
>>>> +                                       &qcfg->sl2vl))
>>>> +                       ret = -1;
>>>> +               /* overwrite default ALL configuration if port's
>>>> +                  op_vl is different */
>>>> +               for (i = 2; i < num_ports; i++) {
>>>> +                       p = osm_node_get_physp_ptr(node, i);
>>>> +                       if (ib_port_info_get_op_vls(&p->port_info) != op_vl1 &&
>>>> +                           sl2vl_update_table(sm, p, 0, 0x20000 | i, force_update,
>>>> +                                               &qcfg->sl2vl))
>>>> +                               ret = -1;
>>>> +               }
>>>> +               return ret;
>>>>        }
>>>>
>>>> -       for (i = 0; i < num_ports; i++) {
>>>> +       /* non optimized sl2vl configuration */
>>>> +       i = ib_switch_info_is_enhanced_port0(&node->sw->switch_info) ? 0 : 1;
>>>> +       for (; i < num_ports; i++) {
>>>>                p = osm_node_get_physp_ptr(node, i);
>>>>                force_update = p->need_update || sm->p_subn->need_update;
>>>>                j = ib_switch_info_is_enhanced_port0(&node->sw->switch_info) ? 0 : 1;
>>>>                for (; j < num_ports; j++)
>>>> -                       if (sl2vl_update_table(sm, p, i, i << 8 | j,
>>>> +                       if (sl2vl_update_table(sm, p, j, j << 8 | i,
>>>>                                               force_update, &qcfg->sl2vl))
>>>>                                ret = -1;
>>>>        }
>>>> --
>>>> 1.5.5
>>>>
>>>> --
>>>> 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: Fix sl2vl configuration

[Hal Rosenstock]

On Wed, Aug 25, 2010 at 4:43 AM, Eli Dorfman (Voltaire)
<dorfman.eli-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> Hal Rosenstock wrote:
>> Hi Eli,
>>
>> On Mon, Aug 23, 2010 at 3:37 PM, Eli Dorfman <dorfman.eli-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
>>> Hi Hal,
>>>
>>> On Mon, Aug 23, 2010 at 5:25 PM, Hal Rosenstock
>>> <hal.rosenstock-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
>>>> On Wed, Jul 28, 2010 at 12:26 PM, Eli Dorfman (Voltaire)
>>>> <dorfman.eli-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
>>>>> Subject: [PATCH] Fix sl2vl configuration
>>>>>
>>>>> For non-optimized sl2vl configuration in and out ports were reversed.
>>>> Are you sure these are reversed ? Any idea which commit introduced
>>>> this reversal of in and out ports ?
>>> I'm sure they are reversed.
>>
>> I looked at it some more and the ports look reversed to me too.
>>
>>> This patch was also tested by Jim Schutt.
>>
>> That only means it works in his environment rather than "correctness".
> It was tested in mine enviornment as well.
> Usually I test the patch in my environment to verify its correctness (in addition to code review).
> I assume you do the same.
>
> Do you expect me to test the patch in your environment as well?

Huh ?

>
>>
>>> I didn't check which commit introduced this - why is it important?
>>
>> I'd like to understand which patch introduced the reversal. I don't
>> see it but might have missed it.  I think it's important to know which
>> versions are broken.
>
> I think that the following commit added the bug:
>
> commit 051a1dd514e63f3a971afad38e377932efca5e18
> Author: Sasha Khapyorsky <sashak-smomgflXvOZWk0Htik3J/w@public.gmane.org>
> Date:   Mon Jan 4 21:06:19 2010 +0200
>
>    opensm/osm_qos.c: split switch external and end ports setup
>
>    This splits QoS related port parameters setup over switch external ports
>    and end ports. Such separation leaves us with simpler code and saves
>    some repeated flows in case of switch external ports (actually required
>    per switch and not per port).
>
>    Another advantages: Optimized Sl2VL Mapping procedure can be implemented
>    easier using this model. A low level port QoS related parameters setup
>    infrastructure is ready for supporting per port QoS related configuration
>    (which hopefully will be implemented some days).
>

I thought it might be that one but wasn't sure. Thanks.

>
>>
>>>>> For optimal sl2vl added override of default ALL settting with port's
>>>>> sl2vl when operational VL was other than the default port.
>>>> What is the motivation to override when the operational VLs is
>>>> different ? Why is that better than what is done currently ?
>>> The idea was to apply the default policy - set sl2vl modulo operational VL.
>>> When applying ALL settings using port 1 we still want to override this
>>> setting for ports with different operational VL.
>>
>> What makes the default policy modulo operational VLs ?
> This is how its implemented in sl2vl_update_table()
>        vl_mask = (1 << (ib_port_info_get_op_vls(&p->port_info) - 1)) - 1;
>
>        for (i = 0; i < IB_MAX_NUM_VLS / 2; i++) {
>                vl1 = sl2vl_table->raw_vl_by_sl[i] >> 4;
>                vl2 = sl2vl_table->raw_vl_by_sl[i] & 0xf;
>                if (vl1 != 15)
>                        vl1 &= vl_mask;
>                if (vl2 != 15)
>                        vl2 &= vl_mask;
>
> Default startup switches configuration uses the same policy.

I view this as further modification on the configuration based on the
operational VLs. Besides, any VL specified above the op VLs is a drop
(same as indicating VL15).

I think what you are doing is changing the default behavior and hence
perhaps an additional policy switch is needed if this change really is
needed and I'm unsure of that.

>>
>>>> Is this really a policy issue ?
>>>>
>>>> IMO there are two separate issues in this patch and they should be in
>>>> separate patches (for better git bisection).
>>> Maybe, but I still think this patch fixes wrong setting for sl2vl
>>> using optimized and non optimized methods.
>>> I'm not sure this should be divided to 2 separate patches.
>>
>> It's one thought per patch and to me this is two different thoughts.
> If this is the only issue, I can split this patch to 2 separate patches.

It's also the issue noted above.

-- Hal

> Eli
>>
>>>> Also, a couple of (possibly related) questions below.
>>> It seems that you are referring to patch v1 which was modified
>>> according to Jim's comments.
>>> Please check the v2 patch .
>>
>> I see my questions are moot in terms of the v2 patch.
>>
>> -- Hal
>>
>>> Thanks,
>>> Eli
>>>
>>>>> Signed-off-by: Eli Dorfman <elid-smomgflXvOZWk0Htik3J/w@public.gmane.org>
>>>>> ---
>>>>>  opensm/opensm/osm_qos.c |   25 ++++++++++++++++++++-----
>>>>>  1 files changed, 20 insertions(+), 5 deletions(-)
>>>>>
>>>>> diff --git a/opensm/opensm/osm_qos.c b/opensm/opensm/osm_qos.c
>>>>> index a571370..de0ae23 100644
>>>>> --- a/opensm/opensm/osm_qos.c
>>>>> +++ b/opensm/opensm/osm_qos.c
>>>>> @@ -182,7 +182,7 @@ static ib_api_status_t sl2vl_update_table(osm_sm_t * sm, osm_physp_t * p,
>>>>>                tbl.raw_vl_by_sl[i] = (vl1 << 4) | vl2;
>>>>>        }
>>>>>
>>>>> -       if (!force_update && (p_tbl = osm_physp_get_slvl_tbl(p, in_port)) &&
>>>>> +       if (!force_update && in_port && (p_tbl = osm_physp_get_slvl_tbl(p, in_port)) &&
>>>>>            !memcmp(p_tbl, &tbl, sizeof(tbl)))
>>>>>                return IB_SUCCESS;
>>>> Why exclude port 0 here ? Is it related to the change noted below ?
>>>>
>>>>> @@ -209,6 +209,7 @@ static int qos_extports_setup(osm_sm_t * sm, osm_node_t *node,
>>>>>        unsigned num_ports = osm_node_get_num_physp(node);
>>>>>        int ret = 0;
>>>>>        unsigned i, j;
>>>>> +       uint8_t op_vl1;
>>>>>
>>>>>        for (i = 1; i < num_ports; i++) {
>>>>>                p = osm_node_get_physp_ptr(node, i);
>>>>> @@ -225,17 +226,31 @@ static int qos_extports_setup(osm_sm_t * sm, osm_node_t *node,
>>>>>        if (ib_switch_info_get_opt_sl2vlmapping(&node->sw->switch_info) &&
>>>>>            sm->p_subn->opt.use_optimized_slvl) {
>>>>>                p = osm_node_get_physp_ptr(node, 1);
>>>>> +               op_vl1 = ib_port_info_get_op_vls(&p->port_info);
>>>>>                force_update = p->need_update || sm->p_subn->need_update;
>>>>> -               return sl2vl_update_table(sm, p, 1, 0x30000, force_update,
>>>>> -                                         &qcfg->sl2vl);
>>>>> +               if (sl2vl_update_table(sm, p, 0, 0x30000, force_update,
>>>> Why is the third parameter (in_port) changed from 1 to 0 here ? Maybe
>>>> that's related to the change above for the skipping of port 0 in
>>>> sl2vl_update_table.
>>>>
>>>> -- Hal
>>>>
>>>>> +                                       &qcfg->sl2vl))
>>>>> +                       ret = -1;
>>>>> +               /* overwrite default ALL configuration if port's
>>>>> +                  op_vl is different */
>>>>> +               for (i = 2; i < num_ports; i++) {
>>>>> +                       p = osm_node_get_physp_ptr(node, i);
>>>>> +                       if (ib_port_info_get_op_vls(&p->port_info) != op_vl1 &&
>>>>> +                           sl2vl_update_table(sm, p, 0, 0x20000 | i, force_update,
>>>>> +                                               &qcfg->sl2vl))
>>>>> +                               ret = -1;
>>>>> +               }
>>>>> +               return ret;
>>>>>        }
>>>>>
>>>>> -       for (i = 0; i < num_ports; i++) {
>>>>> +       /* non optimized sl2vl configuration */
>>>>> +       i = ib_switch_info_is_enhanced_port0(&node->sw->switch_info) ? 0 : 1;
>>>>> +       for (; i < num_ports; i++) {
>>>>>                p = osm_node_get_physp_ptr(node, i);
>>>>>                force_update = p->need_update || sm->p_subn->need_update;
>>>>>                j = ib_switch_info_is_enhanced_port0(&node->sw->switch_info) ? 0 : 1;
>>>>>                for (; j < num_ports; j++)
>>>>> -                       if (sl2vl_update_table(sm, p, i, i << 8 | j,
>>>>> +                       if (sl2vl_update_table(sm, p, j, j << 8 | i,
>>>>>                                               force_update, &qcfg->sl2vl))
>>>>>                                ret = -1;
>>>>>        }
>>>>> --
>>>>> 1.5.5
>>>>>
>>>>> --
>>>>> 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: Fix sl2vl configuration

[Eli Dorfman (Voltaire)]

Hi Hal,

Hal Rosenstock wrote:
> On Wed, Aug 25, 2010 at 4:43 AM, Eli Dorfman (Voltaire)
> <dorfman.eli-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
>> Hal Rosenstock wrote:
>>> Hi Eli,
>>>
>>> On Mon, Aug 23, 2010 at 3:37 PM, Eli Dorfman <dorfman.eli-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
>>>> Hi Hal,
>>>>
>>>> On Mon, Aug 23, 2010 at 5:25 PM, Hal Rosenstock
>>>> <hal.rosenstock-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
>>>>> On Wed, Jul 28, 2010 at 12:26 PM, Eli Dorfman (Voltaire)
>>>>> <dorfman.eli-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
>>>>>> Subject: [PATCH] Fix sl2vl configuration
>>>>>>
>>>>>> For non-optimized sl2vl configuration in and out ports were reversed.
>>>>> Are you sure these are reversed ? Any idea which commit introduced
>>>>> this reversal of in and out ports ?
>>>> I'm sure they are reversed.
>>> I looked at it some more and the ports look reversed to me too.
>>>
>>>> This patch was also tested by Jim Schutt.
>>> That only means it works in his environment rather than "correctness".
>> It was tested in mine enviornment as well.
>> Usually I test the patch in my environment to verify its correctness (in addition to code review).
>> I assume you do the same.
>>
>> Do you expect me to test the patch in your environment as well?
> 
> Huh ?
> 
>>>> I didn't check which commit introduced this - why is it important?
>>> I'd like to understand which patch introduced the reversal. I don't
>>> see it but might have missed it.  I think it's important to know which
>>> versions are broken.
>> I think that the following commit added the bug:
>>
>> commit 051a1dd514e63f3a971afad38e377932efca5e18
>> Author: Sasha Khapyorsky <sashak-smomgflXvOZWk0Htik3J/w@public.gmane.org>
>> Date:   Mon Jan 4 21:06:19 2010 +0200
>>
>>    opensm/osm_qos.c: split switch external and end ports setup
>>
>>    This splits QoS related port parameters setup over switch external ports
>>    and end ports. Such separation leaves us with simpler code and saves
>>    some repeated flows in case of switch external ports (actually required
>>    per switch and not per port).
>>
>>    Another advantages: Optimized Sl2VL Mapping procedure can be implemented
>>    easier using this model. A low level port QoS related parameters setup
>>    infrastructure is ready for supporting per port QoS related configuration
>>    (which hopefully will be implemented some days).
>>
> 
> I thought it might be that one but wasn't sure. Thanks.
> 
>>>>>> For optimal sl2vl added override of default ALL settting with port's
>>>>>> sl2vl when operational VL was other than the default port.
>>>>> What is the motivation to override when the operational VLs is
>>>>> different ? Why is that better than what is done currently ?
>>>> The idea was to apply the default policy - set sl2vl modulo operational VL.
>>>> When applying ALL settings using port 1 we still want to override this
>>>> setting for ports with different operational VL.
>>> What makes the default policy modulo operational VLs ?
>> This is how its implemented in sl2vl_update_table()
>>        vl_mask = (1 << (ib_port_info_get_op_vls(&p->port_info) - 1)) - 1;
>>
>>        for (i = 0; i < IB_MAX_NUM_VLS / 2; i++) {
>>                vl1 = sl2vl_table->raw_vl_by_sl[i] >> 4;
>>                vl2 = sl2vl_table->raw_vl_by_sl[i] & 0xf;
>>                if (vl1 != 15)
>>                        vl1 &= vl_mask;
>>                if (vl2 != 15)
>>                        vl2 &= vl_mask;
>>
>> Default startup switches configuration uses the same policy.
> 
> I view this as further modification on the configuration based on the
> operational VLs. Besides, any VL specified above the op VLs is a drop
> (same as indicating VL15).

This code is already in the trunk and my patch does not change that.
The only diff in the patch is the fix of reversed in/out ports and corresponding fix for
optimized sl2vl configuration.

> 
> I think what you are doing is changing the default behavior and hence
> perhaps an additional policy switch is needed if this change really is
> needed and I'm unsure of that.

I don't change the default behavior and this change is needed.
Without it for example, if you connect a switch with opVL=4 (VL0-VL7) to a node that 
supports only VL0 and try to send traffic on sl>SL0 it will be dropped.

Eli

> 
>>>>> Is this really a policy issue ?
>>>>>
>>>>> IMO there are two separate issues in this patch and they should be in
>>>>> separate patches (for better git bisection).
>>>> Maybe, but I still think this patch fixes wrong setting for sl2vl
>>>> using optimized and non optimized methods.
>>>> I'm not sure this should be divided to 2 separate patches.
>>> It's one thought per patch and to me this is two different thoughts.
>> If this is the only issue, I can split this patch to 2 separate patches.
> 
> It's also the issue noted above.
> 
> -- Hal
> 
>> Eli
>>>>> Also, a couple of (possibly related) questions below.
>>>> It seems that you are referring to patch v1 which was modified
>>>> according to Jim's comments.
>>>> Please check the v2 patch .
>>> I see my questions are moot in terms of the v2 patch.
>>>
>>> -- Hal
>>>
>>>> Thanks,
>>>> Eli
>>>>
>>>>>> Signed-off-by: Eli Dorfman <elid-smomgflXvOZWk0Htik3J/w@public.gmane.org>
>>>>>> ---
>>>>>>  opensm/opensm/osm_qos.c |   25 ++++++++++++++++++++-----
>>>>>>  1 files changed, 20 insertions(+), 5 deletions(-)
>>>>>>
>>>>>> diff --git a/opensm/opensm/osm_qos.c b/opensm/opensm/osm_qos.c
>>>>>> index a571370..de0ae23 100644
>>>>>> --- a/opensm/opensm/osm_qos.c
>>>>>> +++ b/opensm/opensm/osm_qos.c
>>>>>> @@ -182,7 +182,7 @@ static ib_api_status_t sl2vl_update_table(osm_sm_t * sm, osm_physp_t * p,
>>>>>>                tbl.raw_vl_by_sl[i] = (vl1 << 4) | vl2;
>>>>>>        }
>>>>>>
>>>>>> -       if (!force_update && (p_tbl = osm_physp_get_slvl_tbl(p, in_port)) &&
>>>>>> +       if (!force_update && in_port && (p_tbl = osm_physp_get_slvl_tbl(p, in_port)) &&
>>>>>>            !memcmp(p_tbl, &tbl, sizeof(tbl)))
>>>>>>                return IB_SUCCESS;
>>>>> Why exclude port 0 here ? Is it related to the change noted below ?
>>>>>
>>>>>> @@ -209,6 +209,7 @@ static int qos_extports_setup(osm_sm_t * sm, osm_node_t *node,
>>>>>>        unsigned num_ports = osm_node_get_num_physp(node);
>>>>>>        int ret = 0;
>>>>>>        unsigned i, j;
>>>>>> +       uint8_t op_vl1;
>>>>>>
>>>>>>        for (i = 1; i < num_ports; i++) {
>>>>>>                p = osm_node_get_physp_ptr(node, i);
>>>>>> @@ -225,17 +226,31 @@ static int qos_extports_setup(osm_sm_t * sm, osm_node_t *node,
>>>>>>        if (ib_switch_info_get_opt_sl2vlmapping(&node->sw->switch_info) &&
>>>>>>            sm->p_subn->opt.use_optimized_slvl) {
>>>>>>                p = osm_node_get_physp_ptr(node, 1);
>>>>>> +               op_vl1 = ib_port_info_get_op_vls(&p->port_info);
>>>>>>                force_update = p->need_update || sm->p_subn->need_update;
>>>>>> -               return sl2vl_update_table(sm, p, 1, 0x30000, force_update,
>>>>>> -                                         &qcfg->sl2vl);
>>>>>> +               if (sl2vl_update_table(sm, p, 0, 0x30000, force_update,
>>>>> Why is the third parameter (in_port) changed from 1 to 0 here ? Maybe
>>>>> that's related to the change above for the skipping of port 0 in
>>>>> sl2vl_update_table.
>>>>>
>>>>> -- Hal
>>>>>
>>>>>> +                                       &qcfg->sl2vl))
>>>>>> +                       ret = -1;
>>>>>> +               /* overwrite default ALL configuration if port's
>>>>>> +                  op_vl is different */
>>>>>> +               for (i = 2; i < num_ports; i++) {
>>>>>> +                       p = osm_node_get_physp_ptr(node, i);
>>>>>> +                       if (ib_port_info_get_op_vls(&p->port_info) != op_vl1 &&
>>>>>> +                           sl2vl_update_table(sm, p, 0, 0x20000 | i, force_update,
>>>>>> +                                               &qcfg->sl2vl))
>>>>>> +                               ret = -1;
>>>>>> +               }
>>>>>> +               return ret;
>>>>>>        }
>>>>>>
>>>>>> -       for (i = 0; i < num_ports; i++) {
>>>>>> +       /* non optimized sl2vl configuration */
>>>>>> +       i = ib_switch_info_is_enhanced_port0(&node->sw->switch_info) ? 0 : 1;
>>>>>> +       for (; i < num_ports; i++) {
>>>>>>                p = osm_node_get_physp_ptr(node, i);
>>>>>>                force_update = p->need_update || sm->p_subn->need_update;
>>>>>>                j = ib_switch_info_is_enhanced_port0(&node->sw->switch_info) ? 0 : 1;
>>>>>>                for (; j < num_ports; j++)
>>>>>> -                       if (sl2vl_update_table(sm, p, i, i << 8 | j,
>>>>>> +                       if (sl2vl_update_table(sm, p, j, j << 8 | i,
>>>>>>                                               force_update, &qcfg->sl2vl))
>>>>>>                                ret = -1;
>>>>>>        }
>>>>>> --
>>>>>> 1.5.5
>>>>>>
>>>>>> --
>>>>>> 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: Fix sl2vl configuration

[Hal Rosenstock]

Hi Eli,

On Thu, Aug 26, 2010 at 2:24 AM, Eli Dorfman (Voltaire)
<dorfman.eli-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> Hi Hal,
>
> Hal Rosenstock wrote:
>> On Wed, Aug 25, 2010 at 4:43 AM, Eli Dorfman (Voltaire)
>> <dorfman.eli-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
>>> Hal Rosenstock wrote:
>>>> Hi Eli,
>>>>
>>>> On Mon, Aug 23, 2010 at 3:37 PM, Eli Dorfman <dorfman.eli-Re5JQEeQqe8@public.gmane.orgm> wrote:
>>>>> Hi Hal,
>>>>>
>>>>> On Mon, Aug 23, 2010 at 5:25 PM, Hal Rosenstock
>>>>> <hal.rosenstock-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
>>>>>> On Wed, Jul 28, 2010 at 12:26 PM, Eli Dorfman (Voltaire)
>>>>>> <dorfman.eli-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
>>>>>>> Subject: [PATCH] Fix sl2vl configuration
>>>>>>>
>>>>>>> For non-optimized sl2vl configuration in and out ports were reversed.
>>>>>> Are you sure these are reversed ? Any idea which commit introduced
>>>>>> this reversal of in and out ports ?
>>>>> I'm sure they are reversed.
>>>> I looked at it some more and the ports look reversed to me too.
>>>>
>>>>> This patch was also tested by Jim Schutt.
>>>> That only means it works in his environment rather than "correctness".
>>> It was tested in mine enviornment as well.
>>> Usually I test the patch in my environment to verify its correctness (in addition to code review).
>>> I assume you do the same.
>>>
>>> Do you expect me to test the patch in your environment as well?
>>
>> Huh ?
>>
>>>>> I didn't check which commit introduced this - why is it important?
>>>> I'd like to understand which patch introduced the reversal. I don't
>>>> see it but might have missed it.  I think it's important to know which
>>>> versions are broken.
>>> I think that the following commit added the bug:
>>>
>>> commit 051a1dd514e63f3a971afad38e377932efca5e18
>>> Author: Sasha Khapyorsky <sashak-smomgflXvOZWk0Htik3J/w@public.gmane.org>
>>> Date:   Mon Jan 4 21:06:19 2010 +0200
>>>
>>>    opensm/osm_qos.c: split switch external and end ports setup
>>>
>>>    This splits QoS related port parameters setup over switch external ports
>>>    and end ports. Such separation leaves us with simpler code and saves
>>>    some repeated flows in case of switch external ports (actually required
>>>    per switch and not per port).
>>>
>>>    Another advantages: Optimized Sl2VL Mapping procedure can be implemented
>>>    easier using this model. A low level port QoS related parameters setup
>>>    infrastructure is ready for supporting per port QoS related configuration
>>>    (which hopefully will be implemented some days).
>>>
>>
>> I thought it might be that one but wasn't sure. Thanks.
>>
>>>>>>> For optimal sl2vl added override of default ALL settting with port's
>>>>>>> sl2vl when operational VL was other than the default port.
>>>>>> What is the motivation to override when the operational VLs is
>>>>>> different ? Why is that better than what is done currently ?
>>>>> The idea was to apply the default policy - set sl2vl modulo operational VL.
>>>>> When applying ALL settings using port 1 we still want to override this
>>>>> setting for ports with different operational VL.
>>>> What makes the default policy modulo operational VLs ?
>>> This is how its implemented in sl2vl_update_table()
>>>        vl_mask = (1 << (ib_port_info_get_op_vls(&p->port_info) - 1)) - 1;
>>>
>>>        for (i = 0; i < IB_MAX_NUM_VLS / 2; i++) {
>>>                vl1 = sl2vl_table->raw_vl_by_sl[i] >> 4;
>>>                vl2 = sl2vl_table->raw_vl_by_sl[i] & 0xf;
>>>                if (vl1 != 15)
>>>                        vl1 &= vl_mask;
>>>                if (vl2 != 15)
>>>                        vl2 &= vl_mask;
>>>
>>> Default startup switches configuration uses the same policy.
>>
>> I view this as further modification on the configuration based on the
>> operational VLs. Besides, any VL specified above the op VLs is a drop
>> (same as indicating VL15).
>
> This code is already in the trunk and my patch does not change that.
> The only diff in the patch is the fix of reversed in/out ports and corresponding fix for
> optimized sl2vl configuration.
>
>>
>> I think what you are doing is changing the default behavior and hence
>> perhaps an additional policy switch is needed if this change really is
>> needed and I'm unsure of that.
>
> I don't change the default behavior and this change is needed.
> Without it for example, if you connect a switch with opVL=4 (VL0-VL7) to a node that
> supports only VL0 and try to send traffic on sl>SL0 it will be dropped.

Yes, you are right. I stand corrected.

-- Hal

> Eli
>
>>
>>>>>> Is this really a policy issue ?
>>>>>>
>>>>>> IMO there are two separate issues in this patch and they should be in
>>>>>> separate patches (for better git bisection).
>>>>> Maybe, but I still think this patch fixes wrong setting for sl2vl
>>>>> using optimized and non optimized methods.
>>>>> I'm not sure this should be divided to 2 separate patches.
>>>> It's one thought per patch and to me this is two different thoughts.
>>> If this is the only issue, I can split this patch to 2 separate patches.
>>
>> It's also the issue noted above.
>>
>> -- Hal
>>
>>> Eli
>>>>>> Also, a couple of (possibly related) questions below.
>>>>> It seems that you are referring to patch v1 which was modified
>>>>> according to Jim's comments.
>>>>> Please check the v2 patch .
>>>> I see my questions are moot in terms of the v2 patch.
>>>>
>>>> -- Hal
>>>>
>>>>> Thanks,
>>>>> Eli
>>>>>
>>>>>>> Signed-off-by: Eli Dorfman <elid-smomgflXvOZWk0Htik3J/w@public.gmane.org>
>>>>>>> ---
>>>>>>>  opensm/opensm/osm_qos.c |   25 ++++++++++++++++++++-----
>>>>>>>  1 files changed, 20 insertions(+), 5 deletions(-)
>>>>>>>
>>>>>>> diff --git a/opensm/opensm/osm_qos.c b/opensm/opensm/osm_qos.c
>>>>>>> index a571370..de0ae23 100644
>>>>>>> --- a/opensm/opensm/osm_qos.c
>>>>>>> +++ b/opensm/opensm/osm_qos.c
>>>>>>> @@ -182,7 +182,7 @@ static ib_api_status_t sl2vl_update_table(osm_sm_t * sm, osm_physp_t * p,
>>>>>>>                tbl.raw_vl_by_sl[i] = (vl1 << 4) | vl2;
>>>>>>>        }
>>>>>>>
>>>>>>> -       if (!force_update && (p_tbl = osm_physp_get_slvl_tbl(p, in_port)) &&
>>>>>>> +       if (!force_update && in_port && (p_tbl = osm_physp_get_slvl_tbl(p, in_port)) &&
>>>>>>>            !memcmp(p_tbl, &tbl, sizeof(tbl)))
>>>>>>>                return IB_SUCCESS;
>>>>>> Why exclude port 0 here ? Is it related to the change noted below ?
>>>>>>
>>>>>>> @@ -209,6 +209,7 @@ static int qos_extports_setup(osm_sm_t * sm, osm_node_t *node,
>>>>>>>        unsigned num_ports = osm_node_get_num_physp(node);
>>>>>>>        int ret = 0;
>>>>>>>        unsigned i, j;
>>>>>>> +       uint8_t op_vl1;
>>>>>>>
>>>>>>>        for (i = 1; i < num_ports; i++) {
>>>>>>>                p = osm_node_get_physp_ptr(node, i);
>>>>>>> @@ -225,17 +226,31 @@ static int qos_extports_setup(osm_sm_t * sm, osm_node_t *node,
>>>>>>>        if (ib_switch_info_get_opt_sl2vlmapping(&node->sw->switch_info) &&
>>>>>>>            sm->p_subn->opt.use_optimized_slvl) {
>>>>>>>                p = osm_node_get_physp_ptr(node, 1);
>>>>>>> +               op_vl1 = ib_port_info_get_op_vls(&p->port_info);
>>>>>>>                force_update = p->need_update || sm->p_subn->need_update;
>>>>>>> -               return sl2vl_update_table(sm, p, 1, 0x30000, force_update,
>>>>>>> -                                         &qcfg->sl2vl);
>>>>>>> +               if (sl2vl_update_table(sm, p, 0, 0x30000, force_update,
>>>>>> Why is the third parameter (in_port) changed from 1 to 0 here ? Maybe
>>>>>> that's related to the change above for the skipping of port 0 in
>>>>>> sl2vl_update_table.
>>>>>>
>>>>>> -- Hal
>>>>>>
>>>>>>> +                                       &qcfg->sl2vl))
>>>>>>> +                       ret = -1;
>>>>>>> +               /* overwrite default ALL configuration if port's
>>>>>>> +                  op_vl is different */
>>>>>>> +               for (i = 2; i < num_ports; i++) {
>>>>>>> +                       p = osm_node_get_physp_ptr(node, i);
>>>>>>> +                       if (ib_port_info_get_op_vls(&p->port_info) != op_vl1 &&
>>>>>>> +                           sl2vl_update_table(sm, p, 0, 0x20000 | i, force_update,
>>>>>>> +                                               &qcfg->sl2vl))
>>>>>>> +                               ret = -1;
>>>>>>> +               }
>>>>>>> +               return ret;
>>>>>>>        }
>>>>>>>
>>>>>>> -       for (i = 0; i < num_ports; i++) {
>>>>>>> +       /* non optimized sl2vl configuration */
>>>>>>> +       i = ib_switch_info_is_enhanced_port0(&node->sw->switch_info) ? 0 : 1;
>>>>>>> +       for (; i < num_ports; i++) {
>>>>>>>                p = osm_node_get_physp_ptr(node, i);
>>>>>>>                force_update = p->need_update || sm->p_subn->need_update;
>>>>>>>                j = ib_switch_info_is_enhanced_port0(&node->sw->switch_info) ? 0 : 1;
>>>>>>>                for (; j < num_ports; j++)
>>>>>>> -                       if (sl2vl_update_table(sm, p, i, i << 8 | j,
>>>>>>> +                       if (sl2vl_update_table(sm, p, j, j << 8 | i,
>>>>>>>                                               force_update, &qcfg->sl2vl))
>>>>>>>                                ret = -1;
>>>>>>>        }
>>>>>>> --
>>>>>>> 1.5.5
>>>>>>>
>>>>>>> --
>>>>>>> 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 v2] opensm: Fix sl2vl configuration

[Sasha Khapyorsky]

On 12:36 Sun 01 Aug     , Eli Dorfman (Voltaire) wrote:
> Fix the patch according to Jim's comments
> Changed i,j to in,out to make code readable
> 
> For non-optimal sl2vl configuration in and out ports were reversed.
> For optimal sl2vl added override of default ALL settting with port's
> sl2vl when operational VL was other than the default port.
> 
> Signed-off-by: Eli Dorfman <elid-smomgflXvOZWk0Htik3J/w@public.gmane.org>

Applied. Thanks.

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