Re: [PATCH 5/5] net/virtio: fix Tso when mbuf is shared
From: Yuanhan Liu <hidden>
Date: 2017-01-16 06:46:09
On Mon, Jan 09, 2017 at 06:46:25PM +0100, Olivier Matz wrote:
quoted hunk ↗ jump to hunk
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 linearbuffer */ 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.
I hope I could have time to dig this further, since, honestly, I don't
quite like this patch: it makes things un-maintainable.
Besides that, I think we have similar issue with nic drivers. See the
rte_net_intel_cksum_flags_prepare() function introduced at commit
4fb7e803eb1a ("ethdev: add Tx preparation").
Cc more people here. And here is a quick background for them: NIC drivers
doing TSO need change the mbuf (say, for cksum updating), however, as
Stephen pointed out, we could not do that if the mbuf is shared: I don't
see such checks in the driver code as well.
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.
Maybe we could go with a simpler one: COW. Yeah, it costs more, but this would be rare, that it should be OKay, right? Besides, we just need copy the heading mbuf. --yliu