On Aug 28, 2014, at 11:28 AM, Ben Pfaff <b...@nicira.com> wrote:
> On Fri, Aug 22, 2014 at 01:58:21PM -0700, Jarno Rajahalme wrote:
>> The atomics here do not synchronize the state of any other variables,
>> so we can use relaxed atomics.
>>
>> cfm_should_process_flow() is rearranged to set the megaflow mask bits
>> only if necessary, and to avoid the atomic operation on non-BFD
>> packets.
>>
>> Signed-off-by: Jarno Rajahalme <jrajaha...@nicira.com>
>> + /* Most packets are not CFM. */
>> + if (OVS_LIKELY(ntohs(flow->dl_type) != ETH_TYPE_CFM)) {
>> + return false;
>> + }
>
> The common recommendation is to htons the constant instead of ntohsing
> the variable, to generate better code.
(snip)
> so it's still worthwhile. (I was surprised to see Clang generate
> worse code overall. It seems to regard a bool as a 32-bit value, at
> least for the purpose of returning from a function.)
>
Thanks for the reminder and analysis!
>> + atomic_read_relaxed(&cfm->check_tnl_key, &check_tnl_key);
>> +
>> + /* Atomic read is always a memory access, do some work
>> + * before we need the result. */
>
> I believe an atomic_read_relaxed() is just a compiler barrier.
It is not even that (no barrier at all).
> That
> means that unless cfm_should_process_flow() is getting inlined
> somewhere (I doubt it--it has no callers within cfm.c), there isn't
> any way that the memory access could be optimized out anyway. So,
> this optimization and comment seems a little silly to me.
Agree, should let the compiler worry about that.
Jarno
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
