Re: [PATCH net-next v13] openvswitch: enable NSH support

[Yang, Yi]

On Mon, Oct 30, 2017 at 10:01:34AM +0900, David Miller wrote:
> From: Yi Yang 
> Date: Thu, 26 Oct 2017 15:45:16 +0800
> 
> > OVS master and 2.8 branch has merged NSH userspace
> > patch series, this patch is to enable NSH support
> > in kernel data path in order that OVS can support
> > NSH in compat mode by porting this.
> > 
> > Signed-off-by: Yi Yang 
> 
> One small request for function naming:
> 
> > diff --git a/include/net/nsh.h b/include/net/nsh.h
> > index a1eaea2..7dcf377 100644
> > --- a/include/net/nsh.h
> > +++ b/include/net/nsh.h
> > @@ -304,4 +304,7 @@ static inline void nsh_set_flags_ttl_len(struct nshhdr 
> > *nsh, u8 flags,
> > NSH_FLAGS_MASK | NSH_TTL_MASK | NSH_LEN_MASK);
> >  }
> >  
> > +int skb_push_nsh(struct sk_buff *skb, const struct nshhdr *pushed_nh);
> > +int skb_pop_nsh(struct sk_buff *skb);
> > +
> >  #endif /* __NET_NSH_H */
> 
> All functions named "skb_*" should be core SKB handling functions found
> and implemented in include/linux/skbuff.h and net/core/skbuff.c
> 
> Protocol specific helpers like this should be named "$PROTOCOL_NAME_*"
> so please renamed these something like "nsh_header_push()" and
> "nsh_header_pop()".

Dave, thank for your comments, I have changed skb_push_nsh to nsh_push and
changed skb_pop_nsh to nsh_pop, posted v14, please review, thanks.

> 
> Thank you.

Re: [PATCH net-next v13] openvswitch: enable NSH support

[David Miller]

From: Yi Yang 
Date: Thu, 26 Oct 2017 15:45:16 +0800

> OVS master and 2.8 branch has merged NSH userspace
> patch series, this patch is to enable NSH support
> in kernel data path in order that OVS can support
> NSH in compat mode by porting this.
> 
> Signed-off-by: Yi Yang 

One small request for function naming:

> diff --git a/include/net/nsh.h b/include/net/nsh.h
> index a1eaea2..7dcf377 100644
> --- a/include/net/nsh.h
> +++ b/include/net/nsh.h
> @@ -304,4 +304,7 @@ static inline void nsh_set_flags_ttl_len(struct nshhdr 
> *nsh, u8 flags,
>   NSH_FLAGS_MASK | NSH_TTL_MASK | NSH_LEN_MASK);
>  }
>  
> +int skb_push_nsh(struct sk_buff *skb, const struct nshhdr *pushed_nh);
> +int skb_pop_nsh(struct sk_buff *skb);
> +
>  #endif /* __NET_NSH_H */

All functions named "skb_*" should be core SKB handling functions found
and implemented in include/linux/skbuff.h and net/core/skbuff.c

Protocol specific helpers like this should be named "$PROTOCOL_NAME_*"
so please renamed these something like "nsh_header_push()" and
"nsh_header_pop()".

Thank you.

[PATCH net-next v13] openvswitch: enable NSH support

[Yi Yang]

v12->v13
 - Fix NSH header length check in set_nsh

v11->v12
 - Fix missing changes old comments pointed out
 - Fix new comments for v11

v10->v11
 - Fix the left three disputable comments for v9
   but not fixed in v10.

v9->v10
 - Change struct ovs_key_nsh to
   struct ovs_nsh_key_base base;
   __be32 context[NSH_MD1_CONTEXT_SIZE];
 - Fix new comments for v9

v8->v9
 - Fix build error reported by daily intel build
   because nsh module isn't selected by openvswitch

v7->v8
 - Rework nested value and mask for OVS_KEY_ATTR_NSH
 - Change pop_nsh to adapt to nsh kernel module
 - Fix many issues per comments from Jiri Benc

v6->v7
 - Remove NSH GSO patches in v6 because Jiri Benc
   reworked it as another patch series and they have
   been merged.
 - Change it to adapt to nsh kernel module added by NSH
   GSO patch series

v5->v6
 - Fix the rest comments for v4.
 - Add NSH GSO support for VxLAN-gpe + NSH and
   Eth + NSH.

v4->v5
 - Fix many comments by Jiri Benc and Eric Garver
   for v4.

v3->v4
 - Add new NSH match field ttl
 - Update NSH header to the latest format
   which will be final format and won't change
   per its author's confirmation.
 - Fix comments for v3.

v2->v3
 - Change OVS_KEY_ATTR_NSH to nested key to handle
   length-fixed attributes and length-variable
   attriubte more flexibly.
 - Remove struct ovs_action_push_nsh completely
 - Add code to handle nested attribute for SET_MASKED
 - Change PUSH_NSH to use the nested OVS_KEY_ATTR_NSH
   to transfer NSH header data.
 - Fix comments and coding style issues by Jiri and Eric

v1->v2
 - Change encap_nsh and decap_nsh to push_nsh and pop_nsh
 - Dynamically allocate struct ovs_action_push_nsh for
   length-variable metadata.

OVS master and 2.8 branch has merged NSH userspace
patch series, this patch is to enable NSH support
in kernel data path in order that OVS can support
NSH in compat mode by porting this.

Signed-off-by: Yi Yang 
---
 include/net/nsh.h|   3 +
 include/uapi/linux/openvswitch.h |  29 
 net/nsh/nsh.c|  61 
 net/openvswitch/Kconfig  |   1 +
 net/openvswitch/actions.c| 117 +++
 net/openvswitch/flow.c   |  51 +++
 net/openvswitch/flow.h   |   7 +
 net/openvswitch/flow_netlink.c   | 310 ++-
 net/openvswitch/flow_netlink.h   |   5 +
 9 files changed, 583 insertions(+), 1 deletion(-)

diff --git a/include/net/nsh.h b/include/net/nsh.h
index a1eaea2..7dcf377 100644
--- a/include/net/nsh.h
+++ b/include/net/nsh.h
@@ -304,4 +304,7 @@ static inline void nsh_set_flags_ttl_len(struct nshhdr 
*nsh, u8 flags,
NSH_FLAGS_MASK | NSH_TTL_MASK | NSH_LEN_MASK);
 }
 
+int skb_push_nsh(struct sk_buff *skb, const struct nshhdr *pushed_nh);
+int skb_pop_nsh(struct sk_buff *skb);
+
 #endif /* __NET_NSH_H */
diff --git a/include/uapi/linux/openvswitch.h b/include/uapi/linux/openvswitch.h
index 0cd6f88..ac2623b 100644
--- a/include/uapi/linux/openvswitch.h
+++ b/include/uapi/linux/openvswitch.h
@@ -333,6 +333,7 @@ enum ovs_key_attr {
OVS_KEY_ATTR_CT_LABELS, /* 16-octet connection tracking label */
OVS_KEY_ATTR_CT_ORIG_TUPLE_IPV4,   /* struct ovs_key_ct_tuple_ipv4 */
OVS_KEY_ATTR_CT_ORIG_TUPLE_IPV6,   /* struct ovs_key_ct_tuple_ipv6 */
+   OVS_KEY_ATTR_NSH,   /* Nested set of ovs_nsh_key_* */
 
 #ifdef __KERNEL__
OVS_KEY_ATTR_TUNNEL_INFO,  /* struct ip_tunnel_info */
@@ -492,6 +493,30 @@ struct ovs_key_ct_tuple_ipv6 {
__u8   ipv6_proto;
 };
 
+enum ovs_nsh_key_attr {
+   OVS_NSH_KEY_ATTR_UNSPEC,
+   OVS_NSH_KEY_ATTR_BASE,  /* struct ovs_nsh_key_base. */
+   OVS_NSH_KEY_ATTR_MD1,   /* struct ovs_nsh_key_md1. */
+   OVS_NSH_KEY_ATTR_MD2,   /* variable-length octets for MD type 2. */
+   __OVS_NSH_KEY_ATTR_MAX
+};
+
+#define OVS_NSH_KEY_ATTR_MAX (__OVS_NSH_KEY_ATTR_MAX - 1)
+
+struct ovs_nsh_key_base {
+   __u8 flags;
+   __u8 ttl;
+   __u8 mdtype;
+   __u8 np;
+   __be32 path_hdr;
+};
+
+#define NSH_MD1_CONTEXT_SIZE 4
+
+struct ovs_nsh_key_md1 {
+   __be32 context[NSH_MD1_CONTEXT_SIZE];
+};
+
 /**
  * enum ovs_flow_attr - attributes for %OVS_FLOW_* commands.
  * @OVS_FLOW_ATTR_KEY: Nested %OVS_KEY_ATTR_* attributes specifying the flow
@@ -808,6 +833,8 @@ struct ovs_action_push_eth {
  * @OVS_ACTION_ATTR_POP_ETH: Pop the outermost Ethernet header off the
  * packet.
  * @OVS_ACTION_ATTR_CT_CLEAR: Clear conntrack state from the packet.
+ * @OVS_ACTION_ATTR_PUSH_NSH: push NSH header to the packet.
+ * @OVS_ACTION_ATTR_POP_NSH: pop the outermost NSH header off the packet.
  *
  * Only a single header can be set with a single %OVS_ACTION_ATTR_SET.  Not all
  * fields within a header are modifiable, e.g. the IPv4 protocol and fragment
@@ -838,6 +865,8 @@ enum ovs_action_attr {
OVS_ACTION_ATTR_PUSH_ETH, /* struct ovs_action_push_eth. */
OVS_ACTION_ATTR_POP_ETH,  /*