From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-wm0-f41.google.com (mail-wm0-f41.google.com [74.125.82.41]) by dpdk.org (Postfix) with ESMTP id 2FAAFAD84 for ; Thu, 23 Jun 2016 17:14:32 +0200 (CEST) Received: by mail-wm0-f41.google.com with SMTP id f126so131245251wma.1 for ; Thu, 23 Jun 2016 08:14:32 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=6wind-com.20150623.gappssmtp.com; s=20150623; h=date:from:to:subject:message-id:mail-followup-to:references :mime-version:content-disposition:in-reply-to; bh=euickF2GoVJ7V+clHdSfjxQsKriWLmOs/GvdZgpDKz4=; b=D4+yUiI8r359wxcGecT1D1yWR17bl9RWxd8Pn34ilBGQL6mEH4m0E5lXHRkxceQByh skQ4ELuwOYtAf6vYZIQKwTWs/R5sVVKV6T7XJcx/LiaKJZosjFKS/DPUWBaxkab73lPQ YmvNaHK7KVRTVoSuIFcEo/g8n4AX7e4zeu9S7/G62czcpok5fN4brDa4nuFGACmgGXiB VJbRKQHZc1XcszrVN+1WzrsQAMn0Og9oxY2NsQTjPB7jIrnTxK9VOBJWP3N2GVH5ovbf HwLazv5EkxIIsDS7lK1uYM7PVSq34RiMwykvYcXRlmN+sDArZBkaLLshSZhCYMNjDY8p h1jQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:date:from:to:subject:message-id:mail-followup-to :references:mime-version:content-disposition:in-reply-to; bh=euickF2GoVJ7V+clHdSfjxQsKriWLmOs/GvdZgpDKz4=; b=JppeG4lNMB2rQ8zJfi8KEAjrbSgwcs6hmmuQ2PdjF2GDzrMDZDPEyGhkT8ck6sXQjR RxjTsRdcBBEcIGZVMfe4H2RlVwfR+1k7N6vWwQdwWJwedc+HX/qvNGQaUaE2B/wpBREi Iy2z8SI6PABh5VGStmRLN0xnJsy4WgEnlIUxhXxe+1l9WkO9nTmLh6YgPBLoGiUrmVbd x8P27J6wgCMq/vLfbtBEnvKlCzAMqH0gCRNk8E6gs7CnGbzuQ6Z2Ale6ej70YZs0gQws dVpsy/Kct+21o56ST6hVUk2y4K5gdPPKd8jExivhJFaqqiI6t+p4z/U0Kxr7PyLiCcx/ M3nw== X-Gm-Message-State: ALyK8tJ87jJeygu7yh1GFa4MEOqlxlyRE0dvlxDzA13rJvXPcbRbLMYA8UuRhAh2AonXrP9r X-Received: by 10.194.178.199 with SMTP id da7mr30266044wjc.123.1466694871770; Thu, 23 Jun 2016 08:14:31 -0700 (PDT) Received: from 6wind.com (guy78-3-82-239-227-177.fbx.proxad.net. [82.239.227.177]) by smtp.gmail.com with ESMTPSA id q203sm1185363wmd.24.2016.06.23.08.14.29 (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Thu, 23 Jun 2016 08:14:30 -0700 (PDT) Date: Thu, 23 Jun 2016 17:14:29 +0200 From: Adrien Mazarguil To: Bruce Richardson , Ferruh Yigit , Nelio Laranjeiro , dev@dpdk.org Message-ID: <20160623151429.GD7621@6wind.com> Mail-Followup-To: Bruce Richardson , Ferruh Yigit , Nelio Laranjeiro , dev@dpdk.org 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> <20160622091914.GA9728@bricha3-MOBL3> <20160622093005.GA7621@6wind.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20160622093005.GA7621@6wind.com> 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: Thu, 23 Jun 2016 15:14:32 -0000 On Wed, Jun 22, 2016 at 11:30:05AM +0200, Adrien Mazarguil wrote: > On Wed, Jun 22, 2016 at 10:19:14AM +0100, Bruce Richardson wrote: > > 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. > > Right but it won't prevent some of them to still show up anyway. We will > take them into account in the future. > > Now, considering these warnings are minor and were already there before, do > you think Nelio needs to spam the world with a v5? > > Looks like he forgot "v4" in the subject of his last cover letter as well. After discussing this with Nelio (and cooling down a bit), we decided to submit a v5 to address the above warnings, except false positives. Then for the remaining stuff, namely the fact we use positive errno values internally and existing code that does not conform DPDK coding rules, we will submit another set of patches for 16.11 to address them all at once. -- Adrien Mazarguil 6WIND