* [dpdk-dev] [PATCH] Minor fixes in rte_common.h file. @ 2014-12-12 23:03 r k 0 siblings, 0 replies; 12+ messages in thread From: r k @ 2014-12-12 23:03 UTC (permalink / raw) To: dev Ravi Kerur (1): Minor fixes in rte_common.h file. 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(-) -- 1.9.1 ^ permalink raw reply [flat|nested] 12+ messages in thread
* [dpdk-dev] [PATCH] Minor fixes in rte_common.h file. @ 2014-12-12 23:04 r k 2014-12-13 10:39 ` Neil Horman 0 siblings, 1 reply; 12+ messages in thread From: r k @ 2014-12-12 23:04 UTC (permalink / raw) To: dev 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)); \ }) /** @@ -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)); \ }) /*********** 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; uint16_t *hash_list = (uint16_t *)&msgbuf[1]; struct e1000_hw *hw = E1000_DEV_PRIVATE_TO_HW(dev->data->dev_private); diff --git a/lib/librte_pmd_ixgbe/ixgbe_pf.c b/lib/librte_pmd_ixgbe/ixgbe_pf.c index 51da1fd..426caf9 100644 --- a/lib/librte_pmd_ixgbe/ixgbe_pf.c +++ b/lib/librte_pmd_ixgbe/ixgbe_pf.c @@ -390,7 +390,7 @@ ixgbe_vf_set_multicast(struct rte_eth_dev *dev, __rte_unused uint32_t vf, uint32 struct ixgbe_hw *hw = IXGBE_DEV_PRIVATE_TO_HW(dev->data->dev_private); struct ixgbe_vf_info *vfinfo = *(IXGBE_DEV_PRIVATE_TO_P_VFDATA(dev->data->dev_private)); - int nb_entries = (msgbuf[0] & IXGBE_VT_MSGINFO_MASK) >> + int32_t nb_entries = (msgbuf[0] & IXGBE_VT_MSGINFO_MASK) >> IXGBE_VT_MSGINFO_SHIFT; uint16_t *hash_list = (uint16_t *)&msgbuf[1]; uint32_t mta_idx; @@ -399,7 +399,7 @@ ixgbe_vf_set_multicast(struct rte_eth_dev *dev, __rte_unused uint32_t vf, uint32 const uint32_t IXGBE_MTA_BIT_SHIFT = 5; const uint32_t IXGBE_MTA_BIT_MASK = (0x1 << IXGBE_MTA_BIT_SHIFT) - 1; uint32_t reg_val; - int i; + int16_t i; /* only so many hash values supported */ nb_entries = RTE_MIN(nb_entries, IXGBE_MAX_VF_MC_ENTRIES); -- 1.9.1 ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [dpdk-dev] [PATCH] Minor fixes in rte_common.h file. 2014-12-12 23:04 r k @ 2014-12-13 10:39 ` Neil Horman 2014-12-16 16:46 ` Ravi Kerur 0 siblings, 1 reply; 12+ messages in thread From: Neil Horman @ 2014-12-13 10:39 UTC (permalink / raw) To: r k; +Cc: dev 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? > }) > > /** > @@ -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 > }) > > /*********** 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 > uint16_t *hash_list = (uint16_t *)&msgbuf[1]; > struct e1000_hw *hw = > E1000_DEV_PRIVATE_TO_HW(dev->data->dev_private); > diff --git a/lib/librte_pmd_ixgbe/ixgbe_pf.c > b/lib/librte_pmd_ixgbe/ixgbe_pf.c index 51da1fd..426caf9 100644 > --- a/lib/librte_pmd_ixgbe/ixgbe_pf.c > +++ b/lib/librte_pmd_ixgbe/ixgbe_pf.c > @@ -390,7 +390,7 @@ ixgbe_vf_set_multicast(struct rte_eth_dev *dev, > __rte_unused uint32_t vf, uint32 > struct ixgbe_hw *hw = > IXGBE_DEV_PRIVATE_TO_HW(dev->data->dev_private); > struct ixgbe_vf_info *vfinfo = > *(IXGBE_DEV_PRIVATE_TO_P_VFDATA(dev->data->dev_private)); > - int nb_entries = (msgbuf[0] & IXGBE_VT_MSGINFO_MASK) >> > + int32_t nb_entries = (msgbuf[0] & IXGBE_VT_MSGINFO_MASK) >> > IXGBE_VT_MSGINFO_SHIFT; ditto > uint16_t *hash_list = (uint16_t *)&msgbuf[1]; > uint32_t mta_idx; > @@ -399,7 +399,7 @@ ixgbe_vf_set_multicast(struct rte_eth_dev *dev, > __rte_unused uint32_t vf, uint32 > const uint32_t IXGBE_MTA_BIT_SHIFT = 5; > const uint32_t IXGBE_MTA_BIT_MASK = (0x1 << IXGBE_MTA_BIT_SHIFT) - > 1; > uint32_t reg_val; > - int i; > + int16_t i; ditto > > /* only so many hash values supported */ > nb_entries = RTE_MIN(nb_entries, IXGBE_MAX_VF_MC_ENTRIES); > -- > 1.9.1 > ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [dpdk-dev] [PATCH] Minor fixes in rte_common.h file. 2014-12-13 10:39 ` Neil Horman @ 2014-12-16 16:46 ` Ravi Kerur 2014-12-16 17:23 ` Ananyev, Konstantin 2014-12-16 21:40 ` Neil Horman 0 siblings, 2 replies; 12+ messages in thread From: Ravi Kerur @ 2014-12-16 16:46 UTC (permalink / raw) To: Neil Horman; +Cc: dev 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. > }) > > > > /** > > @@ -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. > > > uint16_t *hash_list = (uint16_t *)&msgbuf[1]; > > struct e1000_hw *hw = > > E1000_DEV_PRIVATE_TO_HW(dev->data->dev_private); > > diff --git a/lib/librte_pmd_ixgbe/ixgbe_pf.c > > b/lib/librte_pmd_ixgbe/ixgbe_pf.c index 51da1fd..426caf9 100644 > > --- a/lib/librte_pmd_ixgbe/ixgbe_pf.c > > +++ b/lib/librte_pmd_ixgbe/ixgbe_pf.c > > @@ -390,7 +390,7 @@ ixgbe_vf_set_multicast(struct rte_eth_dev *dev, > > __rte_unused uint32_t vf, uint32 > > struct ixgbe_hw *hw = > > IXGBE_DEV_PRIVATE_TO_HW(dev->data->dev_private); > > struct ixgbe_vf_info *vfinfo = > > *(IXGBE_DEV_PRIVATE_TO_P_VFDATA(dev->data->dev_private)); > > - int nb_entries = (msgbuf[0] & IXGBE_VT_MSGINFO_MASK) >> > > + int32_t nb_entries = (msgbuf[0] & IXGBE_VT_MSGINFO_MASK) >> > > IXGBE_VT_MSGINFO_SHIFT; > ditto > > uint16_t *hash_list = (uint16_t *)&msgbuf[1]; > > uint32_t mta_idx; > > @@ -399,7 +399,7 @@ ixgbe_vf_set_multicast(struct rte_eth_dev *dev, > > __rte_unused uint32_t vf, uint32 > > const uint32_t IXGBE_MTA_BIT_SHIFT = 5; > > const uint32_t IXGBE_MTA_BIT_MASK = (0x1 << IXGBE_MTA_BIT_SHIFT) > - > > 1; > > uint32_t reg_val; > > - int i; > > + int16_t i; > ditto > > <rk> Same as above. > > > > /* only so many hash values supported */ > > nb_entries = RTE_MIN(nb_entries, IXGBE_MAX_VF_MC_ENTRIES); > > -- > > 1.9.1 > > > ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [dpdk-dev] [PATCH] Minor fixes in rte_common.h file. 2014-12-16 16:46 ` Ravi Kerur @ 2014-12-16 17:23 ` Ananyev, Konstantin 2014-12-16 20:13 ` Ravi Kerur 2014-12-16 21:40 ` Neil Horman 1 sibling, 1 reply; 12+ messages in thread From: Ananyev, Konstantin @ 2014-12-16 17:23 UTC (permalink / raw) To: Ravi Kerur, Neil Horman; +Cc: dev > -----Original Message----- > From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Ravi Kerur > Sent: Tuesday, December 16, 2014 4:47 PM > To: Neil Horman > Cc: dev@dpdk.org > Subject: Re: [dpdk-dev] [PATCH] Minor fixes in rte_common.h file. > > 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. I also think most modern compilers are smart enough to avoid any branching here and will use cmov instead. And we are way ahead of Pentium 4 times these days. Konstantin > > > }) > > > > > > /** > > > @@ -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. > > > > > > > > uint16_t *hash_list = (uint16_t *)&msgbuf[1]; > > > struct e1000_hw *hw = > > > E1000_DEV_PRIVATE_TO_HW(dev->data->dev_private); > > > diff --git a/lib/librte_pmd_ixgbe/ixgbe_pf.c > > > b/lib/librte_pmd_ixgbe/ixgbe_pf.c index 51da1fd..426caf9 100644 > > > --- a/lib/librte_pmd_ixgbe/ixgbe_pf.c > > > +++ b/lib/librte_pmd_ixgbe/ixgbe_pf.c > > > @@ -390,7 +390,7 @@ ixgbe_vf_set_multicast(struct rte_eth_dev *dev, > > > __rte_unused uint32_t vf, uint32 > > > struct ixgbe_hw *hw = > > > IXGBE_DEV_PRIVATE_TO_HW(dev->data->dev_private); > > > struct ixgbe_vf_info *vfinfo = > > > *(IXGBE_DEV_PRIVATE_TO_P_VFDATA(dev->data->dev_private)); > > > - int nb_entries = (msgbuf[0] & IXGBE_VT_MSGINFO_MASK) >> > > > + int32_t nb_entries = (msgbuf[0] & IXGBE_VT_MSGINFO_MASK) >> > > > IXGBE_VT_MSGINFO_SHIFT; > > ditto > > > uint16_t *hash_list = (uint16_t *)&msgbuf[1]; > > > uint32_t mta_idx; > > > @@ -399,7 +399,7 @@ ixgbe_vf_set_multicast(struct rte_eth_dev *dev, > > > __rte_unused uint32_t vf, uint32 > > > const uint32_t IXGBE_MTA_BIT_SHIFT = 5; > > > const uint32_t IXGBE_MTA_BIT_MASK = (0x1 << IXGBE_MTA_BIT_SHIFT) > > - > > > 1; > > > uint32_t reg_val; > > > - int i; > > > + int16_t i; > > ditto > > > > <rk> Same as above. > > > > > > > /* only so many hash values supported */ > > > nb_entries = RTE_MIN(nb_entries, IXGBE_MAX_VF_MC_ENTRIES); > > > -- > > > 1.9.1 > > > > > ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [dpdk-dev] [PATCH] Minor fixes in rte_common.h file. 2014-12-16 17:23 ` Ananyev, Konstantin @ 2014-12-16 20:13 ` Ravi Kerur [not found] ` <2601191342CEEE43887BDE71AB977258213C1499@IRSMSX105.ger.corp.intel.com> 0 siblings, 1 reply; 12+ messages in thread From: Ravi Kerur @ 2014-12-16 20:13 UTC (permalink / raw) To: Ananyev, Konstantin; +Cc: dev On Tue, Dec 16, 2014 at 9:23 AM, Ananyev, Konstantin < konstantin.ananyev@intel.com> wrote: > > > > > -----Original Message----- > > From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Ravi Kerur > > Sent: Tuesday, December 16, 2014 4:47 PM > > To: Neil Horman > > Cc: dev@dpdk.org > > Subject: Re: [dpdk-dev] [PATCH] Minor fixes in rte_common.h file. > > > > 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. > > I also think most modern compilers are smart enough to avoid any branching > here and will use cmov instead. > And we are way ahead of Pentium 4 times these days. Konstantin > <rk>Konstantin, Can you please elaborate, is it something done automatically with Intel's icc compiler? My understanding is branch prediction can be influenced only by using compiler builtin i.e. __builtin_expect() , without this compiler will generate regular instructions(cmp/jump instructions). I wrote small program and compiled with gcc -02/-03, don't see cmov instruction. > > > > > }) > > > > > > > > /** > > > > @@ -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. > > > > > > > > > > > > > uint16_t *hash_list = (uint16_t *)&msgbuf[1]; > > > > struct e1000_hw *hw = > > > > E1000_DEV_PRIVATE_TO_HW(dev->data->dev_private); > > > > diff --git a/lib/librte_pmd_ixgbe/ixgbe_pf.c > > > > b/lib/librte_pmd_ixgbe/ixgbe_pf.c index 51da1fd..426caf9 100644 > > > > --- a/lib/librte_pmd_ixgbe/ixgbe_pf.c > > > > +++ b/lib/librte_pmd_ixgbe/ixgbe_pf.c > > > > @@ -390,7 +390,7 @@ ixgbe_vf_set_multicast(struct rte_eth_dev *dev, > > > > __rte_unused uint32_t vf, uint32 > > > > struct ixgbe_hw *hw = > > > > IXGBE_DEV_PRIVATE_TO_HW(dev->data->dev_private); > > > > struct ixgbe_vf_info *vfinfo = > > > > > *(IXGBE_DEV_PRIVATE_TO_P_VFDATA(dev->data->dev_private)); > > > > - int nb_entries = (msgbuf[0] & IXGBE_VT_MSGINFO_MASK) >> > > > > + int32_t nb_entries = (msgbuf[0] & IXGBE_VT_MSGINFO_MASK) >> > > > > IXGBE_VT_MSGINFO_SHIFT; > > > ditto > > > > uint16_t *hash_list = (uint16_t *)&msgbuf[1]; > > > > uint32_t mta_idx; > > > > @@ -399,7 +399,7 @@ ixgbe_vf_set_multicast(struct rte_eth_dev *dev, > > > > __rte_unused uint32_t vf, uint32 > > > > const uint32_t IXGBE_MTA_BIT_SHIFT = 5; > > > > const uint32_t IXGBE_MTA_BIT_MASK = (0x1 << > IXGBE_MTA_BIT_SHIFT) > > > - > > > > 1; > > > > uint32_t reg_val; > > > > - int i; > > > > + int16_t i; > > > ditto > > > > > > <rk> Same as above. > > > > > > > > > > /* only so many hash values supported */ > > > > nb_entries = RTE_MIN(nb_entries, IXGBE_MAX_VF_MC_ENTRIES); > > > > -- > > > > 1.9.1 > > > > > > > > ^ permalink raw reply [flat|nested] 12+ messages in thread
[parent not found: <2601191342CEEE43887BDE71AB977258213C1499@IRSMSX105.ger.corp.intel.com>]
* Re: [dpdk-dev] [PATCH] Minor fixes in rte_common.h file. [not found] ` <2601191342CEEE43887BDE71AB977258213C1499@IRSMSX105.ger.corp.intel.com> @ 2014-12-17 1:05 ` Ananyev, Konstantin 2014-12-17 16:28 ` Ravi Kerur 0 siblings, 1 reply; 12+ messages in thread From: Ananyev, Konstantin @ 2014-12-17 1:05 UTC (permalink / raw) To: dev > > From: Ravi Kerur [mailto:rkerur@gmail.com] > Sent: Tuesday, December 16, 2014 8:14 PM > To: Ananyev, Konstantin > Cc: Neil Horman; dev@dpdk.org > Subject: Re: [dpdk-dev] [PATCH] Minor fixes in rte_common.h file. > > > > On Tue, Dec 16, 2014 at 9:23 AM, Ananyev, Konstantin <konstantin.ananyev@intel.com> wrote: > > > > -----Original Message----- > > From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Ravi Kerur > > Sent: Tuesday, December 16, 2014 4:47 PM > > To: Neil Horman > > Cc: dev@dpdk.org > > Subject: Re: [dpdk-dev] [PATCH] Minor fixes in rte_common.h file. > > > > 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. > I also think most modern compilers are smart enough to avoid any branching here and will use cmov instead. > And we are way ahead of Pentium 4 times these days. > Konstantin > > <rk>Konstantin, Can you please elaborate, is it something done automatically with Intel's icc compiler? My understanding is branch > prediction can be influenced only by using compiler builtin i.e. __builtin_expect() , without this compiler will generate regular > instructions(cmp/jump instructions). I wrote small program and compiled with gcc -02/-03, don't see cmov instruction. I am saying that there is probably no need to modify these macros. On IA , for constructions like: "_a < _b ? _a : _b;" modern compilers in many cases will avoid any branches and emit cmov instead. $ cat tcmv1.c #include <stdint.h> #include <stddef.h> #define RTE_MIN(a, b) ({ \ typeof (a) _a = (a); \ typeof (b) _b = (b); \ _a < _b ? _a : _b; \ }) int fxmini32(int a, int b) { return RTE_MIN(a, b); } int fxminu64(uint64_t a, uint64_t b) { return RTE_MIN(a, b); } $gcc -O3 -m64 -S tcmv1.c $ cat tcmv1.s .file "tcmv1.c" .text .p2align 4,,15 .globl fxmini32 .type fxmini32, @function fxmini32: .LFB0: .cfi_startproc cmpl %esi, %edi movl %esi, %eax cmovle %edi, %eax ret .cfi_endproc .LFE0: .size fxmini32, .-fxmini32 .p2align 4,,15 .globl fxminu64 .type fxminu64, @function fxminu64: .LFB1: .cfi_startproc cmpq %rsi, %rdi movq %rsi, %rax cmovbe %rdi, %rax ret .cfi_endproc gcc version 4.8.3 clang produces similar code. Konstantin > > > > > > }) > > > > > > > > /** > > > > @@ -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. > > > > > > > > > > > > > uint16_t *hash_list = (uint16_t *)&msgbuf[1]; > > > > struct e1000_hw *hw = > > > > E1000_DEV_PRIVATE_TO_HW(dev->data->dev_private); > > > > diff --git a/lib/librte_pmd_ixgbe/ixgbe_pf.c > > > > b/lib/librte_pmd_ixgbe/ixgbe_pf.c index 51da1fd..426caf9 100644 > > > > --- a/lib/librte_pmd_ixgbe/ixgbe_pf.c > > > > +++ b/lib/librte_pmd_ixgbe/ixgbe_pf.c > > > > @@ -390,7 +390,7 @@ ixgbe_vf_set_multicast(struct rte_eth_dev *dev, > > > > __rte_unused uint32_t vf, uint32 > > > > struct ixgbe_hw *hw = > > > > IXGBE_DEV_PRIVATE_TO_HW(dev->data->dev_private); > > > > struct ixgbe_vf_info *vfinfo = > > > > *(IXGBE_DEV_PRIVATE_TO_P_VFDATA(dev->data->dev_private)); > > > > - int nb_entries = (msgbuf[0] & IXGBE_VT_MSGINFO_MASK) >> > > > > + int32_t nb_entries = (msgbuf[0] & IXGBE_VT_MSGINFO_MASK) >> > > > > IXGBE_VT_MSGINFO_SHIFT; > > > ditto > > > > uint16_t *hash_list = (uint16_t *)&msgbuf[1]; > > > > uint32_t mta_idx; > > > > @@ -399,7 +399,7 @@ ixgbe_vf_set_multicast(struct rte_eth_dev *dev, > > > > __rte_unused uint32_t vf, uint32 > > > > const uint32_t IXGBE_MTA_BIT_SHIFT = 5; > > > > const uint32_t IXGBE_MTA_BIT_MASK = (0x1 << IXGBE_MTA_BIT_SHIFT) > > > - > > > > 1; > > > > uint32_t reg_val; > > > > - int i; > > > > + int16_t i; > > > ditto > > > > > > <rk> Same as above. > > > > > > > > > > /* only so many hash values supported */ > > > > nb_entries = RTE_MIN(nb_entries, IXGBE_MAX_VF_MC_ENTRIES); > > > > -- > > > > 1.9.1 > > > > > > > ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [dpdk-dev] [PATCH] Minor fixes in rte_common.h file. 2014-12-17 1:05 ` Ananyev, Konstantin @ 2014-12-17 16:28 ` Ravi Kerur 0 siblings, 0 replies; 12+ messages in thread From: Ravi Kerur @ 2014-12-17 16:28 UTC (permalink / raw) To: Ananyev, Konstantin; +Cc: dev On Tue, Dec 16, 2014 at 5:05 PM, Ananyev, Konstantin < konstantin.ananyev@intel.com> wrote: > > > > > > > From: Ravi Kerur [mailto:rkerur@gmail.com] > > Sent: Tuesday, December 16, 2014 8:14 PM > > To: Ananyev, Konstantin > > Cc: Neil Horman; dev@dpdk.org > > Subject: Re: [dpdk-dev] [PATCH] Minor fixes in rte_common.h file. > > > > > > > > On Tue, Dec 16, 2014 at 9:23 AM, Ananyev, Konstantin < > konstantin.ananyev@intel.com> wrote: > > > > > > > -----Original Message----- > > > From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Ravi Kerur > > > Sent: Tuesday, December 16, 2014 4:47 PM > > > To: Neil Horman > > > Cc: dev@dpdk.org > > > Subject: Re: [dpdk-dev] [PATCH] Minor fixes in rte_common.h file. > > > > > > 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. > > I also think most modern compilers are smart enough to avoid any > branching here and will use cmov instead. > > And we are way ahead of Pentium 4 times these days. > > Konstantin > > > > <rk>Konstantin, Can you please elaborate, is it something done > automatically with Intel's icc compiler? My understanding is branch > > prediction can be influenced only by using compiler builtin i.e. > __builtin_expect() , without this compiler will generate regular > > instructions(cmp/jump instructions). I wrote small program and compiled > with gcc -02/-03, don't see cmov instruction. > > I am saying that there is probably no need to modify these macros. > On IA , for constructions like: "_a < _b ? _a : _b;" > modern compilers in many cases will avoid any branches and emit cmov > instead. > > $ cat tcmv1.c > > #include <stdint.h> > #include <stddef.h> > > #define RTE_MIN(a, b) ({ \ > typeof (a) _a = (a); \ > typeof (b) _b = (b); \ > _a < _b ? _a : _b; \ > }) > > int > fxmini32(int a, int b) > { > return RTE_MIN(a, b); > } > > int > fxminu64(uint64_t a, uint64_t b) > { > return RTE_MIN(a, b); > } > > $gcc -O3 -m64 -S tcmv1.c > > $ cat tcmv1.s > .file "tcmv1.c" > .text > .p2align 4,,15 > .globl fxmini32 > .type fxmini32, @function > fxmini32: > .LFB0: > .cfi_startproc > cmpl %esi, %edi > movl %esi, %eax > cmovle %edi, %eax > ret > .cfi_endproc > .LFE0: > .size fxmini32, .-fxmini32 > .p2align 4,,15 > .globl fxminu64 > .type fxminu64, @function > fxminu64: > .LFB1: > .cfi_startproc > cmpq %rsi, %rdi > movq %rsi, %rax > cmovbe %rdi, %rax > ret > .cfi_endproc > > gcc version 4.8.3 > clang produces similar code. > > Konstantin > <rk> Thanks, looks like it depends on gcc version. I have .ident "GCC: (GNU) 4.4.6 20110731 (Red Hat 4.4.6-3)" .section .note.GNU-stack,"",@progbits ... and it still generates cmp/jump instructions. Anyways i will drop this patch and just send fix for power_of_2. -Ravi > > > > > > > > > }) > > > > > > > > > > /** > > > > > @@ -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. > > > > > > > > > > > > > > > > > > uint16_t *hash_list = (uint16_t *)&msgbuf[1]; > > > > > struct e1000_hw *hw = > > > > > E1000_DEV_PRIVATE_TO_HW(dev->data->dev_private); > > > > > diff --git a/lib/librte_pmd_ixgbe/ixgbe_pf.c > > > > > b/lib/librte_pmd_ixgbe/ixgbe_pf.c index 51da1fd..426caf9 100644 > > > > > --- a/lib/librte_pmd_ixgbe/ixgbe_pf.c > > > > > +++ b/lib/librte_pmd_ixgbe/ixgbe_pf.c > > > > > @@ -390,7 +390,7 @@ ixgbe_vf_set_multicast(struct rte_eth_dev *dev, > > > > > __rte_unused uint32_t vf, uint32 > > > > > struct ixgbe_hw *hw = > > > > > IXGBE_DEV_PRIVATE_TO_HW(dev->data->dev_private); > > > > > struct ixgbe_vf_info *vfinfo = > > > > > > *(IXGBE_DEV_PRIVATE_TO_P_VFDATA(dev->data->dev_private)); > > > > > - int nb_entries = (msgbuf[0] & IXGBE_VT_MSGINFO_MASK) >> > > > > > + int32_t nb_entries = (msgbuf[0] & IXGBE_VT_MSGINFO_MASK) >> > > > > > IXGBE_VT_MSGINFO_SHIFT; > > > > ditto > > > > > uint16_t *hash_list = (uint16_t *)&msgbuf[1]; > > > > > uint32_t mta_idx; > > > > > @@ -399,7 +399,7 @@ ixgbe_vf_set_multicast(struct rte_eth_dev *dev, > > > > > __rte_unused uint32_t vf, uint32 > > > > > const uint32_t IXGBE_MTA_BIT_SHIFT = 5; > > > > > const uint32_t IXGBE_MTA_BIT_MASK = (0x1 << > IXGBE_MTA_BIT_SHIFT) > > > > - > > > > > 1; > > > > > uint32_t reg_val; > > > > > - int i; > > > > > + int16_t i; > > > > ditto > > > > > > > > <rk> Same as above. > > > > > > > > > > > > > /* only so many hash values supported */ > > > > > nb_entries = RTE_MIN(nb_entries, IXGBE_MAX_VF_MC_ENTRIES); > > > > > -- > > > > > 1.9.1 > > > > > > > > > > ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [dpdk-dev] [PATCH] Minor fixes in rte_common.h file. 2014-12-16 16:46 ` Ravi Kerur 2014-12-16 17:23 ` Ananyev, Konstantin @ 2014-12-16 21:40 ` Neil Horman 2014-12-17 16:40 ` Ravi Kerur 1 sibling, 1 reply; 12+ messages in thread From: Neil Horman @ 2014-12-16 21:40 UTC (permalink / raw) To: Ravi Kerur; +Cc: dev 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. ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [dpdk-dev] [PATCH] Minor fixes in rte_common.h file. 2014-12-16 21:40 ` Neil Horman @ 2014-12-17 16:40 ` Ravi Kerur 2014-12-18 19:07 ` Neil Horman 0 siblings, 1 reply; 12+ messages in thread From: Ravi Kerur @ 2014-12-17 16:40 UTC (permalink / raw) To: Neil Horman; +Cc: dev On Tue, Dec 16, 2014 at 1:40 PM, Neil Horman <nhorman@tuxdriver.com> wrote: > > 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. > <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. > > > > }) > > > > > > > > /** > > > > @@ -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. > <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. ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [dpdk-dev] [PATCH] Minor fixes in rte_common.h file. 2014-12-17 16:40 ` Ravi Kerur @ 2014-12-18 19:07 ` Neil Horman 2014-12-19 13:28 ` Ravi Kerur 0 siblings, 1 reply; 12+ messages in thread From: Neil Horman @ 2014-12-18 19:07 UTC (permalink / raw) To: Ravi Kerur; +Cc: dev On Wed, Dec 17, 2014 at 08:40:17AM -0800, Ravi Kerur wrote: > On Tue, Dec 16, 2014 at 1:40 PM, Neil Horman <nhorman@tuxdriver.com> wrote: > > > > 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. > > > > <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. > > > > > > }) > > > > > > > > > > /** > > > > > @@ -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. > > > > <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 ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [dpdk-dev] [PATCH] Minor fixes in rte_common.h file. 2014-12-18 19:07 ` Neil Horman @ 2014-12-19 13:28 ` Ravi Kerur 0 siblings, 0 replies; 12+ messages in thread From: Ravi Kerur @ 2014-12-19 13:28 UTC (permalink / raw) To: Neil Horman; +Cc: dev On Thu, Dec 18, 2014 at 11:07 AM, Neil Horman <nhorman@tuxdriver.com> wrote: > On Wed, Dec 17, 2014 at 08:40:17AM -0800, Ravi Kerur wrote: > > On Tue, Dec 16, 2014 at 1:40 PM, Neil Horman <nhorman@tuxdriver.com> > wrote: > > > > > > 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. > > > > > > > <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. > > <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 > > > > > > > > }) > > > > > > > > > > > > /** > > > > > > @@ -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. > > > > > > > <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 > > ^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2014-12-19 13:28 UTC | newest]
Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2014-12-12 23:03 [dpdk-dev] [PATCH] Minor fixes in rte_common.h file r k
2014-12-12 23:04 r k
2014-12-13 10:39 ` Neil Horman
2014-12-16 16:46 ` Ravi Kerur
2014-12-16 17:23 ` Ananyev, Konstantin
2014-12-16 20:13 ` Ravi Kerur
[not found] ` <2601191342CEEE43887BDE71AB977258213C1499@IRSMSX105.ger.corp.intel.com>
2014-12-17 1:05 ` Ananyev, Konstantin
2014-12-17 16:28 ` Ravi Kerur
2014-12-16 21:40 ` Neil Horman
2014-12-17 16:40 ` Ravi Kerur
2014-12-18 19:07 ` Neil Horman
2014-12-19 13:28 ` Ravi Kerur
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).