[v2] net/e1000: update UPDATE_VF_STAT to handle rollover
Checks
Commit Message
Modified UPDATE_VF_STAT to properly handle rollover conditions.
Fixes: d82170d27918 ("igb: add VF support")
Cc: intel.com
Signed-off-by: David Harton <dharton@cisco.com>
---
drivers/net/e1000/igb_ethdev.c | 14 +++++++++-----
1 file changed, 9 insertions(+), 5 deletions(-)
Comments
On 01/26, David Harton wrote:
>Modified UPDATE_VF_STAT to properly handle rollover conditions.
>
>Fixes: d82170d27918 ("igb: add VF support")
>Cc: intel.com
>
>Signed-off-by: David Harton <dharton@cisco.com>
>---
> drivers/net/e1000/igb_ethdev.c | 14 +++++++++-----
> 1 file changed, 9 insertions(+), 5 deletions(-)
>
>diff --git a/drivers/net/e1000/igb_ethdev.c b/drivers/net/e1000/igb_ethdev.c
>index a3e30dbe5..825663267 100644
>--- a/drivers/net/e1000/igb_ethdev.c
>+++ b/drivers/net/e1000/igb_ethdev.c
>@@ -261,11 +261,15 @@ static int igb_filter_restore(struct rte_eth_dev *dev);
> /*
> * Define VF Stats MACRO for Non "cleared on read" register
> */
>-#define UPDATE_VF_STAT(reg, last, cur) \
>-{ \
>- u32 latest = E1000_READ_REG(hw, reg); \
>- cur += (latest - last) & UINT_MAX; \
>- last = latest; \
>+#define UPDATE_VF_STAT(reg, last, cur) \
>+{ \
>+ u32 latest = E1000_READ_REG(hw, reg); \
>+ if (latest >= last) \
>+ cur += (latest - last); \
>+ else \
>+ cur += ((latest + ((uint64_t)1 << 32)) - last); \
>+ cur &= UINT_MAX; \
>+ last = latest; \
> }
>
> #define IGB_FC_PAUSE_TIME 0x0680
>--
>2.19.1
>
Acked-by: Xiaolong Ye <xiaolong.ye@intel.com>
Applied to dpdk-next-net-intel, Thanks.
On 1/26/2020 5:25 PM, David Harton wrote:
> Modified UPDATE_VF_STAT to properly handle rollover conditions.
>
> Fixes: d82170d27918 ("igb: add VF support")
> Cc: intel.com
>
> Signed-off-by: David Harton <dharton@cisco.com>
> ---
> drivers/net/e1000/igb_ethdev.c | 14 +++++++++-----
> 1 file changed, 9 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/net/e1000/igb_ethdev.c b/drivers/net/e1000/igb_ethdev.c
> index a3e30dbe5..825663267 100644
> --- a/drivers/net/e1000/igb_ethdev.c
> +++ b/drivers/net/e1000/igb_ethdev.c
> @@ -261,11 +261,15 @@ static int igb_filter_restore(struct rte_eth_dev *dev);
> /*
> * Define VF Stats MACRO for Non "cleared on read" register
> */
> -#define UPDATE_VF_STAT(reg, last, cur) \
> -{ \
> - u32 latest = E1000_READ_REG(hw, reg); \
> - cur += (latest - last) & UINT_MAX; \
Why this is wrong? Both 'latest' and 'last' are 'u32', so diff should be correct
'u32' value. And it is added to 'u64' 'cur' value. What I am missing?
> - last = latest; \
> +#define UPDATE_VF_STAT(reg, last, cur) \
> +{ \
> + u32 latest = E1000_READ_REG(hw, reg); \
> + if (latest >= last) \
> + cur += (latest - last); \
> + else \
> + cur += ((latest + ((uint64_t)1 << 32)) - last); \
> + cur &= UINT_MAX; \
Why & with UINT_MAX, won't this limit the value to 32bits which has 64bit storage?
> + last = latest; \
> }
>
> #define IGB_FC_PAUSE_TIME 0x0680
>
> -----Original Message-----
> From: Ferruh Yigit <ferruh.yigit@intel.com>
> Sent: Wednesday, January 29, 2020 5:10 AM
> To: David Harton (dharton) <dharton@cisco.com>; dev@dpdk.org
> Cc: wenzhuo.lu@intel.com; konstantin.ananyev@intel.com;
> xiaolong.ye@intel.com; intel.com@cisco.com
> Subject: Re: [dpdk-dev] [PATCH v2] net/e1000: update UPDATE_VF_STAT to
> handle rollover
>
> On 1/26/2020 5:25 PM, David Harton wrote:
> > Modified UPDATE_VF_STAT to properly handle rollover conditions.
> >
> > Fixes: d82170d27918 ("igb: add VF support")
> > Cc: intel.com
> >
> > Signed-off-by: David Harton <dharton@cisco.com>
> > ---
> > drivers/net/e1000/igb_ethdev.c | 14 +++++++++-----
> > 1 file changed, 9 insertions(+), 5 deletions(-)
> >
> > diff --git a/drivers/net/e1000/igb_ethdev.c
> > b/drivers/net/e1000/igb_ethdev.c index a3e30dbe5..825663267 100644
> > --- a/drivers/net/e1000/igb_ethdev.c
> > +++ b/drivers/net/e1000/igb_ethdev.c
> > @@ -261,11 +261,15 @@ static int igb_filter_restore(struct rte_eth_dev
> > *dev);
> > /*
> > * Define VF Stats MACRO for Non "cleared on read" register
> > */
> > -#define UPDATE_VF_STAT(reg, last, cur) \
> > -{ \
> > - u32 latest = E1000_READ_REG(hw, reg); \
> > - cur += (latest - last) & UINT_MAX; \
>
> Why this is wrong? Both 'latest' and 'last' are 'u32', so diff should be
> correct 'u32' value. And it is added to 'u64' 'cur' value. What I am
> missing?
>
> > - last = latest; \
> > +#define UPDATE_VF_STAT(reg, last, cur) \
> > +{ \
> > + u32 latest = E1000_READ_REG(hw, reg); \
> > + if (latest >= last) \
> > + cur += (latest - last); \
> > + else \
> > + cur += ((latest + ((uint64_t)1 << 32)) - last); \
> > + cur &= UINT_MAX; \
>
> Why & with UINT_MAX, won't this limit the value to 32bits which has 64bit
> storage?
I'm embarrassed. I was upstreaming this on behalf of another and honestly didn't even look. :(
You are right about the '&='. In fact, I'm not convinced these diffs are necessary and have asked our local developer to verify why this change and the ixgbevf change are needed. I'm wondering if they were encountering another issue related to sync that cause pkt counts to get out of sync as they were attempting to fix what looked like a rollover issue (huge packet counts after boot).
Sorry,
Dave
>
> > + last = latest; \
> > }
> >
> > #define IGB_FC_PAUSE_TIME 0x0680
> >
On 01/29, Ferruh Yigit wrote:
>On 1/26/2020 5:25 PM, David Harton wrote:
>> Modified UPDATE_VF_STAT to properly handle rollover conditions.
>>
>> Fixes: d82170d27918 ("igb: add VF support")
>> Cc: intel.com
>>
>> Signed-off-by: David Harton <dharton@cisco.com>
>> ---
>> drivers/net/e1000/igb_ethdev.c | 14 +++++++++-----
>> 1 file changed, 9 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/net/e1000/igb_ethdev.c b/drivers/net/e1000/igb_ethdev.c
>> index a3e30dbe5..825663267 100644
>> --- a/drivers/net/e1000/igb_ethdev.c
>> +++ b/drivers/net/e1000/igb_ethdev.c
>> @@ -261,11 +261,15 @@ static int igb_filter_restore(struct rte_eth_dev *dev);
>> /*
>> * Define VF Stats MACRO for Non "cleared on read" register
>> */
>> -#define UPDATE_VF_STAT(reg, last, cur) \
>> -{ \
>> - u32 latest = E1000_READ_REG(hw, reg); \
>> - cur += (latest - last) & UINT_MAX; \
>
>Why this is wrong? Both 'latest' and 'last' are 'u32', so diff should be correct
>'u32' value. And it is added to 'u64' 'cur' value. What I am missing?
Per my understanding, stat value read from HW reg would be rolled over after
it reaches its maximum, so we need to handle both normal case (latest >= last)
and rollover case (latest < last) here. Wait for the original author for more
explanation.
>
>> - last = latest; \
>> +#define UPDATE_VF_STAT(reg, last, cur) \
>> +{ \
>> + u32 latest = E1000_READ_REG(hw, reg); \
>> + if (latest >= last) \
>> + cur += (latest - last); \
>> + else \
>> + cur += ((latest + ((uint64_t)1 << 32)) - last); \
>> + cur &= UINT_MAX; \
>
>Why & with UINT_MAX, won't this limit the value to 32bits which has 64bit storage?
>
Agree & with UINT_MAX should be removed here.
Thanks,
Xiaolong
>> + last = latest; \
>> }
>>
>> #define IGB_FC_PAUSE_TIME 0x0680
>>
>
@@ -261,11 +261,15 @@ static int igb_filter_restore(struct rte_eth_dev *dev);
/*
* Define VF Stats MACRO for Non "cleared on read" register
*/
-#define UPDATE_VF_STAT(reg, last, cur) \
-{ \
- u32 latest = E1000_READ_REG(hw, reg); \
- cur += (latest - last) & UINT_MAX; \
- last = latest; \
+#define UPDATE_VF_STAT(reg, last, cur) \
+{ \
+ u32 latest = E1000_READ_REG(hw, reg); \
+ if (latest >= last) \
+ cur += (latest - last); \
+ else \
+ cur += ((latest + ((uint64_t)1 << 32)) - last); \
+ cur &= UINT_MAX; \
+ last = latest; \
}
#define IGB_FC_PAUSE_TIME 0x0680