opensm: Reduce heap consumption by unicast routing tables (LFTs)

PATCH Sasha Khapyorsky 2 participants 3 messages Oct 12, 2009 – Oct 13, 2009 3 messages not in the archive InfiniBand

lore

Share & tools

lore

Sasha Khapyorsky

author

2009-10-12 15:46 UTC #2 lore raw

In reply to a message not in the archive

Hi Hal,

Quoted 56 lines · Hal Rosenstock

Heap memory consumption by the unicast and multicast routing tables can be reduced.
Using valgrind --tool=massif (for heap profiling), there are couple of places that consume most of the heap memory:
->38.75% (11,206,656B) 0x43267E: osm_switch_new (osm_switch.c:134)
->12.89% (3,728,256B) 0x40F8C9: osm_mcast_tbl_init (osm_mcast_tbl.c:96)
osm_switch_new (osm_switch.c:108):
       p_sw->lft = malloc(IB_LID_UCAST_END_HO + 1);
From ib_types.h
#define IB_LID_UCAST_END_HO 0xBFFF
The LFT can be allocated after LID assignment, once the number of LIDs assigned is known.
So it looks like for cluster of 2-4K with an LMC of 0 about 40% (!!!) of the heap memory can be saved:
- 39% used by LFTs, each with 48K entries - SM can allocate 4K entries instead.
A similar subsequent change will do this for MFTs.
Signed-off-by Hal Rosenstock <[email protected]>
---
Changes since v2:
Fixed alloc_lft handling of an existing lft Moved alloc_lft into osm_switch_prepare_path_rebuild
Changes since v1:
Basic approach changed to not do in chunks but rather allocate LFT once LID assignment is completed
opensm/include/opensm/osm_switch.h +2 -1
index 502e5a7..d881576 100644
@@ -102,6 +102,7 @@ typedef struct osm_switch {
 	osm_port_profile_t *p_prof;
 	uint8_t *lft;
 	uint8_t *new_lft;
+	uint16_t lft_size;
 	osm_mcast_tbl_t mcast_tbl;
 	int32_t mft_block_num;
 	uint32_t mft_position;
@@ -253,7 +254,7 @@ static inline uint8_t osm_switch_get_hop_count(IN const osm_switch_t * p_sw,
 					       IN uint16_t lid_ho,
 					       IN uint8_t port_num)
 {
-	return (lid_ho > p_sw->max_lid_ho || !p_sw->hops[lid_ho]) ?
+	return (lid_ho >= p_sw->lft_size || !p_sw->hops[lid_ho]) ?
 	    OSM_NO_PATH : p_sw->hops[lid_ho][port_num];
 }
 /*

opensm/include/opensm/osm_switch.h void osm_switch_clear_hops(IN osm_switch_t * p_sw); · line 345

Quoted 5 lines

opensm/include/opensm/osm_switch.h +1 -1

@@ -341,7 +342,7 @@ void osm_switch_clear_hops(IN osm_switch_t * p_sw);
 static inline uint8_t osm_switch_get_least_hops(IN const osm_switch_t * p_sw,
 						IN uint16_t lid_ho)
 {
-	return (lid_ho > p_sw->max_lid_ho || !p_sw->hops[lid_ho]) ?
+	return (lid_ho >= p_sw->lft_size || !p_sw->hops[lid_ho]) ?

Now LFT buffer is only increased in size, so in cases then actual number of lids was decreased (and p_sw->max_lid_ho + 1 < p_sw->lft_size) such changed check may return garbage data from previous cycles. I think the check:

	lid_ho > p_sw->max_lid_ho

should be preserved rather than:

	lid_ho >= p_sw->lft_size

opensm/include/opensm/osm_switch.h uint8_t osm_switch_get_port_least_hops(IN const osm_switch_t * p_sw, · line 409

Quoted 9 lines

opensm/include/opensm/osm_switch.h +1 -1

 	    OSM_NO_PATH : p_sw->hops[lid_ho][0];
 }
 /*

@@ -405,7 +406,7 @@ uint8_t osm_switch_get_port_least_hops(IN const osm_switch_t * p_sw,
 static inline uint8_t osm_switch_get_port_by_lid(IN const osm_switch_t * p_sw,
 						 IN uint16_t lid_ho)
 {
-	if (lid_ho == 0 || lid_ho > IB_LID_UCAST_END_HO)
+	if (lid_ho == 0 || lid_ho >= p_sw->lft_size)

Similar story? Shouldn't this be checked against p_sw->max_lid_ho?

Quoted 62 lines

opensm/include/opensm/osm_switch.h +1 -1

 		return OSM_NO_PATH;
 	return p_sw->lft[lid_ho];
 }

@@ -711,7 +712,7 @@ osm_switch_set_lft_block(IN osm_switch_t * p_sw, IN const uint8_t * p_block,
 		(uint16_t) (block_num * IB_SMP_DATA_SIZE);
 	CL_ASSERT(p_sw);
 
-	if (lid_start + IB_SMP_DATA_SIZE > IB_LID_UCAST_END_HO)
+	if (lid_start + IB_SMP_DATA_SIZE > p_sw->lft_size)
 		return IB_INVALID_PARAMETER;
 
 	memcpy(&p_sw->lft[lid_start], p_block, IB_SMP_DATA_SIZE);

opensm/opensm/osm_state_mgr.c +3 -3

index 185c700..c40c9d3 100644
@@ -1,6 +1,6 @@
 /*
  * Copyright (c) 2004-2008 Voltaire, Inc. All rights reserved.
- * Copyright (c) 2002-2008 Mellanox Technologies LTD. All rights reserved.
+ * Copyright (c) 2002-2009 Mellanox Technologies LTD. All rights reserved.
  * Copyright (c) 1996-2003 Intel Corporation. All rights reserved.
  * Copyright (c) 2009 HNR Consulting. All rights reserved.
  *

@@ -1011,9 +1011,9 @@ static void cleanup_switch(cl_map_item_t * item, void *log)
 	if (!sw->new_lft)
 		return;
 
-	if (memcmp(sw->lft, sw->new_lft, IB_LID_UCAST_END_HO + 1))
+	if (memcmp(sw->lft, sw->new_lft, sw->lft_size))
 		osm_log(log, OSM_LOG_ERROR, "ERR 331D: "
-			"LFT of switch 0x%016" PRIx64 " is not up to date.\n",
+			"LFT of switch 0x%016" PRIx64 " is not up to date\n",
 			cl_ntoh64(sw->p_node->node_info.node_guid));
 	else {
 		free(sw->new_lft);

opensm/opensm/osm_switch.c +1 -7

index ad1018f..7d51c05 100644
@@ -1,6 +1,6 @@
 /*
  * Copyright (c) 2004-2008 Voltaire, Inc. All rights reserved.
- * Copyright (c) 2002-2008 Mellanox Technologies LTD. All rights reserved.
+ * Copyright (c) 2002-2009 Mellanox Technologies LTD. All rights reserved.
  * Copyright (c) 1996-2003 Intel Corporation. All rights reserved.
  * Copyright (c) 2009 HNR Consulting. All rights reserved.
  *

@@ -130,12 +130,6 @@ osm_switch_t *osm_switch_new(IN osm_node_t * p_node,
 	p_sw->num_ports = num_ports;
 	p_sw->need_update = 2;
 
-	p_sw->lft = malloc(IB_LID_UCAST_END_HO + 1);
-	if (!p_sw->lft)
-		goto err;
-
-	memset(p_sw->lft, OSM_NO_PATH, IB_LID_UCAST_END_HO + 1);
-
 	p_sw->p_prof = malloc(sizeof(*p_sw->p_prof) * num_ports);
 	if (!p_sw->p_prof)
 		goto err;

opensm/opensm/osm_switch.c boolean_t osm_switch_get_lft_block(IN const osm_switch_t * p_sw, · line 163

Quoted 5 lines

opensm/opensm/osm_switch.c +1 -1

@@ -166,7 +160,7 @@ boolean_t osm_switch_get_lft_block(IN const osm_switch_t * p_sw,
 	CL_ASSERT(p_sw);
 	CL_ASSERT(p_block);
 
-	if (base_lid_ho > p_sw->max_lid_ho)
+	if (base_lid_ho >= p_sw->lft_size)

Ditto.

Quoted 6 lines

opensm/opensm/osm_switch.c +0 -0

 		return FALSE;
 
 	CL_ASSERT(base_lid_ho + IB_SMP_DATA_SIZE <= IB_LID_UCAST_END_HO);

@@ -498,21 +492,54 @@ void osm_switch_clear_hops(IN osm_switch_t * p_sw)
 
 /**********************************************************************

opensm/opensm/osm_switch.c void osm_switch_clear_hops(IN osm_switch_t * p_sw) · line 503

Quoted 10 lines

opensm/opensm/osm_switch.c +9 -0

  **********************************************************************/
+static int alloc_lft(IN osm_switch_t * p_sw, uint16_t lids)
+{
+	uint16_t lft_size;
+	uint8_t *new_lft;
+
+	lft_size = lids;
+	/* Ensure LFT is in units of LFT block size */
+	if (lids % IB_SMP_DATA_SIZE)
+		lft_size = (lids + IB_SMP_DATA_SIZE) / IB_SMP_DATA_SIZE * IB_SMP_DATA_SIZE;

Could be slightly faster:

	lft_size = (lids + IB_SMP_DATA_SIZE - 1) / IB_SMP_DATA_SIZE * IB_SMP_DATA_SIZE;

Quoted 7 lines

opensm/opensm/osm_switch.c +7 -0

+
+	if (!p_sw->lft) {
+		new_lft = malloc(lft_size);
+		if (!new_lft)
+			return -1;
+		memset(new_lft, OSM_NO_PATH, lft_size);
+		p_sw->lft = new_lft;

opensm/opensm/osm_switch.c void osm_switch_clear_hops(IN osm_switch_t * p_sw) · line 520

Quoted 10 lines

opensm/opensm/osm_switch.c +10 -0

+		p_sw->lft_size = lft_size;
+	} else if (lft_size > p_sw->lft_size) {
+		new_lft = realloc(p_sw->lft, lft_size);
+		if (!new_lft)
+			return -1;
+		memset(new_lft + p_sw->lft_size, OSM_NO_PATH,
+		       lft_size - p_sw->lft_size);
+		p_sw->lft = new_lft;
+		p_sw->lft_size = lft_size;
+	}

I think that this if/else if can be merged since initially p_sw->lft_size is zero.

Sasha

Quoted 101 lines

opensm/opensm/osm_switch.c +11 -4

+	return 0;
+}
+
+/**********************************************************************
+ **********************************************************************/
 int osm_switch_prepare_path_rebuild(IN osm_switch_t * p_sw, IN uint16_t max_lids)
 {
 	uint8_t **hops;
 	unsigned i;
 
+	if (alloc_lft(p_sw, max_lids))
+		return -1;
+
 	for (i = 0; i < p_sw->num_ports; i++)
 		osm_port_prof_construct(&p_sw->p_prof[i]);
 
 	osm_switch_clear_hops(p_sw);
 
-	if (!p_sw->new_lft &&
-	    !(p_sw->new_lft = malloc(IB_LID_UCAST_END_HO + 1)))
-		return IB_INSUFFICIENT_MEMORY;
+	if (!(p_sw->new_lft = realloc(p_sw->new_lft, p_sw->lft_size)))
+		return -1;
 
-	memset(p_sw->new_lft, OSM_NO_PATH, IB_LID_UCAST_END_HO + 1);
+	memset(p_sw->new_lft, OSM_NO_PATH, p_sw->lft_size);
 
 	if (!p_sw->hops) {
 		hops = malloc((max_lids + 1) * sizeof(hops[0]));

opensm/opensm/osm_ucast_cache.c +2 -2

index 6d3c53e..31a5333 100644
@@ -1079,10 +1079,10 @@ int osm_ucast_cache_process(osm_ucast_mgr_t * p_mgr)
 			/* no new routing was recently calculated for this
 			   switch, but the LFT needs to be updated anyway */
 			p_sw->new_lft = p_sw->lft;
-			p_sw->lft = malloc(IB_LID_UCAST_END_HO + 1);
+			p_sw->lft = malloc(p_sw->lft_size);
 			if (!p_sw->lft)
 				return IB_INSUFFICIENT_MEMORY;
-			memset(p_sw->lft, OSM_NO_PATH, IB_LID_UCAST_END_HO + 1);
+			memset(p_sw->lft, OSM_NO_PATH, p_sw->lft_size);
 		}
 
 	}

opensm/opensm/osm_ucast_file.c +1 -2

index 5b73ca5..7c1b5ba 100644
@@ -193,8 +193,7 @@ static int do_ucast_file_load(void *context)
 					cl_ntoh64(sw_guid));
 				continue;
 			}
-			memset(p_sw->new_lft, OSM_NO_PATH,
-			       IB_LID_UCAST_END_HO + 1);
+			memset(p_sw->new_lft, OSM_NO_PATH, p_sw->lft_size);
 		} else if (p_sw && !strncmp(p, "0x", 2)) {
 			p += 2;
 			lid = (uint16_t) strtoul(p, &q, 16);

opensm/opensm/osm_ucast_ftree.c +1 -1

index 1defd95..885c834 100644
@@ -566,7 +566,7 @@ static ftree_sw_t *sw_create(IN ftree_fabric_t * p_ftree,
 		return NULL;
 
 	/* initialize lft buffer */
-	memset(p_osm_sw->new_lft, OSM_NO_PATH, IB_LID_UCAST_END_HO + 1);
+	memset(p_osm_sw->new_lft, OSM_NO_PATH, p_osm_sw->lft_size);
 	p_sw->hops = malloc((p_osm_sw->max_lid_ho + 1) * sizeof(*(p_sw->hops)));
 	if (p_sw->hops == NULL)
 		return NULL;

opensm/opensm/osm_ucast_lash.c +2 -2

index 8b0a190..8015226 100644
@@ -994,7 +994,7 @@ static void populate_fwd_tbls(lash_t * p_lash)
 
 	p_next_sw = (osm_switch_t *) cl_qmap_head(&p_subn->sw_guid_tbl);
 
-	/* Go through each swtich individually */
+	/* Go through each switch individually */
 	while (p_next_sw != (osm_switch_t *) cl_qmap_end(&p_subn->sw_guid_tbl)) {
 		uint64_t current_guid;
 		switch_t *sw;

@@ -1005,7 +1005,7 @@ static void populate_fwd_tbls(lash_t * p_lash)
 		current_guid = p_sw->p_node->node_info.port_guid;
 		sw = p_sw->priv;
 
-		memset(p_sw->new_lft, OSM_NO_PATH, IB_LID_UCAST_END_HO + 1);
+		memset(p_sw->new_lft, OSM_NO_PATH, p_sw->lft_size);
 
 		for (lid = 1; lid <= max_lid_ho; lid++) {
 			port = cl_ptr_vector_get(&p_subn->port_lid_tbl, lid);

opensm/opensm/osm_ucast_mgr.c +0 -0

index be37df9..d0e85b1 100644

opensm/opensm/osm_ucast_mgr.c static void ucast_mgr_process_tbl(IN cl_map_item_t * p_map_item, · line 378

Quoted 8 lines

opensm/opensm/osm_ucast_mgr.c +1 -1

@@ -372,7 +372,7 @@ static void ucast_mgr_process_tbl(IN cl_map_item_t * p_map_item,
 		cl_ntoh64(osm_node_get_node_guid(p_sw->p_node)));
 
 	/* Initialize LIDs in buffer to invalid port number. */
-	memset(p_sw->new_lft, OSM_NO_PATH, IB_LID_UCAST_END_HO + 1);
+	memset(p_sw->new_lft, OSM_NO_PATH, p_sw->max_lid_ho + 1);
 
 	if (p_mgr->p_subn->opt.lmc)
 		alloc_ports_priv(p_mgr);

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

2009-10-13 12:59 UTC #3 lore raw

Hi Sasha,

Quoted 76 lines · Sasha Khapyorsky

Hi Hal,
Quoted 56 lines · Hal Rosenstock
Heap memory consumption by the unicast and multicast routing tables can be reduced.
Using valgrind --tool=massif (for heap profiling), there are couple of places that consume most of the heap memory:
->38.75% (11,206,656B) 0x43267E: osm_switch_new (osm_switch.c:134)
->12.89% (3,728,256B) 0x40F8C9: osm_mcast_tbl_init (osm_mcast_tbl.c:96)
osm_switch_new (osm_switch.c:108):
p_sw->lft = malloc(IB_LID_UCAST_END_HO + 1);
From ib_types.h
#define IB_LID_UCAST_END_HO 0xBFFF
The LFT can be allocated after LID assignment, once the number of LIDs assigned is known.
So it looks like for cluster of 2-4K with an LMC of 0 about 40% (!!!) of the heap memory can be saved:
- 39% used by LFTs, each with 48K entries - SM can allocate 4K entries instead.
A similar subsequent change will do this for MFTs.
Signed-off-by Hal Rosenstock <[email protected]>
---
Changes since v2:
Fixed alloc_lft handling of an existing lft Moved alloc_lft into osm_switch_prepare_path_rebuild
Changes since v1:
Basic approach changed to not do in chunks but rather allocate LFT once LID assignment is completed
opensm/include/opensm/osm_switch.h +2 -1
index 502e5a7..d881576 100644
@@ -102,6 +102,7 @@ typedef struct osm_switch {
       osm_port_profile_t *p_prof;
       uint8_t *lft;
       uint8_t *new_lft;
+     uint16_t lft_size;
       osm_mcast_tbl_t mcast_tbl;
       int32_t mft_block_num;
       uint32_t mft_position;
@@ -253,7 +254,7 @@ static inline uint8_t osm_switch_get_hop_count(IN const osm_switch_t * p_sw,
                                              IN uint16_t lid_ho,
                                              IN uint8_t port_num)
  {
-     return (lid_ho > p_sw->max_lid_ho || !p_sw->hops[lid_ho]) ?
+     return (lid_ho >= p_sw->lft_size || !p_sw->hops[lid_ho]) ?
           OSM_NO_PATH : p_sw->hops[lid_ho][port_num];
  }
  /*
opensm/include/opensm/osm_switch.h void osm_switch_clear_hops(IN osm_switch_t * p_sw); · line 345
Quoted 5 lines
opensm/include/opensm/osm_switch.h +1 -1
@@ -341,7 +342,7 @@ void osm_switch_clear_hops(IN osm_switch_t * p_sw);
  static inline uint8_t osm_switch_get_least_hops(IN const osm_switch_t * p_sw,
                                               IN uint16_t lid_ho)
  {
-     return (lid_ho > p_sw->max_lid_ho || !p_sw->hops[lid_ho]) ?
+     return (lid_ho >= p_sw->lft_size || !p_sw->hops[lid_ho]) ?
Now LFT buffer is only increased in size, so in cases then actual number of lids was decreased (and p_sw->max_lid_ho + 1 < p_sw->lft_size) such changed check may return garbage data from previous cycles. I think the check:
lid_ho > p_sw->max_lid_ho
should be preserved rather than:
lid_ho >= p_sw->lft_size

Quoted 11 lines

opensm/include/opensm/osm_switch.h uint8_t osm_switch_get_port_least_hops(IN const osm_switch_t * p_sw, · line 409

Quoted 9 lines

opensm/include/opensm/osm_switch.h +1 -1

           OSM_NO_PATH : p_sw->hops[lid_ho][0];
  }
  /*

@@ -405,7 +406,7 @@ uint8_t osm_switch_get_port_least_hops(IN const osm_switch_t * p_sw,
  static inline uint8_t osm_switch_get_port_by_lid(IN const osm_switch_t * p_sw,
                                                IN uint16_t lid_ho)
  {
-     if (lid_ho == 0 || lid_ho > IB_LID_UCAST_END_HO)
+     if (lid_ho == 0 || lid_ho >= p_sw->lft_size)

Similar story? Shouldn't this be checked against p_sw->max_lid_ho?

Yes and this change could be considered independent as I think is a minor optimization even with the LFT heap changes.

-- Hal

Quoted 220 lines

Quoted 62 lines

opensm/include/opensm/osm_switch.h +1 -1

               return OSM_NO_PATH;
       return p_sw->lft[lid_ho];
  }

@@ -711,7 +712,7 @@ osm_switch_set_lft_block(IN osm_switch_t * p_sw, IN const uint8_t * p_block,
               (uint16_t) (block_num * IB_SMP_DATA_SIZE);
       CL_ASSERT(p_sw);
 
-     if (lid_start + IB_SMP_DATA_SIZE > IB_LID_UCAST_END_HO)
+     if (lid_start + IB_SMP_DATA_SIZE > p_sw->lft_size)
               return IB_INVALID_PARAMETER;
 
       memcpy(&p_sw->lft[lid_start], p_block, IB_SMP_DATA_SIZE);

opensm/opensm/osm_state_mgr.c +2 -2

index 185c700..c40c9d3 100644
@@ -1,6 +1,6 @@
  /*
   * Copyright (c) 2004-2008 Voltaire, Inc. All rights reserved.
- * Copyright (c) 2002-2008 Mellanox Technologies LTD. All rights reserved.
+ * Copyright (c) 2002-2009 Mellanox Technologies LTD. All rights reserved.
   * Copyright (c) 1996-2003 Intel Corporation. All rights reserved.
   * Copyright (c) 2009 HNR Consulting. All rights reserved.
   *

@@ -1011,9 +1011,9 @@ static void cleanup_switch(cl_map_item_t * item, void *log)
       if (!sw->new_lft)
               return;
 
-     if (memcmp(sw->lft, sw->new_lft, IB_LID_UCAST_END_HO + 1))
+     if (memcmp(sw->lft, sw->new_lft, sw->lft_size))

osm_log(log, OSM_LOG_ERROR, "ERR 331D: "
- "LFT of switch 0x%016" PRIx64 " is not up to date.\n",
+ "LFT of switch 0x%016" PRIx64 " is not up to date\n", cl_ntoh64(sw->p_node->node_info.node_guid)); else {
free(sw->new_lft);

opensm/opensm/osm_switch.c +1 -7

index ad1018f..7d51c05 100644
@@ -1,6 +1,6 @@
  /*
   * Copyright (c) 2004-2008 Voltaire, Inc. All rights reserved.
- * Copyright (c) 2002-2008 Mellanox Technologies LTD. All rights reserved.
+ * Copyright (c) 2002-2009 Mellanox Technologies LTD. All rights reserved.
   * Copyright (c) 1996-2003 Intel Corporation. All rights reserved.
   * Copyright (c) 2009 HNR Consulting. All rights reserved.
   *

@@ -130,12 +130,6 @@ osm_switch_t *osm_switch_new(IN osm_node_t * p_node,
       p_sw->num_ports = num_ports;
       p_sw->need_update = 2;
 
-     p_sw->lft = malloc(IB_LID_UCAST_END_HO + 1);
-     if (!p_sw->lft)
-             goto err;
-
-     memset(p_sw->lft, OSM_NO_PATH, IB_LID_UCAST_END_HO + 1);
-
       p_sw->p_prof = malloc(sizeof(*p_sw->p_prof) * num_ports);
       if (!p_sw->p_prof)
               goto err;

opensm/opensm/osm_switch.c boolean_t osm_switch_get_lft_block(IN const osm_switch_t * p_sw, · line 163

Quoted 5 lines

opensm/opensm/osm_switch.c +1 -1

@@ -166,7 +160,7 @@ boolean_t osm_switch_get_lft_block(IN const osm_switch_t * p_sw,
       CL_ASSERT(p_sw);
       CL_ASSERT(p_block);
 
-     if (base_lid_ho > p_sw->max_lid_ho)
+     if (base_lid_ho >= p_sw->lft_size)

Ditto.

Quoted 6 lines

opensm/opensm/osm_switch.c +0 -0

               return FALSE;
 
       CL_ASSERT(base_lid_ho + IB_SMP_DATA_SIZE <= IB_LID_UCAST_END_HO);

@@ -498,21 +492,54 @@ void osm_switch_clear_hops(IN osm_switch_t * p_sw)
 
  /**********************************************************************

opensm/opensm/osm_switch.c void osm_switch_clear_hops(IN osm_switch_t * p_sw) · line 503

Quoted 10 lines

opensm/opensm/osm_switch.c +9 -0

   **********************************************************************/
+static int alloc_lft(IN osm_switch_t * p_sw, uint16_t lids)
+{
+     uint16_t lft_size;
+     uint8_t *new_lft;
+
+     lft_size = lids;
+     /* Ensure LFT is in units of LFT block size */
+     if (lids % IB_SMP_DATA_SIZE)
+             lft_size = (lids + IB_SMP_DATA_SIZE) / IB_SMP_DATA_SIZE * IB_SMP_DATA_SIZE;

Could be slightly faster:

lft_size = (lids + IB_SMP_DATA_SIZE - 1) / IB_SMP_DATA_SIZE * IB_SMP_DATA_SIZE;

Quoted 7 lines

opensm/opensm/osm_switch.c +7 -0

+
+     if (!p_sw->lft) {
+             new_lft = malloc(lft_size);
+             if (!new_lft)
+                     return -1;
+             memset(new_lft, OSM_NO_PATH, lft_size);
+             p_sw->lft = new_lft;

opensm/opensm/osm_switch.c void osm_switch_clear_hops(IN osm_switch_t * p_sw) · line 520

Quoted 10 lines

opensm/opensm/osm_switch.c +10 -0

+             p_sw->lft_size = lft_size;
+     } else if (lft_size > p_sw->lft_size) {
+             new_lft = realloc(p_sw->lft, lft_size);
+             if (!new_lft)
+                     return -1;
+             memset(new_lft + p_sw->lft_size, OSM_NO_PATH,
+                    lft_size - p_sw->lft_size);
+             p_sw->lft = new_lft;
+             p_sw->lft_size = lft_size;
+     }

I think that this if/else if can be merged since initially p_sw->lft_size is zero.

Sasha

Quoted 106 lines

opensm/opensm/osm_switch.c +11 -4

+     return 0;
+}
+
+/**********************************************************************
+ **********************************************************************/
  int osm_switch_prepare_path_rebuild(IN osm_switch_t * p_sw, IN uint16_t max_lids)
  {
       uint8_t **hops;
       unsigned i;
 
+     if (alloc_lft(p_sw, max_lids))
+             return -1;
+
       for (i = 0; i < p_sw->num_ports; i++)
               osm_port_prof_construct(&p_sw->p_prof[i]);
 
       osm_switch_clear_hops(p_sw);
 
-     if (!p_sw->new_lft &&
-         !(p_sw->new_lft = malloc(IB_LID_UCAST_END_HO + 1)))
-             return IB_INSUFFICIENT_MEMORY;
+     if (!(p_sw->new_lft = realloc(p_sw->new_lft, p_sw->lft_size)))
+             return -1;
 
-     memset(p_sw->new_lft, OSM_NO_PATH, IB_LID_UCAST_END_HO + 1);
+     memset(p_sw->new_lft, OSM_NO_PATH, p_sw->lft_size);
 
       if (!p_sw->hops) {
               hops = malloc((max_lids + 1) * sizeof(hops[0]));

opensm/opensm/osm_ucast_cache.c +0 -0

index 6d3c53e..31a5333 100644
@@ -1079,10 +1079,10 @@ int osm_ucast_cache_process(osm_ucast_mgr_t * p_mgr)
                       /* no new routing was recently calculated for this

switch, but the LFT needs to be updated anyway */ p_sw->new_lft = p_sw->lft;
- p_sw->lft = malloc(IB_LID_UCAST_END_HO + 1);
+ p_sw->lft = malloc(p_sw->lft_size);
if (!p_sw->lft)
return IB_INSUFFICIENT_MEMORY;
- memset(p_sw->lft, OSM_NO_PATH, IB_LID_UCAST_END_HO + 1);
+ memset(p_sw->lft, OSM_NO_PATH, p_sw->lft_size); }

}

opensm/opensm/osm_ucast_file.c +1 -2

index 5b73ca5..7c1b5ba 100644
@@ -193,8 +193,7 @@ static int do_ucast_file_load(void *context)
                                       cl_ntoh64(sw_guid));
                               continue;
                       }
-                     memset(p_sw->new_lft, OSM_NO_PATH,
-                            IB_LID_UCAST_END_HO + 1);
+                     memset(p_sw->new_lft, OSM_NO_PATH, p_sw->lft_size);
               } else if (p_sw && !strncmp(p, "0x", 2)) {
                       p += 2;
                       lid = (uint16_t) strtoul(p, &q, 16);

opensm/opensm/osm_ucast_ftree.c +1 -1

index 1defd95..885c834 100644
@@ -566,7 +566,7 @@ static ftree_sw_t *sw_create(IN ftree_fabric_t * p_ftree,
               return NULL;
 
       /* initialize lft buffer */
-     memset(p_osm_sw->new_lft, OSM_NO_PATH, IB_LID_UCAST_END_HO + 1);
+     memset(p_osm_sw->new_lft, OSM_NO_PATH, p_osm_sw->lft_size);
       p_sw->hops = malloc((p_osm_sw->max_lid_ho + 1) * sizeof(*(p_sw->hops)));
       if (p_sw->hops == NULL)
               return NULL;

opensm/opensm/osm_ucast_lash.c +2 -2

index 8b0a190..8015226 100644
@@ -994,7 +994,7 @@ static void populate_fwd_tbls(lash_t * p_lash)
 
       p_next_sw = (osm_switch_t *) cl_qmap_head(&p_subn->sw_guid_tbl);
 
-     /* Go through each swtich individually */
+     /* Go through each switch individually */
       while (p_next_sw != (osm_switch_t *) cl_qmap_end(&p_subn->sw_guid_tbl)) {
               uint64_t current_guid;
               switch_t *sw;

@@ -1005,7 +1005,7 @@ static void populate_fwd_tbls(lash_t * p_lash)
               current_guid = p_sw->p_node->node_info.port_guid;
               sw = p_sw->priv;
 
-             memset(p_sw->new_lft, OSM_NO_PATH, IB_LID_UCAST_END_HO + 1);
+             memset(p_sw->new_lft, OSM_NO_PATH, p_sw->lft_size);
 
               for (lid = 1; lid <= max_lid_ho; lid++) {
                       port = cl_ptr_vector_get(&p_subn->port_lid_tbl, lid);

opensm/opensm/osm_ucast_mgr.c +1 -1

index be37df9..d0e85b1 100644
@@ -372,7 +372,7 @@ static void ucast_mgr_process_tbl(IN cl_map_item_t * p_map_item,
               cl_ntoh64(osm_node_get_node_guid(p_sw->p_node)));
 
       /* Initialize LIDs in buffer to invalid port number. */
-     memset(p_sw->new_lft, OSM_NO_PATH, IB_LID_UCAST_END_HO + 1);
+     memset(p_sw->new_lft, OSM_NO_PATH, p_sw->max_lid_ho + 1);

Quoted 8 lines

opensm/opensm/osm_ucast_mgr.c static void ucast_mgr_process_tbl(IN cl_map_item_t * p_map_item, · line 378
Quoted 3 lines
opensm/opensm/osm_ucast_mgr.c +0 -0
       if (p_mgr->p_subn->opt.lmc)
               alloc_ports_priv(p_mgr);
--
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

Sasha Khapyorsky

author

2009-10-13 15:44 UTC #5 lore raw

In reply to a message not in the archive

Quoted 11 lines · Hal Rosenstock

Quoted 8 lines
Review uint8_t osm_switch_get_port_least_hops(IN const osm_switch_t * p_sw, · line 409
Quoted 6 lines
(quoted diff) +1 -1
@@ -405,7 +406,7 @@ uint8_t osm_switch_get_port_least_hops(IN const osm_switch_t * p_sw,
  static inline uint8_t osm_switch_get_port_by_lid(IN const osm_switch_t * p_sw,
                                                IN uint16_t lid_ho)
  {
-     if (lid_ho == 0 || lid_ho > IB_LID_UCAST_END_HO)
+     if (lid_ho == 0 || lid_ho >= p_sw->lft_size)
Similar story? Shouldn't this be checked against p_sw->max_lid_ho?
Yes and this change could be considered independent as I think is a minor optimization even with the LFT heap changes.

It is not just optimization, but rather a bug fix - it should prevent potential junk returned by this function. And yes, it exists even without LFT reallocation stuff.

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