From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail.droids-corp.org (zoll.droids-corp.org [94.23.50.67]) by dpdk.org (Postfix) with ESMTP id 062C580B3 for ; Fri, 12 Dec 2014 17:33:30 +0100 (CET) Received: from was59-1-82-226-113-214.fbx.proxad.net ([82.226.113.214] helo=[192.168.0.10]) by mail.droids-corp.org with esmtpsa (TLS1.0:DHE_RSA_AES_128_CBC_SHA1:128) (Exim 4.80) (envelope-from ) id 1XzTCo-0007pe-LP; Fri, 12 Dec 2014 17:36:46 +0100 Message-ID: <548B18C9.3020408@6wind.com> Date: Fri, 12 Dec 2014 17:33:13 +0100 From: Olivier MATZ User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:24.0) Gecko/20100101 Icedove/24.5.0 MIME-Version: 1.0 To: "Liu, Jijiang" References: <1418173403-30202-1-git-send-email-jijiang.liu@intel.com> <54896F4A.4070601@6wind.com> <1ED644BD7E0A5F4091CF203DAFB8E4CC01DA1B70@SHSMSX101.ccr.corp.intel.com> In-Reply-To: <1ED644BD7E0A5F4091CF203DAFB8E4CC01DA1B70@SHSMSX101.ccr.corp.intel.com> Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit 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 16:33:30 -0000 Hello, On 12/12/2014 04:48 AM, Liu, Jijiang wrote: > The 'hw/sw' option is used to set/clear the flag of enabling TX tunneling packet checksum hardware offload in testpmd application. This is not clear at all. In your command, there is (hw|sw|none). Are you talking about inner or outer? Is this command useful for any kind of packet? How does it combine with "tx_checksum set outer-ip (hw|sw)"? >> 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: I read again your cover letter. You enumerate *one* scenario. An enumeration starts with 2 items. Then you mix 1> and 1) numbers, which does not make things readable. >> Another thing: you don't describe what you want to be able to do: >> >> 1/ packet type 1: compute L3 and/or L4 checksum using lX_len 2/ packet type 2: >> compute inner L3 and/or L4 checksum using lX_len 3/ packet type 2: compute >> 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. First, this link was not referenced in the cover letter or patchset. I think it would help to first try to explain what problem is fixed, what is the need, and why. I think in this case it should not require lines and it could be done in a simple way. Indeed, yesterday I spent more than an hour to review your patches and read your descriptions. After that, I still don't understand how the commands impact the behavior of the csumonly application. The possible reasons are: 1) I am too dumb to understand. In this case, it would be better for you and the community to find another reviewer. 2) Your descriptions are not clear. In this case, you need to think about how to reword them so they can be understood, or even maybe think about rework your commands if they cannot be explained with simple words. Note that 1) and 2) are not exclusive ;) >> 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. The community does not have this private thread. And, that's right, I remember this thread: in it, I asked 3 times some precisions about the commands without any clear answer. > First. We still think we need some command to enable/disable tunneling support in testpmd, that's why the command 1 is needed. What does enable/disable tunneling support mean? > 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? > If we mix tunneling packet command and non-tunneling packet together, and the commands will become more complicated and not easy to understand. Really no, it is not simpler. But if you are able to explain it in few words what is done by csumonly, maybe I can change my mind. >> 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 outer-tcp and outer-sctp. For TCP, there is STT, which is used in storage. For SCTP, it could probably be removed. >> select if we use hw or sw calculation for each header type >> >> tx_checksum tunnel (inner|outer|both) >> >> 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 implementation. In terms of 'inner' option, which can't meet the two following cases. > > B) User is aware that it is a tunneled packet and requests HW offload for ipv4_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 fields: > mb->l2_len = udp_hdr_len + vxlan_hdr_len + eth_hdr_in; > mb->l3_len = ipv4_hdr_in; > mb->outer_l2_len = eth_hdr_out; > mb->outer_l3_len = ipv4_hdr_out; > mb->ol_flags |= PKT_TX_UDP_TUNNEL | PKT_TX_IP_CKSUM | PKT_TX_TCP_CKSUM; > > 2. As user doesn't care about outer IP hdr checksum, he can treat everything before ipv4_hdr_in as L2 header. > So he knows, that it is a tunneled packet, but makes HW to treat it as ordinary (non-tunneled) packet: > mb->l2_len = eth_hdr_out + ipv4_hdr_out + udp_hdr_out + vxlan_hdr + ehtr_hdr_in; > mb->l3_len = ipv4_hdr_in; > mb->ol_flags |= PKT_TX_IP_CKSUM | PKT_TX_TCP_CKSUM; Indeed, there are 2 choices for fortville. I could argue that you also have 2 choices to offload the IP checksum of Ether/IP/UDP/xxx: - use lX_len - use outer_lX_len You have also the choice to use lX_len or outer_lX_len if you want to offload the outer IP checksum of Ether/IP/UDP/vxlan/Ether/IP/UDP/xxx That's exactly why I asked to first describe what problem you want to solve in your patchset. You can extend the list I've send in my first answer. Maybe your solution supports all these cases, but as a user, I have no idea how to configure it even after reading your desriptions during a long time, so the result is the same than not supporting. The question is: are all these casez useful? To me, it's equally important that a user can use the application and understand what it does. If they are required I'm sure we can find a good user API that is able to test all these cases. Regards, Olivier