Hi Jerin

On 11/2/2017 3:04 AM, Jerin Jacob Wrote:
Date: Thu, 2 Nov 2017 00:27:46 +0530
From: Jerin Jacob <jerin.ja...@caviumnetworks.com>
To: Jia He <hejia...@gmail.com>
Cc: "Ananyev, Konstantin" <konstantin.anan...@intel.com>,
        "Zhao, Bing" <iloveth...@163.com>,
        Olivier MATZ <olivier.m...@6wind.com>,
        "dev@dpdk.org" <dev@dpdk.org>,
        "jia...@hxt-semitech.com" <jia...@hxt-semitech.com>,
        "jie2....@hxt-semitech.com" <jie2....@hxt-semitech.com>,
        "bing.z...@hxt-semitech.com" <bing.z...@hxt-semitech.com>,
        "Richardson, Bruce" <bruce.richard...@intel.com>,
        jianbo....@arm.com, hemant.agra...@nxp.com
Subject: Re: [dpdk-dev] [PATCH] ring: guarantee ordering of cons/prod loading
  when doing enqueue/dequeue
Message-ID: <20171101185723.GA18759@jerin>
References: 
<2601191342ceee43887bde71ab9772585faab...@irsmsx103.ger.corp.intel.com>
  <3e580cd7-2854-d855-be9c-7c4ce06e3...@gmail.com>
  <20171020054319.GA4249@jerin>
  <ab7154a2-a9f8-f12e-b6a0-2805c2065...@gmail.com>
  <20171023100617.GA17957@jerin>
  <b0e6eae2-e61b-1946-ac44-d78122577...@gmail.com>
  <20171025132642.GA13977@jerin>
  <b54ce545-b75d-9813-209f-4125bd76c...@gmail.com>
  <20171031111433.GA21742@jerin>
  <69adfb00-4582-b362-0540-d1d9d6bcf...@gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=iso-8859-1
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <69adfb00-4582-b362-0540-d1d9d6bcf...@gmail.com>
User-Agent: Mutt/1.9.1 (2017-09-22)

-----Original Message-----
Date: Wed, 1 Nov 2017 10:53:12 +0800
From: Jia He <hejia...@gmail.com>
To: Jerin Jacob <jerin.ja...@caviumnetworks.com>
Cc: "Ananyev, Konstantin" <konstantin.anan...@intel.com>, "Zhao, Bing"
  <iloveth...@163.com>, Olivier MATZ <olivier.m...@6wind.com>,
  "dev@dpdk.org" <dev@dpdk.org>, "jia...@hxt-semitech.com"
  <jia...@hxt-semitech.com>, "jie2....@hxt-semitech.com"
  <jie2....@hxt-semitech.com>, "bing.z...@hxt-semitech.com"
  <bing.z...@hxt-semitech.com>, "Richardson, Bruce"
  <bruce.richard...@intel.com>, jianbo....@arm.com, hemant.agra...@nxp.com
Subject: Re: [dpdk-dev] [PATCH] ring: guarantee ordering of cons/prod
  loading when doing enqueue/dequeue
User-Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:52.0) Gecko/20100101
  Thunderbird/52.4.0

Hi Jerin

Thanks for your suggestions. I will try to use config macro to let it be
chosen by user.
It is better, if we avoid #ifdef's in the common code. I think, you can
do the scheme like examples/l3fwd/l3fwd_em_hlm_neon.h. Where,
the common code will have all generic routine, difference will be
moved to a separate files to reduce #ifdef clutter.

I need to point out one possible issue in your load_acq/store_rel patch

at 
https://github.com/jerinjacobk/mytests/blob/master/ring/0001-ring-using-c11-memory-model.patch

@@ -516,8 +541,13 @@ __rte_ring_move_cons_head(struct rte_ring *r, int
is_sc,
          /* Restore n as it may change every loop */
          n = max;

+#if 0
          *old_head = r->cons.head;
          const uint32_t prod_tail = r->prod.tail;
+#else
+        *old_head = __atomic_load_n(&r->cons.head, __ATOMIC_RELAXED);
                --[1]
+        const uint32_t prod_tail = __atomic_load_n(&r->prod.tail,
__ATOMIC_ACQUIRE);   --[2]
+#endif

line [1] __ATOMIC_RELAXED is not enough for this case(tested in our ARM64
server).

line [2] __ATOMIC_ACQUIRE guarantee the 2nd load will not be reorded before
the 1st load, but will not

guarantee the 1st load will not be reordered after the 2nd load. Please also
For me it looks same. [1] can not cross [2] is same as [2] cannot cross
[1], if [1] are [2] at back to back. No ?
No,
IIUC, load_acquire(a) is equal to the pseudo codes:
load(a)
one-direction-barrier()

instead of
one-direction-barrier()
load(a)

Thus, in below cases, load(a) and load(b) can still be reordered, this is not the semantic violation of
the load_acquire:
load(a)
load_acquire(b)

IIUC, the orginal implementation in https://github.com/freebsd/freebsd/blob/master/sys/sys/buf_ring.h#L170 is not optimal.
I tested the changes as follow and it works fine:

+        *old_head = __atomic_load_n(&r->cons.head, __ATOMIC_ACQUIRE);
         const uint32_t prod_tail = r->prod.tail;

i.e.
load_acquire(a)
load(b)


Cheers,
Jia

Reply via email to