[dpdk-dev] [PATCH v8 10/10] app/testpmd:test VxLAN Tx checksum offload

Liu, Jijiang jijiang.liu at intel.com
Wed Nov 5 07:02:46 CET 2014


Hi Olivier,


> -----Original Message-----
> From: Olivier MATZ [mailto:olivier.matz at 6wind.com]
> Sent: Tuesday, November 4, 2014 4:19 PM
> To: Liu, Jijiang
> Cc: dev at dpdk.org
> Subject: Re: [dpdk-dev] [PATCH v8 10/10] app/testpmd:test VxLAN Tx checksum
> offload
> 
> Hello Jijiang,
> 
> On 10/27/2014 03:13 AM, Jijiang Liu wrote:
> > Add test cases in testpmd to test VxLAN Tx Checksum offload, which include
> >   - IPv4 and IPv6 packet
> >   - outer L3, inner L3 and L4 checksum offload for Tx side.
> >
> > Signed-off-by: Jijiang Liu <jijiang.liu at intel.com>
> 
> I'm trying to port the test of TSO in csum_only.c which was originally part of this
> patch [1].
> 
> Meanwhile, the source was modified by the patch provided by the email I'm
> replying to (also available at [2]), and I would like to understand what is the
> purpose of it.
> 
> First, the code checks if the mbuf has the flag PKT_RX_TUNNEL_IPV4_HDR.
> What is the meaning of this flag? It was added by [3], but there is no description
> in comments or in the commit log explaining in which case this flag is set by the
> driver. The name supposes that this flag is set when the received packet is an IPv4
> tunnel, but the commit log talks about vxlan.

The flag PKT_RX_TUNNEL_IPV4_HDR can be used for all tunneling packet types with outer IPV4 header.
For example:
IPv4 --> GRE/Teredo/VXLAN --> MAC --> IPv4:
MAC, IPV4, GRENAT, MAC, IPV4, SCTP, PAY4 
MAC, IPV4, GRENAT, MAC, IPV6, UDP, PAY4
MAC, IPV4, GRENAT, MAC, IPV6, UDP, PAY4
These tunneling packet formats have a common point that is outer IPv4 header here.

Only VXLAN tunneling packet is supported in DPDK for i40e now, so  the commit log talks about VXLAN .
 

> Then, if this flag was present, the code assumes it's a vxlan packet.
> If one of inner checksum is specified by the user in cmdline, the flag
> PKT_TX_VXLAN_CKSUM is added to the mbuf. This flag definition was added by
> [4] (at the wrong place, I'll fix it in my patchset). 

> What is the meaning of this flag?
> Is it enough to checksum outer L3, inner L3, and inner L4 as specified in commit
> log? If yes, why are the other flags PKT_TX_IPV4_CSUM, PKT_TX_UDP_CKSUM,
> (...) added in the mbuf later?
> In my comprehension, these flags are needed in addition to
> PKT_TX_VXLAN_CKSUM to do the checksum of the inner headers.

Yes, these flags(PKT_TX_IPV4_CSUM, PKT_TX_UDP_CKSUM)  are needed by HW offload of non-tunneling and tunneling  packet.

> In general, I need to understand how to use all the new offloading stuff. In [5],
> new fields were added in the mbuf structure (inner_l3_len and inner_l2_len). I'm
> not sure I understand which fields have to be filled. Below is my understanding,
> can you please check that it is correct?
> 
> A- To send a Ether/IP/TCP packet and process IP and TCP TX
>     checksum in the NIC:
> 
>    - set l2_len = 14, l3_len = 20,
>      ol_flags = PKT_TX_IP_CKSUM | PKT_TX_TCP_CKSUM,
>      write all checksums to 0 in the packet
   IP checksum is 0, but tcp checksum is not 0.
   tcp_hdr->cksum = get_ipv4_psd_sum(ipv4_hdr);

> B- To send a Ether/IP/UDP/vxlan/Ether/IP/TCP packet and
>     process inner IP and inner TCP TX checksum in the NIC:
> 
>    - set l2_len = 14+20+8+8+14, l3_len = 20,

No, l2_len is outer L2 length, its value also is 14. 
>      ol_flags = PKT_TX_IP_CKSUM | PKT_TX_TCP_CKSUM,
>      write all checksums to 0 in the packet

   IP checksum is 0, but tcp checksum is not 0.
   tcp_hdr->cksum = get_ipv4_psd_sum(ipv4_hdr);

> C- To send a Ether/IP/UDP/vxlan/Ether/IP/TCP packet and
>     process outer IP and UDP, plus inner IP and inner TCP TX
>     checksum in the NIC:
> 
>    - set l2_len = 14, l3_len = 20, inner_l2_len = 14,
>      inner_l3_len = 20,
Yes
>      ol_flags = PKT_TX_IP_CKSUM | PKT_TX_TCP_CKSUM |
> PKT_TX_VXLAN_CKSUM,
Yes
>      write all checksums to 0 in the packet

   Outer and inner IP checksum is 0, but tcp checksum is not 0.
   tcp_hdr->cksum = get_ipv4_psd_sum(ipv4_hdr);


> D- To send a Ether/IP/UDP/vxlan/Ether/IP/TCP packet and
>     process outer IP and UDP TX checksum in the NIC:
> 
>    - set l2_len = 14, l3_len = 20,
>      ol_flags = PKT_TX_IP_CKSUM | PKT_TX_UDP_CKSUM,

Yes
>      write all checksums to 0 in the packet
Outer  IP checksum is 0, but tcp checksum is not 0.
   tcp_hdr->cksum = get_ipv4_psd_sum(ipv4_hdr);


> First, can you confirm this? I think it is very important to document it, as this is a
> public API that can be used by DPDK users.

These should be included in documents.

> To validate my modifications, I will try to reproduce the test plan described in [6].
> The test report contains useful information but I'm not sure to understand the
> following:
> 
>     Enable outer IP,UDP,TCP,SCTP and inner IP,UDP checksum offload
>     when inner L4 protocal is UDP.
>        testpmd>tx_checksum set 0 0xf3

"tx_checksum set 0 0xf3" should be "tx_checksum set 0 0x33", but the tester use 0xFX (that is to say, enable all inner TX flag)in order to write test script easily.

>     Enable outer IP,UDP,TCP,SCTP and inner IP,TCP,SCTP checksum offload
>     when inner L4 protocal is TCP or SCTP.
>        testpmd>tx_checksum set 0 0xfd
> 
> Can you give details about the signification of the bits used in the tx_checksum
> command? For instance, there is only one flag to enable tx checksum in the mbuf,
> so I don't understand how it's possible to do 0x44 = (inner tcp + outer tcp).

These bits meanings/help information  were provided in the patch
http://dpdk.org/ml/archives/dev/2014-October/007156.html 


> Today, there are hard-coded values for flags: 4 bits (0-3) for legacy checksums, 4
> bits for inner checksums (4-7), and one bit (bit 11?!) for vlan header. These bits
> are checked without defines at several places in the code. Moreover, there are
> some places in code where the testpmd port flags and the mbuf flags are mixed

> up. Example in macswap.c : mb->ol_flags = txp->tx_ol_flags;
I thought this is an issue.

> I suggest to remove all hardcoded values except in
> cmd_tx_cksum_set_parsed() and replace them by the flags definitions from
> rte_mbuf.h. As a result, the new possible arguments of
> cmd_tx_cksum_set_parsed() will map the mbuf flags:
>    - a flag to enable ip tx cksum
>    - a flag to enable udp tx cksum
>    - a flag to enable tcp tx cksum
>    - a flag to enable sctp tx cksum
>    - a flag to enable vxlan tx cksum
> If vxlan tx cksum is set, therefore the other considered checksum will concern the
> inner headers. Do you think it matches your needs?

As to HW TX checksum offload, do you have special requirement for implementing TSO?

I suggest to keep on using bit to indicate different protocol flag. this approach has good flexibility and extensibility.

For example, a VXLAN packet format: MAC/IP/UDP/VXLAN/inner MAC/inner IP/inner TCP/PAY4.

If the ip tx cksum and vxlan tx cksum  are set, but tcp tx cksum is not set,  and HW offload  of inner TCP TX checksum won't work.

In addition, sometimes we want to get the following performance data using testpmd
1. only enable outer IP TX checksum  
2. only enable inner IP TX checksum
3. only enable inner TCP/UDP/SCTP checksum
4. only enable inner IP TX checksum and inner TCP/UDP/SCTP checksum.

Now we have provided inner BIT masks, we can use these combinations to test it easily.

Actually, we just need to know that if  the PKT_TX_IPV4_CSUM, PKT_TX_TCP_CKSUM, PKT_TX_SCTP_CKSUM, PKT_TX_UDP_CKSUM and PKT_TX_VXLAN_CKSUM are set in ol_flags in the i40e driver.
Could you please look at the function i40e_txd_enable_checksum() .

> I think that the csum_only forward engine is now a bit complicated.
> As it's an example that shows how to configure the tx checksum, I think its
> behavior should be described somewhere, maybe in a comment in the file.
I agree.

> By the way, I was looking at the mbuf structure, and I see that a new packet_type
> field was added. There is no explanation about what this field should contain and
> how we shall use it (in commit log or in comments). Can you please explain it?

+	/**
+	 * The packet type, which is used to indicate ordinary packet and also
+	 * tunneled packet format, i.e. each number is represented a type of
+	 * packet.
+	 */
+	uint16_t packet_type
Regarding adding a packet_type in mbuf, we ever had a lot of discussions as follows:

http://dpdk.org/ml/archives/dev/2014-October/007027.html
http://dpdk.org/ml/archives/dev/2014-September/005240.html
http://dpdk.org/ml/archives/dev/2014-September/005241.html
http://dpdk.org/ml/archives/dev/2014-September/005274.html


> To conclude, I'd like to propose a merge of the TSO series but I'm currently
> blocked by these questions. Please, could you help me to progress on this?

Let me know if you have any questions? 

> Regards,
> Olivier
> 
> 
> [1] http://dpdk.org/ml/archives/dev/2014-May/002549.html
> [2] http://dpdk.org/ml/archives/dev/2014-October/007156.html
> [3]
> http://dpdk.org/browse/dpdk/commit/lib/librte_mbuf/rte_mbuf.h?id=15dbb63ef
> 9e9f108e7dcd837b88234f27a1ec258
> [4]
> http://dpdk.org/browse/dpdk/commit/lib/librte_mbuf/rte_mbuf.h?id=77b83017
> 33c3cd946648ca4a1375e3db0a952216
> [5]
> http://dpdk.org/browse/dpdk/commit/lib/librte_mbuf/rte_mbuf.h?id=73b7d59cf
> 4f6faf5ccd83ce706473e75c6fb8c3b
> [6] http://dpdk.org/ml/archives/dev/2014-October/007157.html



More information about the dev mailing list