On Wed, Jan 16, 2013 at 11:24:42AM -0800, Jesse Gross wrote:
> On Tue, Jan 15, 2013 at 11:05 PM, Ben Pfaff <b...@nicira.com> wrote:
> > On Tue, Jan 08, 2013 at 02:46:02PM +0900, Simon Horman wrote:
> >> diff --git a/include/linux/openvswitch.h b/include/linux/openvswitch.h
> >> index 5e32965..b421753 100644
> >> --- a/include/linux/openvswitch.h
> >> +++ b/include/linux/openvswitch.h
> >> @@ -282,6 +282,7 @@ enum ovs_key_attr {
> >> OVS_KEY_ATTR_ARP, /* struct ovs_key_arp */
> >> OVS_KEY_ATTR_ND, /* struct ovs_key_nd */
> >> OVS_KEY_ATTR_SKB_MARK, /* u32 skb mark */
> >> + OVS_KEY_ATTR_MPLS, /* struct ovs_key_mpls */
> >> OVS_KEY_ATTR_IPV4_TUNNEL, /* struct ovs_key_ipv4_tunnel */
> >> OVS_KEY_ATTR_TUN_ID = 63, /* be64 tunnel ID */
> >> __OVS_KEY_ATTR_MAX
> >
> > I'm pretty sure we shouldn't be inserting OVS_KEY_ATTR_MPLS in front of
> > another value that we're already aiming to get upstream. I'd suggest a
> > value of 62.
> >
> > Jesse, does that sound reasonable to you? Another alternative would be
> > to avoid modifying <linux/openvswitch.h> entirely, one way or another,
> > until we have real kernel support, which would be a little extra work.
>
> It's not ideal but I think it's fine for the time being.
> Incidentally, Pravin is working on something similar from the kernel
> side to allow transformation of Netlink tunnel attributes into
> something more efficient for packet processing.
>
> To ensure that there is no leakage, I would do two things: wrap all
> MPLS definitions in #ifndef __KERNEL__ blocks and check that we don't
> have any handling code into odp-util.c. That way if another new
> attribute later starts using this temporary value then an old version
> of OVS userspace will ignore it rather than trying to interprete it as
> MPLS.
OVS_KEY_ATTR_MPLS is used in userspace:
* lib/odp-util.c uses it in flow key handling routines.
In particular in:
- ovs_key_attr_to_string()
- odp_flow_key_attr_len()
- format_odp_key_attr()
- parse_odp_key_attr()
- odp_flow_key_from_flow()
- parse_l3_onward()
- commit_mpls_action() [new]
* lib/dpif-netdev.c uses it to allow the user-space datapath to handle
MPLS. In particular in execute_set_action().
> If we do that then we can just put the MPLS attribute directly after
> IPV4_TUNNEL and new attributes would be inserted in between. The MPLS
> value could change but that shouldn't matter because there the value
> would be entirely internal. This seems easier to deal with than
> allocating at both ends.
Sure, I'll move OVS_KEY_ATTR_MPLS to after OVS_KEY_ATTR_IPV4_TUNNEL.
> > I'm not sure why we have OVS_ACTION_ATTR_SET_MPLS instead of using
> > OVS_ACTION_SET to set OVS_KEY_ATTR_MPLS.
>
> For the PUSH/POP actions I think we also have the same issue with
> inserting in the middle of the list and can use the same solution.
I'm unsure what "inserting in the middle of the list" refers to.
However, when a flow key is created it should be done so in such
away that there may be at most one element from the following list present:
- OVS_ACTION_SET/OVS_KEY_ATTR_MPLS (previously OVS_ACTION_ATTR_SET_MPLS)
- OVS_ACTION_ATTR_PUSH_MPLS
- OVS_ACTION_ATTR_POP_MPLS
I don't believe that the above is enforced anywhere, however it
should be the way the flow key creation code currently works.
> In the comment about the EtherType above struct ovs_action_push_mpls I
> would also say that non-MPLS EtherTypes will get rejected rather than
> result in a corrupt packet since that seems like better behavior.
I believe that is the behaviour in the code. I'll double check and
add the comment as you suggest.
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
