Hi Martin, I see you already sent out a v6 before I could respond, so I will move/respond to the open items to the v6 review.
This way, all the correspondence is in one thread. //Eelco On 28 Sep 2021, at 15:35, Martin Varghese wrote: > On Fri, Sep 24, 2021 at 02:30:16PM +0200, Eelco Chaudron wrote: >> Hi Martin, >> >> See my comments below... >> >> Cheers, >> >> Eelco >> >> On 30 Aug 2021, at 14:40, Martin Varghese wrote: >> >>> From: Martin Varghese <[email protected]> >>> >>> The encap & decap actions are extended to support MPLS packet type. >>> Encap & decap actions adds and removes MPLS header at start of the >>> packet. >>> >>> The existing PUSH MPLS & POP MPLS actions inserts & removes MPLS >>> header between ethernet header and the IP header. Though this behaviour >>> is fine for L3 VPN where an IP packet is encapsulated inside a MPLS >>> tunnel, it does not suffice the L2 VPN requirements. In L2 VPN the >>> ethernet packets must be encapsulated inside MPLS tunnel. >>> >>> In this change the encap & decap actions are extended to support MPLS >>> packet type. The encap & decap adds and removes MPLS header at the >>> start of packet as depicted below. >>> >>> Encapsulation: >>> >>> Actions - encap(mpls(ether_type=0x8847)),encap(ethernet) >>> >>> Incoming packet -> | ETH | IP | Payload | >>> >>> 1 Actions - encap(mpls(ether_type=0x8847)) [Datapath action - >>> ADD_MPLS:0x8847] >>> >>> Outgoing packet -> | MPLS | ETH | Payload| >>> >>> 2 Actions - encap(ethernet) [ Datapath action - push_eth ] >>> >>> Outgoing packet -> | ETH | MPLS | ETH | Payload| >>> >>> Decapsulation: >>> >>> Incoming packet -> | ETH | MPLS | ETH | IP | Payload | >>> >>> Actions - decap(),decap(packet_type(ns=0,type=0) >>> >>> 1 Actions - decap() [Datapath action - pop_eth) >>> >>> Outgoing packet -> | MPLS | ETH | IP | Payload| >>> >>> 2 Actions - decap(packet_type(ns=0,type=0) [Datapath action - >>> POP_MPLS:0x6558] >>> >>> Outgoing packet -> | ETH | IP | Payload| >>> >>> Signed-off-by: Martin Varghese <[email protected]> >>> --- >>> Changes in v2: >>> - Fixed the compilation error reported by bot. >>> >>> Changes in v3: >>> - Adapted the changes to allign with the kernel implementaion >>> >>> Changes in v4: >>> - Fixed the compilation error reported by bot. >>> - Added SLOW_ACTION support for no datapath support. >>> >>> Changes in v5: >>> >>> - Code styling fixed. >>> - Given reference for packet_type field in documentation. >>> - Modified code to do recirc only for the last label. >>> - Cleaed l2,l3,l4 fields with encap. >>> - Fixed existing encap & decap tests to do set eth to outer header >>> - Added tests to verify Encap & Decap with legacy Push & Pop. >>> - Added xlate tests. >>> - Added seperate tests to inspect packet after encap & decap. >>> - Added tests for multiple mpls test cases. >>> >>> NEWS | 2 +- >>> .../linux/compat/include/linux/openvswitch.h | 33 ++- >>> include/openvswitch/ofp-ed-props.h | 18 ++ >>> lib/dpif-netdev.c | 1 + >>> lib/dpif.c | 1 + >>> lib/odp-execute.c | 12 + >>> lib/odp-util.c | 50 +++- >>> lib/ofp-actions.c | 5 + >>> lib/ofp-ed-props.c | 91 +++++++ >>> lib/ovs-actions.xml | 32 ++- >>> lib/packets.c | 47 +++- >>> lib/packets.h | 2 + >>> ofproto/ofproto-dpif-ipfix.c | 1 + >>> ofproto/ofproto-dpif-sflow.c | 1 + >>> ofproto/ofproto-dpif-xlate.c | 74 ++++++ >>> ofproto/ofproto-dpif.c | 39 +++ >>> ofproto/ofproto-dpif.h | 4 +- >>> tests/mpls-xlate.at | 47 ++++ >>> tests/system-traffic.at | 227 ++++++++++++++++++ >>> 19 files changed, 664 insertions(+), 23 deletions(-) >>> >>> diff --git a/NEWS b/NEWS >>> index 1f2adf718..8afc62534 100644 >>> --- a/NEWS >>> +++ b/NEWS >>> @@ -209,7 +209,7 @@ v2.14.0 - 17 Aug 2020 >>> - GTP-U Tunnel Protocol >>> * Add two new fields: tun_gtpu_flags, tun_gtpu_msgtype. >>> * Only support for userspace datapath. >>> - >>> + - Encap & Decap action support for MPLS packet type. >>> >>> v2.13.0 - 14 Feb 2020 >>> --------------------- >>> diff --git a/datapath/linux/compat/include/linux/openvswitch.h >>> b/datapath/linux/compat/include/linux/openvswitch.h >>> index f0595eeba..88a653771 100644 >>> --- a/datapath/linux/compat/include/linux/openvswitch.h >>> +++ b/datapath/linux/compat/include/linux/openvswitch.h >>> @@ -817,8 +817,32 @@ struct ovs_action_push_tnl { >>> }; >>> #endif >>> >>> -/** >>> - * enum ovs_ct_attr - Attributes for %OVS_ACTION_ATTR_CT action. >>> +/* struct ovs_action_add_mpls - %OVS_ACTION_ATTR_ADD_MPLS action >>> + * argument. >>> + * @mpls_lse: MPLS label stack entry to push. >>> + * @mpls_ethertype: Ethertype to set in the encapsulating ethernet frame. >>> + * @tun_flags: MPLS tunnel attributes. >>> + * >>> + * The only values @mpls_ethertype should ever be given are %ETH_P_MPLS_UC >>> and >>> + * %ETH_P_MPLS_MC, indicating MPLS unicast or multicast. Other are >>> rejected. >>> + */ >>> +struct ovs_action_add_mpls { >>> + __be32 mpls_lse; >>> + __be16 mpls_ethertype; /* Either %ETH_P_MPLS_UC or %ETH_P_MPLS_MC */ >>> + __u16 tun_flags; >>> +}; >>> + >>> +#define OVS_MPLS_L3_TUNNEL_FLAG_MASK (1 << 0) /* Flag to specify the >>> place of >>> + * insertion of MPLS header. >>> + * When false, the MPLS header >>> + * will be inserted at the start >>> + * of the packet. >>> + * When true, the MPLS header >>> + * will be inserted at the start >>> + * of the l3 header. >>> + */ >>> + >>> +/* enum ovs_ct_attr - Attributes for %OVS_ACTION_ATTR_CT action. >>> * @OVS_CT_ATTR_COMMIT: If present, commits the connection to the conntrack >>> * table. This allows future packets for the same connection to be >>> identified >>> * as 'established' or 'related'. The flow key for the current packet will >>> @@ -1008,6 +1032,10 @@ struct check_pkt_len_arg { >>> * @OVS_ACTION_ATTR_CHECK_PKT_LEN: Check the packet length and execute a >>> set >>> * of actions if greater than the specified packet length, else execute >>> * another set of actions. >>> + * @OVS_ACTION_ATTR_ADD_MPLS: Push a new MPLS label stack entry at the >>> + * start of the packet or at the start of the l3 header depending on the >>> value >>> + * of l3 tunnel flag in the tun_flags field of OVS_ACTION_ATTR_ADD_MPLS >>> + * argument. >>> * @OVS_ACTION_ATTR_DROP: Explicit drop action. >>> */ >>> >>> @@ -1037,6 +1065,7 @@ enum ovs_action_attr { >>> OVS_ACTION_ATTR_METER, /* u32 meter number. */ >>> OVS_ACTION_ATTR_CLONE, /* Nested OVS_CLONE_ATTR_*. */ >>> OVS_ACTION_ATTR_CHECK_PKT_LEN, /* Nested OVS_CHECK_PKT_LEN_ATTR_*. */ >>> + OVS_ACTION_ATTR_ADD_MPLS, /* struct ovs_action_add_mpls. */ >>> >>> #ifndef __KERNEL__ >>> OVS_ACTION_ATTR_TUNNEL_PUSH, /* struct ovs_action_push_tnl*/ >>> diff --git a/include/openvswitch/ofp-ed-props.h >>> b/include/openvswitch/ofp-ed-props.h >>> index 306c6fe73..c85f3c283 100644 >>> --- a/include/openvswitch/ofp-ed-props.h >>> +++ b/include/openvswitch/ofp-ed-props.h >>> @@ -46,6 +46,11 @@ enum ofp_ed_nsh_prop_type { >>> OFPPPT_PROP_NSH_TLV = 2, /* property TLV in NSH */ >>> }; >>> >>> +enum ofp_ed_mpls_prop_type { >>> + OFPPPT_PROP_MPLS_NONE = 0, /* unused */ >> >> Can you align the above comment to the one below? >> >>> + OFPPPT_PROP_MPLS_ETHERTYPE = 1, /* MPLS Ethertype */ >>> +}; >>> + >>> /* >>> * External representation of encap/decap properties. >>> * These must be padded to a multiple of 8 bytes. >>> @@ -72,6 +77,13 @@ struct ofp_ed_prop_nsh_tlv { >>> uint8_t data[0]; >>> }; >>> >>> +struct ofp_ed_prop_mpls_ethertype { >>> + struct ofp_ed_prop_header header; >>> + uint16_t ether_type; /* MPLS ethertype .*/ >> >> Can you align the above comment to the one below? >> >>> + uint8_t pad[2]; /* Padding to 8 bytes. */ >>> +}; >>> + >>> + >>> /* >>> * Internal representation of encap/decap properties >>> */ >>> @@ -96,6 +108,12 @@ struct ofpact_ed_prop_nsh_tlv { >>> /* tlv_len octets of metadata value, padded to a multiple of 8 bytes. >>> */ >>> uint8_t data[0]; >>> }; >>> + >>> +struct ofpact_ed_prop_mpls_ethertype { >>> + struct ofpact_ed_prop header; >>> + uint16_t ether_type; /* MPLS ethertype .*/ >>> + uint8_t pad[2]; /* Padding to 8 bytes. */ >>> +}; >>> enum ofperr decode_ed_prop(const struct ofp_ed_prop_header **ofp_prop, >>> struct ofpbuf *out, size_t *remaining); >>> enum ofperr encode_ed_prop(const struct ofpact_ed_prop **prop, >>> diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c >>> index b3e57bb95..1d3287c32 100644 >>> --- a/lib/dpif-netdev.c >>> +++ b/lib/dpif-netdev.c >>> @@ -8293,6 +8293,7 @@ dp_execute_cb(void *aux_, struct dp_packet_batch >>> *packets_, >>> case OVS_ACTION_ATTR_CT_CLEAR: >>> case OVS_ACTION_ATTR_CHECK_PKT_LEN: >>> case OVS_ACTION_ATTR_DROP: >>> + case OVS_ACTION_ATTR_ADD_MPLS: >>> case __OVS_ACTION_ATTR_MAX: >>> OVS_NOT_REACHED(); >>> } >>> diff --git a/lib/dpif.c b/lib/dpif.c >>> index 8c4aed47b..99aba3cae 100644 >>> --- a/lib/dpif.c >>> +++ b/lib/dpif.c >>> @@ -1274,6 +1274,7 @@ dpif_execute_helper_cb(void *aux_, struct >>> dp_packet_batch *packets_, >>> case OVS_ACTION_ATTR_UNSPEC: >>> case OVS_ACTION_ATTR_CHECK_PKT_LEN: >>> case OVS_ACTION_ATTR_DROP: >>> + case OVS_ACTION_ATTR_ADD_MPLS: >>> case __OVS_ACTION_ATTR_MAX: >>> OVS_NOT_REACHED(); >>> } >>> diff --git a/lib/odp-execute.c b/lib/odp-execute.c >>> index 6eeda2a61..2f4cdd92c 100644 >>> --- a/lib/odp-execute.c >>> +++ b/lib/odp-execute.c >>> @@ -819,6 +819,7 @@ requires_datapath_assistance(const struct nlattr *a) >>> case OVS_ACTION_ATTR_POP_NSH: >>> case OVS_ACTION_ATTR_CT_CLEAR: >>> case OVS_ACTION_ATTR_CHECK_PKT_LEN: >>> + case OVS_ACTION_ATTR_ADD_MPLS: >>> case OVS_ACTION_ATTR_DROP: >>> return false; >>> >>> @@ -1061,6 +1062,17 @@ odp_execute_actions(void *dp, struct dp_packet_batch >>> *batch, bool steal, >>> } >>> break; >>> >>> + case OVS_ACTION_ATTR_ADD_MPLS: { >>> + const struct ovs_action_add_mpls *mpls = nl_attr_get(a); >>> + bool l3_flag = mpls->tun_flags & OVS_MPLS_L3_TUNNEL_FLAG_MASK; >>> + >>> + DP_PACKET_BATCH_FOR_EACH (i, packet, batch) { >>> + add_mpls(packet, mpls->mpls_ethertype, mpls->mpls_lse, >>> + l3_flag); >>> + } >>> + break; >>> + } >>> + >>> case OVS_ACTION_ATTR_DROP:{ >>> const enum xlate_error *drop_reason = nl_attr_get(a); >>> >>> diff --git a/lib/odp-util.c b/lib/odp-util.c >>> index 7729a9060..5f5dee35b 100644 >>> --- a/lib/odp-util.c >>> +++ b/lib/odp-util.c >>> @@ -142,6 +142,7 @@ odp_action_len(uint16_t type) >>> case OVS_ACTION_ATTR_PUSH_NSH: return ATTR_LEN_VARIABLE; >>> case OVS_ACTION_ATTR_POP_NSH: return 0; >>> case OVS_ACTION_ATTR_CHECK_PKT_LEN: return ATTR_LEN_VARIABLE; >>> + case OVS_ACTION_ATTR_ADD_MPLS: return sizeof(struct >>> ovs_action_add_mpls); >>> case OVS_ACTION_ATTR_DROP: return sizeof(uint32_t); >>> >>> case OVS_ACTION_ATTR_UNSPEC: >>> @@ -1254,6 +1255,14 @@ format_odp_action(struct ds *ds, const struct nlattr >>> *a, >>> case OVS_ACTION_ATTR_CHECK_PKT_LEN: >>> format_odp_check_pkt_len_action(ds, a, portno_names); >>> break; >>> + case OVS_ACTION_ATTR_ADD_MPLS: { >>> + const struct ovs_action_push_mpls *mpls = nl_attr_get(a); >>> + ds_put_cstr(ds, "add_mpls("); >>> + format_mpls_lse(ds, mpls->mpls_lse); >>> + ds_put_format(ds, ",eth_type=0x%"PRIx16")", >>> + ntohs(mpls->mpls_ethertype)); >> >> Can we add a test case for this new dp action in odp.at:AT_SETUP([OVS >> datapath actions parsing and formatting - valid forms])? >> >>> + break; >>> + } >>> case OVS_ACTION_ATTR_DROP: >>> ds_put_cstr(ds, "drop"); >>> break; >>> @@ -7890,7 +7899,7 @@ commit_vlan_action(const struct flow* flow, struct >>> flow *base, >>> /* Wildcarding already done at action translation time. */ >>> static void >>> commit_mpls_action(const struct flow *flow, struct flow *base, >>> - struct ofpbuf *odp_actions) >>> + struct ofpbuf *odp_actions, bool pending_encap) >>> { >>> int base_n = flow_count_mpls_labels(base, NULL); >>> int flow_n = flow_count_mpls_labels(flow, NULL); >>> @@ -7938,18 +7947,29 @@ commit_mpls_action(const struct flow *flow, struct >>> flow *base, >>> /* If, after the above popping and setting, there are more LSEs in flow >>> * than base then some LSEs need to be pushed. */ >>> while (base_n < flow_n) { >>> - struct ovs_action_push_mpls *mpls; >>> >>> - mpls = nl_msg_put_unspec_zero(odp_actions, >>> - OVS_ACTION_ATTR_PUSH_MPLS, >>> - sizeof *mpls); >>> - mpls->mpls_ethertype = flow->dl_type; >>> - mpls->mpls_lse = flow->mpls_lse[flow_n - base_n - 1]; >>> + if (pending_encap) { >>> + struct ovs_action_add_mpls *mpls; >>> + >>> + mpls = nl_msg_put_unspec_zero(odp_actions, >>> + OVS_ACTION_ATTR_ADD_MPLS, >>> + sizeof *mpls); >>> + mpls->mpls_ethertype = flow->dl_type; >>> + mpls->mpls_lse = flow->mpls_lse[flow_n - base_n - 1]; >>> + } else { >>> + struct ovs_action_push_mpls *mpls; >>> + >>> + mpls = nl_msg_put_unspec_zero(odp_actions, >>> + OVS_ACTION_ATTR_PUSH_MPLS, >>> + sizeof *mpls); >>> + mpls->mpls_ethertype = flow->dl_type; >>> + mpls->mpls_lse = flow->mpls_lse[flow_n - base_n - 1]; >>> + } >>> /* Update base flow's MPLS stack, but do not clear L3. We need >>> the L3 >>> * headers if the flow is restored later due to returning from a >>> patch >>> * port or group bucket. */ >>> - flow_push_mpls(base, base_n, mpls->mpls_ethertype, NULL, false); >>> - flow_set_mpls_lse(base, 0, mpls->mpls_lse); >>> + flow_push_mpls(base, base_n, flow->dl_type, NULL, false); >>> + flow_set_mpls_lse(base, 0, flow->mpls_lse[flow_n - base_n - 1]); >>> base_n++; >>> } >>> } >>> @@ -8600,6 +8620,10 @@ commit_encap_decap_action(const struct flow *flow, >>> memcpy(&base_flow->dl_dst, &flow->dl_dst, >>> sizeof(*flow) - offsetof(struct flow, dl_dst)); >>> break; >>> + case PT_MPLS: >>> + commit_mpls_action(flow, base_flow, odp_actions, >>> + pending_encap); >>> + break; >>> default: >>> /* Only the above protocols are supported for encap. >>> * The check is done at action translation. */ >>> @@ -8622,6 +8646,10 @@ commit_encap_decap_action(const struct flow *flow, >>> /* pop_nsh. */ >>> odp_put_pop_nsh_action(odp_actions); >>> break; >>> + case PT_MPLS: >>> + commit_mpls_action(flow, base_flow, odp_actions, >>> + pending_encap); >>> + break; >>> default: >>> /* Checks are done during translation. */ >>> OVS_NOT_REACHED(); >>> @@ -8667,7 +8695,7 @@ commit_odp_actions(const struct flow *flow, struct >>> flow *base, >>> /* Make packet a non-MPLS packet before committing L3/4 actions, >>> * which would otherwise do nothing. */ >>> if (eth_type_mpls(base->dl_type) && !eth_type_mpls(flow->dl_type)) { >>> - commit_mpls_action(flow, base, odp_actions); >>> + commit_mpls_action(flow, base, odp_actions, false); >>> mpls_done = true; >>> } >>> commit_set_nsh_action(flow, base, odp_actions, wc, use_masked); >>> @@ -8675,7 +8703,7 @@ commit_odp_actions(const struct flow *flow, struct >>> flow *base, >>> commit_set_port_action(flow, base, odp_actions, wc, use_masked); >>> slow2 = commit_set_icmp_action(flow, base, odp_actions, wc); >>> if (!mpls_done) { >>> - commit_mpls_action(flow, base, odp_actions); >>> + commit_mpls_action(flow, base, odp_actions, false); >>> } >>> commit_vlan_action(flow, base, odp_actions, wc); >>> commit_set_priority_action(flow, base, odp_actions, wc, use_masked); >>> diff --git a/lib/ofp-actions.c b/lib/ofp-actions.c >>> index ecf914eac..8b74386b1 100644 >>> --- a/lib/ofp-actions.c >>> +++ b/lib/ofp-actions.c >>> @@ -4468,6 +4468,7 @@ decode_NXAST_RAW_ENCAP(const struct nx_action_encap >>> *nae, >>> switch (ntohl(nae->new_pkt_type)) { >>> case PT_ETH: >>> case PT_NSH: >>> + case PT_MPLS: >>> /* Add supported encap header types here. */ >>> break; >>> default: >>> @@ -4519,6 +4520,8 @@ parse_encap_header(const char *hdr, ovs_be32 >>> *packet_type) >>> *packet_type = htonl(PT_ETH); >>> } else if (strcmp(hdr, "nsh") == 0) { >>> *packet_type = htonl(PT_NSH); >>> + } else if (strcmp(hdr, "mpls") == 0) { >>> + *packet_type = htonl(PT_MPLS); >>> } else { >>> return false; >>> } >>> @@ -4600,6 +4603,8 @@ format_encap_pkt_type(const ovs_be32 pkt_type) >>> return "ethernet"; >>> case PT_NSH: >>> return "nsh"; >>> + case PT_MPLS: >>> + return "mpls"; >>> default: >>> return "UNKNOWN"; >>> } >>> diff --git a/lib/ofp-ed-props.c b/lib/ofp-ed-props.c >>> index 02a9235d5..fc261e4c6 100644 >>> --- a/lib/ofp-ed-props.c >>> +++ b/lib/ofp-ed-props.c >>> @@ -79,6 +79,27 @@ decode_ed_prop(const struct ofp_ed_prop_header >>> **ofp_prop, >>> } >>> break; >>> } >>> + case OFPPPC_MPLS: { >>> + switch (prop_type) { >> >> "switch" should be move one space to the right. >> >>> + case OFPPPT_PROP_MPLS_ETHERTYPE: { >>> + struct ofp_ed_prop_mpls_ethertype *opnmt = >> >> Guess you copied this from above, but based on the naming I guess opnmt >> should be opmet (or even opme) >> >>> + ALIGNED_CAST(struct ofp_ed_prop_mpls_ethertype *, >>> *ofp_prop); >>> + if (len > sizeof(*opnmt) || len > *remaining) { >>> + return OFPERR_NXBAC_BAD_ED_PROP; >>> + } >>> + struct ofpact_ed_prop_mpls_ethertype *pnmt = >> >> Same here guess it should be '*pmet = ' >> >>> + ofpbuf_put_uninit(out, sizeof(*pnmt)); >>> + pnmt->header.prop_class = prop_class; >>> + pnmt->header.type = prop_type; >>> + pnmt->header.len = len; >>> + pnmt->ether_type = opnmt->ether_type; >>> + break; >>> + } >>> + default: >>> + return OFPERR_NXBAC_UNKNOWN_ED_PROP; >>> + } >>> + break; >>> + } >>> default: >>> return OFPERR_NXBAC_UNKNOWN_ED_PROP; >>> } >>> @@ -134,6 +155,27 @@ encode_ed_prop(const struct ofpact_ed_prop **prop, >>> } >>> break; >>> } >>> + case OFPPPC_MPLS: { >>> + switch ((*prop)->type) { >> >> Indentation of switch (and lines below) >> >>> + case OFPPPT_PROP_MPLS_ETHERTYPE: { >>> + struct ofpact_ed_prop_mpls_ethertype *pnmt = >> >> See naming above pme(t) >> >>> + ALIGNED_CAST(struct ofpact_ed_prop_mpls_ethertype *, >>> *prop); >>> + struct ofp_ed_prop_mpls_ethertype *opnmt = >>> + ofpbuf_put_uninit(out, sizeof(*opnmt)); >> >> See naming above opme(t) >> >>> + opnmt->header.prop_class = htons((*prop)->prop_class); >>> + opnmt->header.type = (*prop)->type; >>> + opnmt->header.len = >>> + offsetof(struct ofpact_ed_prop_mpls_ethertype, pad); >>> + opnmt->ether_type = pnmt->ether_type; >>> + prop_len = sizeof(*pnmt); >>> + break; >>> + >> >> Remove empty line >> >>> + } >>> + default: >>> + return OFPERR_OFPBAC_BAD_ARGUMENT; >>> + } >>> + break; >>> + } >>> default: >>> return OFPERR_OFPBAC_BAD_ARGUMENT; >>> } >>> @@ -181,6 +223,13 @@ parse_ed_prop_type(uint16_t prop_class, >>> } else { >>> return false; >>> } >>> + case OFPPPC_MPLS: >>> + if (!strcmp(str, "ether_type")) { >>> + *type = OFPPPT_PROP_MPLS_ETHERTYPE; >>> + return true; >>> + } else { >>> + return false; >>> + } >>> default: >>> return false; >>> } >>> @@ -259,6 +308,28 @@ parse_ed_prop_value(uint16_t prop_class, uint8_t >>> prop_type OVS_UNUSED, >>> OVS_NOT_REACHED(); >>> } >>> break; >>> + case OFPPPC_MPLS: >>> + switch (prop_type) { >>> + case OFPPPT_PROP_MPLS_ETHERTYPE: { >>> + uint16_t ethertype; >>> + error = str_to_u16(value, "ether_type", ðertype); >>> + if (error != NULL) { >>> + return error; >>> + } >>> + struct ofpact_ed_prop_mpls_ethertype *pnmt = >>> + ofpbuf_put_uninit(out, sizeof(*pnmt)); >> >> See naming above pme(t) >> >>> + pnmt->header.prop_class = prop_class; >>> + pnmt->header.type = prop_type; >>> + pnmt->header.len = >>> + offsetof(struct ofpact_ed_prop_mpls_ethertype, pad); >>> + pnmt->ether_type = ethertype; >>> + >>> + break; >>> + } >>> + default: >> >> Default should be same as other types: >> >> /* Unsupported property types rejected before. */ >> OVS_NOT_REACHED(); >> >>> + break; >>> + } >>> + break; >>> default: >>> /* Unsupported property classes rejected before. */ >>> OVS_NOT_REACHED(); >>> @@ -300,6 +371,14 @@ format_ed_prop_type(const struct ofpact_ed_prop *prop) >>> OVS_NOT_REACHED(); >>> } >>> break; >>> + case OFPPPC_MPLS: >> >> Indentation of the below is off by one space. >> >>> + switch (prop->type) { >>> + case OFPPPT_PROP_MPLS_ETHERTYPE: >>> + return "ether_type"; >>> + default: >>> + OVS_NOT_REACHED(); >>> + } >>> + break; >>> default: >>> OVS_NOT_REACHED(); >>> } >>> @@ -332,6 +411,18 @@ format_ed_prop(struct ds *s OVS_UNUSED, >>> default: >>> OVS_NOT_REACHED(); >>> } >>> + case OFPPPC_MPLS: >> >> Check alignment. >> >>> + switch (prop->type) { >>> + case OFPPPT_PROP_MPLS_ETHERTYPE: { >>> + struct ofpact_ed_prop_mpls_ethertype *pnmt = >>> + ALIGNED_CAST(struct ofpact_ed_prop_mpls_ethertype *, prop); >>> + ds_put_format(s, "%s=%d", format_ed_prop_type(prop), >>> + pnmt->ether_type); >>> + return; >>> + } >>> + default: >>> + OVS_NOT_REACHED(); >>> + } >>> default: >>> OVS_NOT_REACHED(); >>> } >>> diff --git a/lib/ovs-actions.xml b/lib/ovs-actions.xml >>> index c94b5f3b3..a0cdab85c 100644 >>> --- a/lib/ovs-actions.xml >>> +++ b/lib/ovs-actions.xml >> >> The xml based documentation has been converted to >> Documentation/ref/ovs-actions.7.rst, so you need to update it there. >> >>> @@ -265,13 +265,13 @@ >>> </p> >>> >>> <p> >>> - When a <code>decap</code> action decapsulates a packet, Open >>> vSwitch >>> - raises this error if it does not support the type of inner packet. >>> - <code>decap</code> of an Ethernet header raises this error if a >>> VLAN >>> - header is present, <code>decap</code> of a NSH packet raises this >>> error >>> - if the NSH inner packet is not Ethernet, IPv4, IPv6, or NSH, and >>> - <code>decap</code> of other types of packets is unsupported and >>> also >>> - raises this error. >>> + The <code>decap</code> action is supported only for packet types >>> + ethernet, NSH and MPLS. Openvswitch raises this error for other >>> + packet types. When a <code>decap</code> action decapsulates a >>> packet, >>> + Open vSwitch raises this error if it does not support the type of >>> inner >>> + packet. <code>decap</code> of an Ethernet header raises this error >>> if a >>> + VLAN header is present, <code>decap</code> of a NSH packet raises >>> this >>> + error if the NSH inner packet is not Ethernet, IPv4, IPv6, or NSH. >>> </p> >>> >>> <p> >>> @@ -1396,6 +1396,8 @@ for <var>i</var> in [1,<var>n_members</var>]: >>> <h2>The <code>encap</code> action</h2> >>> >>> <syntax><code>encap(nsh(</code>[<code>md_type=<var>md_type</var></code>]<code>, >>> >>> </code>[<code>tlv(<var>class</var>,<var>type</var>,<var>value</var>)</code>]...<code>))</code></syntax> >>> <syntax><code>encap(ethernet)</code></syntax> >>> + <syntax><code>encap(mpls(ether_type=<var>ether_type</var>))</code> >>> + </syntax> >>> >>> <p> >>> The <code>encap</code> action encapsulates a packet with a >>> specified >>> @@ -1434,6 +1436,12 @@ for <var>i</var> in [1,<var>n_members</var>]: >>> source and destination are initially zeroed. >>> </p> >>> >>> + <p> >>> + The <code>encap(mpls(ethertype=....))</code> variant encapsulates >>> an >>> + ethernet or L3 packet with a MPLS header. The <var>ethertype</var> >>> + could be MPLS unicast (0x8847) or multicast (0x8848) ethertypes. >>> + </p> >>> + >>> <conformance> >>> This action is an Open vSwitch extension to OpenFlow 1.3 and later, >>> introduced in Open vSwitch 2.8. >>> @@ -1443,6 +1451,9 @@ for <var>i</var> in [1,<var>n_members</var>]: >>> <action name="DECAP"> >>> <h2>The <code>decap</code> action</h2> >>> <syntax><code>decap</code></syntax> >>> + <syntax><code>decap(packet_type(ns=<var>name_space</var>, >>> + type=<var>ethertype</var>))</code></syntax> for decapsulating MPLS >>> + packets. >>> >>> <p> >>> Removes an outermost encapsulation from the packet: >>> @@ -1463,6 +1474,13 @@ for <var>i</var> in [1,<var>n_members</var>]: >>> packet type errors. >>> </li> >>> >>> + <li> >>> + Otherwise, if the packet is a MPLS packet, removes the MPLS >>> header >>> + and classifies the inner packet as mentioned in the packet type >>> + argument of the decap. The packet_type field specifies the type >>> of >>> + the packet in the format specified in OpenFlow 1.5. >> >> Can we explain more what the ns and ethertype values mean? Or maybe be more >> specific and add reference to OpenFlow 1.5 chapter "7.2.3.11 Packet Type >> Match Field" >> >>> + </li> >>> + >>> <li> >>> Otherwise, raises an unsupported packet type error. >>> </li> >>> diff --git a/lib/packets.c b/lib/packets.c >>> index 4a7643c5d..66fefdaba 100644 >>> --- a/lib/packets.c >>> +++ b/lib/packets.c >>> @@ -418,6 +418,38 @@ push_mpls(struct dp_packet *packet, ovs_be16 ethtype, >>> ovs_be32 lse) >>> pkt_metadata_init_conn(&packet->md); >>> } >>> >>> +void >>> +add_mpls(struct dp_packet *packet, ovs_be16 ethtype, ovs_be32 lse, bool l3) >> >> Maybe rename l3 to l3_encap to be more explicit? >> >>> +{ >>> + char * header; >> >> Remove space after * >> >>> + >>> + if (!eth_type_mpls(ethtype)) { >>> + return; >>> + } >>> + >>> + if (!l3) { >>> + header = dp_packet_push_uninit(packet, MPLS_HLEN); >> >> Remove extra space after = >> Also, does it maybe make more sense to do a direct assignment here instead >> of memcpy, or do we rely on the compiler to do this for us (also below)? >> >> ovs_be32 *header = dp_packet_push_uninit(packet, MPLS_HLEN); >> *header = lse; >> >>> + memcpy(header, &lse, sizeof lse); >>> + packet->l2_5_ofs = 0; >>> + packet->packet_type = htonl(PT_MPLS); >>> + } else { >>> + size_t len; >>> + >>> + if (!is_mpls(packet)) { >>> + /* Set MPLS label stack offset. */ >>> + packet->l2_5_ofs = packet->l3_ofs; >>> + } >>> + set_ethertype(packet, ethtype); >>> + >>> + /* Push new MPLS shim header onto packet. */ >>> + len = packet->l2_5_ofs; >>> + header = dp_packet_resize_l2_5(packet, MPLS_HLEN); >>> + memmove(header, header + MPLS_HLEN, len); >>> + memcpy(header + len, &lse, sizeof lse); >>> + } >>> + pkt_metadata_init_conn(&packet->md); >>> +} >>> + >>> /* If 'packet' is an MPLS packet, removes its outermost MPLS label stack >>> entry. >>> * If the label that was removed was the only MPLS label, changes >>> 'packet''s >>> * Ethertype to 'ethtype' (which ordinarily should not be an MPLS >>> @@ -426,10 +458,14 @@ void >>> pop_mpls(struct dp_packet *packet, ovs_be16 ethtype) >>> { >>> if (is_mpls(packet)) { >>> + >> >> Remove introduced new line >> >>> struct mpls_hdr *mh = dp_packet_l2_5(packet); >>> size_t len = packet->l2_5_ofs; >>> >>> - set_ethertype(packet, ethtype); >>> + if (ethtype != htons(ETH_TYPE_TEB)) { >> >> It looks like we are using ETH_TYPE_TEB as some magic marker, what happens >> if a packet really has ETH_TYPE_TEB? >> Can you explain the rational behind this? >> > The above check is redundant. I will remove thie check in the next version. > > when ETH_TYE_TEB is configured as the ethertype in pop_mpls, it means > the inner packet is ethernet. Not sure if a packet can have ETH_TYPE_TEB > as the ethertype to support ethernet in ethernet. As I know ETH_TYPE_TEB > is used with NSH encap. > This piece of code get hit only for MPLS packets so it doesnot matter if > packet with ETH_TYPE_TEB as ethertype really comes. >>> + set_ethertype(packet, ethtype); >> >> Indentation is one to deep. >> >>> + } >>> + >>> if (get_16aligned_be32(&mh->mpls_lse) & htonl(MPLS_BOS_MASK)) { >>> dp_packet_set_l2_5(packet, NULL); >>> } >>> @@ -440,6 +476,15 @@ pop_mpls(struct dp_packet *packet, ovs_be16 ethtype) >>> /* Invalidate offload flags as they are not valid after >>> * decapsulation of MPLS header. */ >>> dp_packet_reset_offload(packet); >> >> Can we group the above and below ethtype checks/mods together and add some >> comments, so it's clear why there is a difference, i.e. L2 vs L3? >> > I will add a comment here. >>> + >>> + if (!len) { >>> + if (ethtype == htons(ETH_TYPE_TEB)) { >>> + packet->packet_type = htonl(PT_ETH); >>> + } else { >>> + packet->packet_type = PACKET_TYPE_BE(OFPHTN_ETHERTYPE, >>> + ntohs(ethtype)); >>> + } >>> + } >>> } >>> } >>> >>> diff --git a/lib/packets.h b/lib/packets.h >>> index 515bb59b1..8f72af328 100644 >>> --- a/lib/packets.h >>> +++ b/lib/packets.h >>> @@ -356,6 +356,8 @@ void set_mpls_lse_label(ovs_be32 *lse, ovs_be32 label); >>> void set_mpls_lse_bos(ovs_be32 *lse, uint8_t bos); >>> ovs_be32 set_mpls_lse_values(uint8_t ttl, uint8_t tc, uint8_t bos, >>> ovs_be32 label); >>> +void add_mpls(struct dp_packet *packet, ovs_be16 ethtype, ovs_be32 lse, >>> + bool l3_flag); >> >> l3_flag, to l3_encap (see above). >> >>> >>> /* Example: >>> * >>> diff --git a/ofproto/ofproto-dpif-ipfix.c b/ofproto/ofproto-dpif-ipfix.c >>> index 796eb6f88..9280e008e 100644 >>> --- a/ofproto/ofproto-dpif-ipfix.c >>> +++ b/ofproto/ofproto-dpif-ipfix.c >>> @@ -3018,6 +3018,7 @@ dpif_ipfix_read_actions(const struct flow *flow, >>> case OVS_ACTION_ATTR_CHECK_PKT_LEN: >>> case OVS_ACTION_ATTR_UNSPEC: >>> case OVS_ACTION_ATTR_DROP: >>> + case OVS_ACTION_ATTR_ADD_MPLS: >>> case __OVS_ACTION_ATTR_MAX: >>> default: >>> break; >>> diff --git a/ofproto/ofproto-dpif-sflow.c b/ofproto/ofproto-dpif-sflow.c >>> index 864c136b5..30e7caf54 100644 >>> --- a/ofproto/ofproto-dpif-sflow.c >>> +++ b/ofproto/ofproto-dpif-sflow.c >>> @@ -1226,6 +1226,7 @@ dpif_sflow_read_actions(const struct flow *flow, >>> case OVS_ACTION_ATTR_UNSPEC: >>> case OVS_ACTION_ATTR_CHECK_PKT_LEN: >>> case OVS_ACTION_ATTR_DROP: >>> + case OVS_ACTION_ATTR_ADD_MPLS: >>> case __OVS_ACTION_ATTR_MAX: >>> default: >>> break; >>> diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c >>> index 8723cb4e8..831c71275 100644 >>> --- a/ofproto/ofproto-dpif-xlate.c >>> +++ b/ofproto/ofproto-dpif-xlate.c >>> @@ -6403,6 +6403,50 @@ rewrite_flow_encap_ethernet(struct xlate_ctx *ctx, >>> ctx->error = XLATE_UNSUPPORTED_PACKET_TYPE; >>> } >>> } >>> +static void >>> +rewrite_flow_encap_mpls(struct xlate_ctx *ctx, >>> + const struct ofpact_encap *encap, >>> + struct flow *flow, >>> + struct flow_wildcards *wc) >>> +{ >>> + int n; >>> + uint32_t i; >>> + uint16_t ether_type; >>> + const char *ptr = (char *) encap->props; >>> + >>> + for (i = 0; i < encap->n_props; i++) { >> >> "for" is not aligned correctly, also look at alignment below. >> >>> + struct ofpact_ed_prop *prop_ptr = >>> + ALIGNED_CAST(struct ofpact_ed_prop *, ptr); >>> + if (prop_ptr->prop_class == OFPPPC_MPLS) { >>> + switch (prop_ptr->type) { >>> + case OFPPPT_PROP_MPLS_ETHERTYPE: { >>> + struct ofpact_ed_prop_mpls_ethertype *prop_ether_type >>> = >>> + ALIGNED_CAST(struct ofpact_ed_prop_mpls_ethertype >>> *, >>> + prop_ptr); >>> + ether_type = prop_ether_type->ether_type; >>> + break; >>> + } >>> + } >>> + } >>> + } >>> + >>> + wc->masks.packet_type = OVS_BE32_MAX; >>> + if (flow->packet_type != htonl(PT_MPLS)) { >>> + memset(&ctx->wc->masks.mpls_lse, 0x0, >>> + sizeof *wc->masks.mpls_lse * FLOW_MAX_MPLS_LABELS); >>> + memset(&flow->mpls_lse, 0x0, sizeof *flow->mpls_lse * >>> + FLOW_MAX_MPLS_LABELS); >>> + memset(&ctx->base_flow.mpls_lse, 0x0, sizeof >>> *ctx->base_flow.mpls_lse * >>> + FLOW_MAX_MPLS_LABELS); >> >> Do we really need to memset these? Asking as normally these get cleared at >> start? >> If so it might be worth adding a comment why they need to be cleared. >> > This is to handle case where the incoming packet is already a MPLS > packet. I will add a comment. >>> + } >>> + flow->packet_type = htonl(PT_MPLS); >> >> This can be moved to the if branch above. >> >>> + n = flow_count_mpls_labels(flow, ctx->wc); >>> + flow_push_mpls(flow, n, htons(ether_type), ctx->wc, true); >> >> Personally I would get ride of the n variable, but thats just a nit. >> >> + flow_push_mpls(flow, flow_count_mpls_labels(flow, ctx->wc), >> + htons(ether_type), ctx->wc, true); >> >>> + flow->dl_src = eth_addr_zero; >>> + flow->dl_dst = eth_addr_zero; >>> + >> >> Remove extra new line >> >>> +} >>> + >>> >>> /* For an MD2 NSH header returns a pointer to an ofpbuf with the encoded >>> * MD2 TLVs provided as encap properties to the encap operation. This >>> @@ -6535,6 +6579,12 @@ xlate_generic_encap_action(struct xlate_ctx *ctx, >>> case PT_NSH: >>> encap_data = rewrite_flow_push_nsh(ctx, encap, flow, wc); >>> break; >>> + case PT_MPLS: >>> + rewrite_flow_encap_mpls(ctx, encap, flow, wc); >>> + if (!ctx->xbridge->support.add_mpls) { >>> + ctx->xout->slow |= SLOW_ACTION; >>> + } >>> + break; >>> default: >>> /* New packet type was checked during decoding. */ >>> OVS_NOT_REACHED(); >>> @@ -6606,6 +6656,30 @@ xlate_generic_decap_action(struct xlate_ctx *ctx, >>> ctx->pending_decap = true; >>> /* Trigger recirculation. */ >>> return true; >>> + case PT_MPLS: { >>> + int n; >>> + ovs_be16 ethertype; >>> + >>> + flow->packet_type = decap->new_pkt_type; >>> + ethertype = pt_ns_type_be(flow->packet_type); >>> + >>> + n = flow_count_mpls_labels(flow, ctx->wc); >>> + if (!ethertype) { >>> + ethertype = htons(ETH_TYPE_TEB); >>> + } >>> + flow_pop_mpls(flow, n, ethertype, ctx->wc); >>> + >>> + if (!ctx->xbridge->support.add_mpls) { >>> + ctx->xout->slow |= SLOW_ACTION; >>> + } >>> + ctx->pending_decap = true; >>> + if (n == 1) { >>> + /* Trigger recirculation. */ >>> + return true; >>> + } else { >>> + return false; >>> + } >>> + } >>> default: >>> /* Error handling: drop packet. */ >>> xlate_report_debug( >>> diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c >>> index cba49a99e..7e48eb12d 100644 >>> --- a/ofproto/ofproto-dpif.c >>> +++ b/ofproto/ofproto-dpif.c >>> @@ -1538,6 +1538,44 @@ check_nd_extensions(struct dpif_backer *backer) >>> >>> return !error; >>> } >>> +/* Tests whether 'backer''s datapath supports the >>> + * OVS_ACTION_ATTR_ADD_MPLS action. */ >>> +static bool >>> +check_add_mpls(struct dpif_backer *backer) >>> +{ >>> + struct odputil_keybuf keybuf; >>> + struct ofpbuf actions; >>> + struct ofpbuf key; >>> + struct flow flow; >>> + bool supported; >>> + >>> + struct odp_flow_key_parms odp_parms = { >>> + .flow = &flow, >>> + .probe = true, >>> + }; >>> + >>> + memset(&flow, 0, sizeof flow); >>> + ofpbuf_use_stack(&key, &keybuf, sizeof keybuf); >>> + odp_flow_key_from_flow(&odp_parms, &key); >>> + ofpbuf_init(&actions, 64); >>> + >>> + struct ovs_action_add_mpls *mpls; >>> + >>> + mpls = nl_msg_put_unspec_zero(&actions, >>> + OVS_ACTION_ATTR_ADD_MPLS, >>> + sizeof *mpls); >>> + mpls->mpls_ethertype = htons(ETH_TYPE_MPLS); >>> + >>> + supported = dpif_probe_feature(backer->dpif, "add_mpls", &key, >>> + &actions, NULL); >>> + ofpbuf_uninit(&actions); >>> + VLOG_INFO("%s: Datapath %s add_mpls action", >>> + dpif_name(backer->dpif), supported ? "supports" >>> + : "does not support"); >> >> I would also move this as follows: >> >> + VLOG_INFO("%s: Datapath %s add_mpls action", >> + dpif_name(backer->dpif), >> + supported ? "supports" : "does not support"); >> >> >>> + return supported; >>> + >> Remove extra empty line >>> +} >> >> Can we also make sure the support table is updated, i.e update the >> get_datapath_cap() function. >> >>> + >> >> Remove extra empty line >> >>> >>> #define CHECK_FEATURE__(NAME, SUPPORT, FIELD, VALUE, ETHTYPE) >>> \ >>> static bool >>> \ >>> @@ -1609,6 +1647,7 @@ check_support(struct dpif_backer *backer) >>> backer->rt_support.lb_output_action = >>> dpif_supports_lb_output_action(backer->dpif); >>> backer->rt_support.ct_zero_snat = dpif_supports_ct_zero_snat(backer); >>> + backer->rt_support.add_mpls = check_add_mpls(backer); >>> >>> /* Flow fields. */ >>> backer->rt_support.odp.ct_state = check_ct_state(backer); >>> diff --git a/ofproto/ofproto-dpif.h b/ofproto/ofproto-dpif.h >>> index 191cfcb0d..6ce534ad6 100644 >>> --- a/ofproto/ofproto-dpif.h >>> +++ b/ofproto/ofproto-dpif.h >>> @@ -207,7 +207,9 @@ struct group_dpif *group_dpif_lookup(struct >>> ofproto_dpif *, >>> DPIF_SUPPORT_FIELD(bool, lb_output_action, "Optimized Balance TCP >>> mode")\ >>> >>> \ >>> /* True if the datapath supports all-zero IP SNAT. */ >>> \ >>> - DPIF_SUPPORT_FIELD(bool, ct_zero_snat, "Conntrack all-zero IP SNAT") >>> + DPIF_SUPPORT_FIELD(bool, ct_zero_snat, "Conntrack all-zero IP SNAT") >>> \ >>> + /* True if the datapath supports layer 2 MPLS tunnelling */ >>> \ >> >> Guess the description is not correct, as this is what the kernel header >> tells us: >> >> * @OVS_ACTION_ATTR_ADD_MPLS: Push a new MPLS label stack entry at the >> * start of the packet or at the start of the l3 header depending on the >> value >> * of l3 tunnel flag in the tun_flags field of OVS_ACTION_ATTR_ADD_MPLS >> * argument. >> >>> + DPIF_SUPPORT_FIELD(bool, add_mpls, "l2 MPLS tunnelling") >> >> I would simply change this to: "MPLS label add") >> >>> >>> >>> /* Stores the various features which the corresponding backer supports. */ >>> diff --git a/tests/mpls-xlate.at b/tests/mpls-xlate.at >>> index aafa89ba6..f8ba1fe87 100644 >>> --- a/tests/mpls-xlate.at >>> +++ b/tests/mpls-xlate.at >>> @@ -207,3 +207,50 @@ AT_CHECK([tail -1 stdout], [0], >>> >>> OVS_VSWITCHD_STOP >>> AT_CLEANUP >>> + >>> +AT_SETUP([Encap Decap MPLS xlate action]) >>> + >>> +OVS_VSWITCHD_START( >>> + [add-port br0 p1 -- set Interface p1 type=dummy ofport_request=1 -- \ >>> + add-port br0 p2 -- set Interface p2 type=patch \ >>> + options:peer=p3 ofport_request=2 -- >>> \ >>> + add-br br1 -- \ >>> + set bridge br1 other-config:hwaddr=aa:66:aa:66:00:00 -- \ >>> + set bridge br1 datapath-type=dummy other-config:datapath-id=1234 \ >>> + fail-mode=secure -- \ >>> + add-port br1 p3 -- set Interface p3 type=patch \ >>> + options:peer=p2 ofport_request=3 -- >>> \ >>> + add-port br1 p4 -- set Interface p4 type=dummy ofport_request=4]) >>> + >>> +AT_CHECK([ovs-appctl dpif/show], [0], [dnl >>> +dummy@ovs-dummy: hit:0 missed:0 >>> + br0: >>> + br0 65534/100: (dummy-internal) >>> + p1 1/1: (dummy) >>> + p2 2/none: (patch: peer=p3) >>> + br1: >>> + br1 65534/101: (dummy-internal) >>> + p3 3/none: (patch: peer=p2) >>> + p4 4/4: (dummy) >>> +]) >>> + >>> +AT_CHECK([ovs-ofctl del-flows br0]) >>> +AT_CHECK([ovs-ofctl -O OpenFlow13 add-flow br0 >>> "in_port=p1,actions=encap(mpls(ether_type=0x8847)),encap(ethernet),set_field:00:00:00:00:00:02->dl_dst,set_field:00:00:00:00:00:01->dl_src,output:p2"]) >>> +AT_CHECK([ovs-ofctl -O OpenFlow13 add-flow br1 "in_port=p3,dl_type=0x8847 >>> actions=decap(),decap(packet_type(ns=0,type=0)),output:p4"]) >>> + >>> +# Now send two real ICMP echo request packets in on port p1 >>> + >>> +AT_CHECK([ovs-appctl netdev-dummy/receive p1 >>> 1e2ce92a669e3a6dd2099cab0800450000548a53400040011addc0a80a0ac0a80a1e08006f200a4d0001fc509a58000000002715020000000000101112131415161718191a1b1c1d1e1f202122232425262728292a2b2c2d2e2f3031323334353637] >>> ,[0], [ignore]) >>> + >>> +AT_CHECK([ovs-appctl netdev-dummy/receive p1 >>> 1e2ce92a669e3a6dd2099cab0800450000548a53400040011addc0a80a0ac0a80a1e08006f200a4d0001fc509a58000000002715020000000000101112131415161718191a1b1c1d1e1f202122232425262728292a2b2c2d2e2f3031323334353637] >>> ,[0], [ignore]) >>> + >>> +ovs-appctl time/warp 1000 >>> + >>> +AT_CHECK([ovs-appctl dpctl/dump-flows dummy@ovs-dummy | strip_used | grep >>> -v ipv6 |sort], [0], >>> +[flow-dump from the main thread: >>> +recirc_id(0),in_port(1),packet_type(ns=0,id=0),eth(src=3a:6d:d2:09:9c:ab,dst=1e:2c:e9:2a:66:9e),eth_type(0x0800),ipv4(frag=no), >>> packets:1, bytes:98, used:0.0s, >>> actions:add_mpls(label=0,tc=0,ttl=64,bos=1,eth_type=0x8847),push_eth(src=00:00:00:00:00:01,dst=00:00:00:00:00:02),pop_eth,pop_mpls(eth_type=0x6558),recirc(0x1) >>> +recirc_id(0x1),in_port(1),packet_type(ns=0,id=0),eth_type(0x0800),ipv4(frag=no), >>> packets:1, bytes:98, used:0.0s, actions:4 >>> +]) >>> + >> >> Can we do the same test with DP support disabled, to see it goes slow path, >> i.e., "ovs-appctl dpif/set-dp-features" >> >>> +OVS_VSWITCHD_STOP >>> +AT_CLEANUP >>> diff --git a/tests/system-traffic.at b/tests/system-traffic.at >>> index f400cfabc..b1f88b932 100644 >>> --- a/tests/system-traffic.at >>> +++ b/tests/system-traffic.at >>> @@ -1081,9 +1081,236 @@ NS_CHECK_EXEC([at_ns0], [ping -q -c 3 -i 0.3 -w 2 >>> 10.1.1.2 | FORMAT_PING], [0], >>> 3 packets transmitted, 3 received, 0% packet loss, time 0ms >>> ]) >>> >>> +OVS_TRAFFIC_VSWITCHD_STOP >>> +AT_CLEANUP >>> + >>> +AT_SETUP([mpls - encap header]) >>> +OVS_TRAFFIC_VSWITCHD_START() >>> + >>> +ADD_NAMESPACES(at_ns0, at_ns1) >>> + >>> +ADD_VETH(p0, at_ns0, br0, "10.1.1.1/24", 36:b1:ee:7c:01:03) >>> +ADD_VETH(p1, at_ns1, br0, "10.1.1.2/24", 36:b1:ee:7c:01:02) >>> + >>> +dnl The flow will encap a mpls header to the ip packet >>> +dnl eth/ip/icmp --> OVS --> eth/mpls/eth/ip/icmp >>> +AT_CHECK([ovs-ofctl -Oopenflow13 add-flow br0 >>> "table=0,priority=100,dl_type=0x0800 >>> actions=encap(mpls(ether_type=0x8847)),set_mpls_label:2,encap(ethernet),set_field:00:00:00:00:00:02->dl_dst,set_field:00:00:00:00:00:01->dl_src,ovs-p1"]) >>> + >>> +rm -rf p1.pcap >>> +NS_CHECK_EXEC([at_ns1], [tcpdump -l -n -xx -U -i p1 > p1.pcap &]) >>> +sleep 1 >>> + >>> +dnl The hex dump is a icmp packet. pkt=eth/ip/icmp >>> +dnl The packet is sent from p0(at_ns0) interface directed to >>> +dnl p1(at_ns1) interface >>> +NS_CHECK_EXEC([at_ns0], [$PYTHON3 $srcdir/sendpkt.py p0 36 b1 ee 7c 01 02 >>> 36 b1 ee 7c 01 03 08 00 45 00 00 54 03 44 40 00 40 01 21 61 0a 01 01 01 0a >>> 01 01 02 08 00 ef ac 7c e4 00 03 5b 2c 1f 61 00 00 00 00 50 0b 02 00 00 00 >>> 00 00 10 11 12 13 14 15 16 17 18 19 1a 1b 1c 1d 1e 1f 20 21 22 23 24 25 26 >>> 27 28 29 2a 2b 2c 2d 2e 2f 30 31 32 33 34 35 36 37 > /dev/null]) >>> + >>> +dnl Check the expected nsh encapsulated packet on the egress interface >>> +OVS_WAIT_UNTIL([cat p1.pcap | egrep "0x0000: *0000 *0000 *0002 *0000 >>> *0000 *0001 *8847 *0000" 2>&1 1>/dev/null]) >>> +OVS_WAIT_UNTIL([cat p1.pcap | egrep "0x0010: *2140 *36b1 *ee7c *0102 >>> *36b1 *ee7c *0103 *0800" 2>&1 1>/dev/null]) >>> +OVS_WAIT_UNTIL([cat p1.pcap | egrep "0x0020: *4500 *0054 *0344 *4000 >>> *4001 *2161 *0a01 *0101" 2>&1 1>/dev/null]) >>> +OVS_WAIT_UNTIL([cat p1.pcap | egrep "0x0030: *0a01 *0102 *0800 *efac >>> *7ce4 *0003 *5b2c *1f61" 2>&1 1>/dev/null]) >>> + >>> +OVS_TRAFFIC_VSWITCHD_STOP >>> +AT_CLEANUP >>> + >> >> For the "mpls - encap header" above, and the decap header below we should >> have two tests. >> The one as is, but call it something like "mpls - encap header [DP support]" >> and skip it if the DP does not support it. > > In this case we can skip the test only once the test is started. ie once the > bridge is created to fetch the DP features. > I have not see this approach being used in any other tests and it > appears unclean. Hence i prefer to let the test run in slow path if the > datapath does not have the support > >> And one called "mpls - encap header [slowpath]" and always run it with >> "ovs-appctl dpif/set-dp-features disabled" >> >>> +AT_SETUP([mpls - decap header]) >>> +OVS_TRAFFIC_VSWITCHD_START() >>> + >>> +ADD_NAMESPACES(at_ns0, at_ns1) >>> + >>> +ADD_VETH(p0, at_ns0, br0, "10.1.1.1/24", 36:b1:ee:7c:01:03) >>> +ADD_VETH(p1, at_ns1, br0, "10.1.1.2/24", 36:b1:ee:7c:01:02) >>> + >>> +dnl The flow will decap a mpls header which in turn carries a icmp packet >>> +dnl eth/mpls/eth/ip/icmp --> OVS --> eth/ip/icmp >>> + >>> +AT_CHECK([ovs-ofctl -Oopenflow13 add-flow br0 >>> "table=0,priority=100,dl_type=0x8847,mpls_label=2 >>> actions=decap(),decap(packet_type(ns=0,type=0)),ovs-p1"]) >>> + >>> +rm -rf p1.pcap >>> +NS_CHECK_EXEC([at_ns1], [tcpdump -l -n -xx -U -i p1 > p1.pcap &]) >>> +sleep 1 >>> + >>> +dnl The hex dump is an mpls packet encapsulating ethernet packet. >>> pkt=eth/mpls/eth/ip/icmp >>> +dnl The packet is sent from p0(at_ns0) interface directed to >>> +dnl p1(at_ns1) interface >>> +NS_CHECK_EXEC([at_ns0], [$PYTHON3 $srcdir/sendpkt.py p0 00 00 00 00 00 02 >>> 00 00 00 00 00 01 88 47 00 00 21 40 36 b1 ee 7c 01 02 36 b1 ee 7c 01 03 08 >>> 00 45 00 00 54 03 44 40 00 40 01 21 61 0a 01 01 01 0a 01 01 02 08 00 ef ac >>> 7c e4 00 03 5b 2c 1f 61 00 00 00 00 50 0b 02 00 00 00 00 00 10 11 12 13 14 >>> 15 16 17 18 19 1a 1b 1c 1d 1e 1f 20 21 22 23 24 25 26 27 28 29 2a 2b 2c 2d >>> 2e 2f 30 31 32 33 34 35 36 37 > /dev/null]) >>> + >>> +dnl Check the expected nsh encapsulated packet on the egress interface >>> +OVS_WAIT_UNTIL([cat p1.pcap | egrep "0x0000: *36b1 *ee7c *0102 *36b1 >>> *ee7c *0103 *0800 *4500" 2>&1 1>/dev/null]) >>> +OVS_WAIT_UNTIL([cat p1.pcap | egrep "0x0010: *0054 *0344 *4000 *4001 >>> *2161 *0a01 *0101 *0a01" 2>&1 1>/dev/null]) >>> +OVS_WAIT_UNTIL([cat p1.pcap | egrep "0x0020: *0102 *0800 *efac *7ce4 >>> *0003 *5b2c *1f61 *0000" 2>&1 1>/dev/null]) >>> +OVS_WAIT_UNTIL([cat p1.pcap | egrep "0x0030: *0000 *500b *0200 *0000 >>> *0000 *1011 *1213 *1415" 2>&1 1>/dev/null]) >>> +OVS_WAIT_UNTIL([cat p1.pcap | egrep "0x0040: *1617 *1819 *1a1b *1c1d >>> *1e1f *2021 *2223 *2425" 2>&1 1>/dev/null]) >>> +OVS_WAIT_UNTIL([cat p1.pcap | egrep "0x0050: *2627 *2829 *2a2b *2c2d >>> *2e2f *3031 *3233 *3435" 2>&1 1>/dev/null]) >>> +OVS_WAIT_UNTIL([cat p1.pcap | egrep "0x0060: *3637" 2>&1 1>/dev/null]) >>> + >>> + >>> +OVS_TRAFFIC_VSWITCHD_STOP >>> +AT_CLEANUP >>> + >>> + >>> +AT_SETUP([datapath - Encap Decap mpls actions]) >>> +OVS_TRAFFIC_VSWITCHD_START([_ADD_BR([br1])]) >>> + >>> +ADD_NAMESPACES(at_ns0, at_ns1) >>> + >>> +ADD_VETH(p0, at_ns0, br0, "10.1.1.1/24") >>> +ADD_VETH(p1, at_ns1, br1, "10.1.1.2/24") >>> + >>> +AT_CHECK([ip link add patch0 type veth peer name patch1]) >>> +on_exit 'ip link del patch0' >>> + >>> +AT_CHECK([ip link set dev patch0 up]) >>> +AT_CHECK([ip link set dev patch1 up]) >>> +AT_CHECK([ovs-vsctl add-port br0 patch0 -- set interface patch0 >>> ofport_request=100]) >>> +AT_CHECK([ovs-vsctl add-port br1 patch1 -- set interface patch1 >>> ofport_request=100]) >>> + >>> +AT_DATA([flows.txt], [dnl >>> +table=0,priority=100,dl_type=0x0800 >>> actions=encap(mpls(ether_type=0x8847)),set_mpls_label:2,encap(ethernet),set_field:00:00:00:00:00:02->dl_dst,set_field:00:00:00:00:00:01->dl_src,output:100 >>> +table=0,priority=100,dl_type=0x8847,mpls_label=2 >>> actions=decap(),decap(packet_type(ns=0,type=0)),resubmit(,3) >>> +table=0,priority=10 actions=resubmit(,3) >> >> Can we change this as follows, so all traffic is going over the mpls tunnel? >> >> +table=0,priority=100,dl_type=0x0806 >> actions=encap(mpls(ether_type=0x8847)),set_mpls_label:2,encap(ethernet),set_field:00:00:00:00:00:02->dl_dst,set_field:00:00:00:00:00:01->dl_src,output:100 >> table=0,priority=100,dl_type=0x8847,mpls_label=2 >> actions=decap(),decap(packet_type(ns=0,type=0)),resubmit(,3) >> -table=0,priority=10 actions=resubmit(,3) >> >> >>> +table=3,priority=10 actions=normal >>> +]) >>> + >>> +AT_CHECK([ovs-ofctl -Oopenflow13 add-flows br0 flows.txt]) >>> +AT_CHECK([ovs-ofctl -Oopenflow13 add-flows br1 flows.txt]) >>> + >>> +NS_CHECK_EXEC([at_ns0], [ping -q -c 3 -i 0.3 -w 2 10.1.1.2 | FORMAT_PING], >>> [0], [dnl >>> +3 packets transmitted, 3 received, 0% packet loss, time 0ms >>> +]) >>> + >>> + >>> +NS_CHECK_EXEC([at_ns1], [ping -q -c 3 -i 0.3 -w 2 10.1.1.1 | FORMAT_PING], >>> [0], [dnl >>> +3 packets transmitted, 3 received, 0% packet loss, time 0ms >>> +]) >>> + >>> +OVS_TRAFFIC_VSWITCHD_STOP >>> +AT_CLEANUP >>> + >>> +AT_SETUP([datapath - multiple encap decap mpls actions]) >>> +OVS_TRAFFIC_VSWITCHD_START([_ADD_BR([br1])]) >>> + >>> +ADD_NAMESPACES(at_ns0, at_ns1) >>> + >>> +ADD_VETH(p0, at_ns0, br0, "10.1.1.1/24") >>> +ADD_VETH(p1, at_ns1, br1, "10.1.1.2/24") >>> + >>> +AT_CHECK([ip link add patch0 type veth peer name patch1]) >>> +on_exit 'ip link del patch0' >>> + >>> +AT_CHECK([ip link set dev patch0 up]) >>> +AT_CHECK([ip link set dev patch1 up]) >>> +AT_CHECK([ovs-vsctl add-port br0 patch0 -- set interface patch0 >>> ofport_request=100]) >>> +AT_CHECK([ovs-vsctl add-port br1 patch1 -- set interface patch1 >>> ofport_request=100]) >>> + >>> +AT_DATA([flows.txt], [dnl >>> +table=0,priority=100,dl_type=0x0800 >>> actions=encap(mpls(ether_type=0x8847)),set_mpls_label:3, >>> encap(mpls(ether_type=0x8847)),set_mpls_label:2,encap(ethernet),set_field:00:00:00:00:00:02->dl_dst,set_field:00:00:00:00:00:01->dl_src,output:100 >>> +table=0,priority=100,dl_type=0x8847,mpls_label=2 >>> actions=decap(),decap(packet_type(ns=1,type=0x8847)),decap(packet_type(ns=0,type=0)),resubmit(,3) >>> +table=0,priority=10 actions=resubmit(,3) >> >> Same as above, remove general traffic rule, and move all traffic over MPLS >> >>> +table=3,priority=10 actions=normal >>> +]) >>> + >>> +AT_CHECK([ovs-ofctl -Oopenflow13 add-flows br0 flows.txt]) >>> +AT_CHECK([ovs-ofctl -Oopenflow13 add-flows br1 flows.txt]) >>> + >>> +NS_CHECK_EXEC([at_ns0], [ping -q -c 3 -i 0.3 -w 2 10.1.1.2 | FORMAT_PING], >>> [0], [dnl >>> +3 packets transmitted, 3 received, 0% packet loss, time 0ms >>> +]) >>> + >>> + >>> NS_CHECK_EXEC([at_ns1], [ping -q -c 3 -i 0.3 -w 2 10.1.1.1 | FORMAT_PING], >>> [0], [dnl >>> 3 packets transmitted, 3 received, 0% packet loss, time 0ms >>> ]) >>> + >>> +OVS_TRAFFIC_VSWITCHD_STOP >>> +AT_CLEANUP >>> + >>> +AT_SETUP([datapath - encap mpls pop mpls actions]) >>> +OVS_TRAFFIC_VSWITCHD_START([_ADD_BR([br1])]) >>> + >>> +ADD_NAMESPACES(at_ns0, at_ns1) >>> + >>> +ADD_VETH(p0, at_ns0, br0, "10.1.1.1/24", 36:b1:ee:7c:01:03) >>> +ADD_VETH(p1, at_ns1, br1, "10.1.1.2/24", 36:b1:ee:7c:01:02) >>> + >>> +AT_CHECK([ip link add patch0 type veth peer name patch1]) >>> +on_exit 'ip link del patch0' >>> + >>> +AT_CHECK([ip link set dev patch0 up]) >>> +AT_CHECK([ip link set dev patch1 up]) >>> +AT_CHECK([ovs-vsctl add-port br0 patch0 -- set interface patch0 >>> ofport_request=100]) >>> +AT_CHECK([ovs-vsctl add-port br1 patch1 -- set interface patch1 >>> ofport_request=100]) >>> + >>> +AT_DATA([flows.txt], [dnl >>> +table=0,priority=100,dl_type=0x0800 >>> actions=decap,encap(mpls(ether_type=0x8847)),set_mpls_label:2,encap(ethernet),mod_dl_dst:36:b1:ee:7c:01:02,mod_dl_src:36:b1:ee:7c:01:03,output:100 >>> +table=0,priority=100,dl_type=0x8847,mpls_label=2 >>> actions=pop_mpls:0x0800,resubmit(,3) >>> +table=0,priority=10 actions=resubmit(,3) >>> +table=3,priority=10 actions=normal >>> +]) >> >> Same as earlier, remove general traffic rule (in flows and flows1), and move >> all traffic over MPLS >> > unlike previous 2 tests this is a layer 3 MPLS tunnelling test. So we > cannot remove the generic rule. Or we need to add a seperate rule for > ARP which generally not seen in a L3 VPN use case >>> +AT_DATA([flows1.txt], [dnl >>> +table=0,priority=100,dl_type=0x0800 >>> actions=decap,encap(mpls(ether_type=0x8847)),set_mpls_label:2,encap(ethernet),mod_dl_dst:36:b1:ee:7c:01:03,mod_dl_src:36:b1:ee:7c:01:02,output:100 >>> +table=0,priority=100,dl_type=0x8847,mpls_label=2 >>> actions=pop_mpls:0x0800,resubmit(,3) >>> +table=0,priority=10 actions=resubmit(,3) >>> +table=3,priority=10 actions=normal >>> +]) >>> + >>> +AT_CHECK([ovs-ofctl -Oopenflow13 add-flows br0 flows.txt]) >>> +AT_CHECK([ovs-ofctl -Oopenflow13 add-flows br1 flows1.txt]) >>> + >>> +NS_CHECK_EXEC([at_ns0], [ping -q -c 3 -i 0.3 -w 2 10.1.1.2 | FORMAT_PING], >>> [0], [dnl >>> +3 packets transmitted, 3 received, 0% packet loss, time 0ms >>> +]) >>> + >>> +NS_CHECK_EXEC([at_ns1], [ping -q -c 3 -i 0.3 -w 2 10.1.1.1 | FORMAT_PING], >>> [0], [dnl >>> +3 packets transmitted, 3 received, 0% packet loss, time 0ms >>> +]) >>> + >>> +OVS_TRAFFIC_VSWITCHD_STOP >>> +AT_CLEANUP >>> + >>> +AT_SETUP([datapath - push mpls decap mpls actions]) >>> +OVS_TRAFFIC_VSWITCHD_START([_ADD_BR([br1])]) >>> + >>> +ADD_NAMESPACES(at_ns0, at_ns1) >>> + >>> +ADD_VETH(p0, at_ns0, br0, "10.1.1.1/24", 36:b1:ee:7c:01:03) >>> +ADD_VETH(p1, at_ns1, br1, "10.1.1.2/24", 36:b1:ee:7c:01:02) >>> + >>> +AT_CHECK([ip link add patch0 type veth peer name patch1]) >>> +on_exit 'ip link del patch0' >>> + >>> +AT_CHECK([ip link set dev patch0 up]) >>> +AT_CHECK([ip link set dev patch1 up]) >>> +AT_CHECK([ovs-vsctl add-port br0 patch0 -- set interface patch0 >>> ofport_request=100]) >>> +AT_CHECK([ovs-vsctl add-port br1 patch1 -- set interface patch1 >>> ofport_request=100]) >>> + >>> +AT_DATA([flows.txt], [dnl >>> +table=0,priority=100,dl_type=0x0800 >>> actions=push_mpls:0x8847,set_field:2->mpls_label,output:100 >>> +table=0,priority=100,dl_type=0x8847,mpls_label=2 >>> actions=decap,decap(packet_type(ns=1,type=0x0800)),encap(ethernet),mod_dl_dst:36:b1:ee:7c:01:03,mod_dl_src:36:b1:ee:7c:01:02,resubmit(,3) >>> +table=0,priority=10 actions=resubmit(,3) >>> +table=3,priority=10 actions=normal >>> +]) >> >> Same as earlier, remove general traffic rule (in flows and flows1), and move >> all traffic over MPLS >> > same as above >>> +AT_DATA([flows1.txt], [dnl >>> +table=0,priority=100,dl_type=0x0800 >>> actions=push_mpls:0x8847,set_field:2->mpls_label,output:100 >>> +table=0,priority=100,dl_type=0x8847,mpls_label=2 >>> actions=decap,decap(packet_type(ns=1,type=0x0800)),encap(ethernet),mod_dl_dst:36:b1:ee:7c:01:02,mod_dl_src:36:b1:ee:7c:01:03,resubmit(,3) >>> +table=0,priority=10 actions=resubmit(,3) >>> +table=3,priority=10 actions=normal >>> +]) >>> + >>> +AT_CHECK([ovs-ofctl -Oopenflow13 add-flows br0 flows.txt]) >>> +AT_CHECK([ovs-ofctl -Oopenflow13 add-flows br1 flows1.txt]) >>> + >>> +NS_CHECK_EXEC([at_ns0], [ping -q -c 3 -i 0.3 -w 2 10.1.1.2 | FORMAT_PING], >>> [0], [dnl >>> +3 packets transmitted, 3 received, 0% packet loss, time 0ms >>> +]) >>> + >>> +NS_CHECK_EXEC([at_ns1], [ping -q -c 3 -i 0.3 -w 2 10.1.1.1 | FORMAT_PING], >>> [0], [dnl >>> +3 packets transmitted, 3 received, 0% packet loss, time 0ms >>> +]) >>> + >>> OVS_TRAFFIC_VSWITCHD_STOP >>> AT_CLEANUP >>> >>> -- >>> 2.18.4 >> > > Thanks Eelco for your time _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
