[dpdk-dev] [PATCH v4 1/3] lib/lpm: memory orderings to avoid race conditions for v1604

[dpdk-dev] [PATCH v4 1/3] lib/lpm: memory orderings to avoid race conditions for v1604

[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

[dpdk-dev] [PATCH v4 2/3] lib/lpm: memory orderings to avoid race conditions for v20

[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

[dpdk-dev] [PATCH v4 3/3] lib/lpm: use atomic store to avoid partial update

[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

Re: [dpdk-dev] [PATCH v4 1/3] lib/lpm: memory orderings to avoid race conditions for v1604

[Thomas Monjalon]

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

Re: [dpdk-dev] [PATCH v4 1/3] lib/lpm: memory orderings to avoid race conditions for v1604

[Ruifeng Wang (Arm Technology China)]

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

Re: [dpdk-dev] [PATCH v4 2/3] lib/lpm: memory orderings to avoid race conditions for v20

[Medvedkin, Vladimir]

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

Re: [dpdk-dev] [PATCH v4 3/3] lib/lpm: use atomic store to avoid partial update

[Medvedkin, Vladimir]

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

Re: [dpdk-dev] [PATCH v4 2/3] lib/lpm: memory orderings to avoid race conditions for v20

[Vladimir Medvedkin]

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

Re: [dpdk-dev] [PATCH v4 3/3] lib/lpm: use atomic store to avoid partial update

[Honnappa Nagarahalli]

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

Re: [dpdk-dev] [PATCH v4 3/3] lib/lpm: use atomic store to avoid partial update

[Ruifeng Wang (Arm Technology China)]

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

Re: [dpdk-dev] [PATCH v4 3/3] lib/lpm: use atomic store to avoid partial update

[Ruifeng Wang (Arm Technology China)]

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

Re: [dpdk-dev] [PATCH v4 3/3] lib/lpm: use atomic store to avoid partial update

[Honnappa Nagarahalli]

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

Re: [dpdk-dev] [PATCH v4 3/3] lib/lpm: use atomic store to avoid partial update

[Ruifeng Wang (Arm Technology China)]

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