Hi Yi, Some more comments below.
/Jan > -----Original Message----- > From: Yang, Yi [mailto:yi.y.y...@intel.com] > Sent: Tuesday, 29 August, 2017 08:31 > To: Jan Scheurich <jan.scheur...@ericsson.com> > Cc: d...@openvswitch.org; b...@ovn.org > Subject: Re: [PATCH v4 2/3] nsh: add new flow key 'ttl' > > > > > 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? Not arguing alignment here. That resulted in merging spi and si into path_hdr and is OK. All structs ovs_key_xxx are defined in datapath/linux/.../openvswitch.h, while all structs flow_xxx are defined in lib/packets.h. You have defined struct ovs_key_nsh in lib/packets.h and refer to it both in struct flow and in the netlink attribute. This violates the standard practices. Even if they are isomorphic, I think we should have independent type definitions and a (trivial) conversion function. > > > > > +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. The use of __* prefix is reserved for the C compiler. In OVS we typically use *__ postfix for internal functions. > > > +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. They are set in different combinations. I'd say it is better not have a dedicated setter function for every required combination. Only a significant run-time overhead might tempt one to introduce collapsed functions. > > 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 :-) I don't agree. You have nsh_get_path_hdr(), so why are set and get handled differently? The main point of these functions is to shield the user from the nasty alignment details of struct nsh_hdr when accessing the packet headers. > > > > 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. Please see above. Struct ovs_key_nsh should be defined in datapath/linux/.../openvswitch.h to be used on the netlink interface. struct flow_nsh should be defined here in lib/packets.h and used in struct flow. > > > 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. MFF_MPLS_LABEL is not comparable because the label is not maskable. MFF_IPV6_LABEL, in contrast, is a maskable 20 bit field in a 32 bit OXM container. And this is about including the mask in the in the OXM. But I double checked the MFF_IPV6_LABEL code. It indeed sets the internal Mask to OVS_BE32_MAX for an exact match of the IPv6 label field. That's why we can use the generic nxm_put_32_m() function to encode the MFF_IPV6_LABEL OXM into the OpenFlow match. That function internally calls is_all_ones(mask, 4) to check for an exact match. We need to do the same for NSH SPI. So your code was doing the right thing. > > > 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. OK, see also my conclusion on mask expansion above. > > > @@ -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. This code is parsing the datapath action in an ovs-dpctl add-flow command. We cannot rely on that the hex string in the command is 4-byte aligned. If it is not, we must make sure that the buffer for the MD2 TLV is padded with zeros to 4 byte boundary, even though the TLV length field may be a non- integer multiple of 4. > > 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. The code in lib/ofp-actions.c parses the OpenFlow generic encap() action in ovs-ofctl commands. This is completely independent. > > > > > 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 _______________________________________________ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
