opensm/osm_sa_path_record.c: livelock in pr_rcv_get_path_parms

PATCH Applied Line Holen 4 participants 8 messages Apr 19, 2010 – Apr 21, 2010 3 messages not in the archive InfiniBand

Share & tools

Line Holen

author

2010-04-19 09:15 UTC #0 lore raw

PATCH 1/1 opensm/osm_sa_path_record.c: livelock in pr_rcv_get_path_parms

SA path request handling can end up in a livelock in pr_rcv_get_path_parms(). This can happen if a path request is handled while LFT updates to the fabric are in progress.
The LFT of the switch data structure is updated as part of the LFT response processing. So while the SM is busy pushing the LFT updates, some switches have up to date LFT info while others are not yet updated and contains the LFT of the previous routing. For a (short) time interval there is a potential for loops in the fabric. The livelock occurs if a path request is received during this time interval.
Both LFT response handling and path request processing needs the SM lock. When the livelock occurs the LFT response handling blocks forever waiting for the lock to be released.

The suggested fix is simply to introduce a max number of hops that should be traversed while handling the path request. If this max is reached then the request will return with NO_RECORD response and release the SM lock. This way the LFT processing will be able to complete.

Signed-off-by Line Holen <[email protected]>

---

opensm/opensm/osm_sa_path_record.c +24 -0

index c4c3f86..b399b70 100644
@@ -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 HNR Consulting. All rights reserved.
+ * Copyright (c) 2010 Sun Microsystems, Inc. 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

@@ -69,6 +70,9 @@
 #include <opensm/osm_prefix_route.h>
 #include <opensm/osm_ucast_lash.h>
 
+
+#define MAX_HOPS 128
+
 typedef struct osm_pr_item {
 	cl_list_item_t list_item;
 	ib_path_rec_t path_rec;

@@ -178,6 +182,7 @@ static ib_api_status_t pr_rcv_get_path_parms(IN osm_sa_t * sa,
 	osm_qos_level_t *p_qos_level = NULL;
 	uint16_t valid_sl_mask = 0xffff;
 	int is_lash;
+	int hops = 0;
 
 	OSM_LOG_ENTER(sa->p_log);

@@ -369,6 +374,25 @@ static ib_api_status_t pr_rcv_get_path_parms(IN osm_sa_t * sa,
 				goto Exit;
 			}
 		}
+
+		/* update number of hops traversed */
+		hops++;
+		if (hops > MAX_HOPS) {
+
+			OSM_LOG(sa->p_log, OSM_LOG_ERROR,
+			    "Path from GUID 0x%016" PRIx64 " (%s) to lid %u GUID 0x%016"
+			    PRIx64 " (%s) needs more than %d hops, "
+			    "max %d hops allowed\n",
+			    cl_ntoh64(osm_physp_get_port_guid(p_src_physp)),
+			    p_src_physp->p_node->print_desc,
+			    dest_lid_ho,
+			    cl_ntoh64(osm_physp_get_port_guid(p_dest_physp)),
+			    p_dest_physp->p_node->print_desc,
+			    hops, MAX_HOPS);
+
+			status = IB_NOT_FOUND;
+			goto Exit;
+		}
 	}
 
 	/*

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

A message that is not in the archive (sent off-list or not yet received): 4BCC1F3F.5080000-UdXhSnd/[email protected]

Sasha Khapyorsky

Applied

2010-04-19 15:34 UTC #2 lore raw

Quoted 13 lines · Line Holen

SA path request handling can end up in a livelock in pr_rcv_get_path_parms(). This can happen if a path request is handled while LFT updates to the fabric are in progress.
The LFT of the switch data structure is updated as part of the LFT response processing. So while the SM is busy pushing the LFT updates, some switches have up to date LFT info while others are not yet updated and contains the LFT of the previous routing. For a (short) time interval there is a potential for loops in the fabric. The livelock occurs if a path request is received during this time interval.
Both LFT response handling and path request processing needs the SM lock. When the livelock occurs the LFT response handling blocks forever waiting for the lock to be released.

Quoted 6 lines

The suggested fix is simply to introduce a max number of hops that should be traversed while handling the path request. If this max is reached then the request will return with NO_RECORD response and release the SM lock. This way the LFT processing will be able to complete.
Signed-off-by Line Holen <[email protected]>

Applied. Thanks. See minor question/note below.

Quoted 16 lines

---

opensm/opensm/osm_sa_path_record.c +1 -0

index c4c3f86..b399b70 100644
@@ -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 HNR Consulting. All rights reserved.
+ * Copyright (c) 2010 Sun Microsystems, Inc. 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

opensm/opensm/osm_sa_path_record.c line 74

Quoted 5 lines

opensm/opensm/osm_sa_path_record.c +2 -0

@@ -69,6 +70,9 @@
 #include <opensm/osm_prefix_route.h>
 #include <opensm/osm_ucast_lash.h>
 
+
+#define MAX_HOPS 128

IB spec defines maximal number of hops for a fabric which is 64. Would it be netter to use this value here?

Sasha

Quoted 29 lines

opensm/opensm/osm_sa_path_record.c +14 -0

+
 typedef struct osm_pr_item {
 	cl_list_item_t list_item;
 	ib_path_rec_t path_rec;

@@ -178,6 +182,7 @@ static ib_api_status_t pr_rcv_get_path_parms(IN osm_sa_t * sa,
 	osm_qos_level_t *p_qos_level = NULL;
 	uint16_t valid_sl_mask = 0xffff;
 	int is_lash;
+	int hops = 0;
 
 	OSM_LOG_ENTER(sa->p_log);

@@ -369,6 +374,25 @@ static ib_api_status_t pr_rcv_get_path_parms(IN osm_sa_t * sa,
 				goto Exit;
 			}
 		}
+
+		/* update number of hops traversed */
+		hops++;
+		if (hops > MAX_HOPS) {
+
+			OSM_LOG(sa->p_log, OSM_LOG_ERROR,
+			    "Path from GUID 0x%016" PRIx64 " (%s) to lid %u GUID 0x%016"
+			    PRIx64 " (%s) needs more than %d hops, "
+			    "max %d hops allowed\n",
+			    cl_ntoh64(osm_physp_get_port_guid(p_src_physp)),
+			    p_src_physp->p_node->print_desc,
+			    dest_lid_ho,

opensm/opensm/osm_sa_path_record.c static ib_api_status_t pr_rcv_get_path_parms(IN osm_sa_t * sa, · line 398

Quoted 10 lines

opensm/opensm/osm_sa_path_record.c +7 -0

+			    cl_ntoh64(osm_physp_get_port_guid(p_dest_physp)),
+			    p_dest_physp->p_node->print_desc,
+			    hops, MAX_HOPS);
+
+			status = IB_NOT_FOUND;
+			goto Exit;
+		}
 	}
 
 	/*

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

Line Holen

2010-04-19 18:32 UTC #3 lore raw

Quoted 41 lines · Sasha Khapyorsky

Quoted 13 lines · Line Holen
SA path request handling can end up in a livelock in pr_rcv_get_path_parms(). This can happen if a path request is handled while LFT updates to the fabric are in progress.
The LFT of the switch data structure is updated as part of the LFT response processing. So while the SM is busy pushing the LFT updates, some switches have up to date LFT info while others are not yet updated and contains the LFT of the previous routing. For a (short) time interval there is a potential for loops in the fabric. The livelock occurs if a path request is received during this time interval.
Both LFT response handling and path request processing needs the SM lock. When the livelock occurs the LFT response handling blocks forever waiting for the lock to be released.
Quoted 6 lines
The suggested fix is simply to introduce a max number of hops that should be traversed while handling the path request. If this max is reached then the request will return with NO_RECORD response and release the SM lock. This way the LFT processing will be able to complete.
Signed-off-by Line Holen <[email protected]>
Applied. Thanks. See minor question/note below.
Quoted 18 lines
---
opensm/opensm/osm_sa_path_record.c +1 -0
index c4c3f86..b399b70 100644
@@ -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 HNR Consulting. All rights reserved.
+ * Copyright (c) 2010 Sun Microsystems, Inc. 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
@@ -69,6 +70,9 @@
 #include <opensm/osm_prefix_route.h>
 #include <opensm/osm_ucast_lash.h>
 

Quoted 7 lines

opensm/opensm/osm_sa_path_record.c line 74
Quoted 2 lines
opensm/opensm/osm_sa_path_record.c +2 -0
+
+#define MAX_HOPS 128
IB spec defines maximal number of hops for a fabric which is 64. Would it be netter to use this value here?
Sasha

The value of 128 was chosen as 2x max DR path allowing the SM to be in the middle of a fabric. But I have no problem lowering to 64.

Line

Quoted 38 lines

opensm/opensm/osm_sa_path_record.c +21 -0

+
 typedef struct osm_pr_item {
 	cl_list_item_t list_item;
 	ib_path_rec_t path_rec;

@@ -178,6 +182,7 @@ static ib_api_status_t pr_rcv_get_path_parms(IN osm_sa_t * sa,
 	osm_qos_level_t *p_qos_level = NULL;
 	uint16_t valid_sl_mask = 0xffff;
 	int is_lash;
+	int hops = 0;
 
 	OSM_LOG_ENTER(sa->p_log);

@@ -369,6 +374,25 @@ static ib_api_status_t pr_rcv_get_path_parms(IN osm_sa_t * sa,
 				goto Exit;
 			}
 		}
+
+		/* update number of hops traversed */
+		hops++;
+		if (hops > MAX_HOPS) {
+
+			OSM_LOG(sa->p_log, OSM_LOG_ERROR,
+			    "Path from GUID 0x%016" PRIx64 " (%s) to lid %u GUID 0x%016"
+			    PRIx64 " (%s) needs more than %d hops, "
+			    "max %d hops allowed\n",
+			    cl_ntoh64(osm_physp_get_port_guid(p_src_physp)),
+			    p_src_physp->p_node->print_desc,
+			    dest_lid_ho,
+			    cl_ntoh64(osm_physp_get_port_guid(p_dest_physp)),
+			    p_dest_physp->p_node->print_desc,
+			    hops, MAX_HOPS);
+
+			status = IB_NOT_FOUND;
+			goto Exit;
+		}
 	}

Quoted 2 lines

opensm/opensm/osm_sa_path_record.c static ib_api_status_t pr_rcv_get_path_parms(IN osm_sa_t * sa, · line 398
Quoted 2 lines
opensm/opensm/osm_sa_path_record.c +0 -0
 	/*

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

A message that is not in the archive (sent off-list or not yet received): 4BCCA1C5.5000904-UdXhSnd/[email protected]

Sasha Khapyorsky

2010-04-21 10:16 UTC #5 lore raw

Quoted 10 lines · Line Holen

Quoted 10 lines
opensm/opensm/osm_sa_path_record.c line 74
Quoted 6 lines
opensm/opensm/osm_sa_path_record.c +2 -0
@@ -69,6 +70,9 @@
 #include <opensm/osm_prefix_route.h>
 #include <opensm/osm_ucast_lash.h>
 
+
+#define MAX_HOPS 128
IB spec defines maximal number of hops for a fabric which is 64. Would it be netter to use this value here?

Quoted 4 lines

Quoted 1 line
Sasha
The value of 128 was chosen as 2x max DR path allowing the SM to be in the middle of a fabric. But I have no problem lowering to 64.

The path in this calculation is between ports and SM is not part of the game.

For me it seems that 64 would be better number. Hypothetically it could be even unrelated to LFTs transition issue - when path exceeds 64 hops SA can return NOT FOUND just well.

Sasha
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

Sasha Khapyorsky

2010-04-21 10:21 UTC #6 lore raw

In reply to a message not in the archive

Quoted 3 lines · Line Holen

The value of 128 was chosen as 2x max DR path allowing the SM to be in the middle of a fabric. But I have no problem lowering to 64.

Would you care about patch?

Sasha
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

Line Holen

author

2010-04-21 10:40 UTC #7 lore raw

Quoted 5 lines · Sasha Khapyorsky

Quoted 2 lines · Line Holen
The value of 128 was chosen as 2x max DR path allowing the SM to be in the middle of a fabric. But I have no problem lowering to 64.
Would you care about patch?

Sure, I can send a patch.

Line

Quoted 6 lines

Sasha
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to [email protected] 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 [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

Hal Rosenstock

2010-04-19 18:20 UTC #8 lore raw

In reply to a message not in the archive

Quoted 13 lines · Line Holen

SA path request handling can end up in a livelock in pr_rcv_get_path_parms(). This can happen if a path request is handled while LFT updates to the fabric are in progress.
The LFT of the switch data structure is updated as part of the LFT response processing. So while the SM is busy pushing the LFT updates, some switches have up to date LFT info while others are not yet updated and contains the LFT of the previous routing. For a (short) time interval there is a potential for loops in the fabric. The livelock occurs if a path request is received during this time interval.
Both LFT response handling and path request processing needs the SM lock. When the livelock occurs the LFT response handling blocks forever waiting for the lock to be released.

Quoted 3 lines

The suggested fix is simply to introduce a max number of hops that should be traversed while handling the path request. If this max is reached then the request will return with NO_RECORD response

To me, this begs the question of whether this should return a BUSY status rather than no record (and whether SA clients should handle those two differently) but that is a bigger change (and may require some end node change as well).

Also, should a similar change be made in SA MPR mpr_rcv_get_path_parms ?

-- Hal

Quoted 64 lines

and release the SM lock.
This way the LFT processing will be able to complete.

Signed-off-by Line Holen <[email protected]>

---

opensm/opensm/osm_sa_path_record.c +24 -1

index c4c3f86..b399b70 100644
@@ -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 HNR Consulting. All rights reserved.
+ * Copyright (c) 2010 Sun Microsystems, Inc. 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

@@ -69,6 +70,9 @@
  #include <opensm/osm_prefix_route.h>
  #include <opensm/osm_ucast_lash.h>
 
+
+#define MAX_HOPS 128
+
  typedef struct osm_pr_item {
        cl_list_item_t list_item;
        ib_path_rec_t path_rec;

@@ -178,6 +182,7 @@ static ib_api_status_t pr_rcv_get_path_parms(IN osm_sa_t * sa,
        osm_qos_level_t *p_qos_level = NULL;
        uint16_t valid_sl_mask = 0xffff;
        int is_lash;
+       int hops = 0;
 
        OSM_LOG_ENTER(sa->p_log);

@@ -369,6 +374,25 @@ static ib_api_status_t pr_rcv_get_path_parms(IN osm_sa_t * sa,
                                goto Exit;
                        }
                }
+
+               /* update number of hops traversed */
+               hops++;
+               if (hops > MAX_HOPS) {
+
+                       OSM_LOG(sa->p_log, OSM_LOG_ERROR,
+                           "Path from GUID 0x%016" PRIx64 " (%s) to lid %u GUID 0x%016"
+                           PRIx64 " (%s) needs more than %d hops, "
+                           "max %d hops allowed\n",
+                           cl_ntoh64(osm_physp_get_port_guid(p_src_physp)),
+                           p_src_physp->p_node->print_desc,
+                           dest_lid_ho,
+                           cl_ntoh64(osm_physp_get_port_guid(p_dest_physp)),
+                           p_dest_physp->p_node->print_desc,
+                           hops, MAX_HOPS);
+
+                       status = IB_NOT_FOUND;
+                       goto Exit;
+               }
        }
 
        /*
--

Quoted 4 lines

To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to [email protected] 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 [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

A message that is not in the archive (sent off-list or not yet received): j2uf0e08f231004191120oc1e78130l683b9ae0ca51003a-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org

Line Holen

author

2010-04-19 18:48 UTC #10 lore raw

Quoted 14 lines · Hal Rosenstock

Quoted 13 lines · Line Holen
SA path request handling can end up in a livelock in pr_rcv_get_path_parms(). This can happen if a path request is handled while LFT updates to the fabric are in progress.
The LFT of the switch data structure is updated as part of the LFT response processing. So while the SM is busy pushing the LFT updates, some switches have up to date LFT info while others are not yet updated and contains the LFT of the previous routing. For a (short) time interval there is a potential for loops in the fabric. The livelock occurs if a path request is received during this time interval.
Both LFT response handling and path request processing needs the SM lock. When the livelock occurs the LFT response handling blocks forever waiting for the lock to be released.

Quoted 8 lines

Quoted 3 lines
The suggested fix is simply to introduce a max number of hops that should be traversed while handling the path request. If this max is reached then the request will return with NO_RECORD response
To me, this begs the question of whether this should return a BUSY status rather than no record (and whether SA clients should handle those two differently) but that is a bigger change (and may require some end node change as well).

I think the fundamental issue here is that the path request handling is operating on inconsistent data - a mixture of old and new lft setup. A proper fix would be to use a consistent lft setup (either old or new) or deny service (return BUSY) while LFT updates are in progress. A check on number of hops still make sense though, because the routing could generate loops too.

Quoted 2 lines

Also, should a similar change be made in SA MPR mpr_rcv_get_path_parms ?

Could be. I haven't checked that code.

Line

Quoted 65 lines

-- Hal

Quoted 62 lines

and release the SM lock.
This way the LFT processing will be able to complete.

Signed-off-by Line Holen <[email protected]>

---

opensm/opensm/osm_sa_path_record.c +24 -0

index c4c3f86..b399b70 100644
@@ -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 HNR Consulting. All rights reserved.
+ * Copyright (c) 2010 Sun Microsystems, Inc. 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

@@ -69,6 +70,9 @@
 #include <opensm/osm_prefix_route.h>
 #include <opensm/osm_ucast_lash.h>
 
+
+#define MAX_HOPS 128
+
 typedef struct osm_pr_item {
       cl_list_item_t list_item;
       ib_path_rec_t path_rec;

@@ -178,6 +182,7 @@ static ib_api_status_t pr_rcv_get_path_parms(IN osm_sa_t * sa,
       osm_qos_level_t *p_qos_level = NULL;
       uint16_t valid_sl_mask = 0xffff;
       int is_lash;
+       int hops = 0;
 
       OSM_LOG_ENTER(sa->p_log);

@@ -369,6 +374,25 @@ static ib_api_status_t pr_rcv_get_path_parms(IN osm_sa_t * sa,
                               goto Exit;
                       }
               }
+
+               /* update number of hops traversed */
+               hops++;
+               if (hops > MAX_HOPS) {
+
+                       OSM_LOG(sa->p_log, OSM_LOG_ERROR,
+                           "Path from GUID 0x%016" PRIx64 " (%s) to lid %u GUID 0x%016"
+                           PRIx64 " (%s) needs more than %d hops, "
+                           "max %d hops allowed\n",
+                           cl_ntoh64(osm_physp_get_port_guid(p_src_physp)),
+                           p_src_physp->p_node->print_desc,
+                           dest_lid_ho,
+                           cl_ntoh64(osm_physp_get_port_guid(p_dest_physp)),
+                           p_dest_physp->p_node->print_desc,
+                           hops, MAX_HOPS);
+
+                       status = IB_NOT_FOUND;
+                       goto Exit;
+               }
       }

Quoted 10 lines

Quoted 6 lines
opensm/opensm/osm_sa_path_record.c +0 -1
       /*
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to [email protected] 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 [email protected] 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 [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html