[ovs-dev] [PATCH v2.40 0/7] MPLS actions and matches
Joe Stringer
joe at wand.net.nz
Sun Sep 29 02:35:56 UTC 2013
Hi Simon,
I've dropped some comments on a couple of the userspace patches, but I
probably don't need to comment on everything as Ben's given some pretty
clear feedback. The crux would be dropping #3 in favour of something a bit
tidier. Feel free to prod my brain if need be :-).
On Fri, Sep 27, 2013 at 12:18 PM, Simon Horman <horms at verge.net.au> wrote:
> Hi,
>
> This series implements MPLS actions and matches based on work by
> Ravi K, Leo Alterman, Yamahata-san and Joe Stringer.
>
> This series provides two changes
>
> * Patches 1 - 5
>
> Provide user-space support for the VLAN/MPLS tag insertion order
> up to and including OpenFlow 1.2, and the different ordering
> specified from OpenFlow 1.3. In a nutshell the datapath always
> uses the OpenFlow 1.3 ordering, which is to always insert tags
> immediately after the L2 header, regardless of the presence of other
> tags. And ovs-vswtichd provides compatibility for the behaviour up
> to OpenFlow 1.2, which is that MPLS tags should follow VLAN tags
> if present.
>
> Ben, these are for you to review.
>
> * Patches 6 and 7
>
> Adding basic MPLS action and match support to the kernel datapath
>
> Jesse, these are for you to review.
>
>
> Differences between v2.40 and v2.39:
>
> * Rebase for:
> + New dev_queue_xmit compat code
> + Updated put_vlan()
> + Removal of mpls_depth field from struct flow
> * As suggested by Jesse Gross
> + Remove bogus mac_len update from push_mpls()
> + Slightly simplify push_mpls() by using eth_hdr()
> + Remove dubious condition !eth_p_mpls(inner_protocol) on
> an skb being considered to be MPLS in netdev_send()
> + Only use compatibility code for MPLS GSO segmentation on kernels
> older than 3.11
> + Revamp setting of inner_protocol
> 1. Do not unconditionally set inner_protocol to the value of
> skb->protocol in ovs_execute_actions().
> 2. Initialise inner_protocol it to zero only if compatibility code is
> in
> use. In the case where compatibility code is not in use it will
> either
> be zero due since the allocation of the skb or some other value set
> by some other user.
> 3. Conditionally set the inner_protocol in push_mpls() to the value of
> skb->protocol when entering push_mpls(). The condition is that
> inner_protocol is zero and the value of skb->protocol is not an MPLS
> ethernet type.
> - This new scheme:
> + Pushes logic to set inner_protocol closer to the case where it is
> needed.
> + Avoids over-writing values set by other users.
> * As suggested by Pravin Shelar
> + Only set and restore skb->protocol in rpl___skb_gso_segment() in the
> case of MPLS
> + Add inner_protocol field to struct ovs_gso_cb instead of ovs_skb_cb.
> This moves compatibility code closer to where it is used
> and creates fewer differences with mainline.
> * Update comment on mac_len updates in datapath/actions.c
> * Remove HAVE_INNER_PROCOTOL and instead just check
> against kernel version 3.11 directly.
> HAVE_INNER_PROCOTOL is a hang-over from work done prior
> to the merge of inner_protocol into the kernel.
> * Remove dubious condition !eth_p_mpls(inner_protocol) on
> using inner_protocol as the type in rpl_skb_network_protocol()
> * Do not update type of features in rpl_dev_queue_xmit.
> Though arguably correct this is not an inherent part of
> the changes made by this patch.
> * Use skb_cow_head() in push_mpls()
> + Call skb_cow_head(skb, MPLS_HLEN) instead of
> make_writable(skb, skb->mac_len) to ensure that there is enough head
> room to push an MPLS LSE regardless of whether the skb is cloned or
> not.
> + This is consistent with the behaviour of rpl__vlan_put_tag().
> + This is a fix for crashes reported when performing mpls_push
> with headroom less than 4. This problem was introduced in v3.36.
> * Skip popping in mpls_pop if the skb is too short to contain an MPLS LSE
>
>
> Differences between v2.39 and v2.38:
>
> * Rebase for removal of vlan, checksum and skb->mark compat code
> - This includes adding adding a new patch,
> "[PATCH v2.39 6/7] datapath: Break out deacceleration portion of
> vlan_push" to allow re-use of some existing code.
>
>
> Differences between v2.38 and v2.37:
>
> * Rebase for SCTP support
> * Refactor validate_tp_port() to iterate over eth_types rather
> than open-coding the loop. With the addition of SCTP this logic
> is now used three times.
>
>
> Differences between v2.37 and v2.36:
>
> * Rebase
>
>
> Differences between v2.36 and v2.35:
>
> * Rebase
>
> * Do not add set_ethertype() to datapath/actions.c.
> As this patch has evolved this function had devolved into
> to sets of functionality wrapped into a single function with
> only one line of common code. Refactor things to simply
> open-code setting the ether type in the two locations where
> set_ethertype() was previously used. The aim here is to improve
> readability.
>
> * Update setting skb->ethertype after mpls push and pop.
> - In the case of push_mpls it should be set unconditionally
> as in v2.35 the behaviour of this function to always push
> an MPLS LSE before any VLAN tags.
> - In the case of mpls_pop eth_p_mpls(skb->protocol) is a better
> test than skb->protocol != htons(ETH_P_8021Q) as it will give the
> correct behaviour in the presence of other VLAN ethernet types,
> for example 0x88a8 which is used by 802.1ad. Moreover, it seems
> correct to update the ethernet type if it was previously set
> according to the top-most MPLS LSE.
>
> * Deaccelerate VLANs when pushing MPLS tags the
> - Since v2.35 MPLS push will insert an MPLS LSE before any VLAN tags.
> This means that if an accelerated tag is present it should be
> deaccelerated to ensure it ends up in the correct position.
>
> * Update skb->mac_len in push_mpls() so that it will be correct
> when used by a subsequent call to pop_mpls().
>
> As things stand I do not believe this is strictly necessary as
> ovs-vswitchd will not send a pop MPLS action after a push MPLS action.
> However, I have added this in order to code more defensively as I believe
> that if such a sequence did occur it would be rather unobvious why
> it didn't work.
>
> * Do not add skb_cow_head() call in push_mpls().
> It is unnecessary as there is a make_writable() call.
> This change was also made in v2.30 but some how the
> code regressed between then and v2.35.
>
>
> Differences between v2.35 and v2.34:
>
> * Add support for the tag ordering specified up until OpenFlow 1.2 and
> the ordering specified from OpenFlow 1.3.
>
> * Correct error in datapath patch's handling of GSO in the presence
> of MPLS and absence of VLANs.
>
>
> Pre-requisites.
>
> This series applies on top of "[PATCH v3] Remove mpls_depth field from
> flow"
>
> To aid review this series and its pre-requisite is available in git at:
>
> git://github.com/horms/openvswitch.git devel/mpls-v2.40
>
>
> Patch list and overall diffstat:
>
> Joe Stringer (5):
> odp: Only pass vlan_tci to commit_vlan_action()
> odp: Allow VLAN actions after MPLS actions
> ofp-actions: Add OFPUTIL_OFPAT13_PUSH_MPLS
> ofp-actions: Add separate OpenFlow 1.3 action parser
> lib: Push MPLS tags in the OpenFlow 1.3 ordering
>
> Simon Horman (2):
> datapath: Break out deacceleration portion of vlan_push
> datapath: Add basic MPLS support to kernel
>
> datapath/Modules.mk | 1 +
> datapath/actions.c | 156 ++++++++-
> datapath/datapath.c | 259 ++++++++++++--
> datapath/datapath.h | 2 +
> datapath/flow.c | 58 ++-
> datapath/flow.h | 17 +-
> datapath/linux/compat/gso.c | 117 ++++++-
> datapath/linux/compat/gso.h | 53 +++
> datapath/linux/compat/include/linux/netdevice.h | 14 +-
> datapath/linux/compat/netdevice.c | 28 --
> datapath/mpls.h | 15 +
> include/linux/openvswitch.h | 7 +-
> lib/flow.c | 2 +-
> lib/odp-util.c | 21 +-
> lib/odp-util.h | 2 +-
> lib/ofp-actions.c | 68 +++-
> lib/ofp-parse.c | 1 +
> lib/ofp-util.c | 3 +
> lib/ofp-util.h | 1 +
> lib/packets.c | 10 +-
> lib/packets.h | 2 +-
> ofproto/ofproto-dpif-xlate.c | 98 ++++--
> ofproto/ofproto-dpif-xlate.h | 5 +
> tests/ofproto-dpif.at | 446
> ++++++++++++++++++++++++
> 24 files changed, 1246 insertions(+), 140 deletions(-)
> create mode 100644 datapath/mpls.h
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openvswitch.org/pipermail/ovs-dev/attachments/20130929/faf55b65/attachment-0003.html>
More information about the dev
mailing list