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

["David Harton (dharton)" <dharton@cisco.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
> >