From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga06.intel.com (mga06.intel.com [134.134.136.31]) by dpdk.org (Postfix) with ESMTP id E2129DE5 for ; Sat, 28 Jan 2017 13:29:59 +0100 (CET) Received: from fmsmga001.fm.intel.com ([10.253.24.23]) by orsmga104.jf.intel.com with ESMTP; 28 Jan 2017 04:29:58 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.33,301,1477983600"; d="scan'208";a="1100088666" Received: from yliu-dev.sh.intel.com (HELO yliu-dev) ([10.239.67.162]) by fmsmga001.fm.intel.com with ESMTP; 28 Jan 2017 04:29:56 -0800 Date: Sat, 28 Jan 2017 20:32:21 +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: <20170128123221.GJ10293@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> <20170116064819.GL9770@yliu-dev.sh.intel.com> <20170117121825.3db48369@platinum> <20170118050348.GO10293@yliu-dev.sh.intel.com> <20170124115120.2c2667f4@glumotte.dev.6wind.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20170124115120.2c2667f4@glumotte.dev.6wind.com> 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: Sat, 28 Jan 2017 12:30:00 -0000 On Tue, Jan 24, 2017 at 11:51:20AM +0100, Olivier MATZ wrote: > On Wed, 18 Jan 2017 13:03:48 +0800, Yuanhan Liu > wrote: > > On Tue, Jan 17, 2017 at 12:18:25PM +0100, Olivier Matz wrote: > > > > I hope I could have time to dig this further, since, honestly, I > > > > don't quite like this patch: it makes things un-maintainable. > > > > > > Well, I'm not that proud of the patch, but that's the best solution > > > I've found. Nevertheless saying it makes things un-maintainable > > > looks a bit excessive to me :) > > > > Aha... really sorry about that! > > > > But honestly, I'd say again, it makes thing more complex, just for > > fixing a corner and rare issue. I'd try to avoid that. > > > > > > > > The option of reallocating a mbuf, copy and fix network headers in > > > it looks even more complex to me (that was my first approach). > > > > > > > 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"). > > > > > > Yes, that was discussed a bit. See [1] and the subsequent mails. > > > http://dpdk.org/ml/archives/dev/2016-December/051014.html > > > > Thanks for the info, and I'm pretty Okay with that. > > > > > My opinion is that tx_burst() should not change the mbuf data, it's > > > always been like this. For Intel NICs, there is no issue since the > > > DPDK API is derived from Intel NICs API, so there is no fix to do > > > in the mbuf data. > > > > > > For tx_prepare(), it's explicitly said that it can update the data. > > > If tx_prepare() becomes mandatory, it will naturally fix this issue > > > without modifying the driver, because the phdr csum calculation > > > will be done in tx_prepare(). > > > > > > An alternative is to mark this as a known issue for now, and wait > > > until tx_prepare() is mandatory. > > > > I see no reason to wait. Though my understanding is it may not be a > > mandatory so far, but user is supposed to calculate the pseudo-header > > checksum by themself before. Now they have one more option: > > tx_prepare. > > > > That means, in either way, user has to do some extra works to make > > TSO work (either by themself or call tx_prepare). So I don't think > > it'd be an issue? > > Yes sounds good. I'll check how a tx_prepare() could be implemented for > virtio, in order to fix this issue. Thanks. > In the meanwhile, for the 17.02, I think it could be good to highlight > the problem in the known issues, what do you think? Yes, I think it's a good idea. --yliu