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 63BEE1B3DE for ; Fri, 13 Oct 2017 03:03:04 +0200 (CEST) Received: by mail-pf0-f194.google.com with SMTP id 17so7426176pfn.12 for ; Thu, 12 Oct 2017 18:03:04 -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=4jwhrLc7+44Zm0/HzuAjjMIQBfBgpGVPMi6WKnEOXPU=; b=c8XJFWDqgcD1GS6tW3ZaHz6poyMQ5pXMomrMnTPgOB1gsxEn8FnTf5XhlIIVd+wOwq D2+TwArHgk+rBw44Yyl0UYL5tC+30Tc12fa6LHy3YdubbGbqIV8urfkqDNWaRdy6yelO idNl0bMowwlqJGX1BZiF/yqqKIR5uNNUq7lL6wLJBaoNrch7gkPEVyBeT7s0ywsX0LsP EsBshpXedMwEtzFf8ViEmtQ9jWPjIcA/bzJxCKZtTLsT6eS5+84a8VcIeRnmgU3Dxi7r 76ggjjmpc/H2tVwMdx8MjGjQcltD9RwjG8w5rRd6TL9SeEBmLmx5Vd/1m83geUDWyLuk S+vw== 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=4jwhrLc7+44Zm0/HzuAjjMIQBfBgpGVPMi6WKnEOXPU=; b=PPWOc26zbVzGAEDf+74KZ/h/gwSlLe/MlE12w7ltU8XGgEtCZ4ModZt9tv/bB2ldVh aWOD8TnF5wkiPrB44HUKykDTH+4lN9Djha6wSqhfBfdLNdIl5b5aEQHOHfIDn/rJi6wh 67TrJAkUfOEJJxEri4NfncHGTB82t7o0p4nKWASAiiu3yapsqb+Z8YwrmU/N6Sld0YaO q7vnZ8Q/t5QbWckoUS5b+HoyVXOw/j1yOMHIRvGnaTnu6OcTcMKiWzlMxLwU+Gypr/vZ GyoKuAn20bDFZdrmbdIablubrIa5W1LeMGsYKiKzZR3wCptaPiuzUchr7BbvUYiJC4aP eeQw== X-Gm-Message-State: AMCzsaXXBH1YH7DiA4A7WYm671k7e1ZFHQHdsNrbFBu7b8ell1ENk0RH PYTPH3cFGFAJ/UNXqbgr2Mg= X-Google-Smtp-Source: AOwi7QAp3yqRiWLYerOxYMl73kbM3cyPppqDOma3HsCzlQZHSe39dhAePyn0DRm0x/AND2OiOUzrrA== X-Received: by 10.101.91.68 with SMTP id y4mr1495161pgr.137.1507856583417; Thu, 12 Oct 2017 18:03:03 -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 y27sm19417752pfi.107.2017.10.12.18.02.59 (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Thu, 12 Oct 2017 18:03:02 -0700 (PDT) To: Jerin Jacob , "Ananyev, Konstantin" Cc: Olivier MATZ , "dev@dpdk.org" , "jia.he@hxt-semitech.com" , "jie2.liu@hxt-semitech.com" , "bing.zhao@hxt-semitech.com" References: <20171010095636.4507-1-hejianet@gmail.com> <20171012155350.j34ddtivxzd27pag@platinum> <2601191342CEEE43887BDE71AB9772585FAA859F@IRSMSX103.ger.corp.intel.com> <20171012172311.GA8524@jerin> From: Jia He Message-ID: Date: Fri, 13 Oct 2017 09:02:55 +0800 User-Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:52.0) Gecko/20100101 Thunderbird/52.3.0 MIME-Version: 1.0 In-Reply-To: <20171012172311.GA8524@jerin> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [dpdk-dev] [PATCH] ring: guarantee ordering of cons/prod loading when doing enqueue/dequeue 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: Fri, 13 Oct 2017 01:03:04 -0000 Hi Jerin On 10/13/2017 1:23 AM, Jerin Jacob Wrote: > -----Original Message----- >> Date: Thu, 12 Oct 2017 17:05:50 +0000 >> From: "Ananyev, Konstantin" >> To: Olivier MATZ , Jia He >> CC: "dev@dpdk.org" , "jia.he@hxt-semitech.com" >> , "jie2.liu@hxt-semitech.com" >> , "bing.zhao@hxt-semitech.com" >> , "jerin.jacob@caviumnetworks.com" >> >> Subject: RE: [PATCH] ring: guarantee ordering of cons/prod loading when >> doing enqueue/dequeue >> >> Hi guys, >> >>> -----Original Message----- >>> From: Olivier MATZ [mailto:olivier.matz@6wind.com] >>> Sent: Thursday, October 12, 2017 4:54 PM >>> To: Jia He >>> Cc: dev@dpdk.org; jia.he@hxt-semitech.com; jie2.liu@hxt-semitech.com; bing.zhao@hxt-semitech.com; Ananyev, Konstantin >>> ; jerin.jacob@caviumnetworks.com >>> Subject: Re: [PATCH] ring: guarantee ordering of cons/prod loading when doing enqueue/dequeue >>> >>> Hi, >>> >>> On Tue, Oct 10, 2017 at 05:56:36PM +0800, Jia He wrote: >>>> Before this patch: >>>> In __rte_ring_move_cons_head() >>>> ... >>>> do { >>>> /* Restore n as it may change every loop */ >>>> n = max; >>>> >>>> *old_head = r->cons.head; //1st load >>>> const uint32_t prod_tail = r->prod.tail; //2nd load >>>> >>>> In weak memory order architectures(powerpc,arm), the 2nd load might be >>>> reodered before the 1st load, that makes *entries is bigger than we wanted. >>>> This nasty reording messed enque/deque up. >>>> >>>> cpu1(producer) cpu2(consumer) cpu3(consumer) >>>> load r->prod.tail >>>> in enqueue: >>>> load r->cons.tail >>>> load r->prod.head >>>> >>>> store r->prod.tail >>>> >>>> load r->cons.head >>>> load r->prod.tail >>>> ... >>>> store r->cons.{head,tail} >>>> load r->cons.head >>>> >>>> THEN,r->cons.head will be bigger than prod_tail, then make *entries very big >>>> >>>> After this patch, the old cons.head will be recaculated after failure of >>>> rte_atomic32_cmpset >>>> >>>> There is no such issue in X86 cpu, because X86 is strong memory order model >>>> >>>> Signed-off-by: Jia He >>>> Signed-off-by: jia.he@hxt-semitech.com >>>> Signed-off-by: jie2.liu@hxt-semitech.com >>>> Signed-off-by: bing.zhao@hxt-semitech.com >>>> >>>> --- >>>> lib/librte_ring/rte_ring.h | 8 ++++++++ >>>> 1 file changed, 8 insertions(+) >>>> >>>> diff --git a/lib/librte_ring/rte_ring.h b/lib/librte_ring/rte_ring.h >>>> index 5e9b3b7..15c72e2 100644 >>>> --- a/lib/librte_ring/rte_ring.h >>>> +++ b/lib/librte_ring/rte_ring.h >>>> @@ -409,6 +409,10 @@ __rte_ring_move_prod_head(struct rte_ring *r, int is_sp, >>>> n = max; >>>> >>>> *old_head = r->prod.head; >>>> + >>>> + /* load of prod.tail can't be reordered before cons.head */ >>>> + rte_smp_rmb(); >>>> + >>>> const uint32_t cons_tail = r->cons.tail; >>>> /* >>>> * The subtraction is done between two unsigned 32bits value >>>> @@ -517,6 +521,10 @@ __rte_ring_move_cons_head(struct rte_ring *r, int is_sc, >>>> n = max; >>>> >>>> *old_head = r->cons.head; >>>> + >>>> + /* load of prod.tail can't be reordered before cons.head */ >>>> + rte_smp_rmb(); >>>> + >>>> const uint32_t prod_tail = r->prod.tail; >>>> /* The subtraction is done between two unsigned 32bits value >>>> * (the result is always modulo 32 bits even if we have >>>> -- >>>> 2.7.4 >>>> >>> The explanation convinces me. >>> >>> However, since it's in a critical path, it would be good to have other >>> opinions. This patch reminds me this discussion, that was also related to >>> memory barrier, but at another place: >>> http://dpdk.org/ml/archives/dev/2016-July/043765.html >>> Lead to that patch: http://dpdk.org/browse/dpdk/commit/?id=ecc7d10e448e >>> But finally reverted: http://dpdk.org/browse/dpdk/commit/?id=c3acd92746c3 >>> >>> Konstatin, Jerin, do you have any comment? >> For IA, as rte_smp_rmb() is just a compiler_barrier, that patch shouldn't make any difference, >> but I can't see how read reordering would screw things up here... >> Probably just me and arm or ppc guys could explain what will be the problem >> if let say cons.tail will be read before prod.head in __rte_ring_move_prod_head(). >> I wonder Is there a simple test-case to reproduce that problem (on arm or ppc)? >> Probably new test-case for rte_ring autotest is needed, or is it possible to reproduce >> it with existing one? > On the same lines, > > Jia He, jie2.liu, bing.zhao, > > Is this patch based on code review or do you saw this issue on any of the > arm/ppc target? arm64 will have performance impact with this change. Based on mbuf_autotest, the rte_panic will be invoked in seconds. PANIC in test_refcnt_iter(): (lcore=0, iter=0): after 10s only 61 of 64 mbufs left free 1: [./test(rte_dump_stack+0x38) [0x58d868]] Aborted (core dumped) Cheers, Jia > > >> Konstantin