DPDK patches and discussions
 help / color / Atom feed
From: Ferruh Yigit <ferruh.yigit@intel.com>
To: Ophir Munk <ophirmu@nvidia.com>, "dev@dpdk.org" <dev@dpdk.org>,
	Wenzhuo Lu <wenzhuo.lu@intel.com>,
	Beilei Xing <beilei.xing@intel.com>,
	Bernard Iremonger <bernard.iremonger@intel.com>,
	Olivier Matz <olivier.matz@6wind.com>
Cc: Ophir Munk <ophirmu@mellanox.com>
Subject: Re: [dpdk-dev] [PATCH v5 1/3] app/testpmd: add GENEVE parsing
Date: Wed, 7 Oct 2020 17:25:11 +0100
Message-ID: <24b36e2f-e601-d8b6-9a9e-b626d1ee9fd4@intel.com> (raw)
In-Reply-To: <DM5PR12MB116152B83A0A86E3AC0A8A6FDC0A0@DM5PR12MB1161.namprd12.prod.outlook.com>

On 10/7/2020 3:52 PM, Ophir Munk wrote:
> Hi Ferruh,
> Please find comments inline.
> 
>> -----Original Message-----
>> From: Ferruh Yigit <ferruh.yigit@intel.com>
>> Sent: Tuesday, October 6, 2020 5:31 PM
>> To: Ophir Munk <ophirmu@nvidia.com>; dev@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.
> 

I am not sure about adding database for geneve ports, I think no need to make it 
more complex.

Only when user set via "--geneve-port=N", it is the port for testpmd to parse 
(same for 'geneve_udp_port' global variable), but when user give command "port 
config N add geneve X" it is to configure the NIC to offload the parsing.
This is too confusing, user can't know (or remember) this without checking the 
source code.

Can we rename the command and variable to highlight that it is for testpmd to 
parse, I think that will be enough, instead of trying to merge them, which is 
hard as you described above.

What do you think for "--testpmd-geneve-port=N" parameter and 
'testpmd_geneve_udp_port' variable name?
We can also update the documentation to say this is only the port testpmd uses 
for parsing, HW may be configured to use another port.


>>
>>> @@ -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.
> 

Would you mind adding the 'geneve' with the patch adding 'geneve' support (1/3), 
and add the other missing ones in the patch 3/3 ?

  reply index

Thread overview: 50+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-07-29  8:29 [dpdk-dev] [PATCH v1 0/3] Add GENEVE protocol parsing to testpmd Ophir Munk
2020-07-29  8:29 ` [dpdk-dev] [PATCH v1 1/3] app/testpmd: add GENEVE parsing Ophir Munk
2020-07-29  8:29 ` [dpdk-dev] [PATCH v1 2/3] app/testpmd: enable configuring GENEVE port Ophir Munk
2020-07-29  8:29 ` [dpdk-dev] [PATCH v1 3/3] app/testpmd: reduce tunnel parsing code duplication Ophir Munk
2020-08-27  7:02   ` [dpdk-dev] [PATCH v2 0/3] Add GENEVE protocol parsing to testpmd Ophir Munk
2020-08-27  7:02     ` [dpdk-dev] [PATCH v2 1/3] app/testpmd: add GENEVE parsing Ophir Munk
2020-09-14 17:27       ` Ferruh Yigit
2020-09-15 12:53       ` [dpdk-dev] [PATCH v3 0/3] Add GENEVE protocol parsing to testpmd Ophir Munk
2020-09-15 12:53         ` [dpdk-dev] [PATCH v3 1/3] app/testpmd: add GENEVE parsing Ophir Munk
2020-09-15 13:17           ` [dpdk-dev] [PATCH v4 0/3] Add GENEVE protocol parsing to testpmd Ophir Munk
2020-09-15 13:17             ` [dpdk-dev] [PATCH v4 1/3] app/testpmd: add GENEVE parsing Ophir Munk
2020-09-15 13:56               ` Ophir Munk
2020-09-17 12:23               ` Olivier Matz
2020-09-18 14:21                 ` Ophir Munk
2020-09-18 14:17               ` [dpdk-dev] [PATCH v5 0/3] Add GENEVE protocol parsing to testpmd Ophir Munk
2020-09-18 14:17                 ` [dpdk-dev] [PATCH v5 1/3] app/testpmd: add GENEVE parsing Ophir Munk
2020-10-06 14:30                   ` Ferruh Yigit
2020-10-07 14:52                     ` Ophir Munk
2020-10-07 16:25                       ` Ferruh Yigit [this message]
2020-10-08  8:44                         ` Ophir Munk
2020-10-08 13:37                           ` Ferruh Yigit
2020-10-07 15:30                   ` [dpdk-dev] [PATCH v6 0/3] Add GENEVE protocol parsing to testpmd Ophir Munk
2020-10-07 15:30                     ` [dpdk-dev] [PATCH v6 1/3] app/testpmd: add GENEVE parsing Ophir Munk
2020-10-08 20:16                       ` [dpdk-dev] [PATCH v7 0/3] Add GENEVE protocol parsing to testpmd Ophir Munk
2020-10-08 20:16                         ` [dpdk-dev] [PATCH v7 1/3] app/testpmd: add GENEVE parsing Ophir Munk
2020-10-08 20:16                         ` [dpdk-dev] [PATCH v7 2/3] app/testpmd: enable configuring GENEVE port Ophir Munk
2020-10-08 20:16                         ` [dpdk-dev] [PATCH v7 3/3] app/testpmd: tunnel parsing protocols cleanup Ophir Munk
2020-10-09 12:49                         ` [dpdk-dev] [PATCH v7 0/3] Add GENEVE protocol parsing to testpmd Ferruh Yigit
2020-10-07 15:30                     ` [dpdk-dev] [PATCH v6 2/3] app/testpmd: enable configuring GENEVE port Ophir Munk
2020-10-07 15:30                     ` [dpdk-dev] [PATCH v6 3/3] app/testpmd: tunnel parsing protocols cleanup Ophir Munk
2020-10-07 16:05                       ` Ferruh Yigit
2020-09-18 14:17                 ` [dpdk-dev] [PATCH v5 2/3] app/testpmd: enable configuring GENEVE port Ophir Munk
2020-09-18 14:17                 ` [dpdk-dev] [PATCH v5 3/3] app/testpmd: reduce tunnel parsing code duplication Ophir Munk
2020-10-06 14:30                   ` Ferruh Yigit
2020-10-07 10:56                     ` Ophir Munk
2020-10-06 14:58                 ` [dpdk-dev] [PATCH v5 0/3] Add GENEVE protocol parsing to testpmd Ferruh Yigit
2020-10-07 15:43                   ` Ophir Munk
2020-10-07 16:00                     ` Ferruh Yigit
2020-09-15 13:17             ` [dpdk-dev] [PATCH v4 2/3] app/testpmd: enable configuring GENEVE port Ophir Munk
2020-09-15 13:17             ` [dpdk-dev] [PATCH v4 3/3] app/testpmd: reduce tunnel parsing code duplication Ophir Munk
2020-09-15 12:53         ` [dpdk-dev] [PATCH v3 2/3] app/testpmd: enable configuring GENEVE port Ophir Munk
2020-09-15 12:53         ` [dpdk-dev] [PATCH v3 3/3] app/testpmd: reduce tunnel parsing code duplication Ophir Munk
2020-08-27  7:02     ` [dpdk-dev] [PATCH v2 2/3] app/testpmd: enable configuring GENEVE port Ophir Munk
2020-09-14 17:31       ` Ferruh Yigit
2020-09-15  8:46         ` Ophir Munk
2020-09-15 11:07           ` Ferruh Yigit
2020-09-15 12:59             ` Ophir Munk
2020-09-15 13:19               ` Ophir Munk
2020-08-27  7:02     ` [dpdk-dev] [PATCH v2 3/3] app/testpmd: reduce tunnel parsing code duplication Ophir Munk
2020-08-31  6:40     ` [dpdk-dev] [PATCH v2 0/3] Add GENEVE protocol parsing to testpmd Ophir Munk

Reply instructions:

You may reply publically to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=24b36e2f-e601-d8b6-9a9e-b626d1ee9fd4@intel.com \
    --to=ferruh.yigit@intel.com \
    --cc=beilei.xing@intel.com \
    --cc=bernard.iremonger@intel.com \
    --cc=dev@dpdk.org \
    --cc=olivier.matz@6wind.com \
    --cc=ophirmu@mellanox.com \
    --cc=ophirmu@nvidia.com \
    --cc=wenzhuo.lu@intel.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

DPDK patches and discussions

Archives are clonable:
	git clone --mirror https://inbox.dpdk.org/dev/0 dev/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 dev dev/ https://inbox.dpdk.org/dev \
		dev@dpdk.org
	public-inbox-index dev


Newsgroup available over NNTP:
	nntp://inbox.dpdk.org/inbox.dpdk.dev


AGPL code for this site: git clone https://public-inbox.org/ public-inbox