Re: [dpdk-dev] [PATCH v4] app/testpmd: enable GTP header parse and Tx checksum offload

["Xu, Ting" <ting.xu@intel.com>]

Hi, Ferruh, Olivier,

Thanks for your advice. I think I misunderstood the comments in the last commit.
This time I move the GTP header definition in the new file rte_gtp.h. Hope it would be better.

Thanks.

-----Original Message-----
From: Olivier Matz [mailto:olivier.matz@6wind.com] 
Sent: Tuesday, October 22, 2019 12:51 AM
To: Yigit, Ferruh <ferruh.yigit@intel.com>
Cc: Xu, Ting <ting.xu@intel.com>; dev@dpdk.org; Lu, Wenzhuo <wenzhuo.lu@intel.com>; Wu, Jingjing <jingjing.wu@intel.com>; Iremonger, Bernard <bernard.iremonger@intel.com>
Subject: Re: [dpdk-dev] [PATCH v4] app/testpmd: enable GTP header parse and Tx checksum offload

On Mon, Oct 21, 2019 at 11:52:46AM +0100, Ferruh Yigit wrote:
> On 10/21/2019 10:28 AM, Ferruh Yigit wrote:
> > On 10/21/2019 1:29 PM, Ting Xu wrote:
> >> This patch enables testpmd to forward GTP packet in csum fwd mode.
> >> GTP header structure (without optional fields and extension header) 
> >> and parser function are added. GTPU and GTPC packets are both 
> >> supported, with respective UDP destination port and GTP message 
> >> type.
> >>
> >> Signed-off-by: Ting Xu <ting.xu@intel.com>
> >>
> >> ---
> >> v4: Move GTP header defination to rte_ether.h
> >> v3: correct coding style issue.
> >> v2: modify commit log
> >> depend on patch: lib/mbuf: add GTP tunnel type flag.
> >> ---
> >>  app/test-pmd/csumonly.c    | 96 ++++++++++++++++++++++++++++++++++----
> >>  lib/librte_net/rte_ether.h | 23 +++++++++
> >>  2 files changed, 109 insertions(+), 10 deletions(-)
> > 
> > <...>
> > 
> >> --- a/lib/librte_net/rte_ether.h
> >> +++ b/lib/librte_net/rte_ether.h
> >> @@ -342,6 +342,29 @@ struct rte_vxlan_gpe_hdr {
> >>  			      sizeof(struct rte_vxlan_gpe_hdr))  /**< VXLAN-GPE tunnel 
> >> header length. */
> >>  
> >> +/**
> >> + * Simplified GTP protocol header.
> >> + * Contains 8-bit flag, 8-bit message type,
> >> + * 16-bit message length, 32-bit TEID.
> >> + * No optional fields and next extension header.
> >> + */
> >> +struct rte_gtp_hdr {
> >> +	uint8_t gtp_hdr_info;
> >> +	uint8_t msg_type;
> >> +	uint16_t msg_len;
> >> +	uint32_t teid;
> >> +} __attribute__((__packed__));
> >> +
> >> +/* GTP header length */
> >> +#define RTE_ETHER_GTP_HLEN \
> >> +	(sizeof(struct rte_udp_hdr) + sizeof(struct rte_gtp_hdr))
> >> +/* GTP next protocal type */
> >> +#define RTE_GTP_TYPE_IPV4 0x40
> >> +#define RTE_GTP_TYPE_IPV6 0x60
> >> +/* GTP destination port number */
> >> +#define RTE_GTPC_UDP_PORT 2123
> >> +#define RTE_GTPU_UDP_PORT 2152
> >> +
> >>  /**
> >>   * Extract VLAN tag information into mbuf
> >>   *
> >>
> > 
> > lgtm, @Oliver, any comment/objection on GTP part in rte_ether.h?
> > 
> 
> What about moving the defines to its own header, like rte_gtp? Is 
> these defines part of the ether?

+1

I also think it deserves its own file.