Re: [PATCH] Minor fixes in rte_common.h file.

From: Ravi Kerur <hidden>
Date: 2014-12-19 13:28:24

On Thu, Dec 18, 2014 at 11:07 AM, Neil Horman [off-list ref] wrote:

On Wed, Dec 17, 2014 at 08:40:17AM -0800, Ravi Kerur wrote:

quoted

On Tue, Dec 16, 2014 at 1:40 PM, Neil Horman [off-list ref]

wrote:

quoted

On Tue, Dec 16, 2014 at 08:46:51AM -0800, Ravi Kerur wrote:

quoted

On Sat, Dec 13, 2014 at 2:39 AM, Neil Horman [off-list ref]

wrote:

quoted

On Fri, Dec 12, 2014 at 03:04:34PM -0800, r k wrote:

quoted

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 <redacted>
---
 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

quoted

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,

quoted

b)

quoted

({ \

quoted

                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

quoted

using
a cmov instead?

<rk> i am pretty sure modified code is faster than branching. I

remember

quoted

cmov had performance issues esp. on Pentuim-4 not sure how new intel

cpu's

quoted

perform.

Pretty sure isn't sure.  Theres no point in code churn if theres no

obvious

quoted

advantage.  Some perf tests to deomonstrate the advantage here would be
great.

<rk> I have used this before with the intent to avoid branching and it

was

quoted

part of other changes I did for performance improvement in our code.

Then it should be pretty easy to provide the perf data demonstrating the
advantage in this code.

<rk> I decided to drop this change because

1. DPDK manual suggests gcc version 4.5.x or greater
2. I was testing code against gcc 4.4.6 (which generated cmp/jump
instructions) and Konstantin showed using gcc 4.8.3 it generates cmov
instructions

If you think it should be pursued further let me know. I can look into
difference between the code generated for both using gcc > 4.5 .x and
update.

Thanks,
Ravi

quoted

})

quoted

/**

@@ -268,7 +268,7 @@ rte_align64pow2(uint64_t v)  #define

RTE_MAX(a,

quoted

b)

quoted

({ \

quoted

                typeof (a) _a = (a); \
                typeof (b) _b = (b); \
-               _a > _b ? _a : _b; \
+               _a ^ ((_a ^ _b) & -(_a < _b)); \

Same as above

<rk> Same as above.

quoted

        })

 /*********** Other general functions / macros ********/ diff

--git

quoted

a/lib/librte_pmd_e1000/igb_pf.c b/lib/librte_pmd_e1000/igb_pf.c

index

quoted

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,

quoted

uint32_t

quoted

vf, uint32_t *msgbuf)  static int  igb_vf_set_multicast(struct

rte_eth_dev

quoted

*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

<rk> 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

quoted

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

quoted

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

quoted

clearly
indicates what the break is and how your fixing it. Don't just toss it

in

quoted

with
whatever other work you happen to be doing.

<rk> Main reason was to replace int with explicit sized int, it happened

to

quoted

fix compiler errors as well. I will make sure comments cover everything
next time. Anyways I will drop this patch and just include fix for
power_of_2.

Please separate the compiler warning fixes from the performance enhancing
fixes.
They shouldn't be mashed together.

Neil

`h`	back out one level
`j`	next message in thread
`k`	previous message in thread
`l`	drill in
`Esc`	close help / fold thread tree
`?`	toggle this help