On Thu, Nov 21, 2013 at 12:46:41PM +0900, Simon Horman 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 three changes
>
> * Patches 1 - 3
>
> 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.
Hi Ben,
I wonder if you could find some time to look over these.
I believe that the first patch has not previously been reviewed
but other than that there should be few areas of excitement.
>
> * Patches 4 and 5
>
> Adding basic MPLS action and match support to the kernel datapath
>
> Jesse, these are for you to review after patches 1 - 4 are reviewed.
>
>
> Difference between v2.51 and v2.50:
>
> * New approach to consistency checking actions using of OpenFlow1.3+ tag order
> * Use OF1.2 for all ovs-ofctl commands for new OF1.2 tests. Likewise for
> OF1.3.
> * Add write_actions check for OF1.3.
> This further exercises consistency checking using OpenFlow1.3+ tag order.
>
>
> Difference between v2.50 and v2.49:
>
> * Correct typo in comment
>
>
> Difference between v2.49 and v2.48:
>
> * Include action consistency checking changes.
>
>
> Difference between v2.48 and v2.47:
> * Manual rebase for
> - "OF 1.1 set vlan vid/pcp compatibility"
> - "Native Set-Field action"
> * Only use OpenFlow1.2 actions in OpenFlow1.2 tests
>
>
> Difference between v2.47 and v2.46:
>
> * Rebase of patch 4 for HAVE_RHEL_OVS_HOOK and OVS_KEY_ATTR_TCP_FLAGS
>
>
> Difference between v2.46 and v2.45:
>
> * Update changelog of "odp: Allow VLAN actions after MPLS actions"
> to reflect the current implementation
>
>
> Difference between v2.45 and v2.44:
>
> * As pointed out by Ben Pfaff and Joe Stringer
> + Update VLAN handling in the presence of MPLS push
>
> Previously the test for committing ODP VLAN actions after MPLS actions
> was that the VLAN TCI differed before and after the MPLS push action.
> This results in a false negative in some cases including if a VLAN tag
> is pushed after the MPLS push action in such a way that it duplicates
> the VLAN tag present before the MPLS push action.
>
> This is resolved by ensuring the VLAN_CFI bit of the value used to
> track the desired VLAN TCI after an MPLS push action is zero unless
> VLAN actions should be committed after MPLS actions.
>
> + Update tests to use ovs-ofctl monitor "-m" to allow full inspection of
> MPLS LSE and VLAN tags present in packets.
>
> Differences between v2.44 and v2.43:
>
> * Rebase for the following changes:
> f47ea02 ("Set datapath mask bits when setting a flow field.")
> 7fdb60a ("Add support for write-actions")
> 7358063 ("odp-util: Elaborate the comment for odp_flow_format() function.")
> * Correct final_vlan_tci and next_vlan_tci initialisation in xlate_actions__()
>
>
> Differences between v2.43 and v2.42:
>
> * As suggested by Ben Pfaff
> Move vlan state into struct xlate_ctx
> 1. Add final_vlan_tci member to struct xlate_ctx instead of vlan_tci member
> struct xlate_xin. This seems to be a better palace for it as it does
> not need to be accessible from the caller.
> 2. Move local vlan_tci variable of do_xlate_actions() to be the
> next_vlan_tci member of strict xlate_ctx. This allows for it to work
> across resubmit actions and goto table instructions.
> * Code contributed by Ben Pfaff
> + Use enum for to control order of MPLS LSE insertion
> This makes the logic somewhat clearer
> * Add a helper push_mpls_from_openflow() to consolidate
> the same logic that appears in three locations.
>
>
> Differences between v2.42 and v2.41:
>
> * Rebase for:
> + 0585f7a ("datapath: Simplify mega-flow APIs.")
> + a097c0b ("datapath: Restructure datapath.c and flow.c")
> * As suggested by Jesse Gross
> + Take into account that push_mpls() will have freed the skb on error
> + Remove dubious !eth_p_mpls(skb->protocol) condition from push_mpls
> The !eth_p_mpls(skb->protocol) condition on setting inner_protocol
> has no effect. Its motivation was to ensure that inner_protocol was
> only set the first time that mpls_push occured. However this is already
> ensured by the !ovs_skb_get_inner_protocol(skb) condition.
> + Return -EINVAL instead of -ENOMEM from pop_mpls() if the skb is too short
> + Do not add @inner_protocol to kernel doc for struct ovs_skb_cb.
> The patch no longer adds an inner_protocol member to struct ovs_skb_cb
> + Do not add and set otherwise unsued inner_protocol variable in
> rpl_dev_queue_xmit()
> * As suggested by Pravin Shelar
> + Implement compatibility code in existing rpl_skb_gso_segment
> rather than introducing to use rpl___skb_gso_segment
>
>
> Differences between v2.41 and v2.40:
>
> * As suggested by Ben Pfaff
> + Expand struct ofpact_reg_load to include a mpls_before_vlan field
> rather than using the compat field of the ofpact field of
> struct ofpact_reg_load.
> + Pass version to ofpacts_pull_openflow11_actions and
> ofpacts_pull_openflow11_instructions. This removes the invalid
> assumption that that these functions are passed a full message and are
> thus able to deduce the OpenFlow version.
>
>
> 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.
>
>
> To aid review this series is available in git at:
>
> git://github.com/horms/openvswitch.git devel/mpls-v2.51
>
>
> Patch list and overall diffstat:
>
> Joe Stringer (1):
> odp: Allow VLAN actions after MPLS actions
>
> Simon Horman (4):
> ofp-actions: Allow Consistency checking of OF1.3+ VLAN actions after
> mpls_push
> lib: Support pushing of MPLS LSE before or after VLAN tag
> datapath: Break out deacceleration portion of vlan_push
> datapath: Add basic MPLS support to kernel
>
> OPENFLOW-1.1+ | 12 -
> datapath/Modules.mk | 1 +
> datapath/actions.c | 157 ++++-
> datapath/datapath.c | 4 +-
> datapath/flow.c | 29 +
> datapath/flow.h | 17 +-
> datapath/flow_netlink.c | 286 +++++++-
> datapath/flow_netlink.h | 2 +-
> datapath/linux/compat/gso.c | 70 +-
> datapath/linux/compat/gso.h | 41 ++
> datapath/linux/compat/include/linux/netdevice.h | 6 +-
> datapath/linux/compat/netdevice.c | 10 +-
> datapath/mpls.h | 15 +
> include/linux/openvswitch.h | 7 +-
> lib/flow.c | 2 +-
> lib/odp-util.c | 12 +-
> lib/odp-util.h | 3 +-
> lib/ofp-actions.c | 194 +++++-
> lib/packets.c | 10 +-
> lib/packets.h | 2 +-
> ofproto/ofproto-dpif-xlate.c | 161 ++++-
> tests/ofproto-dpif.at | 869
> ++++++++++++++++++++++++
> 22 files changed, 1765 insertions(+), 145 deletions(-)
> create mode 100644 datapath/mpls.h
>
> --
> 1.8.4
>
> _______________________________________________
> dev mailing list
> dev@openvswitch.org
> http://openvswitch.org/mailman/listinfo/dev
>
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
