[dpdk-dev] [PATCH] ring: guarantee ordering of cons/prod loading when doing enqueue/dequeue

Jia He hejianet at gmail.com
Mon Oct 23 10:49:01 CEST 2017


Hi Jerin


On 10/20/2017 1:43 PM, Jerin Jacob Wrote:
> -----Original Message-----
>>
[...]
>> dependant on each other.
>> Thus a memory barrier is neccessary.
> Yes. The barrier is necessary.
> In fact, upstream freebsd fixed this issue for arm64. DPDK ring
> implementation is derived from freebsd's buf_ring.h.
> https://github.com/freebsd/freebsd/blob/master/sys/sys/buf_ring.h#L166
>
> I think, the only outstanding issue is, how to reduce the performance
> impact for arm64. I believe using accurate/release semantics instead
> of rte_smp_rmb() will reduce the performance overhead like similar ring implementations below,
> freebsd: https://github.com/freebsd/freebsd/blob/master/sys/sys/buf_ring.h#L166
> odp: https://github.com/Linaro/odp/blob/master/platform/linux-generic/pktio/ring.c
>
> Jia,
> 1) Can you verify the use of accurate/release semantics fixes the problem in your
> platform? like use of atomic_load_acq* in the reference code.
> 2) If so, What is the overhead between accurate/release and plane smp_smb()
> barriers. Based on that we need decide what path to take.
I've tested 3 cases.  The new 3rd case is to use the load_acquire 
barrier (half barrier) you mentioned
at above link.
The patch seems like:
@@ -408,8 +466,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;
-               const uint32_t cons_tail = r->cons.tail;
+               *old_head = atomic_load_acq_32(&r->prod.head);
+               const uint32_t cons_tail = 
atomic_load_acq_32(&r->cons.tail);

@@ -516,14 +576,15 @@ __rte_ring_move_cons_head(struct rte_ring *r, int is_s
                 /* Restore n as it may change every loop */
                 n = max;

-               *old_head = r->cons.head;
-               const uint32_t prod_tail = r->prod.tail;
+               *old_head = atomic_load_acq_32(&r->cons.head);
+               const uint32_t prod_tail = atomic_load_acq_32(&r->prod.tail)
                 /* The subtraction is done between two unsigned 32bits 
value
                  * (the result is always modulo 32 bits even if we have
                  * cons_head > prod_tail). So 'entries' is always between 0
                  * and size(ring)-1. */

The half barrier patch passed the fuctional test.

As for the performance comparision on *arm64*(the debug patch is at
http://dpdk.org/ml/archives/dev/2017-October/079012.html), please see 
the test results
below:

[case 1] old codes, no barrier
============================================
  Performance counter stats for './test --no-huge -l 1-10':

      689275.001200      task-clock (msec)         #    9.771 CPUs utilized
               6223      context-switches          #    0.009 K/sec
                 10      cpu-migrations            #    0.000 K/sec
                653      page-faults               #    0.001 K/sec
      1721190914583      cycles                    #    2.497 GHz
      3363238266430      instructions              #    1.95  insn per 
cycle
    <not supported> branches
           27804740      branch-misses             #    0.00% of all 
branches

       70.540618825 seconds time elapsed

[case 2] full barrier with rte_smp_rmb()
============================================
  Performance counter stats for './test --no-huge -l 1-10':

      582557.895850      task-clock (msec)         #    9.752 CPUs utilized
               5242      context-switches          #    0.009 K/sec
                 10      cpu-migrations            #    0.000 K/sec
                665      page-faults               #    0.001 K/sec
      1454360730055      cycles                    #    2.497 GHz
       587197839907      instructions              #    0.40  insn per 
cycle
    <not supported> branches
           27799687      branch-misses             #    0.00% of all 
branches

       59.735582356 seconds time elapse

[case 1] half barrier with load_acquire
============================================
  Performance counter stats for './test --no-huge -l 1-10':

      660758.877050      task-clock (msec)         #    9.764 CPUs utilized
               5982      context-switches          #    0.009 K/sec
                 11      cpu-migrations            #    0.000 K/sec
                657      page-faults               #    0.001 K/sec
      1649875318044      cycles                    #    2.497 GHz
       591583257765      instructions              #    0.36  insn per 
cycle
    <not supported> branches
           27994903      branch-misses             #    0.00% of all 
branches

       67.672855107 seconds time elapsed

Please see the context-switches in the perf results
test result  sorted by time is:
full barrier < half barrier < no barrier

AFAICT, in this case ,the cpu reordering will add the possibility for 
context switching and
increase the running time.

Any ideas?

Cheers,
Jia

>
> Note:
> This issue wont come in all the arm64 implementation. it comes on arm64
> implementation with OOO(out of order) implementations.
>
>
>> Cheers,
>> Jia
>>
>>> Konstantin
>>>
>>>>> . In another
>>>>> mail of this thread, we've made a simple test based on this and captured
>>>>> some information and I pasted there.(I pasted the patch there :-))
>>>> Are you talking about that one:
>>>> http://dpdk.org/dev/patchwork/patch/30405/
>>>> ?
>>>> It still uses test/test/test_mbuf.c...,
>>>> but anyway I don't really understand how mbuf_autotest supposed
>>>> to work with these changes:
>>>> @@ -730,7 +739,7 @@ test_refcnt_iter(unsigned int lcore, unsigned int iter,
>>>> rte_ring_enqueue(refcnt_mbuf_ring, m);
>>>>                            }
>>>>                    }
>>>> -               rte_pktmbuf_free(m);
>>>> +               // rte_pktmbuf_free(m);
>>>>            }
>>>> @@ -741,6 +750,12 @@ test_refcnt_iter(unsigned int lcore, unsigned int iter,
>>>>            while (!rte_ring_empty(refcnt_mbuf_ring))
>>>>                    ;
>>>>
>>>> +       if (NULL != m) {
>>>> +               if (1 != rte_mbuf_refcnt_read(m))
>>>> +                       printf("m ref is %u\n", rte_mbuf_refcnt_read(m));
>>>> +               rte_pktmbuf_free(m);
>>>> +       }
>>>> +
>>>>            /* check that all mbufs are back into mempool by now */
>>>>            for (wn = 0; wn != REFCNT_MAX_TIMEOUT; wn++) {
>>>>                    if ((i = rte_mempool_avail_count(refcnt_pool)) == n) {
>>>>
>>>> That means all your mbufs (except the last one) will still be allocated.
>>>> So the test would fail - as it should, I think.
>>>>
>>>>> And
>>>>> it seems that Juhamatti & Jacod found some reverting action several
>>>>> months ago.
>>>> Didn't get that one either.
>>>> Konstantin

-- 
Cheers,
Jia



More information about the dev mailing list