From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp.tuxdriver.com (charlotte.tuxdriver.com [70.61.120.58]) by dpdk.org (Postfix) with ESMTP id 922513F9 for ; Tue, 16 Dec 2014 22:40:16 +0100 (CET) Received: from hmsreliant.think-freely.org ([2001:470:8:a08:7aac:c0ff:fec2:933b] helo=localhost) by smtp.tuxdriver.com with esmtpsa (TLSv1:AES128-SHA:128) (Exim 4.63) (envelope-from ) id 1Y0zqm-0003BE-Mx; Tue, 16 Dec 2014 16:40:15 -0500 Date: Tue, 16 Dec 2014 16:40:06 -0500 From: Neil Horman To: Ravi Kerur Message-ID: <20141216213420.GE13806@hmsreliant.think-freely.org> References: <20141213103921.GA1162@localhost.localdomain> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.5.23 (2014-03-12) X-Spam-Score: -2.9 (--) X-Spam-Status: No Cc: "dev@dpdk.org" Subject: Re: [dpdk-dev] [PATCH] Minor fixes in rte_common.h file. 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: Tue, 16 Dec 2014 21:40:17 -0000 On Tue, Dec 16, 2014 at 08:46:51AM -0800, Ravi Kerur wrote: > On Sat, Dec 13, 2014 at 2:39 AM, Neil Horman wrote: > > > > On Fri, Dec 12, 2014 at 03:04:34PM -0800, r k wrote: > > > Subject: [PATCH] Minor fixes in rte_common.h file. > > > > > > Fix rte_is_power_of_2 since 0 is not. > > > Avoid branching instructions in RTE_MAX and RTE_MIN. > > > > > > Signed-off-by: Ravi Kerur > > > --- > > > lib/librte_eal/common/include/rte_common.h | 6 +++--- > > > lib/librte_pmd_e1000/igb_pf.c | 4 ++-- > > > lib/librte_pmd_ixgbe/ixgbe_pf.c | 4 ++-- > > > 3 files changed, 7 insertions(+), 7 deletions(-) > > > > > > diff --git a/lib/librte_eal/common/include/rte_common.h > > > b/lib/librte_eal/common/include/rte_common.h > > > index 921b91f..e163f35 100644 > > > --- a/lib/librte_eal/common/include/rte_common.h > > > +++ b/lib/librte_eal/common/include/rte_common.h > > > @@ -203,7 +203,7 @@ extern int RTE_BUILD_BUG_ON_detected_error; static > > > inline int rte_is_power_of_2(uint32_t n) { > > > - return ((n-1) & n) == 0; > > > + return n && !(n & (n - 1)); > > > } > > > > > > /** > > > @@ -259,7 +259,7 @@ rte_align64pow2(uint64_t v) #define RTE_MIN(a, b) > > ({ \ > > > typeof (a) _a = (a); \ > > > typeof (b) _b = (b); \ > > > - _a < _b ? _a : _b; \ > > > + _b ^ ((_a ^ _b) & -(_a < _b)); \ > > Are you sure this is actually faster than the branch version? What about > > using > > a cmov instead? > > > > > i am pretty sure modified code is faster than branching. I remember > cmov had performance issues esp. on Pentuim-4 not sure how new intel cpu's > perform. > Pretty sure isn't sure. Theres no point in code churn if theres no obvious advantage. Some perf tests to deomonstrate the advantage here would be great. > > }) > > > > > > /** > > > @@ -268,7 +268,7 @@ rte_align64pow2(uint64_t v) #define RTE_MAX(a, b) > > ({ \ > > > typeof (a) _a = (a); \ > > > typeof (b) _b = (b); \ > > > - _a > _b ? _a : _b; \ > > > + _a ^ ((_a ^ _b) & -(_a < _b)); \ > > Same as above > > > > Same as above. > > > > }) > > > > > > /*********** Other general functions / macros ********/ diff --git > > > a/lib/librte_pmd_e1000/igb_pf.c b/lib/librte_pmd_e1000/igb_pf.c index > > > bc3816a..546499c 100644 > > > --- a/lib/librte_pmd_e1000/igb_pf.c > > > +++ b/lib/librte_pmd_e1000/igb_pf.c > > > @@ -321,11 +321,11 @@ igb_vf_set_mac_addr(struct rte_eth_dev *dev, > > uint32_t > > > vf, uint32_t *msgbuf) static int igb_vf_set_multicast(struct > > rte_eth_dev > > > *dev, __rte_unused uint32_t vf, uint32_t *msgbuf) { > > > - int i; > > > + int16_t i; > > > uint32_t vector_bit; > > > uint32_t vector_reg; > > > uint32_t mta_reg; > > > - int entries = (msgbuf[0] & E1000_VT_MSGINFO_MASK) >> > > > + int32_t entries = (msgbuf[0] & E1000_VT_MSGINFO_MASK) >> > > > E1000_VT_MSGINFO_SHIFT; > > NAK, this has nothing to do with the included changelog > > > > It does, it causes compilation errors such as > > /root/dpdk-new/dpdk/lib/librte_pmd_e1000/igb_pf.c: In function > \u2018igb_pf_mbx_process\u2019: > /root/dpdk-new/dpdk/lib/librte_pmd_e1000/igb_pf.c:350:23: error: array > subscript is above array bounds [-Werror=array-bounds] > vfinfo->vf_mc_hashes[i] = hash_list[i]; > ^ > cc1: all warnings being treated as errors > > Also it is always better to use explicit int definitions esp. for 64bit > systems. > This is your changelog: ============================================================= Subject: [PATCH] Minor fixes in rte_common.h file. Fix rte_is_power_of_2 since 0 is not. Avoid branching instructions in RTE_MAX and RTE_MIN ============================================================= Nowhere does your changelog indicate that you are fixing compliation errors. That would in and of itself be far more serious that making micro optimizations. If you want to fix build breaks, great, please do, but send a patch that clearly indicates what the break is and how your fixing it. Don't just toss it in with whatever other work you happen to be doing.