RE: [PATCH] ixgbevf - Prevent RX/TX statistics getting reset to zero

[<Narendra_K@Dell.com>]

> -----Original Message-----
> From: Greg Rose [mailto:gregory.v.rose@intel.com]
> Sent: Friday, July 13, 2012 10:44 PM
> To: K, Narendra
> Cc: jeffrey.t.kirsher@intel.com; netdev@vger.kernel.org
> Subject: Re: [PATCH] ixgbevf - Prevent RX/TX statistics getting reset to zero
> 
> On Fri, 13 Jul 2012 05:36:37 -0700
> <Narendra_K@Dell.com> wrote:
> 
> > > -----Original Message-----
> > > From: Jeff Kirsher [mailto:tarbal@gmail.com]
> > > Sent: Thursday, July 12, 2012 11:56 PM
> > > To: K, Narendra; gregory.v.rose@intel.com
> > > Cc: netdev@vger.kernel.org
> > > Subject: Re: [PATCH] ixgbevf - Prevent RX/TX statistics getting
> > > reset to zero
> > >
> > > On 07/12/2012 06:55 AM, Narendra_K@Dell.com wrote:
> > > > Hello,
> > > >
> > > > [Apologies if you are receiving this message twice. I am resending
> > > > the
> > > message, as I got message delivery failure note].
> > > >
> > > > While exploring SR-IOV on Intel 82599EB 10-Gigabit SFP+ adapter, I
> > > > had the
> > > following observation.  I enabled two VFs by passing 'max_vfs=2' to
> > > ixgbe driver. One of the VFs was assigned to a guest.
> > > > In the guest, the ifconfig and ip tools reported 'RX packets' and
> > > > 'TX packets'
> > > as zero, after pinging to a remote host. Looking into it further,
> > > the commit 4197aa7bb81877ebb06e4f2cc1b5fea2da23a7bd implements
> 64
> > > bit per ring statistics. It seemed like the 'total_bytes' and
> > > 'total_packets' of RX and TX ring were being reset to zero by the RX
> > > and TX interrupt handlers, resulting in the user space tools
> > > reporting zero RX and TX bytes.
> > > >
> > > > The attached patch addresses the issue by preventing the resetting
> > > > of RX
> > > and TX ring statistics to zero. The patch was taken against latest
> > > mainline 3.5- rc6 kernel.
> > > >
> > > > I tested the patch by pinging  from the guest OS to a remote host.
> > > >
> > > > ping -f <remote host> -c 10000
> > > >
> > > > The ip and ifcofig showed the statistics increased by 10000
> > > > packets.
> > > >
> > > > # lspci | grep 82599
> > > > 04:00.0 Ethernet controller: Intel Corporation 82599EB 10-Gigabit
> > > > SFP+
> > > Network Connection (rev 01)
> > > > 04:00.1 Ethernet controller: Intel Corporation 82599EB 10-Gigabit
> > > > SFP+
> > > Network Connection (rev 01)
> > > > 04:10.0 Ethernet controller: Intel Corporation 82599 Ethernet
> > > > Controller
> > > Virtual Function (rev 01)
> > > > 04:10.1 Ethernet controller: Intel Corporation 82599 Ethernet
> > > > Controller
> > > Virtual Function (rev 01)
> > > > 04:10.2 Ethernet controller: Intel Corporation 82599 Ethernet
> > > > Controller
> > > Virtual Function (rev 01)
> > > > 04:10.3 Ethernet controller: Intel Corporation 82599 Ethernet
> > > > Controller
> > > Virtual Function (rev 01)
> > > >
> > > > # lspci -s 04:00.0 -n
> > > > 04:00.0 0200: 8086:154d (rev 01)
> > > > # lspci -s 04:10.0 -n
> > > > 04:10.0 0200: 8086:10ed (rev 01)
> > > >
> > > > Please let me know if additional details and logs are required.[>]
> > > >
> > > > With regards,
> > > > Narendra K
> > > >
> > > >
> > > >
> > >
> > > Thanks, I will add the patch to my queue
> > >
> > [>]
> >
> > Hi Greg,
> >
> > I was re-looking at why ' rx_ring->total_packets' and '
> > rx_ring->total_bytes' were being set to zero in '
> > ixgbevf_msix_clean_rx'.  It looks like ' rx_ring->total_packets' and '
> > rx_ring->total_packets'  are computed per one run of
> > 'ixgbevf_clean_rx_irq' .  Then in 'ixgbevf_clean_rxonly' if
> > 'adapter->itr_setting & 1' is true, the count is  used in
> > 'ixgbevf_set_itr_msix'. When the interrupts are enabled, the '
> > rx_ring->total_packets'  and ' rx_ring->total_bytes' are set to zero
> > so that they can be re-computed in the poll function and  fed to the
> > 'ixgbevf_set_itr_msix'.
> >
> > This results in statistics reported by 'ip' and 'ifconfig' as zero.
> > The patch addresses the scenario.  But it seems it would change the
> > intended behavior in the scenario  when 'adapter->itr_setting & 1' is
> > true.  It could be addressed by storing the 'total_rx_packets' and
> > 'total_rx_bytes'  computed every time in the poll function in 'struct
> > ixgbevf_adapter' . Then the interrupt handler could reset them to zero
> > instead of resetting  ' rx_ring->total_packets' and  '
> > rx_ring->total_bytes'.
> >
> > Also, I observed that  'adapter->itr_setting & 1'  was not true by
> > default.  I tried setting it by 'ethtool  -C eth0 adaptive-rx on', and
> > it returned 'operation not supported'.
> >
> > I could be missing something here, please let me know.
> 
> Nope, you're correct in your analysis.  The ixgbevf driver hasn't supported
> adaptive interrupt moderation in the past.  However, a set of patches we
> have in the pipeline will turn it on by default.  Also, as a result of those
> patches the bug you've reported will be fixed.
> We'll go ahead and accept your patch for the net tree and then fix up any
> conflicts between that and our new set of patches when they get pushed to
> net-next.
> 
> Thanks for your work on this.
> 
> - Greg

Thank you Greg.

With regards,
Narendra K