Re: [dpdk-dev] [PATCH v5 0/3] Add GENEVE protocol parsing to testpmd

[Ophir Munk]

Hi Ferruh,
I sent V6 which addresses your last review.

Please see more comments inline.

> -----Original Message-----
> From: Ferruh Yigit <ferruh.yi...@intel.com>
> Sent: Tuesday, October 6, 2020 5:59 PM
> On 9/18/2020 3:17 PM, Ophir Munk wrote:
> > v1:
> > Initial version
> > v2:
> > Rebased + Minor update in protocol options field:
> > char opts[0] ===> uint8_t opts[]
> > v3:
> > Rebase
> > document "geneve-port=N" parameter
> > v4:
> > Mispelling corrections
> > v5:
> > Rebase + Updates following review
> >
> https://nam11.safelinks.protection.outlook.com/?url=http%3A%2F%2Fpatch
> >
> es.dpdk.org%2Fpatch%2F77734%2F&amp;data=02%7C01%7Cophirmu%40nv
> idia.com
> >
> %7C1b01b5de39d24d61e38008d86a086ae2%7C43083d15727340c1b7db39e
> fd9ccc17a
> >
> %7C0%7C0%7C637375931691373733&amp;sdata=csLb5OdTmWlpv1k4Z7ZZ
> YN1b1d2cd8
> > %2BTxxnydgNnyQ4%3D&amp;reserved=0
> >
> > Ophir Munk (3):
> >    app/testpmd: add GENEVE parsing
> >    app/testpmd: enable configuring GENEVE port
> >    app/testpmd: reduce tunnel parsing code duplication
> >
> 
> Hi Ophir,
> 
> The patchset looks good except a few comments I put into the patches.
> 
> But I have two highlevel questions/comments,
> 
> 1) The testpmd tunnel parsing feature is not documented properly, there are
> various related commands but there is no documentation to put all together.
> What do you think putting a new section for it under the "Testpmd Runtime
> Functions" (testpmd_funcs.rst) with this patchset?

I prefer this to be in a separate patchset.
Geneve in testpmd is not complete yet. I know that there is current work in 
progress to add geneve options to flows.
Maybe its worth waiting till Geneve is finalized in testpmd.

> 
> 2) The 'csum' forwarding engine seems become forwarding engine for the
> case where packet payload needs to be parsed, like gro/gso, tunnel parse.
> Even the description of the forwarding engine in the documentation is not
> accurate now.
> I wonder if we should rename the forwarding engine at this stage?

Can you please be more specific to which renaming you are referring to?
Can you give examples?