From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga01.intel.com (mga01.intel.com [192.55.52.88]) by dpdk.org (Postfix) with ESMTP id 1085F2E89 for ; Wed, 5 Nov 2014 06:54:49 +0100 (CET) Received: from fmsmga003.fm.intel.com ([10.253.24.29]) by fmsmga101.fm.intel.com with ESMTP; 04 Nov 2014 22:04:10 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="4.97,862,1389772800"; d="scan'208";a="411643408" Received: from pgsmsx101.gar.corp.intel.com ([10.221.44.78]) by FMSMGA003.fm.intel.com with ESMTP; 04 Nov 2014 21:55:39 -0800 Received: from kmsmsx153.gar.corp.intel.com (172.21.73.88) by PGSMSX101.gar.corp.intel.com (10.221.44.78) with Microsoft SMTP Server (TLS) id 14.3.195.1; Wed, 5 Nov 2014 14:02:48 +0800 Received: from shsmsx152.ccr.corp.intel.com (10.239.6.52) by KMSMSX153.gar.corp.intel.com (172.21.73.88) with Microsoft SMTP Server (TLS) id 14.3.195.1; Wed, 5 Nov 2014 14:02:47 +0800 Received: from shsmsx101.ccr.corp.intel.com ([169.254.1.202]) by SHSMSX152.ccr.corp.intel.com ([169.254.6.13]) with mapi id 14.03.0195.001; Wed, 5 Nov 2014 14:02:46 +0800 From: "Liu, Jijiang" To: Olivier MATZ Thread-Topic: [dpdk-dev] [PATCH v8 10/10] app/testpmd:test VxLAN Tx checksum offload Thread-Index: AQHP8Yux0VUY397yik2xjVM3DjesSpxPpsiAgAGaY5A= Date: Wed, 5 Nov 2014 06:02:46 +0000 Message-ID: <1ED644BD7E0A5F4091CF203DAFB8E4CC01D8510E@SHSMSX101.ccr.corp.intel.com> References: <1414376006-31402-1-git-send-email-jijiang.liu@intel.com> <1414376006-31402-11-git-send-email-jijiang.liu@intel.com> <54588BF7.309@6wind.com> In-Reply-To: <54588BF7.309@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 v8 10/10] app/testpmd:test VxLAN Tx checksum offload 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: Wed, 05 Nov 2014 05:54:52 -0000 Hi Olivier, > -----Original Message----- > From: Olivier MATZ [mailto:olivier.matz@6wind.com] > Sent: Tuesday, November 4, 2014 4:19 PM > To: Liu, Jijiang > Cc: dev@dpdk.org > Subject: Re: [dpdk-dev] [PATCH v8 10/10] app/testpmd:test VxLAN Tx checks= um > offload >=20 > Hello Jijiang, >=20 > On 10/27/2014 03:13 AM, Jijiang Liu wrote: > > Add test cases in testpmd to test VxLAN Tx Checksum offload, which incl= ude > > - IPv4 and IPv6 packet > > - outer L3, inner L3 and L4 checksum offload for Tx side. > > > > Signed-off-by: Jijiang Liu >=20 > I'm trying to port the test of TSO in csum_only.c which was originally pa= rt of this > patch [1]. >=20 > Meanwhile, the source was modified by the patch provided by the email I'm > replying to (also available at [2]), and I would like to understand what = is the > purpose of it. >=20 > First, the code checks if the mbuf has the flag PKT_RX_TUNNEL_IPV4_HDR. > What is the meaning of this flag? It was added by [3], but there is no de= scription > in comments or in the commit log explaining in which case this flag is se= t by the > driver. The name supposes that this flag is set when the received packet = is an IPv4 > tunnel, but the commit log talks about vxlan. The flag PKT_RX_TUNNEL_IPV4_HDR can be used for all tunneling packet types = with outer IPV4 header. For example: IPv4 --> GRE/Teredo/VXLAN --> MAC --> IPv4: MAC, IPV4, GRENAT, MAC, IPV4, SCTP, PAY4=20 MAC, IPV4, GRENAT, MAC, IPV6, UDP, PAY4 MAC, IPV4, GRENAT, MAC, IPV6, UDP, PAY4 These tunneling packet formats have a common point that is outer IPv4 heade= r here. Only VXLAN tunneling packet is supported in DPDK for i40e now, so the comm= it log talks about VXLAN . =20 > Then, if this flag was present, the code assumes it's a vxlan packet. > If one of inner checksum is specified by the user in cmdline, the flag > PKT_TX_VXLAN_CKSUM is added to the mbuf. This flag definition was added b= y > [4] (at the wrong place, I'll fix it in my patchset).=20 > What is the meaning of this flag? > Is it enough to checksum outer L3, inner L3, and inner L4 as specified in= commit > log? If yes, why are the other flags PKT_TX_IPV4_CSUM, PKT_TX_UDP_CKSUM, > (...) added in the mbuf later? > In my comprehension, these flags are needed in addition to > PKT_TX_VXLAN_CKSUM to do the checksum of the inner headers. Yes, these flags(PKT_TX_IPV4_CSUM, PKT_TX_UDP_CKSUM) are needed by HW offl= oad of non-tunneling and tunneling packet. > In general, I need to understand how to use all the new offloading stuff.= In [5], > new fields were added in the mbuf structure (inner_l3_len and inner_l2_le= n). I'm > not sure I understand which fields have to be filled. Below is my underst= anding, > can you please check that it is correct? >=20 > A- To send a Ether/IP/TCP packet and process IP and TCP TX > checksum in the NIC: >=20 > - set l2_len =3D 14, l3_len =3D 20, > ol_flags =3D PKT_TX_IP_CKSUM | PKT_TX_TCP_CKSUM, > write all checksums to 0 in the packet IP checksum is 0, but tcp checksum is not 0. tcp_hdr->cksum =3D get_ipv4_psd_sum(ipv4_hdr); > B- To send a Ether/IP/UDP/vxlan/Ether/IP/TCP packet and > process inner IP and inner TCP TX checksum in the NIC: >=20 > - set l2_len =3D 14+20+8+8+14, l3_len =3D 20, No, l2_len is outer L2 length, its value also is 14.=20 > ol_flags =3D PKT_TX_IP_CKSUM | PKT_TX_TCP_CKSUM, > write all checksums to 0 in the packet IP checksum is 0, but tcp checksum is not 0. tcp_hdr->cksum =3D get_ipv4_psd_sum(ipv4_hdr); > C- To send a Ether/IP/UDP/vxlan/Ether/IP/TCP packet and > process outer IP and UDP, plus inner IP and inner TCP TX > checksum in the NIC: >=20 > - set l2_len =3D 14, l3_len =3D 20, inner_l2_len =3D 14, > inner_l3_len =3D 20, Yes > ol_flags =3D PKT_TX_IP_CKSUM | PKT_TX_TCP_CKSUM | > PKT_TX_VXLAN_CKSUM, Yes > write all checksums to 0 in the packet Outer and inner IP checksum is 0, but tcp checksum is not 0. tcp_hdr->cksum =3D get_ipv4_psd_sum(ipv4_hdr); > D- To send a Ether/IP/UDP/vxlan/Ether/IP/TCP packet and > process outer IP and UDP TX checksum in the NIC: >=20 > - set l2_len =3D 14, l3_len =3D 20, > ol_flags =3D PKT_TX_IP_CKSUM | PKT_TX_UDP_CKSUM, Yes > write all checksums to 0 in the packet Outer IP checksum is 0, but tcp checksum is not 0. tcp_hdr->cksum =3D get_ipv4_psd_sum(ipv4_hdr); > First, can you confirm this? I think it is very important to document it,= as this is a > public API that can be used by DPDK users. These should be included in documents. > To validate my modifications, I will try to reproduce the test plan descr= ibed in [6]. > The test report contains useful information but I'm not sure to understan= d the > following: >=20 > Enable outer IP,UDP,TCP,SCTP and inner IP,UDP checksum offload > when inner L4 protocal is UDP. > testpmd>tx_checksum set 0 0xf3 "tx_checksum set 0 0xf3" should be "tx_checksum set 0 0x33", but the tester= use 0xFX (that is to say, enable all inner TX flag)in order to write test = script easily. > Enable outer IP,UDP,TCP,SCTP and inner IP,TCP,SCTP checksum offload > when inner L4 protocal is TCP or SCTP. > testpmd>tx_checksum set 0 0xfd >=20 > Can you give details about the signification of the bits used in the tx_c= hecksum > command? For instance, there is only one flag to enable tx checksum in th= e mbuf, > so I don't understand how it's possible to do 0x44 =3D (inner tcp + outer= tcp). These bits meanings/help information were provided in the patch http://dpdk.org/ml/archives/dev/2014-October/007156.html=20 > Today, there are hard-coded values for flags: 4 bits (0-3) for legacy che= cksums, 4 > bits for inner checksums (4-7), and one bit (bit 11?!) for vlan header. T= hese bits > are checked without defines at several places in the code. Moreover, ther= e are > some places in code where the testpmd port flags and the mbuf flags are m= ixed > up. Example in macswap.c : mb->ol_flags =3D txp->tx_ol_flags; I thought this is an issue. > I suggest to remove all hardcoded values except in > cmd_tx_cksum_set_parsed() and replace them by the flags definitions from > rte_mbuf.h. As a result, the new possible arguments of > cmd_tx_cksum_set_parsed() will map the mbuf flags: > - a flag to enable ip tx cksum > - a flag to enable udp tx cksum > - a flag to enable tcp tx cksum > - a flag to enable sctp tx cksum > - a flag to enable vxlan tx cksum > If vxlan tx cksum is set, therefore the other considered checksum will co= ncern the > inner headers. Do you think it matches your needs? As to HW TX checksum offload, do you have special requirement for implement= ing TSO? I suggest to keep on using bit to indicate different protocol flag. this ap= proach has good flexibility and extensibility. For example, a VXLAN packet format: MAC/IP/UDP/VXLAN/inner MAC/inner IP/inn= er TCP/PAY4. If the ip tx cksum and vxlan tx cksum are set, but tcp tx cksum is not set= , and HW offload of inner TCP TX checksum won't work. In addition, sometimes we want to get the following performance data using = testpmd 1. only enable outer IP TX checksum =20 2. only enable inner IP TX checksum 3. only enable inner TCP/UDP/SCTP checksum 4. only enable inner IP TX checksum and inner TCP/UDP/SCTP checksum. Now we have provided inner BIT masks, we can use these combinations to test= it easily. Actually, we just need to know that if the PKT_TX_IPV4_CSUM, PKT_TX_TCP_CK= SUM, PKT_TX_SCTP_CKSUM, PKT_TX_UDP_CKSUM and PKT_TX_VXLAN_CKSUM are set in = ol_flags in the i40e driver. Could you please look at the function i40e_txd_enable_checksum() . > I think that the csum_only forward engine is now a bit complicated. > As it's an example that shows how to configure the tx checksum, I think i= ts > behavior should be described somewhere, maybe in a comment in the file. I agree. > By the way, I was looking at the mbuf structure, and I see that a new pac= ket_type > field was added. There is no explanation about what this field should con= tain and > how we shall use it (in commit log or in comments). Can you please explai= n it? + /** + * The packet type, which is used to indicate ordinary packet and also + * tunneled packet format, i.e. each number is represented a type of + * packet. + */ + uint16_t packet_type Regarding adding a packet_type in mbuf, we ever had a lot of discussions as= follows: http://dpdk.org/ml/archives/dev/2014-October/007027.html http://dpdk.org/ml/archives/dev/2014-September/005240.html http://dpdk.org/ml/archives/dev/2014-September/005241.html http://dpdk.org/ml/archives/dev/2014-September/005274.html > To conclude, I'd like to propose a merge of the TSO series but I'm curren= tly > blocked by these questions. Please, could you help me to progress on this= ? Let me know if you have any questions?=20 > Regards, > Olivier >=20 >=20 > [1] http://dpdk.org/ml/archives/dev/2014-May/002549.html > [2] http://dpdk.org/ml/archives/dev/2014-October/007156.html > [3] > http://dpdk.org/browse/dpdk/commit/lib/librte_mbuf/rte_mbuf.h?id=3D15dbb6= 3ef > 9e9f108e7dcd837b88234f27a1ec258 > [4] > http://dpdk.org/browse/dpdk/commit/lib/librte_mbuf/rte_mbuf.h?id=3D77b830= 17 > 33c3cd946648ca4a1375e3db0a952216 > [5] > http://dpdk.org/browse/dpdk/commit/lib/librte_mbuf/rte_mbuf.h?id=3D73b7d5= 9cf > 4f6faf5ccd83ce706473e75c6fb8c3b > [6] http://dpdk.org/ml/archives/dev/2014-October/007157.html