From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga09.intel.com (mga09.intel.com [134.134.136.24]) by dpdk.org (Postfix) with ESMTP id B18F7803F for ; Fri, 12 Dec 2014 04:48:44 +0100 (CET) Received: from orsmga003.jf.intel.com ([10.7.209.27]) by orsmga102.jf.intel.com with ESMTP; 11 Dec 2014 19:47:08 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.04,691,1406617200"; d="scan'208";a="497617361" Received: from pgsmsx104.gar.corp.intel.com ([10.221.44.91]) by orsmga003.jf.intel.com with ESMTP; 11 Dec 2014 19:44:40 -0800 Received: from shsmsx103.ccr.corp.intel.com (10.239.4.69) by PGSMSX104.gar.corp.intel.com (10.221.44.91) with Microsoft SMTP Server (TLS) id 14.3.195.1; Fri, 12 Dec 2014 11:48:29 +0800 Received: from shsmsx101.ccr.corp.intel.com ([169.254.1.110]) by SHSMSX103.ccr.corp.intel.com ([169.254.4.240]) with mapi id 14.03.0195.001; Fri, 12 Dec 2014 11:48:27 +0800 From: "Liu, Jijiang" To: Olivier MATZ Thread-Topic: [dpdk-dev] [PATCH v3 0/3] enhance TX checksum command and csum forwarding engine Thread-Index: AQHQFSv0txinLaq/UEmWgzF3v0CZupyLMv5g Date: Fri, 12 Dec 2014 03:48:26 +0000 Message-ID: <1ED644BD7E0A5F4091CF203DAFB8E4CC01DA1B70@SHSMSX101.ccr.corp.intel.com> References: <1418173403-30202-1-git-send-email-jijiang.liu@intel.com> <54896F4A.4070601@6wind.com> In-Reply-To: <54896F4A.4070601@6wind.com> Accept-Language: en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-originating-ip: [10.239.127.40] Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 Cc: "dev@dpdk.org" Subject: Re: [dpdk-dev] [PATCH v3 0/3] enhance TX checksum command and csum forwarding engine X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: patches and discussions about DPDK List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 12 Dec 2014 03:48:46 -0000 Hi Olivier, Thanks for your comments. > -----Original Message----- > From: Olivier MATZ [mailto:olivier.matz@6wind.com] > Sent: Thursday, December 11, 2014 6:18 PM > To: Liu, Jijiang; dev@dpdk.org > Subject: Re: [dpdk-dev] [PATCH v3 0/3] enhance TX checksum command and > csum forwarding engine >=20 > Hi Jijiang, >=20 > Sorry for the late review, I was very busy these last days. Please find m= y > comments below. >=20 > On 12/10/2014 02:03 AM, Jijiang Liu wrote: > > In the current codes, the "tx_checksum set (ip|udp|tcp|sctp|vxlan) (hw|= sw) > (port-id)" command is not easy to understand and extend, so the patch set > enhances the tx_checksum command and reworks csum forwarding engine due > to the change of tx_checksum command. > > The main changes of the tx_checksum command are listed below, > > > > 1> add "tx_checksum set tunnel (hw|sw|none) (port-id)" command > > > > The command is used to set/clear tunnel flag that is used to tell the N= IC that the > packetg is a tunneing packet and application want hardware TX checksum of= fload > for outer layer, or inner layer, or both. >=20 > packetg -> packet > tunneing -> tunneling >=20 > I don't understand the description: this flag cannot be set to tell the N= IC that it's a > tunnel packet because it's a configuration flag. > Whatever the value of this configuration option, the packets can be eithe= r tunnel > or non-tunnel packets. The real question is, what is the behavior for eac= h packet > type for each value for this option. Ok, Will replace the above the description with the following: The 'hw/sw' option is used to set/clear the flag of enabling TX tunneling = packet checksum hardware offload in testpmd application. > > The 'none' option means that user treat tunneling packet as ordinary pa= cket > when using the csum forward engine. > > for example, let say we have a tunnel packet: > eth_hdr_out/ipv4_hdr_out/udp_hdr_out/vxlan_hdr/ehtr_hdr_in/ipv4_hdr_in/tc= p > _hdr_in. one of several scenarios: > > > > 1) User requests HW offload for ipv4_hdr_out checksum, and doesn't car= e is it > a tunnelled packet or not. So he sets: >=20 > tunnelled -> tunneled >=20 > > > > tx_checksum set tunnel none 0 > > > > tx_checksum set ip hw 0 > > > > So for such case we should set tx_tunnel to 'none'. > > > > 2> add "tx_checksum set outer-ip (hw|sw) (port-id)" command > > > > The command is used to set/clear outer IP flag that is used to tell the= NIC that > application want hardware offload for outer layer. > > > > 3> remove the 'vxlan' option from the "tx_checksum set > > 3> (ip|udp|tcp|sctp|vxlan) (hw|sw) (port-id)" command > > > > The command is used to set IP, UDP, TCP and SCTP TX checksum flag. In t= he case > of tunneling packet, the IP, UDP, TCP and SCTP flags always concern inner= layer. > > > > Moreover, replace the TESTPMD_TX_OFFLOAD_VXLAN_CKSUM flag with > TESTPMD_TX_OFFLOAD_TUNNEL_CKSUM flag and add the > TESTPMD_TX_OFFLOAD_OUTER_IP_CKSUM and > TESTPMD_TX_OFFLOAD_NON_TUNNEL_CKSUM flag in test-pmd application. >=20 > You are mixing scenario descriptions with what you do in your patchset: > 1/ is a scenario > 2/ and 3/ are descriptions of added/removed commands No. Please note the symbols for command descriptions and scenario descriptions= . The command descriptions with ">" symbol. 1> add "tx_checksum set tunnel (hw|sw|none) (port-id)" command 2> add "tx_checksum set outer-ip (hw|sw) (port-id)" command 3> (ip|udp|tcp|sctp|vxlan) (hw|sw) (port-id)" command The scenario descriptions with ")" symbol. 1) User requests HW offload for ipv4_hdr_out checksum, and doesn't care is= it a tunneled packet or not. So he sets: > Let's first sumarize what was the behavior before this patch. This is the > description in csumonly code. >=20 > Receive a burst of packets, and for each packet: > - parse packet, and try to recognize a supported packet type (1) > - if it's not a supported packet type, don't touch the packet, else: > - modify the IPs in inner headers and in outer headers if any > - reprocess the checksum of all supported layers. This is done in SW > or HW, depending on testpmd command line configuration > - if TSO is enabled in testpmd command line, also flag the mbuf for TCP > segmentation offload (this implies HW TCP checksum) Then transmit pac= kets on > the output port. >=20 > (1) Supported packets are: > Ether / (vlan) / IP|IP6 / UDP|TCP|SCTP . > Ether / (vlan) / outer IP|IP6 / outer UDP / VxLAN / Ether / IP|IP6 / > UDP|TCP|SCTP >=20 > The testpmd command line for this forward engine sets the flags > TESTPMD_TX_OFFLOAD_* in ports[tx_port].tx_ol_flags. They control wether a > checksum must be calculated in software or in hardware. The IP, UDP, TCP = and > SCTP flags always concern the inner layer. The VxLAN flag concerns the o= uter IP > (if packet is recognized as a vxlan packet). >=20 > From this description, it is easy to deduct this table: >=20 > Packet type 1: > Ether / (vlan) / IP|IP6 / UDP|TCP|SCTP . >=20 > Packet type 2 > Ether / (vlan) / outer IP|IP6 / outer UDP / VxLAN / Ether / IP|IP6 / > UDP|TCP|SCTP >=20 > +---------------------------+-------------------+-------------------+ > |test-pmd config \ packets |Packet type 1 |Packet type 2 | > +---------------------------+-------------------+-------------------+ > |whatever the flags |IP addresses |inner and outer IP | > | |incremented |addresses | > | | |incremented | > +---------------------------+-------------------+-------------------+ > |IP =3D sw |IP cksum is sw |inner IP cksum is | > | | |sw | > +---------------------------+-------------------+-------------------+ > |IP =3D hw |IP cksum is hw |inner IP cksum is | > | |(using lX_len) |hw (using lX_len) | > +---------------------------+-------------------+-------------------+ > |UDP or TCP or SCTP =3D sw |L4 cksum is sw |inner L4 cksum is | > | | |sw | > +---------------------------+-------------------+-------------------+ > |UDP or TCP or SCTP =3D hw |L4 cksum is hw |inner L4 cksum is | > | |(using lX_len) |hw (using lX_len) | > +---------------------------+-------------------+-------------------+ > |VxLAN =3D sw |N/A |outer IP cksum is | > | | |sw, outer UDP cksum| > | | |is sw | > +---------------------------+-------------------+-------------------+ > |VxLAN =3D hw |N/A |outer IP cksum is | > | | |hw (using | > | | |outer_lX_len), | > | | |outer UDP cksum is | > | | |sw (no hw support) | > +---------------------------+-------------------+-------------------+ >=20 >=20 > It could be really helpful to have a table like this for what you are imp= lementing, > because I feel there are too many options. Here is an example of what cou= ld be > done here (if options are not independent like before, it can be presente= d in a > different way in the first column). >=20 > Another thing: you don't describe what you want to be able to do: >=20 > 1/ packet type 1: compute L3 and/or L4 checksum using lX_len 2/ packet ty= pe 2: > compute inner L3 and/or L4 checksum using lX_len 3/ packet type 2: comput= e > outer L3 and/or L4 checksum using lX_len 4/ packet type 2: compute inner = L3 > and/or L4 checksum using lX_len > and outer L3 and/or L4 checksum using outer_lX_len These details have already covered in http://dpdk.org/ml/archives/dev/2014-= December/009213.html, if the patch set is applied, and we aslo have to update the some documents. > why not having the 2 following commands: > I have talked about why we need the current 3 commands in another mail loop= , let me explain it here again. =20 First. We still think we need some command to enable/disable tunneling su= pport in testpmd, that's why the command 1 is needed. 1. tx_checksum set tunnel (hw|sw|none) (port-id) command 2. tx_cksum set (outer-ip) (hw|sw) (port_id) 3. tx_cksum set (ip|l4) (hw|sw) (port_id) Secondly, in most of cases, user application use non-tunneling packet, so= he just care how to use 3, don't need to care 1 and 2, don't you think it= becomes simpler? =20 If we mix tunneling packet command and non-tunneling packet together, and t= he commands will become more complicated and not easy to understand. > tx_checksum set > (ip|udp|tcp|sctp|outer-ip|outer-udp|outer-tcp|outer-sctp) As far as I know, so far, there is no a type of tunneling packet with oute= r-tcp and outer-sctp. > select if we use hw or sw calculation for each header type >=20 > tx_checksum tunnel (inner|outer|both) >=20 > when a tunnel packet is received in csum only, control wether > we want to process inner, outer or both headers This command can't meet/match our previous discussions and current implemen= tation. In terms of 'inner' option, which can't meet the two following cas= es. B) User is aware that it is a tunneled packet and requests HW offload for i= pv4_hdr_in and tcp_hdr_in *only*. He doesn't care about outer IP checksum offload. In that case, for FVL he has 2 choices: 1. Treat that packet as a 'proper' tunnelled packet, and fill all the fi= elds: mb->l2_len =3D udp_hdr_len + vxlan_hdr_len + eth_hdr_in; mb->l3_len =3D ipv4_hdr_in; mb->outer_l2_len =3D eth_hdr_out; mb->outer_l3_len =3D ipv4_hdr_out; mb->ol_flags |=3D PKT_TX_UDP_TUNNEL | PKT_TX_IP_CKSUM | PKT_TX_TCP_CK= SUM; 2. As user doesn't care about outer IP hdr checksum, he can treat everyt= hing before ipv4_hdr_in as L2 header. So he knows, that it is a tunneled packet, but makes HW to treat it as o= rdinary (non-tunneled) packet: mb->l2_len =3D eth_hdr_out + ipv4_hdr_out + udp_hdr_out + vxlan_hdr + = ehtr_hdr_in; mb->l3_len =3D ipv4_hdr_in; mb->ol_flags |=3D PKT_TX_IP_CKSUM | PKT_TX_TCP_CKSUM; >=20 > The resulting table would be: >=20 > +------------------+-----------+-------------------------------------+ > |test-pmd \ packets|Packet type|Packet type 2 | > | |1 +-----------+-----------+-------------+ > | | |tun =3D inner|tun =3D outer|tun =3D both = | > | | | | | | > | | | | | | > +------------------+-----------+-----------+-----------+-------------+ > |whatever the flags|IP |inner IP |outer IP |inner and | > | |addresses |addresses |addresses |outer IP | > | |incremented|incremented|incremented|addresses | > | | | | |incremented | > +------------------+-----------+-----------+-----------+-------------+ > |IP =3D sw |IP cksum is|inner IP | |inner IP | > | |sw |cksum is sw| |cksum is sw | > | | | | | | > +------------------+-----------+-----------+-----------+-------------+ > |IP =3D hw |IP cksum is|inner IP | |inner IP | > | |hw (using |cksum is hw| |cksum is hw | > | |lX_len) |(using | |(using | > | | |lX_len) | |lX_len) | > +------------------+-----------+-----------+-----------+-------------+ > |UDP or TCP or SCTP|L4 cksum is|inner L4 | |inner L4 | > |=3D sw |sw |cksum is sw| |cksum is sw | > | | | | | | > +------------------+-----------+-----------+-----------+-------------+ > |UDP or TCP or SCTP|L4 cksum is|inner L4 | |inner L4 | > |=3D hw |hw (using |cksum is hw| |cksum is hw | > | |lX_len) |(using | |(using | > | | |lX_len) | |lX_len) | > +------------------+-----------+-----------+-----------+-------------+ > |outer IP =3D sw |N/A | |outer IP |outer IP | > | | | |cksum is sw|cksum is sw | > | | | | | | > +------------------+-----------+-----------+-----------+-------------+ > |outer IP =3D hw |N/A | |outer IP |outer IP | > | | | |cksum is hw|cksum is hw | > | | | |(using |(using | > | | | |lX_len) |outer_lX_len)| > | | | | | | > +------------------+-----------+-----------+-----------+-------------+ > |outer UDP or TCP |N/A | |outer L4 |outer L4 | > |or SCTP =3D sw | | |cksum is sw|cksum is sw | > | | | | | | > +------------------+-----------+-----------+-----------+-------------+ > |outer UDP or TCP |N/A | |outer L4 |outer L4 | > |or SCTP =3D hw | | |cksum is hw|cksum is hw | > | | | |(using |(using | > | | | |lX_len) |outer_lX_len,| > | | | | |knowing that | > | | | | |no hw support| > | | | | |it today) | > +------------------+-----------+-----------+-----------+-------------+ >=20 > > v2 change: > > redefine the 'none' behaviour for "tx_checksum set tunnel (hw|sw|= none) > (port-id)" command. > > v3 change: > > typo correction in cmdline help > > > > Jijiang Liu (3): > > add outer IP offload capability in librte_ether. > > add outer IP checksum capability in i40e PMD > > testpmd command lines of the tx_checksum and csum forwarding rework > > > > app/test-pmd/cmdline.c | 201 > +++++++++++++++++++++++++++++++++++-- > > app/test-pmd/csumonly.c | 35 ++++--- > > app/test-pmd/testpmd.h | 6 +- > > lib/librte_ether/rte_ethdev.h | 1 + > > lib/librte_pmd_i40e/i40e_ethdev.c | 3 +- > > 5 files changed, 218 insertions(+), 28 deletions(-) > > >=20 > One more comment, which is not critical. I think the commit log would be = more > readable if the lines are truncated at 72 columns, like described here: > http://tbaggery.com/2008/04/19/a-note-about-git-commit-messages.html >=20 > Regards, > Olivier