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

[Ophir Munk]

Hi Ferruh,
Please find comments inline.

> -----Original Message-----
> From: Ferruh Yigit <ferruh.yigit at intel.com>
> Sent: Tuesday, October 6, 2020 5:31 PM
> To: Ophir Munk <ophirmu at nvidia.com>; dev at dpdk.org; Wenzhuo Lu
<...>
> >
> >   uint16_t vxlan_gpe_udp_port = 4790;
> > +uint16_t geneve_udp_port = RTE_GENEVE_DEFAULT_PORT;
> >
> 
> There is a testpmd command to configure the NIC for GENEVE port, "port
> config (port_id) udp_tunnel_port add|rm vxlan|geneve|vxlan-gpe
> (udp_port)"
> 
> You are adding support to testpmd to parse the GENEVE packets, but I think
> these two commads should be consistent.
> 
> What do you think "port config N add geneve X" update the
> 'geneve_udp_port=X"?
> 
> <...>
> 

It is not as simple as suggested.
The command "port config N add geneve X" can be repeated several times - adding another geneve port to the NIC Hardware/Firmware to recognize. As a result the NIC is configured with multiple geneve ports. On the other hand geneve_udp_port is a single variable - so whenever a new "port config" command is executed - we will override the last geneve port and forget all the precedent ones.
The port config command also has a "rm" option in addition to "add". So if we removed the last port - what will we assigned to geneve_udp_port? We need to have a small data base for managing geneve ports.
testpmd also has an option " --geneve-port=N". It should be synched with the "port config" command.

Another issue to consider is if there is a need for the suggested feature. I think the "port config" is mainly targeted for offloading. So the NIC will probably encap/decap a packet with geneve - leaving testpmd paring unneeded.

My point is that your suggestion requires an RFC before implementation.

> 
> > @@ -865,9 +925,17 @@ pkt_burst_checksum_forward(struct fwd_stream
> *fs)
> >   				}
> >   				parse_vxlan(udp_hdr, &info,
> >   					    m->packet_type);
> > -				if (info.is_tunnel)
> > +				if (info.is_tunnel) {
> >   					tx_ol_flags |=
> >   						PKT_TX_TUNNEL_VXLAN;
> > +					goto tunnel_update;
> > +				}
> > +				parse_geneve(udp_hdr, &info);
> > +				if (info.is_tunnel) {
> > +					tx_ol_flags |=
> > +						PKT_TX_TUNNEL_GENEVE;
> > +					goto tunnel_update;
> > +				}
> 
> Can you please update the "csum parse-tunnel" documentation to mention
> "geneve"
> protocol?

Done in v6. I added other missing tunnel protocols (in alphabetical order) such as "gtp". Since it is more than geneve I added it to the 3rd (cleanup) commit.