opensm/mcast_tbl: set max_mlid_ho as actually configured mlid

PATCH New Hal Rosenstock 2 participants 9 messages Oct 30, 2009 – Nov 2, 2009 6 messages not in the archive InfiniBand

Share & tools

A message that is not in the archive (sent off-list or not yet received): [email protected]

A message that is not in the archive (sent off-list or not yet received): 4AEAEA86.8080309-klaOcWyJdxkshyMvu7JE4pqQE7yCjDx5@public.gmane.org

Hal Rosenstock

author

2009-10-30 13:48 UTC #2 lore raw

Quoted 15 lines · Sasha Khapyorsky

Set switch's mcast table max_mlid_ho as actually configured mlid for the switch instead of a value extended to MFT block size aligned. This makes mlid handling checks to be more reliable.
Signed-off-by Sasha Khapyorsky <sashak-smomgflXvOZWk0Htik3J/[email protected]>
---
opensm/opensm/osm_mcast_tbl.c | 5 +++--
1 files changed, 3 insertions(+), 2 deletions(-)
opensm/opensm/osm_mcast_tbl.c +0 -0
index b5ae6f2..eee9290 100644

opensm/opensm/osm_mcast_tbl.c int osm_mcast_tbl_realloc(IN osm_mcast_tbl_t * p_tbl, IN · line 126

Quoted 6 lines

opensm/opensm/osm_mcast_tbl.c +1 -1

@@ -122,7 +122,7 @@ int osm_mcast_tbl_realloc(IN osm_mcast_tbl_t * p_tbl, IN
 uintn_t mlid_offset)
        uint16_t (*p_mask_tbl)[][IB_MCAST_POSITION_MAX];
 
        if (mlid_offset < p_tbl->mft_depth)
-               return 0;
+               goto done;

Why should max_mlid_ho be changed when < mft_depth ?

Doing so makes group removal not work properly.

-- Hal

Quoted 14 lines

opensm/opensm/osm_mcast_tbl.c +0 -0
        /*
The number of bytes needed in the mask table is:
opensm/opensm/osm_mcast_tbl.c +0 -0
@@ -144,7 +144,8 @@ int osm_mcast_tbl_realloc(IN osm_mcast_tbl_t * p_tbl, IN
 uintn_t mlid_offset)
size - p_tbl->mft_depth * (IB_MCAST_POSITION_MAX + 1) * IB_MCAST_MASK_SIZE / 8);
p_tbl->p_mask_tbl = p_mask_tbl;
p_tbl->mft_depth = mft_depth;
- p_tbl->max_mlid_ho = mft_depth + IB_LID_MCAST_START_HO - 1; +done:
+ p_tbl->max_mlid_ho = mlid_offset + IB_LID_MCAST_START_HO - 1; return 0;
}

Quoted 10 lines

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

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

Sasha Khapyorsky

2009-10-30 20:06 UTC #4 lore raw

Quoted 9 lines · Hal Rosenstock

Quoted 9 lines

opensm/opensm/osm_mcast_tbl.c +0 -0

index b5ae6f2..eee9290 100644
@@ -122,7 +122,7 @@ int osm_mcast_tbl_realloc(IN osm_mcast_tbl_t * p_tbl, IN
 uintn_t mlid_offset)
        uint16_t (*p_mask_tbl)[][IB_MCAST_POSITION_MAX];

Quoted 5 lines

opensm/opensm/osm_mcast_tbl.c int osm_mcast_tbl_realloc(IN osm_mcast_tbl_t * p_tbl, IN · line 126
Quoted 3 lines
opensm/opensm/osm_mcast_tbl.c +1 -1
        if (mlid_offset < p_tbl->mft_depth)
-               return 0;
+               goto done;
Why should max_mlid_ho be changed when < mft_depth ?

To be equal to an actually configured max mlid value (not a size of mcast table allocation).

Quoted 1 line

Doing so makes group removal not work properly.

This may have this issue, but not due to fact of max_mlid_ho change, but due to invalid check in osm_mcast_tbl_clear_mlid(). I think there it should be something like:

opensm/opensm/osm_mcast_tbl.c +2 -2

index 0a45904..a599e56 100644
@@ -245,8 +245,8 @@ void osm_mcast_tbl_clear_mlid(IN osm_mcast_tbl_t * p_tbl, IN uint16_t mlid_ho)
 	CL_ASSERT(p_tbl);
 	CL_ASSERT(mlid_ho >= IB_LID_MCAST_START_HO);
 
-	if (p_tbl->p_mask_tbl && mlid_ho <= p_tbl->max_mlid_ho) {
-		mlid_offset = mlid_ho - IB_LID_MCAST_START_HO;
+	mlid_offset = mlid_ho - IB_LID_MCAST_START_HO;
+	if (p_tbl->p_mask_tbl && mlid_offset < p_tbl->mft_depth) {
 		for (i = 0; i <= p_tbl->max_position; i++)
 			(*p_tbl->p_mask_tbl)[mlid_offset][i] = 0;
 	}

Does it make sense?

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

Hal Rosenstock

author Tested-by

2009-10-30 20:44 UTC #5 lore raw

Quoted 41 lines · Sasha Khapyorsky

Quoted 9 lines · Hal Rosenstock
Quoted 9 lines
opensm/opensm/osm_mcast_tbl.c +0 -0
index b5ae6f2..eee9290 100644
@@ -122,7 +122,7 @@ int osm_mcast_tbl_realloc(IN osm_mcast_tbl_t * p_tbl, IN
 uintn_t mlid_offset)
        uint16_t (*p_mask_tbl)[][IB_MCAST_POSITION_MAX];
 
Quoted 5 lines
opensm/opensm/osm_mcast_tbl.c int osm_mcast_tbl_realloc(IN osm_mcast_tbl_t * p_tbl, IN · line 126
Quoted 3 lines
opensm/opensm/osm_mcast_tbl.c +1 -1
        if (mlid_offset < p_tbl->mft_depth)
-               return 0;
+               goto done;
Why should max_mlid_ho be changed when < mft_depth ?
To be equal to an actually configured max mlid value (not a size of mcast table allocation).
Quoted 1 line
Doing so makes group removal not work properly.
This may have this issue, but not due to fact of max_mlid_ho change, but due to invalid check in osm_mcast_tbl_clear_mlid(). I think there it should be something like:
opensm/opensm/osm_mcast_tbl.c +2 -2
index 0a45904..a599e56 100644
@@ -245,8 +245,8 @@ void osm_mcast_tbl_clear_mlid(IN osm_mcast_tbl_t * p_tbl, IN uint16_t mlid_ho)
        CL_ASSERT(p_tbl);
        CL_ASSERT(mlid_ho >= IB_LID_MCAST_START_HO);
 
-       if (p_tbl->p_mask_tbl && mlid_ho <= p_tbl->max_mlid_ho) {
-               mlid_offset = mlid_ho - IB_LID_MCAST_START_HO;
+       mlid_offset = mlid_ho - IB_LID_MCAST_START_HO;
+       if (p_tbl->p_mask_tbl && mlid_offset < p_tbl->mft_depth) {
                for (i = 0; i <= p_tbl->max_position; i++)
                        (*p_tbl->p_mask_tbl)[mlid_offset][i] = 0;
        }

Quoted 1 line

Does it make sense?

Yes, that fixes the removal issue.

Tested-by Hal Rosenstock <[email protected]>

-- Hal

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

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

Sasha Khapyorsky

2009-10-30 21:31 UTC #7 lore raw

Quoted 17 lines · Hal Rosenstock

Quoted 17 lines

opensm/opensm/osm_mcast_tbl.c +2 -2

index 0a45904..a599e56 100644
@@ -245,8 +245,8 @@ void osm_mcast_tbl_clear_mlid(IN osm_mcast_tbl_t * p_tbl, IN uint16_t mlid_ho)
        CL_ASSERT(p_tbl);
        CL_ASSERT(mlid_ho >= IB_LID_MCAST_START_HO);
 
-       if (p_tbl->p_mask_tbl && mlid_ho <= p_tbl->max_mlid_ho) {
-               mlid_offset = mlid_ho - IB_LID_MCAST_START_HO;
+       mlid_offset = mlid_ho - IB_LID_MCAST_START_HO;
+       if (p_tbl->p_mask_tbl && mlid_offset < p_tbl->mft_depth) {
                for (i = 0; i <= p_tbl->max_position; i++)
                        (*p_tbl->p_mask_tbl)[mlid_offset][i] = 0;
        }

Quoted 5 lines

Quoted 1 line
Does it make sense?
Yes, that fixes the removal issue.
Tested-by Hal Rosenstock <[email protected]>

Thanks I will apply the patch.

Yet another (likely even more efficient) approach would be memset()ing MFTs in realloc function above requested mlid_offset, then we will be able to remove osm_mcast_tbl_clear_mlid() completely.

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

Hal Rosenstock

author

2009-11-02 16:18 UTC #8 lore raw

Quoted 22 lines · Sasha Khapyorsky

Quoted 21 lines · Hal Rosenstock

Quoted 17 lines

opensm/opensm/osm_mcast_tbl.c +2 -2

index 0a45904..a599e56 100644
@@ -245,8 +245,8 @@ void osm_mcast_tbl_clear_mlid(IN osm_mcast_tbl_t * p_tbl, IN uint16_t mlid_ho)
        CL_ASSERT(p_tbl);
        CL_ASSERT(mlid_ho >= IB_LID_MCAST_START_HO);
 
-       if (p_tbl->p_mask_tbl && mlid_ho <= p_tbl->max_mlid_ho) {
-               mlid_offset = mlid_ho - IB_LID_MCAST_START_HO;
+       mlid_offset = mlid_ho - IB_LID_MCAST_START_HO;
+       if (p_tbl->p_mask_tbl && mlid_offset < p_tbl->mft_depth) {
                for (i = 0; i <= p_tbl->max_position; i++)
                        (*p_tbl->p_mask_tbl)[mlid_offset][i] = 0;
        }

Quoted 1 line

Does it make sense?

Yes, that fixes the removal issue.

Quoted 7 lines

Quoted 1 line
Tested-by Hal Rosenstock <[email protected]>
Thanks I will apply the patch.
Yet another (likely even more efficient) approach would be memset()ing MFTs in realloc function above requested mlid_offset, then we will be able to remove osm_mcast_tbl_clear_mlid() completely.

Isn't mlid clearing done on a per mlid basis rather than based on above some mlid (offset) ? Also and perhaps more significantly, an mlid can be removed in the middle of a range of mlids. So I don't see how clear_mlid can be removed.

-- Hal

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

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

Sasha Khapyorsky

2009-11-02 16:43 UTC #10 lore raw

Quoted 9 lines · Hal Rosenstock

Quoted 4 lines
Yet another (likely even more efficient) approach would be memset()ing MFTs in realloc function above requested mlid_offset, then we will be able to remove osm_mcast_tbl_clear_mlid() completely.
Isn't mlid clearing done on a per mlid basis rather than based on above some mlid (offset) ? Also and perhaps more significantly, an mlid can be removed in the middle of a range of mlids. So I don't see how clear_mlid can be removed.

Yes, correct, we cannot remove clear_mlid completely.

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

Hal Rosenstock

author

2009-11-02 16:44 UTC #11 lore raw

Quoted 6 lines · Sasha Khapyorsky

Quoted 5 lines · Hal Rosenstock
Quoted 4 lines
Yet another (likely even more efficient) approach would be memset()ing MFTs in realloc function above requested mlid_offset, then we will be able to remove osm_mcast_tbl_clear_mlid() completely.

Quoted 6 lines

Quoted 4 lines
Isn't mlid clearing done on a per mlid basis rather than based on above some mlid (offset) ? Also and perhaps more significantly, an mlid can be removed in the middle of a range of mlids. So I don't see how clear_mlid can be removed.
Yes, correct, we cannot remove clear_mlid completely.

clear_mlid could be implemented with memset rather than loop. Is that worthwhile ?

-- Hal

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

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

Sasha Khapyorsky

2009-11-02 16:50 UTC #13 lore raw

Quoted 2 lines · Hal Rosenstock

clear_mlid could be implemented with memset rather than loop.

Yes, memset() would be better.

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

2009-10-30 21:32 UTC #14 lore raw

PATCH 1/1 opensm/osm_mcast_tbl: fix mlid removal

In reply to Sasha Khapyorsky

Removed mlid can exceed a current max_mlid_ho value, so to prevent MFT garbage above this the requested mlid should be checked against actual MFT size.

Signed-off-by Sasha Khapyorsky <sashak-smomgflXvOZWk0Htik3J/[email protected]>

---
 opensm/opensm/osm_mcast_tbl.c |    5 ++---
 1 files changed, 2 insertions(+), 3 deletions(-)

opensm/opensm/osm_mcast_tbl.c +2 -3

index 0a45904..8136c37 100644
@@ -245,11 +245,10 @@ void osm_mcast_tbl_clear_mlid(IN osm_mcast_tbl_t * p_tbl, IN uint16_t mlid_ho)
 	CL_ASSERT(p_tbl);
 	CL_ASSERT(mlid_ho >= IB_LID_MCAST_START_HO);
 
-	if (p_tbl->p_mask_tbl && mlid_ho <= p_tbl->max_mlid_ho) {
-		mlid_offset = mlid_ho - IB_LID_MCAST_START_HO;
+	mlid_offset = mlid_ho - IB_LID_MCAST_START_HO;
+	if (p_tbl->p_mask_tbl && mlid_offset < p_tbl->mft_depth)
 		for (i = 0; i <= p_tbl->max_position; i++)
 			(*p_tbl->p_mask_tbl)[mlid_offset][i] = 0;
-	}
 }
 
 /**********************************************************************

Signature · 6 lines

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