[{"id":1760822,"web_url":"http://patchwork.ozlabs.org/comment/1760822/","msgid":"<20170831124516.0c5db686@griffin>","list_archive_url":null,"date":"2017-08-31T10:45:16","subject":"Re: [PATCH net-next v7] openvswitch: enable NSH support","submitter":{"id":9287,"url":"http://patchwork.ozlabs.org/api/people/9287/","name":"Jiri Benc","email":"jbenc@redhat.com"},"content":"On Wed, 30 Aug 2017 20:39:12 +0800, Yi Yang wrote:\n> --- a/net/nsh/nsh.c\n> +++ b/net/nsh/nsh.c\n> @@ -14,6 +14,47 @@\n> #include \n> #include \n> \n> +int skb_push_nsh(struct sk_buff *skb, const struct nshhdr *nsh_src, bool is_eth)\n> +{\n> +\tstruct nshhdr *nsh;\n> +\tsize_t length = nsh_hdr_len(nsh_src);\n> +\tu8 next_proto;\n> +\n> +\tif (is_eth) {\n> +\t\tnext_proto = TUN_P_ETHERNET;\n\nI don't like the is_eth parameter. It should be clear from the skb being\npassed to the function, specifically from skb->mac_len.\n\n> +\t} else {\n> +\t\tnext_proto = tun_p_from_eth_p(skb->protocol);\n> +\t\tif (!next_proto)\n> +\t\t\treturn -ENOTSUPP;\n\nEAFNOSUPPORT is more suitable here.\n\n> +\t}\n> +\n> +\t/* Add the NSH header */\n> +\tif (skb_cow_head(skb, length) < 0)\n> +\t\treturn -ENOMEM;\n> +\n> +\t/* Add the NSH header */\n> +\tif (skb_cow_head(skb, length) < 0)\n> +\t\treturn -ENOMEM;\n\nDuplicate statement.\n\n> +\n> +\tif (!skb->inner_protocol) {\n> +\t\tskb_set_inner_network_header(skb, skb->mac_len);\n> +\t\tskb_set_inner_protocol(skb, skb->protocol);\n\nIt doesn't make sense to set either of those. Nothing uses the fields for\nNSH.\n\n> +\t}\n> +\n> +\tskb_push(skb, length);\n> +\tnsh = (struct nshhdr *)(skb->data);\n\nUse nsh_hdr.\n\n> +\tmemcpy(nsh, nsh_src, length);\n> +\tnsh->np = next_proto;\n> +\tnsh->mdtype &= NSH_MDTYPE_MASK;\n\nThe \"& NSH_MDTYPE_MASK\" operation does not make sense. Just trust the caller\nto provide a meaningful value. It's its job to verify the parameters.\n\n> +\tskb->protocol = htons(ETH_P_NSH);\n> +\tskb_reset_mac_header(skb);\n> +\tskb_reset_mac_len(skb);\n\nYou have to reset also the network header.\n\n> +\n> +\treturn 0;\n> +}\n> +EXPORT_SYMBOL_GPL(skb_push_nsh);\n> +\n> static struct sk_buff *nsh_gso_segment(struct sk_buff *skb,\n> \t\t\t\t netdev_features_t features)\n> {\n> diff --git a/net/openvswitch/actions.c b/net/openvswitch/actions.c\n> index a54a556..e969fad 100644\n> --- a/net/openvswitch/actions.c\n> +++ b/net/openvswitch/actions.c\n> @@ -38,11 +38,13 @@\n> #include \n> #include \n> #include \n> +#include \n> \n> #include \"datapath.h\"\n> #include \"flow.h\"\n> #include \"conntrack.h\"\n> #include \"vport.h\"\n> +#include \"flow_netlink.h\"\n> \n> struct deferred_action {\n> \tstruct sk_buff *skb;\n> @@ -380,6 +382,57 @@ static int push_eth(struct sk_buff *skb, struct sw_flow_key *key,\n> \treturn 0;\n> }\n> \n> +static int push_nsh(struct sk_buff *skb, struct sw_flow_key *key,\n> +\t\t const struct nshhdr *nsh_src)\n> +{\n> +\tbool is_eth = false;\n> +\tint ret;\n> +\n> +\tif (key->mac_proto == MAC_PROTO_ETHERNET)\n> +\t\tis_eth = true;\n> +\n> +\tret = skb_push_nsh(skb, nsh_src, is_eth);\n> +\tif (ret != 0)\n\nThe usual idiom is \"if (ret)\". And for consistency with the rest of the\nfile, the name of the variable should be \"err\".\n\n> +\t\treturn ret;\n> +\n> +\tkey->eth.type = htons(ETH_P_NSH);\n> +\n> +\t/* safe right before invalidate_flow_key */\n> +\tkey->mac_proto = MAC_PROTO_NONE;\n> +\tinvalidate_flow_key(key);\n> +\treturn 0;\n> +}\n> +\n> +static int pop_nsh(struct sk_buff *skb, struct sw_flow_key *key)\n> +{\n> +\tstruct nshhdr *nsh = (struct nshhdr *)(skb->data);\n> +\tsize_t length;\n> +\tu16 inner_proto;\n> +\n> +\tif (ovs_key_mac_proto(key) != MAC_PROTO_NONE ||\n> +\t skb->protocol != htons(ETH_P_NSH)) {\n> +\t\treturn -EINVAL;\n> +\t}\n> +\n> +\tinner_proto = tun_p_to_eth_p(nsh->np);\n> +\tif (!inner_proto)\n> +\t\treturn -ENOTSUPP;\n> +\n> +\tlength = nsh_hdr_len(nsh);\n> +\tskb_pull(skb, length);\n> +\tskb_reset_mac_header(skb);\n> +\tskb_reset_mac_len(skb);\n> +\tskb->protocol = inner_proto;\n\nPlease factor this out to skb_pop_nsh, similarly to what you did with\nskb_push_nsh.\n\n> +\n> +\t/* safe right before invalidate_flow_key */\n> +\tif (inner_proto == htons(ETH_P_TEB))\n> +\t\tkey->mac_proto = MAC_PROTO_ETHERNET;\n> +\telse\n> +\t\tkey->mac_proto = MAC_PROTO_NONE;\n> +\tinvalidate_flow_key(key);\n> +\treturn 0;\n> +}\n> +\n> static void update_ip_l4_checksum(struct sk_buff *skb, struct iphdr *nh,\n> \t\t\t\t __be32 addr, __be32 new_addr)\n> {\n> @@ -602,6 +655,53 @@ static int set_ipv6(struct sk_buff *skb, struct sw_flow_key *flow_key,\n> \treturn 0;\n> }\n> \n> +static int set_nsh(struct sk_buff *skb, struct sw_flow_key *flow_key,\n> +\t\t const struct ovs_key_nsh *key,\n> +\t\t const struct ovs_key_nsh *mask)\n> +{\n> +\tstruct nshhdr *nsh;\n> +\tint err;\n> +\tu8 flags;\n> +\tu8 ttl;\n> +\tint i;\n> +\n> +\terr = skb_ensure_writable(skb, skb_network_offset(skb) +\n> +\t\t\t\t sizeof(struct nshhdr));\n> +\tif (unlikely(err))\n> +\t\treturn err;\n> +\n> +\tnsh = (struct nshhdr *)skb_network_header(skb);\n\nnsh_hdr\n\n> +\n> +\tflags = nsh_get_flags(nsh);\n> +\tflags = OVS_MASKED(flags, key->flags, mask->flags);\n> +\tflow_key->nsh.flags = flags;\n> +\tttl = nsh_get_ttl(nsh);\n> +\tttl = OVS_MASKED(ttl, key->ttl, mask->ttl);\n> +\tflow_key->nsh.ttl = ttl;\n> +\tnsh_set_flags_and_ttl(nsh, flags, ttl);\n> +\tnsh->path_hdr = OVS_MASKED(nsh->path_hdr, key->path_hdr,\n> +\t\t\t\t mask->path_hdr);\n> +\tflow_key->nsh.path_hdr = nsh->path_hdr;\n> +\tswitch (nsh->mdtype) {\n> +\tcase NSH_M_TYPE1:\n> +\t\tfor (i = 0; i < NSH_MD1_CONTEXT_SIZE; i++) {\n> +\t\t\tnsh->md1.context[i] =\n> +\t\t\t OVS_MASKED(nsh->md1.context[i], key->context[i],\n> +\t\t\t\t mask->context[i]);\n> +\t\t}\n> +\t\tmemcpy(flow_key->nsh.context, nsh->md1.context,\n> +\t\t sizeof(nsh->md1.context));\n\nDo you follow the discussion that Hannes Sowa started on the ovs list\nregarding matching on the context fields? It would be better to hold on this\nuntil there's a conclusion reached.\n\n> +\t\tbreak;\n> +\tcase NSH_M_TYPE2:\n> +\t\tmemset(flow_key->nsh.context, 0,\n> +\t\t sizeof(flow_key->nsh.context));\n> +\t\tbreak;\n> +\tdefault:\n> +\t\treturn -EINVAL;\n> +\t}\n> +\treturn 0;\n> +}\n> +\n> /* Must follow skb_ensure_writable() since that can move the skb data. */\n> static void set_tp_port(struct sk_buff *skb, __be16 *port,\n> \t\t\t__be16 new_port, __sum16 *check)\n> @@ -1024,6 +1124,32 @@ static int execute_masked_set_action(struct sk_buff *skb,\n> \t\t\t\t get_mask(a, struct ovs_key_ethernet *));\n> \t\tbreak;\n> \n> +\tcase OVS_KEY_ATTR_NSH: {\n> +\t\tstruct ovs_key_nsh nsh;\n> +\t\tstruct ovs_key_nsh nsh_mask;\n> +\t\tsize_t size = nla_len(a) / 2;\n> +\t\tstruct {\n> +\t\t\tstruct nlattr nla;\n> +\t\t\tu8 data[size];\n> +\t\t} attr, mask;\n> +\n> +\t\tattr.nla.nla_type = nla_type(a);\n> +\t\tattr.nla.nla_len = NLA_HDRLEN + size;\n> +\t\tmemcpy(attr.data, (char *)(a + 1), size);\n> +\n> +\t\tmask.nla = attr.nla;\n> +\t\tmemcpy(mask.data, (char *)(a + 1) + size, size);\n\nThis is much cleaner than before. Thank you for doing the change. It now\nallows me to understand what's actually going on here.\n\n> +\t\terr = nsh_key_from_nlattr(&attr.nla, &nsh);\n> +\t\tif (err)\n> +\t\t\tbreak;\n> +\t\terr = nsh_key_from_nlattr(&mask.nla, &nsh_mask);\n> +\t\tif (err)\n> +\t\t\tbreak;\n\nThis is a lot of copying to just be able to use nla_for_each_nested. While\nI'm not a fan of how ovs uses the attributes to store both value and mask,\nit's not completely irrational. However, I don't think that the intent was\nto store a copy of the whole nested group. It's quite obvious that it does\nnot fit the pattern from the gymnastics you need to do.\n\nInstead, store the mask in each of the nested attributes individually. There\nwill be no need for the copying above and the code will get cleaner and\nfaster. It's not that the nsh_key_from_nlattr function needs to be able to\nwork separately for the key and mask. Nothing else than the line above uses\nthis function. Just move the logic inside.\n\nActually, it seems it can be moved directly to set_nsh.\n\n> +\t\terr = set_nsh(skb, flow_key, &nsh, &nsh_mask);\n> +\t\tbreak;\n> +\t}\n> +\n> \tcase OVS_KEY_ATTR_IPV4:\n> \t\terr = set_ipv4(skb, flow_key, nla_data(a),\n> \t\t\t get_mask(a, struct ovs_key_ipv4 *));\n> @@ -1210,6 +1336,21 @@ static int do_execute_actions(struct datapath *dp, struct sk_buff *skb,\n> \t\tcase OVS_ACTION_ATTR_POP_ETH:\n> \t\t\terr = pop_eth(skb, key);\n> \t\t\tbreak;\n> +\n> +\t\tcase OVS_ACTION_ATTR_PUSH_NSH: {\n> +\t\t\tu8 buffer[NSH_HDR_MAX_LEN];\n> +\t\t\tstruct nshhdr *nsh_hdr = (struct nshhdr *)buffer;\n> +\t\t\tconst struct nshhdr *nsh_src = nsh_hdr;\n> +\n> +\t\t\tnsh_hdr_from_nlattr(nla_data(a), nsh_hdr,\n> +\t\t\t\t\t NSH_HDR_MAX_LEN);\n\nThis is costly. This is called for every packet in the fast path. We should\nprecompute and store the header instead.\n\nI know I had comments to this part before and it would be ideal if I raised\nthis earlier but since you had trivial bugs like not taking the buffer size\ninto account, it was hard to focus on the design side of things. You can\nprevent this churn by paying more attention to details before submission.\nIt's really hard to focus on high level picture when the first thing one\nencounters is a trivial bug.\n\n> +\t\t\terr = push_nsh(skb, key, nsh_src);\n> +\t\t\tbreak;\n> +\t\t}\n> +\n> +\t\tcase OVS_ACTION_ATTR_POP_NSH:\n> +\t\t\terr = pop_nsh(skb, key);\n> +\t\t\tbreak;\n> \t\t}\n> \n> \t\tif (unlikely(err)) {\n> diff --git a/net/openvswitch/flow.c b/net/openvswitch/flow.c\n> index 8c94cef..7a178d1 100644\n> --- a/net/openvswitch/flow.c\n> +++ b/net/openvswitch/flow.c\n> @@ -46,6 +46,7 @@\n> #include \n> #include \n> #include \n> +#include \n> \n> #include \"conntrack.h\"\n> #include \"datapath.h\"\n> @@ -490,6 +491,56 @@ static int parse_icmpv6(struct sk_buff *skb, struct sw_flow_key *key,\n> \treturn 0;\n> }\n> \n> +static int parse_nsh(struct sk_buff *skb, struct sw_flow_key *key)\n> +{\n> +\tstruct nshhdr *nsh;\n> +\tunsigned int nh_ofs = skb_network_offset(skb);\n> +\tu8 version, length;\n> +\tint err;\n> +\n> +\terr = check_header(skb, nh_ofs + NSH_BASE_HDR_LEN);\n> +\tif (unlikely(err))\n> +\t\treturn err;\n> +\n> +\tnsh = (struct nshhdr *)skb_network_header(skb);\n\nnsh_hdr\n\n> +\tversion = nsh_get_ver(nsh);\n> +\tlength = nsh_hdr_len(nsh);\n> +\n> +\tif (version != 0)\n> +\t\treturn -EINVAL;\n> +\n> +\terr = check_header(skb, nh_ofs + length);\n> +\tif (unlikely(err))\n> +\t\treturn err;\n> +\n> +\tnsh = (struct nshhdr *)skb_network_header(skb);\n\nnsh_hdr\n\n> +\tkey->nsh.flags = nsh_get_flags(nsh);\n> +\tkey->nsh.ttl = nsh_get_ttl(nsh);\n> +\tkey->nsh.mdtype = nsh->mdtype;\n> +\tkey->nsh.np = nsh->np;\n> +\tkey->nsh.path_hdr = nsh->path_hdr;\n> +\tswitch (key->nsh.mdtype) {\n> +\tcase NSH_M_TYPE1:\n> +\t\tif (length != NSH_M_TYPE1_LEN)\n> +\t\t\treturn -EINVAL;\n> +\t\tmemcpy(key->nsh.context, nsh->md1.context,\n> +\t\t sizeof(nsh->md1));\n> +\t\tbreak;\n> +\tcase NSH_M_TYPE2:\n> +\t\t/* Don't support MD type 2 metedata parsing yet */\n> +\t\tif (length < NSH_BASE_HDR_LEN)\n> +\t\t\treturn -EINVAL;\n\nShouldn't we reject the packet, then? Pretending that something was parsed\ncorrectly while in fact it was not, does not look correct.\n\n> +\n> +\t\tmemset(key->nsh.context, 0,\n> +\t\t sizeof(nsh->md1));\n> +\t\tbreak;\n> +\tdefault:\n> +\t\treturn -EINVAL;\n> +\t}\n> +\n> +\treturn 0;\n> +}\n> +\n> /**\n> * key_extract - extracts a flow key from an Ethernet frame.\n> * @skb: sk_buff that contains the frame, with skb->data pointing to the\n> @@ -735,6 +786,10 @@ static int key_extract(struct sk_buff *skb, struct sw_flow_key *key)\n> \t\t\t\tmemset(&key->tp, 0, sizeof(key->tp));\n> \t\t\t}\n> \t\t}\n> +\t} else if (key->eth.type == htons(ETH_P_NSH)) {\n> +\t\terror = parse_nsh(skb, key);\n> +\t\tif (error)\n> +\t\t\treturn error;\n> \t}\n> \treturn 0;\n> }\n> diff --git a/net/openvswitch/flow.h b/net/openvswitch/flow.h\n> index 1875bba..6a3cd9c 100644\n> --- a/net/openvswitch/flow.h\n> +++ b/net/openvswitch/flow.h\n> @@ -35,6 +35,7 @@\n> #include \n> #include \n> #include \n> +#include \n> \n> struct sk_buff;\n> \n> @@ -66,6 +67,15 @@ struct vlan_head {\n> \t(offsetof(struct sw_flow_key, recirc_id) +\t\\\n> \tFIELD_SIZEOF(struct sw_flow_key, recirc_id))\n> \n> +struct ovs_key_nsh {\n> +\tu8 flags;\n> +\tu8 ttl;\n> +\tu8 mdtype;\n> +\tu8 np;\n> +\t__be32 path_hdr;\n> +\t__be32 context[NSH_MD1_CONTEXT_SIZE];\n> +};\n> +\n> struct sw_flow_key {\n> \tu8 tun_opts[IP_TUNNEL_OPTS_MAX];\n> \tu8 tun_opts_len;\n> @@ -144,6 +154,7 @@ struct sw_flow_key {\n> \t\t\t};\n> \t\t} ipv6;\n> \t};\n> +\tstruct ovs_key_nsh nsh; /* network service header */\n> \tstruct {\n> \t\t/* Connection tracking fields not packed above. */\n> \t\tstruct {\n> diff --git a/net/openvswitch/flow_netlink.c b/net/openvswitch/flow_netlink.c\n> index e8eb427..17df00a 100644\n> --- a/net/openvswitch/flow_netlink.c\n> +++ b/net/openvswitch/flow_netlink.c\n> @@ -48,6 +48,7 @@\n> #include \n> #include \n> #include \n> +#include \n> \n> #include \"flow_netlink.h\"\n> \n> @@ -78,9 +79,11 @@ static bool actions_may_change_flow(const struct nlattr *actions)\n> \t\tcase OVS_ACTION_ATTR_HASH:\n> \t\tcase OVS_ACTION_ATTR_POP_ETH:\n> \t\tcase OVS_ACTION_ATTR_POP_MPLS:\n> +\t\tcase OVS_ACTION_ATTR_POP_NSH:\n> \t\tcase OVS_ACTION_ATTR_POP_VLAN:\n> \t\tcase OVS_ACTION_ATTR_PUSH_ETH:\n> \t\tcase OVS_ACTION_ATTR_PUSH_MPLS:\n> +\t\tcase OVS_ACTION_ATTR_PUSH_NSH:\n> \t\tcase OVS_ACTION_ATTR_PUSH_VLAN:\n> \t\tcase OVS_ACTION_ATTR_SAMPLE:\n> \t\tcase OVS_ACTION_ATTR_SET:\n> @@ -322,12 +325,27 @@ size_t ovs_tun_key_attr_size(void)\n> \t\t+ nla_total_size(2); /* OVS_TUNNEL_KEY_ATTR_TP_DST */\n> }\n> \n> +size_t ovs_nsh_key_attr_size(void)\n> +{\n> +\t/* Whenever adding new OVS_NSH_KEY_ FIELDS, we should consider\n> +\t * updating this function.\n> +\t */\n> +\treturn nla_total_size(NSH_BASE_HDR_LEN) /* OVS_NSH_KEY_ATTR_BASE */\n> +\t\t/* OVS_NSH_KEY_ATTR_MD1 and OVS_NSH_KEY_ATTR_MD2 are\n> +\t\t * mutually exclusive, so the bigger one can cover\n> +\t\t * the small one.\n> +\t\t *\n> +\t\t * OVS_NSH_KEY_ATTR_MD2\n> +\t\t */\n> +\t\t+ nla_total_size(NSH_CTX_HDRS_MAX_LEN);\n> +}\n> +\n> size_t ovs_key_attr_size(void)\n> {\n> \t/* Whenever adding new OVS_KEY_ FIELDS, we should consider\n> \t * updating this function.\n> \t */\n> -\tBUILD_BUG_ON(OVS_KEY_ATTR_TUNNEL_INFO != 28);\n> +\tBUILD_BUG_ON(OVS_KEY_ATTR_TUNNEL_INFO != 29);\n> \n> \treturn nla_total_size(4) /* OVS_KEY_ATTR_PRIORITY */\n> \t\t+ nla_total_size(0) /* OVS_KEY_ATTR_TUNNEL */\n> @@ -341,6 +359,8 @@ size_t ovs_key_attr_size(void)\n> \t\t+ nla_total_size(4) /* OVS_KEY_ATTR_CT_MARK */\n> \t\t+ nla_total_size(16) /* OVS_KEY_ATTR_CT_LABELS */\n> \t\t+ nla_total_size(40) /* OVS_KEY_ATTR_CT_ORIG_TUPLE_IPV6 */\n> +\t\t+ nla_total_size(0) /* OVS_KEY_ATTR_NSH */\n> +\t\t + ovs_nsh_key_attr_size()\n> \t\t+ nla_total_size(12) /* OVS_KEY_ATTR_ETHERNET */\n> \t\t+ nla_total_size(2) /* OVS_KEY_ATTR_ETHERTYPE */\n> \t\t+ nla_total_size(4) /* OVS_KEY_ATTR_VLAN */\n> @@ -373,6 +393,13 @@ static const struct ovs_len_tbl ovs_tunnel_key_lens[OVS_TUNNEL_KEY_ATTR_MAX + 1]\n> \t[OVS_TUNNEL_KEY_ATTR_IPV6_DST] = { .len = sizeof(struct in6_addr) },\n> };\n> \n> +static const struct ovs_len_tbl\n> +ovs_nsh_key_attr_lens[OVS_NSH_KEY_ATTR_MAX + 1] = {\n> +\t[OVS_NSH_KEY_ATTR_BASE] = { .len = 8 },\n\nsizeof(struct ovs_nsh_key_base)\n\n> +\t[OVS_NSH_KEY_ATTR_MD1] = { .len = 16 },\n\nsizeof(struct ovs_nsh_key_md1)\n\n> +\t[OVS_NSH_KEY_ATTR_MD2] = { .len = OVS_ATTR_VARIABLE },\n> +};\n> +\n> /* The size of the argument for each %OVS_KEY_ATTR_* Netlink attribute. */\n> static const struct ovs_len_tbl ovs_key_lens[OVS_KEY_ATTR_MAX + 1] = {\n> \t[OVS_KEY_ATTR_ENCAP]\t = { .len = OVS_ATTR_NESTED },\n> @@ -405,6 +432,8 @@ static const struct ovs_len_tbl ovs_key_lens[OVS_KEY_ATTR_MAX + 1] = {\n> \t\t.len = sizeof(struct ovs_key_ct_tuple_ipv4) },\n> \t[OVS_KEY_ATTR_CT_ORIG_TUPLE_IPV6] = {\n> \t\t.len = sizeof(struct ovs_key_ct_tuple_ipv6) },\n> +\t[OVS_KEY_ATTR_NSH] = { .len = OVS_ATTR_NESTED,\n> +\t\t\t\t .next = ovs_nsh_key_attr_lens, },\n> };\n> \n> static bool check_attr_len(unsigned int attr_len, unsigned int expected_len)\n> @@ -1179,6 +1208,304 @@ static int metadata_from_nlattrs(struct net *net, struct sw_flow_match *match,\n> \treturn 0;\n> }\n> \n> +int nsh_hdr_from_nlattr(const struct nlattr *attr,\n> +\t\t\tstruct nshhdr *nsh, size_t size)\n> +{\n> +\tstruct nlattr *a;\n> +\tint rem;\n> +\tu8 flags = 0;\n> +\tu8 ttl = 0;\n> +\tint mdlen = 0;\n> +\tbool has_md1 = false;\n> +\tbool has_md2 = false;\n> +\tu8 len;\n> +\n> +\tnla_for_each_nested(a, attr, rem) {\n> +\t\tint type = nla_type(a);\n> +\n> +\t\tif (type > OVS_NSH_KEY_ATTR_MAX) {\n> +\t\t\tOVS_NLERR(1, \"nsh attr %d is out of range max %d\",\n> +\t\t\t\t type, OVS_NSH_KEY_ATTR_MAX);\n> +\t\t\treturn -EINVAL;\n> +\t\t}\n> +\n> +\t\tif (!check_attr_len(nla_len(a),\n> +\t\t\t\t ovs_nsh_key_attr_lens[type].len)) {\n> +\t\t\tOVS_NLERR(\n> +\t\t\t 1,\n> +\t\t\t \"nsh attr %d has unexpected len %d expected %d\",\n> +\t\t\t type,\n> +\t\t\t nla_len(a),\n> +\t\t\t ovs_nsh_key_attr_lens[type].len\n> +\t\t\t);\n> +\t\t\treturn -EINVAL;\n> +\t\t}\n\nThese checks should be done only once when the action is configured, not for\neach packet.\n\n> +\n> +\t\tswitch (type) {\n> +\t\tcase OVS_NSH_KEY_ATTR_BASE: {\n> +\t\t\tconst struct ovs_nsh_key_base *base =\n> +\t\t\t\t(struct ovs_nsh_key_base *)nla_data(a);\n> +\t\t\tflags = base->flags;\n> +\t\t\tttl = base->ttl;\n> +\t\t\tnsh->np = base->np;\n> +\t\t\tnsh->mdtype = base->mdtype;\n> +\t\t\tnsh->path_hdr = base->path_hdr;\n> +\t\t\tbreak;\n> +\t\t}\n> +\t\tcase OVS_NSH_KEY_ATTR_MD1: {\n> +\t\t\tconst struct ovs_nsh_key_md1 *md1 =\n> +\t\t\t\t(struct ovs_nsh_key_md1 *)nla_data(a);\n> +\t\t\tstruct nsh_md1_ctx *md1_dst = &nsh->md1;\n> +\n> +\t\t\thas_md1 = true;\n> +\t\t\tmdlen = nla_len(a);\n> +\t\t\tif (((mdlen + NSH_BASE_HDR_LEN) != NSH_M_TYPE1_LEN) ||\n> +\t\t\t ((mdlen + NSH_BASE_HDR_LEN) > size) ||\n> +\t\t\t (mdlen <= 0)) {\n> +\t\t\t\tOVS_NLERR(\n> +\t\t\t\t 1,\n> +\t\t\t\t \"length %d of nsh attr %d is invalid\",\n> +\t\t\t\t mdlen,\n> +\t\t\t\t type\n> +\t\t\t\t);\n> +\t\t\t\treturn -EINVAL;\n> +\t\t\t}\n\nDitto for most parts of the check. Plus the maximum size should be validated\nelsewhere. In this function, there should be a WARN_ON_ONCE if the computed\nlength > size.\n\n> +\t\t\tmemcpy(md1_dst, md1, mdlen);\n> +\t\t\tbreak;\n> +\t\t}\n> +\t\tcase OVS_NSH_KEY_ATTR_MD2: {\n> +\t\t\tconst struct u8 *md2 = nla_data(a);\n> +\t\t\tstruct nsh_md2_tlv *md2_dst = &nsh->md2;\n> +\n> +\t\t\thas_md2 = true;\n> +\t\t\tmdlen = nla_len(a);\n> +\t\t\tif (((mdlen + NSH_BASE_HDR_LEN) > size) ||\n> +\t\t\t (mdlen <= 0)) {\n> +\t\t\t\tOVS_NLERR(\n> +\t\t\t\t 1,\n> +\t\t\t\t \"length %d of nsh attr %d is invalid\",\n> +\t\t\t\t mdlen,\n> +\t\t\t\t type\n> +\t\t\t\t);\n> +\t\t\t\treturn -EINVAL;\n> +\t\t\t}\n\nDitto.\n\n> +\t\t\tmemcpy(md2_dst, md2, mdlen);\n> +\t\t\tbreak;\n> +\t\t}\n> +\t\tdefault:\n> +\t\t\tOVS_NLERR(1, \"Unknown nsh attribute %d\",\n> +\t\t\t\t type);\n> +\t\t\treturn -EINVAL;\n\nDitto.\n\n> +\t\t}\n> +\t}\n> +\n> +\tif (rem > 0) {\n> +\t\tOVS_NLERR(1, \"nsh attribute has %d unknown bytes.\", rem);\n> +\t\treturn -EINVAL;\n> +\t}\n> +\n> +\tif ((has_md1 && nsh->mdtype != NSH_M_TYPE1) ||\n> +\t (has_md2 && nsh->mdtype != NSH_M_TYPE2)) {\n> +\t\tOVS_NLERR(1,\n> +\t\t\t \"nsh attribute has unmatched MD type %d.\",\n> +\t\t\t nsh->mdtype);\n> +\t\treturn -EINVAL;\n> +\t}\n> +\n> +\tif (unlikely(has_md1 && has_md2)) {\n> +\t\tOVS_NLERR(1, \"both nsh md1 and md2 attribute are there\");\n> +\t\treturn -EINVAL;\n> +\t}\n> +\n> +\tif (unlikely(!has_md1 && !has_md2)) {\n> +\t\tOVS_NLERR(1, \"neither nsh md1 nor md2 attribute is there\");\n> +\t\treturn -EINVAL;\n> +\t}\n\nDitto. Plus I don't see a check that the OVS_NSH_KEY_ATTR_BASE attribute is\npresent. Seems that the compiler warned you about possibly unitialized flags\nand ttl variables and you just silenced the warning without considering\nwhether it's not actually justified.\n\n> +\n> +\t/* nsh header length = NSH_BASE_HDR_LEN + mdlen */\n> +\tnsh->ver_flags_ttl_len = 0;\n> +\tlen = NSH_BASE_HDR_LEN + mdlen;\n\nIn the whole function you open code NSH_BASE_HDR_LEN + mdlen and suddenly,\nat the very end, you store it to a variable to be used just once? Call me\nconfused.\n\n> +\tnsh_set_flags_ttl_len(nsh, flags, ttl, len);\n> +\n> +\treturn 0;\n> +}\n\nSo, this whole function does a lot of processing and copying. Why don't we\njust parse the attributes once, cache the resulting header and just memcpy\nit in the hot path?\n\n> +\n> +int nsh_key_from_nlattr(const struct nlattr *attr,\n> +\t\t\tstruct ovs_key_nsh *nsh)\n\nSee above plus what I wrote for the previous function, it applies here as\nwell.\n\nI stop here. I suspect there are similar things in the rest of the patch.\nPlease think about what I wrote and apply it to all similar situations. Not\nas you do with nsh_push where you apparenly ignored nsh_pop just because\nI mentioned nsh_push only.\n\n Jiri","headers":{"Return-Path":"","X-Original-To":"patchwork-incoming@ozlabs.org","Delivered-To":"patchwork-incoming@ozlabs.org","Authentication-Results":["ozlabs.org;\n\tspf=none (mailfrom) smtp.mailfrom=vger.kernel.org\n\t(client-ip=209.132.180.67; helo=vger.kernel.org;\n\tenvelope-from=netdev-owner@vger.kernel.org;\n\treceiver=)","ext-mx07.extmail.prod.ext.phx2.redhat.com;\n\tdmarc=none (p=none dis=none) header.from=redhat.com","ext-mx07.extmail.prod.ext.phx2.redhat.com;\n\tspf=fail smtp.mailfrom=jbenc@redhat.com"],"Received":["from vger.kernel.org (vger.kernel.org [209.132.180.67])\n\tby ozlabs.org (Postfix) with ESMTP id 3xjfCn0Wb4z9s7F\n\tfor ;\n\tThu, 31 Aug 2017 20:45:25 +1000 (AEST)","(majordomo@vger.kernel.org) by vger.kernel.org via listexpand\n\tid S1750918AbdHaKpW (ORCPT );\n\tThu, 31 Aug 2017 06:45:22 -0400","from mx1.redhat.com ([209.132.183.28]:38094 \"EHLO mx1.redhat.com\"\n\trhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP\n\tid S1750757AbdHaKpV (ORCPT );\n\tThu, 31 Aug 2017 06:45:21 -0400","from smtp.corp.redhat.com\n\t(int-mx06.intmail.prod.int.phx2.redhat.com [10.5.11.16])\n\t(using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits))\n\t(No client certificate requested)\n\tby mx1.redhat.com (Postfix) with ESMTPS id A15AFC047B72;\n\tThu, 31 Aug 2017 10:45:20 +0000 (UTC)","from griffin (ovpn-116-172.ams2.redhat.com [10.36.116.172])\n\tby smtp.corp.redhat.com (Postfix) with ESMTPS id 846079ECEF;\n\tThu, 31 Aug 2017 10:45:18 +0000 (UTC)"],"DMARC-Filter":"OpenDMARC Filter v1.3.2 mx1.redhat.com A15AFC047B72","Date":"Thu, 31 Aug 2017 12:45:16 +0200","From":"Jiri Benc ","To":"Yi Yang ","Cc":"netdev@vger.kernel.org, davem@davemloft.net, dev@openvswitch.org,\n\te@erig.me, blp@ovn.org, jan.scheurich@ericsson.com","Subject":"Re: [PATCH net-next v7] openvswitch: enable NSH support","Message-ID":"<20170831124516.0c5db686@griffin>","In-Reply-To":"<1504096752-102003-1-git-send-email-yi.y.yang@intel.com>","References":"<1504096752-102003-1-git-send-email-yi.y.yang@intel.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=US-ASCII","Content-Transfer-Encoding":"7bit","X-Scanned-By":"MIMEDefang 2.79 on 10.5.11.16","X-Greylist":"Sender IP whitelisted, not delayed by milter-greylist-4.5.16\n\t(mx1.redhat.com [10.5.110.31]);\n\tThu, 31 Aug 2017 10:45:21 +0000 (UTC)","Sender":"netdev-owner@vger.kernel.org","Precedence":"bulk","List-ID":"","X-Mailing-List":"netdev@vger.kernel.org"}},{"id":1762512,"web_url":"http://patchwork.ozlabs.org/comment/1762512/","msgid":"<20170904080005.GA70767@cran64.bj.intel.com>","list_archive_url":null,"date":"2017-09-04T08:00:05","subject":"Re: [PATCH net-next v7] openvswitch: enable NSH support","submitter":{"id":68962,"url":"http://patchwork.ozlabs.org/api/people/68962/","name":"Yang, Yi","email":"yi.y.yang@intel.com"},"content":"On Thu, Aug 31, 2017 at 06:45:16PM +0800, Jiri Benc wrote:\n> > +\t\t\t\t mask->context[i]);\n> > +\t\t}\n> > +\t\tmemcpy(flow_key->nsh.context, nsh->md1.context,\n> > +\t\t sizeof(nsh->md1.context));\n> \n> Do you follow the discussion that Hannes Sowa started on the ovs list\n> regarding matching on the context fields? It would be better to hold on this\n> until there's a conclusion reached.\n>\n\nI think we have had similiar discussion about this, the issue is we have\nno way to handle both MD type 1 and MD type 2 by using a common flow key struct.\n\nSo we have to do so, there is miniflow to handle such issue in\nuserspace, but kernel data path didn't provide such mechanism. I think\nevery newly-added key will result in the same space-wasting issue for\nkernel data path, isn't it?\n\n> > +\t\terr = nsh_key_from_nlattr(&attr.nla, &nsh);\n> > +\t\tif (err)\n> > +\t\t\tbreak;\n> > +\t\terr = nsh_key_from_nlattr(&mask.nla, &nsh_mask);\n> > +\t\tif (err)\n> > +\t\t\tbreak;\n> \n> This is a lot of copying to just be able to use nla_for_each_nested. While\n> I'm not a fan of how ovs uses the attributes to store both value and mask,\n> it's not completely irrational. However, I don't think that the intent was\n> to store a copy of the whole nested group. It's quite obvious that it does\n> not fit the pattern from the gymnastics you need to do.\n> \n> Instead, store the mask in each of the nested attributes individually. There\n> will be no need for the copying above and the code will get cleaner and\n> faster. It's not that the nsh_key_from_nlattr function needs to be able to\n> work separately for the key and mask. Nothing else than the line above uses\n> this function. Just move the logic inside.\n> \n> Actually, it seems it can be moved directly to set_nsh.\n\nOVS_ATTR_KEY_NSH is the first case to use nested attributes for set\naction, so no reference code for this, set action has two use cases, one\nhas mask but the other hasn't, so it will be easier an nested attribute is\nhandled as a whole, in user space, nsh_key_from_nlattr is used in\nseveral places. If we change it per your proposal, there will be a lot\nof rework, we have to provide two functions to handle this, one is for\nthe case with mask, the other is for the case without mask.\n\n> \n> > +\t\terr = set_nsh(skb, flow_key, &nsh, &nsh_mask);\n> > +\t\tbreak;\n> > +\t}\n> > +\n> > \tcase OVS_KEY_ATTR_IPV4:\n> > \t\terr = set_ipv4(skb, flow_key, nla_data(a),\n> > \t\t\t get_mask(a, struct ovs_key_ipv4 *));\n> > @@ -1210,6 +1336,21 @@ static int do_execute_actions(struct datapath *dp, struct sk_buff *skb,\n> > \t\tcase OVS_ACTION_ATTR_POP_ETH:\n> > \t\t\terr = pop_eth(skb, key);\n> > \t\t\tbreak;\n> > +\n> > +\t\tcase OVS_ACTION_ATTR_PUSH_NSH: {\n> > +\t\t\tu8 buffer[NSH_HDR_MAX_LEN];\n> > +\t\t\tstruct nshhdr *nsh_hdr = (struct nshhdr *)buffer;\n> > +\t\t\tconst struct nshhdr *nsh_src = nsh_hdr;\n> > +\n> > +\t\t\tnsh_hdr_from_nlattr(nla_data(a), nsh_hdr,\n> > +\t\t\t\t\t NSH_HDR_MAX_LEN);\n> \n> This is costly. This is called for every packet in the fast path. We should\n> precompute and store the header instead.\n\nHow can we do so? I see batch process code in user space implementation,\nbut kernel data path didn't such infrastructure, how can we know next\npush_nsh uses the same nsh header as previous one?\n\n> > +\tcase NSH_M_TYPE2:\n> > +\t\t/* Don't support MD type 2 metedata parsing yet */\n> > +\t\tif (length < NSH_BASE_HDR_LEN)\n> > +\t\t\treturn -EINVAL;\n> \n> Shouldn't we reject the packet, then? Pretending that something was parsed\n> correctly while in fact it was not, does not look correct.\n\nFor MD type 2, we don't implement metadata parsing, but it can still\nwork. I'm not sure what you mean here, how do we reject a packet here?\n\n> \n> These checks should be done only once when the action is configured, not for\n> each packet.\n\nI don't know how to implement a batch processing in kernel data path, it\nseems there isn't such code for reference.","headers":{"Return-Path":"","X-Original-To":"patchwork-incoming@ozlabs.org","Delivered-To":"patchwork-incoming@ozlabs.org","Authentication-Results":"ozlabs.org;\n\tspf=none (mailfrom) smtp.mailfrom=vger.kernel.org\n\t(client-ip=209.132.180.67; helo=vger.kernel.org;\n\tenvelope-from=netdev-owner@vger.kernel.org;\n\treceiver=)","Received":["from vger.kernel.org (vger.kernel.org [209.132.180.67])\n\tby ozlabs.org (Postfix) with ESMTP id 3xm2hb4TQgz9s7h\n\tfor ;\n\tMon, 4 Sep 2017 18:15:11 +1000 (AEST)","(majordomo@vger.kernel.org) by vger.kernel.org via listexpand\n\tid S1753410AbdIDIPH (ORCPT );\n\tMon, 4 Sep 2017 04:15:07 -0400","from mga09.intel.com ([134.134.136.24]:56928 \"EHLO mga09.intel.com\"\n\trhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP\n\tid S1753329AbdIDIPE (ORCPT );\n\tMon, 4 Sep 2017 04:15:04 -0400","from orsmga001.jf.intel.com ([10.7.209.18])\n\tby orsmga102.jf.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384;\n\t04 Sep 2017 01:15:03 -0700","from unknown (HELO cran64.bj.intel.com) ([10.240.224.181])\n\tby orsmga001.jf.intel.com with ESMTP; 04 Sep 2017 01:15:01 -0700"],"X-ExtLoop1":"1","X-IronPort-AV":"E=Sophos;i=\"5.41,473,1498546800\"; d=\"scan'208\";a=\"1168892709\"","Date":"Mon, 4 Sep 2017 16:00:05 +0800","From":"\"Yang, Yi\" ","To":"Jiri Benc ","Cc":"\"netdev@vger.kernel.org\" ,\n\t\"davem@davemloft.net\" ,\n\t\"dev@openvswitch.org\" ,\n\t\"e@erig.me\" , \"blp@ovn.org\" ,\n\t\"jan.scheurich@ericsson.com\" ","Subject":"Re: [PATCH net-next v7] openvswitch: enable NSH support","Message-ID":"<20170904080005.GA70767@cran64.bj.intel.com>","References":"<1504096752-102003-1-git-send-email-yi.y.yang@intel.com>\n\t<20170831124516.0c5db686@griffin>","MIME-Version":"1.0","Content-Type":"text/plain; charset=us-ascii","Content-Disposition":"inline","In-Reply-To":"<20170831124516.0c5db686@griffin>","User-Agent":"Mutt/1.5.23 (2014-03-12)","Sender":"netdev-owner@vger.kernel.org","Precedence":"bulk","List-ID":"","X-Mailing-List":"netdev@vger.kernel.org"}},{"id":1762565,"web_url":"http://patchwork.ozlabs.org/comment/1762565/","msgid":"<20170904124216.6db68e8c@griffin>","list_archive_url":null,"date":"2017-09-04T10:42:16","subject":"Re: [PATCH net-next v7] openvswitch: enable NSH support","submitter":{"id":9287,"url":"http://patchwork.ozlabs.org/api/people/9287/","name":"Jiri Benc","email":"jbenc@redhat.com"},"content":"On Mon, 4 Sep 2017 16:00:05 +0800, Yang, Yi wrote:\n> I think we have had similiar discussion about this, the issue is we have\n> no way to handle both MD type 1 and MD type 2 by using a common flow key struct.\n> \n> So we have to do so, there is miniflow to handle such issue in\n> userspace, but kernel data path didn't provide such mechanism. I think\n> every newly-added key will result in the same space-wasting issue for\n> kernel data path, isn't it?\n\nYes. And it would be surprising if it didn't have an effect on\nperformance.\n\nI don't care that much as this is a result of openvswitch module design\ndecisions but it still would be good to reach a consensus before\nimplementing uAPI that might not be needed in the end.\n\n> OVS_ATTR_KEY_NSH is the first case to use nested attributes for set\n> action, so no reference code for this, set action has two use cases, one\n> has mask but the other hasn't, so it will be easier an nested attribute is\n> handled as a whole, in user space, nsh_key_from_nlattr is used in\n> several places.\n\nI very much disagree. What you're doing is very poor design as can be\nseen from the code where you have to do weird gymnastics to implement\nthat. Compare that to a much simple case where each attribute carries\nits own value and mask.\n\n> If we change it per your proposal, there will be a lot\n> of rework,\n\nThat's not an argument. We care about doing things right, we don't do\nthings hastily and with as little effort as possible.\n\n> we have to provide two functions to handle this, one is for\n> the case with mask, the other is for the case without mask.\n\nI don't see that. The function is called from a single place only.\n\n> How can we do so? I see batch process code in user space implementation,\n> but kernel data path didn't such infrastructure,\n\nYou're right that there's no infrastructure. I somewhat agree that it\nmight be out of scope of this patch and it can be optimized later.\nThat's why I also included other suggestions to speed this up until we\nimplement better parsing: namely, verify correctness once (at the time\nit is set from user space) and just expect things to be correct in the\nfast path.\n\n> how can we know next push_nsh uses the same nsh header as previous\n> one?\n\nWe store the prepopulated header together with the action.\n\n> > Shouldn't we reject the packet, then? Pretending that something was parsed\n> > correctly while in fact it was not, does not look correct.\n> \n> For MD type 2, we don't implement metadata parsing, but it can still\n> work. I'm not sure what you mean here, how do we reject a packet here?\n\nOkay, we can match on mdtype and thus can find out about this type of\npackets. No problem, then.\n\n> > These checks should be done only once when the action is configured, not for\n> > each packet.\n> \n> I don't know how to implement a batch processing in kernel data path, it\n> seems there isn't such code for reference.\n\nThe checks should be done somewhere in __ovs_nla_copy_action. Which\nseems to be done even in your patch by nsh_key_put_from_nlattr\n(I didn't get that far during the review last week. The names of\nthose nsh_(key|hdr)_*_nlattr are so similar that it's impossible to\ntell what they do without looking at the call sites - something you\nshould also consider to improve). That makes it even more wrong: you\nappear to check everything twice, first on configuration and then for\nevery packet.\n\n Jiri","headers":{"Return-Path":"","X-Original-To":"patchwork-incoming@ozlabs.org","Delivered-To":"patchwork-incoming@ozlabs.org","Authentication-Results":["ozlabs.org;\n\tspf=none (mailfrom) smtp.mailfrom=vger.kernel.org\n\t(client-ip=209.132.180.67; helo=vger.kernel.org;\n\tenvelope-from=netdev-owner@vger.kernel.org;\n\treceiver=)","ext-mx08.extmail.prod.ext.phx2.redhat.com;\n\tdmarc=none (p=none dis=none) header.from=redhat.com","ext-mx08.extmail.prod.ext.phx2.redhat.com;\n\tspf=fail smtp.mailfrom=jbenc@redhat.com"],"Received":["from vger.kernel.org (vger.kernel.org [209.132.180.67])\n\tby ozlabs.org (Postfix) with ESMTP id 3xm5yW1ncGz9s8J\n\tfor ;\n\tMon, 4 Sep 2017 20:42:27 +1000 (AEST)","(majordomo@vger.kernel.org) by vger.kernel.org via listexpand\n\tid S1753420AbdIDKmW (ORCPT );\n\tMon, 4 Sep 2017 06:42:22 -0400","from mx1.redhat.com ([209.132.183.28]:51192 \"EHLO mx1.redhat.com\"\n\trhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP\n\tid S1751368AbdIDKmV (ORCPT );\n\tMon, 4 Sep 2017 06:42:21 -0400","from smtp.corp.redhat.com\n\t(int-mx02.intmail.prod.int.phx2.redhat.com [10.5.11.12])\n\t(using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits))\n\t(No client certificate requested)\n\tby mx1.redhat.com (Postfix) with ESMTPS id 0A953C058EA2;\n\tMon, 4 Sep 2017 10:42:21 +0000 (UTC)","from griffin (ovpn-116-46.ams2.redhat.com [10.36.116.46])\n\tby smtp.corp.redhat.com (Postfix) with ESMTPS id AB616822F3;\n\tMon, 4 Sep 2017 10:42:18 +0000 (UTC)"],"DMARC-Filter":"OpenDMARC Filter v1.3.2 mx1.redhat.com 0A953C058EA2","Date":"Mon, 4 Sep 2017 12:42:16 +0200","From":"Jiri Benc ","To":"\"Yang, Yi\" ","Cc":"\"netdev@vger.kernel.org\" ,\n\t\"davem@davemloft.net\" ,\n\t\"dev@openvswitch.org\" ,\n\t\"e@erig.me\" , \"blp@ovn.org\" ,\n\t\"jan.scheurich@ericsson.com\" ","Subject":"Re: [PATCH net-next v7] openvswitch: enable NSH support","Message-ID":"<20170904124216.6db68e8c@griffin>","In-Reply-To":"<20170904080005.GA70767@cran64.bj.intel.com>","References":"<1504096752-102003-1-git-send-email-yi.y.yang@intel.com>\n\t<20170831124516.0c5db686@griffin>\n\t<20170904080005.GA70767@cran64.bj.intel.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=US-ASCII","Content-Transfer-Encoding":"7bit","X-Scanned-By":"MIMEDefang 2.79 on 10.5.11.12","X-Greylist":"Sender IP whitelisted, not delayed by milter-greylist-4.5.16\n\t(mx1.redhat.com [10.5.110.32]);\n\tMon, 04 Sep 2017 10:42:21 +0000 (UTC)","Sender":"netdev-owner@vger.kernel.org","Precedence":"bulk","List-ID":"","X-Mailing-List":"netdev@vger.kernel.org"}},{"id":1762598,"web_url":"http://patchwork.ozlabs.org/comment/1762598/","msgid":"<20170904120907.GA75461@cran64.bj.intel.com>","list_archive_url":null,"date":"2017-09-04T12:09:07","subject":"Re: [PATCH net-next v7] openvswitch: enable NSH support","submitter":{"id":68962,"url":"http://patchwork.ozlabs.org/api/people/68962/","name":"Yang, Yi","email":"yi.y.yang@intel.com"},"content":"On Mon, Sep 04, 2017 at 12:42:16PM +0200, Jiri Benc wrote:\n> On Mon, 4 Sep 2017 16:00:05 +0800, Yang, Yi wrote:\n> > I think we have had similiar discussion about this, the issue is we have\n> > no way to handle both MD type 1 and MD type 2 by using a common flow key struct.\n> > \n> > So we have to do so, there is miniflow to handle such issue in\n> > userspace, but kernel data path didn't provide such mechanism. I think\n> > every newly-added key will result in the same space-wasting issue for\n> > kernel data path, isn't it?\n> \n> Yes. And it would be surprising if it didn't have an effect on\n> performance.\n> \n> I don't care that much as this is a result of openvswitch module design\n> decisions but it still would be good to reach a consensus before\n> implementing uAPI that might not be needed in the end.\n> \n> > OVS_ATTR_KEY_NSH is the first case to use nested attributes for set\n> > action, so no reference code for this, set action has two use cases, one\n> > has mask but the other hasn't, so it will be easier an nested attribute is\n> > handled as a whole, in user space, nsh_key_from_nlattr is used in\n> > several places.\n> \n> I very much disagree. What you're doing is very poor design as can be\n> seen from the code where you have to do weird gymnastics to implement\n> that. Compare that to a much simple case where each attribute carries\n> its own value and mask.\n> \n> > If we change it per your proposal, there will be a lot\n> > of rework,\n> \n> That's not an argument. We care about doing things right, we don't do\n> things hastily and with as little effort as possible.\n\nJiri, I check OVS source code carefully, if you check OVS code in\nformat_odp_action in lib/odp-util.c, it has a strong assumption for set\naction, mask is following value no matter it is simple attribute or\nnested attribute, I copy source code here for your reference,\n\n case OVS_ACTION_ATTR_SET_MASKED:\n a = nl_attr_get(a);\n size = nl_attr_get_size(a) / 2;\n ds_put_cstr(ds, \"set(\");\n\n /* Masked set action not supported for tunnel key, which is\n * bigger. */\n if (size <= sizeof(struct ovs_key_ipv6)) {\n struct nlattr attr[1 + DIV_ROUND_UP(sizeof(struct\novs_key_ipv6),\n sizeof(struct nlattr))];\n struct nlattr mask[1 + DIV_ROUND_UP(sizeof(struct\novs_key_ipv6),\n sizeof(struct nlattr))];\n\n mask->nla_type = attr->nla_type = nl_attr_type(a);\n mask->nla_len = attr->nla_len = NLA_HDRLEN + size;\n memcpy(attr + 1, (char *)(a + 1), size);\n memcpy(mask + 1, (char *)(a + 1) + size, size);\n format_odp_key_attr(attr, mask, NULL, ds, false);\n } else {\n format_odp_key_attr(a, NULL, NULL, ds, false);\n }\n ds_put_cstr(ds, \")\");\n break;\n\nSo we must do many changes if we want to break this assumption.\n\n> \n> > we have to provide two functions to handle this, one is for\n> > the case with mask, the other is for the case without mask.\n> \n> I don't see that. The function is called from a single place only.\n> \n> > How can we do so? I see batch process code in user space implementation,\n> > but kernel data path didn't such infrastructure,\n> \n> You're right that there's no infrastructure. I somewhat agree that it\n> might be out of scope of this patch and it can be optimized later.\n> That's why I also included other suggestions to speed this up until we\n> implement better parsing: namely, verify correctness once (at the time\n> it is set from user space) and just expect things to be correct in the\n> fast path.\n> \n> > how can we know next push_nsh uses the same nsh header as previous\n> > one?\n> \n> We store the prepopulated header together with the action.\n> \n> > > Shouldn't we reject the packet, then? Pretending that something was parsed\n> > > correctly while in fact it was not, does not look correct.\n> > \n> > For MD type 2, we don't implement metadata parsing, but it can still\n> > work. I'm not sure what you mean here, how do we reject a packet here?\n> \n> Okay, we can match on mdtype and thus can find out about this type of\n> packets. No problem, then.\n> \n> > > These checks should be done only once when the action is configured, not for\n> > > each packet.\n> > \n> > I don't know how to implement a batch processing in kernel data path, it\n> > seems there isn't such code for reference.\n> \n> The checks should be done somewhere in __ovs_nla_copy_action. Which\n> seems to be done even in your patch by nsh_key_put_from_nlattr\n> (I didn't get that far during the review last week. The names of\n> those nsh_(key|hdr)_*_nlattr are so similar that it's impossible to\n> tell what they do without looking at the call sites - something you\n> should also consider to improve). That makes it even more wrong: you\n> appear to check everything twice, first on configuration and then for\n> every packet.\n\nOk, I'll carefully remove unnecessary checks in next version.\n\n> \n> Jiri","headers":{"Return-Path":"","X-Original-To":"patchwork-incoming@ozlabs.org","Delivered-To":"patchwork-incoming@ozlabs.org","Authentication-Results":"ozlabs.org;\n\tspf=none (mailfrom) smtp.mailfrom=vger.kernel.org\n\t(client-ip=209.132.180.67; helo=vger.kernel.org;\n\tenvelope-from=netdev-owner@vger.kernel.org;\n\treceiver=)","Received":["from vger.kernel.org (vger.kernel.org [209.132.180.67])\n\tby ozlabs.org (Postfix) with ESMTP id 3xm8Cm0XzPz9t2M\n\tfor ;\n\tMon, 4 Sep 2017 22:24:03 +1000 (AEST)","(majordomo@vger.kernel.org) by vger.kernel.org via listexpand\n\tid S1753504AbdIDMYA (ORCPT );\n\tMon, 4 Sep 2017 08:24:00 -0400","from mga07.intel.com ([134.134.136.100]:54798 \"EHLO\n\tmga07.intel.com\"\n\trhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP\n\tid S1753477AbdIDMX7 (ORCPT );\n\tMon, 4 Sep 2017 08:23:59 -0400","from orsmga004.jf.intel.com ([10.7.209.38])\n\tby orsmga105.jf.intel.com with ESMTP; 04 Sep 2017 05:23:58 -0700","from unknown (HELO cran64.bj.intel.com) ([10.240.224.181])\n\tby orsmga004.jf.intel.com with ESMTP; 04 Sep 2017 05:23:57 -0700"],"X-ExtLoop1":"1","X-IronPort-AV":"E=Sophos;i=\"5.41,474,1498546800\"; d=\"scan'208\";a=\"125399904\"","Date":"Mon, 4 Sep 2017 20:09:07 +0800","From":"\"Yang, Yi\" ","To":"Jiri Benc ","Cc":"\"netdev@vger.kernel.org\" ,\n\t\"davem@davemloft.net\" ,\n\t\"dev@openvswitch.org\" ,\n\t\"e@erig.me\" , \"blp@ovn.org\" ,\n\t\"jan.scheurich@ericsson.com\" ","Subject":"Re: [PATCH net-next v7] openvswitch: enable NSH support","Message-ID":"<20170904120907.GA75461@cran64.bj.intel.com>","References":"<1504096752-102003-1-git-send-email-yi.y.yang@intel.com>\n\t<20170831124516.0c5db686@griffin>\n\t<20170904080005.GA70767@cran64.bj.intel.com>\n\t<20170904124216.6db68e8c@griffin>","MIME-Version":"1.0","Content-Type":"text/plain; charset=us-ascii","Content-Disposition":"inline","In-Reply-To":"<20170904124216.6db68e8c@griffin>","User-Agent":"Mutt/1.5.23 (2014-03-12)","Sender":"netdev-owner@vger.kernel.org","Precedence":"bulk","List-ID":"","X-Mailing-List":"netdev@vger.kernel.org"}},{"id":1762611,"web_url":"http://patchwork.ozlabs.org/comment/1762611/","msgid":"<20170904145744.4d8b7fd5@griffin>","list_archive_url":null,"date":"2017-09-04T12:57:44","subject":"Re: [PATCH net-next v7] openvswitch: enable NSH support","submitter":{"id":9287,"url":"http://patchwork.ozlabs.org/api/people/9287/","name":"Jiri Benc","email":"jbenc@redhat.com"},"content":"On Mon, 4 Sep 2017 20:09:07 +0800, Yang, Yi wrote:\n> So we must do many changes if we want to break this assumption.\n\nWe may do as many changes as we want to. This is uAPI we're talking\nabout and we need to get it right since the beginning. Sure, it may\nmean that some user space programs need some changes in order to make\nuse of the new features. That happens every day.\n\nI also don't understand where's the problem. It's very easy to check\nfor NLA_F_NESTED and generically act based on that in the function you\nquoted. Just call a different function than format_odp_key_attr to\nhandle ovs_nsh_key_attr attributes in case the nested flag is set and\nthe attribute is OVS_KEY_ATTR_NSH and you're done. You'll need such\nfunction anyway, it's not much different code size wise to call it from\nformat_odp_key_attr or from format_odp_action.\n\n Jiri","headers":{"Return-Path":"","X-Original-To":"patchwork-incoming@ozlabs.org","Delivered-To":"patchwork-incoming@ozlabs.org","Authentication-Results":["ozlabs.org;\n\tspf=none (mailfrom) smtp.mailfrom=vger.kernel.org\n\t(client-ip=209.132.180.67; helo=vger.kernel.org;\n\tenvelope-from=netdev-owner@vger.kernel.org;\n\treceiver=)","ext-mx03.extmail.prod.ext.phx2.redhat.com;\n\tdmarc=none (p=none dis=none) header.from=redhat.com","ext-mx03.extmail.prod.ext.phx2.redhat.com;\n\tspf=fail smtp.mailfrom=jbenc@redhat.com"],"Received":["from vger.kernel.org (vger.kernel.org [209.132.180.67])\n\tby ozlabs.org (Postfix) with ESMTP id 3xm8yv18HBz9sNr\n\tfor ;\n\tMon, 4 Sep 2017 22:57:59 +1000 (AEST)","(majordomo@vger.kernel.org) by vger.kernel.org via listexpand\n\tid S1753683AbdIDM54 (ORCPT );\n\tMon, 4 Sep 2017 08:57:56 -0400","from mx1.redhat.com ([209.132.183.28]:61481 \"EHLO mx1.redhat.com\"\n\trhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP\n\tid S1753543AbdIDM5z (ORCPT );\n\tMon, 4 Sep 2017 08:57:55 -0400","from smtp.corp.redhat.com\n\t(int-mx05.intmail.prod.int.phx2.redhat.com [10.5.11.15])\n\t(using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits))\n\t(No client certificate requested)\n\tby mx1.redhat.com (Postfix) with ESMTPS id 4A0A27E43C;\n\tMon, 4 Sep 2017 12:57:50 +0000 (UTC)","from griffin (ovpn-116-46.ams2.redhat.com [10.36.116.46])\n\tby smtp.corp.redhat.com (Postfix) with ESMTPS id 8CF84C0DB1;\n\tMon, 4 Sep 2017 12:57:47 +0000 (UTC)"],"DMARC-Filter":"OpenDMARC Filter v1.3.2 mx1.redhat.com 4A0A27E43C","Date":"Mon, 4 Sep 2017 14:57:44 +0200","From":"Jiri Benc ","To":"\"Yang, Yi\" ","Cc":"\"netdev@vger.kernel.org\" ,\n\t\"davem@davemloft.net\" ,\n\t\"dev@openvswitch.org\" ,\n\t\"e@erig.me\" , \"blp@ovn.org\" ,\n\t\"jan.scheurich@ericsson.com\" ","Subject":"Re: [PATCH net-next v7] openvswitch: enable NSH support","Message-ID":"<20170904145744.4d8b7fd5@griffin>","In-Reply-To":"<20170904120907.GA75461@cran64.bj.intel.com>","References":"<1504096752-102003-1-git-send-email-yi.y.yang@intel.com>\n\t<20170831124516.0c5db686@griffin>\n\t<20170904080005.GA70767@cran64.bj.intel.com>\n\t<20170904124216.6db68e8c@griffin>\n\t<20170904120907.GA75461@cran64.bj.intel.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=US-ASCII","Content-Transfer-Encoding":"7bit","X-Scanned-By":"MIMEDefang 2.79 on 10.5.11.15","X-Greylist":"Sender IP whitelisted, not delayed by milter-greylist-4.5.16\n\t(mx1.redhat.com [10.5.110.27]);\n\tMon, 04 Sep 2017 12:57:55 +0000 (UTC)","Sender":"netdev-owner@vger.kernel.org","Precedence":"bulk","List-ID":"","X-Mailing-List":"netdev@vger.kernel.org"}},{"id":1762613,"web_url":"http://patchwork.ozlabs.org/comment/1762613/","msgid":"","list_archive_url":null,"date":"2017-09-04T12:57:15","subject":"RE: [PATCH net-next v7] openvswitch: enable NSH support","submitter":{"id":68449,"url":"http://patchwork.ozlabs.org/api/people/68449/","name":"Jan Scheurich","email":"jan.scheurich@ericsson.com"},"content":"> On Mon, 4 Sep 2017 16:00:05 +0800, Yang, Yi wrote:\n> > I think we have had similiar discussion about this, the issue is we have\n> > no way to handle both MD type 1 and MD type 2 by using a common flow key struct.\n> >\n> > So we have to do so, there is miniflow to handle such issue in\n> > userspace, but kernel data path didn't provide such mechanism. I think\n> > every newly-added key will result in the same space-wasting issue for\n> > kernel data path, isn't it?\n> \n> Yes. And it would be surprising if it didn't have an effect on\n> performance.\n> \n> I don't care that much as this is a result of openvswitch module design\n> decisions but it still would be good to reach a consensus before\n> implementing uAPI that might not be needed in the end.\n\nMatching on NSH MD1 context headers is definitely required. So these must\nbe part of the flow.\n\n> \n> > OVS_ATTR_KEY_NSH is the first case to use nested attributes for set\n> > action, so no reference code for this, set action has two use cases, one\n> > has mask but the other hasn't, so it will be easier an nested attribute is\n> > handled as a whole, in user space, nsh_key_from_nlattr is used in\n> > several places.\n> \n> I very much disagree. What you're doing is very poor design as can be\n> seen from the code where you have to do weird gymnastics to implement\n> that. Compare that to a much simple case where each attribute carries\n> its own value and mask.\n\nI have no principle objections against splitting NSH base header and MD1 context\nheaders into independent key attributes on the uAPI if that simplifies the code.\nBut we need to full picture here:\n\nSo is what you are suggesting the following?\n\nFor matching:\nOVS_KEY_ATTR_NSH_BASE_HEADER\t\tmandatory\nOVS_KEY_ATTR_NSH_MD1_CONTEXT\t\toptional, in case MDTYPE == MD1\n\nFor setting:\nOVS_ACTION_ATTR_SET_MASKED\n- OVS_KEY_ATTR_NSH_BASE_HEADER\t\twhen setting any base header fields\nOVS_ACTION_ATTR_SET_MASKED\n- OVS_KEY_ATTR_NSH_MD1_CONTEXT\twhen setting any MD1 context data fields\n\nFor push_nsh:\nOVS_ACTION_ATTR_PUSH_NSH\n- OVS_KEY_ATTR_NSH_BASE_HEADER\t\tmandatory\n- OVS_NSH_ATTR_CONTEXT_HEADERS\t\toptional (opaque metadata octet stream) \n\nSo push_nsh would still require nested attributes. Is that what we want (see below).\n\n> > How can we do so? I see batch process code in user space implementation,\n> > but kernel data path didn't such infrastructure,\n> \n> You're right that there's no infrastructure. I somewhat agree that it\n> might be out of scope of this patch and it can be optimized later.\n> That's why I also included other suggestions to speed this up until we\n> implement better parsing: namely, verify correctness once (at the time\n> it is set from user space) and just expect things to be correct in the\n> fast path.\n> \n> > how can we know next push_nsh uses the same nsh header as previous\n> > one?\n> \n> We store the prepopulated header together with the action.\n\nI agree.\n\nIn our original modelling of OVS_ACTION_ATTR_PUSH_NSH in OVS 2.8 we \nintroduced a monolithic push_nsh action struct including the NSH base header\nand variable length opaque context headers. That avoids any logic in the \nkernel datapath to translate a nested action attribute into an internal format\nwith pre-populated header. The user space does that work.\n\nShould we consider to stick to that simple and efficient approach? As far \nAs I can see it will be generic for the foreseeable future.\n\n> > > These checks should be done only once when the action is configured, not for\n> > > each packet.\n> >\n> > I don't know how to implement a batch processing in kernel data path, it\n> > seems there isn't such code for reference.\n> \n> The checks should be done somewhere in __ovs_nla_copy_action. Which\n> seems to be done even in your patch by nsh_key_put_from_nlattr\n> (I didn't get that far during the review last week. The names of\n> those nsh_(key|hdr)_*_nlattr are so similar that it's impossible to\n> tell what they do without looking at the call sites - something you\n> should also consider to improve). That makes it even more wrong: you\n> appear to check everything twice, first on configuration and then for\n> every packet.\n\nAgree. Validation should only happen at action configuration time.\n\nBR, Jan","headers":{"Return-Path":"","X-Original-To":"patchwork-incoming@ozlabs.org","Delivered-To":"patchwork-incoming@ozlabs.org","Authentication-Results":"ozlabs.org;\n\tspf=none (mailfrom) smtp.mailfrom=vger.kernel.org\n\t(client-ip=209.132.180.67; helo=vger.kernel.org;\n\tenvelope-from=netdev-owner@vger.kernel.org;\n\treceiver=)","Received":["from vger.kernel.org (vger.kernel.org [209.132.180.67])\n\tby ozlabs.org (Postfix) with ESMTP id 3xm8zL3BQWz9sNr\n\tfor ;\n\tMon, 4 Sep 2017 22:58:22 +1000 (AEST)","(majordomo@vger.kernel.org) by vger.kernel.org via listexpand\n\tid S1753618AbdIDM6U convert rfc822-to-8bit (ORCPT\n\t);\n\tMon, 4 Sep 2017 08:58:20 -0400","from sessmg22.ericsson.net ([193.180.251.58]:62033 \"EHLO\n\tsessmg22.ericsson.net\" rhost-flags-OK-OK-OK-OK) by vger.kernel.org\n\twith ESMTP id S1753602AbdIDM6S (ORCPT\n\t); Mon, 4 Sep 2017 08:58:18 -0400","from ESESSHC009.ericsson.se (Unknown_Domain [153.88.183.45])\n\tby sessmg22.ericsson.net (Symantec Mail Security) with SMTP id\n\t04.EF.20899.9ED4DA95; Mon, 4 Sep 2017 14:58:17 +0200 (CEST)","from ESESSMB107.ericsson.se ([169.254.7.26]) by\n\tESESSHC009.ericsson.se ([153.88.183.45]) with mapi id 14.03.0352.000;\n\tMon, 4 Sep 2017 14:57:16 +0200"],"X-AuditID":"c1b4fb3a-617ff700000051a3-da-59ad4de94c40","From":"Jan Scheurich ","To":"Jiri Benc , \"Yang, Yi\" ","CC":"\"netdev@vger.kernel.org\" ,\n\t\"davem@davemloft.net\" ,\n\t\"dev@openvswitch.org\" ,\n\t\"e@erig.me\" , \"blp@ovn.org\" ","Subject":"RE: [PATCH net-next v7] openvswitch: enable NSH support","Thread-Topic":"[PATCH net-next v7] openvswitch: enable NSH support","Thread-Index":"AQHTIY2Lg9q8QO7fWEy80RgWemsrXKKeJ9QAgAYbLICAAC1QAIAAN2Qw","Date":"Mon, 4 Sep 2017 12:57:15 +0000","Message-ID":"","References":"<1504096752-102003-1-git-send-email-yi.y.yang@intel.com>\n\t<20170831124516.0c5db686@griffin>\n\t<20170904080005.GA70767@cran64.bj.intel.com>\n\t<20170904124216.6db68e8c@griffin>","In-Reply-To":"<20170904124216.6db68e8c@griffin>","Accept-Language":"en-US","Content-Language":"en-US","X-MS-Has-Attach":"","X-MS-TNEF-Correlator":"","x-originating-ip":"[153.88.183.20]","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"8BIT","MIME-Version":"1.0","X-Brightmail-Tracker":"H4sIAAAAAAAAA+NgFlrDIsWRmVeSWpSXmKPExsUyM2K7ru5L37WRBi0PxC1eTW5gtJhzvoXF\n\t4ujpPcwWi3tOMFms2HSa2eLYAjGL31+3MTmwe2xZeZPJ48iig4wei/e8ZPJ4dvM/o8fzaz0s\n\tHu/3XWXz+LxJLoA9issmJTUnsyy1SN8ugSvj279+5oJJShV73k1mb2CcIN3FyMkhIWAicWdO\n\tF1MXIxeHkMARRonnMyazQziLGCUOTzrB0sXIwcEmYCAxe7cDSIOIgJPE4rO/WUBqmAVOMkps\n\tapzJBpIQFnCQ6Du9jQWiyFGi+d8CRpBeEQE3iZWbHEHCLAIqEkcOzmEDCfMK+Ep83coKsWo3\n\to8Tio+1MIDWcAnoSc67+ZAWxGQXEJL6fWgMWZxYQl7j1ZD4TxNECEkv2nGeGsEUlXj7+xwph\n\tK0pcnb4cql5HYsHuT2wQtrbEsoWvwep5BQQlTs58wjKBUXQWkrGzkLTMQtIyC0nLAkaWVYyi\n\txanFxbnpRkZ6qUWZycXF+Xl6eaklmxiBcXhwy2+rHYwHnzseYhTgYFTi4T1rvTZSiDWxrLgy\n\t9xCjBAezkghvtBdQiDclsbIqtSg/vqg0J7X4EKM0B4uSOK/DvgsRQgLpiSWp2ampBalFMFkm\n\tDk6pBka7nEdT8hnFF9lyepU4zC/8nbgqsUOEaUPdkR6OCd+7vftqpVd+CKiYXbxgtVBBA7v1\n\tMxN2bp9fc6IdWzdEct9QeyTItTeZSdNWUOffzRM7mcQLnaetP3/IYR5L5b99hlOb041qjv4q\n\tbNpyz/Pa1qaOd6zH+1f08iyYoq1RundlU9qUKTuYnJRYijMSDbWYi4oTAaFDBMe/AgAA","Sender":"netdev-owner@vger.kernel.org","Precedence":"bulk","List-ID":"","X-Mailing-List":"netdev@vger.kernel.org"}},{"id":1762624,"web_url":"http://patchwork.ozlabs.org/comment/1762624/","msgid":"<20170904151024.707f614e@griffin>","list_archive_url":null,"date":"2017-09-04T13:10:24","subject":"Re: [PATCH net-next v7] openvswitch: enable NSH support","submitter":{"id":9287,"url":"http://patchwork.ozlabs.org/api/people/9287/","name":"Jiri Benc","email":"jbenc@redhat.com"},"content":"On Wed, 30 Aug 2017 20:39:12 +0800, Yi Yang wrote:\n> +enum ovs_nsh_key_attr {\n> +\tOVS_NSH_KEY_ATTR_BASE, /* struct ovs_nsh_key_base. */\n> +\tOVS_NSH_KEY_ATTR_MD1, /* struct ovs_nsh_key_md1. */\n> +\tOVS_NSH_KEY_ATTR_MD2, /* variable-length octets for MD type 2. */\n> +\t__OVS_NSH_KEY_ATTR_MAX\n> +};\n\nOne more thing: 0 is reserved, the first attr must be\nOVS_NSH_KEY_ATTR_UNSPEC.\n\n Jiri","headers":{"Return-Path":"","X-Original-To":"patchwork-incoming@ozlabs.org","Delivered-To":"patchwork-incoming@ozlabs.org","Authentication-Results":["ozlabs.org;\n\tspf=none (mailfrom) smtp.mailfrom=vger.kernel.org\n\t(client-ip=209.132.180.67; helo=vger.kernel.org;\n\tenvelope-from=netdev-owner@vger.kernel.org;\n\treceiver=)","ext-mx10.extmail.prod.ext.phx2.redhat.com;\n\tdmarc=none (p=none dis=none) header.from=redhat.com","ext-mx10.extmail.prod.ext.phx2.redhat.com;\n\tspf=fail smtp.mailfrom=jbenc@redhat.com"],"Received":["from vger.kernel.org (vger.kernel.org [209.132.180.67])\n\tby ozlabs.org (Postfix) with ESMTP id 3xm9FQ3nFXz9sR9\n\tfor ;\n\tMon, 4 Sep 2017 23:10:34 +1000 (AEST)","(majordomo@vger.kernel.org) by vger.kernel.org via listexpand\n\tid S1753566AbdIDNK2 (ORCPT );\n\tMon, 4 Sep 2017 09:10:28 -0400","from mx1.redhat.com ([209.132.183.28]:57140 \"EHLO mx1.redhat.com\"\n\trhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP\n\tid S1753504AbdIDNK1 (ORCPT );\n\tMon, 4 Sep 2017 09:10:27 -0400","from smtp.corp.redhat.com\n\t(int-mx02.intmail.prod.int.phx2.redhat.com [10.5.11.12])\n\t(using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits))\n\t(No client certificate requested)\n\tby mx1.redhat.com (Postfix) with ESMTPS id 65DDC61B9D;\n\tMon, 4 Sep 2017 13:10:27 +0000 (UTC)","from griffin (ovpn-116-46.ams2.redhat.com [10.36.116.46])\n\tby smtp.corp.redhat.com (Postfix) with ESMTPS id B3B7477DE1;\n\tMon, 4 Sep 2017 13:10:25 +0000 (UTC)"],"DMARC-Filter":"OpenDMARC Filter v1.3.2 mx1.redhat.com 65DDC61B9D","Date":"Mon, 4 Sep 2017 15:10:24 +0200","From":"Jiri Benc ","To":"Yi Yang ","Cc":"netdev@vger.kernel.org, davem@davemloft.net, dev@openvswitch.org,\n\te@erig.me, blp@ovn.org, jan.scheurich@ericsson.com","Subject":"Re: [PATCH net-next v7] openvswitch: enable NSH support","Message-ID":"<20170904151024.707f614e@griffin>","In-Reply-To":"<1504096752-102003-1-git-send-email-yi.y.yang@intel.com>","References":"<1504096752-102003-1-git-send-email-yi.y.yang@intel.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=US-ASCII","Content-Transfer-Encoding":"7bit","X-Scanned-By":"MIMEDefang 2.79 on 10.5.11.12","X-Greylist":"Sender IP whitelisted, not delayed by milter-greylist-4.5.16\n\t(mx1.redhat.com [10.5.110.39]);\n\tMon, 04 Sep 2017 13:10:27 +0000 (UTC)","Sender":"netdev-owner@vger.kernel.org","Precedence":"bulk","List-ID":"","X-Mailing-List":"netdev@vger.kernel.org"}},{"id":1762628,"web_url":"http://patchwork.ozlabs.org/comment/1762628/","msgid":"<20170904151601.2f198a53@griffin>","list_archive_url":null,"date":"2017-09-04T13:16:01","subject":"Re: [PATCH net-next v7] openvswitch: enable NSH support","submitter":{"id":9287,"url":"http://patchwork.ozlabs.org/api/people/9287/","name":"Jiri Benc","email":"jbenc@redhat.com"},"content":"On Mon, 4 Sep 2017 12:57:15 +0000, Jan Scheurich wrote:\n> So is what you are suggesting the following?\n> \n> For matching:\n> OVS_KEY_ATTR_NSH_BASE_HEADER\t\tmandatory\n> OVS_KEY_ATTR_NSH_MD1_CONTEXT\t\toptional, in case MDTYPE == MD1\n\nThis needs to be:\n\nOVS_KEY_ATTR_NSH\n\tOVS_KEY_ATTR_NSH_BASE_HEADER\n\tOVS_KEY_ATTR_NSH_MD1_CONTEXT\n\n> For setting:\n> OVS_ACTION_ATTR_SET_MASKED\n> - OVS_KEY_ATTR_NSH_BASE_HEADER\t\twhen setting any base header fields\n> OVS_ACTION_ATTR_SET_MASKED\n> - OVS_KEY_ATTR_NSH_MD1_CONTEXT\twhen setting any MD1 context data fields\n\nThis needs to be:\n\nOVS_ACTION_ATTR_SET_MASKED\n\tOVS_KEY_ATTR_NSH\n\t\tOVS_KEY_ATTR_NSH_BASE_HEADER\n\t\tOVS_KEY_ATTR_NSH_MD1_CONTEXT\n\nIn your example, there's no way to distinguish OVS_KEY_ATTR_ENCAP and\nOVS_KEY_ATTR_NSH_BASE_HEADER, they are both \"1\".\n\n> Should we consider to stick to that simple and efficient approach? As far \n> As I can see it will be generic for the foreseeable future.\n\nI'm not sure. From the kernel point of view, we want the same\nfunctionality offered by the openvswitch module and by a tc action.\n\nIn theory, it can be the tc tool that constructs the header from the\ncommand line but there's no precedent. Dunno.\n\n Jiri","headers":{"Return-Path":"","X-Original-To":"patchwork-incoming@ozlabs.org","Delivered-To":"patchwork-incoming@ozlabs.org","Authentication-Results":["ozlabs.org;\n\tspf=none (mailfrom) smtp.mailfrom=vger.kernel.org\n\t(client-ip=209.132.180.67; helo=vger.kernel.org;\n\tenvelope-from=netdev-owner@vger.kernel.org;\n\treceiver=)","ext-mx06.extmail.prod.ext.phx2.redhat.com;\n\tdmarc=none (p=none dis=none) header.from=redhat.com","ext-mx06.extmail.prod.ext.phx2.redhat.com;\n\tspf=fail smtp.mailfrom=jbenc@redhat.com"],"Received":["from vger.kernel.org (vger.kernel.org [209.132.180.67])\n\tby ozlabs.org (Postfix) with ESMTP id 3xm9Mt2fdMz9t2c\n\tfor ;\n\tMon, 4 Sep 2017 23:16:10 +1000 (AEST)","(majordomo@vger.kernel.org) by vger.kernel.org via listexpand\n\tid S1753591AbdIDNQG (ORCPT );\n\tMon, 4 Sep 2017 09:16:06 -0400","from mx1.redhat.com ([209.132.183.28]:45492 \"EHLO mx1.redhat.com\"\n\trhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP\n\tid S1753515AbdIDNQF (ORCPT );\n\tMon, 4 Sep 2017 09:16:05 -0400","from smtp.corp.redhat.com\n\t(int-mx03.intmail.prod.int.phx2.redhat.com [10.5.11.13])\n\t(using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits))\n\t(No client certificate requested)\n\tby mx1.redhat.com (Postfix) with ESMTPS id 77179267C0;\n\tMon, 4 Sep 2017 13:16:05 +0000 (UTC)","from griffin (ovpn-116-46.ams2.redhat.com [10.36.116.46])\n\tby smtp.corp.redhat.com (Postfix) with ESMTPS id D491E7F8DF;\n\tMon, 4 Sep 2017 13:16:03 +0000 (UTC)"],"DMARC-Filter":"OpenDMARC Filter v1.3.2 mx1.redhat.com 77179267C0","Date":"Mon, 4 Sep 2017 15:16:01 +0200","From":"Jiri Benc ","To":"Jan Scheurich ","Cc":"\"Yang, Yi\" ,\n\t\"netdev@vger.kernel.org\" ,\n\t\"davem@davemloft.net\" ,\n\t\"dev@openvswitch.org\" ,\n\t\"e@erig.me\" , \"blp@ovn.org\" ","Subject":"Re: [PATCH net-next v7] openvswitch: enable NSH support","Message-ID":"<20170904151601.2f198a53@griffin>","In-Reply-To":"","References":"<1504096752-102003-1-git-send-email-yi.y.yang@intel.com>\n\t<20170831124516.0c5db686@griffin>\n\t<20170904080005.GA70767@cran64.bj.intel.com>\n\t<20170904124216.6db68e8c@griffin>\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=US-ASCII","Content-Transfer-Encoding":"7bit","X-Scanned-By":"MIMEDefang 2.79 on 10.5.11.13","X-Greylist":"Sender IP whitelisted, not delayed by milter-greylist-4.5.16\n\t(mx1.redhat.com [10.5.110.30]);\n\tMon, 04 Sep 2017 13:16:05 +0000 (UTC)","Sender":"netdev-owner@vger.kernel.org","Precedence":"bulk","List-ID":"","X-Mailing-List":"netdev@vger.kernel.org"}},{"id":1762668,"web_url":"http://patchwork.ozlabs.org/comment/1762668/","msgid":"","list_archive_url":null,"date":"2017-09-04T14:07:45","subject":"RE: [PATCH net-next v7] openvswitch: enable NSH support","submitter":{"id":68449,"url":"http://patchwork.ozlabs.org/api/people/68449/","name":"Jan Scheurich","email":"jan.scheurich@ericsson.com"},"content":"> > So is what you are suggesting the following?\n> >\n> > For matching:\n> > OVS_KEY_ATTR_NSH_BASE_HEADER\t\tmandatory\n> > OVS_KEY_ATTR_NSH_MD1_CONTEXT\t\toptional, in case MDTYPE == MD1\n> \n> This needs to be:\n> \n> OVS_KEY_ATTR_NSH\n> \tOVS_KEY_ATTR_NSH_BASE_HEADER\n> \tOVS_KEY_ATTR_NSH_MD1_CONTEXT\n> \n> > For setting:\n> > OVS_ACTION_ATTR_SET_MASKED\n> > - OVS_KEY_ATTR_NSH_BASE_HEADER\t\twhen setting any base header fields\n> > OVS_ACTION_ATTR_SET_MASKED\n> > - OVS_KEY_ATTR_NSH_MD1_CONTEXT\twhen setting any MD1 context data fields\n> \n> This needs to be:\n> \n> OVS_ACTION_ATTR_SET_MASKED\n> \tOVS_KEY_ATTR_NSH\n> \t\tOVS_KEY_ATTR_NSH_BASE_HEADER\n> \t\tOVS_KEY_ATTR_NSH_MD1_CONTEXT\n> \n\nThen perhaps I misunderstood your comment. I thought you didn't like that the \nSET_MASKED action wrapped OVS_KEY_ATTR_NSH which in itself was nested.\nI was aiming to avoid this by lifting the two components of the NSH header \ncomponents to the top level:\nOVS_NSH_ATTR_BASE_HEADER --> OVS_KEY_ATTR_NSH_BASE_HEADER\nOVS_NSH_ATTR_MD1_CONTEXT --> OVS_KEY_ATTR_NSH_MD1_CONTEXT\n\n> In your example, there's no way to distinguish OVS_KEY_ATTR_ENCAP and\n> OVS_KEY_ATTR_NSH_BASE_HEADER, they are both \"1\".\n\nSee above. The two would be separate values in the same enum, i.e. distinct.\n\n> > Should we consider to stick to that simple and efficient approach? As far\n> > As I can see it will be generic for the foreseeable future.\n> \n> I'm not sure. From the kernel point of view, we want the same\n> functionality offered by the openvswitch module and by a tc action.\n> \n> In theory, it can be the tc tool that constructs the header from the\n> command line but there's no precedent. Dunno.\n\nNot sure I have the full picture here. Are you saying that the tc tool uses the same\nNetlink API to the kernel as OVS? What would be the back-end for tc? Also the\nopenvswitch kernel module? \n\nBR, Jan","headers":{"Return-Path":"","X-Original-To":"patchwork-incoming@ozlabs.org","Delivered-To":"patchwork-incoming@ozlabs.org","Authentication-Results":"ozlabs.org;\n\tspf=none (mailfrom) smtp.mailfrom=vger.kernel.org\n\t(client-ip=209.132.180.67; helo=vger.kernel.org;\n\tenvelope-from=netdev-owner@vger.kernel.org;\n\treceiver=)","Received":["from vger.kernel.org (vger.kernel.org [209.132.180.67])\n\tby ozlabs.org (Postfix) with ESMTP id 3xmBWW2WyQz9t2R\n\tfor ;\n\tTue, 5 Sep 2017 00:07:51 +1000 (AEST)","(majordomo@vger.kernel.org) by vger.kernel.org via listexpand\n\tid S1753885AbdIDOHs convert rfc822-to-8bit (ORCPT\n\t);\n\tMon, 4 Sep 2017 10:07:48 -0400","from sesbmg22.ericsson.net ([193.180.251.48]:59049 \"EHLO\n\tsesbmg22.ericsson.net\" rhost-flags-OK-OK-OK-OK) by vger.kernel.org\n\twith ESMTP id S1753866AbdIDOHr (ORCPT\n\t); Mon, 4 Sep 2017 10:07:47 -0400","from ESESSHC015.ericsson.se (Unknown_Domain [153.88.183.63])\n\tby sesbmg22.ericsson.net (Symantec Mail Security) with SMTP id\n\t77.74.22679.23E5DA95; Mon, 4 Sep 2017 16:07:46 +0200 (CEST)","from ESESSMB107.ericsson.se ([169.254.7.26]) by\n\tESESSHC015.ericsson.se ([153.88.183.63]) with mapi id 14.03.0352.000;\n\tMon, 4 Sep 2017 16:07:45 +0200"],"X-AuditID":"c1b4fb30-57fff70000005897-5d-59ad5e327858","From":"Jan Scheurich ","To":"Jiri Benc ","CC":"\"Yang, Yi\" ,\n\t\"netdev@vger.kernel.org\" ,\n\t\"davem@davemloft.net\" ,\n\t\"dev@openvswitch.org\" ,\n\t\"e@erig.me\" , \"blp@ovn.org\" ","Subject":"RE: [PATCH net-next v7] openvswitch: enable NSH support","Thread-Topic":"[PATCH net-next v7] openvswitch: enable NSH support","Thread-Index":"AQHTIY2Lg9q8QO7fWEy80RgWemsrXKKeJ9QAgAYbLICAAC1QAIAAN2Qw///zkoCAACvjgA==","Date":"Mon, 4 Sep 2017 14:07:45 +0000","Message-ID":"","References":"<1504096752-102003-1-git-send-email-yi.y.yang@intel.com>\n\t<20170831124516.0c5db686@griffin>\n\t<20170904080005.GA70767@cran64.bj.intel.com>\n\t<20170904124216.6db68e8c@griffin>\n\t\n\t<20170904151601.2f198a53@griffin>","In-Reply-To":"<20170904151601.2f198a53@griffin>","Accept-Language":"en-US","Content-Language":"en-US","X-MS-Has-Attach":"","X-MS-TNEF-Correlator":"","x-originating-ip":"[153.88.183.20]","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"8BIT","MIME-Version":"1.0","X-Brightmail-Tracker":"H4sIAAAAAAAAA+NgFlrLIsWRmVeSWpSXmKPExsUyM2K7va5R3NpIg79nZC1eTW5gtJhzvoXF\n\t4ujpPcwWi3tOMFms2HSa2eLYAjGL31+3MTmwe2xZeZPJ48iig4wei/e8ZPJ4dvM/o8fzaz0s\n\tHu/3XWXz+LxJLoA9issmJTUnsyy1SN8ugSuje/9/loI5/BXfHh1ibGBcxNPFyMEhIWAi8eNB\n\tTBcjF4eQwBFGibdT9rFAOIsYJdon/mIGKWITMJCYvduhi5GTQ0RAQWLn//esIDXMAm8ZJa49\n\tWc8GkhAWcJDoO72NBaLIUaL53wJGCDtMYtLXJ8wgNouAikTbxVdgcV4BX4kHO54xQSxbzCSx\n\t9MocJpAEp4CexMdNz8CKGAXEJL6fWgMWZxYQl7j1ZD6YLSEgILFkz3lmCFtU4uXjf6wQtqLE\n\t1enLoep1JBbs/sQGYWtLLFv4mhlisaDEyZlPWCYwis5CMnYWkpZZSFpmIWlZwMiyilG0OLU4\n\tKTfdyEgvtSgzubg4P08vL7VkEyMwEg9u+W2wg/Hlc8dDjAIcjEo8vC7hayOFWBPLiitzDzFK\n\tcDArifDqxwKFeFMSK6tSi/Lji0pzUosPMUpzsCiJ8zruuxAhJJCeWJKanZpakFoEk2Xi4JRq\n\tYOwUuZw5q2XLuxlT/Z1lUiy/ruTPU9HVZG/bxuJVLlP98VLme7uKOgsu9YD94Z1u3bmTVAS+\n\tlSx3/7Lq6admgxfcfu+WNLKcuHmycwYft/3Xm/t+d1ybyXc81lXrCH+vQ+0BkRUG28W4f/LO\n\t5xC8wGbS4K1SNksi5lbNcgfmOatnhPLsZN1uqsRSnJFoqMVcVJwIAB2mIVHAAgAA","Sender":"netdev-owner@vger.kernel.org","Precedence":"bulk","List-ID":"","X-Mailing-List":"netdev@vger.kernel.org"}},{"id":1762678,"web_url":"http://patchwork.ozlabs.org/comment/1762678/","msgid":"<20170904161303.69624386@griffin>","list_archive_url":null,"date":"2017-09-04T14:13:03","subject":"Re: [PATCH net-next v7] openvswitch: enable NSH support","submitter":{"id":9287,"url":"http://patchwork.ozlabs.org/api/people/9287/","name":"Jiri Benc","email":"jbenc@redhat.com"},"content":"On Mon, 4 Sep 2017 14:07:45 +0000, Jan Scheurich wrote:\n> Then perhaps I misunderstood your comment. I thought you didn't like that the \n> SET_MASKED action wrapped OVS_KEY_ATTR_NSH which in itself was nested.\n> I was aiming to avoid this by lifting the two components of the NSH header \n> components to the top level:\n> OVS_NSH_ATTR_BASE_HEADER --> OVS_KEY_ATTR_NSH_BASE_HEADER\n> OVS_NSH_ATTR_MD1_CONTEXT --> OVS_KEY_ATTR_NSH_MD1_CONTEXT\n\nNo, this should be a nested attr.\n\nI objected to the way value+mask combo is handled.\n\n> See above. The two would be separate values in the same enum, i.e. distinct.\n\nThis is not how netlink attrs should be designed.\n\n> Not sure I have the full picture here. Are you saying that the tc tool uses the same\n> Netlink API to the kernel as OVS? What would be the back-end for tc? Also the\n> openvswitch kernel module? \n\nIt does not use the same API but it makes sense for these two to share\ncommon code. Plus hw offloading in ovs is done using tc.\n\n Jiri","headers":{"Return-Path":"","X-Original-To":"patchwork-incoming@ozlabs.org","Delivered-To":"patchwork-incoming@ozlabs.org","Authentication-Results":["ozlabs.org;\n\tspf=none (mailfrom) smtp.mailfrom=vger.kernel.org\n\t(client-ip=209.132.180.67; helo=vger.kernel.org;\n\tenvelope-from=netdev-owner@vger.kernel.org;\n\treceiver=)","ext-mx03.extmail.prod.ext.phx2.redhat.com;\n\tdmarc=none (p=none dis=none) header.from=redhat.com","ext-mx03.extmail.prod.ext.phx2.redhat.com;\n\tspf=fail smtp.mailfrom=jbenc@redhat.com"],"Received":["from vger.kernel.org (vger.kernel.org [209.132.180.67])\n\tby ozlabs.org (Postfix) with ESMTP id 3xmBdg2wymz9s7C\n\tfor ;\n\tTue, 5 Sep 2017 00:13:11 +1000 (AEST)","(majordomo@vger.kernel.org) by vger.kernel.org via listexpand\n\tid S1753730AbdIDONI (ORCPT );\n\tMon, 4 Sep 2017 10:13:08 -0400","from mx1.redhat.com ([209.132.183.28]:57358 \"EHLO mx1.redhat.com\"\n\trhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP\n\tid S1753680AbdIDONH (ORCPT );\n\tMon, 4 Sep 2017 10:13:07 -0400","from smtp.corp.redhat.com\n\t(int-mx05.intmail.prod.int.phx2.redhat.com [10.5.11.15])\n\t(using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits))\n\t(No client certificate requested)\n\tby mx1.redhat.com (Postfix) with ESMTPS id 80E5F7E451;\n\tMon, 4 Sep 2017 14:13:07 +0000 (UTC)","from griffin (ovpn-116-46.ams2.redhat.com [10.36.116.46])\n\tby smtp.corp.redhat.com (Postfix) with ESMTPS id BB2B7C0D8E;\n\tMon, 4 Sep 2017 14:13:05 +0000 (UTC)"],"DMARC-Filter":"OpenDMARC Filter v1.3.2 mx1.redhat.com 80E5F7E451","Date":"Mon, 4 Sep 2017 16:13:03 +0200","From":"Jiri Benc ","To":"Jan Scheurich ","Cc":"\"Yang, Yi\" ,\n\t\"netdev@vger.kernel.org\" ,\n\t\"davem@davemloft.net\" ,\n\t\"dev@openvswitch.org\" ,\n\t\"e@erig.me\" , \"blp@ovn.org\" ","Subject":"Re: [PATCH net-next v7] openvswitch: enable NSH support","Message-ID":"<20170904161303.69624386@griffin>","In-Reply-To":"","References":"<1504096752-102003-1-git-send-email-yi.y.yang@intel.com>\n\t<20170831124516.0c5db686@griffin>\n\t<20170904080005.GA70767@cran64.bj.intel.com>\n\t<20170904124216.6db68e8c@griffin>\n\t\n\t<20170904151601.2f198a53@griffin>\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=US-ASCII","Content-Transfer-Encoding":"7bit","X-Scanned-By":"MIMEDefang 2.79 on 10.5.11.15","X-Greylist":"Sender IP whitelisted, not delayed by milter-greylist-4.5.16\n\t(mx1.redhat.com [10.5.110.27]);\n\tMon, 04 Sep 2017 14:13:07 +0000 (UTC)","Sender":"netdev-owner@vger.kernel.org","Precedence":"bulk","List-ID":"","X-Mailing-List":"netdev@vger.kernel.org"}},{"id":1762701,"web_url":"http://patchwork.ozlabs.org/comment/1762701/","msgid":"","list_archive_url":null,"date":"2017-09-04T14:45:50","subject":"RE: [PATCH net-next v7] openvswitch: enable NSH support","submitter":{"id":68449,"url":"http://patchwork.ozlabs.org/api/people/68449/","name":"Jan Scheurich","email":"jan.scheurich@ericsson.com"},"content":"> On Mon, 4 Sep 2017 14:07:45 +0000, Jan Scheurich wrote:\n> > Then perhaps I misunderstood your comment. I thought you didn't like that the\n> > SET_MASKED action wrapped OVS_KEY_ATTR_NSH which in itself was nested.\n> > I was aiming to avoid this by lifting the two components of the NSH header\n> > components to the top level:\n> > OVS_NSH_ATTR_BASE_HEADER --> OVS_KEY_ATTR_NSH_BASE_HEADER\n> > OVS_NSH_ATTR_MD1_CONTEXT --> OVS_KEY_ATTR_NSH_MD1_CONTEXT\n> \n> No, this should be a nested attr.\n> \n> I objected to the way value+mask combo is handled.\n\nOK, sorry for the confusion. \n\nSo what is the correct layout for MASKED_SET action with nested fields?\n1. All nested values, followed by all nested masks, or\n2. For each nested field value followed by mask?\n\nI guess alternative 1, but just to be sure.\n\nJan","headers":{"Return-Path":"","X-Original-To":"patchwork-incoming@ozlabs.org","Delivered-To":"patchwork-incoming@ozlabs.org","Authentication-Results":"ozlabs.org;\n\tspf=none (mailfrom) smtp.mailfrom=vger.kernel.org\n\t(client-ip=209.132.180.67; helo=vger.kernel.org;\n\tenvelope-from=netdev-owner@vger.kernel.org;\n\treceiver=)","Received":["from vger.kernel.org (vger.kernel.org [209.132.180.67])\n\tby ozlabs.org (Postfix) with ESMTP id 3xmCMw28Nqz9t2R\n\tfor ;\n\tTue, 5 Sep 2017 00:46:20 +1000 (AEST)","(majordomo@vger.kernel.org) by vger.kernel.org via listexpand\n\tid S1753780AbdIDOqR convert rfc822-to-8bit (ORCPT\n\t);\n\tMon, 4 Sep 2017 10:46:17 -0400","from sesbmg23.ericsson.net ([193.180.251.37]:62006 \"EHLO\n\tsesbmg23.ericsson.net\" rhost-flags-OK-OK-OK-OK) by vger.kernel.org\n\twith ESMTP id S1753793AbdIDOqQ (ORCPT\n\t); Mon, 4 Sep 2017 10:46:16 -0400","from ESESSHC009.ericsson.se (Unknown_Domain [153.88.183.45])\n\tby sesbmg23.ericsson.net (Symantec Mail Security) with SMTP id\n\t27.70.21299.6376DA95; Mon, 4 Sep 2017 16:46:14 +0200 (CEST)","from ESESSMB107.ericsson.se ([169.254.7.26]) by\n\tESESSHC009.ericsson.se ([153.88.183.45]) with mapi id 14.03.0352.000;\n\tMon, 4 Sep 2017 16:45:50 +0200"],"X-AuditID":"c1b4fb25-d31059c000005333-9a-59ad67360911","From":"Jan Scheurich ","To":"Jiri Benc ","CC":"\"Yang, Yi\" ,\n\t\"netdev@vger.kernel.org\" ,\n\t\"davem@davemloft.net\" ,\n\t\"dev@openvswitch.org\" ,\n\t\"e@erig.me\" , \"blp@ovn.org\" ","Subject":"RE: [PATCH net-next v7] openvswitch: enable NSH support","Thread-Topic":"[PATCH net-next v7] openvswitch: enable NSH support","Thread-Index":"AQHTIY2Lg9q8QO7fWEy80RgWemsrXKKeJ9QAgAYbLICAAC1QAIAAN2Qw///zkoCAACvjgP//5AyAgAAoYjA=","Date":"Mon, 4 Sep 2017 14:45:50 +0000","Message-ID":"","References":"<1504096752-102003-1-git-send-email-yi.y.yang@intel.com>\n\t<20170831124516.0c5db686@griffin>\n\t<20170904080005.GA70767@cran64.bj.intel.com>\n\t<20170904124216.6db68e8c@griffin>\n\t\n\t<20170904151601.2f198a53@griffin>\n\t\n\t<20170904161303.69624386@griffin>","In-Reply-To":"<20170904161303.69624386@griffin>","Accept-Language":"en-US","Content-Language":"en-US","X-MS-Has-Attach":"","X-MS-TNEF-Correlator":"","x-originating-ip":"[153.88.183.20]","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"8BIT","MIME-Version":"1.0","X-Brightmail-Tracker":"H4sIAAAAAAAAA+NgFlrLIsWRmVeSWpSXmKPExsUyM2K7rq5Z+tpIg+6TVhavJjcwWsw538Ji\n\tcfT0HmaLxT0nmCxWbDrNbHFsgZjF76/bmBzYPbasvMnkcWTRQUaPxXteMnk8u/mf0eP5tR4W\n\tj/f7rrJ5fN4kF8AexWWTkpqTWZZapG+XwJXRdamDveAoW0X327nsDYzzWbsYOTkkBEwkXh9r\n\tZuti5OIQEjjCKDHl9z9mCGcRo8TNz/0sXYwcHGwCBhKzdzuANIgIKEjs/P+eFaSGWeAto8S1\n\tJ+vZQBLCAg4Sfae3sUAUOUo0/1vACGEnSRz4M4EdxGYRUJE4uXk7K8hMXgFfiXf3SiB2dTNL\n\tNMz6wwxSwymgJ3H48m6w6xgFxCS+n1rDBGIzC4hL3HoynwniagGJJXvOM0PYohIvH/+D+kZR\n\t4ur05VD1OhILdn9ig7C1JZYtfA1WzysgKHFy5hOWCYyis5CMnYWkZRaSlllIWhYwsqxiFC1O\n\tLU7KTTcy1kstykwuLs7P08tLLdnECIzEg1t+q+5gvPzG8RCjAAejEg/v+Yi1kUKsiWXFlbmH\n\tGCU4mJVEeJ8lAoV4UxIrq1KL8uOLSnNSiw8xSnOwKInzOu67ECEkkJ5YkpqdmlqQWgSTZeLg\n\tlGpgFLR6OPfuapXme+/3JfZOsXycuX2RN/Pbgi0NK3eoMRe5XXrJfH7jj4/Rh5efbxBc8GTn\n\t4bp5N+1vhFa7bHz/a3LSpEoGs6fL/sV8/svjce/Flcfpq+WkdaZ8cd2x4mzS0XW1cRv2i9e3\n\tPb89yel9wYOJguL5G10+mLGqBb7s4ZW2er2Ia1rCZTklluKMREMt5qLiRADnIAoYwAIAAA==","Sender":"netdev-owner@vger.kernel.org","Precedence":"bulk","List-ID":"","X-Mailing-List":"netdev@vger.kernel.org"}},{"id":1762987,"web_url":"http://patchwork.ozlabs.org/comment/1762987/","msgid":"<20170905055144.GA88800@cran64.bj.intel.com>","list_archive_url":null,"date":"2017-09-05T05:51:45","subject":"Re: [PATCH net-next v7] openvswitch: enable NSH support","submitter":{"id":68962,"url":"http://patchwork.ozlabs.org/api/people/68962/","name":"Yang, Yi","email":"yi.y.yang@intel.com"},"content":"On Mon, Sep 04, 2017 at 08:57:44PM +0800, Jiri Benc wrote:\n> On Mon, 4 Sep 2017 20:09:07 +0800, Yang, Yi wrote:\n> > So we must do many changes if we want to break this assumption.\n> \n> We may do as many changes as we want to. This is uAPI we're talking\n> about and we need to get it right since the beginning. Sure, it may\n> mean that some user space programs need some changes in order to make\n> use of the new features. That happens every day.\n> \n> I also don't understand where's the problem. It's very easy to check\n> for NLA_F_NESTED and generically act based on that in the function you\n> quoted. Just call a different function than format_odp_key_attr to\n> handle ovs_nsh_key_attr attributes in case the nested flag is set and\n> the attribute is OVS_KEY_ATTR_NSH and you're done. You'll need such\n> function anyway, it's not much different code size wise to call it from\n> format_odp_key_attr or from format_odp_action.\n\nBut if we follow your way, how does nla_for_each_nested handle such\npattern?\n\nattribute1\nattribute1_mask\nattribute2\nattribute2_mask\nattribute3\nattribute3_mask\n\nI don't think this can increase performance and readability.\n\nIn current way, we just call nla_for_each_nested twice\n\none is for\n\nattribute1\nattribute2\nattribute3\n\nanother is for\n\nattribute1_mask\nattribute2_mask\nattribute3_mask\n\nif we use one function to handle both attributes and masks, I can't\nsee any substantial difference between two ways as far as the\nperformance is concerned.\n\nSo my proposal is we needn't introduce special handling case for\nOVS_KEY_ATTR_NSH in OVS_ACTION_ATTR_SET_MASKED, that will open Pandora's\nbox.\n\nIf we consider OVS_KEY_ATTR_NSH as a big attribute, current way is\nprecisely following current design pattern\n\nattribute\nmask\n\n> \n> Jiri","headers":{"Return-Path":"","X-Original-To":"patchwork-incoming@ozlabs.org","Delivered-To":"patchwork-incoming@ozlabs.org","Authentication-Results":"ozlabs.org;\n\tspf=none (mailfrom) smtp.mailfrom=vger.kernel.org\n\t(client-ip=209.132.180.67; helo=vger.kernel.org;\n\tenvelope-from=netdev-owner@vger.kernel.org;\n\treceiver=)","Received":["from vger.kernel.org (vger.kernel.org [209.132.180.67])\n\tby ozlabs.org (Postfix) with ESMTP id 3xmbs20m2cz9s8J\n\tfor ;\n\tTue, 5 Sep 2017 16:09:26 +1000 (AEST)","(majordomo@vger.kernel.org) by vger.kernel.org via listexpand\n\tid S1750984AbdIEGGk (ORCPT );\n\tTue, 5 Sep 2017 02:06:40 -0400","from mga09.intel.com ([134.134.136.24]:26870 \"EHLO mga09.intel.com\"\n\trhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP\n\tid S1750762AbdIEGGi (ORCPT );\n\tTue, 5 Sep 2017 02:06:38 -0400","from orsmga003.jf.intel.com ([10.7.209.27])\n\tby orsmga102.jf.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384;\n\t04 Sep 2017 23:06:37 -0700","from unknown (HELO cran64.bj.intel.com) ([10.240.224.181])\n\tby orsmga003.jf.intel.com with ESMTP; 04 Sep 2017 23:06:35 -0700"],"X-ExtLoop1":"1","X-IronPort-AV":"E=Sophos;i=\"5.41,478,1498546800\"; d=\"scan'208\";a=\"1010988463\"","Date":"Tue, 5 Sep 2017 13:51:45 +0800","From":"\"Yang, Yi\" ","To":"Jiri Benc ","Cc":"\"netdev@vger.kernel.org\" ,\n\t\"davem@davemloft.net\" ,\n\t\"dev@openvswitch.org\" ,\n\t\"e@erig.me\" , \"blp@ovn.org\" ,\n\t\"jan.scheurich@ericsson.com\" ","Subject":"Re: [PATCH net-next v7] openvswitch: enable NSH support","Message-ID":"<20170905055144.GA88800@cran64.bj.intel.com>","References":"<1504096752-102003-1-git-send-email-yi.y.yang@intel.com>\n\t<20170831124516.0c5db686@griffin>\n\t<20170904080005.GA70767@cran64.bj.intel.com>\n\t<20170904124216.6db68e8c@griffin>\n\t<20170904120907.GA75461@cran64.bj.intel.com>\n\t<20170904145744.4d8b7fd5@griffin>","MIME-Version":"1.0","Content-Type":"text/plain; charset=us-ascii","Content-Disposition":"inline","In-Reply-To":"<20170904145744.4d8b7fd5@griffin>","User-Agent":"Mutt/1.5.23 (2014-03-12)","Sender":"netdev-owner@vger.kernel.org","Precedence":"bulk","List-ID":"","X-Mailing-List":"netdev@vger.kernel.org"}},{"id":1763000,"web_url":"http://patchwork.ozlabs.org/comment/1763000/","msgid":"<20170905063705.GA89551@cran64.bj.intel.com>","list_archive_url":null,"date":"2017-09-05T06:37:05","subject":"Re: [PATCH net-next v7] openvswitch: enable NSH support","submitter":{"id":68962,"url":"http://patchwork.ozlabs.org/api/people/68962/","name":"Yang, Yi","email":"yi.y.yang@intel.com"},"content":"On Mon, Sep 04, 2017 at 06:42:16PM +0800, Jiri Benc wrote:\n> On Mon, 4 Sep 2017 16:00:05 +0800, Yang, Yi wrote:\n> > how can we know next push_nsh uses the same nsh header as previous\n> > one?\n> \n> We store the prepopulated header together with the action.\n>\n\nI checked source code but can't find where we can prepopulate it and how\nwe can deliver the prepopulated header to push_nsh, can you expand it in\norder that I can know how to do this in code level?","headers":{"Return-Path":"","X-Original-To":"patchwork-incoming@ozlabs.org","Delivered-To":"patchwork-incoming@ozlabs.org","Authentication-Results":"ozlabs.org;\n\tspf=none (mailfrom) smtp.mailfrom=vger.kernel.org\n\t(client-ip=209.132.180.67; helo=vger.kernel.org;\n\tenvelope-from=netdev-owner@vger.kernel.org;\n\treceiver=)","Received":["from vger.kernel.org (vger.kernel.org [209.132.180.67])\n\tby ozlabs.org (Postfix) with ESMTP id 3xmcp84934z9sP3\n\tfor ;\n\tTue, 5 Sep 2017 16:52:00 +1000 (AEST)","(majordomo@vger.kernel.org) by vger.kernel.org via listexpand\n\tid S1750810AbdIEGv6 (ORCPT );\n\tTue, 5 Sep 2017 02:51:58 -0400","from mga11.intel.com ([192.55.52.93]:62619 \"EHLO mga11.intel.com\"\n\trhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP\n\tid S1750729AbdIEGv5 (ORCPT );\n\tTue, 5 Sep 2017 02:51:57 -0400","from fmsmga002.fm.intel.com ([10.253.24.26])\n\tby fmsmga102.fm.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384;\n\t04 Sep 2017 23:51:57 -0700","from unknown (HELO cran64.bj.intel.com) ([10.240.224.181])\n\tby fmsmga002.fm.intel.com with ESMTP; 04 Sep 2017 23:51:55 -0700"],"X-ExtLoop1":"1","X-IronPort-AV":"E=Sophos;i=\"5.41,478,1498546800\"; d=\"scan'208\";a=\"1214787957\"","Date":"Tue, 5 Sep 2017 14:37:05 +0800","From":"\"Yang, Yi\" ","To":"Jiri Benc ","Cc":"\"netdev@vger.kernel.org\" ,\n\t\"davem@davemloft.net\" ,\n\t\"dev@openvswitch.org\" ,\n\t\"e@erig.me\" , \"blp@ovn.org\" ,\n\t\"jan.scheurich@ericsson.com\" ","Subject":"Re: [PATCH net-next v7] openvswitch: enable NSH support","Message-ID":"<20170905063705.GA89551@cran64.bj.intel.com>","References":"<1504096752-102003-1-git-send-email-yi.y.yang@intel.com>\n\t<20170831124516.0c5db686@griffin>\n\t<20170904080005.GA70767@cran64.bj.intel.com>\n\t<20170904124216.6db68e8c@griffin>","MIME-Version":"1.0","Content-Type":"text/plain; charset=us-ascii","Content-Disposition":"inline","In-Reply-To":"<20170904124216.6db68e8c@griffin>","User-Agent":"Mutt/1.5.23 (2014-03-12)","Sender":"netdev-owner@vger.kernel.org","Precedence":"bulk","List-ID":"","X-Mailing-List":"netdev@vger.kernel.org"}},{"id":1763143,"web_url":"http://patchwork.ozlabs.org/comment/1763143/","msgid":"<20170905114645.33bf0e32@griffin>","list_archive_url":null,"date":"2017-09-05T09:46:45","subject":"Re: [PATCH net-next v7] openvswitch: enable NSH support","submitter":{"id":9287,"url":"http://patchwork.ozlabs.org/api/people/9287/","name":"Jiri Benc","email":"jbenc@redhat.com"},"content":"On Tue, 5 Sep 2017 13:51:45 +0800, Yang, Yi wrote:\n> But if we follow your way, how does nla_for_each_nested handle such\n> pattern?\n> \n> attribute1\n> attribute1_mask\n> attribute2\n> attribute2_mask\n> attribute3\n> attribute3_mask\n\nUh? That will just work. Note that nla len contains the size of the\nwhole attribute, i.e. includes both fields.\n\n> I don't think this can increase performance and readability.\n\nDo you realize you're stating that not copying data around in hot path\ncan't increase performance? ;-)\n\n> if we use one function to handle both attributes and masks, I can't\n> see any substantial difference between two ways as far as the\n> performance is concerned.\n\nExcept that what you did is so unexpected to netlink that you had to go\nout of your way to parse it. Those two memcpys speak for themselves.\n\n> If we consider OVS_KEY_ATTR_NSH as a big attribute, current way is\n> precisely following current design pattern\n\nDo you have an idea how nested netlink attributes work? It's very well\ndefined. What you're doing is breaking the definition. You can't do\nthat.\n\nThe (non-nested) attributes contain header followed by data. The data\nis a single value or it is a struct. In ovs for masked actions,\nattributes contain a struct of two fields (value and mask). The struct\naccess is open coded but it's still a struct.\n\nNow, nested attributes are very well defined: it's a nla header\nfollowed by a stream of attributes. There's no requirement on the order\nof the attributes. Do you see how you're breaking this assumption? You\nexpect that each attribute is present exactly twice, once among first\nhalf of attributes, once among second half of attributes. You don't\ncheck that this weird assumption is adhered to. You don't even check\nthat the point where you split the data is between attributes! You may\nvery well be splitting an attribute in half and interpreting its data\nas nla headers.\n\nConsider your proposal NACKed from my side.\n\n Jiri","headers":{"Return-Path":"","X-Original-To":"patchwork-incoming@ozlabs.org","Delivered-To":"patchwork-incoming@ozlabs.org","Authentication-Results":["ozlabs.org;\n\tspf=none (mailfrom) smtp.mailfrom=vger.kernel.org\n\t(client-ip=209.132.180.67; helo=vger.kernel.org;\n\tenvelope-from=netdev-owner@vger.kernel.org;\n\treceiver=)","ext-mx09.extmail.prod.ext.phx2.redhat.com;\n\tdmarc=none (p=none dis=none) header.from=redhat.com","ext-mx09.extmail.prod.ext.phx2.redhat.com;\n\tspf=fail smtp.mailfrom=jbenc@redhat.com"],"Received":["from vger.kernel.org (vger.kernel.org [209.132.180.67])\n\tby ozlabs.org (Postfix) with ESMTP id 3xmhgx4jQPz9s83\n\tfor ;\n\tTue, 5 Sep 2017 19:46:53 +1000 (AEST)","(majordomo@vger.kernel.org) by vger.kernel.org via listexpand\n\tid S1750959AbdIEJqu (ORCPT );\n\tTue, 5 Sep 2017 05:46:50 -0400","from mx1.redhat.com ([209.132.183.28]:55604 \"EHLO mx1.redhat.com\"\n\trhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP\n\tid S1750748AbdIEJqt (ORCPT );\n\tTue, 5 Sep 2017 05:46:49 -0400","from smtp.corp.redhat.com\n\t(int-mx03.intmail.prod.int.phx2.redhat.com [10.5.11.13])\n\t(using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits))\n\t(No client certificate requested)\n\tby mx1.redhat.com (Postfix) with ESMTPS id 3ABE64A707;\n\tTue, 5 Sep 2017 09:46:49 +0000 (UTC)","from griffin (unknown [10.36.118.10])\n\tby smtp.corp.redhat.com (Postfix) with ESMTPS id 733996E51D;\n\tTue, 5 Sep 2017 09:46:47 +0000 (UTC)"],"DMARC-Filter":"OpenDMARC Filter v1.3.2 mx1.redhat.com 3ABE64A707","Date":"Tue, 5 Sep 2017 11:46:45 +0200","From":"Jiri Benc ","To":"\"Yang, Yi\" ","Cc":"\"netdev@vger.kernel.org\" ,\n\t\"davem@davemloft.net\" ,\n\t\"dev@openvswitch.org\" ,\n\t\"e@erig.me\" , \"blp@ovn.org\" ,\n\t\"jan.scheurich@ericsson.com\" ","Subject":"Re: [PATCH net-next v7] openvswitch: enable NSH support","Message-ID":"<20170905114645.33bf0e32@griffin>","In-Reply-To":"<20170905055144.GA88800@cran64.bj.intel.com>","References":"<1504096752-102003-1-git-send-email-yi.y.yang@intel.com>\n\t<20170831124516.0c5db686@griffin>\n\t<20170904080005.GA70767@cran64.bj.intel.com>\n\t<20170904124216.6db68e8c@griffin>\n\t<20170904120907.GA75461@cran64.bj.intel.com>\n\t<20170904145744.4d8b7fd5@griffin>\n\t<20170905055144.GA88800@cran64.bj.intel.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=US-ASCII","Content-Transfer-Encoding":"7bit","X-Scanned-By":"MIMEDefang 2.79 on 10.5.11.13","X-Greylist":"Sender IP whitelisted, not delayed by milter-greylist-4.5.16\n\t(mx1.redhat.com [10.5.110.38]);\n\tTue, 05 Sep 2017 09:46:49 +0000 (UTC)","Sender":"netdev-owner@vger.kernel.org","Precedence":"bulk","List-ID":"","X-Mailing-List":"netdev@vger.kernel.org"}},{"id":1763147,"web_url":"http://patchwork.ozlabs.org/comment/1763147/","msgid":"<20170905114741.537dfc11@griffin>","list_archive_url":null,"date":"2017-09-05T09:47:41","subject":"Re: [PATCH net-next v7] openvswitch: enable NSH support","submitter":{"id":9287,"url":"http://patchwork.ozlabs.org/api/people/9287/","name":"Jiri Benc","email":"jbenc@redhat.com"},"content":"On Tue, 5 Sep 2017 14:37:05 +0800, Yang, Yi wrote:\n> I checked source code but can't find where we can prepopulate it and how\n> we can deliver the prepopulated header to push_nsh, can you expand it in\n> order that I can know how to do this in code level?\n\nI already said that it's not implemented yet and can be done as a\nfuture performance enhancement.\n\n Jiri","headers":{"Return-Path":"","X-Original-To":"patchwork-incoming@ozlabs.org","Delivered-To":"patchwork-incoming@ozlabs.org","Authentication-Results":["ozlabs.org;\n\tspf=none (mailfrom) smtp.mailfrom=vger.kernel.org\n\t(client-ip=209.132.180.67; helo=vger.kernel.org;\n\tenvelope-from=netdev-owner@vger.kernel.org;\n\treceiver=)","ext-mx09.extmail.prod.ext.phx2.redhat.com;\n\tdmarc=none (p=none dis=none) header.from=redhat.com","ext-mx09.extmail.prod.ext.phx2.redhat.com;\n\tspf=fail smtp.mailfrom=jbenc@redhat.com"],"Received":["from vger.kernel.org (vger.kernel.org [209.132.180.67])\n\tby ozlabs.org (Postfix) with ESMTP id 3xmhj03mm4z9s83\n\tfor ;\n\tTue, 5 Sep 2017 19:47:48 +1000 (AEST)","(majordomo@vger.kernel.org) by vger.kernel.org via listexpand\n\tid S1751296AbdIEJrp (ORCPT );\n\tTue, 5 Sep 2017 05:47:45 -0400","from mx1.redhat.com ([209.132.183.28]:56874 \"EHLO mx1.redhat.com\"\n\trhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP\n\tid S1751150AbdIEJrp (ORCPT );\n\tTue, 5 Sep 2017 05:47:45 -0400","from smtp.corp.redhat.com\n\t(int-mx04.intmail.prod.int.phx2.redhat.com [10.5.11.14])\n\t(using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits))\n\t(No client certificate requested)\n\tby mx1.redhat.com (Postfix) with ESMTPS id DA30A61992;\n\tTue, 5 Sep 2017 09:47:44 +0000 (UTC)","from griffin (unknown [10.36.118.10])\n\tby smtp.corp.redhat.com (Postfix) with ESMTPS id 481527DECC;\n\tTue, 5 Sep 2017 09:47:43 +0000 (UTC)"],"DMARC-Filter":"OpenDMARC Filter v1.3.2 mx1.redhat.com DA30A61992","Date":"Tue, 5 Sep 2017 11:47:41 +0200","From":"Jiri Benc ","To":"\"Yang, Yi\" ","Cc":"\"netdev@vger.kernel.org\" ,\n\t\"davem@davemloft.net\" ,\n\t\"dev@openvswitch.org\" ,\n\t\"e@erig.me\" , \"blp@ovn.org\" ,\n\t\"jan.scheurich@ericsson.com\" ","Subject":"Re: [PATCH net-next v7] openvswitch: enable NSH support","Message-ID":"<20170905114741.537dfc11@griffin>","In-Reply-To":"<20170905063705.GA89551@cran64.bj.intel.com>","References":"<1504096752-102003-1-git-send-email-yi.y.yang@intel.com>\n\t<20170831124516.0c5db686@griffin>\n\t<20170904080005.GA70767@cran64.bj.intel.com>\n\t<20170904124216.6db68e8c@griffin>\n\t<20170905063705.GA89551@cran64.bj.intel.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=US-ASCII","Content-Transfer-Encoding":"7bit","X-Scanned-By":"MIMEDefang 2.79 on 10.5.11.14","X-Greylist":"Sender IP whitelisted, not delayed by milter-greylist-4.5.16\n\t(mx1.redhat.com [10.5.110.38]);\n\tTue, 05 Sep 2017 09:47:45 +0000 (UTC)","Sender":"netdev-owner@vger.kernel.org","Precedence":"bulk","List-ID":"","X-Mailing-List":"netdev@vger.kernel.org"}},{"id":1763151,"web_url":"http://patchwork.ozlabs.org/comment/1763151/","msgid":"<20170905114949.5849b75f@griffin>","list_archive_url":null,"date":"2017-09-05T09:49:49","subject":"Re: [PATCH net-next v7] openvswitch: enable NSH support","submitter":{"id":9287,"url":"http://patchwork.ozlabs.org/api/people/9287/","name":"Jiri Benc","email":"jbenc@redhat.com"},"content":"On Mon, 4 Sep 2017 14:45:50 +0000, Jan Scheurich wrote:\n> So what is the correct layout for MASKED_SET action with nested fields?\n> 1. All nested values, followed by all nested masks, or\n> 2. For each nested field value followed by mask?\n> \n> I guess alternative 1, but just to be sure.\n\nIt's 2. Alternative 1 breaks netlink assumptions, is ugly to implement\nand probably impossible to be properly validated.\n\n Jiri","headers":{"Return-Path":"","X-Original-To":"patchwork-incoming@ozlabs.org","Delivered-To":"patchwork-incoming@ozlabs.org","Authentication-Results":["ozlabs.org;\n\tspf=none (mailfrom) smtp.mailfrom=vger.kernel.org\n\t(client-ip=209.132.180.67; helo=vger.kernel.org;\n\tenvelope-from=netdev-owner@vger.kernel.org;\n\treceiver=)","ext-mx03.extmail.prod.ext.phx2.redhat.com;\n\tdmarc=none (p=none dis=none) header.from=redhat.com","ext-mx03.extmail.prod.ext.phx2.redhat.com;\n\tspf=fail smtp.mailfrom=jbenc@redhat.com"],"Received":["from vger.kernel.org (vger.kernel.org [209.132.180.67])\n\tby ozlabs.org (Postfix) with ESMTP id 3xmhlS46H1z9s83\n\tfor ;\n\tTue, 5 Sep 2017 19:49:56 +1000 (AEST)","(majordomo@vger.kernel.org) by vger.kernel.org via listexpand\n\tid S1751264AbdIEJty (ORCPT );\n\tTue, 5 Sep 2017 05:49:54 -0400","from mx1.redhat.com ([209.132.183.28]:52992 \"EHLO mx1.redhat.com\"\n\trhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP\n\tid S1751017AbdIEJtx (ORCPT );\n\tTue, 5 Sep 2017 05:49:53 -0400","from smtp.corp.redhat.com\n\t(int-mx03.intmail.prod.int.phx2.redhat.com [10.5.11.13])\n\t(using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits))\n\t(No client certificate requested)\n\tby mx1.redhat.com (Postfix) with ESMTPS id 303B783F3E;\n\tTue, 5 Sep 2017 09:49:53 +0000 (UTC)","from griffin (unknown [10.36.118.10])\n\tby smtp.corp.redhat.com (Postfix) with ESMTPS id 7A4016E527;\n\tTue, 5 Sep 2017 09:49:51 +0000 (UTC)"],"DMARC-Filter":"OpenDMARC Filter v1.3.2 mx1.redhat.com 303B783F3E","Date":"Tue, 5 Sep 2017 11:49:49 +0200","From":"Jiri Benc ","To":"Jan Scheurich ","Cc":"\"Yang, Yi\" ,\n\t\"netdev@vger.kernel.org\" ,\n\t\"davem@davemloft.net\" ,\n\t\"dev@openvswitch.org\" ,\n\t\"e@erig.me\" , \"blp@ovn.org\" ","Subject":"Re: [PATCH net-next v7] openvswitch: enable NSH support","Message-ID":"<20170905114949.5849b75f@griffin>","In-Reply-To":"","References":"<1504096752-102003-1-git-send-email-yi.y.yang@intel.com>\n\t<20170831124516.0c5db686@griffin>\n\t<20170904080005.GA70767@cran64.bj.intel.com>\n\t<20170904124216.6db68e8c@griffin>\n\t\n\t<20170904151601.2f198a53@griffin>\n\t\n\t<20170904161303.69624386@griffin>\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=US-ASCII","Content-Transfer-Encoding":"7bit","X-Scanned-By":"MIMEDefang 2.79 on 10.5.11.13","X-Greylist":"Sender IP whitelisted, not delayed by milter-greylist-4.5.16\n\t(mx1.redhat.com [10.5.110.27]);\n\tTue, 05 Sep 2017 09:49:53 +0000 (UTC)","Sender":"netdev-owner@vger.kernel.org","Precedence":"bulk","List-ID":"","X-Mailing-List":"netdev@vger.kernel.org"}},{"id":1763196,"web_url":"http://patchwork.ozlabs.org/comment/1763196/","msgid":"","list_archive_url":null,"date":"2017-09-05T10:36:12","subject":"RE: [PATCH net-next v7] openvswitch: enable NSH support","submitter":{"id":68449,"url":"http://patchwork.ozlabs.org/api/people/68449/","name":"Jan Scheurich","email":"jan.scheurich@ericsson.com"},"content":"> From: Jiri Benc [mailto:jbenc@redhat.com]\n\n> > So what is the correct layout for MASKED_SET action with nested fields?\n> > 1. All nested values, followed by all nested masks, or\n> > 2. For each nested field value followed by mask?\n> >\n> > I guess alternative 1, but just to be sure.\n> \n> It's 2. Alternative 1 breaks netlink assumptions, is ugly to implement\n> and probably impossible to be properly validated.\n\nOK. For outsiders this was far from obvious :-)\n\nSo, for OVS_ACTION_ATTR_SET_MASKED any nested attribute, no matter on which nesting level, must contain value directly followed by mask.\n\nIf that is the principle of handling masks in Netlink APIs, than we must adhere to it.\n\nBR, Jan","headers":{"Return-Path":"","X-Original-To":"patchwork-incoming@ozlabs.org","Delivered-To":"patchwork-incoming@ozlabs.org","Authentication-Results":"ozlabs.org;\n\tspf=none (mailfrom) smtp.mailfrom=vger.kernel.org\n\t(client-ip=209.132.180.67; helo=vger.kernel.org;\n\tenvelope-from=netdev-owner@vger.kernel.org;\n\treceiver=)","Received":["from vger.kernel.org (vger.kernel.org [209.132.180.67])\n\tby ozlabs.org (Postfix) with ESMTP id 3xmjmz3SFyz9sNr\n\tfor ;\n\tTue, 5 Sep 2017 20:36:19 +1000 (AEST)","(majordomo@vger.kernel.org) by vger.kernel.org via listexpand\n\tid S1750766AbdIEKgR convert rfc822-to-8bit (ORCPT\n\t);\n\tTue, 5 Sep 2017 06:36:17 -0400","from sessmg23.ericsson.net ([193.180.251.45]:59417 \"EHLO\n\tsessmg23.ericsson.net\" rhost-flags-OK-OK-OK-OK) by vger.kernel.org\n\twith ESMTP id S1750707AbdIEKgQ (ORCPT\n\t); Tue, 5 Sep 2017 06:36:16 -0400","from ESESSHC016.ericsson.se (Unknown_Domain [153.88.183.66])\n\tby sessmg23.ericsson.net (Symantec Mail Security) with SMTP id\n\t3A.93.22436.E1E7EA95; Tue, 5 Sep 2017 12:36:14 +0200 (CEST)","from ESESSMB107.ericsson.se ([169.254.7.26]) by\n\tESESSHC016.ericsson.se ([153.88.183.66]) with mapi id 14.03.0352.000;\n\tTue, 5 Sep 2017 12:36:13 +0200"],"X-AuditID":"c1b4fb2d-11bff700000057a4-16-59ae7e1e7929","From":"Jan Scheurich ","To":"Jiri Benc ","CC":"\"Yang, Yi\" ,\n\t\"netdev@vger.kernel.org\" ,\n\t\"davem@davemloft.net\" ,\n\t\"dev@openvswitch.org\" ,\n\t\"e@erig.me\" , \"blp@ovn.org\" ","Subject":"RE: [PATCH net-next v7] openvswitch: enable NSH support","Thread-Topic":"[PATCH net-next v7] openvswitch: enable NSH support","Thread-Index":"AQHTIY2Lg9q8QO7fWEy80RgWemsrXKKeJ9QAgAYbLICAAC1QAIAAN2Qw///zkoCAACvjgP//5AyAgAAoYjCAASBngIAAKbGQ","Date":"Tue, 5 Sep 2017 10:36:12 +0000","Message-ID":"","References":"<1504096752-102003-1-git-send-email-yi.y.yang@intel.com>\n\t<20170831124516.0c5db686@griffin>\n\t<20170904080005.GA70767@cran64.bj.intel.com>\n\t<20170904124216.6db68e8c@griffin>\n\t\n\t<20170904151601.2f198a53@griffin>\n\t\n\t<20170904161303.69624386@griffin>\n\t\n\t<20170905114949.5849b75f@griffin>","In-Reply-To":"<20170905114949.5849b75f@griffin>","Accept-Language":"en-US","Content-Language":"en-US","X-MS-Has-Attach":"","X-MS-TNEF-Correlator":"","x-originating-ip":"[153.88.183.18]","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"8BIT","MIME-Version":"1.0","X-Brightmail-Tracker":"H4sIAAAAAAAAA+NgFlrDIsWRmVeSWpSXmKPExsUyM2K7k65c3bpIg/sLpC1eTW5gtJhzvoXF\n\t4ujpPcwWi3tOMFms2HSa2eLYAjGL31+3MTmwe2xZeZPJ48iig4wei/e8ZPJ4dvM/o8fzaz0s\n\tHu/3XWXz+LxJLoA9issmJTUnsyy1SN8ugSujc+krtoLLrBW35gs2MO5l6WLk5JAQMJG4f/A9\n\texcjF4eQwBFGibtPp7NAOIsYJdqW7GTuYuTgYBMwkJi92wGkQURAQWLn//esIDXMAm8ZJa49\n\tWc8GkhAWcJDoO72NBaLIUaL53wJGkF4RgTyJq3elQMIsAioSzROesIPYvAK+EteO72CD2PWZ\n\tWeJq2xlGkASngJ7EnIaFYEWMAmIS30+tYQKxmQXEJW49mc8EcbWAxJI955khbFGJl4//sULY\n\tihIfX+1jhKjXkViw+xMbhK0tsWzha2aIxYISJ2c+YZnAKDoLydhZSFpmIWmZhaRlASPLKkbR\n\t4tTi4tx0I2O91KLM5OLi/Dy9vNSSTYzAODy45bfuDsbVrx0PMQpwMCrx8PKVrIsUYk0sK67M\n\tPcQowcGsJMLbXA4U4k1JrKxKLcqPLyrNSS0+xCjNwaIkzuuw70KEkEB6YklqdmpqQWoRTJaJ\n\tg1OqgZFfeO0GVuuyb4y2om/udFwIL/A1KFd+Z+QT+zWNW/DKbB22SKY6DaGF/Nt+3Plh2xaW\n\tn5pyPf3znLWClQFZvybXvXikq7bYr85D9/uLLOWJrwR19kcYc4pneBy+mHCZpWDivzczT5h8\n\tid8sf+h3T0/vhXVJK5cJap4xUUqYdXa+5koZq461PEosxRmJhlrMRcWJANCFIQq/AgAA","Sender":"netdev-owner@vger.kernel.org","Precedence":"bulk","List-ID":"","X-Mailing-List":"netdev@vger.kernel.org"}},{"id":1763227,"web_url":"http://patchwork.ozlabs.org/comment/1763227/","msgid":"<20170905105702.GA92895@cran64.bj.intel.com>","list_archive_url":null,"date":"2017-09-05T10:57:03","subject":"Re: [PATCH net-next v7] openvswitch: enable NSH support","submitter":{"id":68962,"url":"http://patchwork.ozlabs.org/api/people/68962/","name":"Yang, Yi","email":"yi.y.yang@intel.com"},"content":"On Tue, Sep 05, 2017 at 11:47:41AM +0200, Jiri Benc wrote:\n> On Tue, 5 Sep 2017 14:37:05 +0800, Yang, Yi wrote:\n> > I checked source code but can't find where we can prepopulate it and how\n> > we can deliver the prepopulated header to push_nsh, can you expand it in\n> > order that I can know how to do this in code level?\n> \n> I already said that it's not implemented yet and can be done as a\n> future performance enhancement.\n\nGot it, thanks.\n\n> \n> Jiri","headers":{"Return-Path":"","X-Original-To":"patchwork-incoming@ozlabs.org","Delivered-To":"patchwork-incoming@ozlabs.org","Authentication-Results":"ozlabs.org;\n\tspf=none (mailfrom) smtp.mailfrom=vger.kernel.org\n\t(client-ip=209.132.180.67; helo=vger.kernel.org;\n\tenvelope-from=netdev-owner@vger.kernel.org;\n\treceiver=)","Received":["from vger.kernel.org (vger.kernel.org [209.132.180.67])\n\tby ozlabs.org (Postfix) with ESMTP id 3xmkZ70TpJz9s83\n\tfor ;\n\tTue, 5 Sep 2017 21:11:58 +1000 (AEST)","(majordomo@vger.kernel.org) by vger.kernel.org via listexpand\n\tid S1750896AbdIELL4 (ORCPT );\n\tTue, 5 Sep 2017 07:11:56 -0400","from mga06.intel.com ([134.134.136.31]:32664 \"EHLO mga06.intel.com\"\n\trhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP\n\tid S1750731AbdIELLz (ORCPT );\n\tTue, 5 Sep 2017 07:11:55 -0400","from fmsmga004.fm.intel.com ([10.253.24.48])\n\tby orsmga104.jf.intel.com with ESMTP; 05 Sep 2017 04:11:55 -0700","from unknown (HELO cran64.bj.intel.com) ([10.240.224.181])\n\tby fmsmga004.fm.intel.com with ESMTP; 05 Sep 2017 04:11:53 -0700"],"X-ExtLoop1":"1","X-IronPort-AV":"E=Sophos;i=\"5.41,480,1498546800\"; d=\"scan'208\";a=\"308072427\"","Date":"Tue, 5 Sep 2017 18:57:03 +0800","From":"\"Yang, Yi\" ","To":"Jiri Benc ","Cc":"\"netdev@vger.kernel.org\" ,\n\t\"davem@davemloft.net\" ,\n\t\"dev@openvswitch.org\" ,\n\t\"e@erig.me\" , \"blp@ovn.org\" ,\n\t\"jan.scheurich@ericsson.com\" ","Subject":"Re: [PATCH net-next v7] openvswitch: enable NSH support","Message-ID":"<20170905105702.GA92895@cran64.bj.intel.com>","References":"<1504096752-102003-1-git-send-email-yi.y.yang@intel.com>\n\t<20170831124516.0c5db686@griffin>\n\t<20170904080005.GA70767@cran64.bj.intel.com>\n\t<20170904124216.6db68e8c@griffin>\n\t<20170905063705.GA89551@cran64.bj.intel.com>\n\t<20170905114741.537dfc11@griffin>","MIME-Version":"1.0","Content-Type":"text/plain; charset=us-ascii","Content-Disposition":"inline","In-Reply-To":"<20170905114741.537dfc11@griffin>","User-Agent":"Mutt/1.5.23 (2014-03-12)","Sender":"netdev-owner@vger.kernel.org","Precedence":"bulk","List-ID":"","X-Mailing-List":"netdev@vger.kernel.org"}},{"id":1763233,"web_url":"http://patchwork.ozlabs.org/comment/1763233/","msgid":"<20170905110310.GB92895@cran64.bj.intel.com>","list_archive_url":null,"date":"2017-09-05T11:03:11","subject":"Re: [PATCH net-next v7] openvswitch: enable NSH support","submitter":{"id":68962,"url":"http://patchwork.ozlabs.org/api/people/68962/","name":"Yang, Yi","email":"yi.y.yang@intel.com"},"content":"On Tue, Sep 05, 2017 at 11:46:45AM +0200, Jiri Benc wrote:\n> On Tue, 5 Sep 2017 13:51:45 +0800, Yang, Yi wrote:\n> > But if we follow your way, how does nla_for_each_nested handle such\n> > pattern?\n> > \n> > attribute1\n> > attribute1_mask\n> > attribute2\n> > attribute2_mask\n> > attribute3\n> > attribute3_mask\n> \n> Uh? That will just work. Note that nla len contains the size of the\n> whole attribute, i.e. includes both fields.\n> \n> > I don't think this can increase performance and readability.\n>\n\nI got the point, actually value and mask are data of one attribute, so\nthey are three attributes but not six.","headers":{"Return-Path":"","X-Original-To":"patchwork-incoming@ozlabs.org","Delivered-To":"patchwork-incoming@ozlabs.org","Authentication-Results":"ozlabs.org;\n\tspf=none (mailfrom) smtp.mailfrom=vger.kernel.org\n\t(client-ip=209.132.180.67; helo=vger.kernel.org;\n\tenvelope-from=netdev-owner@vger.kernel.org;\n\treceiver=)","Received":["from vger.kernel.org (vger.kernel.org [209.132.180.67])\n\tby ozlabs.org (Postfix) with ESMTP id 3xmkjB1rzYz9s83\n\tfor ;\n\tTue, 5 Sep 2017 21:18:06 +1000 (AEST)","(majordomo@vger.kernel.org) by vger.kernel.org via listexpand\n\tid S1750913AbdIELSE (ORCPT );\n\tTue, 5 Sep 2017 07:18:04 -0400","from mga02.intel.com ([134.134.136.20]:34421 \"EHLO mga02.intel.com\"\n\trhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP\n\tid S1750755AbdIELSD (ORCPT );\n\tTue, 5 Sep 2017 07:18:03 -0400","from orsmga002.jf.intel.com ([10.7.209.21])\n\tby orsmga101.jf.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384;\n\t05 Sep 2017 04:18:03 -0700","from unknown (HELO cran64.bj.intel.com) ([10.240.224.181])\n\tby orsmga002.jf.intel.com with ESMTP; 05 Sep 2017 04:18:01 -0700"],"X-ExtLoop1":"1","X-IronPort-AV":"E=Sophos;i=\"5.41,480,1498546800\"; d=\"scan'208\";a=\"131840936\"","Date":"Tue, 5 Sep 2017 19:03:11 +0800","From":"\"Yang, Yi\" ","To":"Jiri Benc ","Cc":"\"netdev@vger.kernel.org\" ,\n\t\"davem@davemloft.net\" ,\n\t\"dev@openvswitch.org\" ,\n\t\"e@erig.me\" , \"blp@ovn.org\" ,\n\t\"jan.scheurich@ericsson.com\" ","Subject":"Re: [PATCH net-next v7] openvswitch: enable NSH support","Message-ID":"<20170905110310.GB92895@cran64.bj.intel.com>","References":"<1504096752-102003-1-git-send-email-yi.y.yang@intel.com>\n\t<20170831124516.0c5db686@griffin>\n\t<20170904080005.GA70767@cran64.bj.intel.com>\n\t<20170904124216.6db68e8c@griffin>\n\t<20170904120907.GA75461@cran64.bj.intel.com>\n\t<20170904145744.4d8b7fd5@griffin>\n\t<20170905055144.GA88800@cran64.bj.intel.com>\n\t<20170905114645.33bf0e32@griffin>","MIME-Version":"1.0","Content-Type":"text/plain; charset=us-ascii","Content-Disposition":"inline","In-Reply-To":"<20170905114645.33bf0e32@griffin>","User-Agent":"Mutt/1.5.23 (2014-03-12)","Sender":"netdev-owner@vger.kernel.org","Precedence":"bulk","List-ID":"","X-Mailing-List":"netdev@vger.kernel.org"}},{"id":1765105,"web_url":"http://patchwork.ozlabs.org/comment/1765105/","msgid":"<20170908061322.GA15698@cran64.bj.intel.com>","list_archive_url":null,"date":"2017-09-08T06:13:23","subject":"Re: [PATCH net-next v7] openvswitch: enable NSH support","submitter":{"id":68962,"url":"http://patchwork.ozlabs.org/api/people/68962/","name":"Yang, Yi","email":"yi.y.yang@intel.com"},"content":"On Thu, Aug 31, 2017 at 06:45:16PM +0800, Jiri Benc wrote:\n> On Wed, 30 Aug 2017 20:39:12 +0800, Yi Yang wrote:\n> > +\t\t\tnsh->md1.context[i] =\n> > +\t\t\t OVS_MASKED(nsh->md1.context[i], key->context[i],\n> > +\t\t\t\t mask->context[i]);\n> > +\t\t}\n> > +\t\tmemcpy(flow_key->nsh.context, nsh->md1.context,\n> > +\t\t sizeof(nsh->md1.context));\n> \n> Do you follow the discussion that Hannes Sowa started on the ovs list\n> regarding matching on the context fields? It would be better to hold on this\n> until there's a conclusion reached.\n\nWe had several back-and-forth discussions, Jan was also involved, the\nconslusion is current way is still the best way we can have, we need\nset and match for context fields.\n\n> > +\t\t\t type,\n> > +\t\t\t nla_len(a),\n> > +\t\t\t ovs_nsh_key_attr_lens[type].len\n> > +\t\t\t);\n> > +\t\t\treturn -EINVAL;\n> > +\t\t}\n> \n> These checks should be done only once when the action is configured, not for\n> each packet.\n\nin v8, nsh_key_put_from_nlattr is only one check point, validate_nsh calls it\nfor set and push_nsh, nsh_hdr_from_nlattr and nsh_key_from_nlattr\nhaven't these checked anymore because validate_nsh has done them.\n\n> > +\tif (unlikely(!has_md1 && !has_md2)) {\n> > +\t\tOVS_NLERR(1, \"neither nsh md1 nor md2 attribute is there\");\n> > +\t\treturn -EINVAL;\n> > +\t}\n> \n> Ditto. Plus I don't see a check that the OVS_NSH_KEY_ATTR_BASE attribute is\n> present. Seems that the compiler warned you about possibly unitialized flags\n> and ttl variables and you just silenced the warning without considering\n> whether it's not actually justified.\n\nnsh_key_put_from_nlattr is used for both set and push_nsh, so\nOVS_NSH_KEY_ATTR_BASE check is valid only for push_nsh, it is possible\nfor set not to have OVS_NSH_KEY_ATTR_BASE. v8 has been successfully built\nwithout any warnings, it is also verified in sfc test environment.\n\nI think v8 has fixed all the comments for v7, I have sent out v8 to\nmailing list, please help review it, thanks a lot.","headers":{"Return-Path":"","X-Original-To":"patchwork-incoming@ozlabs.org","Delivered-To":"patchwork-incoming@ozlabs.org","Authentication-Results":"ozlabs.org;\n\tspf=none (mailfrom) smtp.mailfrom=vger.kernel.org\n\t(client-ip=209.132.180.67; helo=vger.kernel.org;\n\tenvelope-from=netdev-owner@vger.kernel.org;\n\treceiver=)","Received":["from vger.kernel.org (vger.kernel.org [209.132.180.67])\n\tby ozlabs.org (Postfix) with ESMTP id 3xpS7T3VhZz9sBZ\n\tfor ;\n\tFri, 8 Sep 2017 16:28:21 +1000 (AEST)","(majordomo@vger.kernel.org) by vger.kernel.org via listexpand\n\tid S1754532AbdIHG2S (ORCPT );\n\tFri, 8 Sep 2017 02:28:18 -0400","from mga01.intel.com ([192.55.52.88]:38145 \"EHLO mga01.intel.com\"\n\trhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP\n\tid S1753206AbdIHG2S (ORCPT );\n\tFri, 8 Sep 2017 02:28:18 -0400","from fmsmga004.fm.intel.com ([10.253.24.48])\n\tby fmsmga101.fm.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384;\n\t07 Sep 2017 23:28:17 -0700","from unknown (HELO cran64.bj.intel.com) ([10.240.224.181])\n\tby fmsmga004.fm.intel.com with ESMTP; 07 Sep 2017 23:28:15 -0700"],"X-ExtLoop1":"1","X-IronPort-AV":"E=Sophos;i=\"5.42,360,1500966000\"; d=\"scan'208\";a=\"309368336\"","Date":"Fri, 8 Sep 2017 14:13:23 +0800","From":"\"Yang, Yi\" ","To":"Jiri Benc ","Cc":"\"netdev@vger.kernel.org\" ,\n\t\"davem@davemloft.net\" ,\n\t\"dev@openvswitch.org\" ,\n\t\"e@erig.me\" , \"blp@ovn.org\" ,\n\t\"jan.scheurich@ericsson.com\" ","Subject":"Re: [PATCH net-next v7] openvswitch: enable NSH support","Message-ID":"<20170908061322.GA15698@cran64.bj.intel.com>","References":"<1504096752-102003-1-git-send-email-yi.y.yang@intel.com>\n\t<20170831124516.0c5db686@griffin>","MIME-Version":"1.0","Content-Type":"text/plain; charset=us-ascii","Content-Disposition":"inline","In-Reply-To":"<20170831124516.0c5db686@griffin>","User-Agent":"Mutt/1.5.23 (2014-03-12)","Sender":"netdev-owner@vger.kernel.org","Precedence":"bulk","List-ID":"","X-Mailing-List":"netdev@vger.kernel.org"}}]