From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga03.intel.com (mga03.intel.com [134.134.136.65]) by dpdk.org (Postfix) with ESMTP id 440DC1518 for ; Fri, 28 Nov 2014 09:54:41 +0100 (CET) Received: from orsmga002.jf.intel.com ([10.7.209.21]) by orsmga103.jf.intel.com with ESMTP; 28 Nov 2014 00:51:26 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.07,475,1413270000"; d="scan'208";a="644916798" Received: from kmsmsx152.gar.corp.intel.com ([172.21.73.87]) by orsmga002.jf.intel.com with ESMTP; 28 Nov 2014 00:54:34 -0800 Received: from shsmsx102.ccr.corp.intel.com (10.239.4.154) by KMSMSX152.gar.corp.intel.com (172.21.73.87) with Microsoft SMTP Server (TLS) id 14.3.195.1; Fri, 28 Nov 2014 16:54:11 +0800 Received: from shsmsx101.ccr.corp.intel.com ([169.254.1.110]) by shsmsx102.ccr.corp.intel.com ([169.254.2.216]) with mapi id 14.03.0195.001; Fri, 28 Nov 2014 16:54:09 +0800 From: "Liu, Jijiang" To: Olivier MATZ , "dev@dpdk.org" , "Ananyev, Konstantin" Thread-Topic: [PATCH v4 08/13] testpmd: rework csum forward engine Thread-Index: AQHQCYp0xXVfhiiHtkGlUBA/d8RRSpxyzroAgADcEYCAAB9IgIAB7+oA Date: Fri, 28 Nov 2014 08:54:09 +0000 Message-ID: <1ED644BD7E0A5F4091CF203DAFB8E4CC01D9F23E@SHSMSX101.ccr.corp.intel.com> References: <1416524335-22753-1-git-send-email-olivier.matz@6wind.com> <1417014295-29064-1-git-send-email-olivier.matz@6wind.com> <1417014295-29064-9-git-send-email-olivier.matz@6wind.com> <2601191342CEEE43887BDE71AB977258213BAA86@IRSMSX105.ger.corp.intel.com> <5476EA87.4040807@6wind.com> <2601191342CEEE43887BDE71AB977258213BACAB@IRSMSX105.ger.corp.intel.com> In-Reply-To: <2601191342CEEE43887BDE71AB977258213BACAB@IRSMSX105.ger.corp.intel.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: "jigsaw@gmail.com" Subject: Re: [dpdk-dev] [PATCH v4 08/13] testpmd: rework csum forward 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, 28 Nov 2014 08:54:42 -0000 Hi Olivier, > -----Original Message----- > From: Ananyev, Konstantin > Sent: Thursday, November 27, 2014 7:02 PM > To: Olivier MATZ; dev@dpdk.org > Cc: Walukiewicz, Miroslaw; Liu, Jijiang; Liu, Yong; jigsaw@gmail.com; Ric= hardson, > Bruce > Subject: RE: [PATCH v4 08/13] testpmd: rework csum forward engine >=20 > Hi Oliver, >=20 > > -----Original Message----- > > From: Olivier MATZ [mailto:olivier.matz@6wind.com] > > Sent: Thursday, November 27, 2014 9:11 AM > > To: Ananyev, Konstantin; dev@dpdk.org > > Cc: Walukiewicz, Miroslaw; Liu, Jijiang; Liu, Yong; jigsaw@gmail.com; > > Richardson, Bruce > > Subject: Re: [PATCH v4 08/13] testpmd: rework csum forward engine > > > > Hi Konstantin, > > > > On 11/26/2014 09:02 PM, Ananyev, Konstantin wrote: > > >> +/* if possible, calculate the checksum of a packet in hw or sw, > > >> + * depending on the testpmd command line configuration */ static > > >> +uint64_t process_inner_cksums(void *l3_hdr, uint16_t ethertype, > > >> +uint16_t l3_len, > > >> + uint8_t l4_proto, uint16_t testpmd_ol_flags) { > > >> + struct ipv4_hdr *ipv4_hdr =3D l3_hdr; > > >> + struct udp_hdr *udp_hdr; > > >> + struct tcp_hdr *tcp_hdr; > > >> + struct sctp_hdr *sctp_hdr; > > >> + uint64_t ol_flags =3D 0; > > >> + > > >> + if (ethertype =3D=3D _htons(ETHER_TYPE_IPv4)) { > > >> + ipv4_hdr =3D l3_hdr; > > >> + ipv4_hdr->hdr_checksum =3D 0; > > >> + > > >> + if (testpmd_ol_flags & TESTPMD_TX_OFFLOAD_IP_CKSUM) > > >> + ol_flags |=3D PKT_TX_IP_CKSUM; > > >> + else > > >> + ipv4_hdr->hdr_checksum =3D get_ipv4_cksum(ipv4_hdr); > > >> + > > >> + ol_flags |=3D PKT_TX_IPV4; > > > > > > Flags PKT_TX_IP_CKSUM, PKT_TX_IPV4, PKT_TX_IPV6 are all mutually > exclusive. > > > So it should be, I think: > > > > > > if (testpmd_ol_flags & TESTPMD_TX_OFFLOAD_IP_CKSUM) { > > > ol_flags |=3D PKT_TX_IP_CKSUM; > > > } else { > > > ipv4_hdr->hdr_checksum =3D get_ipv4_cksum(ipv4_hdr); > > > ol_flags |=3D PKT_TX_IPV4; } > > > > It seems normal that PKT_TX_IPV4 are PKT_TX_IPV6 exclusive, but do you > > mean that PKT_TX_IP_CKSUM and PKT_TX_IPV4 are exclusive too? It looks > > strange to me. > > > > My understanding of the meaning of the flags is: > > > > - PKT_TX_IP_CKSUM: tell the NIC to compute IP cksum > My initial thought: > It tells the NIC that it is an IPV4 packet for which it has to compute ch= ecksum. >=20 > > > > - PKT_TX_IPV4: tell the NIC it's an IPv4 packet. Required for L4 > > checksum offload or TSO. >=20 > It tells the NIC that it is an IPV4 packet for which it shouldn't compute= checksum. >=20 > > > > - PKT_TX_IPV6: tell the NIC it's an IPv6 packet. Required for L4 > > checksum offload or TSO. >=20 > Yes. >=20 > > > > If it's a i40e driver requirement, don't you think it's better to > > change the driver? There should be two logics in csum engine, which is that either HW compute= s TX checksum (using PKT_TX_IP_CKSUM) or SW compute TX checksum(use PKT_TX_= IPV4(or another flag) to tell driver no IP checksum offload requirement ), I think we shouldn't use L3 flag to tell driver what HW need do for L4, L3= and L4 flag should be separated . > Yes, it could be done in both ways: > either all 3 flags are mutually exclusive or first two and third one are = mutually > exclusive. >=20 > Current i40e PMD seems to work correctly with the second way too. >=20 > Though the second way implies a specific order for PMD to check flags. > Something like: > if (ol_flags & PKT_TX_IP_CKSUM) {..} else if (ol_flags & PKT_TX_IPV4) {.= ..} else ... > would work correctly. I40e driver use this way. > But: > if (ol_flags & PKT_TX_IPV4) {...} else if (ol_flags & PKT_TX_IP_CKSUM) {.= .} else > wouldn't. > > > > >> +/* Calculate the checksum of outer header (only vxlan is > > >> +supported, > > >> + * meaning IP + UDP). The caller already checked that it's a vxlan > > >> + * packet */ > > >> +static uint64_t > > >> +process_outer_cksums(void *outer_l3_hdr, uint16_t outer_ethertype, > > >> + uint16_t outer_l3_len, uint16_t testpmd_ol_flags) { > > >> + struct ipv4_hdr *ipv4_hdr =3D outer_l3_hdr; > > >> + struct ipv6_hdr *ipv6_hdr =3D outer_l3_hdr; > > >> + struct udp_hdr *udp_hdr; > > >> + uint64_t ol_flags =3D 0; > > >> + > > >> + if (testpmd_ol_flags & TESTPMD_TX_OFFLOAD_VXLAN_CKSUM) > > >> + ol_flags |=3D PKT_TX_VXLAN_CKSUM; > > >> + > > >> + if (outer_ethertype =3D=3D _htons(ETHER_TYPE_IPv4)) { > > >> + ipv4_hdr->hdr_checksum =3D 0; > > >> + > > >> + if ((testpmd_ol_flags & TESTPMD_TX_OFFLOAD_VXLAN_CKSUM) > =3D=3D 0) > > >> + ipv4_hdr->hdr_checksum =3D get_ipv4_cksum(ipv4_hdr); > > >> + } > > >> + > > >> + udp_hdr =3D (struct udp_hdr *)((char *)outer_l3_hdr + outer_l3_len= ); > > >> + /* do not recalculate udp cksum if it was 0 */ > > >> + if (udp_hdr->dgram_cksum !=3D 0) { > > >> + udp_hdr->dgram_cksum =3D 0; > > >> + if ((testpmd_ol_flags & TESTPMD_TX_OFFLOAD_VXLAN_CKSUM) > =3D=3D 0) { > > > > > > In fact, FVL is not able to do HW caclualtion for outer L4, only oute= r IPV4 > cksum is supported. > > > So no need for: > > > if (testpmd_ol_flags & TESTPMD_TX_OFFLOAD_UDP_CKSUM) { above. > > > And yes, if user will select to calculate inner checksums by HW - out= er UDP > checksum might be invalid anyway. > > > > I may have misunderstood how vxlan works, so I agree this code is > > probably wrong. However, I don't find the line you are quoting in the > > function above. >=20 > Function: process_outer_cksums(), line 273: >=20 > if (udp_hdr->dgram_cksum !=3D 0) { > udp_hdr->dgram_cksum =3D 0; > if ((testpmd_ol_flags & TESTPMD_TX_OFFLOAD_VXLAN_CKSUM) = =3D=3D 0) > { /* <-- THAT ONE. */ > if (outer_ethertype =3D=3D _htons(ETHER_TYPE_IPv4= )) >=20 > I think it is no need for it there. >=20 > > > > I'll check how Jijiang fixed the issue. > > > > Regards, > > Olivier