[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