From: "Medvedkin, Vladimir" <vladimir.medvedkin@intel.com>
To: Ruifeng Wang <ruifeng.wang@arm.com>, bruce.richardson@intel.com
Cc: dev@dpdk.org, honnappa.nagarahalli@arm.com, gavin.hu@arm.com, nd@arm.com
Subject: Re: [dpdk-dev] [PATCH v3 3/3] lib/lpm: memory orderings to avoid race conditions for v20
Date: Fri, 28 Jun 2019 14:33:47 +0100 [thread overview]
Message-ID: <6daf7a76-1def-21df-d202-10b2a8f8582a@intel.com> (raw)
In-Reply-To: <20190627093751.7746-3-ruifeng.wang@arm.com>
Hi Wang,
On 27/06/2019 10:37, 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.
>
> 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>
> ---
> v3: no changes
> v2: fixed clang building issue by supplying alignment attribute.
> v1: initail version
>
> lib/librte_lpm/rte_lpm.c | 31 ++++++++++++++++++++++++-------
> lib/librte_lpm/rte_lpm.h | 4 ++--
> 2 files changed, 26 insertions(+), 9 deletions(-)
>
> diff --git a/lib/librte_lpm/rte_lpm.c b/lib/librte_lpm/rte_lpm.c
> index fabd13fb0..5f8d494ae 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,14 @@ 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_tbl_entry = {
> + .valid = INVALID,
> + .depth = 0,
> + .valid_group = 0,
> + };
> + zero_tbl_entry.next_hop = 0;
Please use the same var name in both v20 and v1604 (zero_tbl24_entry).
The same is for struct initialization. In 1604 you use:
struct rte_lpm_tbl_entry zero_tbl24_entry = {0};
> + __atomic_store(&lpm->tbl24[i], &zero_tbl_entry,
> + __ATOMIC_RELEASE);
> } else if (lpm->tbl24[i].valid_group == 1) {
> /*
> * If TBL24 entry is extended, then there has
> @@ -1365,7 +1375,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 +1658,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 +1673,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..98c70ecbe 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(2);
I think it is better to __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(2);
>
> __extension__
> struct rte_lpm_tbl_entry {
As a general remark consider writing all of the tbl entries including
tbl8 with atomic_store. Now "lpm->tbl8[j] = new_tbl8_entry;" is looks like
1e9: 44 88 9c 47 40 01 00 mov
%r11b,0x2000140(%rdi,%rax,2) <-write first byte
1f0: 02
1f1: 48 83 c0 01 add $0x1,%rax
1f5: 42 88 8c 47 41 01 00 mov %cl,0x2000141(%rdi,%r8,2)
<-write second byte
1fc: 02
This may cause an incorrect nexthop to be returned. If the byte with
valid flag is updated first, the old(and maybe invalid) next hop could
be returned.
Please evaluate performance drop after.
--
Regards,
Vladimir
next prev parent reply other threads:[~2019-06-28 13:33 UTC|newest]
Thread overview: 23+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-06-27 9:37 [dpdk-dev] [PATCH v3 1/3] lib/lpm: not inline unnecessary functions Ruifeng Wang
2019-06-27 9:37 ` [dpdk-dev] [PATCH v3 2/3] lib/lpm: memory orderings to avoid race conditions for v1604 Ruifeng Wang
2019-06-27 9:37 ` [dpdk-dev] [PATCH v3 3/3] lib/lpm: memory orderings to avoid race conditions for v20 Ruifeng Wang
2019-06-28 13:33 ` Medvedkin, Vladimir [this message]
2019-06-29 17:35 ` Honnappa Nagarahalli
2019-07-05 13:45 ` Alex Kiselev
2019-07-05 16:56 ` Medvedkin, Vladimir
2019-07-01 7:08 ` Ruifeng Wang (Arm Technology China)
2019-06-27 15:24 ` [dpdk-dev] [PATCH v3 1/3] lib/lpm: not inline unnecessary functions Stephen Hemminger
2019-06-28 2:44 ` Ruifeng Wang (Arm Technology China)
2019-06-28 4:34 ` Stephen Hemminger
2019-06-28 5:48 ` Ruifeng Wang (Arm Technology China)
2019-06-28 13:47 ` Medvedkin, Vladimir
2019-06-28 13:57 ` Honnappa Nagarahalli
2019-06-28 14:16 ` Medvedkin, Vladimir
2019-06-28 15:35 ` Stephen Hemminger
2019-07-01 6:44 ` Ruifeng Wang (Arm Technology China)
2019-07-05 10:40 ` Medvedkin, Vladimir
2019-07-05 10:58 ` Ruifeng Wang (Arm Technology China)
2019-07-05 10:31 ` Medvedkin, Vladimir
2019-07-05 13:37 ` Alex Kiselev
2019-07-05 16:53 ` Medvedkin, Vladimir
2019-06-28 13:38 ` Medvedkin, Vladimir
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=6daf7a76-1def-21df-d202-10b2a8f8582a@intel.com \
--to=vladimir.medvedkin@intel.com \
--cc=bruce.richardson@intel.com \
--cc=dev@dpdk.org \
--cc=gavin.hu@arm.com \
--cc=honnappa.nagarahalli@arm.com \
--cc=nd@arm.com \
--cc=ruifeng.wang@arm.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
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).