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