Hi Olivier, Thanks for your comments.
> -----Original Message----- > From: Olivier MATZ [mailto:olivier.matz at 6wind.com] > Sent: Thursday, December 11, 2014 6:18 PM > To: Liu, Jijiang; dev at dpdk.org > Subject: Re: [dpdk-dev] [PATCH v3 0/3] enhance TX checksum command and > csum forwarding engine > > 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. 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 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 > > 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 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 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. > > 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 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. First. We still think we need some command to enable/disable tunneling support 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? If we mix tunneling packet command and non-tunneling packet together, and the 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) <portid> As far as I know, so far, there is no a type of tunneling packet with outer-tcp and 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 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; > > 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
