Hi Ferruh,
Please find comments inline.
> -----Original Message-----
> From: Ferruh Yigit <[email protected]>
> Sent: Tuesday, October 6, 2020 5:31 PM
> To: Ophir Munk <[email protected]>; [email protected]; 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.
