[ovs-dev] [PATCH v4 2/3] nsh: add new flow key 'ttl'
Jan Scheurich
jan.scheurich at ericsson.com
Mon Aug 28 23:31:22 UTC 2017
Hi Yi,
Thanks for the fixes. Please find my individual comments inline.
BR, Jan
> -----Original Message-----
> -#define OVS_ENCAP_NSH_MAX_MD_LEN 16
> /*
> * struct ovs_action_encap_nsh - %OVS_ACTION_ATTR_ENCAP_NSH
> * @flags: NSH header flags.
> + * @ttl: NSH header TTL.
> * @mdtype: NSH metadata type.
> * @mdlen: Length of NSH metadata in bytes.
> * @np: NSH next_protocol: Inner packet type.
> @@ -805,11 +796,13 @@ struct ovs_action_push_eth {
> */
> struct ovs_action_encap_nsh {
> uint8_t flags;
> + uint8_t ttl;
> uint8_t mdtype;
> - uint8_t mdlen;
> uint8_t np;
> __be32 path_hdr;
> - uint8_t metadata[OVS_ENCAP_NSH_MAX_MD_LEN];
> + uint8_t mdlen;
> + uint8_t pad[7]; /* Aligned to 64 bit boundary for metadata */
> + uint8_t metadata[];
> };
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.
> diff --git a/include/openvswitch/flow.h b/include/openvswitch/flow.h
> index a658a58..cd61fff 100644
> --- a/include/openvswitch/flow.h
> +++ b/include/openvswitch/flow.h
> @@ -146,7 +146,7 @@ struct flow {
> struct eth_addr arp_tha; /* ARP/ND target hardware address. */
> ovs_be16 tcp_flags; /* TCP flags. With L3 to avoid matching L4. */
> ovs_be16 pad2; /* Pad to 64 bits. */
> - struct flow_nsh nsh; /* Network Service Header keys */
> + struct ovs_key_nsh nsh; /* Network Service Header keys */
Why rename this type to struct ovs_key_nsh?
> diff --git a/include/openvswitch/nsh.h b/include/openvswitch/nsh.h
[...]
> @@ -62,7 +210,7 @@ struct nsh_md2_tlv {
> };
>
> struct nsh_hdr {
> - ovs_be16 ver_flags_len;
> + ovs_be16 ver_flags_ttl_len;
> uint8_t md_type;
> uint8_t next_proto;
> ovs_16aligned_be32 path_hdr;
> @@ -75,11 +223,16 @@ struct nsh_hdr {
> /* Masking NSH header fields. */
> #define NSH_VER_MASK 0xc000
> #define NSH_VER_SHIFT 14
> -#define NSH_FLAGS_MASK 0x3fc0
> -#define NSH_FLAGS_SHIFT 6
> +#define NSH_FLAGS_MASK 0x3000
> +#define NSH_FLAGS_SHIFT 12
With this definition of NSH FLAGS the match field NSH_FLAGS covers
only the two bits between Version and TTL field. Bit 1 is the OaM flag,
bit 0 is currently unused. The remaining 4 unused bits between Length
and MD Type fields are excluded. That is perhaps OK as we cannot be
sure the these 4 bits will be used as "flags" in the future. But it needs
to be documented clearly that NSH_FLAGS has only two bits and which
bits in the NSH header it covers.
> +static inline void
> +__nsh_set_xflag(struct nsh_hdr *nsh, uint16_t xflag, uint16_t xmask)
> +{
> + nsh->ver_flags_ttl_len
> + = (nsh->ver_flags_ttl_len & ~htons(xmask)) | htons(xflag);
> +}
Avoid __* prefix. Better call it nsh_set_base_hdr_masked().
> +
> +static inline void nsh_set_flags_and_ttl(struct nsh_hdr *nsh, uint8_t flags,
> + uint8_t ttl)
> +{
> + __nsh_set_xflag(nsh, ((flags << NSH_FLAGS_SHIFT) &
> NSH_FLAGS_MASK) |
> + ((ttl << NSH_TTL_SHIFT) & NSH_TTL_MASK),
> + NSH_FLAGS_MASK | NSH_TTL_MASK);
> +}
> +
> +static inline void nsh_set_flags_ttl_len(struct nsh_hdr *nsh, uint8_t flags,
> + uint8_t ttl, uint8_t len)
> +{
> + len = len >> 2;
> + __nsh_set_xflag(nsh, ((flags << NSH_FLAGS_SHIFT) &
> NSH_FLAGS_MASK) |
> + ((ttl << NSH_TTL_SHIFT) & NSH_TTL_MASK) |
> + ((len << NSH_LEN_SHIFT) & NSH_LEN_MASK),
> + NSH_FLAGS_MASK | NSH_TTL_MASK | NSH_LEN_MASK);
> +}
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.
For symmetry you should also add a function setter function for path_hdr:
static inline void
nsh_set_path_hdr(struct nsh_hdr *nsh, ovs_be32 path_hdr)
{
put_16aligned_be32(&nsh->path_hdr, path_hdr);
}
> diff --git a/include/openvswitch/packets.h b/include/openvswitch/packets.h
> index be91e02..603ec44 100644
> --- a/include/openvswitch/packets.h
> +++ b/include/openvswitch/packets.h
>
> /* Network Service Header keys */
> -struct flow_nsh {
> +struct ovs_key_nsh {
> uint8_t flags;
> + uint8_t ttl;
> uint8_t mdtype;
> uint8_t np;
> - uint8_t si;
> - ovs_be32 spi;
> + ovs_be32 path_hdr;
> ovs_be32 c[4];
> };
Why did you rename this struct? As part of the struct flow, it was
more appropriately called struct flow_nsh.
> diff --git a/lib/flow.c b/lib/flow.c
> index b2b10aa..547ff70 100644
> --- a/lib/flow.c
> +++ b/lib/flow.c
> bool
> -parse_nsh(const void **datap, size_t *sizep, struct flow_nsh *key)
> +parse_nsh(const void **datap, size_t *sizep, struct ovs_key_nsh *key)
> {
[...]
> - /* Check if it is long enough for NSH header, doesn't support
> - * MD type 2 yet
> - */
> if (OVS_UNLIKELY(*sizep < NSH_M_TYPE1_LEN)) {
> return false;
> }
This should compare against the minimum fixed size (NSH_BASE_HDR_LEN)
of an NSH header, not NSH_M_TYPE1_LEN as we want to successfully parse
any MD type header.
> switch (key->mdtype) {
> case NSH_M_TYPE1:
> + if (length != NSH_M_TYPE1_LEN) {
> + return false;
> + }
> for (size_t i = 0; i < 4; i++) {
> key->c[i] = get_16aligned_be32(&nsh->md1.c[i]);
> }
> break;
> case NSH_M_TYPE2:
> - /* Don't support MD type 2 yet, so return false */
> + /* Don't support MD type 2 metedata parsing yet */
> + if (length < NSH_BASE_HDR_LEN) {
> + return false;
> + }
This check is superfluous as it has been done before.
> diff --git a/lib/meta-flow.c b/lib/meta-flow.c
> index 64a8cf1..b50679c 100644
> --- a/lib/meta-flow.c
> +++ b/lib/meta-flow.c
> @@ -906,10 +914,13 @@ mf_get_value(const struct mf_field *mf, const
> struct flow *flow,
> value->u8 = flow->nsh.np;
> break;
> case MFF_NSH_SPI:
> - value->be32 = flow->nsh.spi;
> + value->be32 = nsh_path_hdr_to_spi(flow->nsh.path_hdr);
> + if (value->be32 == htonl(NSH_SPI_MASK >> NSH_SPI_SHIFT)) {
> + value->be32 = OVS_BE32_MAX;
> + }
This looks strange. The SPI is a 24 bit field. Why would you expand it to OVS_BE32_MAX?
> @@ -1221,10 +1235,12 @@ mf_set_value(const struct mf_field *mf,
> MATCH_SET_FIELD_UINT8(match, nsh.np, value->u8);
> break;
> case MFF_NSH_SPI:
> - MATCH_SET_FIELD_BE32(match, nsh.spi, value->be32);
> + match->wc.masks.nsh.path_hdr |= htonl(NSH_SPI_MASK);
> + nsh_path_hdr_set_spi(&match->flow.nsh.path_hdr, value->be32);
Introduce a wrapper function match_set_nsh_spi() where you rely on the setter functions in nsh.h to manipulate the path_hdr.
> break;
> case MFF_NSH_SI:
> - MATCH_SET_FIELD_UINT8(match, nsh.si, value->u8);
> + match->wc.masks.nsh.path_hdr |= htonl(NSH_SI_MASK);
> + nsh_path_hdr_set_si(&match->flow.nsh.path_hdr, value->u8);
> break;
Dito.
> @@ -2103,10 +2126,12 @@ mf_set_wild(const struct mf_field *mf, struct
> match *match, char **err_str)
> MATCH_SET_FIELD_MASKED(match, nsh.np, 0, 0);
> break;
> case MFF_NSH_SPI:
> - MATCH_SET_FIELD_MASKED(match, nsh.spi, htonl(0), htonl(0));
> + match->wc.masks.nsh.path_hdr &= ~htonl(NSH_SPI_MASK);
> + nsh_path_hdr_set_spi(&match->flow.nsh.path_hdr, htonl(0));
Dito.
> break;
> case MFF_NSH_SI:
> - MATCH_SET_FIELD_MASKED(match, nsh.si, 0, 0);
> + match->wc.masks.nsh.path_hdr &= ~htonl(NSH_SI_MASK);
> + nsh_path_hdr_set_si(&match->flow.nsh.path_hdr, 0);
Dito.
> break;
> case MFF_NSH_C1:
> case MFF_NSH_C2:
> @@ -2363,10 +2391,14 @@ mf_set(const struct mf_field *mf,
> MATCH_SET_FIELD_MASKED(match, nsh.np, value->u8, mask->u8);
> break;
> case MFF_NSH_SPI:
> - MATCH_SET_FIELD_MASKED(match, nsh.spi, value->be32, mask-
> >be32);
> + match->wc.masks.nsh.path_hdr |= mask->be32;
I believe this is incorrect. The mask is related to the value and requires shifting.
One more reason to use wrapper functions.
> + nsh_path_hdr_set_spi(&match->flow.nsh.path_hdr,
> + value->be32 & mask->be32);
> break;
> case MFF_NSH_SI:
> - MATCH_SET_FIELD_MASKED(match, nsh.si, value->u8, mask->u8);
> + match->wc.masks.nsh.path_hdr |= htonl(mask->u8);
Dito.
> + nsh_path_hdr_set_si(&match->flow.nsh.path_hdr,
> + value->u8 & mask->u8);
> diff --git a/lib/meta-flow.xml b/lib/meta-flow.xml
> index 065fb03..a4a0735 100644
> --- a/lib/meta-flow.xml
> +++ b/lib/meta-flow.xml
> @@ -1311,7 +1311,9 @@ tcp,tp_src=0x07c0/0xfff0
>
> <group title="Network Service Header">
> <field id="MFF_NSH_FLAGS"
> - title="flags field (8 bits)"/>
> + title="flags field (2 bits)"/>
The NSH_FLAGS field requires more explanation as it is not defined in the RFC.
> diff --git a/lib/nx-match.c b/lib/nx-match.c
> index b782e8c..dc52566 100644
> --- a/lib/nx-match.c
> +++ b/lib/nx-match.c
> @@ -1157,13 +1158,22 @@ nx_put_raw(struct ofpbuf *b, enum
> ofp_version oxm, const struct match *match,
> /* Network Service Header */
> nxm_put_8m(&ctx, MFF_NSH_FLAGS, oxm, flow->nsh.flags,
> match->wc.masks.nsh.flags);
> + nxm_put_8m(&ctx, MFF_NSH_TTL, oxm, flow->nsh.ttl,
> + match->wc.masks.nsh.ttl);
> nxm_put_8m(&ctx, MFF_NSH_MDTYPE, oxm, flow->nsh.mdtype,
> match->wc.masks.nsh.mdtype);
> nxm_put_8m(&ctx, MFF_NSH_NP, oxm, flow->nsh.np,
> match->wc.masks.nsh.np);
> - nxm_put_32m(&ctx, MFF_NSH_SPI, oxm, flow->nsh.spi,
> - match->wc.masks.nsh.spi);
> - nxm_put_8m(&ctx, MFF_NSH_SI, oxm, flow->nsh.si, match-
> >wc.masks.nsh.si);
> + spi_mask = nsh_path_hdr_to_spi(match->wc.masks.nsh.path_hdr);
> + if (spi_mask == htonl(NSH_SPI_MASK >> NSH_SPI_SHIFT)) {
> + spi_mask = OVS_BE32_MAX;
> + }
> + nxm_put_32m(&ctx, MFF_NSH_SPI, oxm,
> + nsh_path_hdr_to_spi(flow->nsh.path_hdr),
> + spi_mask);
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.
> diff --git a/lib/odp-execute.c b/lib/odp-execute.c
> index 5f4d23a..601c8b6 100644
> --- a/lib/odp-execute.c
> +++ b/lib/odp-execute.c
> @@ -277,10 +277,10 @@ odp_set_nsh(struct dp_packet *packet, const
> struct ovs_key_nsh *key,
> const struct ovs_key_nsh *mask)
> {
> struct nsh_hdr *nsh = dp_packet_l3(packet);
> + ovs_be32 path_hdr;
>
> if (!mask) {
> - nsh->ver_flags_len = htons(key->flags << NSH_FLAGS_SHIFT) |
> - (nsh->ver_flags_len & ~htons(NSH_FLAGS_MASK));
> + nsh_set_flags_and_ttl(nsh, key->flags, key->ttl);
> put_16aligned_be32(&nsh->path_hdr, key->path_hdr);
> switch (nsh->md_type) {
> case NSH_M_TYPE1:
> @@ -294,15 +294,28 @@ odp_set_nsh(struct dp_packet *packet, const
[...]
> } else {
> - uint8_t flags = (ntohs(nsh->ver_flags_len) & NSH_FLAGS_MASK) >>
> - NSH_FLAGS_SHIFT;
> + uint8_t flags = nsh_get_flags(nsh);
> + uint8_t ttl = nsh_get_ttl(nsh);
> +
> flags = key->flags | (flags & ~mask->flags);
> - nsh->ver_flags_len = htons(flags << NSH_FLAGS_SHIFT) |
> - (nsh->ver_flags_len & ~htons(NSH_FLAGS_MASK));
> + ttl = key->ttl | (ttl & ~mask->ttl);
> + nsh_set_flags_and_ttl (nsh, flags, ttl);
> +
> + uint32_t spi = ntohl(nsh_get_spi(nsh));
> + uint8_t si = nsh_get_si(nsh);
> + uint32_t spi_mask = nsh_path_hdr_to_spi_uint32(mask->path_hdr);
> + uint8_t si_mask = nsh_path_hdr_to_si(mask->path_hdr);
>
> - ovs_be32 path_hdr = get_16aligned_be32(&nsh->path_hdr);
> - path_hdr = key->path_hdr | (path_hdr & ~mask->path_hdr);
> + if (spi_mask == 0x00ffffff) {
> + spi_mask = UINT32_MAX;
> + }
> + spi = nsh_path_hdr_to_spi_uint32(key->path_hdr) | (spi &
> ~spi_mask);
> + si = nsh_path_hdr_to_si(key->path_hdr) | (si & ~si_mask);
> + path_hdr = nsh_get_path_hdr(nsh);
> + nsh_path_hdr_set_spi(&path_hdr, htonl(spi));
> + nsh_path_hdr_set_si(&path_hdr, si);
> put_16aligned_be32(&nsh->path_hdr, path_hdr);
The above is quite obscure and far more complicated than necessary.
Could be simplified as follows:
uint8_t flags = nsh_get_flags(nsh);
uint8_t ttl = nsh_get_ttl(nsh);
ovs_be32 path_hdr = nsh_get_path_hdr(nsh);
flags = key->flags | (flags & ~mask->flags);
ttl = key->ttl | (ttl & ~mask->ttl);
path_hdr = key->path_hdr | (path_hdr & ~mask->path_hdr);
nsh_set_flags(nsh, flags);
nsh_set_ttl(nsh, ttl);
nsh_set_path_hdr(nsh, path_hdr);
> diff --git a/lib/odp-util.c b/lib/odp-util.c
> index 4f1499e..ac286ea 100644
> --- a/lib/odp-util.c
> +++ b/lib/odp-util.c
> @@ -319,17 +320,16 @@ format_nsh_key_mask(struct ds *ds, const struct
> ovs_key_nsh *key,
> format_nsh_key(ds, key);
> } else {
> bool first = true;
> - uint32_t spi = (ntohl(key->path_hdr) & NSH_SPI_MASK) >>
> NSH_SPI_SHIFT;
> - uint32_t spi_mask = (ntohl(mask->path_hdr) & NSH_SPI_MASK) >>
> - NSH_SPI_SHIFT;
> - if (spi_mask == 0x00ffffff) {
> + uint32_t spi = nsh_path_hdr_to_spi_uint32(key->path_hdr);
> + uint32_t spi_mask = nsh_path_hdr_to_spi_uint32(mask->path_hdr);
> + if (spi_mask == (NSH_SPI_MASK >> NSH_SPI_SHIFT)) {
> spi_mask = UINT32_MAX;
> }
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
would be clearer to introduce a dedicated formatting function for SPI.
> - uint8_t si = (ntohl(key->path_hdr) & NSH_SI_MASK) >> NSH_SI_SHIFT;
> - uint8_t si_mask = (ntohl(mask->path_hdr) & NSH_SI_MASK) >>
> - NSH_SI_SHIFT;
> + uint8_t si = nsh_path_hdr_to_si(key->path_hdr);
> + uint8_t si_mask = nsh_path_hdr_to_si(mask->path_hdr);
>
> format_uint8_masked(ds, &first, "flags", key->flags, mask->flags);
> + format_uint8_masked(ds, &first, "ttl", key->ttl, mask->ttl);
> format_uint8_masked(ds, &first, "mdtype", key->mdtype, mask-
> >mdtype);
> format_uint8_masked(ds, &first, "np", key->np, mask->np);
> format_be32_masked(ds, &first, "spi", htonl(spi), htonl(spi_mask));
> @@ -1808,17 +1810,20 @@ parse_odp_encap_nsh_action(const char *s,
> struct ofpbuf *actions)
> break;
> }
>
> - if (ovs_scan_len(s, &n, "flags=%"SCNi8, &encap_nsh.flags)) {
> + if (ovs_scan_len(s, &n, "flags=%"SCNi8, &encap_nsh->flags)) {
> continue;
Any validity check for value flags<=3?
> }
> - if (ovs_scan_len(s, &n, "mdtype=%"SCNi8, &encap_nsh.mdtype)) {
> - switch (encap_nsh.mdtype) {
> + if (ovs_scan_len(s, &n, "ttl=%"SCNi8, &encap_nsh->ttl)) {
> + continue;
Any validity check for ttl < 64?
> @@ -1826,24 +1831,24 @@ parse_odp_encap_nsh_action(const char *s,
> struct ofpbuf *actions)
> }
> continue;
> }
> - if (ovs_scan_len(s, &n, "np=%"SCNi8, &encap_nsh.np)) {
> + if (ovs_scan_len(s, &n, "np=%"SCNi8, &encap_nsh->np)) {
> continue;
> }
> if (ovs_scan_len(s, &n, "spi=0x%"SCNx32, &spi)) {
> - encap_nsh.path_hdr =
> + encap_nsh->path_hdr =
> htonl(((spi << NSH_SPI_SHIFT) & NSH_SPI_MASK) |
> - (ntohl(encap_nsh.path_hdr) & ~NSH_SPI_MASK));
> + (ntohl(encap_nsh->path_hdr) & ~NSH_SPI_MASK));
> continue;
Why not use the access functions in nsh.h here?
> }
> if (ovs_scan_len(s, &n, "si=%"SCNi8, &si)) {
> - encap_nsh.path_hdr =
> + encap_nsh->path_hdr =
> htonl((si << NSH_SI_SHIFT) |
> - (ntohl(encap_nsh.path_hdr) & ~NSH_SI_MASK));
> + (ntohl(encap_nsh->path_hdr) & ~NSH_SI_MASK));
> continue;
Dito.
> @@ -1861,28 +1866,29 @@ parse_odp_encap_nsh_action(const char *s,
> struct ofpbuf *actions)
> continue;
> }
> }
> - else if (encap_nsh.mdtype == NSH_M_TYPE2) {
> + else if (encap_nsh->mdtype == NSH_M_TYPE2) {
> struct ofpbuf b;
> char buf[512];
> size_t mdlen;
> if (ovs_scan_len(s, &n, "md2=0x%511[0-9a-fA-F]", buf)) {
> - ofpbuf_use_stub(&b, encap_nsh.metadata,
> - OVS_ENCAP_NSH_MAX_MD_LEN);
> + ofpbuf_use_stub(&b, encap_nsh->metadata,
> + NSH_CTX_HDRS_MAX_LEN);
> ofpbuf_put_hex(&b, buf, &mdlen);
Here it might be necessary to pad the metadata buffer with zeros to 4 bytes.
> @@ -6798,19 +6774,20 @@ odp_put_encap_nsh_action(struct ofpbuf
> *odp_actions,
[...]
> - switch (encap_nsh.mdtype) {
> + switch (encap_nsh->mdtype) {
> case NSH_M_TYPE1: {
> struct nsh_md1_ctx *md1 =
> - ALIGNED_CAST(struct nsh_md1_ctx *, encap_nsh.metadata);
> - encap_nsh.mdlen = NSH_M_TYPE1_MDLEN;
> + ALIGNED_CAST(struct nsh_md1_ctx *, encap_nsh->metadata);
> + encap_nsh->mdlen = NSH_M_TYPE1_MDLEN;
> for (int i = 0; i < 4; i++) {
> put_16aligned_be32(&md1->c[i], flow->nsh.c[i]);
No need for put_16aligned_be32() here as the encap_nsh action and the metadata included are 8-byte aligned.
> diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
> index 9e1f837..1b99ad5 100644
> --- a/ofproto/ofproto-dpif-xlate.c
> +++ b/ofproto/ofproto-dpif-xlate.c
> @@ -5885,10 +5885,10 @@ rewrite_flow_encap_nsh(struct xlate_ctx *ctx,
> /* Populate the flow with the new NSH header. */
> flow->packet_type = htonl(PT_NSH);
> flow->dl_type = htons(ETH_TYPE_NSH);
> - flow->nsh.flags = 0; /* */
> + flow->nsh.flags = 0;
> + flow->nsh.ttl = 63;
> flow->nsh.np = np;
> - flow->nsh.spi = 0;
> - flow->nsh.si = 255;
> + flow->nsh.path_hdr = htonl(255);
Use nsh setter functions instead?
> @@ -5901,6 +5901,7 @@ rewrite_flow_encap_nsh(struct xlate_ctx *ctx,
> } else if (md_type == NSH_M_TYPE2) {
> flow->nsh.mdtype = NSH_M_TYPE2;
> }
> + flow->nsh.mdtype &= NSH_MDTYPE_MASK;
Not really needed here, as we set the mdtype to a defined value.
I didn't check the unit test adaptations to the introduction of ttl field.
BR, Jan
More information about the dev
mailing list