On Wed, Aug 30, 2017 at 01:02:07AM +0800, Jan Scheurich wrote:
> 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.
Jan, this is just a way we can aviod breaking kernel uAPI, I also
defined struct ovs_key_nsh in net/openvswitch/flow.h not in
datapath/linux/.../openvswitch.h in kernel datapath patch.
>
> > >
> > > > +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.
Ok, let me move __ as suffix.
>
> > > > +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.
I want to remove nsh_get_path_hdr, I don't want nsh.h to depend on
other stuff. put_16aligned_be32 needs to be defined in nsh.h for this.
>
>
> > >
> > > 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.
Please see my comments in previous section.
>
> > > > 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.
But spi isn't maskable. For MFF_MPLS_LABEL, it used nxm_put_32 not
nxm_put_32m, so it didn't have this issue.
/* "nsh_spi" (aka "nsp").
*
* spi (service path identifier) field in NSH base header.
*
* Type: be32 (low 24 bits).
* Maskable: no.
* Formatting: hexadecimal.
* Prerequisites: NSH.
* Access: read/write.
* NXM: none.
* OXM: NXOXM_NSH_SPI(5) since OF1.3 and v2.8.
*/
>
> > > > 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.
Will add padding for 4 bytes alignment in new version.
>
> >
> > 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
