From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-pf0-f194.google.com (mail-pf0-f194.google.com [209.85.192.194]) by dpdk.org (Postfix) with ESMTP id 22E631B619 for ; Thu, 2 Nov 2017 16:43:06 +0100 (CET) Received: by mail-pf0-f194.google.com with SMTP id 17so4898991pfn.12 for ; Thu, 02 Nov 2017 08:43:06 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=subject:to:cc:references:from:message-id:date:user-agent :mime-version:in-reply-to:content-transfer-encoding; bh=0cnaJUfcVRIT857zqql3HfUPw189UQz6V7GogjUJyEs=; b=Hwr4qF6bEDM7s3TfFIcESGIxX8ZnTfPsAgtAR4UZzYUmzLYn8wxXtomRFVyvNEkSX4 P0sEYSvh/MtF0NGCzTPbJmmTWDJ+GDbM+R8DqpqSe4KektjeVBnRs4DRb9jVrx7RMB81 BoGBmlElqTXfSM4EJsISRLEBuAwwdhHgFaa7/rVBmBLiTYQiXLQz7PJv5uYL89wE8g4g R0khMg4MeBcjwNH1ULmeVN2pLKIAER4qNnU4cAKZTSXnt7IlONlrjAwE2Z2mG/MOV9fi +s+h8ilwHAkLpjhj8wM670YUzCBltXRzhbEhPfRfLl3VDe/+p4rrl3+9DSiPWbAGFlmy ZnTA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:subject:to:cc:references:from:message-id:date :user-agent:mime-version:in-reply-to:content-transfer-encoding; bh=0cnaJUfcVRIT857zqql3HfUPw189UQz6V7GogjUJyEs=; b=cy5vmp5yJ38A2ffvTqOKLmUxBBf0trOgt6PQxkjNgAlo9LyeklxggRsJkH2wMjysRc 6DYANldFbk3H0Tzw+ju6woh5k8+wKhgOttcliSjlfAWZqf0oI3bSgAstDC4P7xLLrBzA po0K1sBLkRCbqkIGAFa3IMvAqpOxkgdzpRK2Plc7vH3+u4+7K/yRsPVyTwNEk/VoddNw UeLNUrAZZ1I7PATDo5ue6auUua+Z0ALLR4BS8wMA+3JTcPFCgG2KFApX4DW8bB8+8JZx 4JAVUzEjuf4rJK0pkSeaowuNMlMnQp7dklcsJRFBJsJcPkUBLQQGLhLU/SSPVeRM6NC7 tTBQ== X-Gm-Message-State: AMCzsaXLMoGbx3ItT3Lpp8a4/+Wn6B41451pOFwJRpgCjJwcXjaFuIhB mZpV+zLxKLEN9nsff9plF/o= X-Google-Smtp-Source: ABhQp+QS1ThQCpISMp3uKPyqshGlAj21sbMES312oDwGj6lYt3P/dYRzPCWVnYqhuNXJVhDZV76bKg== X-Received: by 10.98.59.65 with SMTP id i62mr4304500pfa.143.1509637386092; Thu, 02 Nov 2017 08:43:06 -0700 (PDT) Received: from [0.0.0.0] (67.209.179.165.16clouds.com. [67.209.179.165]) by smtp.gmail.com with ESMTPSA id n10sm7003838pfk.174.2017.11.02.08.42.58 (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Thu, 02 Nov 2017 08:43:05 -0700 (PDT) To: "Ananyev, Konstantin" , "jerin.jacob@caviumnetworks.com" , "dev@dpdk.org" , "olivier.matz@6wind.com" Cc: "Richardson, Bruce" , "jianbo.liu@arm.com" , "hemant.agrawal@nxp.com" , "jie2.liu@hxt-semitech.com" , "bing.zhao@hxt-semitech.com" , "jia.he@hxt-semitech.com" References: <1509612210-5499-1-git-send-email-hejianet@gmail.com> <2601191342CEEE43887BDE71AB9772585FAB8703@irsmsx105.ger.corp.intel.com> From: Jia He Message-ID: Date: Thu, 2 Nov 2017 23:42:57 +0800 User-Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:52.0) Gecko/20100101 Thunderbird/52.4.0 MIME-Version: 1.0 In-Reply-To: <2601191342CEEE43887BDE71AB9772585FAB8703@irsmsx105.ger.corp.intel.com> Content-Type: text/plain; charset=gbk; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [dpdk-dev] [PATCH v2] ring: guarantee ordering of cons/prod loading when doing 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: Thu, 02 Nov 2017 15:43:07 -0000 Hi Ananyev On 11/2/2017 9:26 PM, Ananyev, Konstantin Wrote: > Hi Jia, > >> -----Original Message----- >> From: Jia He [mailto:hejianet@gmail.com] >> Sent: Thursday, November 2, 2017 8:44 AM >> To: jerin.jacob@caviumnetworks.com; dev@dpdk.org; olivier.matz@6wind.com >> Cc: Ananyev, Konstantin ; Richardson, Bruce ; jianbo.liu@arm.com; >> hemant.agrawal@nxp.com; Jia He ; jie2.liu@hxt-semitech.com; bing.zhao@hxt-semitech.com; jia.he@hxt- >> semitech.com >> Subject: [PATCH v2] ring: guarantee ordering of cons/prod loading when doing >> >> We watched a rte panic of mbuf_autotest in our qualcomm arm64 server. >> As for the possible race condition, please refer to [1]. >> >> Furthermore, there are 2 options as suggested by Jerin: >> 1. use rte_smp_rmb >> 2. use load_acquire/store_release(refer to [2]). >> CONFIG_RTE_ATOMIC_ACQUIRE_RELEASE_BARRIER_PREFER is provided, and by >> default it is n; >> >> The reason why providing 2 options is due to the performance benchmark >> difference in different arm machines, please refer to [3]. >> >> Already fuctionally tested on the machines as follows: >> on X86(passed the compilation) >> on arm64 with CONFIG_RTE_ATOMIC_ACQUIRE_RELEASE_BARRIER_PREFER=y >> on arm64 with CONFIG_RTE_ATOMIC_ACQUIRE_RELEASE_BARRIER_PREFER=n >> >> [1] http://dpdk.org/ml/archives/dev/2017-October/078366.html >> [2] https://github.com/freebsd/freebsd/blob/master/sys/sys/buf_ring.h#L170 >> [3] http://dpdk.org/ml/archives/dev/2017-October/080861.html >> >> --- >> Changelog: >> V2: let users choose whether using load_acquire/store_release >> V1: rte_smp_rmb() between 2 loads >> >> Signed-off-by: Jia He >> Signed-off-by: jie2.liu@hxt-semitech.com >> Signed-off-by: bing.zhao@hxt-semitech.com >> Signed-off-by: jia.he@hxt-semitech.com >> Suggested-by: jerin.jacob@caviumnetworks.com >> --- >> lib/librte_ring/Makefile | 4 +++- >> lib/librte_ring/rte_ring.h | 38 ++++++++++++++++++++++++------ >> lib/librte_ring/rte_ring_arm64.h | 48 ++++++++++++++++++++++++++++++++++++++ >> lib/librte_ring/rte_ring_generic.h | 45 +++++++++++++++++++++++++++++++++++ >> 4 files changed, 127 insertions(+), 8 deletions(-) >> create mode 100644 lib/librte_ring/rte_ring_arm64.h >> create mode 100644 lib/librte_ring/rte_ring_generic.h >> >> diff --git a/lib/librte_ring/Makefile b/lib/librte_ring/Makefile >> index e34d9d9..fa57a86 100644 >> --- a/lib/librte_ring/Makefile >> +++ b/lib/librte_ring/Makefile >> @@ -45,6 +45,8 @@ LIBABIVER := 1 >> SRCS-$(CONFIG_RTE_LIBRTE_RING) := rte_ring.c >> >> # install includes >> -SYMLINK-$(CONFIG_RTE_LIBRTE_RING)-include := rte_ring.h >> +SYMLINK-$(CONFIG_RTE_LIBRTE_RING)-include := rte_ring.h \ >> + rte_ring_arm64.h \ >> + rte_ring_generic.h >> >> include $(RTE_SDK)/mk/rte.lib.mk >> diff --git a/lib/librte_ring/rte_ring.h b/lib/librte_ring/rte_ring.h >> index 5e9b3b7..943b1f9 100644 >> --- a/lib/librte_ring/rte_ring.h >> +++ b/lib/librte_ring/rte_ring.h >> @@ -103,6 +103,18 @@ extern "C" { >> #include >> #include >> >> +/* In those strong memory models (e.g. x86), there is no need to add rmb() >> + * between load and load. >> + * In those weak models(powerpc/arm), there are 2 choices for the users >> + * 1.use rmb() memory barrier >> + * 2.use one-direcion load_acquire/store_release barrier >> + * It depends on performance test results. */ >> +#ifdef RTE_ARCH_ARM64 >> +#include "rte_ring_arm64.h" >> +#else >> +#include "rte_ring_generic.h" >> +#endif >> + >> #define RTE_TAILQ_RING_NAME "RTE_RING" >> >> enum rte_ring_queue_behavior { >> @@ -368,7 +380,7 @@ update_tail(struct rte_ring_headtail *ht, uint32_t old_val, uint32_t new_val, >> while (unlikely(ht->tail != old_val)) >> rte_pause(); >> >> - ht->tail = new_val; >> + arch_rte_atomic_store(&ht->tail, new_val, __ATOMIC_RELEASE); >> } >> >> /** >> @@ -408,7 +420,8 @@ __rte_ring_move_prod_head(struct rte_ring *r, int is_sp, >> /* Reset n to the initial burst count */ >> n = max; >> >> - *old_head = r->prod.head; >> + *old_head = arch_rte_atomic_load(&r->prod.head, >> + __ATOMIC_ACQUIRE); >> const uint32_t cons_tail = r->cons.tail; > The code starts to look a bit messy with all these arch specific macros... > So I wonder wouldn't it be more cleaner to: > > 1. move existing __rte_ring_move_prod_head/__rte_ring_move_cons_head/update_tail > into rte_ring_generic.h > 2. Add rte_smp_rmb into generic __rte_ring_move_prod_head/__rte_ring_move_cons_head > (as was in v1 of your patch). > 3. Introduce ARM specific versions of __rte_ring_move_prod_head/__rte_ring_move_cons_head/update_tail > in the rte_ring_arm64.h > > That way we will keep ogneric code simple and clean, while still allowing arch specific optimizations. Thanks for your review. But as per your suggestion, there will be at least 2 copies of __rte_ring_move_prod_head/__rte_ring_move_cons_head/update_tail. Thus, if there are any bugs in the future, both 2 copies have to be changed, right? > >> /* >> * The subtraction is done between two unsigned 32bits value >> @@ -430,8 +443,10 @@ __rte_ring_move_prod_head(struct rte_ring *r, int is_sp, >> if (is_sp) >> r->prod.head = *new_head, success = 1; >> else >> - success = rte_atomic32_cmpset(&r->prod.head, >> - *old_head, *new_head); >> + success = arch_rte_atomic32_cmpset(&r->prod.head, >> + old_head, *new_head, >> + 0, __ATOMIC_ACQUIRE, >> + __ATOMIC_RELAXED); >> } while (unlikely(success == 0)); >> return n; >> } >> @@ -470,7 +485,10 @@ __rte_ring_do_enqueue(struct rte_ring *r, void * const *obj_table, >> goto end; >> >> ENQUEUE_PTRS(r, &r[1], prod_head, obj_table, n, void *); >> + >> +#ifndef RTE_ATOMIC_ACQUIRE_RELEASE_BARRIER_PREFER > I wonder why do we need that macro? > Would be there situations when smp_wmb() are not needed here? If the dpdk user chooses the config acquire/release, the store_release barrier in update_tail together with the load_acquire barrier pair in __rte_ring_move_{prod,cons}_head guarantee the order. So smp_wmb() is not required here. Please refer to the freebsd ring implementation and Jerin's debug patch. https://github.com/freebsd/freebsd/blob/master/sys/sys/buf_ring.h https://github.com/jerinjacobk/mytests/blob/master/ring/0001-ring-using-c11-memory-model.patch --- Cheers, Jia > Konstantin > > -- Cheers, Jia