Re: [PATCH v2] ring: guarantee ordering of cons/prod loading when doing

[Jerin Jacob <jerin.jacob@caviumnetworks.com>]

-----Original Message-----
> Date: Mon, 6 Nov 2017 15:25:12 +0800
> From: Jia He <hejianet@gmail.com>
> To: Jerin Jacob <jerin.jacob@caviumnetworks.com>
> Cc: dev@dpdk.org, olivier.matz@6wind.com, konstantin.ananyev@intel.com,
>  bruce.richardson@intel.com, jianbo.liu@arm.com, hemant.agrawal@nxp.com,
>  jie2.liu@hxt-semitech.com, bing.zhao@hxt-semitech.com,
>  jia.he@hxt-semitech.com
> Subject: Re: [PATCH v2] ring: guarantee ordering of cons/prod loading when
>  doing
> User-Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:52.0) Gecko/20100101
>  Thunderbird/52.4.0
> 
> Hi Jerin
> 
> 
> On 11/3/2017 8:56 PM, Jerin Jacob Wrote:
> > -----Original Message-----
> > > 
> [...]
> > > g like that.
> > > Ok, but how to distinguish following 2 options?
> > No clearly understood this question. For arm64 case, you can add
> > CONFIG_RTE_RING_USE_C11_MEM_MODEL=y in config/defconfig_arm64-armv8a-*
> Sorry for my unclear expressions.
> I mean there should be one additional config macro besides
> CONFIG_RTE_RING_USE_C11_MEM_MODEL
> for users to choose?
> 
> i.e.
>  - On X86:CONFIG_RTE_RING_USE_C11_MEM_MODEL=n
> include rte_ring_generic.h, no changes
> - On arm64，CONFIG_RTE_RING_USE_C11_MEM_MODEL=y
> include rte_ring_c11_mem.h by default.
> In rte_ring_c11_mem.h, implement new version of
> __rte_ring_move_prod_head/__rte_ring_move_cons_head/update_tail
> 
> Then, how to distinguish the option of using rte_smp_rmb() or
> __atomic_load/store_n()?

On option could be to change the prototype of update_tail() and make
compiler accommodate it for zero cost for arm64(Which I think, it it the
case. But you can check the generated instructions)
If not, move, __rte_ring_do_dequeue() and __rte_ring_do_enqueue() instead of
__rte_ring_move_prod_head/__rte_ring_move_cons_head/update_tail()


➜ [master][dpdk.org] $ git diff
diff --git a/lib/librte_ring/rte_ring.h b/lib/librte_ring/rte_ring.h
index 5e9b3b7b4..b32648825 100644
--- a/lib/librte_ring/rte_ring.h
+++ b/lib/librte_ring/rte_ring.h
@@ -358,8 +358,12 @@ void rte_ring_dump(FILE *f, const struct rte_ring
*r);
 
 static __rte_always_inline void
 update_tail(struct rte_ring_headtail *ht, uint32_t old_val, uint32_t
new_val,
-               uint32_t single)
+               uint32_t single, const uint32_t enqueue)
 {
+       if (enqueue)
+               rte_smp_wmb();
+       else
+               rte_smp_rmb();
        /*
         * If there are other enqueues/dequeues in progress that
         * preceded us,
         * we need to wait for them to complete
@@ -470,9 +474,8 @@ __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 *);
-       rte_smp_wmb();
 
-       update_tail(&r->prod, prod_head, prod_next, is_sp);
+       update_tail(&r->prod, prod_head, prod_next, is_sp, 1);
 end:
        if (free_space != NULL)
                *free_space = free_entries - n;
@@ -575,9 +578,8 @@ __rte_ring_do_dequeue(struct rte_ring *r, void
**obj_table,
                goto end;
 
        DEQUEUE_PTRS(r, &r[1], cons_head, obj_table, n, void *);
-       rte_smp_rmb();
 
-       update_tail(&r->cons, cons_head, cons_next, is_sc);
+       update_tail(&r->cons, cons_head, cons_next, is_sc, 0);
 
 end:
        if (available != NULL)


> 
> Thanks for the clarification.
> 
> -- 
> Cheers,
> Jia
>