[ovs-dev] [PATCH v4 2/3] nsh: add new flow key 'ttl'
Yang, Yi
yi.y.yang at intel.com
Tue Aug 29 06:30:35 UTC 2017
On Tue, Aug 29, 2017 at 07:31:22AM +0800, Jan Scheurich wrote:
> 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.
>
Ok, I will change it to pad[3].
> > 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?
I have explained it in previous review, new ttl field results in it
isn't 64 bit aligned, moreover, we musnt't do some unnecessary
conversion if we use struct ovs_key_nsh, netlink also used struct
ovs_key_nsh, it is very reasonable to use struct ovs_key_nsh for this,
why don't we use it?
>
> > 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.
Make sense, we can add more information for this in lib/meta-flow.xml.
>
> > +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().
We just use __ to mark it as an internal function which will not be used
outside of nsh.h, this is C coding convention.
>
> > +
> > +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.
Basically these three functions will be executed together, so one is
better.
>
> For symmetry you should also add a function setter function for path_hdr:
I think put_16aligned_be32(&nsh->path_hdr, path_hdr) is better than
nsh_set_path_hdr, here isn't C++ programming :-)
>
> 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.
It isn't name issue, we want to use common struct ovs_key_nsh in
multiple function modules.
>
> > 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.
My bad, I will use NSH_BASE_HDR_LEN.
>
> > 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.
Yeah, I'll remove it.
>
> > 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?
Indeed, 24 bit mask is enough, I'll remove it.
>
> > @@ -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.
Make sense, I have done in new version.
>
> > 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.
You're right, but 4 nsh unit tests are ok, I'm not sure why. I have used
wrapper function to do this in new version.
>
> > + 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.
Ok, I'll add more information here.
>
> > 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.
MFF_IPV6_LABEL is different from this, MPLS_LABEL is same as this one.
But it will return BAD_MASK error if we don't expand it ot OVS_BE32_MAX
because mf_is_mask_valid expects mask is all one, the actual mask is
0x00FFFFFF. I have tried this many times, please do tell me a better solution
for this if you have a better one.
>
> > 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);
I tried the one you provided, it is ok, so I changed it to your version
in new version.
>
> > 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.
But a dedicated formatting function for SPI will have the same code as
format_be32_masked does, so we mustn't duplicate code.
>
> > - 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?
Added check in new version.
>
> > }
> > - 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?
Added check in ne version.
>
> > @@ -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?
Forgot that, Ben wanted me to do changes as less as possible :-), I
haved use nsh helper in new version.
>
> > }
> > 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.
This is parsing the data netlink message put, md2 hex string has been
formated which has followed metadata format, so I don't think this is
necessary, in addition, pad will be helpless because md2 hex string has
set tlv len, we can't change it because of such padding.
Actucally lib/ofp-actions.c has handled this, all the data is from
lib/ofp-actions.c, so we mustn't do duplicate and helpless thing here.
>
> > @@ -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.
I used memcpy(md1, flow->nsh.c, sizeof(*md1)) to do this in new version.
>
> > 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?
Done in new version.
>
> > @@ -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.
Removed it in new version.
>
> I didn't check the unit test adaptations to the introduction of ttl field.
I have changed nsh unit tests for ttl and dec_nsh_ttl, they have been
verified by nsh unit tests.
I'll send out new version after Ben gives me feedbacks.
>
> BR, Jan
More information about the dev
mailing list