[dpdk-dev] [PATCH v3 0/3] enhance TX checksum command and csum forwarding engine
Liu, Jijiang
jijiang.liu at intel.com
Fri Dec 12 04:48:26 CET 2014
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
More information about the dev
mailing list