From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from dpdk.org (dpdk.org [92.243.14.124]) by dpdk.space (Postfix) with ESMTP id E4B1CA0096 for ; Wed, 5 Jun 2019 11:38:42 +0200 (CEST) Received: from [92.243.14.124] (localhost [127.0.0.1]) by dpdk.org (Postfix) with ESMTP id B05BA1B9D6; Wed, 5 Jun 2019 11:38:42 +0200 (CEST) Received: from mga06.intel.com (mga06.intel.com [134.134.136.31]) by dpdk.org (Postfix) with ESMTP id 605A51B9B8; Wed, 5 Jun 2019 11:38:40 +0200 (CEST) X-Amp-Result: UNSCANNABLE X-Amp-File-Uploaded: False Received: from fmsmga002.fm.intel.com ([10.253.24.26]) by orsmga104.jf.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 05 Jun 2019 02:38:39 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.60,550,1549958400"; d="scan'208";a="181869711" Received: from npg-dpdk-virtio-tbie-2.sh.intel.com (HELO ___) ([10.67.104.151]) by fmsmga002.fm.intel.com with ESMTP; 05 Jun 2019 02:38:37 -0700 Date: Wed, 5 Jun 2019 17:37:29 +0800 From: Tiwei Bie To: Andrew Rybchenko Cc: Maxime Coquelin , Zhihong Wang , dev@dpdk.org, Dilshod Urazov , stable@dpdk.org Message-ID: <20190605093729.GA16974@___> References: <1559587805-1637-1-git-send-email-arybchenko@solarflare.com> <20190605014117.GA20728@___> <8fb39bf5-2bd2-7faa-9ab7-551e234d77c5@solarflare.com> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <8fb39bf5-2bd2-7faa-9ab7-551e234d77c5@solarflare.com> User-Agent: Mutt/1.9.4 (2018-02-28) Subject: Re: [dpdk-dev] [PATCH v2] net/virtio: add Tx preparation 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: , Errors-To: dev-bounces@dpdk.org Sender: "dev" On Wed, Jun 05, 2019 at 10:28:14AM +0300, Andrew Rybchenko wrote: > Hi, > > On 6/5/19 4:41 AM, Tiwei Bie wrote: > > Hi, > > Thanks for the patch! > > > More will follow. At least Tx checksum offload is broken when used together > with VLAN insertion since the later prepend to mbuf, but do nothing with > l2_len/outer_l2_len. We'll send a patch to move rte_vlan_insert() > to Tx prepare and suggest separate patch to update l2_len or outer_l2_len > when software VLAN insertion is done. > Thanks :) > > On Mon, Jun 03, 2019 at 07:50:05PM +0100, Andrew Rybchenko wrote: > [...] > > uint16_t > +virtio_xmit_pkts_prepare(void *tx_queue __rte_unused, struct rte_mbuf **tx_pkts, > + uint16_t nb_pkts) > +{ > + uint16_t nb_tx; > + int error; > + > + for (nb_tx = 0; nb_tx < nb_pkts; nb_tx++) { > + struct rte_mbuf *m = tx_pkts[nb_tx]; > + > +#ifdef RTE_LIBRTE_ETHDEV_DEBUG > + error = rte_validate_tx_offload(m); > + if (unlikely(error)) { > + rte_errno = -error; > + break; > + } > +#endif > + > + error = rte_net_intel_cksum_prepare(m); > + if (unlikely(error)) { > + rte_errno = -error; > > It's a bit confusing here. > Based on the API doc of rte_eth_tx_prepare(): > > https://github.com/DPDK/dpdk/blob/7f9f46d6cef5b03681a3935b9a18378e08ca6f62/lib/librte_ethdev/rte_ethdev.h#L4360-L4362 > > It should set negative value to rte_errno when error happens, > > > I'm pretty sure that it is a bug in documentation. rte_errno must be positive. Yeah. Negative rte_errno looks confusing. > I'll send a patch to fix it. > Even the code just below sets positive rte_errno. Simple cases are very easy to > find: > $ git grep 'rte_errno = E' | wc -l > 557 > $ git grep 'rte_errno = -E' | wc -l > 50 > > A bit more complex cases which require careful review: > $ git grep -e 'rte_errno = -[a-z]' | wc -l > 37 > $ git grep -e 'rte_errno = [a-z]' | wc -l > 150 > > Cases which look right from the first sight overweight wrong 3 times. > But it is still too many cases which are potentially wrong. > > Andrew. Thanks, Tiwei