* [dpdk-dev] [PATCH] net/e1000: update UPDATE_VF_STAT to handle rollover
@ 2019-12-11 2:48 David Harton
2020-01-26 9:41 ` Ye Xiaolong
2020-01-26 17:25 ` [dpdk-dev] [PATCH v2] " David Harton
0 siblings, 2 replies; 7+ messages in thread
From: David Harton @ 2019-12-11 2:48 UTC (permalink / raw)
To: dev, wenzhuo.lu; +Cc: David Harton
Modified UPDATE_VF_STAT to properly handle rollover conditions.
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
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [dpdk-dev] [PATCH] net/e1000: update UPDATE_VF_STAT to handle rollover
2019-12-11 2:48 [dpdk-dev] [PATCH] net/e1000: update UPDATE_VF_STAT to handle rollover David Harton
@ 2020-01-26 9:41 ` Ye Xiaolong
2020-01-26 17:25 ` [dpdk-dev] [PATCH v2] " David Harton
1 sibling, 0 replies; 7+ messages in thread
From: Ye Xiaolong @ 2020-01-26 9:41 UTC (permalink / raw)
To: David Harton; +Cc: dev, wenzhuo.lu
On 12/10, David Harton wrote:
>Modified UPDATE_VF_STAT to properly handle rollover conditions.
>
>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
>
Could you provide Fixes: tag and cc stable?
Acked-by: Xiaolong Ye <xiaolong.ye@intel.com>
^ permalink raw reply [flat|nested] 7+ messages in thread* [dpdk-dev] [PATCH v2] net/e1000: update UPDATE_VF_STAT to handle rollover
2019-12-11 2:48 [dpdk-dev] [PATCH] net/e1000: update UPDATE_VF_STAT to handle rollover David Harton
2020-01-26 9:41 ` Ye Xiaolong
@ 2020-01-26 17:25 ` David Harton
2020-01-29 7:53 ` Ye Xiaolong
2020-01-29 10:10 ` Ferruh Yigit
1 sibling, 2 replies; 7+ messages in thread
From: David Harton @ 2020-01-26 17:25 UTC (permalink / raw)
To: dev; +Cc: wenzhuo.lu, konstantin.ananyev, xiaolong.ye, David Harton, intel.com
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
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [dpdk-dev] [PATCH v2] net/e1000: update UPDATE_VF_STAT to handle rollover
2020-01-26 17:25 ` [dpdk-dev] [PATCH v2] " David Harton
@ 2020-01-29 7:53 ` Ye Xiaolong
2020-01-29 10:10 ` Ferruh Yigit
1 sibling, 0 replies; 7+ messages in thread
From: Ye Xiaolong @ 2020-01-29 7:53 UTC (permalink / raw)
To: David Harton; +Cc: dev, wenzhuo.lu, konstantin.ananyev, intel.com
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.
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [dpdk-dev] [PATCH v2] net/e1000: update UPDATE_VF_STAT to handle rollover
2020-01-26 17:25 ` [dpdk-dev] [PATCH v2] " David Harton
2020-01-29 7:53 ` Ye Xiaolong
@ 2020-01-29 10:10 ` Ferruh Yigit
2020-01-29 17:56 ` David Harton (dharton)
2020-01-31 8:46 ` Ye Xiaolong
1 sibling, 2 replies; 7+ messages in thread
From: Ferruh Yigit @ 2020-01-29 10:10 UTC (permalink / raw)
To: David Harton, dev; +Cc: wenzhuo.lu, konstantin.ananyev, xiaolong.ye, intel.com
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
>
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [dpdk-dev] [PATCH v2] net/e1000: update UPDATE_VF_STAT to handle rollover
2020-01-29 10:10 ` Ferruh Yigit
@ 2020-01-29 17:56 ` David Harton (dharton)
2020-01-31 8:46 ` Ye Xiaolong
1 sibling, 0 replies; 7+ messages in thread
From: David Harton (dharton) @ 2020-01-29 17:56 UTC (permalink / raw)
To: Ferruh Yigit, dev; +Cc: wenzhuo.lu, konstantin.ananyev, xiaolong.ye, intel.com
> -----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
> >
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [dpdk-dev] [PATCH v2] net/e1000: update UPDATE_VF_STAT to handle rollover
2020-01-29 10:10 ` Ferruh Yigit
2020-01-29 17:56 ` David Harton (dharton)
@ 2020-01-31 8:46 ` Ye Xiaolong
1 sibling, 0 replies; 7+ messages in thread
From: Ye Xiaolong @ 2020-01-31 8:46 UTC (permalink / raw)
To: Ferruh Yigit; +Cc: David Harton, dev, wenzhuo.lu, konstantin.ananyev, intel.com
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
>>
>
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2020-01-31 8:48 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-12-11 2:48 [dpdk-dev] [PATCH] net/e1000: update UPDATE_VF_STAT to handle rollover David Harton
2020-01-26 9:41 ` Ye Xiaolong
2020-01-26 17:25 ` [dpdk-dev] [PATCH v2] " David Harton
2020-01-29 7:53 ` Ye Xiaolong
2020-01-29 10:10 ` Ferruh Yigit
2020-01-29 17:56 ` David Harton (dharton)
2020-01-31 8:46 ` Ye Xiaolong
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).