On Mar 28, 2014, at 2:52 PM, Jesse Gross <je...@nicira.com> wrote:
> On Fri, Mar 28, 2014 at 8:50 AM, Jarno Rajahalme <jrajaha...@nicira.com>
> wrote:
>>
>>> On Mar 27, 2014, at 3:59 PM, Jesse Gross <je...@nicira.com> wrote:
>>>
>>>> On Fri, Feb 21, 2014 at 11:41 AM, Jarno Rajahalme <jrajaha...@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
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
