[ovs-dev] [PATCH v2] Change in openvswitch kernel module to support MPLS label depth of 3 in ingress direction.
Varghese, Martin (Nokia - IN/Bangalore)
martin.varghese at nokia.com
Fri Nov 22 06:11:00 UTC 2019
Hi
I have posted again.
Thanks,
Martin
-----Original Message-----
From: Tonghao Zhang <xiangxia.m.yue at gmail.com>
Sent: Friday, November 22, 2019 7:47 AM
To: Gregory Rose <gvrose8192 at gmail.com>; Varghese, Martin (Nokia - IN/Bangalore) <martin.varghese at nokia.com>
Cc: ovs dev <dev at openvswitch.org>
Subject: Re: [ovs-dev] [PATCH v2] Change in openvswitch kernel module to support MPLS label depth of 3 in ingress direction.
On Fri, Nov 22, 2019 at 2:15 AM Gregory Rose <gvrose8192 at gmail.com> wrote:
>
>
> On 11/21/2019 7:49 AM, Martin Varghese wrote:
> > From: Martin Varghese <martin.varghese at nokia.com>
> >
> > The openvswitch kernel module was supporting a MPLS label depth of 1
> > in the ingress direction though the userspace OVS supports a max
> > depth of 3 labels. This change enables openvswitch module to support
> > a max depth of 3 labels in the ingress.
> >
> > Signed-off-by: Martin Varghese <martin.varghese at nokia.com>
>
> Did you submit this patch upstream as well?
This patch was applied in upstream, commit id
fbdcdd78da7c95f1b970d371e1b23cbd3aa990f3
Hi Martin, when you backport the patch, the commit message should be formatted:
http://docs.openvswitch.org/en/latest/internals/contributing/backporting-patches/
> - Greg
>
> > ---
> > Changes in v2
> > - support added for nested actions.
> >
> > datapath/actions.c | 2 +-
> > datapath/flow.c | 20 ++++++++----
> > datapath/flow.h | 8 +++--
> > datapath/flow_netlink.c | 85 ++++++++++++++++++++++++++++++++++++-------------
> > tests/system-traffic.at | 39 +++++++++++++++++++++++
> > 5 files changed, 122 insertions(+), 32 deletions(-)
> >
> > diff --git a/datapath/actions.c b/datapath/actions.c index
> > a44e804..fbf4457 100644
> > --- a/datapath/actions.c
> > +++ b/datapath/actions.c
> > @@ -276,7 +276,7 @@ static int set_mpls(struct sk_buff *skb, struct sw_flow_key *flow_key,
> > }
> >
> > stack->label_stack_entry = lse;
> > - flow_key->mpls.top_lse = lse;
> > + flow_key->mpls.lse[0] = lse;
> > return 0;
> > }
> >
> > diff --git a/datapath/flow.c b/datapath/flow.c index
> > 916f7f4..6dc7402 100644
> > --- a/datapath/flow.c
> > +++ b/datapath/flow.c
> > @@ -659,27 +659,35 @@ static int key_extract_l3l4(struct sk_buff *skb, struct sw_flow_key *key)
> > memset(&key->ipv4, 0, sizeof(key->ipv4));
> > }
> > } else if (eth_p_mpls(key->eth.type)) {
> > - size_t stack_len = MPLS_HLEN;
> > + u8 label_count = 1;
> >
> > + memset(&key->mpls, 0, sizeof(key->mpls));
> > skb_set_inner_network_header(skb, skb->mac_len);
> > while (1) {
> > __be32 lse;
> >
> > - error = check_header(skb, skb->mac_len + stack_len);
> > + error = check_header(skb, skb->mac_len +
> > + label_count * MPLS_HLEN);
> > if (unlikely(error))
> > return 0;
> >
> > memcpy(&lse, skb_inner_network_header(skb),
> > MPLS_HLEN);
> >
> > - if (stack_len == MPLS_HLEN)
> > - memcpy(&key->mpls.top_lse, &lse, MPLS_HLEN);
> > + if (label_count <= MPLS_LABEL_DEPTH)
> > + memcpy(&key->mpls.lse[label_count - 1], &lse,
> > + MPLS_HLEN);
> >
> > - skb_set_inner_network_header(skb, skb->mac_len + stack_len);
> > + skb_set_inner_network_header(skb, skb->mac_len +
> > + label_count *
> > + MPLS_HLEN);
> > if (lse & htonl(MPLS_LS_S_MASK))
> > break;
> >
> > - stack_len += MPLS_HLEN;
> > + label_count++;
> > }
> > + if (label_count > MPLS_LABEL_DEPTH)
> > + label_count = MPLS_LABEL_DEPTH;
> > +
> > + key->mpls.num_labels_mask = GENMASK(label_count - 1,
> > + 0);
> > } else if (key->eth.type == htons(ETH_P_IPV6)) {
> > int nh_len; /* IPv6 Header + Extensions */
> >
> > diff --git a/datapath/flow.h b/datapath/flow.h index
> > 5560300..4ad5363 100644
> > --- a/datapath/flow.h
> > +++ b/datapath/flow.h
> > @@ -43,6 +43,7 @@ enum sw_flow_mac_proto {
> > MAC_PROTO_ETHERNET,
> > };
> > #define SW_FLOW_KEY_INVALID 0x80
> > +#define MPLS_LABEL_DEPTH 3
> >
> > /* Store options at the end of the array if they are less than the
> > * maximum size. This allows us to get the benefits of variable
> > length @@ -98,9 +99,6 @@ struct sw_flow_key {
> > */
> > union {
> > struct {
> > - __be32 top_lse; /* top label stack entry */
> > - } mpls;
> > - struct {
> > u8 proto; /* IP protocol or lower 8 bits of ARP opcode. */
> > u8 tos; /* IP ToS. */
> > u8 ttl; /* IP TTL/hop limit. */
> > @@ -148,6 +146,10 @@ struct sw_flow_key {
> > } nd;
> > };
> > } ipv6;
> > + struct {
> > + u32 num_labels_mask; /* labels present bitmap of effective length MPLS_LABEL_DEPTH */
> > + __be32 lse[MPLS_LABEL_DEPTH]; /* label stack entry */
> > + } mpls;
> > struct ovs_key_nsh nsh; /* network service header */
> > };
> > struct {
> > diff --git a/datapath/flow_netlink.c b/datapath/flow_netlink.c index
> > 35f13d7..9fc1a19 100644
> > --- a/datapath/flow_netlink.c
> > +++ b/datapath/flow_netlink.c
> > @@ -438,7 +438,7 @@ static const struct ovs_len_tbl ovs_key_lens[OVS_KEY_ATTR_MAX + 1] = {
> > [OVS_KEY_ATTR_DP_HASH] = { .len = sizeof(u32) },
> > [OVS_KEY_ATTR_TUNNEL] = { .len = OVS_ATTR_NESTED,
> > .next = ovs_tunnel_key_lens, },
> > - [OVS_KEY_ATTR_MPLS] = { .len = sizeof(struct ovs_key_mpls) },
> > + [OVS_KEY_ATTR_MPLS] = { .len = OVS_ATTR_VARIABLE },
> > [OVS_KEY_ATTR_CT_STATE] = { .len = sizeof(u32) },
> > [OVS_KEY_ATTR_CT_ZONE] = { .len = sizeof(u16) },
> > [OVS_KEY_ATTR_CT_MARK] = { .len = sizeof(u32) },
> > @@ -1619,10 +1619,27 @@ static int ovs_key_from_nlattrs(struct net
> > *net, struct sw_flow_match *match,
> >
> > if (attrs & (1ULL << OVS_KEY_ATTR_MPLS)) {
> > const struct ovs_key_mpls *mpls_key;
> > + u32 hdr_len;
> > + u32 label_count, label_count_mask, i;
> > +
> >
> > mpls_key = nla_data(a[OVS_KEY_ATTR_MPLS]);
> > - SW_FLOW_KEY_PUT(match, mpls.top_lse,
> > - mpls_key->mpls_lse, is_mask);
> > + hdr_len = nla_len(a[OVS_KEY_ATTR_MPLS]);
> > + label_count = hdr_len / sizeof(struct ovs_key_mpls);
> > +
> > + if (label_count == 0 || label_count > MPLS_LABEL_DEPTH ||
> > + hdr_len % sizeof(struct ovs_key_mpls))
> > + return -EINVAL;
> > +
> > + label_count_mask = GENMASK(label_count - 1, 0);
> > +
> > + for (i = 0 ; i < label_count; i++)
> > + SW_FLOW_KEY_PUT(match, mpls.lse[i],
> > + mpls_key[i].mpls_lse,
> > + is_mask);
> > +
> > + SW_FLOW_KEY_PUT(match, mpls.num_labels_mask,
> > + label_count_mask, is_mask);
> > +
> >
> > attrs &= ~(1ULL << OVS_KEY_ATTR_MPLS);
> > }
> > @@ -2104,13 +2121,18 @@ static int __ovs_nla_put_key(const struct sw_flow_key *swkey,
> > ether_addr_copy(arp_key->arp_sha, output->ipv4.arp.sha);
> > ether_addr_copy(arp_key->arp_tha, output->ipv4.arp.tha);
> > } else if (eth_p_mpls(swkey->eth.type)) {
> > + u8 num_labels, i;
> > struct ovs_key_mpls *mpls_key;
> >
> > - nla = nla_reserve(skb, OVS_KEY_ATTR_MPLS, sizeof(*mpls_key));
> > + num_labels = hweight_long(output->mpls.num_labels_mask);
> > + nla = nla_reserve(skb, OVS_KEY_ATTR_MPLS,
> > + num_labels * sizeof(*mpls_key));
> > if (!nla)
> > goto nla_put_failure;
> > +
> > mpls_key = nla_data(nla);
> > - mpls_key->mpls_lse = output->mpls.top_lse;
> > + for (i = 0; i < num_labels; i++)
> > + mpls_key[i].mpls_lse = output->mpls.lse[i];
> > }
> >
> > if ((swkey->eth.type == htons(ETH_P_IP) || @@ -2400,13
> > +2422,14 @@ static inline void add_nested_action_end(struct sw_flow_actions *sfa,
> > static int __ovs_nla_copy_actions(struct net *net, const struct nlattr *attr,
> > const struct sw_flow_key *key,
> > struct sw_flow_actions **sfa,
> > - __be16 eth_type, __be16 vlan_tci, bool log);
> > + __be16 eth_type, __be16 vlan_tci,
> > + u32 mpls_label_count, bool log);
> >
> > static int validate_and_copy_sample(struct net *net, const struct nlattr *attr,
> > const struct sw_flow_key *key,
> > struct sw_flow_actions **sfa,
> > __be16 eth_type, __be16 vlan_tci,
> > - bool log, bool last)
> > + u32 mpls_label_count, bool log,
> > + bool last)
> > {
> > const struct nlattr *attrs[OVS_SAMPLE_ATTR_MAX + 1];
> > const struct nlattr *probability, *actions; @@ -2457,7 +2480,7
> > @@ static int validate_and_copy_sample(struct net *net, const struct nlattr *attr,
> > return err;
> >
> > err = __ovs_nla_copy_actions(net, actions, key, sfa,
> > - eth_type, vlan_tci, log);
> > + eth_type, vlan_tci,
> > + mpls_label_count, log);
> >
> > if (err)
> > return err;
> > @@ -2472,7 +2495,7 @@ static int validate_and_copy_clone(struct net *net,
> > const struct sw_flow_key *key,
> > struct sw_flow_actions **sfa,
> > __be16 eth_type, __be16 vlan_tci,
> > - bool log, bool last)
> > + u32 mpls_label_count, bool log,
> > + bool last)
> > {
> > int start, err;
> > u32 exec;
> > @@ -2492,7 +2515,7 @@ static int validate_and_copy_clone(struct net *net,
> > return err;
> >
> > err = __ovs_nla_copy_actions(net, attr, key, sfa,
> > - eth_type, vlan_tci, log);
> > + eth_type, vlan_tci,
> > + mpls_label_count, log);
> > if (err)
> > return err;
> >
> > @@ -2859,6 +2882,7 @@ static int validate_and_copy_check_pkt_len(struct net *net,
> > const struct sw_flow_key *key,
> > struct sw_flow_actions **sfa,
> > __be16 eth_type, __be16
> > vlan_tci,
> > + u32 mpls_label_count,
> > bool log, bool last)
> > {
> > const struct nlattr *acts_if_greater, *acts_if_lesser_eq; @@
> > -2906,7 +2930,7 @@ static int validate_and_copy_check_pkt_len(struct net *net,
> > return nested_acts_start;
> >
> > err = __ovs_nla_copy_actions(net, acts_if_lesser_eq, key, sfa,
> > - eth_type, vlan_tci, log);
> > + eth_type, vlan_tci,
> > + mpls_label_count, log);
> >
> > if (err)
> > return err;
> > @@ -2919,7 +2943,7 @@ static int validate_and_copy_check_pkt_len(struct net *net,
> > return nested_acts_start;
> >
> > err = __ovs_nla_copy_actions(net, acts_if_greater, key, sfa,
> > - eth_type, vlan_tci, log);
> > + eth_type, vlan_tci,
> > + mpls_label_count, log);
> >
> > if (err)
> > return err;
> > @@ -2946,7 +2970,8 @@ static int copy_action(const struct nlattr *from,
> > static int __ovs_nla_copy_actions(struct net *net, const struct nlattr *attr,
> > const struct sw_flow_key *key,
> > struct sw_flow_actions **sfa,
> > - __be16 eth_type, __be16 vlan_tci, bool log)
> > + __be16 eth_type, __be16 vlan_tci,
> > + u32 mpls_label_count, bool log)
> > {
> > u8 mac_proto = ovs_key_mac_proto(key);
> > const struct nlattr *a;
> > @@ -3059,26 +3084,35 @@ static int __ovs_nla_copy_actions(struct net *net, const struct nlattr *attr,
> > !eth_p_mpls(eth_type)))
> > return -EINVAL;
> > eth_type = mpls->mpls_ethertype;
> > + mpls_label_count++;
> > break;
> > }
> >
> > - case OVS_ACTION_ATTR_POP_MPLS:
> > + case OVS_ACTION_ATTR_POP_MPLS: {
> > + __be16 proto;
> > if (vlan_tci & htons(VLAN_CFI_MASK) ||
> > !eth_p_mpls(eth_type))
> > return -EINVAL;
> >
> > - /* Disallow subsequent L2.5+ set and mpls_pop actions
> > - * as there is no check here to ensure that the new
> > - * eth_type is valid and thus set actions could
> > - * write off the end of the packet or otherwise
> > - * corrupt it.
> > + /* Disallow subsequent L2.5+ set actions and mpls_pop
> > + * actions once the last MPLS label in the packet is
> > + * popped as there is no check here to ensure that
> > + * the new eth type is valid and thus set actions could
> > + * write off the end of the packet or otherwise corrupt
> > + * it.
> > *
> > * Support for these actions is planned using packet
> > * recirculation.
> > */
> > - eth_type = htons(0);
> > - break;
> > + proto = nla_get_be16(a);
> > + mpls_label_count--;
> >
> > + if (!eth_p_mpls(proto) || !mpls_label_count)
> > + eth_type = htons(0);
> > + else
> > + eth_type = proto;
> > + break;
> > + }
> > case OVS_ACTION_ATTR_SET:
> > err = validate_set(a, key, sfa,
> > &skip_copy, mac_proto,
> > eth_type, @@ -3100,6 +3134,7 @@ static int
> > __ovs_nla_copy_actions(struct net *net, const struct nlattr *attr,
> >
> > err = validate_and_copy_sample(net, a, key, sfa,
> > eth_type,
> > vlan_tci,
> > +
> > + mpls_label_count,
> > log, last);
> > if (err)
> > return err; @@ -3170,6 +3205,7 @@
> > static int __ovs_nla_copy_actions(struct net *net, const struct
> > nlattr *attr,
> >
> > err = validate_and_copy_clone(net, a, key, sfa,
> > eth_type,
> > vlan_tci,
> > +
> > + mpls_label_count,
> > log, last);
> > if (err)
> > return err; @@ -3183,6 +3219,7 @@
> > static int __ovs_nla_copy_actions(struct net *net, const struct nlattr *attr,
> > err = validate_and_copy_check_pkt_len(net, a, key, sfa,
> > eth_type,
> >
> > vlan_tci, log,
> > +
> > + mpls_label_count,
> > last);
> > if (err)
> > return err; @@ -3213,14 +3250,18
> > @@ int ovs_nla_copy_actions(struct net *net, const struct nlattr *attr,
> > struct sw_flow_actions **sfa, bool log)
> > {
> > int err;
> > + u32 mpls_label_count = 0;
> >
> > *sfa = nla_alloc_flow_actions(min(nla_len(attr), MAX_ACTIONS_BUFSIZE));
> > if (IS_ERR(*sfa))
> > return PTR_ERR(*sfa);
> >
> > + if (eth_p_mpls(key->eth.type))
> > + mpls_label_count =
> > + hweight_long(key->mpls.num_labels_mask);
> > +
> > (*sfa)->orig_len = nla_len(attr);
> > err = __ovs_nla_copy_actions(net, attr, key, sfa, key->eth.type,
> > - key->eth.vlan.tci, log);
> > + key->eth.vlan.tci,
> > + mpls_label_count, log);
> > if (err)
> > ovs_nla_free_flow_actions(*sfa);
> >
> > diff --git a/tests/system-traffic.at b/tests/system-traffic.at index
> > 870a05e..cde7429 100644
> > --- a/tests/system-traffic.at
> > +++ b/tests/system-traffic.at
> > @@ -992,6 +992,45 @@ NS_CHECK_EXEC([at_ns1], [ping -q -c 3 -i 0.3 -w
> > 2 10.1.1.1 | FORMAT_PING], [0],
> >
> > OVS_TRAFFIC_VSWITCHD_STOP
> > AT_CLEANUP
> > +
> > +AT_SETUP([datapath - multiple mpls label pop])
> > +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])
> > +AT_CHECK([ovs-vsctl add-port br1 patch1])
> > +
> > +AT_DATA([flows.txt], [dnl
> > +table=0,priority=100,dl_type=0x0800
> > +actions=push_mpls:0x8847,set_mpls_label:3,push_mpls:0x8847,set_mpls
> > +_label:2,push_mpls:0x8847,set_mpls_label:1,resubmit(,3)
> > +table=0,priority=100,dl_type=0x8847,mpls_label=1
> > +actions=pop_mpls:0x8847,resubmit(,1)
> > +table=1,priority=100,dl_type=0x8847,mpls_label=2
> > +actions=pop_mpls:0x8847,resubmit(,2)
> > +table=2,priority=100,dl_type=0x8847,mpls_label=3
> > +actions=pop_mpls:0x0800,resubmit(,3)
> > +table=0,priority=10 actions=resubmit(,3)
> > +table=3,priority=10 actions=normal
> > +])
> > +
> > +AT_CHECK([ovs-ofctl add-flows br0 flows.txt]) AT_CHECK([ovs-ofctl
> > +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 - basic truncate action])
> > AT_SKIP_IF([test $HAVE_NC = no])
> > OVS_TRAFFIC_VSWITCHD_START()
>
> _______________________________________________
> dev mailing list
> dev at openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
More information about the dev
mailing list