From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga11.intel.com (mga11.intel.com [192.55.52.93]) by dpdk.org (Postfix) with ESMTP id 270AFC3AA for ; Wed, 22 Jun 2016 11:19:25 +0200 (CEST) Received: from fmsmga003.fm.intel.com ([10.253.24.29]) by fmsmga102.fm.intel.com with ESMTP; 22 Jun 2016 02:19:17 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.26,509,1459839600"; d="scan'208";a="723279808" Received: from bricha3-mobl3.ger.corp.intel.com ([10.237.220.108]) by FMSMGA003.fm.intel.com with SMTP; 22 Jun 2016 02:19:16 -0700 Received: by (sSMTP sendmail emulation); Wed, 22 Jun 2016 10:19:14 +0025 Date: Wed, 22 Jun 2016 10:19:14 +0100 From: Bruce Richardson To: Ferruh Yigit , Nelio Laranjeiro , dev@dpdk.org Message-ID: <20160622091914.GA9728@bricha3-MOBL3> References: <1465379291-25310-1-git-send-email-nelio.laranjeiro@6wind.com> <1466493818-1877-1-git-send-email-nelio.laranjeiro@6wind.com> <57696E75.8050402@intel.com> <20160622082054.GY7621@6wind.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20160622082054.GY7621@6wind.com> Organization: Intel Research and =?iso-8859-1?Q?De=ACvel?= =?iso-8859-1?Q?opment?= Ireland Ltd. User-Agent: Mutt/1.5.23 (2014-03-12) Subject: Re: [dpdk-dev] [PATCH v3 00/25] Refactor mlx5 to improve performance X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: patches and discussions about DPDK List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 22 Jun 2016 09:19:25 -0000 On Wed, Jun 22, 2016 at 10:20:54AM +0200, Adrien Mazarguil wrote: > On Tue, Jun 21, 2016 at 05:42:29PM +0100, Ferruh Yigit wrote: > > On 6/21/2016 8:23 AM, Nelio Laranjeiro wrote: > > > Enhance mlx5 with a data path that bypasses Verbs. > > > > > > The first half of this patchset removes support for functionality completely > > > rewritten in the second half (scatter/gather, inline send), while the data > > > path is refactored without Verbs. > > > > > > The PMD remains usable during the transition. > > > > > > This patchset must be applied after "Miscellaneous fixes for mlx4 and mlx5". > > > > > > Changes in v3: > > > - Rebased patchset on top of next-net/rel_16_07. > > > > > > Changes in v2: > > > - Rebased patchset on top of dpdk/master. > > > - Fixed CQE size on Power8. > > > - Fixed mbuf assertion failure in debug mode. > > > - Fixed missing class_id field in rte_pci_id by using RTE_PCI_DEVICE. > > > > > > Adrien Mazarguil (8): > > > mlx5: replace countdown with threshold for Tx completions > > > mlx5: add debugging information about Tx queues capabilities > > > mlx5: check remaining space while processing Tx burst > > > mlx5: resurrect Tx gather support > > > mlx5: work around spurious compilation errors > > > mlx5: remove redundant Rx queue initialization code > > > mlx5: make Rx queue reinitialization safer > > > mlx5: resurrect Rx scatter support > > > > > > Nelio Laranjeiro (16): > > > drivers: fix PCI class id support > > > mlx5: split memory registration function > > > mlx5: remove Tx gather support > > > mlx5: remove Rx scatter support > > > mlx5: remove configuration variable > > > mlx5: remove inline Tx support > > > mlx5: split Tx queue structure > > > mlx5: split Rx queue structure > > > mlx5: update prerequisites for upcoming enhancements > > > mlx5: add definitions for data path without Verbs > > > mlx5: add support for configuration through kvargs > > > mlx5: add Tx/Rx burst function selection wrapper > > > mlx5: refactor Rx data path > > > mlx5: refactor Tx data path > > > mlx5: handle Rx CQE compression > > > mlx5: add support for multi-packet send > > > > > > Yaacov Hazan (1): > > > mlx5: add support for inline send > > > > > > > Patchset applies and compiles fine, thanks. > > > > But still has some checkpatch warnings, -btw, I am using the checkpatch > > script from latest master branch of Linux repo. > > > > Following is the sample type of warnings (not instances, there are more > > than one instance per type): > > While Nelio is preparing a v4 to address the kvargs issue, the remaining > warnings can be safely ignored. > > A few of them are in relocated but unmodified code as this patchset > refactors the entire PMD, others are documented. We settled on positive > errno values internally because mlx5 uses syscalls and switching the sign > bit all over the place quickly became unmanageable. They are made negative > when returning from public callbacks (except for kvargs by mistake). > > In short, we did run checkpatch, fixed a million warnings and other errors > and left those on purpose, nothing to worry about. > Yes, they are nothing to worry about, but at the same time, I fail to see why most of them should not be fixed. Even if you are moving code, unless it's a whole file it's not going to show up as a move in the diff, so some small changes during the move can be ok. > > WARNING:UNSPECIFIED_INT: Prefer 'unsigned int' to bare use of 'unsigned' > > #112: FILE: drivers/net/mlx5/mlx5_mr.c:65: > > + unsigned mem_idx) > > This looks easily fixable. > > WARNING:BLOCK_COMMENT_STYLE: Block comments use a trailing */ on a > > separate line > > #288: FILE: drivers/net/mlx5/mlx5_mr.c:241: > > + * pointer is valid. */ > > I also think this should be fixed. > > WARNING:USE_NEGATIVE_ERRNO: return of an errno should typically be > > negative (ie: return -EINVAL) > > #524: FILE: drivers/net/mlx5/mlx5_txq.c:265: > > + return EINVAL; > > > > WARNING:LONG_LINE: line over 80 characters > > #108: FILE: drivers/net/mlx5/mlx5_ethdev.c:1250: > > + txq_ctrl->txq.stats.idx = > > primary_txq->stats.idx; > > > > WARNING:STATIC_CONST_CHAR_ARRAY: static const char * array should > > probably be static const char * const > > #88: FILE: drivers/net/mlx5/mlx5.c:281: > > + static const char *params[] = { > > > > ERROR:ASSIGN_IN_IF: do not use assignment in if condition > > #218: FILE: drivers/net/mlx5/mlx5_rxtx.c:92: > > + if (!ret || !(ret = ((*buf)[i] == magic[i]))) > > > > CHECK:SPACING: spaces preferred around that '&' (ctx:VxV) > > #414: FILE: drivers/net/mlx5/mlx5_rxtx.c:625: > > + (uintptr_t)&(*rxq->cqes)[rxq->cq_ci & > > ^ > > > > WARNING:INDENTED_LABEL: labels should not be indented > > #520: FILE: drivers/net/mlx5/mlx5_rxtx.c:789: > > + skip: This I also feel should be fixed too. Regards, /Bruce