[{"id":1758932,"web_url":"http://patchwork.ozlabs.org/comment/1758932/","msgid":"","list_archive_url":null,"date":"2017-08-28T23:31:22","subject":"Re: [ovs-dev] [PATCH v4 2/3] nsh: add new flow key 'ttl'","submitter":{"id":68449,"url":"http://patchwork.ozlabs.org/api/people/68449/","name":"Jan Scheurich","email":"jan.scheurich@ericsson.com"},"content":"Hi Yi,\n\nThanks for the fixes. Please find my individual comments inline.\n\nBR, Jan\n\n> -----Original Message-----\n> -#define OVS_ENCAP_NSH_MAX_MD_LEN 16\n> /*\n> * struct ovs_action_encap_nsh - %OVS_ACTION_ATTR_ENCAP_NSH\n> * @flags: NSH header flags.\n> + * @ttl: NSH header TTL.\n> * @mdtype: NSH metadata type.\n> * @mdlen: Length of NSH metadata in bytes.\n> * @np: NSH next_protocol: Inner packet type.\n> @@ -805,11 +796,13 @@ struct ovs_action_push_eth {\n> */\n> struct ovs_action_encap_nsh {\n> uint8_t flags;\n> + uint8_t ttl;\n> uint8_t mdtype;\n> - uint8_t mdlen;\n> uint8_t np;\n> __be32 path_hdr;\n> - uint8_t metadata[OVS_ENCAP_NSH_MAX_MD_LEN];\n> + uint8_t mdlen;\n> + uint8_t pad[7]; /* Aligned to 64 bit boundary for metadata */\n> + uint8_t metadata[];\n> };\n\nAlignment of metadata to 32 bits should suffice here. Netlink attrs are only 4-byte aligned anyhow. The same goes for the MD1 context headers inside metadata.\n\n> diff --git a/include/openvswitch/flow.h b/include/openvswitch/flow.h\n> index a658a58..cd61fff 100644\n> --- a/include/openvswitch/flow.h\n> +++ b/include/openvswitch/flow.h\n> @@ -146,7 +146,7 @@ struct flow {\n> struct eth_addr arp_tha; /* ARP/ND target hardware address. */\n> ovs_be16 tcp_flags; /* TCP flags. With L3 to avoid matching L4. */\n> ovs_be16 pad2; /* Pad to 64 bits. */\n> - struct flow_nsh nsh; /* Network Service Header keys */\n> + struct ovs_key_nsh nsh; /* Network Service Header keys */\n\nWhy rename this type to struct ovs_key_nsh?\n\n> diff --git a/include/openvswitch/nsh.h b/include/openvswitch/nsh.h\n[...]\n> @@ -62,7 +210,7 @@ struct nsh_md2_tlv {\n> };\n> \n> struct nsh_hdr {\n> - ovs_be16 ver_flags_len;\n> + ovs_be16 ver_flags_ttl_len;\n> uint8_t md_type;\n> uint8_t next_proto;\n> ovs_16aligned_be32 path_hdr;\n> @@ -75,11 +223,16 @@ struct nsh_hdr {\n> /* Masking NSH header fields. */\n> #define NSH_VER_MASK 0xc000\n> #define NSH_VER_SHIFT 14\n> -#define NSH_FLAGS_MASK 0x3fc0\n> -#define NSH_FLAGS_SHIFT 6\n> +#define NSH_FLAGS_MASK 0x3000\n> +#define NSH_FLAGS_SHIFT 12\n\nWith this definition of NSH FLAGS the match field NSH_FLAGS covers\nonly the two bits between Version and TTL field. Bit 1 is the OaM flag, \nbit 0 is currently unused. The remaining 4 unused bits between Length\nand MD Type fields are excluded. That is perhaps OK as we cannot be\nsure the these 4 bits will be used as \"flags\" in the future. But it needs\nto be documented clearly that NSH_FLAGS has only two bits and which\nbits in the NSH header it covers.\n\n> +static inline void\n> +__nsh_set_xflag(struct nsh_hdr *nsh, uint16_t xflag, uint16_t xmask)\n> +{\n> + nsh->ver_flags_ttl_len\n> + = (nsh->ver_flags_ttl_len & ~htons(xmask)) | htons(xflag);\n> +}\n\nAvoid __* prefix. Better call it nsh_set_base_hdr_masked(). \n\n> +\n> +static inline void nsh_set_flags_and_ttl(struct nsh_hdr *nsh, uint8_t flags,\n> + uint8_t ttl)\n> +{\n> + __nsh_set_xflag(nsh, ((flags << NSH_FLAGS_SHIFT) &\n> NSH_FLAGS_MASK) |\n> + ((ttl << NSH_TTL_SHIFT) & NSH_TTL_MASK),\n> + NSH_FLAGS_MASK | NSH_TTL_MASK);\n> +}\n> +\n> +static inline void nsh_set_flags_ttl_len(struct nsh_hdr *nsh, uint8_t flags,\n> + uint8_t ttl, uint8_t len)\n> +{\n> + len = len >> 2;\n> + __nsh_set_xflag(nsh, ((flags << NSH_FLAGS_SHIFT) &\n> NSH_FLAGS_MASK) |\n> + ((ttl << NSH_TTL_SHIFT) & NSH_TTL_MASK) |\n> + ((len << NSH_LEN_SHIFT) & NSH_LEN_MASK),\n> + NSH_FLAGS_MASK | NSH_TTL_MASK | NSH_LEN_MASK);\n> +}\n\nDo you really need to these hybrid functions? Why not use modular functions nsh_set_flags(), nsh_set_ttl() and nsh_set_len()? I believe the run-time overhead is negligible with an optimizing compiler.\n\nFor symmetry you should also add a function setter function for path_hdr:\n\nstatic inline void\nnsh_set_path_hdr(struct nsh_hdr *nsh, ovs_be32 path_hdr)\n{\n put_16aligned_be32(&nsh->path_hdr, path_hdr);\n}\n\n> diff --git a/include/openvswitch/packets.h b/include/openvswitch/packets.h\n> index be91e02..603ec44 100644\n> --- a/include/openvswitch/packets.h\n> +++ b/include/openvswitch/packets.h\n>\n> /* Network Service Header keys */\n> -struct flow_nsh {\n> +struct ovs_key_nsh {\n> uint8_t flags;\n> + uint8_t ttl;\n> uint8_t mdtype;\n> uint8_t np;\n> - uint8_t si;\n> - ovs_be32 spi;\n> + ovs_be32 path_hdr;\n> ovs_be32 c[4];\n> };\n\nWhy did you rename this struct? As part of the struct flow, it was\nmore appropriately called struct flow_nsh.\n\n> diff --git a/lib/flow.c b/lib/flow.c\n> index b2b10aa..547ff70 100644\n> --- a/lib/flow.c\n> +++ b/lib/flow.c\n> bool\n> -parse_nsh(const void **datap, size_t *sizep, struct flow_nsh *key)\n> +parse_nsh(const void **datap, size_t *sizep, struct ovs_key_nsh *key)\n> {\n[...]\n> - /* Check if it is long enough for NSH header, doesn't support\n> - * MD type 2 yet\n> - */\n> if (OVS_UNLIKELY(*sizep < NSH_M_TYPE1_LEN)) {\n> return false;\n> }\n\nThis should compare against the minimum fixed size (NSH_BASE_HDR_LEN)\nof an NSH header, not NSH_M_TYPE1_LEN as we want to successfully parse \nany MD type header.\n\n> switch (key->mdtype) {\n> case NSH_M_TYPE1:\n> + if (length != NSH_M_TYPE1_LEN) {\n> + return false;\n> + }\n> for (size_t i = 0; i < 4; i++) {\n> key->c[i] = get_16aligned_be32(&nsh->md1.c[i]);\n> }\n> break;\n> case NSH_M_TYPE2:\n> - /* Don't support MD type 2 yet, so return false */\n> + /* Don't support MD type 2 metedata parsing yet */\n> + if (length < NSH_BASE_HDR_LEN) {\n> + return false;\n> + }\n\nThis check is superfluous as it has been done before.\n\n> diff --git a/lib/meta-flow.c b/lib/meta-flow.c\n> index 64a8cf1..b50679c 100644\n> --- a/lib/meta-flow.c\n> +++ b/lib/meta-flow.c\n> @@ -906,10 +914,13 @@ mf_get_value(const struct mf_field *mf, const\n> struct flow *flow,\n> value->u8 = flow->nsh.np;\n> break;\n> case MFF_NSH_SPI:\n> - value->be32 = flow->nsh.spi;\n> + value->be32 = nsh_path_hdr_to_spi(flow->nsh.path_hdr);\n> + if (value->be32 == htonl(NSH_SPI_MASK >> NSH_SPI_SHIFT)) {\n> + value->be32 = OVS_BE32_MAX;\n> + }\n\nThis looks strange. The SPI is a 24 bit field. Why would you expand it to OVS_BE32_MAX?\n\n> @@ -1221,10 +1235,12 @@ mf_set_value(const struct mf_field *mf,\n> MATCH_SET_FIELD_UINT8(match, nsh.np, value->u8);\n> break;\n> case MFF_NSH_SPI:\n> - MATCH_SET_FIELD_BE32(match, nsh.spi, value->be32);\n> + match->wc.masks.nsh.path_hdr |= htonl(NSH_SPI_MASK);\n> + nsh_path_hdr_set_spi(&match->flow.nsh.path_hdr, value->be32);\n\nIntroduce a wrapper function match_set_nsh_spi() where you rely on the setter functions in nsh.h to manipulate the path_hdr.\n\n> break;\n> case MFF_NSH_SI:\n> - MATCH_SET_FIELD_UINT8(match, nsh.si, value->u8);\n> + match->wc.masks.nsh.path_hdr |= htonl(NSH_SI_MASK);\n> + nsh_path_hdr_set_si(&match->flow.nsh.path_hdr, value->u8);\n> break;\n\nDito.\n\n> @@ -2103,10 +2126,12 @@ mf_set_wild(const struct mf_field *mf, struct\n> match *match, char **err_str)\n> MATCH_SET_FIELD_MASKED(match, nsh.np, 0, 0);\n> break;\n> case MFF_NSH_SPI:\n> - MATCH_SET_FIELD_MASKED(match, nsh.spi, htonl(0), htonl(0));\n> + match->wc.masks.nsh.path_hdr &= ~htonl(NSH_SPI_MASK);\n> + nsh_path_hdr_set_spi(&match->flow.nsh.path_hdr, htonl(0));\n\n\nDito.\n\n> break;\n> case MFF_NSH_SI:\n> - MATCH_SET_FIELD_MASKED(match, nsh.si, 0, 0);\n> + match->wc.masks.nsh.path_hdr &= ~htonl(NSH_SI_MASK);\n> + nsh_path_hdr_set_si(&match->flow.nsh.path_hdr, 0);\n\nDito.\n\n> break;\n> case MFF_NSH_C1:\n> case MFF_NSH_C2:\n\n> @@ -2363,10 +2391,14 @@ mf_set(const struct mf_field *mf,\n> MATCH_SET_FIELD_MASKED(match, nsh.np, value->u8, mask->u8);\n> break;\n> case MFF_NSH_SPI:\n> - MATCH_SET_FIELD_MASKED(match, nsh.spi, value->be32, mask-\n> >be32);\n> + match->wc.masks.nsh.path_hdr |= mask->be32;\n\nI believe this is incorrect. The mask is related to the value and requires shifting.\nOne more reason to use wrapper functions.\n\n> + nsh_path_hdr_set_spi(&match->flow.nsh.path_hdr,\n> + value->be32 & mask->be32);\n> break;\n> case MFF_NSH_SI:\n> - MATCH_SET_FIELD_MASKED(match, nsh.si, value->u8, mask->u8);\n> + match->wc.masks.nsh.path_hdr |= htonl(mask->u8);\n\nDito.\n\n> + nsh_path_hdr_set_si(&match->flow.nsh.path_hdr,\n> + value->u8 & mask->u8);\n\n> diff --git a/lib/meta-flow.xml b/lib/meta-flow.xml\n> index 065fb03..a4a0735 100644\n> --- a/lib/meta-flow.xml\n> +++ b/lib/meta-flow.xml\n> @@ -1311,7 +1311,9 @@ tcp,tp_src=0x07c0/0xfff0\n> \n> \n> - title=\"flags field (8 bits)\"/>\n> + title=\"flags field (2 bits)\"/>\n\nThe NSH_FLAGS field requires more explanation as it is not defined in the RFC.\n\n> diff --git a/lib/nx-match.c b/lib/nx-match.c\n> index b782e8c..dc52566 100644\n> --- a/lib/nx-match.c\n> +++ b/lib/nx-match.c\n> @@ -1157,13 +1158,22 @@ nx_put_raw(struct ofpbuf *b, enum\n> ofp_version oxm, const struct match *match,\n> /* Network Service Header */\n> nxm_put_8m(&ctx, MFF_NSH_FLAGS, oxm, flow->nsh.flags,\n> match->wc.masks.nsh.flags);\n> + nxm_put_8m(&ctx, MFF_NSH_TTL, oxm, flow->nsh.ttl,\n> + match->wc.masks.nsh.ttl);\n> nxm_put_8m(&ctx, MFF_NSH_MDTYPE, oxm, flow->nsh.mdtype,\n> match->wc.masks.nsh.mdtype);\n> nxm_put_8m(&ctx, MFF_NSH_NP, oxm, flow->nsh.np,\n> match->wc.masks.nsh.np);\n> - nxm_put_32m(&ctx, MFF_NSH_SPI, oxm, flow->nsh.spi,\n> - match->wc.masks.nsh.spi);\n> - nxm_put_8m(&ctx, MFF_NSH_SI, oxm, flow->nsh.si, match-\n> >wc.masks.nsh.si);\n> + spi_mask = nsh_path_hdr_to_spi(match->wc.masks.nsh.path_hdr);\n> + if (spi_mask == htonl(NSH_SPI_MASK >> NSH_SPI_SHIFT)) {\n> + spi_mask = OVS_BE32_MAX;\n> + }\n> + nxm_put_32m(&ctx, MFF_NSH_SPI, oxm,\n> + nsh_path_hdr_to_spi(flow->nsh.path_hdr),\n> + spi_mask);\n\nI think it is not correct to expand a 24 bit all-ones mask for the SPI field to a full 32-bit all-ones mask. Compare to the MFF_IPV6_LABEL case which is defined as a 20-bit field in a 4-byte OXM. The codes just puts the mask as is.\n\n> diff --git a/lib/odp-execute.c b/lib/odp-execute.c\n> index 5f4d23a..601c8b6 100644\n> --- a/lib/odp-execute.c\n> +++ b/lib/odp-execute.c\n> @@ -277,10 +277,10 @@ odp_set_nsh(struct dp_packet *packet, const\n> struct ovs_key_nsh *key,\n> const struct ovs_key_nsh *mask)\n> {\n> struct nsh_hdr *nsh = dp_packet_l3(packet);\n> + ovs_be32 path_hdr;\n> \n> if (!mask) {\n> - nsh->ver_flags_len = htons(key->flags << NSH_FLAGS_SHIFT) |\n> - (nsh->ver_flags_len & ~htons(NSH_FLAGS_MASK));\n> + nsh_set_flags_and_ttl(nsh, key->flags, key->ttl);\n> put_16aligned_be32(&nsh->path_hdr, key->path_hdr);\n> switch (nsh->md_type) {\n> case NSH_M_TYPE1:\n> @@ -294,15 +294,28 @@ odp_set_nsh(struct dp_packet *packet, const\n[...]\n> } else {\n> - uint8_t flags = (ntohs(nsh->ver_flags_len) & NSH_FLAGS_MASK) >>\n> - NSH_FLAGS_SHIFT;\n> + uint8_t flags = nsh_get_flags(nsh);\n> + uint8_t ttl = nsh_get_ttl(nsh);\n> +\n> flags = key->flags | (flags & ~mask->flags);\n> - nsh->ver_flags_len = htons(flags << NSH_FLAGS_SHIFT) |\n> - (nsh->ver_flags_len & ~htons(NSH_FLAGS_MASK));\n> + ttl = key->ttl | (ttl & ~mask->ttl);\n> + nsh_set_flags_and_ttl (nsh, flags, ttl);\n> +\n> + uint32_t spi = ntohl(nsh_get_spi(nsh));\n> + uint8_t si = nsh_get_si(nsh);\n> + uint32_t spi_mask = nsh_path_hdr_to_spi_uint32(mask->path_hdr);\n> + uint8_t si_mask = nsh_path_hdr_to_si(mask->path_hdr);\n> \n> - ovs_be32 path_hdr = get_16aligned_be32(&nsh->path_hdr);\n> - path_hdr = key->path_hdr | (path_hdr & ~mask->path_hdr);\n> + if (spi_mask == 0x00ffffff) {\n> + spi_mask = UINT32_MAX;\n> + }\n> + spi = nsh_path_hdr_to_spi_uint32(key->path_hdr) | (spi &\n> ~spi_mask);\n> + si = nsh_path_hdr_to_si(key->path_hdr) | (si & ~si_mask);\n> + path_hdr = nsh_get_path_hdr(nsh);\n> + nsh_path_hdr_set_spi(&path_hdr, htonl(spi));\n> + nsh_path_hdr_set_si(&path_hdr, si);\n> put_16aligned_be32(&nsh->path_hdr, path_hdr);\n\nThe above is quite obscure and far more complicated than necessary.\nCould be simplified as follows: \n\n uint8_t flags = nsh_get_flags(nsh);\n uint8_t ttl = nsh_get_ttl(nsh);\n ovs_be32 path_hdr = nsh_get_path_hdr(nsh);\n\n flags = key->flags | (flags & ~mask->flags);\n ttl = key->ttl | (ttl & ~mask->ttl);\n path_hdr = key->path_hdr | (path_hdr & ~mask->path_hdr);\n\n nsh_set_flags(nsh, flags);\n nsh_set_ttl(nsh, ttl);\n nsh_set_path_hdr(nsh, path_hdr);\n\n> diff --git a/lib/odp-util.c b/lib/odp-util.c\n> index 4f1499e..ac286ea 100644\n> --- a/lib/odp-util.c\n> +++ b/lib/odp-util.c\n> @@ -319,17 +320,16 @@ format_nsh_key_mask(struct ds *ds, const struct\n> ovs_key_nsh *key,\n> format_nsh_key(ds, key);\n> } else {\n> bool first = true;\n> - uint32_t spi = (ntohl(key->path_hdr) & NSH_SPI_MASK) >>\n> NSH_SPI_SHIFT;\n> - uint32_t spi_mask = (ntohl(mask->path_hdr) & NSH_SPI_MASK) >>\n> - NSH_SPI_SHIFT;\n> - if (spi_mask == 0x00ffffff) {\n> + uint32_t spi = nsh_path_hdr_to_spi_uint32(key->path_hdr);\n> + uint32_t spi_mask = nsh_path_hdr_to_spi_uint32(mask->path_hdr);\n> + if (spi_mask == (NSH_SPI_MASK >> NSH_SPI_SHIFT)) {\n> spi_mask = UINT32_MAX;\n> }\n\nI understand that you expand the mask here to 32 bits to be able to use format_be32_masked helper below, but it is confusing anyhow. Perhaps it\nwould be clearer to introduce a dedicated formatting function for SPI.\n\n> - uint8_t si = (ntohl(key->path_hdr) & NSH_SI_MASK) >> NSH_SI_SHIFT;\n> - uint8_t si_mask = (ntohl(mask->path_hdr) & NSH_SI_MASK) >>\n> - NSH_SI_SHIFT;\n> + uint8_t si = nsh_path_hdr_to_si(key->path_hdr);\n> + uint8_t si_mask = nsh_path_hdr_to_si(mask->path_hdr);\n> \n> format_uint8_masked(ds, &first, \"flags\", key->flags, mask->flags);\n> + format_uint8_masked(ds, &first, \"ttl\", key->ttl, mask->ttl);\n> format_uint8_masked(ds, &first, \"mdtype\", key->mdtype, mask-\n> >mdtype);\n> format_uint8_masked(ds, &first, \"np\", key->np, mask->np);\n> format_be32_masked(ds, &first, \"spi\", htonl(spi), htonl(spi_mask));\n\n> @@ -1808,17 +1810,20 @@ parse_odp_encap_nsh_action(const char *s,\n> struct ofpbuf *actions)\n> break;\n> }\n> \n> - if (ovs_scan_len(s, &n, \"flags=%\"SCNi8, &encap_nsh.flags)) {\n> + if (ovs_scan_len(s, &n, \"flags=%\"SCNi8, &encap_nsh->flags)) {\n> continue;\n\nAny validity check for value flags<=3?\n\n> }\n> - if (ovs_scan_len(s, &n, \"mdtype=%\"SCNi8, &encap_nsh.mdtype)) {\n> - switch (encap_nsh.mdtype) {\n> + if (ovs_scan_len(s, &n, \"ttl=%\"SCNi8, &encap_nsh->ttl)) {\n> + continue;\n\nAny validity check for ttl < 64?\n\n> @@ -1826,24 +1831,24 @@ parse_odp_encap_nsh_action(const char *s,\n> struct ofpbuf *actions)\n> }\n> continue;\n> }\n> - if (ovs_scan_len(s, &n, \"np=%\"SCNi8, &encap_nsh.np)) {\n> + if (ovs_scan_len(s, &n, \"np=%\"SCNi8, &encap_nsh->np)) {\n> continue;\n> }\n> if (ovs_scan_len(s, &n, \"spi=0x%\"SCNx32, &spi)) {\n> - encap_nsh.path_hdr =\n> + encap_nsh->path_hdr =\n> htonl(((spi << NSH_SPI_SHIFT) & NSH_SPI_MASK) |\n> - (ntohl(encap_nsh.path_hdr) & ~NSH_SPI_MASK));\n> + (ntohl(encap_nsh->path_hdr) & ~NSH_SPI_MASK));\n> continue;\n\nWhy not use the access functions in nsh.h here?\n\n> }\n> if (ovs_scan_len(s, &n, \"si=%\"SCNi8, &si)) {\n> - encap_nsh.path_hdr =\n> + encap_nsh->path_hdr =\n> htonl((si << NSH_SI_SHIFT) |\n> - (ntohl(encap_nsh.path_hdr) & ~NSH_SI_MASK));\n> + (ntohl(encap_nsh->path_hdr) & ~NSH_SI_MASK));\n> continue;\n\nDito.\n\n> @@ -1861,28 +1866,29 @@ parse_odp_encap_nsh_action(const char *s,\n> struct ofpbuf *actions)\n> continue;\n> }\n> }\n> - else if (encap_nsh.mdtype == NSH_M_TYPE2) {\n> + else if (encap_nsh->mdtype == NSH_M_TYPE2) {\n> struct ofpbuf b;\n> char buf[512];\n> size_t mdlen;\n> if (ovs_scan_len(s, &n, \"md2=0x%511[0-9a-fA-F]\", buf)) {\n> - ofpbuf_use_stub(&b, encap_nsh.metadata,\n> - OVS_ENCAP_NSH_MAX_MD_LEN);\n> + ofpbuf_use_stub(&b, encap_nsh->metadata,\n> + NSH_CTX_HDRS_MAX_LEN);\n> ofpbuf_put_hex(&b, buf, &mdlen);\n\nHere it might be necessary to pad the metadata buffer with zeros to 4 bytes.\n\n> @@ -6798,19 +6774,20 @@ odp_put_encap_nsh_action(struct ofpbuf\n> *odp_actions,\n[...]\n> - switch (encap_nsh.mdtype) {\n> + switch (encap_nsh->mdtype) {\n> case NSH_M_TYPE1: {\n> struct nsh_md1_ctx *md1 =\n> - ALIGNED_CAST(struct nsh_md1_ctx *, encap_nsh.metadata);\n> - encap_nsh.mdlen = NSH_M_TYPE1_MDLEN;\n> + ALIGNED_CAST(struct nsh_md1_ctx *, encap_nsh->metadata);\n> + encap_nsh->mdlen = NSH_M_TYPE1_MDLEN;\n> for (int i = 0; i < 4; i++) {\n> put_16aligned_be32(&md1->c[i], flow->nsh.c[i]);\n\nNo need for put_16aligned_be32() here as the encap_nsh action and the metadata included are 8-byte aligned.\n\n> diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c\n> index 9e1f837..1b99ad5 100644\n> --- a/ofproto/ofproto-dpif-xlate.c\n> +++ b/ofproto/ofproto-dpif-xlate.c\n> @@ -5885,10 +5885,10 @@ rewrite_flow_encap_nsh(struct xlate_ctx *ctx,\n> /* Populate the flow with the new NSH header. */\n> flow->packet_type = htonl(PT_NSH);\n> flow->dl_type = htons(ETH_TYPE_NSH);\n> - flow->nsh.flags = 0; /* */\n> + flow->nsh.flags = 0;\n> + flow->nsh.ttl = 63;\n> flow->nsh.np = np;\n> - flow->nsh.spi = 0;\n> - flow->nsh.si = 255;\n> + flow->nsh.path_hdr = htonl(255);\n\nUse nsh setter functions instead?\n\n> @@ -5901,6 +5901,7 @@ rewrite_flow_encap_nsh(struct xlate_ctx *ctx,\n> } else if (md_type == NSH_M_TYPE2) {\n> flow->nsh.mdtype = NSH_M_TYPE2;\n> }\n> + flow->nsh.mdtype &= NSH_MDTYPE_MASK;\n\nNot really needed here, as we set the mdtype to a defined value.\n\nI didn't check the unit test adaptations to the introduction of ttl field.\n\nBR, Jan","headers":{"Return-Path":"","X-Original-To":["incoming@patchwork.ozlabs.org","dev@openvswitch.org"],"Delivered-To":["patchwork-incoming@bilbo.ozlabs.org","ovs-dev@mail.linuxfoundation.org"],"Authentication-Results":"ozlabs.org;\n\tspf=pass (mailfrom) smtp.mailfrom=openvswitch.org\n\t(client-ip=140.211.169.12; helo=mail.linuxfoundation.org;\n\tenvelope-from=ovs-dev-bounces@openvswitch.org;\n\treceiver=)","Received":["from mail.linuxfoundation.org (mail.linuxfoundation.org\n\t[140.211.169.12])\n\t(using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256\n\tbits)) (No client certificate requested)\n\tby ozlabs.org (Postfix) with ESMTPS id 3xh7MK6nDyz9sNr\n\tfor ;\n\tTue, 29 Aug 2017 09:31:41 +1000 (AEST)","from mail.linux-foundation.org (localhost [127.0.0.1])\n\tby mail.linuxfoundation.org (Postfix) with ESMTP id 9A7C487A;\n\tMon, 28 Aug 2017 23:31:28 +0000 (UTC)","from smtp1.linuxfoundation.org (smtp1.linux-foundation.org\n\t[172.17.192.35])\n\tby mail.linuxfoundation.org (Postfix) with ESMTPS id C4DE887A\n\tfor ; Mon, 28 Aug 2017 23:31:27 +0000 (UTC)","from sesbmg23.ericsson.net (sesbmg23.ericsson.net [193.180.251.37])\n\tby smtp1.linuxfoundation.org (Postfix) with ESMTPS id F23BF79\n\tfor ; Mon, 28 Aug 2017 23:31:25 +0000 (UTC)","from ESESSHC005.ericsson.se (Unknown_Domain [153.88.183.33])\n\tby sesbmg23.ericsson.net (Symantec Mail Security) with SMTP id\n\t7A.B0.21299.CC7A4A95; Tue, 29 Aug 2017 01:31:24 +0200 (CEST)","from ESESSMB107.ericsson.se ([169.254.7.26]) by\n\tESESSHC005.ericsson.se ([153.88.183.33]) with mapi id 14.03.0352.000; \n\tTue, 29 Aug 2017 01:31:23 +0200"],"X-Greylist":"domain auto-whitelisted by SQLgrey-1.7.6","X-AuditID":"c1b4fb25-94fff70000005333-c4-59a4a7ccf836","From":"Jan Scheurich ","To":"Yi Yang ,\n\t\"dev@openvswitch.org\" ","Thread-Topic":"[PATCH v4 2/3] nsh: add new flow key 'ttl'","Thread-Index":"AQHTHVeXt1MQLcjG70KsWi3CUoBQEqKZvTRA","Date":"Mon, 28 Aug 2017 23:31:22 +0000","Message-ID":"","References":"<1503633771-112384-1-git-send-email-yi.y.yang@intel.com>\n\t<1503633771-112384-3-git-send-email-yi.y.yang@intel.com>","In-Reply-To":"<1503633771-112384-3-git-send-email-yi.y.yang@intel.com>","Accept-Language":"en-US","Content-Language":"en-US","X-MS-Has-Attach":"","X-MS-TNEF-Correlator":"","x-originating-ip":"[153.88.183.149]","MIME-Version":"1.0","X-Brightmail-Tracker":"H4sIAAAAAAAAA+NgFmpjkeLIzCtJLcpLzFFi42KZGbFdUffM8iWRBpe6BCxeTW5gtDh6eg+z\n\txe+v25gcmD0W73nJ5PHs5n9Gj+fXelgCmKO4bFJSczLLUov07RK4Mjb99ipYNJexomHnE8YG\n\txheFXYycHBICJhLtcz4ydTFycQgJHGGUuHDgHCOEs5hR4tSHZcxdjBwcbAIGErN3O4A0iAj4\n\tSRy51McEYjMLyEs0rdrKBmILC5hL7Oj/wAxRYyHx9cMhNgjbSGLn+YlgNouAqsTT34fBenkF\n\tfCW+PjgOFhcSqJd48Pw6I4jNKeAqMenRYnYQm1FATOL7qTVQu8Qlbj2ZzwRxtIDEkj3nmSFs\n\tUYmXj/+xQthKEmsPb2eBqNeRWLD7ExuErS2xbOFrZoi9ghInZz5hmcAoOgvJ2FlIWmYhaZmF\n\tpGUBI8sqRtHi1OKk3HQjY73Uoszk4uL8PL281JJNjMDYObjlt+oOxstvHA8xCnAwKvHw2k9f\n\tEinEmlhWXJl7iFGCg1lJhDduDlCINyWxsiq1KD++qDQntfgQozQHi5I4r+O+CxFCAumJJanZ\n\tqakFqUUwWSYOTqkGxuRr/L5hV7dyckx44xa0p1/HLKZfKl5M63uSR0v/rgmC0Yc/J3RGM2me\n\tM5NPvyFuM5ufh11859JV3tmPqia8Keno2a1c9dGx2fHiPzcLb6WWiOhku7BGRoGlut9Xnw7Y\n\tLWcZzP9gxzOvh2XF81OXvdvwZMab6Nzl/M/FFk/NuLToV+227crHlViKMxINtZiLihMBrai+\n\tcpkCAAA=","X-Spam-Status":"No, score=-2.3 required=5.0 tests=RCVD_IN_DNSWL_MED\n\tautolearn=disabled version=3.3.1","X-Spam-Checker-Version":"SpamAssassin 3.3.1 (2010-03-16) on\n\tsmtp1.linux-foundation.org","Subject":"Re: [ovs-dev] [PATCH v4 2/3] nsh: add new flow key 'ttl'","X-BeenThere":"ovs-dev@openvswitch.org","X-Mailman-Version":"2.1.12","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Sender":"ovs-dev-bounces@openvswitch.org","Errors-To":"ovs-dev-bounces@openvswitch.org"}},{"id":1759060,"web_url":"http://patchwork.ozlabs.org/comment/1759060/","msgid":"<20170829063035.GA88227@cran64.bj.intel.com>","list_archive_url":null,"date":"2017-08-29T06:30:35","subject":"Re: [ovs-dev] [PATCH v4 2/3] nsh: add new flow key 'ttl'","submitter":{"id":68962,"url":"http://patchwork.ozlabs.org/api/people/68962/","name":"Yang, Yi","email":"yi.y.yang@intel.com"},"content":"On Tue, Aug 29, 2017 at 07:31:22AM +0800, Jan Scheurich wrote:\n> Hi Yi,\n> \n> Thanks for the fixes. Please find my individual comments inline.\n> \n> BR, Jan\n> \n> > -----Original Message-----\n> > -#define OVS_ENCAP_NSH_MAX_MD_LEN 16\n> > /*\n> > * struct ovs_action_encap_nsh - %OVS_ACTION_ATTR_ENCAP_NSH\n> > * @flags: NSH header flags.\n> > + * @ttl: NSH header TTL.\n> > * @mdtype: NSH metadata type.\n> > * @mdlen: Length of NSH metadata in bytes.\n> > * @np: NSH next_protocol: Inner packet type.\n> > @@ -805,11 +796,13 @@ struct ovs_action_push_eth {\n> > */\n> > struct ovs_action_encap_nsh {\n> > uint8_t flags;\n> > + uint8_t ttl;\n> > uint8_t mdtype;\n> > - uint8_t mdlen;\n> > uint8_t np;\n> > __be32 path_hdr;\n> > - uint8_t metadata[OVS_ENCAP_NSH_MAX_MD_LEN];\n> > + uint8_t mdlen;\n> > + uint8_t pad[7]; /* Aligned to 64 bit boundary for metadata */\n> > + uint8_t metadata[];\n> > };\n> \n> Alignment of metadata to 32 bits should suffice here. Netlink attrs are only 4-byte aligned anyhow. The same goes for the MD1 context headers inside metadata.\n>\n\nOk, I will change it to pad[3].\n\n> > diff --git a/include/openvswitch/flow.h b/include/openvswitch/flow.h\n> > index a658a58..cd61fff 100644\n> > --- a/include/openvswitch/flow.h\n> > +++ b/include/openvswitch/flow.h\n> > @@ -146,7 +146,7 @@ struct flow {\n> > struct eth_addr arp_tha; /* ARP/ND target hardware address. */\n> > ovs_be16 tcp_flags; /* TCP flags. With L3 to avoid matching L4. */\n> > ovs_be16 pad2; /* Pad to 64 bits. */\n> > - struct flow_nsh nsh; /* Network Service Header keys */\n> > + struct ovs_key_nsh nsh; /* Network Service Header keys */\n> \n> Why rename this type to struct ovs_key_nsh?\n\nI have explained it in previous review, new ttl field results in it\nisn't 64 bit aligned, moreover, we musnt't do some unnecessary\nconversion if we use struct ovs_key_nsh, netlink also used struct\novs_key_nsh, it is very reasonable to use struct ovs_key_nsh for this,\nwhy don't we use it?\n\n> \n> > diff --git a/include/openvswitch/nsh.h b/include/openvswitch/nsh.h\n> [...]\n> > @@ -62,7 +210,7 @@ struct nsh_md2_tlv {\n> > };\n> > \n> > struct nsh_hdr {\n> > - ovs_be16 ver_flags_len;\n> > + ovs_be16 ver_flags_ttl_len;\n> > uint8_t md_type;\n> > uint8_t next_proto;\n> > ovs_16aligned_be32 path_hdr;\n> > @@ -75,11 +223,16 @@ struct nsh_hdr {\n> > /* Masking NSH header fields. */\n> > #define NSH_VER_MASK 0xc000\n> > #define NSH_VER_SHIFT 14\n> > -#define NSH_FLAGS_MASK 0x3fc0\n> > -#define NSH_FLAGS_SHIFT 6\n> > +#define NSH_FLAGS_MASK 0x3000\n> > +#define NSH_FLAGS_SHIFT 12\n> \n> With this definition of NSH FLAGS the match field NSH_FLAGS covers\n> only the two bits between Version and TTL field. Bit 1 is the OaM flag, \n> bit 0 is currently unused. The remaining 4 unused bits between Length\n> and MD Type fields are excluded. That is perhaps OK as we cannot be\n> sure the these 4 bits will be used as \"flags\" in the future. But it needs\n> to be documented clearly that NSH_FLAGS has only two bits and which\n> bits in the NSH header it covers.\n\nMake sense, we can add more information for this in lib/meta-flow.xml.\n\n> \n> > +static inline void\n> > +__nsh_set_xflag(struct nsh_hdr *nsh, uint16_t xflag, uint16_t xmask)\n> > +{\n> > + nsh->ver_flags_ttl_len\n> > + = (nsh->ver_flags_ttl_len & ~htons(xmask)) | htons(xflag);\n> > +}\n> \n> Avoid __* prefix. Better call it nsh_set_base_hdr_masked(). \n\nWe just use __ to mark it as an internal function which will not be used\noutside of nsh.h, this is C coding convention.\n\n> \n> > +\n> > +static inline void nsh_set_flags_and_ttl(struct nsh_hdr *nsh, uint8_t flags,\n> > + uint8_t ttl)\n> > +{\n> > + __nsh_set_xflag(nsh, ((flags << NSH_FLAGS_SHIFT) &\n> > NSH_FLAGS_MASK) |\n> > + ((ttl << NSH_TTL_SHIFT) & NSH_TTL_MASK),\n> > + NSH_FLAGS_MASK | NSH_TTL_MASK);\n> > +}\n> > +\n> > +static inline void nsh_set_flags_ttl_len(struct nsh_hdr *nsh, uint8_t flags,\n> > + uint8_t ttl, uint8_t len)\n> > +{\n> > + len = len >> 2;\n> > + __nsh_set_xflag(nsh, ((flags << NSH_FLAGS_SHIFT) &\n> > NSH_FLAGS_MASK) |\n> > + ((ttl << NSH_TTL_SHIFT) & NSH_TTL_MASK) |\n> > + ((len << NSH_LEN_SHIFT) & NSH_LEN_MASK),\n> > + NSH_FLAGS_MASK | NSH_TTL_MASK | NSH_LEN_MASK);\n> > +}\n> \n> Do you really need to these hybrid functions? Why not use modular functions nsh_set_flags(), nsh_set_ttl() and nsh_set_len()? I believe the run-time overhead is negligible with an optimizing compiler.\n\nBasically these three functions will be executed together, so one is\nbetter.\n\n> \n> For symmetry you should also add a function setter function for path_hdr:\n\nI think put_16aligned_be32(&nsh->path_hdr, path_hdr) is better than\nnsh_set_path_hdr, here isn't C++ programming :-)\n\n> \n> static inline void\n> nsh_set_path_hdr(struct nsh_hdr *nsh, ovs_be32 path_hdr)\n> {\n> put_16aligned_be32(&nsh->path_hdr, path_hdr);\n> }\n> \n> > diff --git a/include/openvswitch/packets.h b/include/openvswitch/packets.h\n> > index be91e02..603ec44 100644\n> > --- a/include/openvswitch/packets.h\n> > +++ b/include/openvswitch/packets.h\n> >\n> > /* Network Service Header keys */\n> > -struct flow_nsh {\n> > +struct ovs_key_nsh {\n> > uint8_t flags;\n> > + uint8_t ttl;\n> > uint8_t mdtype;\n> > uint8_t np;\n> > - uint8_t si;\n> > - ovs_be32 spi;\n> > + ovs_be32 path_hdr;\n> > ovs_be32 c[4];\n> > };\n> \n> Why did you rename this struct? As part of the struct flow, it was\n> more appropriately called struct flow_nsh.\n\nIt isn't name issue, we want to use common struct ovs_key_nsh in\nmultiple function modules.\n\n> \n> > diff --git a/lib/flow.c b/lib/flow.c\n> > index b2b10aa..547ff70 100644\n> > --- a/lib/flow.c\n> > +++ b/lib/flow.c\n> > bool\n> > -parse_nsh(const void **datap, size_t *sizep, struct flow_nsh *key)\n> > +parse_nsh(const void **datap, size_t *sizep, struct ovs_key_nsh *key)\n> > {\n> [...]\n> > - /* Check if it is long enough for NSH header, doesn't support\n> > - * MD type 2 yet\n> > - */\n> > if (OVS_UNLIKELY(*sizep < NSH_M_TYPE1_LEN)) {\n> > return false;\n> > }\n> \n> This should compare against the minimum fixed size (NSH_BASE_HDR_LEN)\n> of an NSH header, not NSH_M_TYPE1_LEN as we want to successfully parse \n> any MD type header.\n\nMy bad, I will use NSH_BASE_HDR_LEN.\n\n> \n> > switch (key->mdtype) {\n> > case NSH_M_TYPE1:\n> > + if (length != NSH_M_TYPE1_LEN) {\n> > + return false;\n> > + }\n> > for (size_t i = 0; i < 4; i++) {\n> > key->c[i] = get_16aligned_be32(&nsh->md1.c[i]);\n> > }\n> > break;\n> > case NSH_M_TYPE2:\n> > - /* Don't support MD type 2 yet, so return false */\n> > + /* Don't support MD type 2 metedata parsing yet */\n> > + if (length < NSH_BASE_HDR_LEN) {\n> > + return false;\n> > + }\n> \n> This check is superfluous as it has been done before.\n\nYeah, I'll remove it.\n\n> \n> > diff --git a/lib/meta-flow.c b/lib/meta-flow.c\n> > index 64a8cf1..b50679c 100644\n> > --- a/lib/meta-flow.c\n> > +++ b/lib/meta-flow.c\n> > @@ -906,10 +914,13 @@ mf_get_value(const struct mf_field *mf, const\n> > struct flow *flow,\n> > value->u8 = flow->nsh.np;\n> > break;\n> > case MFF_NSH_SPI:\n> > - value->be32 = flow->nsh.spi;\n> > + value->be32 = nsh_path_hdr_to_spi(flow->nsh.path_hdr);\n> > + if (value->be32 == htonl(NSH_SPI_MASK >> NSH_SPI_SHIFT)) {\n> > + value->be32 = OVS_BE32_MAX;\n> > + }\n> \n> This looks strange. The SPI is a 24 bit field. Why would you expand it to OVS_BE32_MAX?\n\nIndeed, 24 bit mask is enough, I'll remove it.\n\n> \n> > @@ -1221,10 +1235,12 @@ mf_set_value(const struct mf_field *mf,\n> > MATCH_SET_FIELD_UINT8(match, nsh.np, value->u8);\n> > break;\n> > case MFF_NSH_SPI:\n> > - MATCH_SET_FIELD_BE32(match, nsh.spi, value->be32);\n> > + match->wc.masks.nsh.path_hdr |= htonl(NSH_SPI_MASK);\n> > + nsh_path_hdr_set_spi(&match->flow.nsh.path_hdr, value->be32);\n> \n> Introduce a wrapper function match_set_nsh_spi() where you rely on the setter functions in nsh.h to manipulate the path_hdr.\n\nMake sense, I have done in new version.\n\n> \n> > break;\n> > case MFF_NSH_SI:\n> > - MATCH_SET_FIELD_UINT8(match, nsh.si, value->u8);\n> > + match->wc.masks.nsh.path_hdr |= htonl(NSH_SI_MASK);\n> > + nsh_path_hdr_set_si(&match->flow.nsh.path_hdr, value->u8);\n> > break;\n> \n> Dito.\n> \n> > @@ -2103,10 +2126,12 @@ mf_set_wild(const struct mf_field *mf, struct\n> > match *match, char **err_str)\n> > MATCH_SET_FIELD_MASKED(match, nsh.np, 0, 0);\n> > break;\n> > case MFF_NSH_SPI:\n> > - MATCH_SET_FIELD_MASKED(match, nsh.spi, htonl(0), htonl(0));\n> > + match->wc.masks.nsh.path_hdr &= ~htonl(NSH_SPI_MASK);\n> > + nsh_path_hdr_set_spi(&match->flow.nsh.path_hdr, htonl(0));\n> \n> \n> Dito.\n> \n> > break;\n> > case MFF_NSH_SI:\n> > - MATCH_SET_FIELD_MASKED(match, nsh.si, 0, 0);\n> > + match->wc.masks.nsh.path_hdr &= ~htonl(NSH_SI_MASK);\n> > + nsh_path_hdr_set_si(&match->flow.nsh.path_hdr, 0);\n> \n> Dito.\n> \n> > break;\n> > case MFF_NSH_C1:\n> > case MFF_NSH_C2:\n> \n> > @@ -2363,10 +2391,14 @@ mf_set(const struct mf_field *mf,\n> > MATCH_SET_FIELD_MASKED(match, nsh.np, value->u8, mask->u8);\n> > break;\n> > case MFF_NSH_SPI:\n> > - MATCH_SET_FIELD_MASKED(match, nsh.spi, value->be32, mask-\n> > >be32);\n> > + match->wc.masks.nsh.path_hdr |= mask->be32;\n> \n> I believe this is incorrect. The mask is related to the value and requires shifting.\n> One more reason to use wrapper functions.\n\nYou're right, but 4 nsh unit tests are ok, I'm not sure why. I have used\nwrapper function to do this in new version.\n\n> \n> > + nsh_path_hdr_set_spi(&match->flow.nsh.path_hdr,\n> > + value->be32 & mask->be32);\n> > break;\n> > case MFF_NSH_SI:\n> > - MATCH_SET_FIELD_MASKED(match, nsh.si, value->u8, mask->u8);\n> > + match->wc.masks.nsh.path_hdr |= htonl(mask->u8);\n> \n> Dito.\n> \n> > + nsh_path_hdr_set_si(&match->flow.nsh.path_hdr,\n> > + value->u8 & mask->u8);\n> \n> > diff --git a/lib/meta-flow.xml b/lib/meta-flow.xml\n> > index 065fb03..a4a0735 100644\n> > --- a/lib/meta-flow.xml\n> > +++ b/lib/meta-flow.xml\n> > @@ -1311,7 +1311,9 @@ tcp,tp_src=0x07c0/0xfff0\n> > \n> > \n> > > - title=\"flags field (8 bits)\"/>\n> > + title=\"flags field (2 bits)\"/>\n> \n> The NSH_FLAGS field requires more explanation as it is not defined in the RFC.\n\nOk, I'll add more information here.\n\n> \n> > diff --git a/lib/nx-match.c b/lib/nx-match.c\n> > index b782e8c..dc52566 100644\n> > --- a/lib/nx-match.c\n> > +++ b/lib/nx-match.c\n> > @@ -1157,13 +1158,22 @@ nx_put_raw(struct ofpbuf *b, enum\n> > ofp_version oxm, const struct match *match,\n> > /* Network Service Header */\n> > nxm_put_8m(&ctx, MFF_NSH_FLAGS, oxm, flow->nsh.flags,\n> > match->wc.masks.nsh.flags);\n> > + nxm_put_8m(&ctx, MFF_NSH_TTL, oxm, flow->nsh.ttl,\n> > + match->wc.masks.nsh.ttl);\n> > nxm_put_8m(&ctx, MFF_NSH_MDTYPE, oxm, flow->nsh.mdtype,\n> > match->wc.masks.nsh.mdtype);\n> > nxm_put_8m(&ctx, MFF_NSH_NP, oxm, flow->nsh.np,\n> > match->wc.masks.nsh.np);\n> > - nxm_put_32m(&ctx, MFF_NSH_SPI, oxm, flow->nsh.spi,\n> > - match->wc.masks.nsh.spi);\n> > - nxm_put_8m(&ctx, MFF_NSH_SI, oxm, flow->nsh.si, match-\n> > >wc.masks.nsh.si);\n> > + spi_mask = nsh_path_hdr_to_spi(match->wc.masks.nsh.path_hdr);\n> > + if (spi_mask == htonl(NSH_SPI_MASK >> NSH_SPI_SHIFT)) {\n> > + spi_mask = OVS_BE32_MAX;\n> > + }\n> > + nxm_put_32m(&ctx, MFF_NSH_SPI, oxm,\n> > + nsh_path_hdr_to_spi(flow->nsh.path_hdr),\n> > + spi_mask);\n> \n> I think it is not correct to expand a 24 bit all-ones mask for the SPI field to a full 32-bit all-ones mask. Compare to the MFF_IPV6_LABEL case which is defined as a 20-bit field in a 4-byte OXM. The codes just puts the mask as is.\n\nMFF_IPV6_LABEL is different from this, MPLS_LABEL is same as this one.\nBut it will return BAD_MASK error if we don't expand it ot OVS_BE32_MAX\nbecause mf_is_mask_valid expects mask is all one, the actual mask is\n0x00FFFFFF. I have tried this many times, please do tell me a better solution\nfor this if you have a better one.\n\n> \n> > diff --git a/lib/odp-execute.c b/lib/odp-execute.c\n> > index 5f4d23a..601c8b6 100644\n> > --- a/lib/odp-execute.c\n> > +++ b/lib/odp-execute.c\n> > @@ -277,10 +277,10 @@ odp_set_nsh(struct dp_packet *packet, const\n> > struct ovs_key_nsh *key,\n> > const struct ovs_key_nsh *mask)\n> > {\n> > struct nsh_hdr *nsh = dp_packet_l3(packet);\n> > + ovs_be32 path_hdr;\n> > \n> > if (!mask) {\n> > - nsh->ver_flags_len = htons(key->flags << NSH_FLAGS_SHIFT) |\n> > - (nsh->ver_flags_len & ~htons(NSH_FLAGS_MASK));\n> > + nsh_set_flags_and_ttl(nsh, key->flags, key->ttl);\n> > put_16aligned_be32(&nsh->path_hdr, key->path_hdr);\n> > switch (nsh->md_type) {\n> > case NSH_M_TYPE1:\n> > @@ -294,15 +294,28 @@ odp_set_nsh(struct dp_packet *packet, const\n> [...]\n> > } else {\n> > - uint8_t flags = (ntohs(nsh->ver_flags_len) & NSH_FLAGS_MASK) >>\n> > - NSH_FLAGS_SHIFT;\n> > + uint8_t flags = nsh_get_flags(nsh);\n> > + uint8_t ttl = nsh_get_ttl(nsh);\n> > +\n> > flags = key->flags | (flags & ~mask->flags);\n> > - nsh->ver_flags_len = htons(flags << NSH_FLAGS_SHIFT) |\n> > - (nsh->ver_flags_len & ~htons(NSH_FLAGS_MASK));\n> > + ttl = key->ttl | (ttl & ~mask->ttl);\n> > + nsh_set_flags_and_ttl (nsh, flags, ttl);\n> > +\n> > + uint32_t spi = ntohl(nsh_get_spi(nsh));\n> > + uint8_t si = nsh_get_si(nsh);\n> > + uint32_t spi_mask = nsh_path_hdr_to_spi_uint32(mask->path_hdr);\n> > + uint8_t si_mask = nsh_path_hdr_to_si(mask->path_hdr);\n> > \n> > - ovs_be32 path_hdr = get_16aligned_be32(&nsh->path_hdr);\n> > - path_hdr = key->path_hdr | (path_hdr & ~mask->path_hdr);\n> > + if (spi_mask == 0x00ffffff) {\n> > + spi_mask = UINT32_MAX;\n> > + }\n> > + spi = nsh_path_hdr_to_spi_uint32(key->path_hdr) | (spi &\n> > ~spi_mask);\n> > + si = nsh_path_hdr_to_si(key->path_hdr) | (si & ~si_mask);\n> > + path_hdr = nsh_get_path_hdr(nsh);\n> > + nsh_path_hdr_set_spi(&path_hdr, htonl(spi));\n> > + nsh_path_hdr_set_si(&path_hdr, si);\n> > put_16aligned_be32(&nsh->path_hdr, path_hdr);\n> \n> The above is quite obscure and far more complicated than necessary.\n> Could be simplified as follows: \n> \n> uint8_t flags = nsh_get_flags(nsh);\n> uint8_t ttl = nsh_get_ttl(nsh);\n> ovs_be32 path_hdr = nsh_get_path_hdr(nsh);\n> \n> flags = key->flags | (flags & ~mask->flags);\n> ttl = key->ttl | (ttl & ~mask->ttl);\n> path_hdr = key->path_hdr | (path_hdr & ~mask->path_hdr);\n> \n> nsh_set_flags(nsh, flags);\n> nsh_set_ttl(nsh, ttl);\n> nsh_set_path_hdr(nsh, path_hdr);\n\nI tried the one you provided, it is ok, so I changed it to your version\nin new version.\n\n> \n> > diff --git a/lib/odp-util.c b/lib/odp-util.c\n> > index 4f1499e..ac286ea 100644\n> > --- a/lib/odp-util.c\n> > +++ b/lib/odp-util.c\n> > @@ -319,17 +320,16 @@ format_nsh_key_mask(struct ds *ds, const struct\n> > ovs_key_nsh *key,\n> > format_nsh_key(ds, key);\n> > } else {\n> > bool first = true;\n> > - uint32_t spi = (ntohl(key->path_hdr) & NSH_SPI_MASK) >>\n> > NSH_SPI_SHIFT;\n> > - uint32_t spi_mask = (ntohl(mask->path_hdr) & NSH_SPI_MASK) >>\n> > - NSH_SPI_SHIFT;\n> > - if (spi_mask == 0x00ffffff) {\n> > + uint32_t spi = nsh_path_hdr_to_spi_uint32(key->path_hdr);\n> > + uint32_t spi_mask = nsh_path_hdr_to_spi_uint32(mask->path_hdr);\n> > + if (spi_mask == (NSH_SPI_MASK >> NSH_SPI_SHIFT)) {\n> > spi_mask = UINT32_MAX;\n> > }\n> \n> I understand that you expand the mask here to 32 bits to be able to use format_be32_masked helper below, but it is confusing anyhow. Perhaps it\n> would be clearer to introduce a dedicated formatting function for SPI.\n\nBut a dedicated formatting function for SPI will have the same code as\nformat_be32_masked does, so we mustn't duplicate code.\n\n> \n> > - uint8_t si = (ntohl(key->path_hdr) & NSH_SI_MASK) >> NSH_SI_SHIFT;\n> > - uint8_t si_mask = (ntohl(mask->path_hdr) & NSH_SI_MASK) >>\n> > - NSH_SI_SHIFT;\n> > + uint8_t si = nsh_path_hdr_to_si(key->path_hdr);\n> > + uint8_t si_mask = nsh_path_hdr_to_si(mask->path_hdr);\n> > \n> > format_uint8_masked(ds, &first, \"flags\", key->flags, mask->flags);\n> > + format_uint8_masked(ds, &first, \"ttl\", key->ttl, mask->ttl);\n> > format_uint8_masked(ds, &first, \"mdtype\", key->mdtype, mask-\n> > >mdtype);\n> > format_uint8_masked(ds, &first, \"np\", key->np, mask->np);\n> > format_be32_masked(ds, &first, \"spi\", htonl(spi), htonl(spi_mask));\n> \n> > @@ -1808,17 +1810,20 @@ parse_odp_encap_nsh_action(const char *s,\n> > struct ofpbuf *actions)\n> > break;\n> > }\n> > \n> > - if (ovs_scan_len(s, &n, \"flags=%\"SCNi8, &encap_nsh.flags)) {\n> > + if (ovs_scan_len(s, &n, \"flags=%\"SCNi8, &encap_nsh->flags)) {\n> > continue;\n> \n> Any validity check for value flags<=3?\n\nAdded check in new version.\n\n> \n> > }\n> > - if (ovs_scan_len(s, &n, \"mdtype=%\"SCNi8, &encap_nsh.mdtype)) {\n> > - switch (encap_nsh.mdtype) {\n> > + if (ovs_scan_len(s, &n, \"ttl=%\"SCNi8, &encap_nsh->ttl)) {\n> > + continue;\n> \n> Any validity check for ttl < 64?\n\nAdded check in ne version.\n\n> \n> > @@ -1826,24 +1831,24 @@ parse_odp_encap_nsh_action(const char *s,\n> > struct ofpbuf *actions)\n> > }\n> > continue;\n> > }\n> > - if (ovs_scan_len(s, &n, \"np=%\"SCNi8, &encap_nsh.np)) {\n> > + if (ovs_scan_len(s, &n, \"np=%\"SCNi8, &encap_nsh->np)) {\n> > continue;\n> > }\n> > if (ovs_scan_len(s, &n, \"spi=0x%\"SCNx32, &spi)) {\n> > - encap_nsh.path_hdr =\n> > + encap_nsh->path_hdr =\n> > htonl(((spi << NSH_SPI_SHIFT) & NSH_SPI_MASK) |\n> > - (ntohl(encap_nsh.path_hdr) & ~NSH_SPI_MASK));\n> > + (ntohl(encap_nsh->path_hdr) & ~NSH_SPI_MASK));\n> > continue;\n> \n> Why not use the access functions in nsh.h here?\n\nForgot that, Ben wanted me to do changes as less as possible :-), I\nhaved use nsh helper in new version.\n\n> \n> > }\n> > if (ovs_scan_len(s, &n, \"si=%\"SCNi8, &si)) {\n> > - encap_nsh.path_hdr =\n> > + encap_nsh->path_hdr =\n> > htonl((si << NSH_SI_SHIFT) |\n> > - (ntohl(encap_nsh.path_hdr) & ~NSH_SI_MASK));\n> > + (ntohl(encap_nsh->path_hdr) & ~NSH_SI_MASK));\n> > continue;\n> \n> Dito.\n> \n> > @@ -1861,28 +1866,29 @@ parse_odp_encap_nsh_action(const char *s,\n> > struct ofpbuf *actions)\n> > continue;\n> > }\n> > }\n> > - else if (encap_nsh.mdtype == NSH_M_TYPE2) {\n> > + else if (encap_nsh->mdtype == NSH_M_TYPE2) {\n> > struct ofpbuf b;\n> > char buf[512];\n> > size_t mdlen;\n> > if (ovs_scan_len(s, &n, \"md2=0x%511[0-9a-fA-F]\", buf)) {\n> > - ofpbuf_use_stub(&b, encap_nsh.metadata,\n> > - OVS_ENCAP_NSH_MAX_MD_LEN);\n> > + ofpbuf_use_stub(&b, encap_nsh->metadata,\n> > + NSH_CTX_HDRS_MAX_LEN);\n> > ofpbuf_put_hex(&b, buf, &mdlen);\n> \n> Here it might be necessary to pad the metadata buffer with zeros to 4 bytes.\n\nThis is parsing the data netlink message put, md2 hex string has been\nformated which has followed metadata format, so I don't think this is\nnecessary, in addition, pad will be helpless because md2 hex string has\nset tlv len, we can't change it because of such padding.\n\nActucally lib/ofp-actions.c has handled this, all the data is from\nlib/ofp-actions.c, so we mustn't do duplicate and helpless thing here.\n\n> \n> > @@ -6798,19 +6774,20 @@ odp_put_encap_nsh_action(struct ofpbuf\n> > *odp_actions,\n> [...]\n> > - switch (encap_nsh.mdtype) {\n> > + switch (encap_nsh->mdtype) {\n> > case NSH_M_TYPE1: {\n> > struct nsh_md1_ctx *md1 =\n> > - ALIGNED_CAST(struct nsh_md1_ctx *, encap_nsh.metadata);\n> > - encap_nsh.mdlen = NSH_M_TYPE1_MDLEN;\n> > + ALIGNED_CAST(struct nsh_md1_ctx *, encap_nsh->metadata);\n> > + encap_nsh->mdlen = NSH_M_TYPE1_MDLEN;\n> > for (int i = 0; i < 4; i++) {\n> > put_16aligned_be32(&md1->c[i], flow->nsh.c[i]);\n> \n> No need for put_16aligned_be32() here as the encap_nsh action and the metadata included are 8-byte aligned.\n\nI used memcpy(md1, flow->nsh.c, sizeof(*md1)) to do this in new version.\n\n> \n> > diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c\n> > index 9e1f837..1b99ad5 100644\n> > --- a/ofproto/ofproto-dpif-xlate.c\n> > +++ b/ofproto/ofproto-dpif-xlate.c\n> > @@ -5885,10 +5885,10 @@ rewrite_flow_encap_nsh(struct xlate_ctx *ctx,\n> > /* Populate the flow with the new NSH header. */\n> > flow->packet_type = htonl(PT_NSH);\n> > flow->dl_type = htons(ETH_TYPE_NSH);\n> > - flow->nsh.flags = 0; /* */\n> > + flow->nsh.flags = 0;\n> > + flow->nsh.ttl = 63;\n> > flow->nsh.np = np;\n> > - flow->nsh.spi = 0;\n> > - flow->nsh.si = 255;\n> > + flow->nsh.path_hdr = htonl(255);\n> \n> Use nsh setter functions instead?\n\nDone in new version.\n\n> \n> > @@ -5901,6 +5901,7 @@ rewrite_flow_encap_nsh(struct xlate_ctx *ctx,\n> > } else if (md_type == NSH_M_TYPE2) {\n> > flow->nsh.mdtype = NSH_M_TYPE2;\n> > }\n> > + flow->nsh.mdtype &= NSH_MDTYPE_MASK;\n> \n> Not really needed here, as we set the mdtype to a defined value.\n\nRemoved it in new version.\n\n> \n> I didn't check the unit test adaptations to the introduction of ttl field.\n\nI have changed nsh unit tests for ttl and dec_nsh_ttl, they have been\nverified by nsh unit tests.\n\nI'll send out new version after Ben gives me feedbacks.\n\n> \n> BR, Jan","headers":{"Return-Path":"","X-Original-To":["incoming@patchwork.ozlabs.org","dev@openvswitch.org"],"Delivered-To":["patchwork-incoming@bilbo.ozlabs.org","ovs-dev@mail.linuxfoundation.org"],"Authentication-Results":"ozlabs.org;\n\tspf=pass (mailfrom) smtp.mailfrom=openvswitch.org\n\t(client-ip=140.211.169.12; helo=mail.linuxfoundation.org;\n\tenvelope-from=ovs-dev-bounces@openvswitch.org;\n\treceiver=)","Received":["from mail.linuxfoundation.org (mail.linuxfoundation.org\n\t[140.211.169.12])\n\t(using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256\n\tbits)) (No client certificate requested)\n\tby ozlabs.org (Postfix) with ESMTPS id 3xhJzn3BLCz9ryr\n\tfor ;\n\tTue, 29 Aug 2017 16:45:25 +1000 (AEST)","from mail.linux-foundation.org (localhost [127.0.0.1])\n\tby mail.linuxfoundation.org (Postfix) with ESMTP id D797C499;\n\tTue, 29 Aug 2017 06:45:21 +0000 (UTC)","from smtp1.linuxfoundation.org (smtp1.linux-foundation.org\n\t[172.17.192.35])\n\tby mail.linuxfoundation.org (Postfix) with ESMTPS id 10641486\n\tfor ; Tue, 29 Aug 2017 06:45:21 +0000 (UTC)","from mga05.intel.com (mga05.intel.com [192.55.52.43])\n\tby smtp1.linuxfoundation.org (Postfix) with ESMTPS id C8BFA3CD\n\tfor ; Tue, 29 Aug 2017 06:45:19 +0000 (UTC)","from fmsmga005.fm.intel.com ([10.253.24.32])\n\tby fmsmga105.fm.intel.com with ESMTP; 28 Aug 2017 23:45:19 -0700","from unknown (HELO cran64.bj.intel.com) ([10.240.224.181])\n\tby fmsmga005.fm.intel.com with ESMTP; 28 Aug 2017 23:45:18 -0700"],"X-Greylist":"domain auto-whitelisted by SQLgrey-1.7.6","X-ExtLoop1":"1","X-IronPort-AV":"E=Sophos;i=\"5.41,444,1498546800\"; d=\"scan'208\";a=\"145029122\"","Date":"Tue, 29 Aug 2017 14:30:35 +0800","From":"\"Yang, Yi\" ","To":"Jan Scheurich ","Message-ID":"<20170829063035.GA88227@cran64.bj.intel.com>","References":"<1503633771-112384-1-git-send-email-yi.y.yang@intel.com>\n\t<1503633771-112384-3-git-send-email-yi.y.yang@intel.com>\n\t","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"","User-Agent":"Mutt/1.5.23 (2014-03-12)","X-Spam-Status":"No, score=-2.3 required=5.0 tests=RCVD_IN_DNSWL_MED,\n\tRP_MATCHES_RCVD autolearn=disabled version=3.3.1","X-Spam-Checker-Version":"SpamAssassin 3.3.1 (2010-03-16) on\n\tsmtp1.linux-foundation.org","Cc":"\"dev@openvswitch.org\" ","Subject":"Re: [ovs-dev] [PATCH v4 2/3] nsh: add new flow key 'ttl'","X-BeenThere":"ovs-dev@openvswitch.org","X-Mailman-Version":"2.1.12","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Sender":"ovs-dev-bounces@openvswitch.org","Errors-To":"ovs-dev-bounces@openvswitch.org"}},{"id":1759483,"web_url":"http://patchwork.ozlabs.org/comment/1759483/","msgid":"<20170829154407.GZ6175@ovn.org>","list_archive_url":null,"date":"2017-08-29T15:44:07","subject":"Re: [ovs-dev] [PATCH v4 2/3] nsh: add new flow key 'ttl'","submitter":{"id":67603,"url":"http://patchwork.ozlabs.org/api/people/67603/","name":"Ben Pfaff","email":"blp@ovn.org"},"content":"On Tue, Aug 29, 2017 at 02:30:35PM +0800, Yang, Yi wrote:\n> I'll send out new version after Ben gives me feedbacks.\n\nI'd like to see v5. I'm hoping to just go ahead and apply it.","headers":{"Return-Path":"","X-Original-To":["incoming@patchwork.ozlabs.org","dev@openvswitch.org"],"Delivered-To":["patchwork-incoming@bilbo.ozlabs.org","ovs-dev@mail.linuxfoundation.org"],"Authentication-Results":"ozlabs.org;\n\tspf=pass (mailfrom) smtp.mailfrom=openvswitch.org\n\t(client-ip=140.211.169.12; helo=mail.linuxfoundation.org;\n\tenvelope-from=ovs-dev-bounces@openvswitch.org;\n\treceiver=)","Received":["from mail.linuxfoundation.org (mail.linuxfoundation.org\n\t[140.211.169.12])\n\t(using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256\n\tbits)) (No client certificate requested)\n\tby ozlabs.org (Postfix) with ESMTPS id 3xhXxc0X9vz9t38\n\tfor ;\n\tWed, 30 Aug 2017 01:44:20 +1000 (AEST)","from mail.linux-foundation.org (localhost [127.0.0.1])\n\tby mail.linuxfoundation.org (Postfix) with ESMTP id 40FA8ABC;\n\tTue, 29 Aug 2017 15:44:18 +0000 (UTC)","from smtp1.linuxfoundation.org (smtp1.linux-foundation.org\n\t[172.17.192.35])\n\tby mail.linuxfoundation.org (Postfix) with ESMTPS id C6821A81\n\tfor ; Tue, 29 Aug 2017 15:44:16 +0000 (UTC)","from relay2-d.mail.gandi.net (relay2-d.mail.gandi.net\n\t[217.70.183.194])\n\tby smtp1.linuxfoundation.org (Postfix) with ESMTPS id 75863B0\n\tfor ; Tue, 29 Aug 2017 15:44:16 +0000 (UTC)","from ovn.org (173-228-112-34.dsl.dynamic.fusionbroadband.com\n\t[173.228.112.34]) (Authenticated sender: blp@ovn.org)\n\tby relay2-d.mail.gandi.net (Postfix) with ESMTPSA id 94E9DC5A7B;\n\tTue, 29 Aug 2017 17:44:12 +0200 (CEST)"],"X-Greylist":"domain auto-whitelisted by SQLgrey-1.7.6","X-Originating-IP":"173.228.112.34","Date":"Tue, 29 Aug 2017 08:44:07 -0700","From":"Ben Pfaff ","To":"\"Yang, Yi\" ","Message-ID":"<20170829154407.GZ6175@ovn.org>","References":"<1503633771-112384-1-git-send-email-yi.y.yang@intel.com>\n\t<1503633771-112384-3-git-send-email-yi.y.yang@intel.com>\n\t\n\t<20170829063035.GA88227@cran64.bj.intel.com>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20170829063035.GA88227@cran64.bj.intel.com>","User-Agent":"Mutt/1.5.23 (2014-03-12)","X-Spam-Status":"No, score=-0.7 required=5.0 tests=RCVD_IN_DNSWL_LOW\n\tautolearn=disabled version=3.3.1","X-Spam-Checker-Version":"SpamAssassin 3.3.1 (2010-03-16) on\n\tsmtp1.linux-foundation.org","Cc":"\"dev@openvswitch.org\" ","Subject":"Re: [ovs-dev] [PATCH v4 2/3] nsh: add new flow key 'ttl'","X-BeenThere":"ovs-dev@openvswitch.org","X-Mailman-Version":"2.1.12","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Sender":"ovs-dev-bounces@openvswitch.org","Errors-To":"ovs-dev-bounces@openvswitch.org"}},{"id":1759545,"web_url":"http://patchwork.ozlabs.org/comment/1759545/","msgid":"","list_archive_url":null,"date":"2017-08-29T17:02:07","subject":"Re: [ovs-dev] [PATCH v4 2/3] nsh: add new flow key 'ttl'","submitter":{"id":68449,"url":"http://patchwork.ozlabs.org/api/people/68449/","name":"Jan Scheurich","email":"jan.scheurich@ericsson.com"},"content":"Hi Yi,\n\nSome more comments below.\n\n/Jan\n\n> -----Original Message-----\n> From: Yang, Yi [mailto:yi.y.yang@intel.com]\n> Sent: Tuesday, 29 August, 2017 08:31\n> To: Jan Scheurich \n> Cc: dev@openvswitch.org; blp@ovn.org\n> Subject: Re: [PATCH v4 2/3] nsh: add new flow key 'ttl'\n> \n> \n> > > diff --git a/include/openvswitch/flow.h b/include/openvswitch/flow.h\n> > > index a658a58..cd61fff 100644\n> > > --- a/include/openvswitch/flow.h\n> > > +++ b/include/openvswitch/flow.h\n> > > @@ -146,7 +146,7 @@ struct flow {\n> > > struct eth_addr arp_tha; /* ARP/ND target hardware address. */\n> > > ovs_be16 tcp_flags; /* TCP flags. With L3 to avoid matching L4.\n> */\n> > > ovs_be16 pad2; /* Pad to 64 bits. */\n> > > - struct flow_nsh nsh; /* Network Service Header keys */\n> > > + struct ovs_key_nsh nsh; /* Network Service Header keys */\n> >\n> > Why rename this type to struct ovs_key_nsh?\n> \n> I have explained it in previous review, new ttl field results in it isn't 64 bit\n> aligned, moreover, we musnt't do some unnecessary conversion if we use\n> struct ovs_key_nsh, netlink also used struct ovs_key_nsh, it is very\n> reasonable to use struct ovs_key_nsh for this, why don't we use it?\n\nNot arguing alignment here. That resulted in merging spi and si into path_hdr\nand is OK.\n\nAll structs ovs_key_xxx are defined in datapath/linux/.../openvswitch.h, while all \nstructs flow_xxx are defined in lib/packets.h. You have defined struct \novs_key_nsh in lib/packets.h and refer to it both in struct flow and in the \nnetlink attribute. This violates the standard practices. Even if they are\nisomorphic, I think we should have independent type definitions and a (trivial)\nconversion function.\n\n> >\n> > > +static inline void\n> > > +__nsh_set_xflag(struct nsh_hdr *nsh, uint16_t xflag, uint16_t\n> > > +xmask) {\n> > > + nsh->ver_flags_ttl_len\n> > > + = (nsh->ver_flags_ttl_len & ~htons(xmask)) | htons(xflag);\n> > > +}\n> >\n> > Avoid __* prefix. Better call it nsh_set_base_hdr_masked().\n> \n> We just use __ to mark it as an internal function which will not be used\n> outside of nsh.h, this is C coding convention.\n\nThe use of __* prefix is reserved for the C compiler. In OVS we typically \nuse *__ postfix for internal functions.\n\n> > > +static inline void nsh_set_flags_and_ttl(struct nsh_hdr *nsh, uint8_t\n> flags,\n> > > + uint8_t ttl) {\n> > > + __nsh_set_xflag(nsh, ((flags << NSH_FLAGS_SHIFT) &\n> > > NSH_FLAGS_MASK) |\n> > > + ((ttl << NSH_TTL_SHIFT) & NSH_TTL_MASK),\n> > > + NSH_FLAGS_MASK | NSH_TTL_MASK); }\n> > > +\n> > > +static inline void nsh_set_flags_ttl_len(struct nsh_hdr *nsh, uint8_t\n> flags,\n> > > + uint8_t ttl, uint8_t len)\n> > > +{\n> > > + len = len >> 2;\n> > > + __nsh_set_xflag(nsh, ((flags << NSH_FLAGS_SHIFT) &\n> > > NSH_FLAGS_MASK) |\n> > > + ((ttl << NSH_TTL_SHIFT) & NSH_TTL_MASK) |\n> > > + ((len << NSH_LEN_SHIFT) & NSH_LEN_MASK),\n> > > + NSH_FLAGS_MASK | NSH_TTL_MASK | NSH_LEN_MASK);\n> > > +}\n> >\n> > Do you really need to these hybrid functions? Why not use modular\n> functions nsh_set_flags(), nsh_set_ttl() and nsh_set_len()? I believe the run-\n> time overhead is negligible with an optimizing compiler.\n> \n> Basically these three functions will be executed together, so one is better.\n\nThey are set in different combinations. I'd say it is better not have a dedicated\nsetter function for every required combination. Only a significant run-time \noverhead might tempt one to introduce collapsed functions.\n\n> > For symmetry you should also add a function setter function for path_hdr:\n> \n> I think put_16aligned_be32(&nsh->path_hdr, path_hdr) is better than\n> nsh_set_path_hdr, here isn't C++ programming :-)\n\nI don't agree. You have nsh_get_path_hdr(), so why are set and get handled \ndifferently? The main point of these functions is to shield the user from the\nnasty alignment details of struct nsh_hdr when accessing the packet headers.\n\n\n> >\n> > Why did you rename this struct? As part of the struct flow, it was\n> > more appropriately called struct flow_nsh.\n> \n> It isn't name issue, we want to use common struct ovs_key_nsh in multiple\n> function modules.\n\nPlease see above. Struct ovs_key_nsh should be defined in \ndatapath/linux/.../openvswitch.h to be used on the netlink interface. \nstruct flow_nsh should be defined here in lib/packets.h and used in struct flow.\n\n> > > diff --git a/lib/nx-match.c b/lib/nx-match.c index b782e8c..dc52566\n> > > 100644\n> > > --- a/lib/nx-match.c\n> > > +++ b/lib/nx-match.c\n> > > @@ -1157,13 +1158,22 @@ nx_put_raw(struct ofpbuf *b, enum\n> > > ofp_version oxm, const struct match *match,\n> > > /* Network Service Header */\n> > > nxm_put_8m(&ctx, MFF_NSH_FLAGS, oxm, flow->nsh.flags,\n> > > match->wc.masks.nsh.flags);\n> > > + nxm_put_8m(&ctx, MFF_NSH_TTL, oxm, flow->nsh.ttl,\n> > > + match->wc.masks.nsh.ttl);\n> > > nxm_put_8m(&ctx, MFF_NSH_MDTYPE, oxm, flow->nsh.mdtype,\n> > > match->wc.masks.nsh.mdtype);\n> > > nxm_put_8m(&ctx, MFF_NSH_NP, oxm, flow->nsh.np,\n> > > match->wc.masks.nsh.np);\n> > > - nxm_put_32m(&ctx, MFF_NSH_SPI, oxm, flow->nsh.spi,\n> > > - match->wc.masks.nsh.spi);\n> > > - nxm_put_8m(&ctx, MFF_NSH_SI, oxm, flow->nsh.si, match-\n> > > >wc.masks.nsh.si);\n> > > + spi_mask = nsh_path_hdr_to_spi(match->wc.masks.nsh.path_hdr);\n> > > + if (spi_mask == htonl(NSH_SPI_MASK >> NSH_SPI_SHIFT)) {\n> > > + spi_mask = OVS_BE32_MAX;\n> > > + }\n> > > + nxm_put_32m(&ctx, MFF_NSH_SPI, oxm,\n> > > + nsh_path_hdr_to_spi(flow->nsh.path_hdr),\n> > > + spi_mask);\n> >\n> > I think it is not correct to expand a 24 bit all-ones mask for the SPI field to\n> a full 32-bit all-ones mask. Compare to the MFF_IPV6_LABEL case which is\n> defined as a 20-bit field in a 4-byte OXM. The codes just puts the mask as\n> is.\n> \n> MFF_IPV6_LABEL is different from this, MPLS_LABEL is same as this one.\n> But it will return BAD_MASK error if we don't expand it ot OVS_BE32_MAX\n> because mf_is_mask_valid expects mask is all one, the actual mask is\n> 0x00FFFFFF. I have tried this many times, please do tell me a better\n> solution for this if you have a better one.\n\nMFF_MPLS_LABEL is not comparable because the label is not maskable.\nMFF_IPV6_LABEL, in contrast, is a maskable 20 bit field in a 32 bit OXM \ncontainer. And this is about including the mask in the in the OXM.\n\nBut I double checked the MFF_IPV6_LABEL code. It indeed sets the internal \nMask to OVS_BE32_MAX for an exact match of the IPv6 label field. \nThat's why we can use the generic nxm_put_32_m() function to encode the \nMFF_IPV6_LABEL OXM into the OpenFlow match. That function internally \ncalls is_all_ones(mask, 4) to check for an exact match.\n\nWe need to do the same for NSH SPI. So your code was doing the right thing.\n\n> > > diff --git a/lib/odp-util.c b/lib/odp-util.c index 4f1499e..ac286ea\n> > > 100644\n> > > --- a/lib/odp-util.c\n> > > +++ b/lib/odp-util.c\n> > > @@ -319,17 +320,16 @@ format_nsh_key_mask(struct ds *ds, const\n> > > struct ovs_key_nsh *key,\n> > > format_nsh_key(ds, key);\n> > > } else {\n> > > bool first = true;\n> > > - uint32_t spi = (ntohl(key->path_hdr) & NSH_SPI_MASK) >>\n> > > NSH_SPI_SHIFT;\n> > > - uint32_t spi_mask = (ntohl(mask->path_hdr) & NSH_SPI_MASK)\n> >>\n> > > - NSH_SPI_SHIFT;\n> > > - if (spi_mask == 0x00ffffff) {\n> > > + uint32_t spi = nsh_path_hdr_to_spi_uint32(key->path_hdr);\n> > > + uint32_t spi_mask = nsh_path_hdr_to_spi_uint32(mask-\n> >path_hdr);\n> > > + if (spi_mask == (NSH_SPI_MASK >> NSH_SPI_SHIFT)) {\n> > > spi_mask = UINT32_MAX;\n> > > }\n> >\n> > I understand that you expand the mask here to 32 bits to be able to\n> > use format_be32_masked helper below, but it is confusing anyhow.\n> Perhaps it would be clearer to introduce a dedicated formatting function\n> for SPI.\n> \n> But a dedicated formatting function for SPI will have the same code as\n> format_be32_masked does, so we mustn't duplicate code.\n\nOK, see also my conclusion on mask expansion above.\n\n> > > @@ -1861,28 +1866,29 @@ parse_odp_encap_nsh_action(const char\n> *s,\n> > > struct ofpbuf *actions)\n> > > continue;\n> > > }\n> > > }\n> > > - else if (encap_nsh.mdtype == NSH_M_TYPE2) {\n> > > + else if (encap_nsh->mdtype == NSH_M_TYPE2) {\n> > > struct ofpbuf b;\n> > > char buf[512];\n> > > size_t mdlen;\n> > > if (ovs_scan_len(s, &n, \"md2=0x%511[0-9a-fA-F]\", buf)) {\n> > > - ofpbuf_use_stub(&b, encap_nsh.metadata,\n> > > - OVS_ENCAP_NSH_MAX_MD_LEN);\n> > > + ofpbuf_use_stub(&b, encap_nsh->metadata,\n> > > + NSH_CTX_HDRS_MAX_LEN);\n> > > ofpbuf_put_hex(&b, buf, &mdlen);\n> >\n> > Here it might be necessary to pad the metadata buffer with zeros to 4\n> bytes.\n> \n> This is parsing the data netlink message put, md2 hex string has been\n> formated which has followed metadata format, so I don't think this is\n> necessary, in addition, pad will be helpless because md2 hex string has set\n> tlv len, we can't change it because of such padding.\n\nThis code is parsing the datapath action in an ovs-dpctl add-flow command.\nWe cannot rely on that the hex string in the command is 4-byte aligned. If it\nis not, we must make sure that the buffer for the MD2 TLV is padded with \nzeros to 4 byte boundary, even though the TLV length field may be a non-\ninteger multiple of 4.\n\n> \n> Actucally lib/ofp-actions.c has handled this, all the data is from lib/ofp-\n> actions.c, so we mustn't do duplicate and helpless thing here.\n\nThe code in lib/ofp-actions.c parses the OpenFlow generic encap() action in\novs-ofctl commands. This is completely independent.\n\n> \n> >\n> > I didn't check the unit test adaptations to the introduction of ttl field.\n> \n> I have changed nsh unit tests for ttl and dec_nsh_ttl, they have been\n> verified by nsh unit tests.\n> \n> I'll send out new version after Ben gives me feedbacks.\n> \n> >\n> > BR, Jan","headers":{"Return-Path":"","X-Original-To":["incoming@patchwork.ozlabs.org","dev@openvswitch.org"],"Delivered-To":["patchwork-incoming@bilbo.ozlabs.org","ovs-dev@mail.linuxfoundation.org"],"Authentication-Results":"ozlabs.org;\n\tspf=pass (mailfrom) smtp.mailfrom=openvswitch.org\n\t(client-ip=140.211.169.12; helo=mail.linuxfoundation.org;\n\tenvelope-from=ovs-dev-bounces@openvswitch.org;\n\treceiver=)","Received":["from mail.linuxfoundation.org (mail.linuxfoundation.org\n\t[140.211.169.12])\n\t(using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256\n\tbits)) (No client certificate requested)\n\tby ozlabs.org (Postfix) with ESMTPS id 3xhZgY1prHz9s76\n\tfor ;\n\tWed, 30 Aug 2017 03:02:16 +1000 (AEST)","from mail.linux-foundation.org (localhost [127.0.0.1])\n\tby mail.linuxfoundation.org (Postfix) with ESMTP id 2B229B0C;\n\tTue, 29 Aug 2017 17:02:13 +0000 (UTC)","from smtp1.linuxfoundation.org (smtp1.linux-foundation.org\n\t[172.17.192.35])\n\tby mail.linuxfoundation.org (Postfix) with ESMTPS id EBCD3AD6\n\tfor ; Tue, 29 Aug 2017 17:02:11 +0000 (UTC)","from sesbmg23.ericsson.net (sesbmg23.ericsson.net [193.180.251.37])\n\tby smtp1.linuxfoundation.org (Postfix) with ESMTPS id B7D531D7\n\tfor ; Tue, 29 Aug 2017 17:02:10 +0000 (UTC)","from ESESSHC015.ericsson.se (Unknown_Domain [153.88.183.63])\n\tby sesbmg23.ericsson.net (Symantec Mail Security) with SMTP id\n\tB5.B6.21299.01E95A95; Tue, 29 Aug 2017 19:02:08 +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\tTue, 29 Aug 2017 19:02:08 +0200"],"X-Greylist":"domain auto-whitelisted by SQLgrey-1.7.6","X-AuditID":"c1b4fb25-967ff70000005333-9f-59a59e100d5b","From":"Jan Scheurich ","To":"\"Yang, Yi\" ","Thread-Topic":"[PATCH v4 2/3] nsh: add new flow key 'ttl'","Thread-Index":"AQHTHVeXt1MQLcjG70KsWi3CUoBQEqKZvTRAgAEHOoCAAIxpwA==","Date":"Tue, 29 Aug 2017 17:02:07 +0000","Message-ID":"","References":"<1503633771-112384-1-git-send-email-yi.y.yang@intel.com>\n\t<1503633771-112384-3-git-send-email-yi.y.yang@intel.com>\n\t\n\t<20170829063035.GA88227@cran64.bj.intel.com>","In-Reply-To":"<20170829063035.GA88227@cran64.bj.intel.com>","Accept-Language":"en-US","Content-Language":"en-US","X-MS-Has-Attach":"","X-MS-TNEF-Correlator":"","x-originating-ip":"[153.88.183.18]","MIME-Version":"1.0","X-Brightmail-Tracker":"H4sIAAAAAAAAA+NgFmplkeLIzCtJLcpLzFFi42KZGbHdXldg3tJIg+7FlhavJjcwWhw9vYfZ\n\t4vfXbUwOzB6L97xk8nh28z+jx/NrPSwBzFFcNimpOZllqUX6dglcGd2zLrAXbA2pmLRzPVsD\n\t43+7LkZODgkBE4lVy08xg9hCAkcYJXqPKHQxcgHZixkl3hx/BJTg4GATMJCYvdsBpEZEQEVi\n\t8val7CA2s4CPxJvmY2C2sIC5xI7+D8wQNRYSXz8cYgNpFRFwkrj2SxMkzCKgKvG2aQlYCa+A\n\tr8TFh+vYIFZ9YpS48q8FbA6ngKXEqfUQRYwCYhLfT61hgtglLnHryXwmiJsFJJbsOc8MYYtK\n\tvHz8jxXCVpT4+GofI0S9jsSC3Z/YIGxtiWULX0MtFpQ4OfMJywRG0VlIxs5C0jILScssJC0L\n\tGFlWMYoWpxYn5aYbGeulFmUmFxfn5+nlpZZsYgRGzsEtv1V3MF5+43iIUYCDUYmH9/LspZFC\n\trIllxZW5hxglOJiVRHib24BCvCmJlVWpRfnxRaU5qcWHGKU5WJTEeR33XYgQEkhPLEnNTk0t\n\tSC2CyTJxcEo1MDaf+ciZVvrnyTfn/O02B7tN3G/NztRw2yVtZq/T4iNaKnPB5uUNg7VqmeVR\n\tPB28z9TPrbbLfRG+Tq624bQk9/pWZgfmsukKB7boS/BOy/R69N/Z9XnFHNVdP07Pv6x25QLX\n\tqmPLsi42hgj3bb9jI3mqOnWZxqkX7BXfItg8n76NM2BYy3d5iRJLcUaioRZzUXEiALX9EcSY\n\tAgAA","X-Spam-Status":"No, score=-2.3 required=5.0 tests=RCVD_IN_DNSWL_MED\n\tautolearn=disabled version=3.3.1","X-Spam-Checker-Version":"SpamAssassin 3.3.1 (2010-03-16) on\n\tsmtp1.linux-foundation.org","Cc":"\"dev@openvswitch.org\" ","Subject":"Re: [ovs-dev] [PATCH v4 2/3] nsh: add new flow key 'ttl'","X-BeenThere":"ovs-dev@openvswitch.org","X-Mailman-Version":"2.1.12","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Sender":"ovs-dev-bounces@openvswitch.org","Errors-To":"ovs-dev-bounces@openvswitch.org"}},{"id":1759575,"web_url":"http://patchwork.ozlabs.org/comment/1759575/","msgid":"<20170829172920.GA99037@cran64.bj.intel.com>","list_archive_url":null,"date":"2017-08-29T17:29:20","subject":"Re: [ovs-dev] [PATCH v4 2/3] nsh: add new flow key 'ttl'","submitter":{"id":68962,"url":"http://patchwork.ozlabs.org/api/people/68962/","name":"Yang, Yi","email":"yi.y.yang@intel.com"},"content":"On Wed, Aug 30, 2017 at 01:02:07AM +0800, Jan Scheurich wrote:\n> Hi Yi,\n> \n> Some more comments below.\n> \n> /Jan\n> \n> > -----Original Message-----\n> > From: Yang, Yi [mailto:yi.y.yang@intel.com]\n> > Sent: Tuesday, 29 August, 2017 08:31\n> > To: Jan Scheurich \n> > Cc: dev@openvswitch.org; blp@ovn.org\n> > Subject: Re: [PATCH v4 2/3] nsh: add new flow key 'ttl'\n> > \n> > \n> > > > diff --git a/include/openvswitch/flow.h b/include/openvswitch/flow.h\n> > > > index a658a58..cd61fff 100644\n> > > > --- a/include/openvswitch/flow.h\n> > > > +++ b/include/openvswitch/flow.h\n> > > > @@ -146,7 +146,7 @@ struct flow {\n> > > > struct eth_addr arp_tha; /* ARP/ND target hardware address. */\n> > > > ovs_be16 tcp_flags; /* TCP flags. With L3 to avoid matching L4.\n> > */\n> > > > ovs_be16 pad2; /* Pad to 64 bits. */\n> > > > - struct flow_nsh nsh; /* Network Service Header keys */\n> > > > + struct ovs_key_nsh nsh; /* Network Service Header keys */\n> > >\n> > > Why rename this type to struct ovs_key_nsh?\n> > \n> > I have explained it in previous review, new ttl field results in it isn't 64 bit\n> > aligned, moreover, we musnt't do some unnecessary conversion if we use\n> > struct ovs_key_nsh, netlink also used struct ovs_key_nsh, it is very\n> > reasonable to use struct ovs_key_nsh for this, why don't we use it?\n> \n> Not arguing alignment here. That resulted in merging spi and si into path_hdr\n> and is OK.\n> \n> All structs ovs_key_xxx are defined in datapath/linux/.../openvswitch.h, while all \n> structs flow_xxx are defined in lib/packets.h. You have defined struct \n> ovs_key_nsh in lib/packets.h and refer to it both in struct flow and in the \n> netlink attribute. This violates the standard practices. Even if they are\n> isomorphic, I think we should have independent type definitions and a (trivial)\n> conversion function.\n\nJan, this is just a way we can aviod breaking kernel uAPI, I also\ndefined struct ovs_key_nsh in net/openvswitch/flow.h not in\ndatapath/linux/.../openvswitch.h in kernel datapath patch.\n\n> \n> > >\n> > > > +static inline void\n> > > > +__nsh_set_xflag(struct nsh_hdr *nsh, uint16_t xflag, uint16_t\n> > > > +xmask) {\n> > > > + nsh->ver_flags_ttl_len\n> > > > + = (nsh->ver_flags_ttl_len & ~htons(xmask)) | htons(xflag);\n> > > > +}\n> > >\n> > > Avoid __* prefix. Better call it nsh_set_base_hdr_masked().\n> > \n> > We just use __ to mark it as an internal function which will not be used\n> > outside of nsh.h, this is C coding convention.\n> \n> The use of __* prefix is reserved for the C compiler. In OVS we typically \n> use *__ postfix for internal functions.\n\nOk, let me move __ as suffix.\n\n> \n> > > > +static inline void nsh_set_flags_and_ttl(struct nsh_hdr *nsh, uint8_t\n> > flags,\n> > > > + uint8_t ttl) {\n> > > > + __nsh_set_xflag(nsh, ((flags << NSH_FLAGS_SHIFT) &\n> > > > NSH_FLAGS_MASK) |\n> > > > + ((ttl << NSH_TTL_SHIFT) & NSH_TTL_MASK),\n> > > > + NSH_FLAGS_MASK | NSH_TTL_MASK); }\n> > > > +\n> > > > +static inline void nsh_set_flags_ttl_len(struct nsh_hdr *nsh, uint8_t\n> > flags,\n> > > > + uint8_t ttl, uint8_t len)\n> > > > +{\n> > > > + len = len >> 2;\n> > > > + __nsh_set_xflag(nsh, ((flags << NSH_FLAGS_SHIFT) &\n> > > > NSH_FLAGS_MASK) |\n> > > > + ((ttl << NSH_TTL_SHIFT) & NSH_TTL_MASK) |\n> > > > + ((len << NSH_LEN_SHIFT) & NSH_LEN_MASK),\n> > > > + NSH_FLAGS_MASK | NSH_TTL_MASK | NSH_LEN_MASK);\n> > > > +}\n> > >\n> > > Do you really need to these hybrid functions? Why not use modular\n> > functions nsh_set_flags(), nsh_set_ttl() and nsh_set_len()? I believe the run-\n> > time overhead is negligible with an optimizing compiler.\n> > \n> > Basically these three functions will be executed together, so one is better.\n> \n> They are set in different combinations. I'd say it is better not have a dedicated\n> setter function for every required combination. Only a significant run-time \n> overhead might tempt one to introduce collapsed functions.\n> \n> > > For symmetry you should also add a function setter function for path_hdr:\n> > \n> > I think put_16aligned_be32(&nsh->path_hdr, path_hdr) is better than\n> > nsh_set_path_hdr, here isn't C++ programming :-)\n> \n> I don't agree. You have nsh_get_path_hdr(), so why are set and get handled \n> differently? The main point of these functions is to shield the user from the\n> nasty alignment details of struct nsh_hdr when accessing the packet headers.\n\nI want to remove nsh_get_path_hdr, I don't want nsh.h to depend on\nother stuff. put_16aligned_be32 needs to be defined in nsh.h for this.\n\n> \n> \n> > >\n> > > Why did you rename this struct? As part of the struct flow, it was\n> > > more appropriately called struct flow_nsh.\n> > \n> > It isn't name issue, we want to use common struct ovs_key_nsh in multiple\n> > function modules.\n> \n> Please see above. Struct ovs_key_nsh should be defined in \n> datapath/linux/.../openvswitch.h to be used on the netlink interface. \n> struct flow_nsh should be defined here in lib/packets.h and used in struct flow.\n\nPlease see my comments in previous section.\n\n> \n> > > > diff --git a/lib/nx-match.c b/lib/nx-match.c index b782e8c..dc52566\n> > > > 100644\n> > > > --- a/lib/nx-match.c\n> > > > +++ b/lib/nx-match.c\n> > > > @@ -1157,13 +1158,22 @@ nx_put_raw(struct ofpbuf *b, enum\n> > > > ofp_version oxm, const struct match *match,\n> > > > /* Network Service Header */\n> > > > nxm_put_8m(&ctx, MFF_NSH_FLAGS, oxm, flow->nsh.flags,\n> > > > match->wc.masks.nsh.flags);\n> > > > + nxm_put_8m(&ctx, MFF_NSH_TTL, oxm, flow->nsh.ttl,\n> > > > + match->wc.masks.nsh.ttl);\n> > > > nxm_put_8m(&ctx, MFF_NSH_MDTYPE, oxm, flow->nsh.mdtype,\n> > > > match->wc.masks.nsh.mdtype);\n> > > > nxm_put_8m(&ctx, MFF_NSH_NP, oxm, flow->nsh.np,\n> > > > match->wc.masks.nsh.np);\n> > > > - nxm_put_32m(&ctx, MFF_NSH_SPI, oxm, flow->nsh.spi,\n> > > > - match->wc.masks.nsh.spi);\n> > > > - nxm_put_8m(&ctx, MFF_NSH_SI, oxm, flow->nsh.si, match-\n> > > > >wc.masks.nsh.si);\n> > > > + spi_mask = nsh_path_hdr_to_spi(match->wc.masks.nsh.path_hdr);\n> > > > + if (spi_mask == htonl(NSH_SPI_MASK >> NSH_SPI_SHIFT)) {\n> > > > + spi_mask = OVS_BE32_MAX;\n> > > > + }\n> > > > + nxm_put_32m(&ctx, MFF_NSH_SPI, oxm,\n> > > > + nsh_path_hdr_to_spi(flow->nsh.path_hdr),\n> > > > + spi_mask);\n> > >\n> > > I think it is not correct to expand a 24 bit all-ones mask for the SPI field to\n> > a full 32-bit all-ones mask. Compare to the MFF_IPV6_LABEL case which is\n> > defined as a 20-bit field in a 4-byte OXM. The codes just puts the mask as\n> > is.\n> > \n> > MFF_IPV6_LABEL is different from this, MPLS_LABEL is same as this one.\n> > But it will return BAD_MASK error if we don't expand it ot OVS_BE32_MAX\n> > because mf_is_mask_valid expects mask is all one, the actual mask is\n> > 0x00FFFFFF. I have tried this many times, please do tell me a better\n> > solution for this if you have a better one.\n> \n> MFF_MPLS_LABEL is not comparable because the label is not maskable.\n> MFF_IPV6_LABEL, in contrast, is a maskable 20 bit field in a 32 bit OXM \n> container. And this is about including the mask in the in the OXM.\n> \n> But I double checked the MFF_IPV6_LABEL code. It indeed sets the internal \n> Mask to OVS_BE32_MAX for an exact match of the IPv6 label field. \n> That's why we can use the generic nxm_put_32_m() function to encode the \n> MFF_IPV6_LABEL OXM into the OpenFlow match. That function internally \n> calls is_all_ones(mask, 4) to check for an exact match.\n> \n> We need to do the same for NSH SPI. So your code was doing the right thing.\n\nBut spi isn't maskable. For MFF_MPLS_LABEL, it used nxm_put_32 not\nnxm_put_32m, so it didn't have this issue.\n\n /* \"nsh_spi\" (aka \"nsp\").\n *\n * spi (service path identifier) field in NSH base header.\n *\n * Type: be32 (low 24 bits).\n * Maskable: no.\n * Formatting: hexadecimal.\n * Prerequisites: NSH.\n * Access: read/write.\n * NXM: none.\n * OXM: NXOXM_NSH_SPI(5) since OF1.3 and v2.8.\n */\n\n> \n> > > > diff --git a/lib/odp-util.c b/lib/odp-util.c index 4f1499e..ac286ea\n> > > > 100644\n> > > > --- a/lib/odp-util.c\n> > > > +++ b/lib/odp-util.c\n> > > > @@ -319,17 +320,16 @@ format_nsh_key_mask(struct ds *ds, const\n> > > > struct ovs_key_nsh *key,\n> > > > format_nsh_key(ds, key);\n> > > > } else {\n> > > > bool first = true;\n> > > > - uint32_t spi = (ntohl(key->path_hdr) & NSH_SPI_MASK) >>\n> > > > NSH_SPI_SHIFT;\n> > > > - uint32_t spi_mask = (ntohl(mask->path_hdr) & NSH_SPI_MASK)\n> > >>\n> > > > - NSH_SPI_SHIFT;\n> > > > - if (spi_mask == 0x00ffffff) {\n> > > > + uint32_t spi = nsh_path_hdr_to_spi_uint32(key->path_hdr);\n> > > > + uint32_t spi_mask = nsh_path_hdr_to_spi_uint32(mask-\n> > >path_hdr);\n> > > > + if (spi_mask == (NSH_SPI_MASK >> NSH_SPI_SHIFT)) {\n> > > > spi_mask = UINT32_MAX;\n> > > > }\n> > >\n> > > I understand that you expand the mask here to 32 bits to be able to\n> > > use format_be32_masked helper below, but it is confusing anyhow.\n> > Perhaps it would be clearer to introduce a dedicated formatting function\n> > for SPI.\n> > \n> > But a dedicated formatting function for SPI will have the same code as\n> > format_be32_masked does, so we mustn't duplicate code.\n> \n> OK, see also my conclusion on mask expansion above.\n> \n> > > > @@ -1861,28 +1866,29 @@ parse_odp_encap_nsh_action(const char\n> > *s,\n> > > > struct ofpbuf *actions)\n> > > > continue;\n> > > > }\n> > > > }\n> > > > - else if (encap_nsh.mdtype == NSH_M_TYPE2) {\n> > > > + else if (encap_nsh->mdtype == NSH_M_TYPE2) {\n> > > > struct ofpbuf b;\n> > > > char buf[512];\n> > > > size_t mdlen;\n> > > > if (ovs_scan_len(s, &n, \"md2=0x%511[0-9a-fA-F]\", buf)) {\n> > > > - ofpbuf_use_stub(&b, encap_nsh.metadata,\n> > > > - OVS_ENCAP_NSH_MAX_MD_LEN);\n> > > > + ofpbuf_use_stub(&b, encap_nsh->metadata,\n> > > > + NSH_CTX_HDRS_MAX_LEN);\n> > > > ofpbuf_put_hex(&b, buf, &mdlen);\n> > >\n> > > Here it might be necessary to pad the metadata buffer with zeros to 4\n> > bytes.\n> > \n> > This is parsing the data netlink message put, md2 hex string has been\n> > formated which has followed metadata format, so I don't think this is\n> > necessary, in addition, pad will be helpless because md2 hex string has set\n> > tlv len, we can't change it because of such padding.\n> \n> This code is parsing the datapath action in an ovs-dpctl add-flow command.\n> We cannot rely on that the hex string in the command is 4-byte aligned. If it\n> is not, we must make sure that the buffer for the MD2 TLV is padded with \n> zeros to 4 byte boundary, even though the TLV length field may be a non-\n> integer multiple of 4.\n\nWill add padding for 4 bytes alignment in new version.\n\n> \n> > \n> > Actucally lib/ofp-actions.c has handled this, all the data is from lib/ofp-\n> > actions.c, so we mustn't do duplicate and helpless thing here.\n> \n> The code in lib/ofp-actions.c parses the OpenFlow generic encap() action in\n> ovs-ofctl commands. This is completely independent.\n> \n> > \n> > >\n> > > I didn't check the unit test adaptations to the introduction of ttl field.\n> > \n> > I have changed nsh unit tests for ttl and dec_nsh_ttl, they have been\n> > verified by nsh unit tests.\n> > \n> > I'll send out new version after Ben gives me feedbacks.\n> > \n> > >\n> > > BR, Jan","headers":{"Return-Path":"","X-Original-To":["incoming@patchwork.ozlabs.org","dev@openvswitch.org"],"Delivered-To":["patchwork-incoming@bilbo.ozlabs.org","ovs-dev@mail.linuxfoundation.org"],"Authentication-Results":"ozlabs.org;\n\tspf=pass (mailfrom) smtp.mailfrom=openvswitch.org\n\t(client-ip=140.211.169.12; helo=mail.linuxfoundation.org;\n\tenvelope-from=ovs-dev-bounces@openvswitch.org;\n\treceiver=)","Received":["from mail.linuxfoundation.org (mail.linuxfoundation.org\n\t[140.211.169.12])\n\t(using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256\n\tbits)) (No client certificate requested)\n\tby ozlabs.org (Postfix) with ESMTPS id 3xhbc05gkhz9t2Q\n\tfor ;\n\tWed, 30 Aug 2017 03:44:15 +1000 (AEST)","from mail.linux-foundation.org (localhost [127.0.0.1])\n\tby mail.linuxfoundation.org (Postfix) with ESMTP id 743ADB0A;\n\tTue, 29 Aug 2017 17:44:11 +0000 (UTC)","from smtp1.linuxfoundation.org (smtp1.linux-foundation.org\n\t[172.17.192.35])\n\tby mail.linuxfoundation.org (Postfix) with ESMTPS id 1697BA95\n\tfor ; Tue, 29 Aug 2017 17:44:10 +0000 (UTC)","from mga09.intel.com (mga09.intel.com [134.134.136.24])\n\tby smtp1.linuxfoundation.org (Postfix) with ESMTPS id 7D34B42B\n\tfor ; Tue, 29 Aug 2017 17:44:05 +0000 (UTC)","from fmsmga002.fm.intel.com ([10.253.24.26])\n\tby orsmga102.jf.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384;\n\t29 Aug 2017 10:44:04 -0700","from unknown (HELO cran64.bj.intel.com) ([10.240.224.181])\n\tby fmsmga002.fm.intel.com with ESMTP; 29 Aug 2017 10:44:03 -0700"],"X-Greylist":"domain auto-whitelisted by SQLgrey-1.7.6","X-ExtLoop1":"1","X-IronPort-AV":"E=Sophos; i=\"5.41,445,1498546800\"; d=\"scan'208\";\n\ta=\"1212065371\"","Date":"Wed, 30 Aug 2017 01:29:20 +0800","From":"\"Yang, Yi\" ","To":"Jan Scheurich ","Message-ID":"<20170829172920.GA99037@cran64.bj.intel.com>","References":"<1503633771-112384-1-git-send-email-yi.y.yang@intel.com>\n\t<1503633771-112384-3-git-send-email-yi.y.yang@intel.com>\n\t\n\t<20170829063035.GA88227@cran64.bj.intel.com>\n\t","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"","User-Agent":"Mutt/1.5.23 (2014-03-12)","X-Spam-Status":"No, score=-5.0 required=5.0 tests=RCVD_IN_DNSWL_HI,\n\tRP_MATCHES_RCVD autolearn=disabled version=3.3.1","X-Spam-Checker-Version":"SpamAssassin 3.3.1 (2010-03-16) on\n\tsmtp1.linux-foundation.org","Cc":"\"dev@openvswitch.org\" ","Subject":"Re: [ovs-dev] [PATCH v4 2/3] nsh: add new flow key 'ttl'","X-BeenThere":"ovs-dev@openvswitch.org","X-Mailman-Version":"2.1.12","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Sender":"ovs-dev-bounces@openvswitch.org","Errors-To":"ovs-dev-bounces@openvswitch.org"}}]