Hi Olivier, > -----Original Message----- > From: Olivier MATZ [mailto:olivier.matz at 6wind.com] > Sent: Friday, January 16, 2015 5:28 PM > To: Ananyev, Konstantin; Liu, Jijiang > Cc: dev at dpdk.org > Subject: Re: [dpdk-dev] [PATCH v3 0/3] enhance TX checksum command and csum > forwarding engine > > Hi Konstantin, Hi Jijiang, > > On 01/15/2015 02:31 PM, Ananyev, Konstantin wrote: > > To be honest, there are so many mails around that subject, so I am already > > lost :) > > Oliver, as I understand you are not happy with the test-pmd commands Frank > > is proposing. > > Both syntax and semantics. > > Is that correct? > > If so, could you suggest something from your side? > > That would allow to configure test-pmd to behave in any of 4 possible ways > > we discussed previously: > > http://dpdk.org/ml/archives/dev/2014-December/009213.html > > I first wanted to send a mail to describe the current problem with > testpmd command line and the 2 solutions (Jijiang's and mine). > But, first, I think we need to fully clarify the checksum offload > API through examples as it will help to implement testpmd and do > the documentation. They are based on Jijiang's previous mail [1]. > > I will submit a patchset fixing the problems described below in > the coming days. If we agree on it, I'll submit another one for testpmd. > > Let's use the following packet for all the examples below: > out_eth / out_ip / out_udp / vxlan / in_eth / in_ip / in_tcp > > > The following cases are supposed to work on niantic and fortville: > > case 1) calculate checksum of out_ip (was case A in [1]) > > mb->l2_len = len(out_eth) > mb->l3_len = len(out_ip) > mb->ol_flags |= PKT_TX_IPV4 | PKT_TX_IP_CSUM > set out_ip checksum to 0 in the packet > > supported on hardware advertising DEV_TX_OFFLOAD_IPV4_CKSUM > > case 2) calculate checksum of out_ip and out_udp > > mb->l2_len = len(out_eth) > mb->l3_len = len(out_ip) > mb->ol_flags |= PKT_TX_IPV4 | PKT_TX_IP_CSUM | PKT_TX_UDP_CKSUM > set out_ip checksum to 0 in the packet > set out_udp checksum to pseudo header using rte_ipv4_phdr_cksum() > > supported on hardware advertising DEV_TX_OFFLOAD_IPV4_CKSUM and > DEV_TX_OFFLOAD_UDP_CKSUM > > *Problem 1*: The comment above PKT_TX_IPV4 says "Packet is IPv4 > without requiring IP checksum offload" [2], and the help of L4 > checksum and TSO says that it is required to set the PKT_TX_IPV4 > flag [3]. This is not coherent.
So what is the problem? Comments in rte_mbuf.h are not coherent? > > We are back on the debate about the meaning of PKT_TX_IPV4 vs > PKT_TX_IP_CSUM from [4]. This incoherency in comments are introduced > by patch [5]. The question is "when an application should set > this flag? for any IP packet that does not require IP checksum?". Yes, if it is an IPv4 packet and application required TX offload for L4 checksum or TSO, but doesn't want HW offload ofr IPV4 checksum calculation. > This would break many applications. Which ones? As I know, so far nothing is broken. > I think a good definition would > be: > > Packet is IPv4. This flag must be set when using any offload > feature (TSO, L3 or L4 checksum) to tell the NIC that the packet > is an IPv4 packet. > > That's why I added PKT_TX_IPV4 in the examples. I suppose we discussed it several times: both ways are possible. >From PMD perspective - treating PKT_TX_IPV4 and PKT_TX_IP_CSUM As mutually exclusive seems a bit more plausible. >From the upper layer - my understanding, that it is doesn't really matter. I thought we had an agreement about it in 1.8, no? > > case 3) calculate checksum of in_ip > > mb->l2_len = len(out_eth + out_ip + out_udp + vxlan + in_eth) > mb->l3_len = len(in_ip) > mb->ol_flags |= PKT_TX_IPV4 | PKT_TX_IP_CSUM > set in_ip checksum to 0 in the packet > > This is similar to case 1), but l2_len is different. > > supported on hardware advertising DEV_TX_OFFLOAD_IPV4_CKSUM > > Note that it can only work if outer L4 checksum is 0. > > case 4) calculate checksum of in_ip and in_tcp (was case B.2 in [1]) > > mb->l2_len = len(out_eth + out_ip + out_udp + vxlan + in_eth) > mb->l3_len = len(in_ip) > mb->ol_flags |= PKT_TX_IPV4 | PKT_TX_IP_CSUM | PKT_TX_TCP_CKSUM > set in_ip checksum to 0 in the packet > set in_tcp checksum to pseudo header using rte_ipv4_phdr_cksum() > > This is similar to case 2), but l2_len is different. > > supported on hardware advertising DEV_TX_OFFLOAD_IPV4_CKSUM and > DEV_TX_OFFLOAD_TCP_CKSUM > > Note that it can only work if outer L4 checksum is 0. > > case 5) segment inner TCP > > mb->l2_len = len(out_eth + out_ip + out_udp + vxlan + in_eth) > mb->l3_len = len(in_ip) > mb->l4_len = len(in_tcp) > mb->ol_flags |= PKT_TX_IPV4 | PKT_TX_IP_CKSUM | PKT_TX_TCP_CKSUM | > PKT_TX_TCP_SEG; > set in_ip checksum to 0 in the packet > set in_tcp checksum to pseudo header without including the IP > payload length using rte_ipv4_phdr_cksum() > > supported on hardware advertising DEV_TX_OFFLOAD_TCP_TSO. > > Note that it can only work if outer L4 checksum is 0. > > Problem 1 is also visible here. > > > The following cases are supposed to *work on fortville*: > > case 6) calculate checksum of out_ip, in_ip, in_tcp (was case C in [1]) > > mb->outer_l2_len = len(out_eth) > mb->outer_l3_len = len(out_ip) > mb->l2_len = len(out_udp + vxlan + in_eth) > mb->l3_len = len(in_ip) > mb->ol_flags |= PKT_TX_OUTER_IPV4 | PKT_TX_OUTER_IP_CKSUM | \ > PKT_TX_IP_CKSUM | PKT_TX_TCP_CKSUM; > set out_ip checksum to 0 in the packet > set in_ip checksum to 0 in the packet > set in_tcp checksum to pseudo header using rte_ipv4_phdr_cksum() > > supported on hardware advertising DEV_TX_OFFLOAD_IPV4_CKSUM, > DEV_TX_OFFLOAD_UDP_CKSUM and DEV_TX_OFFLOAD_OUTER_IPV4_CKSUM > > *Problem 2*: it is not written in the API comments that out_ip > checksum field should be set to 0 by the application. They should > be enhanced. Ok > > *Problem 3*: without using the word "fortville", it is difficult > to understand the goal of the flag PKT_TX_UDP_TUNNEL_PKT. Indeed, > once PKT_TX_OUTER_IPV4/6 is set, it looks obvious that it's a > tunnel packet. I suggest to remove the PKT_TX_UDP_TUNNEL_PKT > flag. In linux, the driver doesn't care about the tunnel type, > it always set I40E_TXD_CTX_UDP_TUNNELING for all encapsulations [6]. It might be obvious that it is a tunnel packet from PKT_TX_OUTER_* is set, but it is not obvious what type of tunnelling it would be. FVL HW supports HW TX offloads for different type of tunnelling and requires that SW provide information about tunnelling type. >From i40e datasheet: L4TUNT L4 Tunneling Type (Teredo / GRE header / VXLAN header) indication: 00b - No UDP / GRE tunneling (field must be set to zero if EIPT equals to zero) 01b - UDP tunneling header (any UDP tunneling, VXLAN and Geneve). 10b - GRE tunneling header As we do plan to support other than UDP tunnelling types, I suppose we'll need to keep PKT_TX_UDP_TUNNEL_PKT flag. > > *Problem 4*: features flags are missing here. A flag > DEV_TX_OFFLOAD_OUTER_IPV4_CKSUM should be added. This is already > addressed by one patch from Jijiang [7] Ok, yes I think Frank already submit a patch for that. > > > The cases should work in some *future drivers*: > > case 7) calculate checksum of out_ip, out_udp, in_ip and in_tcp > > mb->outer_l2_len = len(out_eth) > mb->outer_l3_len = len(out_ip) > mb->l2_len = len(out_udp + vxlan + in_eth) > mb->l3_len = len(in_ip) > mb->ol_flags |= PKT_TX_OUTER_IPV4 | PKT_TX_OUTER_IP_CKSUM | \ > PKT_TX_OUTER_UDP_CKSUM | PKT_TX_IP_CKSUM | PKT_TX_TCP_CKSUM; > set out_ip checksum to 0 in the packet > set out_udp checksum to pseudo header using rte_ipv4_phdr_cksum() > set in_ip checksum to 0 in the packet > set in_tcp checksum to pseudo header using rte_ipv4_phdr_cksum() > > We need to add the flag PKT_TX_OUTER_UDP_CKSUM. We can, though right now we don't have a HW that is able to do that. Why need to do it now? > > case 8) TSO on inner header + out_ip checksum > > This is not supported yet, but latest patch from Jijiang [8] > implements this feature. > > mb->outer_l2_len = len(out_eth) > mb->outer_l3_len = len(out_ip) > mb->l2_len = len(out_udp + vxlan + in_eth) > mb->l3_len = len(in_ip) > mb->l4_len = len(in_tcp) > mb->ol_flags |= PKT_TX_OUTER_IP_CKSUM | PKT_TX_OUTER_IPV4 | \ > PKT_TX_IPV4 | PKT_TX_IP_CKSUM | PKT_TX_TCP_CKSUM | \ > PKT_TX_TCP_SEG; > set out_ip checksum to 0 in the packet > set in_ip checksum to 0 in the packet > set in_tcp checksum to pseudo header without including the IP > payload length using rte_ipv4_phdr_cksum() > > supported on hardware advertising DEV_TX_OFFLOAD_OUTER_IPV4_CKSUM > and DEV_TX_OFFLOAD_TCP_TSO. > > > I think the following cases should be *forbidden by the API*: > > case 9) calculate checksum of in_ip and in_tcp (was case B.1 in [1]) > > mb->outer_l2_len = len(out_eth) > mb->outer_l3_len = len(out_ip) > mb->l2_len = len(out_udp + vxlan + in_eth) > mb->l3_len = len(out_ip) > mb->ol_flags |= PKT_TX_IPV4 | PKT_TX_UDP_TUNNEL_PKT | \ > PKT_TX_IP_CSUM | PKT_TX_UDP_CKSUM; > set out_ip checksum to 0 in the packet > set out_udp checksum to pseudo header using rte_ipv4_phdr_cksum() > > If we remove the flag PKT_TX_UDP_TUNNEL_PKT, this cannot be > supported, but there is no reason to support it as there is > already one way to do the same. > > I think the driver should not even look at mb->outer_l2_len > and mb->outer_l3_len if no flag PKT_TX_OUTER_* is set. Why it should be forbidden? I admit it might be a bit slower than case 4), but I think absolutely legal way to setup HW offloads for inner L3/L4. As I said we need a PKT_TX_UDP_TUNNEL_PKT anyway, so I suppose PKT_TX_*_TUNNEL_PKT should be an indication is it a tunnel packet or not. PKT_TX_OUTER_* flags indicate does outer cksum offload is required or not. > > case 10) calculate a checksum using only outer_lX fields > > The outer_lX fields or PKT_TX_OUTER_* flags can only be used > if a inner checksum is enabled. So it's not possible to do > the following: > > mb->outer_l2_len = len(out_eth) > mb->outer_l3_len = len(out_ip) > mb->ol_flags |= PKT_TX_OUTER_IPV4 | PKT_TX_OUTER_IP_CSUM > set out_ip checksum to 0 in the packet Ok, I think no one plans to use it anyway. Konstantin > > Regards, > Olivier > > > > [1] http://dpdk.org/ml/archives/dev/2014-December/009213.html > [2] > http://dpdk.org/browse/dpdk/tree/lib/librte_mbuf/rte_mbuf.h?id=v1.8.0#n147 > [3] > http://dpdk.org/browse/dpdk/tree/lib/librte_mbuf/rte_mbuf.h?id=v1.8.0#n108 > [4] http://dpdk.org/ml/archives/dev/2014-December/009352.html > [5] > http://dpdk.org/browse/dpdk/commit/lib/librte_mbuf/rte_mbuf.h?id=711ba9e23e681b97d547219de8af199ea03a33b3 > [6] > http://lxr.free-electrons.com/source/drivers/net/ethernet/intel/i40evf/i40e_txrx.c?v=3.17#L1223 > [7] http://dpdk.org/dev/patchwork/patch/1907/ > [8] http://dpdk.org/dev/patchwork/patch/2329/
