Re: [dpdk-dev] [PATCH v4 1/3] app/testpmd: add GENEVE parsing

[Ophir Munk <ophirmu@nvidia.com>]

Hi Olivier,
Please find comments inline.
I sent v5 based on your review.
http://patches.dpdk.org/project/dpdk/list/?series=12354

> -----Original Message-----
> From: Olivier Matz <olivier.matz@6wind.com>
> Sent: Thursday, September 17, 2020 3:23 PM
> To: Ophir Munk <ophirmu@nvidia.com>
> Cc: dev@dpdk.org; Wenzhuo Lu <wenzhuo.lu@intel.com>; Beilei Xing
> <beilei.xing@intel.com>; Bernard Iremonger
> <bernard.iremonger@intel.com>; Ferruh Yigit <ferruh.yigit@intel.com>;
> Ophir Munk <ophirmu@mellanox.com>
> Subject: Re: [dpdk-dev] [PATCH v4 1/3] app/testpmd: add GENEVE parsing
> 
> Hi Ophir,
> 
> Please find some comments below.
> 
> On Tue, Sep 15, 2020 at 01:17:15PM +0000, Ophir Munk wrote:
> > From: Ophir Munk <ophirmu@mellanox.com>
> >
> > GENEVE is a widely used tunneling protocol in modern Virtualized
> > Networks. testpmd already supports parsing of several tunneling
> > protocols including VXLAN, VXLAN-GPE, GRE. This commit adds GENEVE
> > parsing of inner protocols (IPv4-0x0800, IPv6-0x86dd, Ethernet-0x6558)
> > based on IETF draft-ietf-nvo3-geneve-09. GENEVE is considered more
> > flexible than the other protocols.  In terms of protocol format GENEVE
> > header has a variable length options as opposed to other tunneling
> > protocols which have a fixed header size.
> >
> > Signed-off-by: Ophir Munk <ophirmu@mellanox.com>
> 
> 
> >  app/test-pmd/csumonly.c     | 70
> ++++++++++++++++++++++++++++++++++++++++++-
> >  app/test-pmd/testpmd.h      |  1 +
> >  lib/librte_net/meson.build  |  3 +-
> >  lib/librte_net/rte_geneve.h | 72
> > +++++++++++++++++++++++++++++++++++++++++++++
> >  4 files changed, 144 insertions(+), 2 deletions(-)  create mode
> > 100644 lib/librte_net/rte_geneve.h
> 
> An entry could be added in doc/api/doxy-api-index.md. Some more protocols
> are missing, I'll send a patch to add them.
> 

Thanks for sending a patch. I would appreciate it if it includes Geneve as well.

> > --- /dev/null
> > +++ b/lib/librte_net/rte_geneve.h
> > @@ -0,0 +1,72 @@
> > +/* SPDX-License-Identifier: BSD-3-Clause
> > + * Copyright 2020 Mellanox Technologies, Ltd  */
> > +
> > +#ifndef _RTE_GENEVE_H_
> > +#define _RTE_GENEVE_H_
> > +
> > +/**
> > + * @file
> > + *
> > + * GENEVE-related definitions
> > + */
> > +
> > +#include <stdint.h>
> > +
> > +#include <rte_udp.h>
> 
> Is this include needed? Maybe it comes from a copy/paste of VXLAN?
> 

The include was dropped in v5 (not needed).

> > +
> > +
> > +#ifdef __cplusplus
> > +extern "C" {
> > +#endif
> > +
> > +/** GENEVE default port. */
> > +#define RTE_GENEVE_DEFAULT_PORT 6081
> > +
> > +/**
> > + * GENEVE protocol header. (draft-ietf-nvo3-geneve-09)
> > + * Contains:
> > + * 2-bits version (must be 0).
> > + * 6-bits option length in four byte multiples, not including the eight
> > + *	bytes of the fixed tunnel header.
> > + * 1-bit control packet.
> > + * 1-bit critical options in packet.
> > + * 6-bits reserved
> > + * 16-bits Protocol Type. The protocol data unit after the Geneve header
> > + *	following the EtherType convention. Ethernet itself is represented by
> > + *	the value 0x6558.
> > + * 24-bits Virtual Network Identifier (VNI). Virtual network unique
> identified.
> > + * 8-bits reserved bits (must be 0 on transmission and ignored on receipt).
> > + * More-bits (optional) variable length options.
> > + */
> > +__extension__
> > +struct rte_geneve_hdr {
> > +#if RTE_BYTE_ORDER == RTE_BIG_ENDIAN
> > +	uint8_t ver:2;		/**< Version (2).  */
> 
> Isn't the (2) in the comment redundant with the :2 in the type?
> Here is how bitfield look like in doxygen documentation:
> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fdoc.
> dpdk.org%2Fapi%2Fstructrte__flow__attr.html%23ae4d19341d5298a2bc61f
> 9eb941b1179c&amp;data=02%7C01%7Cophirmu%40nvidia.com%7C95918f4
> 2491c4832d8a308d85b047a22%7C43083d15727340c1b7db39efd9ccc17a%7
> C0%7C0%7C637359422086641153&amp;sdata=LJjYkeHy9eHGc9xu42E%2F8
> h54dzxrmiO7V0NCeLXndpU%3D&amp;reserved=0
> 
> It's true that the field documentation miss the number of bits.
> So if you feel it's needed, I prefer something more explicit like "(2 bits)"
> instead of just "(2)".
> 

The number of bits is removed from the comments (v5).

> By the way, there are 2 spaces at the end of the comment.
> 

Extra spaces removed.

> > +	uint8_t opt_len:6;	/**< Options length (6). */
> > +	uint8_t oam:1;		/**< Control packet (1). */
> > +	uint8_t critical:1;	/**< Critical packet (1). */
> > +	uint8_t rsvd1:6;	/**< Reserved (6). */
> 
> "reserved" instead of "rsvd"?
> 

In v5 all "rsvd" fields are renamed as "reserved".

> The Internet-Draft says "Rsvd" for this one, but "Reserved" for the other.
> 
> > +#else
> > +	uint8_t opt_len:6;	/**< Options length (6). */
> > +	uint8_t ver:2;		/**< Version (2).  */
> > +	uint8_t rsvd1:6;	/**< Reserved (6). */
> > +	uint8_t critical:1;	/**< Critical packet (1). */
> > +	uint8_t oam:1;		/**< Control packet (1). */
> > +#endif
> > +	rte_be16_t proto;	/**< Protocol type (16). */
> > +	uint8_t vni[3];		/**< Virtual network identifier (24). */
> > +	uint8_t rsvd2;		/**< Reserved (8). */
> 
> vni is an identifier, so I wonder if it would make sense to have it as an integer
> instead of an array of uint8. Something like this:
> 
> 	#if RTE_BYTE_ORDER == RTE_BIG_ENDIAN
> 		uint32_t vni:24;
> 		uint32_t reserved2:8;
> 	#else
> 		uint32_t vni:24;
> 		uint32_t reserved2:8;
> 	#endif
> 

I suggest leaving the vni as is (array of unsigned chars) since I do not see a gain by changing it to an integer of 24 bits.
If we did change - it will not turn VNI from network order to host order. So in case you wanted to print the VNI (host order) you will have to manipulate the bytes order anyway based on endianness. 
What do you think?

> > +	uint8_t opts[];		/**<Variable length options. */
> 
> Since the option length is a multiple of four-bytes, would uint32_t[] make
> more sense here?
> 

opts[] type changed to uint32_t  in v5.

> Missing a space after "<".
> 

Space added.

> > +} __rte_packed;
> > +
> > +/* GENEVE next protocol types */
> > +#define RTE_GENEVE_TYPE_IPV4		0x0800 /**< IPv4 Protocol.
> */
> > +#define RTE_GENEVE_TYPE_IPV6		0x86dd /**< IPv6 Protocol.
> */
> > +#define RTE_GENEVE_TYPE_ETH		0x6558 /**< Ethernet
> Protocol. */
> 
> From what I understand in the draft, I think only RTE_GENEVE_TYPE_ETH is
> needed.
> 
>    Protocol Type (16 bits):  The type of the protocol data unit
>    appearing after the Geneve header.  This follows the EtherType
>    [ETYPES] convention; with Ethernet itself being represented by the
>    value 0x6558.
> 
> Shouldn't we use RTE_ETHER_TYPE_* instead of redefining them here?
> 
> 0x6558 is RTE_ETHER_TYPE_TEB (Transparent Ethernet Bridging) and is also
> used in case of NVGRE.
> 
> 

Definitions RTE_GENEVE_TYPE_IPV4 and RTE_GENEVE_TYPE_IPV6 dropped (remaining with RTE_GENEVE_TYPE_ETH only).

> Regards,
> Olivier

Regards,
Ophir