Re: [dpdk-dev] [PATCH v2] net/e1000: update UPDATE_VF_STAT to handle rollover

[Ye Xiaolong <xiaolong.ye@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
>> 
>