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

Reply via email to