From: Olivier Matz <olivier.matz@6wind.com> 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 Date: Thu, 17 Sep 2020 14:23:14 +0200 Message-ID: <20200917122314.GQ21395@platinum> (raw) In-Reply-To: <20200915131717.18252-2-ophirmu@nvidia.com> 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. > --- /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? > + > + > +#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://doc.dpdk.org/api/structrte__flow__attr.html#ae4d19341d5298a2bc61f9eb941b1179c 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)". By the way, there are 2 spaces at the end of the comment. > + 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"? 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 > + uint8_t opts[]; /**<Variable length options. */ Since the option length is a multiple of four-bytes, would uint32_t[] make more sense here? Missing a space after "<". > +} __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. Regards, Olivier
next prev parent reply other threads:[~2020-09-17 12:23 UTC|newest] Thread overview: 50+ messages / expand[flat|nested] mbox.gz Atom feed top 2020-07-29 8:29 [dpdk-dev] [PATCH v1 0/3] Add GENEVE protocol parsing to testpmd Ophir Munk 2020-07-29 8:29 ` [dpdk-dev] [PATCH v1 1/3] app/testpmd: add GENEVE parsing Ophir Munk 2020-07-29 8:29 ` [dpdk-dev] [PATCH v1 2/3] app/testpmd: enable configuring GENEVE port Ophir Munk 2020-07-29 8:29 ` [dpdk-dev] [PATCH v1 3/3] app/testpmd: reduce tunnel parsing code duplication Ophir Munk 2020-08-27 7:02 ` [dpdk-dev] [PATCH v2 0/3] Add GENEVE protocol parsing to testpmd Ophir Munk 2020-08-27 7:02 ` [dpdk-dev] [PATCH v2 1/3] app/testpmd: add GENEVE parsing Ophir Munk 2020-09-14 17:27 ` Ferruh Yigit 2020-09-15 12:53 ` [dpdk-dev] [PATCH v3 0/3] Add GENEVE protocol parsing to testpmd Ophir Munk 2020-09-15 12:53 ` [dpdk-dev] [PATCH v3 1/3] app/testpmd: add GENEVE parsing Ophir Munk 2020-09-15 13:17 ` [dpdk-dev] [PATCH v4 0/3] Add GENEVE protocol parsing to testpmd Ophir Munk 2020-09-15 13:17 ` [dpdk-dev] [PATCH v4 1/3] app/testpmd: add GENEVE parsing Ophir Munk 2020-09-15 13:56 ` Ophir Munk 2020-09-17 12:23 ` Olivier Matz [this message] 2020-09-18 14:21 ` Ophir Munk 2020-09-18 14:17 ` [dpdk-dev] [PATCH v5 0/3] Add GENEVE protocol parsing to testpmd Ophir Munk 2020-09-18 14:17 ` [dpdk-dev] [PATCH v5 1/3] app/testpmd: add GENEVE parsing Ophir Munk 2020-10-06 14:30 ` Ferruh Yigit 2020-10-07 14:52 ` Ophir Munk 2020-10-07 16:25 ` Ferruh Yigit 2020-10-08 8:44 ` Ophir Munk 2020-10-08 13:37 ` Ferruh Yigit 2020-10-07 15:30 ` [dpdk-dev] [PATCH v6 0/3] Add GENEVE protocol parsing to testpmd Ophir Munk 2020-10-07 15:30 ` [dpdk-dev] [PATCH v6 1/3] app/testpmd: add GENEVE parsing Ophir Munk 2020-10-08 20:16 ` [dpdk-dev] [PATCH v7 0/3] Add GENEVE protocol parsing to testpmd Ophir Munk 2020-10-08 20:16 ` [dpdk-dev] [PATCH v7 1/3] app/testpmd: add GENEVE parsing Ophir Munk 2020-10-08 20:16 ` [dpdk-dev] [PATCH v7 2/3] app/testpmd: enable configuring GENEVE port Ophir Munk 2020-10-08 20:16 ` [dpdk-dev] [PATCH v7 3/3] app/testpmd: tunnel parsing protocols cleanup Ophir Munk 2020-10-09 12:49 ` [dpdk-dev] [PATCH v7 0/3] Add GENEVE protocol parsing to testpmd Ferruh Yigit 2020-10-07 15:30 ` [dpdk-dev] [PATCH v6 2/3] app/testpmd: enable configuring GENEVE port Ophir Munk 2020-10-07 15:30 ` [dpdk-dev] [PATCH v6 3/3] app/testpmd: tunnel parsing protocols cleanup Ophir Munk 2020-10-07 16:05 ` Ferruh Yigit 2020-09-18 14:17 ` [dpdk-dev] [PATCH v5 2/3] app/testpmd: enable configuring GENEVE port Ophir Munk 2020-09-18 14:17 ` [dpdk-dev] [PATCH v5 3/3] app/testpmd: reduce tunnel parsing code duplication Ophir Munk 2020-10-06 14:30 ` Ferruh Yigit 2020-10-07 10:56 ` Ophir Munk 2020-10-06 14:58 ` [dpdk-dev] [PATCH v5 0/3] Add GENEVE protocol parsing to testpmd Ferruh Yigit 2020-10-07 15:43 ` Ophir Munk 2020-10-07 16:00 ` Ferruh Yigit 2020-09-15 13:17 ` [dpdk-dev] [PATCH v4 2/3] app/testpmd: enable configuring GENEVE port Ophir Munk 2020-09-15 13:17 ` [dpdk-dev] [PATCH v4 3/3] app/testpmd: reduce tunnel parsing code duplication Ophir Munk 2020-09-15 12:53 ` [dpdk-dev] [PATCH v3 2/3] app/testpmd: enable configuring GENEVE port Ophir Munk 2020-09-15 12:53 ` [dpdk-dev] [PATCH v3 3/3] app/testpmd: reduce tunnel parsing code duplication Ophir Munk 2020-08-27 7:02 ` [dpdk-dev] [PATCH v2 2/3] app/testpmd: enable configuring GENEVE port Ophir Munk 2020-09-14 17:31 ` Ferruh Yigit 2020-09-15 8:46 ` Ophir Munk 2020-09-15 11:07 ` Ferruh Yigit 2020-09-15 12:59 ` Ophir Munk 2020-09-15 13:19 ` Ophir Munk 2020-08-27 7:02 ` [dpdk-dev] [PATCH v2 3/3] app/testpmd: reduce tunnel parsing code duplication Ophir Munk 2020-08-31 6:40 ` [dpdk-dev] [PATCH v2 0/3] Add GENEVE protocol parsing to testpmd Ophir Munk
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=20200917122314.GQ21395@platinum \ --to=olivier.matz@6wind.com \ --cc=beilei.xing@intel.com \ --cc=bernard.iremonger@intel.com \ --cc=dev@dpdk.org \ --cc=ferruh.yigit@intel.com \ --cc=ophirmu@mellanox.com \ --cc=ophirmu@nvidia.com \ --cc=wenzhuo.lu@intel.com \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: link
DPDK patches and discussions This inbox may be cloned and mirrored by anyone: git clone --mirror https://inbox.dpdk.org/dev/0 dev/git/0.git # If you have public-inbox 1.1+ installed, you may # initialize and index your mirror using the following commands: public-inbox-init -V2 dev dev/ https://inbox.dpdk.org/dev \ dev@dpdk.org public-inbox-index dev Example config snippet for mirrors. Newsgroup available over NNTP: nntp://inbox.dpdk.org/inbox.dpdk.dev AGPL code for this site: git clone https://public-inbox.org/public-inbox.git