[PATCH] opensm: perfmgr, only set orig_lid when we have a valid port. Otherwise leave it as 0

[PATCH] opensm: perfmgr, only set orig_lid when we have a valid port. Otherwise leave it as 0

[Ira Weiny]

Subject: [PATCH] opensm: perfmgr, only set orig_lid when we have a valid port.  Otherwise leave it as 0


Signed-off-by: Ira Weiny <weiny2-i2BcT+NCU+M@public.gmane.org>
---
 opensm/osm_perfmgr.c |    4 +++-
 1 files changed, 3 insertions(+), 1 deletions(-)

diff --git a/opensm/osm_perfmgr.c b/opensm/osm_perfmgr.c
index 6a1fa63..3e1575a 100644
--- a/opensm/osm_perfmgr.c
+++ b/opensm/osm_perfmgr.c
@@ -454,7 +454,9 @@ static void collect_guids(cl_map_item_t * p_map_item, void *context)
 				  ib_switch_info_is_enhanced_port0(&node->sw->
 								   switch_info));
 		for (port = mon_node->esp0 ? 0 : 1; port < num_ports; port++) {
-			mon_node->port[port].orig_lid = get_base_lid(node, port);
+			mon_node->port[port].orig_lid = 0;
+			if (osm_physp_is_valid(&node->physp_table[port]))
+				mon_node->port[port].orig_lid = get_base_lid(node, port);
 			mon_node->port[port].valid = TRUE;
 		}
 
-- 
1.7.1

--
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: perfmgr, only set orig_lid when we have a valid port. Otherwise leave it as 0

[Alex Netes]

Hi Ira,

On 15:17 Fri 15 Apr     , Ira Weiny wrote:
> 
> Subject: [PATCH] opensm: perfmgr, only set orig_lid when we have a valid port.  Otherwise leave it as 0
> 
> 
> Signed-off-by: Ira Weiny <weiny2-i2BcT+NCU+M@public.gmane.org>
> ---
>  opensm/osm_perfmgr.c |    4 +++-
>  1 files changed, 3 insertions(+), 1 deletions(-)
> 
> diff --git a/opensm/osm_perfmgr.c b/opensm/osm_perfmgr.c
> index 6a1fa63..3e1575a 100644
> --- a/opensm/osm_perfmgr.c
> +++ b/opensm/osm_perfmgr.c
> @@ -454,7 +454,9 @@ static void collect_guids(cl_map_item_t * p_map_item, void *context)
>  				  ib_switch_info_is_enhanced_port0(&node->sw->
>  								   switch_info));
>  		for (port = mon_node->esp0 ? 0 : 1; port < num_ports; port++) {
> -			mon_node->port[port].orig_lid = get_base_lid(node, port);
> +			mon_node->port[port].orig_lid = 0;
> +			if (osm_physp_is_valid(&node->physp_table[port]))
> +				mon_node->port[port].orig_lid = get_base_lid(node, port);
>  			mon_node->port[port].valid = TRUE;

Shouldn't this port marked with mon_node->port[port].valid = FALSE ?


-- Alex
--
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: perfmgr, only set orig_lid when we have a valid port. Otherwise leave it as 0

[Hal Rosenstock]

On 4/15/2011 6:17 PM, Ira Weiny wrote:
> 
> Subject: [PATCH] opensm: perfmgr, only set orig_lid when we have a valid port.  Otherwise leave it as 0
> 
> 
> Signed-off-by: Ira Weiny <weiny2-i2BcT+NCU+M@public.gmane.org>
> ---
>  opensm/osm_perfmgr.c |    4 +++-
>  1 files changed, 3 insertions(+), 1 deletions(-)
> 
> diff --git a/opensm/osm_perfmgr.c b/opensm/osm_perfmgr.c
> index 6a1fa63..3e1575a 100644
> --- a/opensm/osm_perfmgr.c
> +++ b/opensm/osm_perfmgr.c
> @@ -454,7 +454,9 @@ static void collect_guids(cl_map_item_t * p_map_item, void *context)
>  				  ib_switch_info_is_enhanced_port0(&node->sw->
>  								   switch_info));
>  		for (port = mon_node->esp0 ? 0 : 1; port < num_ports; port++) {
> -			mon_node->port[port].orig_lid = get_base_lid(node, port);
> +			mon_node->port[port].orig_lid = 0;
> +			if (osm_physp_is_valid(&node->physp_table[port]))
> +				mon_node->port[port].orig_lid = get_base_lid(node, port);
>  			mon_node->port[port].valid = TRUE;
>  		}
>  

There are several other calls to get_base_lid. Is it also an issue there
too or is port always valid there ? If it's not always valid for those
cases, why not just move this check into get_base_lid ?
--
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: perfmgr, only set orig_lid when we have a valid port. Otherwise leave it as 0

[Weiny, Ira K.]

On Apr 17, 2011, at 8:21 AM, Hal Rosenstock wrote:

> On 4/15/2011 6:17 PM, Ira Weiny wrote:
>> 
>> Subject: [PATCH] opensm: perfmgr, only set orig_lid when we have a valid port.  Otherwise leave it as 0
>> 
>> 
>> Signed-off-by: Ira Weiny <weiny2-i2BcT+NCU+M@public.gmane.org>
>> ---
>> opensm/osm_perfmgr.c |    4 +++-
>> 1 files changed, 3 insertions(+), 1 deletions(-)
>> 
>> diff --git a/opensm/osm_perfmgr.c b/opensm/osm_perfmgr.c
>> index 6a1fa63..3e1575a 100644
>> --- a/opensm/osm_perfmgr.c
>> +++ b/opensm/osm_perfmgr.c
>> @@ -454,7 +454,9 @@ static void collect_guids(cl_map_item_t * p_map_item, void *context)
>> 				  ib_switch_info_is_enhanced_port0(&node->sw->
>> 								   switch_info));
>> 		for (port = mon_node->esp0 ? 0 : 1; port < num_ports; port++) {
>> -			mon_node->port[port].orig_lid = get_base_lid(node, port);
>> +			mon_node->port[port].orig_lid = 0;
>> +			if (osm_physp_is_valid(&node->physp_table[port]))
>> +				mon_node->port[port].orig_lid = get_base_lid(node, port);
>> 			mon_node->port[port].valid = TRUE;
>> 		}
>> 
> 
> There are several other calls to get_base_lid. Is it also an issue there
> too or is port always valid there ? If it's not always valid for those
> cases, why not just move this check into get_base_lid ?

I believe this is a special case because we are looping through the ports of a CA node and not all of it's ports are valid on this fabric.  Most of the other places where get_base_lid is called I believe the port is known to be good, therefore there is an assertion in there just to protect us.

I think the better way to "fix" this would probably be to change the perfmgr to track ports rather than nodes.[*]  Unfortunately this will take a significant amount of coding effort.  I think the best thing to do is just check if the physical port is valid as I did above.

As to Alex's comment I think the port should be marked invalid as well.  (I should have CC'ed the list on my response to him.)  I will resend the patch with that change.

Ira

[*] At the time I coded the perfmgr it seemed to make more sense to track nodes rather than ports.  In hindsight this might have been a mistake but for this small place I don't see a reason to re-architect it all yet.

> --
> 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: perfmgr, only set orig_lid when we have a valid port. Otherwise leave it as 0

[Weiny, Ira K.]

On Apr 17, 2011, at 8:06 AM, Alex Netes wrote:

> Hi Ira,
> 
> On 15:17 Fri 15 Apr     , Ira Weiny wrote:
>> 
>> Subject: [PATCH] opensm: perfmgr, only set orig_lid when we have a valid port.  Otherwise leave it as 0
>> 
>> 
>> Signed-off-by: Ira Weiny <weiny2-i2BcT+NCU+M@public.gmane.org>
>> ---
>> opensm/osm_perfmgr.c |    4 +++-
>> 1 files changed, 3 insertions(+), 1 deletions(-)
>> 
>> diff --git a/opensm/osm_perfmgr.c b/opensm/osm_perfmgr.c
>> index 6a1fa63..3e1575a 100644
>> --- a/opensm/osm_perfmgr.c
>> +++ b/opensm/osm_perfmgr.c
>> @@ -454,7 +454,9 @@ static void collect_guids(cl_map_item_t * p_map_item, void *context)
>> 				  ib_switch_info_is_enhanced_port0(&node->sw->
>> 								   switch_info));
>> 		for (port = mon_node->esp0 ? 0 : 1; port < num_ports; port++) {
>> -			mon_node->port[port].orig_lid = get_base_lid(node, port);
>> +			mon_node->port[port].orig_lid = 0;
>> +			if (osm_physp_is_valid(&node->physp_table[port]))
>> +				mon_node->port[port].orig_lid = get_base_lid(node, port);
>> 			mon_node->port[port].valid = TRUE;
> 
> Shouldn't this port marked with mon_node->port[port].valid = FALSE ?

New patch below.


Subject: [PATCH 1/5] opensm/perfmgr: set redirect orig_lid and valid flag only when we have a valid port.

Signed-off-by: Ira Weiny <weiny2-i2BcT+NCU+M@public.gmane.org>
---
 opensm/osm_perfmgr.c |    8 ++++++--
 1 files changed, 6 insertions(+), 2 deletions(-)

diff --git a/opensm/osm_perfmgr.c b/opensm/osm_perfmgr.c
index 5758587..3578e13 100644
--- a/opensm/osm_perfmgr.c
+++ b/opensm/osm_perfmgr.c
@@ -454,8 +454,12 @@ static void collect_guids(cl_map_item_t * p_map_item, void *context)
 				  ib_switch_info_is_enhanced_port0(&node->sw->
 								   switch_info));
 		for (port = mon_node->esp0 ? 0 : 1; port < num_ports; port++) {
-			mon_node->port[port].orig_lid = get_base_lid(node, port);
-			mon_node->port[port].valid = TRUE;
+			mon_node->port[port].orig_lid = 0;
+			mon_node->port[port].valid = FALSE;
+			if (osm_physp_is_valid(&node->physp_table[port])) {
+				mon_node->port[port].orig_lid = get_base_lid(node, port);
+				mon_node->port[port].valid = TRUE;
+			}
 		}
 
 		cl_qmap_insert(&pm->monitored_map, node_guid,
-- 
1.7.1


--
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: perfmgr, only set orig_lid when we have a valid port. Otherwise leave it as 0

[Hal Rosenstock]

On 4/17/2011 9:26 PM, Weiny, Ira K. wrote:
> On Apr 17, 2011, at 8:21 AM, Hal Rosenstock wrote:
> 
>> On 4/15/2011 6:17 PM, Ira Weiny wrote:
>>>
>>> Subject: [PATCH] opensm: perfmgr, only set orig_lid when we have a valid port.  Otherwise leave it as 0
>>>
>>>
>>> Signed-off-by: Ira Weiny <weiny2-i2BcT+NCU+M@public.gmane.org>
>>> ---
>>> opensm/osm_perfmgr.c |    4 +++-
>>> 1 files changed, 3 insertions(+), 1 deletions(-)
>>>
>>> diff --git a/opensm/osm_perfmgr.c b/opensm/osm_perfmgr.c
>>> index 6a1fa63..3e1575a 100644
>>> --- a/opensm/osm_perfmgr.c
>>> +++ b/opensm/osm_perfmgr.c
>>> @@ -454,7 +454,9 @@ static void collect_guids(cl_map_item_t * p_map_item, void *context)
>>> 				  ib_switch_info_is_enhanced_port0(&node->sw->
>>> 								   switch_info));
>>> 		for (port = mon_node->esp0 ? 0 : 1; port < num_ports; port++) {
>>> -			mon_node->port[port].orig_lid = get_base_lid(node, port);
>>> +			mon_node->port[port].orig_lid = 0;
>>> +			if (osm_physp_is_valid(&node->physp_table[port]))
>>> +				mon_node->port[port].orig_lid = get_base_lid(node, port);
>>> 			mon_node->port[port].valid = TRUE;
>>> 		}
>>>
>>
>> There are several other calls to get_base_lid. Is it also an issue there
>> too or is port always valid there ? If it's not always valid for those
>> cases, why not just move this check into get_base_lid ?
> 
> I believe this is a special case because we are looping through the ports of a CA node and not all of it's ports are valid on this fabric.  Most of the other places where get_base_lid is called I believe the port is known to be good, therefore there is an assertion in there just to protect us.
> 
> I think the better way to "fix" this would probably be to change the perfmgr to track ports rather than nodes.[*]

Why would tracking ports be better than tracking nodes ? Just to
eliminate this issue ?

> Unfortunately this will take a significant amount of coding effort.  I think the best thing to do is just check if the physical port is valid as I did above.
> 
> As to Alex's comment I think the port should be marked invalid as well.  (I should have CC'ed the list on my response to him.)  I will resend the patch with that change.
> 
> Ira
> 
> [*] At the time I coded the perfmgr it seemed to make more sense to track nodes rather than ports.  
> In hindsight this might have been a mistake but for this small place I don't see a reason to re-architect it all yet.

I'm not following what you're thinking here other than this small issue.
Is there something more behind this ?

-- Hal

--
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: perfmgr, only set orig_lid when we have a valid port. Otherwise leave it as 0

[Ira Weiny]

On Mon, 18 Apr 2011 05:42:56 -0700
Hal Rosenstock <hal-LDSdmyG8hGV8YrgS2mwiifqBs+8SCbDb@public.gmane.org> wrote:

> On 4/17/2011 9:26 PM, Weiny, Ira K. wrote:
> > On Apr 17, 2011, at 8:21 AM, Hal Rosenstock wrote:
> > 
> >> On 4/15/2011 6:17 PM, Ira Weiny wrote:
> >>>

[snip]

> >>
> >> There are several other calls to get_base_lid. Is it also an issue there
> >> too or is port always valid there ? If it's not always valid for those
> >> cases, why not just move this check into get_base_lid ?
> > 
> > I believe this is a special case because we are looping through the ports of a CA node and not all of it's ports are valid on this fabric.  Most of the other places where get_base_lid is called I believe the port is known to be good, therefore there is an assertion in there just to protect us.
> > 
> > I think the better way to "fix" this would probably be to change the perfmgr to track ports rather than nodes.[*]
> 
> Why would tracking ports be better than tracking nodes ? Just to
> eliminate this issue ?
> 
> > Unfortunately this will take a significant amount of coding effort.  I think the best thing to do is just check if the physical port is valid as I did above.
> > 
> > As to Alex's comment I think the port should be marked invalid as well.  (I should have CC'ed the list on my response to him.)  I will resend the patch with that change.
> > 
> > Ira
> > 
> > [*] At the time I coded the perfmgr it seemed to make more sense to track nodes rather than ports.  
> > In hindsight this might have been a mistake but for this small place I don't see a reason to re-architect it all yet.
> 
> I'm not following what you're thinking here other than this small issue.
> Is there something more behind this ?

I agree that it would not be worth the effort to fix this minor issue.  My thoughts were: we are tracking values which are provided by ports not nodes.  (They are PortCounters after all.)  So in some ways it makes sense to keep track of all the ports in the system.

However, I did think about it more after my email and tracking ports may have other disadvantages.  One example I could think of was that the perfmgr keeps it's own list of monitored nodes.  This list would be much larger if we were tracking ports.  So finding the "monitored port" would be slower when processing response mads.  Would that be a big deal?  Perhaps not.  I would have to do some performance tests.

In the end I just wanted to acknowledge that there might be another way.

:-D

Ira

> 
> -- Hal
> 
> --
> 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


-- 
Ira Weiny
Math Programmer/Computer Scientist
Lawrence Livermore National Lab
925-423-8008
weiny2-i2BcT+NCU+M@public.gmane.org
--
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