Hi Adrien > -----Original Message----- > From: Adrien Mazarguil [mailto:adrien.mazarg...@6wind.com] > Sent: Tuesday, October 31, 2017 12:17 PM > To: Matan Azrad <ma...@mellanox.com> > Cc: dev@dpdk.org; Ophir Munk <ophi...@mellanox.com> > Subject: Re: [PATCH v3 6/7] net/mlx4: mitigate Tx path memory barriers > > Hi Matan, > > On Mon, Oct 30, 2017 at 07:47:20PM +0000, Matan Azrad wrote: > > Hi Adrien > > > > > -----Original Message----- > > > From: Adrien Mazarguil [mailto:adrien.mazarg...@6wind.com] > > > Sent: Monday, October 30, 2017 4:24 PM > > > To: Matan Azrad <ma...@mellanox.com> > > > Cc: dev@dpdk.org; Ophir Munk <ophi...@mellanox.com> > > > Subject: Re: [PATCH v3 6/7] net/mlx4: mitigate Tx path memory > > > barriers > > > > > > On Mon, Oct 30, 2017 at 10:07:28AM +0000, Matan Azrad wrote: > > > > Replace most of the memory barriers by compiler barriers since > > > > they are all targeted to the DRAM; This improves code efficiency > > > > for systems which force store order between different addresses. > > > > > > > > Only the doorbell record store should be protected by memory > > > > barrier since it is targeted to the PCI memory domain. > > > > > > > > Limit pre byte count store compiler barrier for systems with cache > > > > line size smaller than 64B (TXBB size). > > > > > > > > Signed-off-by: Matan Azrad <ma...@mellanox.com> > > > > > > This sounds like an interesting performance improvement, can you > > > share the typical or expected amount (percentage/hard numbers) for a > > > given use case as part of the commit log? > > > > > > > Yes, it improves performance, I will share numbers. > > First I must add I thought rte_io_[rw]mb() was really only a renamed > compiler barrier, I better understand its purpose now, thanks. > > (more below.) > > > > More comments below. > > > > > > > --- > > > > drivers/net/mlx4/mlx4_rxtx.c | 11 ++++++----- > > > > 1 file changed, 6 insertions(+), 5 deletions(-) > > > > > > > > diff --git a/drivers/net/mlx4/mlx4_rxtx.c > > > > b/drivers/net/mlx4/mlx4_rxtx.c index 8ea8851..482c399 100644 > > > > --- a/drivers/net/mlx4/mlx4_rxtx.c > > > > +++ b/drivers/net/mlx4/mlx4_rxtx.c > > > > @@ -168,7 +168,7 @@ struct pv { > > > > /* > > > > * Make sure we read the CQE after we read the ownership > > > bit. > > > > */ > > > > - rte_rmb(); > > > > + rte_io_rmb(); > > > > > > OK for this one since the rest of the code should not be run due to > > > the condition (I'm not even sure even a compiler barrier is necessary at > > > all > here). > > > > > > > #ifndef NDEBUG > > > > if (unlikely((cqe->owner_sr_opcode & > > > MLX4_CQE_OPCODE_MASK) == > > > > MLX4_CQE_OPCODE_ERROR)) { @@ -203,7 +203,7 > @@ struct pv { > > > > */ > > > > cq->cons_index = cons_index; > > > > *cq->set_ci_db = rte_cpu_to_be_32(cq->cons_index & > > > MLX4_CQ_DB_CI_MASK); > > > > - rte_wmb(); > > > > + rte_io_wmb(); > > > > > > This one could be removed entirely as well, which is more or less > > > what the move to a compiler barrier does. Nothing in subsequent code > > > depends on this doorbell being written, so this can piggy back on > > > any subsequent rte_wmb(). > > > > Yes, you right, probably this code was taken from multi thread > implementation. > > > > > > On the other hand in my opinion a barrier (compiler or otherwise) > > > might be needed before the doorbell write, to make clear it cannot > > > somehow be done earlier in case something attempts to optimize it > away. > > > > > I think we can remove it entirely (compiler can't optimize the ci_db store > since in depends in previous code (cons_index). > > Right, however you may still run into issues if the compiler determines the > final cons_index value by looking at the loop and decides to store it before > entering/leaving it. That's the kind of problematic optimization I was > thinking > of. > > The barrier in that sense is just to assert the order of seemingly unrelated > load/stores.
I think that If I left the rte_io_rmb after CQE owner check we can earn both: 1. The concern of read ordering while reading the owner before using CQE. 2. The ci_db concern: the compiler must read the last CQE(which is not valid and we have no more stamp to do) before it knows the last value of cons_index. So we can remove entirely this rte_io_wmb in completion function. What do you think? > > > > /* Fill the control parameters for this packet. > > > > */ @@ - > > > 533,7 > > > > +534,7 @@ static int handle_multi_segs(struct rte_mbuf *buf, > > > > * setting ownership bit (because HW can start > > > > * executing as soon as we do). > > > > */ > > > > - rte_wmb(); > > > > + rte_io_wmb(); > > > > > > This one looks dangerous. A compiler barrier is not strong enough to > > > guarantee the order in which CPU will execute instructions, it only > > > makes sure what follows the barrier doesn't appear before it in the > generated code. > > > > > As I investigated, I understood that for CPUs which don't save store order > between different addresses(arm,ppc), the rte_io_wmb is converted to > rte_wmb. > > So for thus who save it(x86) we just need the right order in compiler code > because all the relevant stores are targeted to same memory domain(DRAM) > and therefore also the actual store is guaranteed. > > Unlike doorbell store which directed to different memory domain (PCI). > > So the only place which need rte_wmb() is before doorbell write. > > Fair enough, although after re-reading the code I think there's another issue > present since the beginning: both ctrl and dseg pointers are not volatile, > this > fact doesn't guarantee intermediate writes will occur in the expected order > or even at all, even in the presence of a barrier. > > The volatile attribute should be inherited from both struct mlx4_cq and struct > mlx4_sq (buf, db and most if not all other pointers). I think a separate fixes > commit should add it for safety. Great notice , I will add it, Thanks! > > > > Unless the comment above this barrier is wrong, this change may > > > cause hard- to-debug issues down the road, you should drop it. > > > > > > > ctrl->owner_opcode = rte_cpu_to_be_32(owner_opcode | > > > > ((sq->head & > > > > sq->txbb_cnt) ? > > > > MLX4_BIT_WQE_OWN > > > > : > > > 0)); > > > > -- > > > > 1.8.3.1 > > > > > > > > > > -- > > > Adrien Mazarguil > > > 6WIND > > > > Thanks! > > -- > Adrien Mazarguil > 6WIND
