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

[Neil Horman <nhorman@tuxdriver.com>]

On Tue, Dec 16, 2014 at 08:46:51AM -0800, Ravi Kerur wrote:
> On Sat, Dec 13, 2014 at 2:39 AM, Neil Horman <nhorman@tuxdriver.com> 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 <rkerur@gmail.com>
> > > ---
> > >  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?
> >
> >
> <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
> 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
> >
> > <rk> 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
> >
> 
> <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.