dev.dpdk.org archive mirror
 help / color / mirror / Atom feed
* [dpdk-dev] [PATCH v4 1/3] lib/lpm: memory orderings to avoid race conditions for v1604
@ 2019-07-03  5:44 Ruifeng Wang
  2019-07-03  5:44 ` [dpdk-dev] [PATCH v4 2/3] lib/lpm: memory orderings to avoid race conditions for v20 Ruifeng Wang
                   ` (2 more replies)
  0 siblings, 3 replies; 13+ messages in thread
From: Ruifeng Wang @ 2019-07-03  5:44 UTC (permalink / raw)
  To: vladimir.medvedkin, bruce.richardson
  Cc: dev, honnappa.nagarahalli, gavin.hu, nd, Ruifeng Wang

When a tbl8 group is getting attached to a tbl24 entry, lookup
might fail even though the entry is configured in the table.

For ex: consider a LPM table configured with 10.10.10.1/24.
When a new entry 10.10.10.32/28 is being added, a new tbl8
group is allocated and tbl24 entry is changed to point to
the tbl8 group. If the tbl24 entry is written without the tbl8
group entries updated, a lookup on 10.10.10.9 will return
failure.

Correct memory orderings are required to ensure that the
store to tbl24 does not happen before the stores to tbl8 group
entries complete.

The ordering patches in general have no notable impact on LPM
performance test on both Arm A72 platform and x86 E5 platform.

Signed-off-by: Honnappa Nagarahalli <honnappa.nagarahalli@arm.com>
Signed-off-by: Ruifeng Wang <ruifeng.wang@arm.com>
Reviewed-by: Gavin Hu <gavin.hu@arm.com>
---
v4: updated test data
v3: updated test data
v2: no changes

 lib/librte_lpm/rte_lpm.c | 32 +++++++++++++++++++++++++-------
 lib/librte_lpm/rte_lpm.h |  4 ++++
 2 files changed, 29 insertions(+), 7 deletions(-)

diff --git a/lib/librte_lpm/rte_lpm.c b/lib/librte_lpm/rte_lpm.c
index 6b7b28a2e..6ec450a08 100644
--- a/lib/librte_lpm/rte_lpm.c
+++ b/lib/librte_lpm/rte_lpm.c
@@ -806,7 +806,8 @@ add_depth_small_v1604(struct rte_lpm *lpm, uint32_t ip, uint8_t depth,
 			/* Setting tbl24 entry in one go to avoid race
 			 * conditions
 			 */
-			lpm->tbl24[i] = new_tbl24_entry;
+			__atomic_store(&lpm->tbl24[i], &new_tbl24_entry,
+					__ATOMIC_RELEASE);
 
 			continue;
 		}
@@ -1017,7 +1018,11 @@ add_depth_big_v1604(struct rte_lpm *lpm, uint32_t ip_masked, uint8_t depth,
 			.depth = 0,
 		};
 
-		lpm->tbl24[tbl24_index] = new_tbl24_entry;
+		/* The tbl24 entry must be written only after the
+		 * tbl8 entries are written.
+		 */
+		__atomic_store(&lpm->tbl24[tbl24_index], &new_tbl24_entry,
+				__ATOMIC_RELEASE);
 
 	} /* If valid entry but not extended calculate the index into Table8. */
 	else if (lpm->tbl24[tbl24_index].valid_group == 0) {
@@ -1063,7 +1068,11 @@ add_depth_big_v1604(struct rte_lpm *lpm, uint32_t ip_masked, uint8_t depth,
 				.depth = 0,
 		};
 
-		lpm->tbl24[tbl24_index] = new_tbl24_entry;
+		/* The tbl24 entry must be written only after the
+		 * tbl8 entries are written.
+		 */
+		__atomic_store(&lpm->tbl24[tbl24_index], &new_tbl24_entry,
+				__ATOMIC_RELEASE);
 
 	} else { /*
 		* If it is valid, extended entry calculate the index into tbl8.
@@ -1391,6 +1400,7 @@ delete_depth_small_v1604(struct rte_lpm *lpm, uint32_t ip_masked,
 	/* Calculate the range and index into Table24. */
 	tbl24_range = depth_to_range(depth);
 	tbl24_index = (ip_masked >> 8);
+	struct rte_lpm_tbl_entry zero_tbl24_entry = {0};
 
 	/*
 	 * Firstly check the sub_rule_index. A -1 indicates no replacement rule
@@ -1405,7 +1415,8 @@ delete_depth_small_v1604(struct rte_lpm *lpm, uint32_t ip_masked,
 
 			if (lpm->tbl24[i].valid_group == 0 &&
 					lpm->tbl24[i].depth <= depth) {
-				lpm->tbl24[i].valid = INVALID;
+				__atomic_store(&lpm->tbl24[i],
+					&zero_tbl24_entry, __ATOMIC_RELEASE);
 			} else if (lpm->tbl24[i].valid_group == 1) {
 				/*
 				 * If TBL24 entry is extended, then there has
@@ -1450,7 +1461,8 @@ delete_depth_small_v1604(struct rte_lpm *lpm, uint32_t ip_masked,
 
 			if (lpm->tbl24[i].valid_group == 0 &&
 					lpm->tbl24[i].depth <= depth) {
-				lpm->tbl24[i] = new_tbl24_entry;
+				__atomic_store(&lpm->tbl24[i], &new_tbl24_entry,
+						__ATOMIC_RELEASE);
 			} else  if (lpm->tbl24[i].valid_group == 1) {
 				/*
 				 * If TBL24 entry is extended, then there has
@@ -1713,8 +1725,11 @@ delete_depth_big_v1604(struct rte_lpm *lpm, uint32_t ip_masked,
 	tbl8_recycle_index = tbl8_recycle_check_v1604(lpm->tbl8, tbl8_group_start);
 
 	if (tbl8_recycle_index == -EINVAL) {
-		/* Set tbl24 before freeing tbl8 to avoid race condition. */
+		/* Set tbl24 before freeing tbl8 to avoid race condition.
+		 * Prevent the free of the tbl8 group from hoisting.
+		 */
 		lpm->tbl24[tbl24_index].valid = 0;
+		__atomic_thread_fence(__ATOMIC_RELEASE);
 		tbl8_free_v1604(lpm->tbl8, tbl8_group_start);
 	} else if (tbl8_recycle_index > -1) {
 		/* Update tbl24 entry. */
@@ -1725,8 +1740,11 @@ delete_depth_big_v1604(struct rte_lpm *lpm, uint32_t ip_masked,
 			.depth = lpm->tbl8[tbl8_recycle_index].depth,
 		};
 
-		/* Set tbl24 before freeing tbl8 to avoid race condition. */
+		/* Set tbl24 before freeing tbl8 to avoid race condition.
+		 * Prevent the free of the tbl8 group from hoisting.
+		 */
 		lpm->tbl24[tbl24_index] = new_tbl24_entry;
+		__atomic_thread_fence(__ATOMIC_RELEASE);
 		tbl8_free_v1604(lpm->tbl8, tbl8_group_start);
 	}
 #undef group_idx
diff --git a/lib/librte_lpm/rte_lpm.h b/lib/librte_lpm/rte_lpm.h
index b886f54b4..6f5704c5c 100644
--- a/lib/librte_lpm/rte_lpm.h
+++ b/lib/librte_lpm/rte_lpm.h
@@ -354,6 +354,10 @@ rte_lpm_lookup(struct rte_lpm *lpm, uint32_t ip, uint32_t *next_hop)
 	ptbl = (const uint32_t *)(&lpm->tbl24[tbl24_index]);
 	tbl_entry = *ptbl;
 
+	/* Memory ordering is not required in lookup. Because dataflow
+	 * dependency exists, compiler or HW won't be able to re-order
+	 * the operations.
+	 */
 	/* Copy tbl8 entry (only if needed) */
 	if (unlikely((tbl_entry & RTE_LPM_VALID_EXT_ENTRY_BITMASK) ==
 			RTE_LPM_VALID_EXT_ENTRY_BITMASK)) {
-- 
2.17.1


^ permalink raw reply related	[flat|nested] 13+ messages in thread

* [dpdk-dev] [PATCH v4 2/3] lib/lpm: memory orderings to avoid race conditions for v20
  2019-07-03  5:44 [dpdk-dev] [PATCH v4 1/3] lib/lpm: memory orderings to avoid race conditions for v1604 Ruifeng Wang
@ 2019-07-03  5:44 ` Ruifeng Wang
  2019-07-05 16:52   ` Medvedkin, Vladimir
  2019-07-03  5:44 ` [dpdk-dev] [PATCH v4 3/3] lib/lpm: use atomic store to avoid partial update Ruifeng Wang
  2019-07-04 20:25 ` [dpdk-dev] [PATCH v4 1/3] lib/lpm: memory orderings to avoid race conditions for v1604 Thomas Monjalon
  2 siblings, 1 reply; 13+ messages in thread
From: Ruifeng Wang @ 2019-07-03  5:44 UTC (permalink / raw)
  To: vladimir.medvedkin, bruce.richardson
  Cc: dev, honnappa.nagarahalli, gavin.hu, nd, Ruifeng Wang

When a tbl8 group is getting attached to a tbl24 entry, lookup
might fail even though the entry is configured in the table.

For ex: consider a LPM table configured with 10.10.10.1/24.
When a new entry 10.10.10.32/28 is being added, a new tbl8
group is allocated and tbl24 entry is changed to point to
the tbl8 group. If the tbl24 entry is written without the tbl8
group entries updated, a lookup on 10.10.10.9 will return
failure.

Correct memory orderings are required to ensure that the
store to tbl24 does not happen before the stores to tbl8 group
entries complete.

Besides, explicit structure alignment is used to address atomic
operation building issue with older version clang.

Suggested-by: Honnappa Nagarahalli <honnappa.nagarahalli@arm.com>
Signed-off-by: Ruifeng Wang <ruifeng.wang@arm.com>
Reviewed-by: Honnappa Nagarahalli <honnappa.nagarahalli@arm.com>
Reviewed-by: Gavin Hu <gavin.hu@arm.com>
---
v4: changed alignment attribute parameter
v3: no changes
v2: fixed clang building issue by supplying alignment attribute.

 lib/librte_lpm/rte_lpm.c | 32 +++++++++++++++++++++++++-------
 lib/librte_lpm/rte_lpm.h |  4 ++--
 2 files changed, 27 insertions(+), 9 deletions(-)

diff --git a/lib/librte_lpm/rte_lpm.c b/lib/librte_lpm/rte_lpm.c
index 6ec450a08..baa6e7460 100644
--- a/lib/librte_lpm/rte_lpm.c
+++ b/lib/librte_lpm/rte_lpm.c
@@ -737,7 +737,8 @@ add_depth_small_v20(struct rte_lpm_v20 *lpm, uint32_t ip, uint8_t depth,
 			/* Setting tbl24 entry in one go to avoid race
 			 * conditions
 			 */
-			lpm->tbl24[i] = new_tbl24_entry;
+			__atomic_store(&lpm->tbl24[i], &new_tbl24_entry,
+					__ATOMIC_RELEASE);
 
 			continue;
 		}
@@ -892,7 +893,8 @@ add_depth_big_v20(struct rte_lpm_v20 *lpm, uint32_t ip_masked, uint8_t depth,
 			.depth = 0,
 		};
 
-		lpm->tbl24[tbl24_index] = new_tbl24_entry;
+		__atomic_store(&lpm->tbl24[tbl24_index], &new_tbl24_entry,
+				__ATOMIC_RELEASE);
 
 	} /* If valid entry but not extended calculate the index into Table8. */
 	else if (lpm->tbl24[tbl24_index].valid_group == 0) {
@@ -938,7 +940,8 @@ add_depth_big_v20(struct rte_lpm_v20 *lpm, uint32_t ip_masked, uint8_t depth,
 				.depth = 0,
 		};
 
-		lpm->tbl24[tbl24_index] = new_tbl24_entry;
+		__atomic_store(&lpm->tbl24[tbl24_index], &new_tbl24_entry,
+				__ATOMIC_RELEASE);
 
 	} else { /*
 		* If it is valid, extended entry calculate the index into tbl8.
@@ -1320,7 +1323,15 @@ delete_depth_small_v20(struct rte_lpm_v20 *lpm, uint32_t ip_masked,
 
 			if (lpm->tbl24[i].valid_group == 0 &&
 					lpm->tbl24[i].depth <= depth) {
-				lpm->tbl24[i].valid = INVALID;
+				struct rte_lpm_tbl_entry_v20
+					zero_tbl24_entry = {
+						.valid = INVALID,
+						.depth = 0,
+						.valid_group = 0,
+					};
+					zero_tbl24_entry.next_hop = 0;
+				__atomic_store(&lpm->tbl24[i],
+					&zero_tbl24_entry, __ATOMIC_RELEASE);
 			} else if (lpm->tbl24[i].valid_group == 1) {
 				/*
 				 * If TBL24 entry is extended, then there has
@@ -1365,7 +1376,8 @@ delete_depth_small_v20(struct rte_lpm_v20 *lpm, uint32_t ip_masked,
 
 			if (lpm->tbl24[i].valid_group == 0 &&
 					lpm->tbl24[i].depth <= depth) {
-				lpm->tbl24[i] = new_tbl24_entry;
+				__atomic_store(&lpm->tbl24[i], &new_tbl24_entry,
+						__ATOMIC_RELEASE);
 			} else  if (lpm->tbl24[i].valid_group == 1) {
 				/*
 				 * If TBL24 entry is extended, then there has
@@ -1647,8 +1659,11 @@ delete_depth_big_v20(struct rte_lpm_v20 *lpm, uint32_t ip_masked,
 	tbl8_recycle_index = tbl8_recycle_check_v20(lpm->tbl8, tbl8_group_start);
 
 	if (tbl8_recycle_index == -EINVAL) {
-		/* Set tbl24 before freeing tbl8 to avoid race condition. */
+		/* Set tbl24 before freeing tbl8 to avoid race condition.
+		 * Prevent the free of the tbl8 group from hoisting.
+		 */
 		lpm->tbl24[tbl24_index].valid = 0;
+		__atomic_thread_fence(__ATOMIC_RELEASE);
 		tbl8_free_v20(lpm->tbl8, tbl8_group_start);
 	} else if (tbl8_recycle_index > -1) {
 		/* Update tbl24 entry. */
@@ -1659,8 +1674,11 @@ delete_depth_big_v20(struct rte_lpm_v20 *lpm, uint32_t ip_masked,
 			.depth = lpm->tbl8[tbl8_recycle_index].depth,
 		};
 
-		/* Set tbl24 before freeing tbl8 to avoid race condition. */
+		/* Set tbl24 before freeing tbl8 to avoid race condition.
+		 * Prevent the free of the tbl8 group from hoisting.
+		 */
 		lpm->tbl24[tbl24_index] = new_tbl24_entry;
+		__atomic_thread_fence(__ATOMIC_RELEASE);
 		tbl8_free_v20(lpm->tbl8, tbl8_group_start);
 	}
 
diff --git a/lib/librte_lpm/rte_lpm.h b/lib/librte_lpm/rte_lpm.h
index 6f5704c5c..906ec4483 100644
--- a/lib/librte_lpm/rte_lpm.h
+++ b/lib/librte_lpm/rte_lpm.h
@@ -88,7 +88,7 @@ struct rte_lpm_tbl_entry_v20 {
 	 */
 	uint8_t valid_group :1;
 	uint8_t depth       :6; /**< Rule depth. */
-};
+} __rte_aligned(sizeof(uint16_t));
 
 __extension__
 struct rte_lpm_tbl_entry {
@@ -121,7 +121,7 @@ struct rte_lpm_tbl_entry_v20 {
 		uint8_t group_idx;
 		uint8_t next_hop;
 	};
-};
+} __rte_aligned(sizeof(uint16_t));
 
 __extension__
 struct rte_lpm_tbl_entry {
-- 
2.17.1


^ permalink raw reply related	[flat|nested] 13+ messages in thread

* [dpdk-dev] [PATCH v4 3/3] lib/lpm: use atomic store to avoid partial update
  2019-07-03  5:44 [dpdk-dev] [PATCH v4 1/3] lib/lpm: memory orderings to avoid race conditions for v1604 Ruifeng Wang
  2019-07-03  5:44 ` [dpdk-dev] [PATCH v4 2/3] lib/lpm: memory orderings to avoid race conditions for v20 Ruifeng Wang
@ 2019-07-03  5:44 ` Ruifeng Wang
  2019-07-05 16:53   ` Medvedkin, Vladimir
  2019-07-08  4:56   ` Honnappa Nagarahalli
  2019-07-04 20:25 ` [dpdk-dev] [PATCH v4 1/3] lib/lpm: memory orderings to avoid race conditions for v1604 Thomas Monjalon
  2 siblings, 2 replies; 13+ messages in thread
From: Ruifeng Wang @ 2019-07-03  5:44 UTC (permalink / raw)
  To: vladimir.medvedkin, bruce.richardson
  Cc: dev, honnappa.nagarahalli, gavin.hu, nd, Ruifeng Wang

Compiler could generate non-atomic stores for whole table entry
updating. This may cause incorrect nexthop to be returned, if
the byte with valid flag is updated prior to the byte with next
hot is updated.

Changed to use atomic store to update whole table entry.

Suggested-by: Medvedkin Vladimir <vladimir.medvedkin@intel.com>
Signed-off-by: Ruifeng Wang <ruifeng.wang@arm.com>
Reviewed-by: Gavin Hu <gavin.hu@arm.com>
---
v4: initial version

 lib/librte_lpm/rte_lpm.c | 34 ++++++++++++++++++++++++----------
 1 file changed, 24 insertions(+), 10 deletions(-)

diff --git a/lib/librte_lpm/rte_lpm.c b/lib/librte_lpm/rte_lpm.c
index baa6e7460..5d1dbd7e6 100644
--- a/lib/librte_lpm/rte_lpm.c
+++ b/lib/librte_lpm/rte_lpm.c
@@ -767,7 +767,9 @@ add_depth_small_v20(struct rte_lpm_v20 *lpm, uint32_t ip, uint8_t depth,
 					 * Setting tbl8 entry in one go to avoid
 					 * race conditions
 					 */
-					lpm->tbl8[j] = new_tbl8_entry;
+					__atomic_store(&lpm->tbl8[j],
+						&new_tbl8_entry,
+						__ATOMIC_RELAXED);
 
 					continue;
 				}
@@ -837,7 +839,9 @@ add_depth_small_v1604(struct rte_lpm *lpm, uint32_t ip, uint8_t depth,
 					 * Setting tbl8 entry in one go to avoid
 					 * race conditions
 					 */
-					lpm->tbl8[j] = new_tbl8_entry;
+					__atomic_store(&lpm->tbl8[j],
+						&new_tbl8_entry,
+						__ATOMIC_RELAXED);
 
 					continue;
 				}
@@ -965,7 +969,8 @@ add_depth_big_v20(struct rte_lpm_v20 *lpm, uint32_t ip_masked, uint8_t depth,
 				 * Setting tbl8 entry in one go to avoid race
 				 * condition
 				 */
-				lpm->tbl8[i] = new_tbl8_entry;
+				__atomic_store(&lpm->tbl8[i], &new_tbl8_entry,
+						__ATOMIC_RELAXED);
 
 				continue;
 			}
@@ -1100,7 +1105,8 @@ add_depth_big_v1604(struct rte_lpm *lpm, uint32_t ip_masked, uint8_t depth,
 				 * Setting tbl8 entry in one go to avoid race
 				 * condition
 				 */
-				lpm->tbl8[i] = new_tbl8_entry;
+				__atomic_store(&lpm->tbl8[i], &new_tbl8_entry,
+						__ATOMIC_RELAXED);
 
 				continue;
 			}
@@ -1393,7 +1399,9 @@ delete_depth_small_v20(struct rte_lpm_v20 *lpm, uint32_t ip_masked,
 					RTE_LPM_TBL8_GROUP_NUM_ENTRIES); j++) {
 
 					if (lpm->tbl8[j].depth <= depth)
-						lpm->tbl8[j] = new_tbl8_entry;
+						__atomic_store(&lpm->tbl8[j],
+							&new_tbl8_entry,
+							__ATOMIC_RELAXED);
 				}
 			}
 		}
@@ -1490,7 +1498,9 @@ delete_depth_small_v1604(struct rte_lpm *lpm, uint32_t ip_masked,
 					RTE_LPM_TBL8_GROUP_NUM_ENTRIES); j++) {
 
 					if (lpm->tbl8[j].depth <= depth)
-						lpm->tbl8[j] = new_tbl8_entry;
+						__atomic_store(&lpm->tbl8[j],
+							&new_tbl8_entry,
+							__ATOMIC_RELAXED);
 				}
 			}
 		}
@@ -1646,7 +1656,8 @@ delete_depth_big_v20(struct rte_lpm_v20 *lpm, uint32_t ip_masked,
 		 */
 		for (i = tbl8_index; i < (tbl8_index + tbl8_range); i++) {
 			if (lpm->tbl8[i].depth <= depth)
-				lpm->tbl8[i] = new_tbl8_entry;
+				__atomic_store(&lpm->tbl8[i], &new_tbl8_entry,
+						__ATOMIC_RELAXED);
 		}
 	}
 
@@ -1677,7 +1688,8 @@ delete_depth_big_v20(struct rte_lpm_v20 *lpm, uint32_t ip_masked,
 		/* Set tbl24 before freeing tbl8 to avoid race condition.
 		 * Prevent the free of the tbl8 group from hoisting.
 		 */
-		lpm->tbl24[tbl24_index] = new_tbl24_entry;
+		__atomic_store(&lpm->tbl24[tbl24_index], &new_tbl24_entry,
+				__ATOMIC_RELAXED);
 		__atomic_thread_fence(__ATOMIC_RELEASE);
 		tbl8_free_v20(lpm->tbl8, tbl8_group_start);
 	}
@@ -1730,7 +1742,8 @@ delete_depth_big_v1604(struct rte_lpm *lpm, uint32_t ip_masked,
 		 */
 		for (i = tbl8_index; i < (tbl8_index + tbl8_range); i++) {
 			if (lpm->tbl8[i].depth <= depth)
-				lpm->tbl8[i] = new_tbl8_entry;
+				__atomic_store(&lpm->tbl8[i], &new_tbl8_entry,
+						__ATOMIC_RELAXED);
 		}
 	}
 
@@ -1761,7 +1774,8 @@ delete_depth_big_v1604(struct rte_lpm *lpm, uint32_t ip_masked,
 		/* Set tbl24 before freeing tbl8 to avoid race condition.
 		 * Prevent the free of the tbl8 group from hoisting.
 		 */
-		lpm->tbl24[tbl24_index] = new_tbl24_entry;
+		__atomic_store(&lpm->tbl24[tbl24_index], &new_tbl24_entry,
+				__ATOMIC_RELAXED);
 		__atomic_thread_fence(__ATOMIC_RELEASE);
 		tbl8_free_v1604(lpm->tbl8, tbl8_group_start);
 	}
-- 
2.17.1


^ permalink raw reply related	[flat|nested] 13+ messages in thread

* Re: [dpdk-dev] [PATCH v4 1/3] lib/lpm: memory orderings to avoid race conditions for v1604
  2019-07-03  5:44 [dpdk-dev] [PATCH v4 1/3] lib/lpm: memory orderings to avoid race conditions for v1604 Ruifeng Wang
  2019-07-03  5:44 ` [dpdk-dev] [PATCH v4 2/3] lib/lpm: memory orderings to avoid race conditions for v20 Ruifeng Wang
  2019-07-03  5:44 ` [dpdk-dev] [PATCH v4 3/3] lib/lpm: use atomic store to avoid partial update Ruifeng Wang
@ 2019-07-04 20:25 ` Thomas Monjalon
  2019-07-05  6:39   ` Ruifeng Wang (Arm Technology China)
  2 siblings, 1 reply; 13+ messages in thread
From: Thomas Monjalon @ 2019-07-04 20:25 UTC (permalink / raw)
  To: Ruifeng Wang
  Cc: dev, vladimir.medvedkin, bruce.richardson, honnappa.nagarahalli,
	gavin.hu, nd

03/07/2019 07:44, Ruifeng Wang:
> v4: updated test data
> v3: updated test data
> v2: no changes

Ruifeng, it is not easy to follow your changes
because you don't use --in-reply-to when sending
new versions. So they are not in the same mail thread.
It would also help to make a cover letter and make
every versions a child of the very first cover letter.

Please pass the word to you colleagues in Arm,
it seems to be a common miss in the process.
Thanks




^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [dpdk-dev] [PATCH v4 1/3] lib/lpm: memory orderings to avoid race conditions for v1604
  2019-07-04 20:25 ` [dpdk-dev] [PATCH v4 1/3] lib/lpm: memory orderings to avoid race conditions for v1604 Thomas Monjalon
@ 2019-07-05  6:39   ` Ruifeng Wang (Arm Technology China)
  0 siblings, 0 replies; 13+ messages in thread
From: Ruifeng Wang (Arm Technology China) @ 2019-07-05  6:39 UTC (permalink / raw)
  To: thomas
  Cc: dev, vladimir.medvedkin, bruce.richardson, Honnappa Nagarahalli,
	Gavin Hu (Arm Technology China),
	nd, nd


> -----Original Message-----
> From: Thomas Monjalon <thomas@monjalon.net>
> Sent: Friday, July 5, 2019 04:25
> To: Ruifeng Wang (Arm Technology China) <Ruifeng.Wang@arm.com>
> Cc: dev@dpdk.org; vladimir.medvedkin@intel.com;
> bruce.richardson@intel.com; Honnappa Nagarahalli
> <Honnappa.Nagarahalli@arm.com>; Gavin Hu (Arm Technology China)
> <Gavin.Hu@arm.com>; nd <nd@arm.com>
> Subject: Re: [dpdk-dev] [PATCH v4 1/3] lib/lpm: memory orderings to avoid
> race conditions for v1604
> 
> 03/07/2019 07:44, Ruifeng Wang:
> > v4: updated test data
> > v3: updated test data
> > v2: no changes
> 
> Ruifeng, it is not easy to follow your changes because you don't use --in-
> reply-to when sending new versions. So they are not in the same mail thread.
> It would also help to make a cover letter and make every versions a child of
> the very first cover letter.
> 
> Please pass the word to you colleagues in Arm, it seems to be a common
> miss in the process.
> Thanks
> 
Thanks for reminding. We'll review Contributor's Guidelines and stick to it.


^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [dpdk-dev] [PATCH v4 2/3] lib/lpm: memory orderings to avoid race conditions for v20
  2019-07-03  5:44 ` [dpdk-dev] [PATCH v4 2/3] lib/lpm: memory orderings to avoid race conditions for v20 Ruifeng Wang
@ 2019-07-05 16:52   ` Medvedkin, Vladimir
  2019-07-05 18:20     ` Vladimir Medvedkin
  0 siblings, 1 reply; 13+ messages in thread
From: Medvedkin, Vladimir @ 2019-07-05 16:52 UTC (permalink / raw)
  To: Ruifeng Wang, bruce.richardson; +Cc: dev, honnappa.nagarahalli, gavin.hu, nd

Hi Wang,

On 03/07/2019 06:44, Ruifeng Wang wrote:
> When a tbl8 group is getting attached to a tbl24 entry, lookup
> might fail even though the entry is configured in the table.
>
> For ex: consider a LPM table configured with 10.10.10.1/24.
> When a new entry 10.10.10.32/28 is being added, a new tbl8
> group is allocated and tbl24 entry is changed to point to
> the tbl8 group. If the tbl24 entry is written without the tbl8
> group entries updated, a lookup on 10.10.10.9 will return
> failure.
>
> Correct memory orderings are required to ensure that the
> store to tbl24 does not happen before the stores to tbl8 group
> entries complete.
>
> Besides, explicit structure alignment is used to address atomic
> operation building issue with older version clang.
>
> Suggested-by: Honnappa Nagarahalli <honnappa.nagarahalli@arm.com>
> Signed-off-by: Ruifeng Wang <ruifeng.wang@arm.com>
> Reviewed-by: Honnappa Nagarahalli <honnappa.nagarahalli@arm.com>
> Reviewed-by: Gavin Hu <gavin.hu@arm.com>
> ---
> v4: changed alignment attribute parameter
> v3: no changes
> v2: fixed clang building issue by supplying alignment attribute.
>
>   lib/librte_lpm/rte_lpm.c | 32 +++++++++++++++++++++++++-------
>   lib/librte_lpm/rte_lpm.h |  4 ++--
>   2 files changed, 27 insertions(+), 9 deletions(-)
>
> diff --git a/lib/librte_lpm/rte_lpm.c b/lib/librte_lpm/rte_lpm.c
> index 6ec450a08..baa6e7460 100644
> --- a/lib/librte_lpm/rte_lpm.c
> +++ b/lib/librte_lpm/rte_lpm.c
> @@ -737,7 +737,8 @@ add_depth_small_v20(struct rte_lpm_v20 *lpm, uint32_t ip, uint8_t depth,
>   			/* Setting tbl24 entry in one go to avoid race
>   			 * conditions
>   			 */
> -			lpm->tbl24[i] = new_tbl24_entry;
> +			__atomic_store(&lpm->tbl24[i], &new_tbl24_entry,
> +					__ATOMIC_RELEASE);
>   
>   			continue;
>   		}
> @@ -892,7 +893,8 @@ add_depth_big_v20(struct rte_lpm_v20 *lpm, uint32_t ip_masked, uint8_t depth,
>   			.depth = 0,
>   		};
>   
> -		lpm->tbl24[tbl24_index] = new_tbl24_entry;
> +		__atomic_store(&lpm->tbl24[tbl24_index], &new_tbl24_entry,
> +				__ATOMIC_RELEASE);
>   
>   	} /* If valid entry but not extended calculate the index into Table8. */
>   	else if (lpm->tbl24[tbl24_index].valid_group == 0) {
> @@ -938,7 +940,8 @@ add_depth_big_v20(struct rte_lpm_v20 *lpm, uint32_t ip_masked, uint8_t depth,
>   				.depth = 0,
>   		};
>   
> -		lpm->tbl24[tbl24_index] = new_tbl24_entry;
> +		__atomic_store(&lpm->tbl24[tbl24_index], &new_tbl24_entry,
> +				__ATOMIC_RELEASE);
>   
>   	} else { /*
>   		* If it is valid, extended entry calculate the index into tbl8.
> @@ -1320,7 +1323,15 @@ delete_depth_small_v20(struct rte_lpm_v20 *lpm, uint32_t ip_masked,
>   
>   			if (lpm->tbl24[i].valid_group == 0 &&
>   					lpm->tbl24[i].depth <= depth) {
> -				lpm->tbl24[i].valid = INVALID;
> +				struct rte_lpm_tbl_entry_v20
> +					zero_tbl24_entry = {
> +						.valid = INVALID,
> +						.depth = 0,
> +						.valid_group = 0,
> +					};
> +					zero_tbl24_entry.next_hop = 0;
Why don't you use just "struct rte_lpm_tbl_entry_v20 zero_tbl24_entry = 
{0} " like you do for _v1604?
> +				__atomic_store(&lpm->tbl24[i],
> +					&zero_tbl24_entry, __ATOMIC_RELEASE);
>   			} else if (lpm->tbl24[i].valid_group == 1) {
>   				/*
>   				 * If TBL24 entry is extended, then there has
> @@ -1365,7 +1376,8 @@ delete_depth_small_v20(struct rte_lpm_v20 *lpm, uint32_t ip_masked,
>   
>   			if (lpm->tbl24[i].valid_group == 0 &&
>   					lpm->tbl24[i].depth <= depth) {
> -				lpm->tbl24[i] = new_tbl24_entry;
> +				__atomic_store(&lpm->tbl24[i], &new_tbl24_entry,
> +						__ATOMIC_RELEASE);
>   			} else  if (lpm->tbl24[i].valid_group == 1) {
>   				/*
>   				 * If TBL24 entry is extended, then there has
> @@ -1647,8 +1659,11 @@ delete_depth_big_v20(struct rte_lpm_v20 *lpm, uint32_t ip_masked,
>   	tbl8_recycle_index = tbl8_recycle_check_v20(lpm->tbl8, tbl8_group_start);
>   
>   	if (tbl8_recycle_index == -EINVAL) {
> -		/* Set tbl24 before freeing tbl8 to avoid race condition. */
> +		/* Set tbl24 before freeing tbl8 to avoid race condition.
> +		 * Prevent the free of the tbl8 group from hoisting.
> +		 */
>   		lpm->tbl24[tbl24_index].valid = 0;
> +		__atomic_thread_fence(__ATOMIC_RELEASE);
>   		tbl8_free_v20(lpm->tbl8, tbl8_group_start);
>   	} else if (tbl8_recycle_index > -1) {
>   		/* Update tbl24 entry. */
> @@ -1659,8 +1674,11 @@ delete_depth_big_v20(struct rte_lpm_v20 *lpm, uint32_t ip_masked,
>   			.depth = lpm->tbl8[tbl8_recycle_index].depth,
>   		};
>   
> -		/* Set tbl24 before freeing tbl8 to avoid race condition. */
> +		/* Set tbl24 before freeing tbl8 to avoid race condition.
> +		 * Prevent the free of the tbl8 group from hoisting.
> +		 */
>   		lpm->tbl24[tbl24_index] = new_tbl24_entry;
> +		__atomic_thread_fence(__ATOMIC_RELEASE);
>   		tbl8_free_v20(lpm->tbl8, tbl8_group_start);
>   	}
>   
> diff --git a/lib/librte_lpm/rte_lpm.h b/lib/librte_lpm/rte_lpm.h
> index 6f5704c5c..906ec4483 100644
> --- a/lib/librte_lpm/rte_lpm.h
> +++ b/lib/librte_lpm/rte_lpm.h
> @@ -88,7 +88,7 @@ struct rte_lpm_tbl_entry_v20 {
>   	 */
>   	uint8_t valid_group :1;
>   	uint8_t depth       :6; /**< Rule depth. */
> -};
> +} __rte_aligned(sizeof(uint16_t));
>   
>   __extension__
>   struct rte_lpm_tbl_entry {
> @@ -121,7 +121,7 @@ struct rte_lpm_tbl_entry_v20 {
>   		uint8_t group_idx;
>   		uint8_t next_hop;
>   	};
> -};
> +} __rte_aligned(sizeof(uint16_t));
>   
>   __extension__
>   struct rte_lpm_tbl_entry {

-- 
Regards,
Vladimir


^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [dpdk-dev] [PATCH v4 3/3] lib/lpm: use atomic store to avoid partial update
  2019-07-03  5:44 ` [dpdk-dev] [PATCH v4 3/3] lib/lpm: use atomic store to avoid partial update Ruifeng Wang
@ 2019-07-05 16:53   ` Medvedkin, Vladimir
  2019-07-08  5:42     ` Ruifeng Wang (Arm Technology China)
  2019-07-08  4:56   ` Honnappa Nagarahalli
  1 sibling, 1 reply; 13+ messages in thread
From: Medvedkin, Vladimir @ 2019-07-05 16:53 UTC (permalink / raw)
  To: Ruifeng Wang, bruce.richardson; +Cc: dev, honnappa.nagarahalli, gavin.hu, nd

Hi Wang,

On 03/07/2019 06:44, Ruifeng Wang wrote:
> Compiler could generate non-atomic stores for whole table entry
> updating. This may cause incorrect nexthop to be returned, if
> the byte with valid flag is updated prior to the byte with next
> hot is updated.
>
> Changed to use atomic store to update whole table entry.
>
> Suggested-by: Medvedkin Vladimir <vladimir.medvedkin@intel.com>
> Signed-off-by: Ruifeng Wang <ruifeng.wang@arm.com>
> Reviewed-by: Gavin Hu <gavin.hu@arm.com>
> ---
> v4: initial version
>
>   lib/librte_lpm/rte_lpm.c | 34 ++++++++++++++++++++++++----------
>   1 file changed, 24 insertions(+), 10 deletions(-)
>
> diff --git a/lib/librte_lpm/rte_lpm.c b/lib/librte_lpm/rte_lpm.c
> index baa6e7460..5d1dbd7e6 100644
> --- a/lib/librte_lpm/rte_lpm.c
> +++ b/lib/librte_lpm/rte_lpm.c
> @@ -767,7 +767,9 @@ add_depth_small_v20(struct rte_lpm_v20 *lpm, uint32_t ip, uint8_t depth,
>   					 * Setting tbl8 entry in one go to avoid
>   					 * race conditions
>   					 */
> -					lpm->tbl8[j] = new_tbl8_entry;
> +					__atomic_store(&lpm->tbl8[j],
> +						&new_tbl8_entry,
> +						__ATOMIC_RELAXED);
>   
>   					continue;
>   				}
> @@ -837,7 +839,9 @@ add_depth_small_v1604(struct rte_lpm *lpm, uint32_t ip, uint8_t depth,
>   					 * Setting tbl8 entry in one go to avoid
>   					 * race conditions
>   					 */
> -					lpm->tbl8[j] = new_tbl8_entry;
> +					__atomic_store(&lpm->tbl8[j],
> +						&new_tbl8_entry,
> +						__ATOMIC_RELAXED);
>   
>   					continue;
>   				}
> @@ -965,7 +969,8 @@ add_depth_big_v20(struct rte_lpm_v20 *lpm, uint32_t ip_masked, uint8_t depth,
>   				 * Setting tbl8 entry in one go to avoid race
>   				 * condition
>   				 */
> -				lpm->tbl8[i] = new_tbl8_entry;
> +				__atomic_store(&lpm->tbl8[i], &new_tbl8_entry,
> +						__ATOMIC_RELAXED);
>   
>   				continue;
>   			}
> @@ -1100,7 +1105,8 @@ add_depth_big_v1604(struct rte_lpm *lpm, uint32_t ip_masked, uint8_t depth,
>   				 * Setting tbl8 entry in one go to avoid race
>   				 * condition
>   				 */
> -				lpm->tbl8[i] = new_tbl8_entry;
> +				__atomic_store(&lpm->tbl8[i], &new_tbl8_entry,
> +						__ATOMIC_RELAXED);
>   
>   				continue;
>   			}
> @@ -1393,7 +1399,9 @@ delete_depth_small_v20(struct rte_lpm_v20 *lpm, uint32_t ip_masked,
>   					RTE_LPM_TBL8_GROUP_NUM_ENTRIES); j++) {
>   
>   					if (lpm->tbl8[j].depth <= depth)
> -						lpm->tbl8[j] = new_tbl8_entry;
> +						__atomic_store(&lpm->tbl8[j],
> +							&new_tbl8_entry,
> +							__ATOMIC_RELAXED);
>   				}
>   			}
>   		}
> @@ -1490,7 +1498,9 @@ delete_depth_small_v1604(struct rte_lpm *lpm, uint32_t ip_masked,
>   					RTE_LPM_TBL8_GROUP_NUM_ENTRIES); j++) {
>   
>   					if (lpm->tbl8[j].depth <= depth)
> -						lpm->tbl8[j] = new_tbl8_entry;
> +						__atomic_store(&lpm->tbl8[j],
> +							&new_tbl8_entry,
> +							__ATOMIC_RELAXED);
>   				}
>   			}
>   		}
> @@ -1646,7 +1656,8 @@ delete_depth_big_v20(struct rte_lpm_v20 *lpm, uint32_t ip_masked,
>   		 */
>   		for (i = tbl8_index; i < (tbl8_index + tbl8_range); i++) {
>   			if (lpm->tbl8[i].depth <= depth)
> -				lpm->tbl8[i] = new_tbl8_entry;
> +				__atomic_store(&lpm->tbl8[i], &new_tbl8_entry,
> +						__ATOMIC_RELAXED);
>   		}
>   	}
>   
> @@ -1677,7 +1688,8 @@ delete_depth_big_v20(struct rte_lpm_v20 *lpm, uint32_t ip_masked,
>   		/* Set tbl24 before freeing tbl8 to avoid race condition.
>   		 * Prevent the free of the tbl8 group from hoisting.
>   		 */
> -		lpm->tbl24[tbl24_index] = new_tbl24_entry;
> +		__atomic_store(&lpm->tbl24[tbl24_index], &new_tbl24_entry,
> +				__ATOMIC_RELAXED);
>   		__atomic_thread_fence(__ATOMIC_RELEASE);
>   		tbl8_free_v20(lpm->tbl8, tbl8_group_start);
>   	}
> @@ -1730,7 +1742,8 @@ delete_depth_big_v1604(struct rte_lpm *lpm, uint32_t ip_masked,
>   		 */
>   		for (i = tbl8_index; i < (tbl8_index + tbl8_range); i++) {
>   			if (lpm->tbl8[i].depth <= depth)
> -				lpm->tbl8[i] = new_tbl8_entry;
> +				__atomic_store(&lpm->tbl8[i], &new_tbl8_entry,
> +						__ATOMIC_RELAXED);
>   		}
>   	}
>   
> @@ -1761,7 +1774,8 @@ delete_depth_big_v1604(struct rte_lpm *lpm, uint32_t ip_masked,
>   		/* Set tbl24 before freeing tbl8 to avoid race condition.
>   		 * Prevent the free of the tbl8 group from hoisting.
>   		 */
> -		lpm->tbl24[tbl24_index] = new_tbl24_entry;
> +		__atomic_store(&lpm->tbl24[tbl24_index], &new_tbl24_entry,
> +				__ATOMIC_RELAXED);
>   		__atomic_thread_fence(__ATOMIC_RELEASE);
Do you really need __atomic_thread_fence after atomic_store?
>   		tbl8_free_v1604(lpm->tbl8, tbl8_group_start);
>   	}

-- 
Regards,
Vladimir


^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [dpdk-dev] [PATCH v4 2/3] lib/lpm: memory orderings to avoid race conditions for v20
  2019-07-05 16:52   ` Medvedkin, Vladimir
@ 2019-07-05 18:20     ` Vladimir Medvedkin
  0 siblings, 0 replies; 13+ messages in thread
From: Vladimir Medvedkin @ 2019-07-05 18:20 UTC (permalink / raw)
  To: Medvedkin, Vladimir
  Cc: Ruifeng Wang, Bruce Richardson, dev, honnappa.nagarahalli, gavin.hu, nd

Hi all,

пт, 5 июл. 2019 г. в 17:52, Medvedkin, Vladimir <
vladimir.medvedkin@intel.com>:

> Hi Wang,
>
> On 03/07/2019 06:44, Ruifeng Wang wrote:
> > When a tbl8 group is getting attached to a tbl24 entry, lookup
> > might fail even though the entry is configured in the table.
> >
> > For ex: consider a LPM table configured with 10.10.10.1/24.
> > When a new entry 10.10.10.32/28 is being added, a new tbl8
> > group is allocated and tbl24 entry is changed to point to
> > the tbl8 group. If the tbl24 entry is written without the tbl8
> > group entries updated, a lookup on 10.10.10.9 will return
> > failure.
> >
> > Correct memory orderings are required to ensure that the
> > store to tbl24 does not happen before the stores to tbl8 group
> > entries complete.
> >
> > Besides, explicit structure alignment is used to address atomic
> > operation building issue with older version clang.
> >
> > Suggested-by: Honnappa Nagarahalli <honnappa.nagarahalli@arm.com>
> > Signed-off-by: Ruifeng Wang <ruifeng.wang@arm.com>
> > Reviewed-by: Honnappa Nagarahalli <honnappa.nagarahalli@arm.com>
> > Reviewed-by: Gavin Hu <gavin.hu@arm.com>
> > ---
> > v4: changed alignment attribute parameter
> > v3: no changes
> > v2: fixed clang building issue by supplying alignment attribute.
> >
> >   lib/librte_lpm/rte_lpm.c | 32 +++++++++++++++++++++++++-------
> >   lib/librte_lpm/rte_lpm.h |  4 ++--
> >   2 files changed, 27 insertions(+), 9 deletions(-)
> >
> > diff --git a/lib/librte_lpm/rte_lpm.c b/lib/librte_lpm/rte_lpm.c
> > index 6ec450a08..baa6e7460 100644
> > --- a/lib/librte_lpm/rte_lpm.c
> > +++ b/lib/librte_lpm/rte_lpm.c
> > @@ -737,7 +737,8 @@ add_depth_small_v20(struct rte_lpm_v20 *lpm,
> uint32_t ip, uint8_t depth,
> >                       /* Setting tbl24 entry in one go to avoid race
> >                        * conditions
> >                        */
> > -                     lpm->tbl24[i] = new_tbl24_entry;
> > +                     __atomic_store(&lpm->tbl24[i], &new_tbl24_entry,
> > +                                     __ATOMIC_RELEASE);
> >
> >                       continue;
> >               }
> > @@ -892,7 +893,8 @@ add_depth_big_v20(struct rte_lpm_v20 *lpm, uint32_t
> ip_masked, uint8_t depth,
> >                       .depth = 0,
> >               };
> >
> > -             lpm->tbl24[tbl24_index] = new_tbl24_entry;
> > +             __atomic_store(&lpm->tbl24[tbl24_index], &new_tbl24_entry,
> > +                             __ATOMIC_RELEASE);
> >
> >       } /* If valid entry but not extended calculate the index into
> Table8. */
> >       else if (lpm->tbl24[tbl24_index].valid_group == 0) {
> > @@ -938,7 +940,8 @@ add_depth_big_v20(struct rte_lpm_v20 *lpm, uint32_t
> ip_masked, uint8_t depth,
> >                               .depth = 0,
> >               };
> >
> > -             lpm->tbl24[tbl24_index] = new_tbl24_entry;
> > +             __atomic_store(&lpm->tbl24[tbl24_index], &new_tbl24_entry,
> > +                             __ATOMIC_RELEASE);
> >
> >       } else { /*
> >               * If it is valid, extended entry calculate the index into
> tbl8.
> > @@ -1320,7 +1323,15 @@ delete_depth_small_v20(struct rte_lpm_v20 *lpm,
> uint32_t ip_masked,
> >
> >                       if (lpm->tbl24[i].valid_group == 0 &&
> >                                       lpm->tbl24[i].depth <= depth) {
> > -                             lpm->tbl24[i].valid = INVALID;
> > +                             struct rte_lpm_tbl_entry_v20
> > +                                     zero_tbl24_entry = {
> > +                                             .valid = INVALID,
> > +                                             .depth = 0,
> > +                                             .valid_group = 0,
> > +                                     };
> > +                                     zero_tbl24_entry.next_hop = 0;
> Why don't you use just "struct rte_lpm_tbl_entry_v20 zero_tbl24_entry =
> {0} " like you do for _v1604?
>
Ah, sorry, just found what you mentioned earlier, this will cause meson
build to fail.

> > +                             __atomic_store(&lpm->tbl24[i],
> > +                                     &zero_tbl24_entry,
> __ATOMIC_RELEASE);
> >                       } else if (lpm->tbl24[i].valid_group == 1) {
> >                               /*
> >                                * If TBL24 entry is extended, then there
> has
> > @@ -1365,7 +1376,8 @@ delete_depth_small_v20(struct rte_lpm_v20 *lpm,
> uint32_t ip_masked,
> >
> >                       if (lpm->tbl24[i].valid_group == 0 &&
> >                                       lpm->tbl24[i].depth <= depth) {
> > -                             lpm->tbl24[i] = new_tbl24_entry;
> > +                             __atomic_store(&lpm->tbl24[i],
> &new_tbl24_entry,
> > +                                             __ATOMIC_RELEASE);
> >                       } else  if (lpm->tbl24[i].valid_group == 1) {
> >                               /*
> >                                * If TBL24 entry is extended, then there
> has
> > @@ -1647,8 +1659,11 @@ delete_depth_big_v20(struct rte_lpm_v20 *lpm,
> uint32_t ip_masked,
> >       tbl8_recycle_index = tbl8_recycle_check_v20(lpm->tbl8,
> tbl8_group_start);
> >
> >       if (tbl8_recycle_index == -EINVAL) {
> > -             /* Set tbl24 before freeing tbl8 to avoid race condition.
> */
> > +             /* Set tbl24 before freeing tbl8 to avoid race condition.
> > +              * Prevent the free of the tbl8 group from hoisting.
> > +              */
> >               lpm->tbl24[tbl24_index].valid = 0;
> > +             __atomic_thread_fence(__ATOMIC_RELEASE);
> >               tbl8_free_v20(lpm->tbl8, tbl8_group_start);
> >       } else if (tbl8_recycle_index > -1) {
> >               /* Update tbl24 entry. */
> > @@ -1659,8 +1674,11 @@ delete_depth_big_v20(struct rte_lpm_v20 *lpm,
> uint32_t ip_masked,
> >                       .depth = lpm->tbl8[tbl8_recycle_index].depth,
> >               };
> >
> > -             /* Set tbl24 before freeing tbl8 to avoid race condition.
> */
> > +             /* Set tbl24 before freeing tbl8 to avoid race condition.
> > +              * Prevent the free of the tbl8 group from hoisting.
> > +              */
> >               lpm->tbl24[tbl24_index] = new_tbl24_entry;
> > +             __atomic_thread_fence(__ATOMIC_RELEASE);
> >               tbl8_free_v20(lpm->tbl8, tbl8_group_start);
> >       }
> >
> > diff --git a/lib/librte_lpm/rte_lpm.h b/lib/librte_lpm/rte_lpm.h
> > index 6f5704c5c..906ec4483 100644
> > --- a/lib/librte_lpm/rte_lpm.h
> > +++ b/lib/librte_lpm/rte_lpm.h
> > @@ -88,7 +88,7 @@ struct rte_lpm_tbl_entry_v20 {
> >        */
> >       uint8_t valid_group :1;
> >       uint8_t depth       :6; /**< Rule depth. */
> > -};
> > +} __rte_aligned(sizeof(uint16_t));
> >
> >   __extension__
> >   struct rte_lpm_tbl_entry {
> > @@ -121,7 +121,7 @@ struct rte_lpm_tbl_entry_v20 {
> >               uint8_t group_idx;
> >               uint8_t next_hop;
> >       };
> > -};
> > +} __rte_aligned(sizeof(uint16_t));
> >
> >   __extension__
> >   struct rte_lpm_tbl_entry {
>
> --
> Regards,
> Vladimir
>
>

-- 
Regards,
Vladimir

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [dpdk-dev] [PATCH v4 3/3] lib/lpm: use atomic store to avoid partial update
  2019-07-03  5:44 ` [dpdk-dev] [PATCH v4 3/3] lib/lpm: use atomic store to avoid partial update Ruifeng Wang
  2019-07-05 16:53   ` Medvedkin, Vladimir
@ 2019-07-08  4:56   ` Honnappa Nagarahalli
  2019-07-08  6:01     ` Ruifeng Wang (Arm Technology China)
  1 sibling, 1 reply; 13+ messages in thread
From: Honnappa Nagarahalli @ 2019-07-08  4:56 UTC (permalink / raw)
  To: Ruifeng Wang (Arm Technology China),
	vladimir.medvedkin, bruce.richardson
  Cc: dev, Gavin Hu (Arm Technology China),
	nd, Ruifeng Wang (Arm Technology China),
	Honnappa Nagarahalli, nd

> 
> Compiler could generate non-atomic stores for whole table entry updating.
> This may cause incorrect nexthop to be returned, if the byte with valid flag is
> updated prior to the byte with next hot is updated.
                                                           ^^^^^^^
Should be nexthop

> 
> Changed to use atomic store to update whole table entry.
> 
> Suggested-by: Medvedkin Vladimir <vladimir.medvedkin@intel.com>
> Signed-off-by: Ruifeng Wang <ruifeng.wang@arm.com>
> Reviewed-by: Gavin Hu <gavin.hu@arm.com>
> ---
> v4: initial version
> 
>  lib/librte_lpm/rte_lpm.c | 34 ++++++++++++++++++++++++----------
>  1 file changed, 24 insertions(+), 10 deletions(-)
> 
> diff --git a/lib/librte_lpm/rte_lpm.c b/lib/librte_lpm/rte_lpm.c index
> baa6e7460..5d1dbd7e6 100644
> --- a/lib/librte_lpm/rte_lpm.c
> +++ b/lib/librte_lpm/rte_lpm.c
> @@ -767,7 +767,9 @@ add_depth_small_v20(struct rte_lpm_v20 *lpm,
> uint32_t ip, uint8_t depth,
>  					 * Setting tbl8 entry in one go to
> avoid
>  					 * race conditions
>  					 */
> -					lpm->tbl8[j] = new_tbl8_entry;
> +					__atomic_store(&lpm->tbl8[j],
> +						&new_tbl8_entry,
> +						__ATOMIC_RELAXED);
> 
>  					continue;
>  				}
> @@ -837,7 +839,9 @@ add_depth_small_v1604(struct rte_lpm *lpm,
> uint32_t ip, uint8_t depth,
>  					 * Setting tbl8 entry in one go to
> avoid
>  					 * race conditions
>  					 */
> -					lpm->tbl8[j] = new_tbl8_entry;
> +					__atomic_store(&lpm->tbl8[j],
> +						&new_tbl8_entry,
> +						__ATOMIC_RELAXED);
> 
>  					continue;
>  				}
> @@ -965,7 +969,8 @@ add_depth_big_v20(struct rte_lpm_v20 *lpm,
> uint32_t ip_masked, uint8_t depth,
>  				 * Setting tbl8 entry in one go to avoid race
>  				 * condition
>  				 */
> -				lpm->tbl8[i] = new_tbl8_entry;
> +				__atomic_store(&lpm->tbl8[i],
> &new_tbl8_entry,
> +						__ATOMIC_RELAXED);
> 
>  				continue;
>  			}
> @@ -1100,7 +1105,8 @@ add_depth_big_v1604(struct rte_lpm *lpm,
> uint32_t ip_masked, uint8_t depth,
>  				 * Setting tbl8 entry in one go to avoid race
>  				 * condition
>  				 */
> -				lpm->tbl8[i] = new_tbl8_entry;
> +				__atomic_store(&lpm->tbl8[i],
> &new_tbl8_entry,
> +						__ATOMIC_RELAXED);
> 
>  				continue;
>  			}
> @@ -1393,7 +1399,9 @@ delete_depth_small_v20(struct rte_lpm_v20 *lpm,
> uint32_t ip_masked,
> 
> 	RTE_LPM_TBL8_GROUP_NUM_ENTRIES); j++) {
> 
>  					if (lpm->tbl8[j].depth <= depth)
> -						lpm->tbl8[j] =
> new_tbl8_entry;
> +						__atomic_store(&lpm->tbl8[j],
> +							&new_tbl8_entry,
> +							__ATOMIC_RELAXED);
>  				}
>  			}
>  		}
> @@ -1490,7 +1498,9 @@ delete_depth_small_v1604(struct rte_lpm *lpm,
> uint32_t ip_masked,
> 
> 	RTE_LPM_TBL8_GROUP_NUM_ENTRIES); j++) {
> 
>  					if (lpm->tbl8[j].depth <= depth)
> -						lpm->tbl8[j] =
> new_tbl8_entry;
> +						__atomic_store(&lpm->tbl8[j],
> +							&new_tbl8_entry,
> +							__ATOMIC_RELAXED);
>  				}
>  			}
>  		}
> @@ -1646,7 +1656,8 @@ delete_depth_big_v20(struct rte_lpm_v20 *lpm,
> uint32_t ip_masked,
>  		 */
>  		for (i = tbl8_index; i < (tbl8_index + tbl8_range); i++) {
>  			if (lpm->tbl8[i].depth <= depth)
> -				lpm->tbl8[i] = new_tbl8_entry;
> +				__atomic_store(&lpm->tbl8[i],
> &new_tbl8_entry,
> +						__ATOMIC_RELAXED);
>  		}
>  	}
> 
> @@ -1677,7 +1688,8 @@ delete_depth_big_v20(struct rte_lpm_v20 *lpm,
> uint32_t ip_masked,
>  		/* Set tbl24 before freeing tbl8 to avoid race condition.
>  		 * Prevent the free of the tbl8 group from hoisting.
>  		 */
> -		lpm->tbl24[tbl24_index] = new_tbl24_entry;
> +		__atomic_store(&lpm->tbl24[tbl24_index],
> &new_tbl24_entry,
> +				__ATOMIC_RELAXED);
>  		__atomic_thread_fence(__ATOMIC_RELEASE);
>  		tbl8_free_v20(lpm->tbl8, tbl8_group_start);
tbl8_alloc_v20/tbl8_free_v20 need to be updated to use __atomic_store

>  	}
> @@ -1730,7 +1742,8 @@ delete_depth_big_v1604(struct rte_lpm *lpm,
> uint32_t ip_masked,
>  		 */
>  		for (i = tbl8_index; i < (tbl8_index + tbl8_range); i++) {
>  			if (lpm->tbl8[i].depth <= depth)
> -				lpm->tbl8[i] = new_tbl8_entry;
> +				__atomic_store(&lpm->tbl8[i],
> &new_tbl8_entry,
> +						__ATOMIC_RELAXED);
>  		}
>  	}
> 
> @@ -1761,7 +1774,8 @@ delete_depth_big_v1604(struct rte_lpm *lpm,
> uint32_t ip_masked,
>  		/* Set tbl24 before freeing tbl8 to avoid race condition.
>  		 * Prevent the free of the tbl8 group from hoisting.
>  		 */
> -		lpm->tbl24[tbl24_index] = new_tbl24_entry;
> +		__atomic_store(&lpm->tbl24[tbl24_index],
> &new_tbl24_entry,
> +				__ATOMIC_RELAXED);
>  		__atomic_thread_fence(__ATOMIC_RELEASE);
>  		tbl8_free_v1604(lpm->tbl8, tbl8_group_start);
tbl8_alloc_v1604 /tbl8_free_v1604 need to be updated to use __atomic_store

>  	}
> --
> 2.17.1


^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [dpdk-dev] [PATCH v4 3/3] lib/lpm: use atomic store to avoid partial update
  2019-07-05 16:53   ` Medvedkin, Vladimir
@ 2019-07-08  5:42     ` Ruifeng Wang (Arm Technology China)
  0 siblings, 0 replies; 13+ messages in thread
From: Ruifeng Wang (Arm Technology China) @ 2019-07-08  5:42 UTC (permalink / raw)
  To: Medvedkin, Vladimir, bruce.richardson
  Cc: dev, Honnappa Nagarahalli, Gavin Hu (Arm Technology China), nd, nd

Hi Vladimir,

> -----Original Message-----
> From: Medvedkin, Vladimir <vladimir.medvedkin@intel.com>
> Sent: Saturday, July 6, 2019 00:53
> To: Ruifeng Wang (Arm Technology China) <Ruifeng.Wang@arm.com>;
> bruce.richardson@intel.com
> Cc: dev@dpdk.org; Honnappa Nagarahalli
> <Honnappa.Nagarahalli@arm.com>; Gavin Hu (Arm Technology China)
> <Gavin.Hu@arm.com>; nd <nd@arm.com>
> Subject: Re: [PATCH v4 3/3] lib/lpm: use atomic store to avoid partial update
> 
> Hi Wang,
> 
> On 03/07/2019 06:44, Ruifeng Wang wrote:
> > Compiler could generate non-atomic stores for whole table entry
> > updating. This may cause incorrect nexthop to be returned, if the byte
> > with valid flag is updated prior to the byte with next hot is updated.
> >
> > Changed to use atomic store to update whole table entry.
> >
> > Suggested-by: Medvedkin Vladimir <vladimir.medvedkin@intel.com>
> > Signed-off-by: Ruifeng Wang <ruifeng.wang@arm.com>
> > Reviewed-by: Gavin Hu <gavin.hu@arm.com>
> > ---
> > v4: initial version
> >
> >   lib/librte_lpm/rte_lpm.c | 34 ++++++++++++++++++++++++----------
> >   1 file changed, 24 insertions(+), 10 deletions(-)
> >
> > diff --git a/lib/librte_lpm/rte_lpm.c b/lib/librte_lpm/rte_lpm.c index
> > baa6e7460..5d1dbd7e6 100644
> > --- a/lib/librte_lpm/rte_lpm.c
> > +++ b/lib/librte_lpm/rte_lpm.c
> > @@ -767,7 +767,9 @@ add_depth_small_v20(struct rte_lpm_v20 *lpm,
> uint32_t ip, uint8_t depth,
> >   					 * Setting tbl8 entry in one go to avoid
> >   					 * race conditions
> >   					 */
> > -					lpm->tbl8[j] = new_tbl8_entry;
> > +					__atomic_store(&lpm->tbl8[j],
> > +						&new_tbl8_entry,
> > +						__ATOMIC_RELAXED);
> >
> >   					continue;
> >   				}
> > @@ -837,7 +839,9 @@ add_depth_small_v1604(struct rte_lpm *lpm,
> uint32_t ip, uint8_t depth,
> >   					 * Setting tbl8 entry in one go to avoid
> >   					 * race conditions
> >   					 */
> > -					lpm->tbl8[j] = new_tbl8_entry;
> > +					__atomic_store(&lpm->tbl8[j],
> > +						&new_tbl8_entry,
> > +						__ATOMIC_RELAXED);
> >
> >   					continue;
> >   				}
> > @@ -965,7 +969,8 @@ add_depth_big_v20(struct rte_lpm_v20 *lpm,
> uint32_t ip_masked, uint8_t depth,
> >   				 * Setting tbl8 entry in one go to avoid race
> >   				 * condition
> >   				 */
> > -				lpm->tbl8[i] = new_tbl8_entry;
> > +				__atomic_store(&lpm->tbl8[i],
> &new_tbl8_entry,
> > +						__ATOMIC_RELAXED);
> >
> >   				continue;
> >   			}
> > @@ -1100,7 +1105,8 @@ add_depth_big_v1604(struct rte_lpm *lpm,
> uint32_t ip_masked, uint8_t depth,
> >   				 * Setting tbl8 entry in one go to avoid race
> >   				 * condition
> >   				 */
> > -				lpm->tbl8[i] = new_tbl8_entry;
> > +				__atomic_store(&lpm->tbl8[i],
> &new_tbl8_entry,
> > +						__ATOMIC_RELAXED);
> >
> >   				continue;
> >   			}
> > @@ -1393,7 +1399,9 @@ delete_depth_small_v20(struct rte_lpm_v20
> *lpm, uint32_t ip_masked,
> >
> 	RTE_LPM_TBL8_GROUP_NUM_ENTRIES); j++) {
> >
> >   					if (lpm->tbl8[j].depth <= depth)
> > -						lpm->tbl8[j] =
> new_tbl8_entry;
> > +						__atomic_store(&lpm-
> >tbl8[j],
> > +							&new_tbl8_entry,
> > +							__ATOMIC_RELAXED);
> >   				}
> >   			}
> >   		}
> > @@ -1490,7 +1498,9 @@ delete_depth_small_v1604(struct rte_lpm *lpm,
> uint32_t ip_masked,
> >
> 	RTE_LPM_TBL8_GROUP_NUM_ENTRIES); j++) {
> >
> >   					if (lpm->tbl8[j].depth <= depth)
> > -						lpm->tbl8[j] =
> new_tbl8_entry;
> > +						__atomic_store(&lpm-
> >tbl8[j],
> > +							&new_tbl8_entry,
> > +							__ATOMIC_RELAXED);
> >   				}
> >   			}
> >   		}
> > @@ -1646,7 +1656,8 @@ delete_depth_big_v20(struct rte_lpm_v20 *lpm,
> uint32_t ip_masked,
> >   		 */
> >   		for (i = tbl8_index; i < (tbl8_index + tbl8_range); i++) {
> >   			if (lpm->tbl8[i].depth <= depth)
> > -				lpm->tbl8[i] = new_tbl8_entry;
> > +				__atomic_store(&lpm->tbl8[i],
> &new_tbl8_entry,
> > +						__ATOMIC_RELAXED);
> >   		}
> >   	}
> >
> > @@ -1677,7 +1688,8 @@ delete_depth_big_v20(struct rte_lpm_v20 *lpm,
> uint32_t ip_masked,
> >   		/* Set tbl24 before freeing tbl8 to avoid race condition.
> >   		 * Prevent the free of the tbl8 group from hoisting.
> >   		 */
> > -		lpm->tbl24[tbl24_index] = new_tbl24_entry;
> > +		__atomic_store(&lpm->tbl24[tbl24_index],
> &new_tbl24_entry,
> > +				__ATOMIC_RELAXED);
> >   		__atomic_thread_fence(__ATOMIC_RELEASE);
> >   		tbl8_free_v20(lpm->tbl8, tbl8_group_start);
> >   	}
> > @@ -1730,7 +1742,8 @@ delete_depth_big_v1604(struct rte_lpm *lpm,
> uint32_t ip_masked,
> >   		 */
> >   		for (i = tbl8_index; i < (tbl8_index + tbl8_range); i++) {
> >   			if (lpm->tbl8[i].depth <= depth)
> > -				lpm->tbl8[i] = new_tbl8_entry;
> > +				__atomic_store(&lpm->tbl8[i],
> &new_tbl8_entry,
> > +						__ATOMIC_RELAXED);
> >   		}
> >   	}
> >
> > @@ -1761,7 +1774,8 @@ delete_depth_big_v1604(struct rte_lpm *lpm,
> uint32_t ip_masked,
> >   		/* Set tbl24 before freeing tbl8 to avoid race condition.
> >   		 * Prevent the free of the tbl8 group from hoisting.
> >   		 */
> > -		lpm->tbl24[tbl24_index] = new_tbl24_entry;
> > +		__atomic_store(&lpm->tbl24[tbl24_index],
> &new_tbl24_entry,
> > +				__ATOMIC_RELAXED);
> >   		__atomic_thread_fence(__ATOMIC_RELEASE);
> Do you really need __atomic_thread_fence after atomic_store?

Yes. The __atomic_thread_fence here is to prevent the free of the tbl8 group below from hoisting.
The order of tbl24 update and tbl8 update needs to be kept.

> >   		tbl8_free_v1604(lpm->tbl8, tbl8_group_start);
> >   	}
> 
> --
> Regards,
> Vladimir


^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [dpdk-dev] [PATCH v4 3/3] lib/lpm: use atomic store to avoid partial update
  2019-07-08  4:56   ` Honnappa Nagarahalli
@ 2019-07-08  6:01     ` Ruifeng Wang (Arm Technology China)
  2019-07-09  4:43       ` Honnappa Nagarahalli
  0 siblings, 1 reply; 13+ messages in thread
From: Ruifeng Wang (Arm Technology China) @ 2019-07-08  6:01 UTC (permalink / raw)
  To: Honnappa Nagarahalli, vladimir.medvedkin, bruce.richardson
  Cc: dev, Gavin Hu (Arm Technology China), nd, nd, nd

Hi Honnappa,

> -----Original Message-----
> From: Honnappa Nagarahalli <Honnappa.Nagarahalli@arm.com>
> Sent: Monday, July 8, 2019 12:57
> To: Ruifeng Wang (Arm Technology China) <Ruifeng.Wang@arm.com>;
> vladimir.medvedkin@intel.com; bruce.richardson@intel.com
> Cc: dev@dpdk.org; Gavin Hu (Arm Technology China) <Gavin.Hu@arm.com>;
> nd <nd@arm.com>; Ruifeng Wang (Arm Technology China)
> <Ruifeng.Wang@arm.com>; Honnappa Nagarahalli
> <Honnappa.Nagarahalli@arm.com>; nd <nd@arm.com>
> Subject: RE: [PATCH v4 3/3] lib/lpm: use atomic store to avoid partial update
> 
> >
> > Compiler could generate non-atomic stores for whole table entry updating.
> > This may cause incorrect nexthop to be returned, if the byte with
> > valid flag is updated prior to the byte with next hot is updated.
>                                                            ^^^^^^^ Should be nexthop
> 
> >
> > Changed to use atomic store to update whole table entry.
> >
> > Suggested-by: Medvedkin Vladimir <vladimir.medvedkin@intel.com>
> > Signed-off-by: Ruifeng Wang <ruifeng.wang@arm.com>
> > Reviewed-by: Gavin Hu <gavin.hu@arm.com>
> > ---
> > v4: initial version
> >
> >  lib/librte_lpm/rte_lpm.c | 34 ++++++++++++++++++++++++----------
> >  1 file changed, 24 insertions(+), 10 deletions(-)
> >
> > diff --git a/lib/librte_lpm/rte_lpm.c b/lib/librte_lpm/rte_lpm.c index
> > baa6e7460..5d1dbd7e6 100644
> > --- a/lib/librte_lpm/rte_lpm.c
> > +++ b/lib/librte_lpm/rte_lpm.c
> > @@ -767,7 +767,9 @@ add_depth_small_v20(struct rte_lpm_v20 *lpm,
> > uint32_t ip, uint8_t depth,
> >  					 * Setting tbl8 entry in one go to avoid
> >  					 * race conditions
> >  					 */
> > -					lpm->tbl8[j] = new_tbl8_entry;
> > +					__atomic_store(&lpm->tbl8[j],
> > +						&new_tbl8_entry,
> > +						__ATOMIC_RELAXED);
> >
> >  					continue;
> >  				}
> > @@ -837,7 +839,9 @@ add_depth_small_v1604(struct rte_lpm *lpm,
> > uint32_t ip, uint8_t depth,
> >  					 * Setting tbl8 entry in one go to avoid
> >  					 * race conditions
> >  					 */
> > -					lpm->tbl8[j] = new_tbl8_entry;
> > +					__atomic_store(&lpm->tbl8[j],
> > +						&new_tbl8_entry,
> > +						__ATOMIC_RELAXED);
> >
> >  					continue;
> >  				}
> > @@ -965,7 +969,8 @@ add_depth_big_v20(struct rte_lpm_v20 *lpm,
> > uint32_t ip_masked, uint8_t depth,
> >  				 * Setting tbl8 entry in one go to avoid race
> >  				 * condition
> >  				 */
> > -				lpm->tbl8[i] = new_tbl8_entry;
> > +				__atomic_store(&lpm->tbl8[i],
> > &new_tbl8_entry,
> > +						__ATOMIC_RELAXED);
> >
> >  				continue;
> >  			}
> > @@ -1100,7 +1105,8 @@ add_depth_big_v1604(struct rte_lpm *lpm,
> > uint32_t ip_masked, uint8_t depth,
> >  				 * Setting tbl8 entry in one go to avoid race
> >  				 * condition
> >  				 */
> > -				lpm->tbl8[i] = new_tbl8_entry;
> > +				__atomic_store(&lpm->tbl8[i],
> > &new_tbl8_entry,
> > +						__ATOMIC_RELAXED);
> >
> >  				continue;
> >  			}
> > @@ -1393,7 +1399,9 @@ delete_depth_small_v20(struct rte_lpm_v20
> *lpm,
> > uint32_t ip_masked,
> >
> > 	RTE_LPM_TBL8_GROUP_NUM_ENTRIES); j++) {
> >
> >  					if (lpm->tbl8[j].depth <= depth)
> > -						lpm->tbl8[j] =
> > new_tbl8_entry;
> > +						__atomic_store(&lpm-
> >tbl8[j],
> > +							&new_tbl8_entry,
> > +							__ATOMIC_RELAXED);
> >  				}
> >  			}
> >  		}
> > @@ -1490,7 +1498,9 @@ delete_depth_small_v1604(struct rte_lpm *lpm,
> > uint32_t ip_masked,
> >
> > 	RTE_LPM_TBL8_GROUP_NUM_ENTRIES); j++) {
> >
> >  					if (lpm->tbl8[j].depth <= depth)
> > -						lpm->tbl8[j] =
> > new_tbl8_entry;
> > +						__atomic_store(&lpm-
> >tbl8[j],
> > +							&new_tbl8_entry,
> > +							__ATOMIC_RELAXED);
> >  				}
> >  			}
> >  		}
> > @@ -1646,7 +1656,8 @@ delete_depth_big_v20(struct rte_lpm_v20 *lpm,
> > uint32_t ip_masked,
> >  		 */
> >  		for (i = tbl8_index; i < (tbl8_index + tbl8_range); i++) {
> >  			if (lpm->tbl8[i].depth <= depth)
> > -				lpm->tbl8[i] = new_tbl8_entry;
> > +				__atomic_store(&lpm->tbl8[i],
> > &new_tbl8_entry,
> > +						__ATOMIC_RELAXED);
> >  		}
> >  	}
> >
> > @@ -1677,7 +1688,8 @@ delete_depth_big_v20(struct rte_lpm_v20 *lpm,
> > uint32_t ip_masked,
> >  		/* Set tbl24 before freeing tbl8 to avoid race condition.
> >  		 * Prevent the free of the tbl8 group from hoisting.
> >  		 */
> > -		lpm->tbl24[tbl24_index] = new_tbl24_entry;
> > +		__atomic_store(&lpm->tbl24[tbl24_index],
> > &new_tbl24_entry,
> > +				__ATOMIC_RELAXED);
> >  		__atomic_thread_fence(__ATOMIC_RELEASE);
> >  		tbl8_free_v20(lpm->tbl8, tbl8_group_start);
> tbl8_alloc_v20/tbl8_free_v20 need to be updated to use __atomic_store
> 
tbl8_alloc_v20/tbl8_free_v20 updates a single field of table entry. The process
is already atomic. Do we really need to use __atomic_store?

> >  	}
> > @@ -1730,7 +1742,8 @@ delete_depth_big_v1604(struct rte_lpm *lpm,
> > uint32_t ip_masked,
> >  		 */
> >  		for (i = tbl8_index; i < (tbl8_index + tbl8_range); i++) {
> >  			if (lpm->tbl8[i].depth <= depth)
> > -				lpm->tbl8[i] = new_tbl8_entry;
> > +				__atomic_store(&lpm->tbl8[i],
> > &new_tbl8_entry,
> > +						__ATOMIC_RELAXED);
> >  		}
> >  	}
> >
> > @@ -1761,7 +1774,8 @@ delete_depth_big_v1604(struct rte_lpm *lpm,
> > uint32_t ip_masked,
> >  		/* Set tbl24 before freeing tbl8 to avoid race condition.
> >  		 * Prevent the free of the tbl8 group from hoisting.
> >  		 */
> > -		lpm->tbl24[tbl24_index] = new_tbl24_entry;
> > +		__atomic_store(&lpm->tbl24[tbl24_index],
> > &new_tbl24_entry,
> > +				__ATOMIC_RELAXED);
> >  		__atomic_thread_fence(__ATOMIC_RELEASE);
> >  		tbl8_free_v1604(lpm->tbl8, tbl8_group_start);
> tbl8_alloc_v1604 /tbl8_free_v1604 need to be updated to use
> __atomic_store
Ditto.

> 
> >  	}
> > --
> > 2.17.1


^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [dpdk-dev] [PATCH v4 3/3] lib/lpm: use atomic store to avoid partial update
  2019-07-08  6:01     ` Ruifeng Wang (Arm Technology China)
@ 2019-07-09  4:43       ` Honnappa Nagarahalli
  2019-07-09  9:58         ` Ruifeng Wang (Arm Technology China)
  0 siblings, 1 reply; 13+ messages in thread
From: Honnappa Nagarahalli @ 2019-07-09  4:43 UTC (permalink / raw)
  To: Ruifeng Wang (Arm Technology China),
	vladimir.medvedkin, bruce.richardson
  Cc: dev, Gavin Hu (Arm Technology China), Honnappa Nagarahalli, nd, nd

> >
> > >
> > > Compiler could generate non-atomic stores for whole table entry updating.
> > > This may cause incorrect nexthop to be returned, if the byte with
> > > valid flag is updated prior to the byte with next hot is updated.
> >                                                            ^^^^^^^
> > Should be nexthop
> >
> > >
> > > Changed to use atomic store to update whole table entry.
> > >
> > > Suggested-by: Medvedkin Vladimir <vladimir.medvedkin@intel.com>
> > > Signed-off-by: Ruifeng Wang <ruifeng.wang@arm.com>
> > > Reviewed-by: Gavin Hu <gavin.hu@arm.com>
> > > ---
> > > v4: initial version
> > >
> > >  lib/librte_lpm/rte_lpm.c | 34 ++++++++++++++++++++++++----------
> > >  1 file changed, 24 insertions(+), 10 deletions(-)
> > >
> > > diff --git a/lib/librte_lpm/rte_lpm.c b/lib/librte_lpm/rte_lpm.c
> > > index
> > > baa6e7460..5d1dbd7e6 100644
> > > --- a/lib/librte_lpm/rte_lpm.c
> > > +++ b/lib/librte_lpm/rte_lpm.c
> > > @@ -767,7 +767,9 @@ add_depth_small_v20(struct rte_lpm_v20 *lpm,
> > > uint32_t ip, uint8_t depth,
> > >  					 * Setting tbl8 entry in one go to avoid
> > >  					 * race conditions
> > >  					 */
> > > -					lpm->tbl8[j] = new_tbl8_entry;
> > > +					__atomic_store(&lpm->tbl8[j],
> > > +						&new_tbl8_entry,
> > > +						__ATOMIC_RELAXED);
> > >
> > >  					continue;
> > >  				}
> > > @@ -837,7 +839,9 @@ add_depth_small_v1604(struct rte_lpm *lpm,
> > > uint32_t ip, uint8_t depth,
> > >  					 * Setting tbl8 entry in one go to avoid
> > >  					 * race conditions
> > >  					 */
> > > -					lpm->tbl8[j] = new_tbl8_entry;
> > > +					__atomic_store(&lpm->tbl8[j],
> > > +						&new_tbl8_entry,
> > > +						__ATOMIC_RELAXED);
> > >
> > >  					continue;
> > >  				}
> > > @@ -965,7 +969,8 @@ add_depth_big_v20(struct rte_lpm_v20 *lpm,
> > > uint32_t ip_masked, uint8_t depth,
> > >  				 * Setting tbl8 entry in one go to avoid race
> > >  				 * condition
> > >  				 */
> > > -				lpm->tbl8[i] = new_tbl8_entry;
> > > +				__atomic_store(&lpm->tbl8[i],
> > > &new_tbl8_entry,
> > > +						__ATOMIC_RELAXED);
> > >
> > >  				continue;
> > >  			}
> > > @@ -1100,7 +1105,8 @@ add_depth_big_v1604(struct rte_lpm *lpm,
> > > uint32_t ip_masked, uint8_t depth,
> > >  				 * Setting tbl8 entry in one go to avoid race
> > >  				 * condition
> > >  				 */
> > > -				lpm->tbl8[i] = new_tbl8_entry;
> > > +				__atomic_store(&lpm->tbl8[i],
> > > &new_tbl8_entry,
> > > +						__ATOMIC_RELAXED);
> > >
> > >  				continue;
> > >  			}
> > > @@ -1393,7 +1399,9 @@ delete_depth_small_v20(struct rte_lpm_v20
> > *lpm,
> > > uint32_t ip_masked,
> > >
> > > 	RTE_LPM_TBL8_GROUP_NUM_ENTRIES); j++) {
> > >
> > >  					if (lpm->tbl8[j].depth <= depth)
> > > -						lpm->tbl8[j] =
> > > new_tbl8_entry;
> > > +						__atomic_store(&lpm-
> > >tbl8[j],
> > > +							&new_tbl8_entry,
> > > +							__ATOMIC_RELAXED);
> > >  				}
> > >  			}
> > >  		}
> > > @@ -1490,7 +1498,9 @@ delete_depth_small_v1604(struct rte_lpm *lpm,
> > > uint32_t ip_masked,
> > >
> > > 	RTE_LPM_TBL8_GROUP_NUM_ENTRIES); j++) {
> > >
> > >  					if (lpm->tbl8[j].depth <= depth)
> > > -						lpm->tbl8[j] =
> > > new_tbl8_entry;
> > > +						__atomic_store(&lpm-
> > >tbl8[j],
> > > +							&new_tbl8_entry,
> > > +							__ATOMIC_RELAXED);
> > >  				}
> > >  			}
> > >  		}
> > > @@ -1646,7 +1656,8 @@ delete_depth_big_v20(struct rte_lpm_v20 *lpm,
> > > uint32_t ip_masked,
> > >  		 */
> > >  		for (i = tbl8_index; i < (tbl8_index + tbl8_range); i++) {
> > >  			if (lpm->tbl8[i].depth <= depth)
> > > -				lpm->tbl8[i] = new_tbl8_entry;
> > > +				__atomic_store(&lpm->tbl8[i],
> > > &new_tbl8_entry,
> > > +						__ATOMIC_RELAXED);
> > >  		}
> > >  	}
> > >
> > > @@ -1677,7 +1688,8 @@ delete_depth_big_v20(struct rte_lpm_v20 *lpm,
> > > uint32_t ip_masked,
> > >  		/* Set tbl24 before freeing tbl8 to avoid race condition.
> > >  		 * Prevent the free of the tbl8 group from hoisting.
> > >  		 */
> > > -		lpm->tbl24[tbl24_index] = new_tbl24_entry;
> > > +		__atomic_store(&lpm->tbl24[tbl24_index],
> > > &new_tbl24_entry,
> > > +				__ATOMIC_RELAXED);
> > >  		__atomic_thread_fence(__ATOMIC_RELEASE);
> > >  		tbl8_free_v20(lpm->tbl8, tbl8_group_start);
> > tbl8_alloc_v20/tbl8_free_v20 need to be updated to use __atomic_store
> >
> tbl8_alloc_v20/tbl8_free_v20 updates a single field of table entry. The process
> is already atomic. Do we really need to use __atomic_store?
I thought we agreed that all the tbl8 stores will use __atomic_store.
IMO, it is better to use C11 atomic built-ins entirely, at least for the data structures used in reader-writer scenario. Otherwise, the code does not follow C11 memory model completely. (I do not know what to call such a model).

> 
> > >  	}
> > > @@ -1730,7 +1742,8 @@ delete_depth_big_v1604(struct rte_lpm *lpm,
> > > uint32_t ip_masked,
> > >  		 */
> > >  		for (i = tbl8_index; i < (tbl8_index + tbl8_range); i++) {
> > >  			if (lpm->tbl8[i].depth <= depth)
> > > -				lpm->tbl8[i] = new_tbl8_entry;
> > > +				__atomic_store(&lpm->tbl8[i],
> > > &new_tbl8_entry,
> > > +						__ATOMIC_RELAXED);
> > >  		}
> > >  	}
> > >
> > > @@ -1761,7 +1774,8 @@ delete_depth_big_v1604(struct rte_lpm *lpm,
> > > uint32_t ip_masked,
> > >  		/* Set tbl24 before freeing tbl8 to avoid race condition.
> > >  		 * Prevent the free of the tbl8 group from hoisting.
> > >  		 */
> > > -		lpm->tbl24[tbl24_index] = new_tbl24_entry;
> > > +		__atomic_store(&lpm->tbl24[tbl24_index],
> > > &new_tbl24_entry,
> > > +				__ATOMIC_RELAXED);
> > >  		__atomic_thread_fence(__ATOMIC_RELEASE);
> > >  		tbl8_free_v1604(lpm->tbl8, tbl8_group_start);
> > tbl8_alloc_v1604 /tbl8_free_v1604 need to be updated to use
> > __atomic_store
> Ditto.
> 
> >
> > >  	}
> > > --
> > > 2.17.1


^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [dpdk-dev] [PATCH v4 3/3] lib/lpm: use atomic store to avoid partial update
  2019-07-09  4:43       ` Honnappa Nagarahalli
@ 2019-07-09  9:58         ` Ruifeng Wang (Arm Technology China)
  0 siblings, 0 replies; 13+ messages in thread
From: Ruifeng Wang (Arm Technology China) @ 2019-07-09  9:58 UTC (permalink / raw)
  To: Honnappa Nagarahalli, vladimir.medvedkin, bruce.richardson
  Cc: dev, Gavin Hu (Arm Technology China), nd, nd, nd


> -----Original Message-----
> From: Honnappa Nagarahalli <Honnappa.Nagarahalli@arm.com>
> Sent: Tuesday, July 9, 2019 12:43
> To: Ruifeng Wang (Arm Technology China) <Ruifeng.Wang@arm.com>;
> vladimir.medvedkin@intel.com; bruce.richardson@intel.com
> Cc: dev@dpdk.org; Gavin Hu (Arm Technology China) <Gavin.Hu@arm.com>;
> Honnappa Nagarahalli <Honnappa.Nagarahalli@arm.com>; nd
> <nd@arm.com>; nd <nd@arm.com>
> Subject: RE: [PATCH v4 3/3] lib/lpm: use atomic store to avoid partial update
> 
> > >
> > > >
> > > > Compiler could generate non-atomic stores for whole table entry
> updating.
> > > > This may cause incorrect nexthop to be returned, if the byte with
> > > > valid flag is updated prior to the byte with next hot is updated.
> > >                                                            ^^^^^^^
> > > Should be nexthop
> > >
> > > >
> > > > Changed to use atomic store to update whole table entry.
> > > >
> > > > Suggested-by: Medvedkin Vladimir <vladimir.medvedkin@intel.com>
> > > > Signed-off-by: Ruifeng Wang <ruifeng.wang@arm.com>
> > > > Reviewed-by: Gavin Hu <gavin.hu@arm.com>
> > > > ---
> > > > v4: initial version
> > > >
> > > >  lib/librte_lpm/rte_lpm.c | 34 ++++++++++++++++++++++++----------
> > > >  1 file changed, 24 insertions(+), 10 deletions(-)
> > > >
> > > > diff --git a/lib/librte_lpm/rte_lpm.c b/lib/librte_lpm/rte_lpm.c
> > > > index
> > > > baa6e7460..5d1dbd7e6 100644
> > > > --- a/lib/librte_lpm/rte_lpm.c
> > > > +++ b/lib/librte_lpm/rte_lpm.c
> > > > @@ -767,7 +767,9 @@ add_depth_small_v20(struct rte_lpm_v20 *lpm,
> > > > uint32_t ip, uint8_t depth,
> > > >  					 * Setting tbl8 entry in one go to avoid
> > > >  					 * race conditions
> > > >  					 */
> > > > -					lpm->tbl8[j] = new_tbl8_entry;
> > > > +					__atomic_store(&lpm->tbl8[j],
> > > > +						&new_tbl8_entry,
> > > > +						__ATOMIC_RELAXED);
> > > >
> > > >  					continue;
> > > >  				}
> > > > @@ -837,7 +839,9 @@ add_depth_small_v1604(struct rte_lpm *lpm,
> > > > uint32_t ip, uint8_t depth,
> > > >  					 * Setting tbl8 entry in one go to avoid
> > > >  					 * race conditions
> > > >  					 */
> > > > -					lpm->tbl8[j] = new_tbl8_entry;
> > > > +					__atomic_store(&lpm->tbl8[j],
> > > > +						&new_tbl8_entry,
> > > > +						__ATOMIC_RELAXED);
> > > >
> > > >  					continue;
> > > >  				}
> > > > @@ -965,7 +969,8 @@ add_depth_big_v20(struct rte_lpm_v20 *lpm,
> > > > uint32_t ip_masked, uint8_t depth,
> > > >  				 * Setting tbl8 entry in one go to avoid race
> > > >  				 * condition
> > > >  				 */
> > > > -				lpm->tbl8[i] = new_tbl8_entry;
> > > > +				__atomic_store(&lpm->tbl8[i],
> > > > &new_tbl8_entry,
> > > > +						__ATOMIC_RELAXED);
> > > >
> > > >  				continue;
> > > >  			}
> > > > @@ -1100,7 +1105,8 @@ add_depth_big_v1604(struct rte_lpm *lpm,
> > > > uint32_t ip_masked, uint8_t depth,
> > > >  				 * Setting tbl8 entry in one go to avoid race
> > > >  				 * condition
> > > >  				 */
> > > > -				lpm->tbl8[i] = new_tbl8_entry;
> > > > +				__atomic_store(&lpm->tbl8[i],
> > > > &new_tbl8_entry,
> > > > +						__ATOMIC_RELAXED);
> > > >
> > > >  				continue;
> > > >  			}
> > > > @@ -1393,7 +1399,9 @@ delete_depth_small_v20(struct rte_lpm_v20
> > > *lpm,
> > > > uint32_t ip_masked,
> > > >
> > > > 	RTE_LPM_TBL8_GROUP_NUM_ENTRIES); j++) {
> > > >
> > > >  					if (lpm->tbl8[j].depth <= depth)
> > > > -						lpm->tbl8[j] =
> > > > new_tbl8_entry;
> > > > +						__atomic_store(&lpm-
> > > >tbl8[j],
> > > > +							&new_tbl8_entry,
> > > > +							__ATOMIC_RELAXED);
> > > >  				}
> > > >  			}
> > > >  		}
> > > > @@ -1490,7 +1498,9 @@ delete_depth_small_v1604(struct rte_lpm
> > > > *lpm, uint32_t ip_masked,
> > > >
> > > > 	RTE_LPM_TBL8_GROUP_NUM_ENTRIES); j++) {
> > > >
> > > >  					if (lpm->tbl8[j].depth <= depth)
> > > > -						lpm->tbl8[j] =
> > > > new_tbl8_entry;
> > > > +						__atomic_store(&lpm-
> > > >tbl8[j],
> > > > +							&new_tbl8_entry,
> > > > +							__ATOMIC_RELAXED);
> > > >  				}
> > > >  			}
> > > >  		}
> > > > @@ -1646,7 +1656,8 @@ delete_depth_big_v20(struct rte_lpm_v20
> > > > *lpm, uint32_t ip_masked,
> > > >  		 */
> > > >  		for (i = tbl8_index; i < (tbl8_index + tbl8_range); i++) {
> > > >  			if (lpm->tbl8[i].depth <= depth)
> > > > -				lpm->tbl8[i] = new_tbl8_entry;
> > > > +				__atomic_store(&lpm->tbl8[i],
> > > > &new_tbl8_entry,
> > > > +						__ATOMIC_RELAXED);
> > > >  		}
> > > >  	}
> > > >
> > > > @@ -1677,7 +1688,8 @@ delete_depth_big_v20(struct rte_lpm_v20
> > > > *lpm, uint32_t ip_masked,
> > > >  		/* Set tbl24 before freeing tbl8 to avoid race condition.
> > > >  		 * Prevent the free of the tbl8 group from hoisting.
> > > >  		 */
> > > > -		lpm->tbl24[tbl24_index] = new_tbl24_entry;
> > > > +		__atomic_store(&lpm->tbl24[tbl24_index],
> > > > &new_tbl24_entry,
> > > > +				__ATOMIC_RELAXED);
> > > >  		__atomic_thread_fence(__ATOMIC_RELEASE);
> > > >  		tbl8_free_v20(lpm->tbl8, tbl8_group_start);
> > > tbl8_alloc_v20/tbl8_free_v20 need to be updated to use
> > > __atomic_store
> > >
> > tbl8_alloc_v20/tbl8_free_v20 updates a single field of table entry.
> > The process is already atomic. Do we really need to use __atomic_store?
> I thought we agreed that all the tbl8 stores will use __atomic_store.
> IMO, it is better to use C11 atomic built-ins entirely, at least for the data
> structures used in reader-writer scenario. Otherwise, the code does not
> follow C11 memory model completely. (I do not know what to call such a
> model).
> 
OK, change will be made in next version.

> >
> > > >  	}
> > > > @@ -1730,7 +1742,8 @@ delete_depth_big_v1604(struct rte_lpm *lpm,
> > > > uint32_t ip_masked,
> > > >  		 */
> > > >  		for (i = tbl8_index; i < (tbl8_index + tbl8_range); i++) {
> > > >  			if (lpm->tbl8[i].depth <= depth)
> > > > -				lpm->tbl8[i] = new_tbl8_entry;
> > > > +				__atomic_store(&lpm->tbl8[i],
> > > > &new_tbl8_entry,
> > > > +						__ATOMIC_RELAXED);
> > > >  		}
> > > >  	}
> > > >
> > > > @@ -1761,7 +1774,8 @@ delete_depth_big_v1604(struct rte_lpm *lpm,
> > > > uint32_t ip_masked,
> > > >  		/* Set tbl24 before freeing tbl8 to avoid race condition.
> > > >  		 * Prevent the free of the tbl8 group from hoisting.
> > > >  		 */
> > > > -		lpm->tbl24[tbl24_index] = new_tbl24_entry;
> > > > +		__atomic_store(&lpm->tbl24[tbl24_index],
> > > > &new_tbl24_entry,
> > > > +				__ATOMIC_RELAXED);
> > > >  		__atomic_thread_fence(__ATOMIC_RELEASE);
> > > >  		tbl8_free_v1604(lpm->tbl8, tbl8_group_start);
> > > tbl8_alloc_v1604 /tbl8_free_v1604 need to be updated to use
> > > __atomic_store
> > Ditto.
> >
> > >
> > > >  	}
> > > > --
> > > > 2.17.1


^ permalink raw reply	[flat|nested] 13+ messages in thread

end of thread, other threads:[~2019-07-09  9:58 UTC | newest]

Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-07-03  5:44 [dpdk-dev] [PATCH v4 1/3] lib/lpm: memory orderings to avoid race conditions for v1604 Ruifeng Wang
2019-07-03  5:44 ` [dpdk-dev] [PATCH v4 2/3] lib/lpm: memory orderings to avoid race conditions for v20 Ruifeng Wang
2019-07-05 16:52   ` Medvedkin, Vladimir
2019-07-05 18:20     ` Vladimir Medvedkin
2019-07-03  5:44 ` [dpdk-dev] [PATCH v4 3/3] lib/lpm: use atomic store to avoid partial update Ruifeng Wang
2019-07-05 16:53   ` Medvedkin, Vladimir
2019-07-08  5:42     ` Ruifeng Wang (Arm Technology China)
2019-07-08  4:56   ` Honnappa Nagarahalli
2019-07-08  6:01     ` Ruifeng Wang (Arm Technology China)
2019-07-09  4:43       ` Honnappa Nagarahalli
2019-07-09  9:58         ` Ruifeng Wang (Arm Technology China)
2019-07-04 20:25 ` [dpdk-dev] [PATCH v4 1/3] lib/lpm: memory orderings to avoid race conditions for v1604 Thomas Monjalon
2019-07-05  6:39   ` Ruifeng Wang (Arm Technology China)

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).