[v5,0/3] Add GENEVE protocol parsing to testpmd

  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
http://patches.dpdk.org/patch/77734/

Ophir Munk (3):
  app/testpmd: add GENEVE parsing
  app/testpmd: enable configuring GENEVE port
  app/testpmd: reduce tunnel parsing code duplication

 app/test-pmd/csumonly.c               | 120 ++++++++++++++++++++++------------
 app/test-pmd/parameters.c             |  14 +++-
 app/test-pmd/testpmd.h                |   1 +
 doc/guides/testpmd_app_ug/run_app.rst |   5 ++
 lib/librte_net/meson.build            |   3 +-
 lib/librte_net/rte_geneve.h           |  66 +++++++++++++++++++
 lib/librte_net/rte_vxlan.h            |   1 +
 7 files changed, 165 insertions(+), 45 deletions(-)
 create mode 100644 lib/librte_net/rte_geneve.h
  

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
> http://patches.dpdk.org/patch/77734/
> 
> 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?

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?
  

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

Please see more comments inline.

> -----Original Message-----
> From: Ferruh Yigit <ferruh.yigit@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?
  

On 10/7/2020 4:43 PM, Ophir Munk wrote:
> Hi Ferruh,
> I sent V6 which addresses your last review.
> 
> Please see more comments inline.
> 
>> -----Original Message-----
>> From: Ferruh Yigit <ferruh.yigit@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?
> 

I am asking about forwarding engine name, it is 'csum' and in documentation it 
is described as:
"
Changes the checksum field with hardware or software methods depending on the 
offload flags on the packet.
"

So the initial purpose of the forwarding engine is to calculate checksums (or 
offload calculation to HW) before sending packets, but now it detects and parses 
the tunneling protocols and updates packet accordingly and logs it.
It also does GRO and GSO.

Is the forwarding engine name 'csum' still make sense, or should we find 
something else. I don't have any suggestion, just the question.