From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-wm0-f53.google.com (mail-wm0-f53.google.com [74.125.82.53]) by dpdk.org (Postfix) with ESMTP id A5FAD1E2B for ; Mon, 9 Jan 2017 18:46:29 +0100 (CET) Received: by mail-wm0-f53.google.com with SMTP id k184so129908683wme.1 for ; Mon, 09 Jan 2017 09:46:29 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=6wind-com.20150623.gappssmtp.com; s=20150623; h=date:from:to:cc:subject:message-id:in-reply-to:references :mime-version:content-transfer-encoding; bh=Z4GG+qhn1O82e45b+NROnjwEKx6paGE6ljV73u9NYNM=; b=1nWKoLhUaSSfpSpDFGCmiuOI9EGoHaVUGeHniDiGshAHlJGc3toIeTY0W7UGdbuHoc mOoB5WpTIpIWJVkGs+kyvndVCU6NVqmU/IHFaBqG0fmfPhLsGd+OMn34SmgNuhR3TvG2 pxyQg+TBBb8MQ2DlkR7Gw4U4GtOa5bFJnreviO5Tn1qEKcqJRPv+WUDQw8V9lw4ODneH xx1fIRvVxdBwWHLA5vhPEeTZ0KDRqCiZ1q9orDMsHzqVYzALudewybdBAjqmWasMHQZ2 gj1WxizFc29846Z5urMV4lj0xvsYfTXYUShiTVaZ06z7+NmiQKCfVE1TqCYo8kuO/MgA nTjQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:date:from:to:cc:subject:message-id:in-reply-to :references:mime-version:content-transfer-encoding; bh=Z4GG+qhn1O82e45b+NROnjwEKx6paGE6ljV73u9NYNM=; b=cytanHVMrXd6/+Ox8i7ZbBE6wSvKnVYf5uYUc/PNy3bwqftZ3AqB+QhZ8h5dAO2ruw IJPj4ijLTn72eFnEtwfgt1R5bolzu+LDjVwao5My9ZnZWxGjZ3FpYaVwh71j3IVjlUFo vx+aMBVjtnuBxXQQroe4ugaxVJ9rmTTUHR5TiiptPFLvHGpTGwa+TmGMw7ozoAsZEzNp VqE2UIf2PJEpsf+C8UHZwE3nP/LkBHngKVzsj/FplhSVZKRHKByNDjiQ1lNvIvOsEFIQ 7nDoPClWtNGAYkeTRxlUV2Z8XjwX4YepyjHYQOl8Wk8p/XeMFvxKeedH9OWqsgMurxSt oFOg== X-Gm-Message-State: AIkVDXLgULGh/TOmEPo4tjsk0bYGgY5fYcrPGiuUhYi287jWEWqBAyrYsSFMnEUIDxaoF+e+ X-Received: by 10.28.71.133 with SMTP id m5mr3018221wmi.132.1483983989130; Mon, 09 Jan 2017 09:46:29 -0800 (PST) Received: from platinum (2a01cb0c03c651000226b0fffeed02fc.ipv6.abo.wanadoo.fr. [2a01:cb0c:3c6:5100:226:b0ff:feed:2fc]) by smtp.gmail.com with ESMTPSA id di9sm124989689wjc.37.2017.01.09.09.46.28 (version=TLS1_2 cipher=ECDHE-RSA-CHACHA20-POLY1305 bits=256/256); Mon, 09 Jan 2017 09:46:28 -0800 (PST) Date: Mon, 9 Jan 2017 18:46:25 +0100 From: Olivier Matz To: Yuanhan Liu Cc: dev@dpdk.org, maxime.coquelin@redhat.com, huawei.xie@intel.com, stephen@networkplumber.org, "Tan, Jianfeng" Message-ID: <20170109184625.7884290a@platinum> In-Reply-To: <20161214072750.GK18991@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> X-Mailer: Claws Mail 3.14.1 (GTK+ 2.24.31; x86_64-pc-linux-gnu) MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit 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, 09 Jan 2017 17:46:29 -0000 Hi Yuanhan, On Wed, 14 Dec 2016 15:27:50 +0800, Yuanhan Liu 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