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

[Jiri Benc <jbenc-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>]

On Thu, 10 Aug 2017 21:21:15 +0800, Yi Yang wrote:
> 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 2.8 release in compat mode by porting this.

Please include changelog when posting a new version of a patch.

> +static inline u16
> +nsh_hdr_len(const struct nsh_hdr *nsh)

Single line, please. And all other instances of this in nsh.h, too.

> --- 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,                  /* struct ovs_key_nsh */
>  
>  #ifdef __KERNEL__
>  	OVS_KEY_ATTR_TUNNEL_INFO,  /* struct ip_tunnel_info */
> @@ -491,6 +492,15 @@ struct ovs_key_ct_tuple_ipv6 {
>  	__u8   ipv6_proto;
>  };
>  
> +struct ovs_key_nsh {
> +	__u8 flags;
> +	__u8 mdtype;
> +	__u8 np;
> +	__u8 pad;
> +	__be32 path_hdr;
> +	__be32 c[4];

I still don't like the "c" name. Please change it to something
descriptive. This is uAPI and can't be changed later.

And I still don't see my comment about this not being extensible for
MD type 2 addressed. Please understand this is uAPI and it is set in
stone once it is merged into the kernel. It's very important we get
this right since the beginning.

> +struct ovs_action_push_nsh {
> +	__u8 flags;
> +	__u8 mdtype;
> +	__u8 mdlen;
> +	__u8 np;
> +	__be32 path_hdr;
> +	__u8 metadata[];
> +};

This is not how netlink attributes work. Please reread Ben Pfaff's
explanation on how this needs to be structured (Message-ID:
<20170809180912.GU6175-LZ6Gd1LRuIk@public.gmane.org>) and rework the patch. I 100% agree
with what he wrote, his proposal is very clean and matches how netlink
is designed.

> @@ -835,6 +866,8 @@ enum ovs_action_attr {
>  	OVS_ACTION_ATTR_TRUNC,        /* u32 struct ovs_action_trunc. */
>  	OVS_ACTION_ATTR_PUSH_ETH,     /* struct ovs_action_push_eth. */
>  	OVS_ACTION_ATTR_POP_ETH,      /* No argument. */
> +	OVS_ACTION_ATTR_PUSH_NSH,     /* struct ovs_action_push_nsh. */
> +	OVS_ACTION_ATTR_POP_NSH,      /* No argument. */

Thank you for changing this to push/pop, it looks much cleaner now.

> +static int parse_nsh(struct sk_buff *skb, struct sw_flow_key *key)
> +{
> +	struct nsh_hdr *nsh = (struct nsh_hdr *)skb_network_header(skb);
> +	u16 ver_flags_len;
> +	u8 version, length;
> +	u32 path_hdr;
> +	int i;
> +
> +	memset(&key->nsh, 0, sizeof(struct ovs_key_nsh));
> +	ver_flags_len = ntohs(nsh->ver_flags_len);
> +	version = (ver_flags_len & NSH_VER_MASK) >> NSH_VER_SHIFT;
> +	length = (ver_flags_len & NSH_LEN_MASK) >> NSH_LEN_SHIFT;

A nit: the operation to get/set version, length and flags from the NSH
header seems to be repeated enough to warrant helper functions in
include/net/nsh.h. Something like:

static inline u8 nsh_get_version(const struct nsh_hdr *nsh)
{
	return (ntohs(nsh->ver_flags_len) & NSH_VER_MASK) >> NSH_VER_SHIFT;
}

etc.

Not a blocker, though, it may be done later if needed.

> @@ -76,9 +77,11 @@ static bool actions_may_change_flow(const struct nlattr *actions)
>  
>  		case OVS_ACTION_ATTR_CT:
>  		case OVS_ACTION_ATTR_HASH:
> +		case OVS_ACTION_ATTR_POP_NSH:
>  		case OVS_ACTION_ATTR_POP_ETH:
>  		case OVS_ACTION_ATTR_POP_MPLS:
>  		case OVS_ACTION_ATTR_POP_VLAN:
> +		case OVS_ACTION_ATTR_PUSH_NSH:
>  		case OVS_ACTION_ATTR_PUSH_ETH:
>  		case OVS_ACTION_ATTR_PUSH_MPLS:
>  		case OVS_ACTION_ATTR_PUSH_VLAN:

Alphabetical order, please.

Thanks,

 Jiri