Re: [PATCH v3] openvswitch: enable NSH support

["Yang, Yi" <yi.y.yang@intel.com>]

On Wed, Aug 16, 2017 at 11:19:21AM +0200, Jiri Benc wrote:
> Please always CC reviewers of the previous versions, thanks.

Jiri, thank you for quick review. Sorry, I made a mistake on
sending and missed all the CCs, will indeed do this in next version.

> > +	__be16 ver_flags_len;
> > +	u8 md_type;
> > +	u8 next_proto;
> > +	__be32 path_hdr;
> > +	u8 metadata[NSH_M_TYPE2_MAX_LEN-8];
> > +};
> 
> Please get rid of this struct. It's a copy of struct nsh_hdr with some
> space added to the bottom.
> 
> One of the options (though maybe not the best one, feel free to come up
> with something better) is to change struct nsh_md1_ctx to:
> 
> struct nsh_md1_ctx {
> 	__be32 context[];
> };
> 
> and change struct push_nsh_para:
> 
> struct push_nsh_para {
> 	struct nsh_hdr hdr;
> 	u8 metadata[NSH_M_TYPE2_MAX_LEN-8];
> };
> 
> Another option (a better one, though a bit more work) is to get rid of
> push_nsh_para completely and just pass a properly allocated nsh_hdr
> around. Introduce macros and/or functions to help with the allocation.
>

Yeah, good point, we can use dynamic allocation and struct nsh_hdr * to
handle this.

> > +static inline struct nsh_md1_ctx *nsh_md1_ctx(struct nsh_hdr *nsh)
> > +{
> > +	return &nsh->md1;
> > +}
> > +
> > +static inline struct nsh_md2_tlv *nsh_md2_ctx(struct nsh_hdr *nsh)
> > +{
> > +	return nsh->md2;
> > +}
> 
> These are unused, please remove them.

Will remove them, userspace does use them.

> 
> > --- a/include/uapi/linux/openvswitch.h
> > +++ b/include/uapi/linux/openvswitch.h
> [...]
> > +#define NSH_MD1_CONTEXT_SIZE 4
> 
> Please move this to nsh.h and use it there, too, instead of the open
> coded 4.

ovs code is very ugly, it will convert array[4] in
datapath/linux/compat/include/linux/openvswitch.h to other struct, I
have to change context[4] to such format :-), we can use 4 here for
Linux kernel.

> 
> > +static int push_nsh(struct sk_buff *skb, struct sw_flow_key *key,
> > +		    const struct push_nsh_para *pnp)
> > +{
> > +	struct nsh_hdr *nsh;
> > +	size_t length = ((ntohs(pnp->ver_flags_len) & NSH_LEN_MASK)
> > +			     >> NSH_LEN_SHIFT) << 2;
> 
> Once push_nsh_para is removed/changed, this can be changed to a call to
> nsh_hdr_len.

Yes, will do that way.

> 
> > +	flags = (ntohs(nsh->ver_flags_len) & NSH_FLAGS_MASK) >>
> > +		NSH_FLAGS_SHIFT;
> 
> nsh_get_flags

Missed this one :-)

> 
> > +	case OVS_KEY_ATTR_NSH: {
> > +		struct ovs_key_nsh nsh;
> > +		struct ovs_key_nsh nsh_mask;
> > +		size_t size = nla_len(a) / 2;
> > +		struct nlattr attr[1 + DIV_ROUND_UP(sizeof(struct ovs_key_ipv6)
> > +						    , sizeof(struct nlattr))];
> > +		struct nlattr mask[1 + DIV_ROUND_UP(sizeof(struct ovs_key_ipv6)
> > +						    , sizeof(struct nlattr))];
> > +
> > +		attr->nla_type = nla_type(a);
> > +		mask->nla_type = attr->nla_type;
> > +		attr->nla_len = NLA_HDRLEN + size;
> > +		mask->nla_len = attr->nla_len;
> > +		memcpy(attr + 1, (char *)(a + 1), size);
> > +		memcpy(mask + 1, (char *)(a + 1) + size, size);
> 
> This is too hacky. Please find a better way to handle this.
> 
> One option is to create a struct with struct nlattr as the first member
> followed by a char buffer. Still not nice but at least it's clear
> what's the intent.

The issue is nested attributes only can use this way, nested attribute
for SET_MASKED is very special, we have to handle it specially.

> 
> > +static int parse_nsh(struct sk_buff *skb, struct sw_flow_key *key)
> > +{
> > +	struct nsh_hdr *nsh = (struct nsh_hdr *)skb_network_header(skb);
> > +	u8 version, length;
> > +	u32 path_hdr;
> > +	int i;
> > +
> > +	memset(&key->nsh, 0, sizeof(struct ovs_key_nsh));
> > +	version = nsh_get_ver(nsh);
> > +	length = nsh_get_len(nsh);
> > +
> > +	key->nsh.flags = nsh_get_flags(nsh);
> > +	key->nsh.mdtype = nsh->md_type;
> > +	key->nsh.np = nsh->next_proto;
> > +	path_hdr = ntohl(nsh->path_hdr);
> 
> The path_hdr variable is unused.

Will remove it.

> 
> > +	key->nsh.path_hdr = nsh->path_hdr;
> > +	switch (key->nsh.mdtype) {
> > +	case NSH_M_TYPE1:
> > +		if ((length << 2) != NSH_M_TYPE1_LEN)
> 
> Why length << 2?

len in NSH header is number of 4 octets, so need to multiply 4.

> 
> > +			return -EINVAL;
> > +
> > +		for (i = 0; i < 4; i++)
> 
> NSH_MD1_CONTEXT_SIZE

Ok

> 
> > +			key->nsh.context[i] = nsh->md1.context[i];
> > +
> > +		break;
> 
> Will go through the rest later. Feel free to send a new version
> meanwhile.
> 
> Thanks,
> 
>  Jiri

Thank you so much for your comments, will work out new version ASAP.