[ovs-dev] [PATCH 10/18] lib/cfm: Use relaxed atomics and optimize cfm_should_process_flow().

Jarno Rajahalme jrajahalme at nicira.com
Fri Aug 29 17:25:00 UTC 2014


On Aug 28, 2014, at 11:28 AM, Ben Pfaff <blp at 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 <jrajahalme at 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




More information about the dev mailing list