From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-pf0-f193.google.com (mail-pf0-f193.google.com [209.85.192.193]) by dpdk.org (Postfix) with ESMTP id 6E76A160; Tue, 17 Jul 2018 04:54:20 +0200 (CEST) Received: by mail-pf0-f193.google.com with SMTP id q7-v6so26162369pff.2; Mon, 16 Jul 2018 19:54:20 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=mime-version:in-reply-to:references:from:date:message-id:subject:to :cc; bh=+hLPkx2vXqcK4nalbYlD/tZXy5ReEy3SpygSPe4BF9o=; b=q8bkTG7B4Ejkt9W00LS2UKoklEOoT2lWkug60hMnjMVGzToZO9qi9ZlWGebT/0CQHl lvSnTm0T6qUpMDVBhFbhpi43+mwsEtpkosp+HbxTriAkScbEWTsY4g8zv2uwtwkMCZDg KpEvUxz/Qo27hJw6ohxfxfdQXFOTx5ATONd0XS7fpQlB+J5ovGJ0XMAhLlRUMHdsGW2y t9SIqCZSX+FfcvogtiJXqZX3FxFE9rQIHkA/02gST9VU24bhD2ypTLYtI51C5Blokt63 eY0WLqnynUzkks4bKgutkCLg0ghzItJaljGUljpM4jQd7devLLuNOl9QK3Or9p95IA5U +aVg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:in-reply-to:references:from:date :message-id:subject:to:cc; bh=+hLPkx2vXqcK4nalbYlD/tZXy5ReEy3SpygSPe4BF9o=; b=RGqsdhzQMFYFftdqmWvyRLF4ZDoFmgWntgNq8x6MP0qLEWfIfnLSYvsiZkPmjJvQI3 A3Azhttou9wDnk0Sd/zKaXkk6j3Fzw06S3v0bSXxqehEgg2fzLfMUYH931SM/PANBAW3 R5DSEaJJOyhcnIUc9U8F3LjM+ybVCHNgvVfJsEEYH4RJrSlQpUBM/vi1fzCWnngKwCLE WfGPWTTevGKkEuZ1dl585bQ710MI35KJkac0EAusKA3gH7V2R8LJVflRMsG7I7tfyvIb ueVbXl8iCqpJGOWZQVGUI5eYwtNKZ0K7DhbljgLD54kzNgglgHw5WoPI3Yuf4W9sZG3B UjuA== X-Gm-Message-State: AOUpUlHPejio2kfDT1+7/lrVGZVY1QHgX4u0Xd5zjpQyf5+f+ceqVkHl 3aumyBv7Y6PVo4NsUKGDb61K0dCCdORJEVb0Jn0= X-Google-Smtp-Source: AAOMgpetmkNwHw5stpZEGn6vij1Z2osuLbc8A9Yq/LnjdA1SZUk4ZiZktFnez5/ZK/3b5oIhhYymX5vKzo5fb/H1RfU= X-Received: by 2002:a63:8848:: with SMTP id l69-v6mr17309808pgd.377.1531796059434; Mon, 16 Jul 2018 19:54:19 -0700 (PDT) MIME-Version: 1.0 Received: by 2002:a17:90a:22a8:0:0:0:0 with HTTP; Mon, 16 Jul 2018 19:54:18 -0700 (PDT) In-Reply-To: <20180712170839.GA11626@jerin> References: <20180712024414.4756-1-t.yoshimura8869@gmail.com> <20180712170839.GA11626@jerin> From: Takeshi Yoshimura Date: Tue, 17 Jul 2018 11:54:18 +0900 Message-ID: To: Jerin Jacob Cc: dev@dpdk.org, stable@dpdk.org Content-Type: text/plain; charset="UTF-8" Subject: Re: [dpdk-dev] [PATCH] rte_ring: fix racy dequeue/enqueue in ppc64 X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 17 Jul 2018 02:54:20 -0000 > Adding rte_smp_rmb() cause performance regression on non x86 platforms. > Having said that, load-load barrier can be expressed very well with C11 memory > model. I guess ppc64 supports C11 memory model. If so, > Could you try CONFIG_RTE_RING_USE_C11_MEM_MODEL=y for ppc64 and check > original issue? Yes, the performance regression happens on non-x86 with single producer/consumer. The average latency of an enqueue was increased from 21 nsec to 24 nsec in my simple experiment. But, I think it is worth it. I also tested C11 rte_ring, however, it caused the same race condition in ppc64. I tried to fix the C11 problem as well, but I also found the C11 rte_ring had other potential incorrect choices of memory orders, which caused another race condition in ppc64. For example, __ATOMIC_ACQUIRE is passed to __atomic_compare_exchange_n(), but I am not sure why the load-acquire is used for the compare exchange. Also in update_tail, the pause can be called before the data copy because of ht->tail load without atomic_load_n. The memory order is simply difficult, so it might take a bit longer time to check if the code is correct. I think I can fix the C11 rte_ring as another patch. 2018-07-13 2:08 GMT+09:00 Jerin Jacob : > -----Original Message----- >> Date: Thu, 12 Jul 2018 11:44:14 +0900 >> From: Takeshi Yoshimura >> To: dev@dpdk.org >> Cc: Takeshi Yoshimura , stable@dpdk.org, Takeshi >> Yoshimura >> Subject: [dpdk-dev] [PATCH] rte_ring: fix racy dequeue/enqueue in ppc64 >> X-Mailer: git-send-email 2.15.1 >> >> External Email >> >> SPDK blobfs encountered a crash around rte_ring dequeues in ppc64. >> It uses a single consumer and multiple producers for a rte_ring. >> The problem was a load-load reorder in rte_ring_sc_dequeue_bulk(). > > Adding rte_smp_rmb() cause performance regression on non x86 platforms. > Having said that, load-load barrier can be expressed very well with C11 memory > model. I guess ppc64 supports C11 memory model. If so, > Could you try CONFIG_RTE_RING_USE_C11_MEM_MODEL=y for ppc64 and check > original issue? > >> >> The reordered loads happened on r->prod.tail in >> __rte_ring_move_cons_head() (rte_ring_generic.h) and ring[idx] in >> DEQUEUE_PTRS() (rte_ring.h). They have a load-load control >> dependency, but the code does not satisfy it. Note that they are >> not reordered if __rte_ring_move_cons_head() with is_sc != 1 because >> cmpset invokes a read barrier. >> >> The paired stores on these loads are in ENQUEUE_PTRS() and >> update_tail(). Simplified code around the reorder is the following. >> >> Consumer Producer >> load idx[ring] >> store idx[ring] >> store r->prod.tail >> load r->prod.tail >> >> In this case, the consumer loads old idx[ring] and confirms the load >> is valid with the new r->prod.tail. >> >> I added a read barrier in the case where __IS_SC is passed to >> __rte_ring_move_cons_head(). I also fixed __rte_ring_move_prod_head() >> to avoid similar problems with a single producer. >> >> Cc: stable@dpdk.org >> >> Signed-off-by: Takeshi Yoshimura >> --- >> lib/librte_ring/rte_ring_generic.h | 10 ++++++---- >> 1 file changed, 6 insertions(+), 4 deletions(-) >> >> diff --git a/lib/librte_ring/rte_ring_generic.h b/lib/librte_ring/rte_ring_generic.h >> index ea7dbe5b9..477326180 100644 >> --- a/lib/librte_ring/rte_ring_generic.h >> +++ b/lib/librte_ring/rte_ring_generic.h >> @@ -90,9 +90,10 @@ __rte_ring_move_prod_head(struct rte_ring *r, unsigned int is_sp, >> return 0; >> >> *new_head = *old_head + n; >> - if (is_sp) >> + if (is_sp) { >> + rte_smp_rmb(); >> r->prod.head = *new_head, success = 1; >> - else >> + } else >> success = rte_atomic32_cmpset(&r->prod.head, >> *old_head, *new_head); >> } while (unlikely(success == 0)); >> @@ -158,9 +159,10 @@ __rte_ring_move_cons_head(struct rte_ring *r, unsigned int is_sc, >> return 0; >> >> *new_head = *old_head + n; >> - if (is_sc) >> + if (is_sc) { >> + rte_smp_rmb(); >> r->cons.head = *new_head, success = 1; >> - else >> + } else >> success = rte_atomic32_cmpset(&r->cons.head, *old_head, >> *new_head); >> } while (unlikely(success == 0)); >> -- >> 2.17.1 >>