From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga03.intel.com (mga03.intel.com [134.134.136.65]) by dpdk.org (Postfix) with ESMTP id B34183B5 for ; Mon, 16 Jan 2017 07:46:09 +0100 (CET) Received: from fmsmga004.fm.intel.com ([10.253.24.48]) by orsmga103.jf.intel.com with ESMTP; 15 Jan 2017 22:46:08 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.33,238,1477983600"; d="scan'208";a="213793372" Received: from yliu-dev.sh.intel.com (HELO yliu-dev) ([10.239.67.162]) by fmsmga004.fm.intel.com with ESMTP; 15 Jan 2017 22:46:06 -0800 Date: Mon, 16 Jan 2017 14:48:19 +0800 From: Yuanhan Liu To: Olivier Matz Cc: dev@dpdk.org, maxime.coquelin@redhat.com, huawei.xie@intel.com, stephen@networkplumber.org, "Tan, Jianfeng" , Tomasz Kulasek , Thomas Monjalon , Konstantin Ananyev Message-ID: <20170116064819.GL9770@yliu-dev.sh.intel.com> References: <1479977798-13417-1-git-send-email-olivier.matz@6wind.com> <1479977798-13417-6-git-send-email-olivier.matz@6wind.com> <20161214072750.GK18991@yliu-dev.sh.intel.com> <20170109184625.7884290a@platinum> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20170109184625.7884290a@platinum> User-Agent: Mutt/1.5.23 (2014-03-12) Subject: Re: [dpdk-dev] [PATCH 5/5] net/virtio: fix Tso when mbuf is shared X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 16 Jan 2017 06:46:10 -0000 On Mon, Jan 09, 2017 at 06:46:25PM +0100, Olivier Matz wrote: > 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. 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