On Wed, Jan 16, 2013 at 07:04:06PM -0800, Jesse Gross wrote:
> On Wed, Jan 16, 2013 at 4:27 PM, Simon Horman <ho...@verge.net.au> wrote:
> > 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().
>
> It's the usage in the userspace/kernel interfaces that I'm concerned
> about. If we're saying that this is essentially an internal
> identifier to userspace then there's no benefit to having the code
> that communicates that value to the kernel at this time. If we have
> it but don't use then it's possible that there will be a conflict in
> the future if there is a different user of this temporary identifier.
My understanding is that fundamentally OVS_KEY_ATTR_MPLS is used for as
part of the userspace/kernel interface and that thus code on both sides
uses it. Likewise for OVS_ACTION_ATTR_PUSH_MPLS and OVS_ACTION_ATTR_POP_MPLS.
Perhaps a good approach is to use a values towards the end of the possible
space for OVS_KEY_ATTR_MPLS, OVS_ACTION_ATTR_PUSH_MPLS and
OVS_ACTION_ATTR_POP_MPLS, as has been done for OVS_KEY_ATTR_TUN_ID, to
reduce the likely hood of a clash with another identifier in the short to
medium term.
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
