Re: [dpdk-dev] [PATCH 5/5] net/virtio: fix Tso when mbuf is shared

[Olivier Matz <olivier.matz@6wind.com>]

Hi Yuanhan,

On Wed, 14 Dec 2016 15:27:50 +0800, Yuanhan Liu
<yuanhan.liu@linux.intel.com> wrote:
> Firstly sorry for late response!

No problem, I fully understand ;)

> On Thu, Nov 24, 2016 at 09:56:38AM +0100, Olivier Matz wrote:
> > 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.
> 
> > - 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.
> 
> > - 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).
> 
> >  		/* 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.

> > +				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