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

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

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

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

quoted

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,

b)

quoted

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

quoted

using
a cmov instead?

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

quoted

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.

<rk> I have used this before with the intent to avoid branching and it was
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.

quoted

quoted

quoted

        })

quoted

 /**

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

b)

quoted

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

quoted

        })

 /*********** 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

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

<rk> Main reason was to replace int with explicit sized int, it happened to
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