From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-wi0-f170.google.com (mail-wi0-f170.google.com [209.85.212.170]) by dpdk.org (Postfix) with ESMTP id F2E72804B for ; Thu, 11 Dec 2014 11:17:48 +0100 (CET) Received: by mail-wi0-f170.google.com with SMTP id bs8so943394wib.3 for ; Thu, 11 Dec 2014 02:17:48 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:message-id:date:from:user-agent:mime-version:to :subject:references:in-reply-to:content-type :content-transfer-encoding; bh=SnS5O7kXFht2NYqk8gIBbfx9zPHc2FskvZe8RErmEU4=; b=mM/dAWDEtiyoMAozUdltFV4IuxWEwpH4z3yG1i3VI2ywXtkxGET5dAU8Qlz/gnKbgQ QGfggwxDOAMfPAWEPQN6yGK/yvCX+YF28SvIrqrPgyiugofXnW5WyvdRaMfn9y3ITiPq dmamqxCUH0qi/DClGFFTvOVYdH0R5bDtCW47XBFVUgijxq+EHnp/UiXaNldyhmdqWp0E 8U91ChaDPAA1A/4P6Xcq0/jnawoUwrxXGekSzgpFDpXx2CoV2WiQbPOCCW0HnmdjDb9Q I9lLZsQp7+jgGxTyrjNdE4CE76Kn92jDDq+xwFQckMYu9noTgACmA9q0YWp9svKcT56n NG9A== X-Gm-Message-State: ALoCoQm0U0CWxndUGvvrJ4eFfR1vp6qUKuWC/4Uvpi4ds1JOwzOgBVrOtaaKEjk2hH/wSQJrIb9u X-Received: by 10.180.76.201 with SMTP id m9mr14299840wiw.52.1418293068337; Thu, 11 Dec 2014 02:17:48 -0800 (PST) Received: from [10.16.0.195] (guy78-3-82-239-227-177.fbx.proxad.net. [82.239.227.177]) by mx.google.com with ESMTPSA id ud1sm1100331wjc.7.2014.12.11.02.17.47 for (version=TLSv1 cipher=ECDHE-RSA-RC4-SHA bits=128/128); Thu, 11 Dec 2014 02:17:47 -0800 (PST) Message-ID: <54896F4A.4070601@6wind.com> Date: Thu, 11 Dec 2014 11:17:46 +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: Jijiang Liu , dev@dpdk.org References: <1418173403-30202-1-git-send-email-jijiang.liu@intel.com> In-Reply-To: <1418173403-30202-1-git-send-email-jijiang.liu@intel.com> Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit 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: Thu, 11 Dec 2014 10:17:49 -0000 Hi Jijiang, Sorry for the late review, I was very busy these last days. Please find my comments below. 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 NIC that the packetg is a tunneing packet and application want hardware TX checksum offload for outer layer, or inner layer, or both. packetg -> packet tunneing -> tunneling I don't understand the description: this flag cannot be set to tell the NIC that it's a tunnel packet because it's a configuration flag. Whatever the value of this configuration option, the packets can be either tunnel or non-tunnel packets. The real question is, what is the behavior for each packet type for each value for this option. > The 'none' option means that user treat tunneling packet as ordinary packet 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/tcp_hdr_in. one of several scenarios: > > 1) User requests HW offload for ipv4_hdr_out checksum, and doesn't care is it a tunnelled packet or not. So he sets: tunnelled -> tunneled > > 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 (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 the 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. 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 Let's first sumarize what was the behavior before this patch. This is the description in csumonly code. 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 packets on the output port. (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 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 outer IP (if packet is recognized as a vxlan packet). From this description, it is easy to deduct this table: Packet type 1: Ether / (vlan) / IP|IP6 / UDP|TCP|SCTP . Packet type 2 Ether / (vlan) / outer IP|IP6 / outer UDP / VxLAN / Ether / IP|IP6 / UDP|TCP|SCTP +---------------------------+-------------------+-------------------+ |test-pmd config \ packets |Packet type 1 |Packet type 2 | +---------------------------+-------------------+-------------------+ |whatever the flags |IP addresses |inner and outer IP | | |incremented |addresses | | | |incremented | +---------------------------+-------------------+-------------------+ |IP = sw |IP cksum is sw |inner IP cksum is | | | |sw | +---------------------------+-------------------+-------------------+ |IP = hw |IP cksum is hw |inner IP cksum is | | |(using lX_len) |hw (using lX_len) | +---------------------------+-------------------+-------------------+ |UDP or TCP or SCTP = sw |L4 cksum is sw |inner L4 cksum is | | | |sw | +---------------------------+-------------------+-------------------+ |UDP or TCP or SCTP = hw |L4 cksum is hw |inner L4 cksum is | | |(using lX_len) |hw (using lX_len) | +---------------------------+-------------------+-------------------+ |VxLAN = sw |N/A |outer IP cksum is | | | |sw, outer UDP cksum| | | |is sw | +---------------------------+-------------------+-------------------+ |VxLAN = hw |N/A |outer IP cksum is | | | |hw (using | | | |outer_lX_len), | | | |outer UDP cksum is | | | |sw (no hw support) | +---------------------------+-------------------+-------------------+ It could be really helpful to have a table like this for what you are implementing, because I feel there are too many options. Here is an example of what could be done here (if options are not independent like before, it can be presented in a different way in the first column). 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 why not having the 2 following commands: tx_checksum set (ip|udp|tcp|sctp|outer-ip|outer-udp|outer-tcp|outer-sctp) 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 The resulting table would be: +------------------+-----------+-------------------------------------+ |test-pmd \ packets|Packet type|Packet type 2 | | |1 +-----------+-----------+-------------+ | | |tun = inner|tun = outer|tun = both | | | | | | | | | | | | | +------------------+-----------+-----------+-----------+-------------+ |whatever the flags|IP |inner IP |outer IP |inner and | | |addresses |addresses |addresses |outer IP | | |incremented|incremented|incremented|addresses | | | | | |incremented | +------------------+-----------+-----------+-----------+-------------+ |IP = sw |IP cksum is|inner IP | |inner IP | | |sw |cksum is sw| |cksum is sw | | | | | | | +------------------+-----------+-----------+-----------+-------------+ |IP = 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 | |= sw |sw |cksum is sw| |cksum is sw | | | | | | | +------------------+-----------+-----------+-----------+-------------+ |UDP or TCP or SCTP|L4 cksum is|inner L4 | |inner L4 | |= hw |hw (using |cksum is hw| |cksum is hw | | |lX_len) |(using | |(using | | | |lX_len) | |lX_len) | +------------------+-----------+-----------+-----------+-------------+ |outer IP = sw |N/A | |outer IP |outer IP | | | | |cksum is sw|cksum is sw | | | | | | | +------------------+-----------+-----------+-----------+-------------+ |outer IP = 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 = sw | | |cksum is sw|cksum is sw | | | | | | | +------------------+-----------+-----------+-----------+-------------+ |outer UDP or TCP |N/A | |outer L4 |outer L4 | |or SCTP = hw | | |cksum is hw|cksum is hw | | | | |(using |(using | | | | |lX_len) |outer_lX_len,| | | | | |knowing that | | | | | |no hw support| | | | | |it today) | +------------------+-----------+-----------+-----------+-------------+ > 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(-) > 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 Regards, Olivier