[v2] net/e1000: update UPDATE_VF_STAT to handle rollover
diff mbox series

Message ID 20200126172548.23327-1-dharton@cisco.com
State Changes Requested
Delegated to: xiaolong ye
Headers show
Series
  • [v2] net/e1000: update UPDATE_VF_STAT to handle rollover
Related show

Checks

Context Check Description
ci/iol-mellanox-Performance success Performance Testing PASS
ci/iol-nxp-Performance success Performance Testing PASS
ci/iol-testing success Testing PASS
ci/Intel-compilation success Compilation OK
ci/checkpatch warning coding style issues

Commit Message

David Harton (dharton) Jan. 26, 2020, 5:25 p.m. UTC
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

Xiaolong Ye Jan. 29, 2020, 7:53 a.m. UTC | #1
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.
Ferruh Yigit Jan. 29, 2020, 10:10 a.m. UTC | #2
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
>
David Harton (dharton) Jan. 29, 2020, 5:56 p.m. UTC | #3
> -----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
> >
Xiaolong Ye Jan. 31, 2020, 8:46 a.m. UTC | #4
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
>> 
>

Patch
diff mbox series

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