Re: [PATCH] atomic: clarify use of memory barriers

[Olivier MATZ <olivier.matz-pdR9zngts4EAvxtiuMwx3w@public.gmane.org>]

Hi Konstantin,

Thank you for your review and feedback.

On 05/20/2014 12:05 PM, Ananyev, Konstantin wrote:
>> Note that on x86 CPUs, memory barriers between different cores can be guaranteed by a simple compiler barrier.
>
> I don't think this is totally correct.
> Yes, for  Intel cpus in many cases memory barrier could be avoided due to nearly strict memory ordering.
> Though there are few cases where reordering is possible and when fence instructions would be needed.

I tried to mimic the behavior of linux that differentiates *mb() from
smp_*mb(), but I did too fast. In linux, we have [1]:

   smp_mb() = mb() = asm volatile("mfence":::"memory")
   smp_rmb() = compiler_barrier()
   smp_wmb() = compiler_barrier()

At least this should fixed in the patch. By the way, just for reference,
the idea of the patch came from a discussion we had on the list [2].

> For me:
> +#define	rte_smp_rmb() rte_compiler_barrier()
> Seems a bit misleading, as there is no real fence.
> So I suggest we keep rte_compiler_barrier() naming and usage.

The objectives of the patch (which was probably not explained very
clearly in the commit log) were:
- make the code more readable to distinguish between the 2 kinds of
   memory barrier.
- optimize some code to avoid a real memory barrier when not required
   (timers, virtio, ...)

Having a compiler barrier in place of a memory barrier in the code
does not really help to understand what the developper wanted to do.
In the current code we can see that the use of rte_compiler_barrier()
is ambiguous, as it need a comment to clarify the situation:

	rte_compiler_barrier();   /* rmb */

Don't you think we could fix the patch but keep its logic?

Regards,
Olivier

[1] http://lxr.free-electrons.com/source/arch/x86/include/asm/barrier.h#L81
[2] http://dpdk.org/ml/archives/dev/2014-March/001741.html