On Sat, Mar 29, 2014 at 7:33 PM, Jarno Rajahalme <jrajaha...@nicira.com> wrote: > > 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.
OK, I see now. Thanks for walking through the analysis. > 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; > Hmm, that is an interesting point. _______________________________________________ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
