[ovs-dev] [PATCH v3 04/10] datapath: Compact sw_flow_key.

Jarno Rajahalme jrajahalme at nicira.com
Sun Mar 30 02:33:57 UTC 2014


On Mar 28, 2014, at 2:52 PM, Jesse Gross <jesse at nicira.com> wrote:

> On Fri, Mar 28, 2014 at 8:50 AM, Jarno Rajahalme <jrajahalme at nicira.com> wrote:
>> 
>>> On Mar 27, 2014, at 3:59 PM, Jesse Gross <jesse at nicira.com> wrote:
>>> 
>>>> On Fri, Feb 21, 2014 at 11:41 AM, Jarno Rajahalme <jrajahalme at nicira.com> wrote:
>>>> Minimize padding in sw_flow_key and move 'tp' top the main struct.
>>>> These changes simplify code when accessing the transport port numbers
>>>> and the tcp flags, and makes the sw_flow_key 8 bytes smaller on 64-bit
>>>> systems (128->120 bytes).  These changes also make the keys for IPv4
>>>> packets to fit in one cache line.
>>>> 
>>>> There is a valid concern for safety of packing the struct
>>>> ovs_key_ipv4_tunnel, as it would be possible to take the address of
>>>> the tun_id member as a __be64 * which could result in unaligned access
>>>> in some systems. However:
>>>> 
>>>> - sw_flow_key itself is 64-bit aligned, so the tun_id within is always
>>>> 64-bit aligned.
>>>> - We never make arrays of ovs_key_ipv4_tunnel (which would force every
>>>> second tun_key to be misaligned).
>>>> - We never take the address of the tun_id in to a __be64 *.
>>>> - Whereever we use struct ovs_key_ipv4_tunnel outside the sw_flow_key,
>>>> it is in stack (on tunnel input functions), where compiler has full
>>>> control of the alignment.
>>> 
>>> I'm not sure that I understand the last comment here. On the stack,
>>> the compiler does have control over the layout but it will presumably
>>> use the alignment specified here when doing that layout.
>> 
>> Maybe the last comment is just redundant, then.
> 
> But doesn't it mean that we could now have unaligned accesses?


Access to tun_id member can be unaligned, but the compiler should handle it, so it is safe. It seems we do it exactly once for ingress (ovs_tun_key_init()) and egress from/to a tunnel in the fast path, so if it needs to be as two 32-bit units in some targets, it should not matter.

I see the commit message could have been clearer, though :-( There are two concerns: safety and performance. As we never pass a tun_id member pointer as __be64 *, this should be safe. For performance, some architectures must use multiple instructions to access a “4-aligned” tun_id, but as we have such accesses once per ingress/egress, it does not matter in practice. Other architectures use one instruction and face a small penalty for unaligned access. At the time of writing the commit message I was trying to reason when that unaligned access would occur.

Also, now that I looked into this again, it seems to me that a tun_key in the action list can be unaligned, as netlink attribute alignment is 4. If set tunnel action follows pop vlan action, for example, the tun_key would not be 8-aligned. This would be a safety issue with the tun_key as we had it before this patch as we do the following in actions.c/execute_set_action():

	case OVS_KEY_ATTR_IPV4_TUNNEL:
		OVS_CB(skb)->tun_key = nla_data(nested_attr);
		break;

Jarno






More information about the dev mailing list