Hi Yuanhan,

On Wed, 14 Dec 2016 15:27:50 +0800, Yuanhan Liu
[off-list ref] wrote:

Firstly sorry for late response!

No problem, I fully understand ;)

On Thu, Nov 24, 2016 at 09:56:38AM +0100, Olivier Matz wrote:

quoted

With virtio, doing tso requires to modify the network
packet data:  

I thought more about it this time, and I'm wondering why it's needed.

quoted

- the dpdk API requires to set the l4 checksum to an
  Intel-Nic-like pseudo header checksum that does
  not include the ip length  

If the packet is for a NIC pmd driver in the end, then the NIC driver
(or application) would handle the checksum correctly.  You could check
the tx_prep patchset for example.

quoted

- the virtio peer expects that the l4 checksum is
  a standard pseudo header checksum.  

For this case, the checksum is then not needed: we could assume the
data between virtio to virtio transmission on the same host is always
valid, that checksum validation is unnecessary.

So, in either case, it doesn't seem to me we have to generate the
checksum here. Or am I miss something?

The virtio specifications requires that the L4 checksum is set to the
pseudo header checksum. You can search for "pseudo header" in the
following doc:
http://docs.oasis-open.org/virtio/virtio/v1.0/cs04/virtio-v1.0-cs04.pdf

Especially in 5.1.6.2.1, we can see that if we use the csum flag, we
must set the checksum to phdr, and if we do tso, we must set the csum
flag.

We can check that this is really needed with Linux vhost by replaying
the test plan described at [1].

[1] http://dpdk.org/ml/archives/dev/2016-October/048793.html

If we add the following patch to disable the checksum fix (on top of
this patchset), the test1 "large packets (lro/tso)" won't work.

--- a/drivers/net/virtio/virtio_rxtx.c
+++ b/drivers/net/virtio/virtio_rxtx.c

@@ -224,6 +224,9 @@
        uint32_t tmp;
        int shared = 0;
 
+        if (1)
+               return 0;
+
        /* mbuf is write-only, we need to copy the headers in a linear

buffer */ if (unlikely(rte_pktmbuf_data_is_shared(m, 0, hdrlen))) {
                shared = 1;


In one direction ("flow1" in the test desc), large packets are
transmitted from host on the ixgbe interface, and received by the
guest. Then, testpmd bridges the packet to the virtio interface. But
the packet is not received by the host.

OTOH, even if it does, I still see some issues (see below).

quoted

 		/* TCP Segmentation Offload */
 		if (cookie->ol_flags & PKT_TX_TCP_SEG) {
-			virtio_tso_fix_cksum(cookie);
+			offset = virtio_tso_fix_cksum(cookie,
+				RTE_PTR_ADD(hdr,
start_dp[hdr_idx].len),
+				VIRTIO_MAX_HDR_SZ);
+			if (offset > 0) {
+				RTE_ASSERT(can_push != 0);  

I think it's (can_push == 0) ?

Yes, indeed. I'll fix that in next version.

quoted

+				start_dp[hdr_idx].len += offset;  

Actually, there is an assumption if you do this, that the backend
driver must have to support ANY_LAYOUT. Otherwise, it won't work: the
driver would expect the header and packet data is totally separated
into two desc buffers.

Though the assumption is most likely true in nowadays, I don't think
it's a guarantee.

Right.

There are at least 2 options for this one:

- try to use 2 different descriptors (the patch is probably harder,
  and it may slow-down the case where ANY_LAYOUT is supported)

- refuse to initialize with TSO enabled if ANY_LAYOUT is not supported.

If you think ANY_LAYOUT is most likely true today, we could choose
option 2. Let me know what's your preference here.

Thank you for the review.

Olivier