[net-next PATCH] igb: update adapter stats when reading /proc/net/dev.

[net-next PATCH] igb: update adapter stats when reading /proc/net/dev.

[Jesper Dangaard Brouer]

Network driver igb: Improve the accuracy of stats in /proc/net/dev, by
updating the adapter stats when reading /proc/net/dev.  Currently the
stats are updated by the watchdog timer every 2 sec, or when getting
stats via ethtool -S.

A number of userspace apps read these /proc/net/dev stats every second,
e.g. ifstat, which then gives a perceived very bursty traffic pattern,
which is actually false.

Signed-off-by: Jesper Dangaard Brouer <hawk@comx.dk>
---

 drivers/net/igb/igb_main.c |   12 +++++++++---
 1 files changed, 9 insertions(+), 3 deletions(-)

diff --git a/drivers/net/igb/igb_main.c b/drivers/net/igb/igb_main.c
index d967949..7207dd3 100644
--- a/drivers/net/igb/igb_main.c
+++ b/drivers/net/igb/igb_main.c
@@ -3862,11 +3862,17 @@ static void igb_reset_task(struct work_struct *work)
  * @netdev: network interface device structure
  *
  * Returns the address of the device statistics structure.
- * The statistics are actually updated from the timer callback.
+ * The statistics are also updated from the timer callback
+ * igb_watchdog_task().
  **/
 static struct net_device_stats *igb_get_stats(struct net_device *netdev)
 {
-	/* only return the current stats */
+	struct igb_adapter *adapter = netdev_priv(netdev);
+
+	/* update stats */
+	igb_update_stats(adapter);
+
+	/* return the current stats */
 	return &netdev->stats;
 }
 
@@ -3940,7 +3946,7 @@ static int igb_change_mtu(struct net_device *netdev, int new_mtu)
 
 void igb_update_stats(struct igb_adapter *adapter)
 {
-	struct net_device_stats *net_stats = igb_get_stats(adapter->netdev);
+	struct net_device_stats *net_stats = &adapter->netdev->stats;
 	struct e1000_hw *hw = &adapter->hw;
 	struct pci_dev *pdev = adapter->pdev;
 	u32 rnbc;

Re: [net-next PATCH] igb: update adapter stats when reading /proc/net/dev.

[David Miller]

From: Jesper Dangaard Brouer <hawk@comx.dk>
Date: Tue, 12 Jan 2010 14:41:53 +0100

> Network driver igb: Improve the accuracy of stats in /proc/net/dev, by
> updating the adapter stats when reading /proc/net/dev.  Currently the
> stats are updated by the watchdog timer every 2 sec, or when getting
> stats via ethtool -S.
> 
> A number of userspace apps read these /proc/net/dev stats every second,
> e.g. ifstat, which then gives a perceived very bursty traffic pattern,
> which is actually false.
> 
> Signed-off-by: Jesper Dangaard Brouer <hawk@comx.dk>

Intel folks, I assume you guys are going to integrate this.

Thanks.

Re: [net-next PATCH] igb: update adapter stats when reading /proc/net/dev.

[Jeff Kirsher]

On Wed, Jan 13, 2010 at 17:30, David Miller <davem@davemloft.net> wrote:
> From: Jesper Dangaard Brouer <hawk@comx.dk>
> Date: Tue, 12 Jan 2010 14:41:53 +0100
>
>> Network driver igb: Improve the accuracy of stats in /proc/net/dev, by
>> updating the adapter stats when reading /proc/net/dev.  Currently the
>> stats are updated by the watchdog timer every 2 sec, or when getting
>> stats via ethtool -S.
>>
>> A number of userspace apps read these /proc/net/dev stats every second,
>> e.g. ifstat, which then gives a perceived very bursty traffic pattern,
>> which is actually false.
>>
>> Signed-off-by: Jesper Dangaard Brouer <hawk@comx.dk>
>
> Intel folks, I assume you guys are going to integrate this.
>
> Thanks.
> --

Correct, I have sucked this into my queue of patches.

-- 
Cheers,
Jeff

[net-next PATCH] igb: update adapter stats when reading /proc/net/dev.

[Jesper Dangaard Brouer]

Network driver igb: Improve the accuracy of stats in /proc/net/dev, by
updating the adapter stats when reading /proc/net/dev.  Currently the
stats are updated by the watchdog timer every 2 sec, or when getting
stats via ethtool -S.

A number of userspace apps read these /proc/net/dev stats every second,
e.g. ifstat, which then gives a perceived very bursty traffic pattern,
which is actually false.

Signed-off-by: Jesper Dangaard Brouer <hawk@comx.dk>
---
 drivers/net/igb/igb_main.c |   12 +++++++++---
 1 files changed, 9 insertions(+), 3 deletions(-)

diff --git a/drivers/net/igb/igb_main.c b/drivers/net/igb/igb_main.c
index 55edcb7..6cec297 100644
--- a/drivers/net/igb/igb_main.c
+++ b/drivers/net/igb/igb_main.c
@@ -4218,11 +4218,17 @@ static void igb_reset_task(struct work_struct *work)
  * @netdev: network interface device structure
  *
  * Returns the address of the device statistics structure.
- * The statistics are actually updated from the timer callback.
+ * The statistics are also updated from the timer callback
+ * igb_watchdog_task().
  **/
 static struct net_device_stats *igb_get_stats(struct net_device *netdev)
 {
-	/* only return the current stats */
+	struct igb_adapter *adapter = netdev_priv(netdev);
+
+	/* update stats */
+	igb_update_stats(adapter);
+
+	/* return the current stats */
 	return &netdev->stats;
 }
 
@@ -4307,7 +4313,7 @@ static int igb_change_mtu(struct net_device *netdev, int new_mtu)
 
 void igb_update_stats(struct igb_adapter *adapter)
 {
-	struct net_device_stats *net_stats = igb_get_stats(adapter->netdev);
+	struct net_device_stats *net_stats = &adapter->netdev->stats;
 	struct e1000_hw *hw = &adapter->hw;
 	struct pci_dev *pdev = adapter->pdev;
 	u32 reg, mpc;

Re: [net-next PATCH] igb: update adapter stats when reading /proc/net/dev.

[Eric Dumazet]

Le mardi 05 octobre 2010 à 16:18 +0200, Jesper Dangaard Brouer a écrit :
> Network driver igb: Improve the accuracy of stats in /proc/net/dev, by
> updating the adapter stats when reading /proc/net/dev.  Currently the
> stats are updated by the watchdog timer every 2 sec, or when getting
> stats via ethtool -S.
> 
> A number of userspace apps read these /proc/net/dev stats every second,
> e.g. ifstat, which then gives a perceived very bursty traffic pattern,
> which is actually false.
> 
> Signed-off-by: Jesper Dangaard Brouer <hawk@comx.dk>

Unfortunately this is racy with igb_watchdog_task()

igb_update_stats() must be called under protection of a lock.

Re: [net-next PATCH] igb: update adapter stats when reading /proc/net/dev.

[Jesper Dangaard Brouer]

On Tue, 2010-10-05 at 16:41 +0200, Eric Dumazet wrote:
> Le mardi 05 octobre 2010 à 16:18 +0200, Jesper Dangaard Brouer a écrit :
> > Network driver igb: Improve the accuracy of stats in /proc/net/dev, by
> > updating the adapter stats when reading /proc/net/dev.  Currently the
> > stats are updated by the watchdog timer every 2 sec, or when getting
> > stats via ethtool -S.
> > 
> > A number of userspace apps read these /proc/net/dev stats every second,
> > e.g. ifstat, which then gives a perceived very bursty traffic pattern,
> > which is actually false.
> > 
> > Signed-off-by: Jesper Dangaard Brouer <hawk@comx.dk>
> 
> Unfortunately this is racy with igb_watchdog_task()
> 
> igb_update_stats() must be called under protection of a lock.

Its already racy, because "ethtool -S" reads out the stats immediately,
and thus races with the timer.

See: igb_ethtool.c
 igb_get_ethtool_stats() invoke igb_update_stats(adapter);

-- 
Med venlig hilsen / Best regards
  Jesper Brouer
  ComX Networks A/S
  Linux Network Kernel Developer
  Cand. Scient Datalog / MSc.CS
  Author of http://adsl-optimizer.dk
  LinkedIn: http://www.linkedin.com/in/brouer

Re: [net-next PATCH] igb: update adapter stats when reading /proc/net/dev.

[Eric Dumazet]

Le mardi 05 octobre 2010 à 16:53 +0200, Jesper Dangaard Brouer a écrit :

> Its already racy, because "ethtool -S" reads out the stats immediately,
> and thus races with the timer.
> 
> See: igb_ethtool.c
>  igb_get_ethtool_stats() invoke igb_update_stats(adapter);
> 

You would be surprised how many bugs are waiting to be found and
fixed ;)

Re: [net-next PATCH] igb: update adapter stats when reading /proc/net/dev.

[Eric Dumazet]

Le mardi 05 octobre 2010 à 17:19 +0200, Eric Dumazet a écrit :
> Le mardi 05 octobre 2010 à 16:53 +0200, Jesper Dangaard Brouer a écrit :
> 
> > Its already racy, because "ethtool -S" reads out the stats immediately,
> > and thus races with the timer.
> > 
> > See: igb_ethtool.c
> >  igb_get_ethtool_stats() invoke igb_update_stats(adapter);
> > 
> 
> You would be surprised how many bugs are waiting to be found and
> fixed ;)
> 
> 

I took a look at igb stats, and it appears they also are racy on 32bit
arches. igb uses u64 counters, with no synchronization between
producers(writers) and consumers(readers).

Some fixes are needed ;)

Re: [net-next PATCH] igb: update adapter stats when reading /proc/net/dev.

[Jesper Dangaard Brouer]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 1133 bytes --]

On Tue, 5 Oct 2010, Eric Dumazet wrote:

> Le mardi 05 octobre 2010 à 17:19 +0200, Eric Dumazet a écrit :
>> Le mardi 05 octobre 2010 à 16:53 +0200, Jesper Dangaard Brouer a écrit :
>>
>>> Its already racy, because "ethtool -S" reads out the stats immediately,
>>> and thus races with the timer.
>>>
>>> See: igb_ethtool.c
>>>  igb_get_ethtool_stats() invoke igb_update_stats(adapter);
>>>
>>
>> You would be surprised how many bugs are waiting to be found and
>> fixed ;)
>
> I took a look at igb stats, and it appears they also are racy on 32bit
> arches. igb uses u64 counters, with no synchronization between
> producers(writers) and consumers(readers).

Are you saying, that we need more than a simple mutex in 
igb_update_stats()?


> Some fixes are needed ;)

The question is then if Intel wants to fix it, or let it be up to you or 
me?

Cheers,
   Jesper Brouer

--
-------------------------------------------------------------------
MSc. Master of Computer Science
Dept. of Computer Science, University of Copenhagen
Author of http://www.adsl-optimizer.dk
-------------------------------------------------------------------

Re: [net-next PATCH] igb: update adapter stats when reading /proc/net/dev.

[Jeff Kirsher]

On Tue, Oct 5, 2010 at 14:16, Jesper Dangaard Brouer <hawk@diku.dk> wrote:
> On Tue, 5 Oct 2010, Eric Dumazet wrote:
>
>> Le mardi 05 octobre 2010 à 17:19 +0200, Eric Dumazet a écrit :
>>>
>>> Le mardi 05 octobre 2010 à 16:53 +0200, Jesper Dangaard Brouer a écrit :
>>>
>>>> Its already racy, because "ethtool -S" reads out the stats immediately,
>>>> and thus races with the timer.
>>>>
>>>> See: igb_ethtool.c
>>>>  igb_get_ethtool_stats() invoke igb_update_stats(adapter);
>>>>
>>>
>>> You would be surprised how many bugs are waiting to be found and
>>> fixed ;)
>>
>> I took a look at igb stats, and it appears they also are racy on 32bit
>> arches. igb uses u64 counters, with no synchronization between
>> producers(writers) and consumers(readers).
>
> Are you saying, that we need more than a simple mutex in igb_update_stats()?
>
>
>> Some fixes are needed ;)
>
> The question is then if Intel wants to fix it, or let it be up to you or me?
>

I will make sure that Carolyn and Alex know about the issue.  But,
feel free to submit a patch if you guys have the time.

-- 
Cheers,
Jeff

Re: [net-next PATCH] igb: update adapter stats when reading /proc/net/dev.

[Eric Dumazet]

Le mardi 05 octobre 2010 à 15:34 -0700, Jeff Kirsher a écrit :
> On Tue, Oct 5, 2010 at 14:16, Jesper Dangaard Brouer <hawk@diku.dk> wrote:
> > On Tue, 5 Oct 2010, Eric Dumazet wrote:
> >
> >> Le mardi 05 octobre 2010 à 17:19 +0200, Eric Dumazet a écrit :
> >>>
> >>> Le mardi 05 octobre 2010 à 16:53 +0200, Jesper Dangaard Brouer a écrit :
> >>>
> >>>> Its already racy, because "ethtool -S" reads out the stats immediately,
> >>>> and thus races with the timer.
> >>>>
> >>>> See: igb_ethtool.c
> >>>>  igb_get_ethtool_stats() invoke igb_update_stats(adapter);
> >>>>
> >>>
> >>> You would be surprised how many bugs are waiting to be found and
> >>> fixed ;)
> >>
> >> I took a look at igb stats, and it appears they also are racy on 32bit
> >> arches. igb uses u64 counters, with no synchronization between
> >> producers(writers) and consumers(readers).
> >
> > Are you saying, that we need more than a simple mutex in igb_update_stats()?
> >
> >
> >> Some fixes are needed ;)
> >
> > The question is then if Intel wants to fix it, or let it be up to you or me?
> >
> 
> I will make sure that Carolyn and Alex know about the issue.  But,
> feel free to submit a patch if you guys have the time.
> 

I woke up early this morning, I'll provide patches to fix issues for
net-next-2.6

I'll let Intel guys doing the backporting work, but for old kernels,
you'll probably need to use "unsigned long" instead of "u64"

My plan is :

- Provide 64bit counters even on 32bit arch
- with proper synchro (include/linux/u64_stats_sync.h)
- Add a spinlock so we can apply Jesper patch.

Re: [net-next PATCH] igb: update adapter stats when reading /proc/net/dev.

[Jeff Kirsher]

On Tue, Oct 5, 2010 at 20:28, Eric Dumazet <eric.dumazet@gmail.com> wrote:
> Le mardi 05 octobre 2010 à 15:34 -0700, Jeff Kirsher a écrit :
>> On Tue, Oct 5, 2010 at 14:16, Jesper Dangaard Brouer <hawk@diku.dk> wrote:
>> > On Tue, 5 Oct 2010, Eric Dumazet wrote:
>> >
>> >> Le mardi 05 octobre 2010 à 17:19 +0200, Eric Dumazet a écrit :
>> >>>
>> >>> Le mardi 05 octobre 2010 à 16:53 +0200, Jesper Dangaard Brouer a écrit :
>> >>>
>> >>>> Its already racy, because "ethtool -S" reads out the stats immediately,
>> >>>> and thus races with the timer.
>> >>>>
>> >>>> See: igb_ethtool.c
>> >>>>  igb_get_ethtool_stats() invoke igb_update_stats(adapter);
>> >>>>
>> >>>
>> >>> You would be surprised how many bugs are waiting to be found and
>> >>> fixed ;)
>> >>
>> >> I took a look at igb stats, and it appears they also are racy on 32bit
>> >> arches. igb uses u64 counters, with no synchronization between
>> >> producers(writers) and consumers(readers).
>> >
>> > Are you saying, that we need more than a simple mutex in igb_update_stats()?
>> >
>> >
>> >> Some fixes are needed ;)
>> >
>> > The question is then if Intel wants to fix it, or let it be up to you or me?
>> >
>>
>> I will make sure that Carolyn and Alex know about the issue.  But,
>> feel free to submit a patch if you guys have the time.
>>
>
> I woke up early this morning, I'll provide patches to fix issues for
> net-next-2.6
>
> I'll let Intel guys doing the backporting work, but for old kernels,
> you'll probably need to use "unsigned long" instead of "u64"
>
> My plan is :
>
> - Provide 64bit counters even on 32bit arch
> - with proper synchro (include/linux/u64_stats_sync.h)
> - Add a spinlock so we can apply Jesper patch.
>
>

Thanks Eric, I really appreciate it.

-- 
Cheers,
Jeff

Re: [net-next PATCH] igb: update adapter stats when reading /proc/net/dev.

[David Miller]

From: Jesper Dangaard Brouer <hawk@comx.dk>
Date: Tue, 05 Oct 2010 16:18:33 +0200

> Network driver igb: Improve the accuracy of stats in /proc/net/dev, by
> updating the adapter stats when reading /proc/net/dev.  Currently the
> stats are updated by the watchdog timer every 2 sec, or when getting
> stats via ethtool -S.
> 
> A number of userspace apps read these /proc/net/dev stats every second,
> e.g. ifstat, which then gives a perceived very bursty traffic pattern,
> which is actually false.
> 
> Signed-off-by: Jesper Dangaard Brouer <hawk@comx.dk>

I assume that the Intel folks will take care of integrating this
now that the locking is fixed.

Thanks.

Re: [net-next PATCH] igb: update adapter stats when reading /proc/net/dev.

[Eric Dumazet]

Le mercredi 06 octobre 2010 à 23:36 -0700, David Miller a écrit :
> From: Jesper Dangaard Brouer <hawk@comx.dk>
> Date: Tue, 05 Oct 2010 16:18:33 +0200
> 
> > Network driver igb: Improve the accuracy of stats in /proc/net/dev, by
> > updating the adapter stats when reading /proc/net/dev.  Currently the
> > stats are updated by the watchdog timer every 2 sec, or when getting
> > stats via ethtool -S.
> > 
> > A number of userspace apps read these /proc/net/dev stats every second,
> > e.g. ifstat, which then gives a perceived very bursty traffic pattern,
> > which is actually false.
> > 
> > Signed-off-by: Jesper Dangaard Brouer <hawk@comx.dk>
> 
> I assume that the Intel folks will take care of integrating this
> now that the locking is fixed.
> 

I integrated Jesper patch into my cumulative patch.

BTW, ixgbe has similar locking problem.

Re: [net-next PATCH] igb: update adapter stats when reading /proc/net/dev.

[David Miller]

From: Eric Dumazet <eric.dumazet@gmail.com>
Date: Thu, 07 Oct 2010 08:44:08 +0200

> Le mercredi 06 octobre 2010 à 23:36 -0700, David Miller a écrit :
>> From: Jesper Dangaard Brouer <hawk@comx.dk>
>> Date: Tue, 05 Oct 2010 16:18:33 +0200
>> 
>> > Network driver igb: Improve the accuracy of stats in /proc/net/dev, by
>> > updating the adapter stats when reading /proc/net/dev.  Currently the
>> > stats are updated by the watchdog timer every 2 sec, or when getting
>> > stats via ethtool -S.
>> > 
>> > A number of userspace apps read these /proc/net/dev stats every second,
>> > e.g. ifstat, which then gives a perceived very bursty traffic pattern,
>> > which is actually false.
>> > 
>> > Signed-off-by: Jesper Dangaard Brouer <hawk@comx.dk>
>> 
>> I assume that the Intel folks will take care of integrating this
>> now that the locking is fixed.
>> 
> 
> I integrated Jesper patch into my cumulative patch.

Ok.

Re: [net-next PATCH] igb: update adapter stats when reading /proc/net/dev.

[Jesper Dangaard Brouer]

On Wed, 2010-10-06 at 23:46 -0700, David Miller wrote:
> From: Eric Dumazet <eric.dumazet@gmail.com>
> Date: Thu, 07 Oct 2010 08:44:08 +0200
> 
> > Le mercredi 06 octobre 2010 à 23:36 -0700, David Miller a écrit :
> >> From: Jesper Dangaard Brouer <hawk@comx.dk>
> >> Date: Tue, 05 Oct 2010 16:18:33 +0200
> >> 
> >> > Network driver igb: Improve the accuracy of stats in /proc/net/dev, by
> >> > updating the adapter stats when reading /proc/net/dev.  Currently the
> >> > stats are updated by the watchdog timer every 2 sec, or when getting
> >> > stats via ethtool -S.
> >> > 
> >> > A number of userspace apps read these /proc/net/dev stats every second,
> >> > e.g. ifstat, which then gives a perceived very bursty traffic pattern,
> >> > which is actually false.
> >> > 
> >> > Signed-off-by: Jesper Dangaard Brouer <hawk@comx.dk>
> >> 
> >> I assume that the Intel folks will take care of integrating this
> >> now that the locking is fixed.
> > 
> > I integrated Jesper patch into my cumulative patch.
> 
> Ok.

I'm fine with this, as long as the commit message describe this change
of accuracy of stats in /proc/net/dev.

Something like:

 This patch also increase the accuracy of stats in /proc/net/dev, by
 updating the adapter stats when reading /proc/net/dev.  Previously
 the stats were only updated by the watchdog timer every 2 sec, which
 resulted in false observations from userspace.


-- 
Med venlig hilsen / Best regards
  Jesper Brouer
  ComX Networks A/S
  Linux Network Kernel Developer
  Cand. Scient Datalog / MSc.CS
  Author of http://adsl-optimizer.dk
  LinkedIn: http://www.linkedin.com/in/brouer

Re: [net-next PATCH] igb: update adapter stats when reading /proc/net/dev.

[Eric Dumazet]

Le jeudi 07 octobre 2010 à 12:06 +0200, Jesper Dangaard Brouer a écrit :
> On Wed, 2010-10-06 at 23:46 -0700, David Miller wrote:
> > From: Eric Dumazet <eric.dumazet@gmail.com>
> > Date: Thu, 07 Oct 2010 08:44:08 +0200
> > 
> > > Le mercredi 06 octobre 2010 à 23:36 -0700, David Miller a écrit :
> > >> From: Jesper Dangaard Brouer <hawk@comx.dk>
> > >> Date: Tue, 05 Oct 2010 16:18:33 +0200
> > >> 
> > >> > Network driver igb: Improve the accuracy of stats in /proc/net/dev, by
> > >> > updating the adapter stats when reading /proc/net/dev.  Currently the
> > >> > stats are updated by the watchdog timer every 2 sec, or when getting
> > >> > stats via ethtool -S.
> > >> > 
> > >> > A number of userspace apps read these /proc/net/dev stats every second,
> > >> > e.g. ifstat, which then gives a perceived very bursty traffic pattern,
> > >> > which is actually false.
> > >> > 
> > >> > Signed-off-by: Jesper Dangaard Brouer <hawk@comx.dk>
> > >> 
> > >> I assume that the Intel folks will take care of integrating this
> > >> now that the locking is fixed.
> > > 
> > > I integrated Jesper patch into my cumulative patch.
> > 
> > Ok.
> 
> I'm fine with this, as long as the commit message describe this change
> of accuracy of stats in /proc/net/dev.
> 
> Something like:
> 
>  This patch also increase the accuracy of stats in /proc/net/dev, by
>  updating the adapter stats when reading /proc/net/dev.  Previously
>  the stats were only updated by the watchdog timer every 2 sec, which
>  resulted in false observations from userspace.
> 
> 

Well, its not exactly true :)

Previously, igb stats were updated :
- By watchdog timer, every 2 secs
- Every time an "ethtool -S ethX" was done

There is no general guarantee netdev stats are immediately available to
user.

ndo_get_stats() is not allowed to sleep, (because of bonding...), so
drivers can not always provide accurate stats, if they need to make a
long transaction with hardware.

Other drivers do the same (provide hardware statistics), with about one
second resolution.

So the "resulted in false observations from userspace." is something
that might upset admins, but is not a hard requirement of netdev stats.

Re: [net-next PATCH] igb: update adapter stats when reading /proc/net/dev.

[Jesper Dangaard Brouer]

On Thu, 2010-10-07 at 12:24 +0200, Eric Dumazet wrote:
> Le jeudi 07 octobre 2010 à 12:06 +0200, Jesper Dangaard Brouer a écrit :
> > On Wed, 2010-10-06 at 23:46 -0700, David Miller wrote:
> > > From: Eric Dumazet <eric.dumazet@gmail.com>
> > > Date: Thu, 07 Oct 2010 08:44:08 +0200
> > > 
> > > > Le mercredi 06 octobre 2010 à 23:36 -0700, David Miller a écrit :
> > > >> From: Jesper Dangaard Brouer <hawk@comx.dk>
> > > >> Date: Tue, 05 Oct 2010 16:18:33 +0200
> > > >> 
> > > >> > Network driver igb: Improve the accuracy of stats in /proc/net/dev, by
> > > >> > updating the adapter stats when reading /proc/net/dev.  Currently the
> > > >> > stats are updated by the watchdog timer every 2 sec, or when getting
> > > >> > stats via ethtool -S.
> > > >> > 
> > > >> > A number of userspace apps read these /proc/net/dev stats every second,
> > > >> > e.g. ifstat, which then gives a perceived very bursty traffic pattern,
> > > >> > which is actually false.
> > > >> > 
> > > >> > Signed-off-by: Jesper Dangaard Brouer <hawk@comx.dk>
> > > >> 
> > > >> I assume that the Intel folks will take care of integrating this
> > > >> now that the locking is fixed.
> > > > 
> > > > I integrated Jesper patch into my cumulative patch.
> > > 
> > > Ok.
> > 
> > I'm fine with this, as long as the commit message describe this change
> > of accuracy of stats in /proc/net/dev.
> > 
> > Something like:
> > 
> >  This patch also increase the accuracy of stats in /proc/net/dev, by
> >  updating the adapter stats when reading /proc/net/dev.  Previously
> >  the stats were only updated by the watchdog timer every 2 sec, which
> >  resulted in false observations from userspace.
> > 
> > 
> 
> Well, its not exactly true :)
> 
> Previously, igb stats were updated :
> - By watchdog timer, every 2 secs
> - Every time an "ethtool -S ethX" was done
> 
> There is no general guarantee netdev stats are immediately available to
> user.

I'm not talking general, just for this driver.
And I'm not giving any guarantees, that is why I choose the wording
"increase the accuracy" and not "provide accurate stats".

> ndo_get_stats() is not allowed to sleep, (because of bonding...), so
> drivers can not always provide accurate stats, if they need to make a
> long transaction with hardware.
> 
> Other drivers do the same (provide hardware statistics), with about one
> second resolution.

Yes, a lot of drivers don't provide accurate states. And one second
resolution, as most drivers use, will usually be good enough for most
admins.

Our usage pattern is a bit different, as we are very interested in
measuring bursty traffic.  I'm explain you why during the Netfilter
Workshop, if I get my talk about IPTV accepted ;-)

I also use the increased accuracy, when running my pktgen testing
scripts.


> So the "resulted in false observations from userspace." is something
> that might upset admins, but is not a hard requirement of netdev stats.

Its definitly not a hard requirement, but lets fix it where we can.

Then change the text:
 "resulted in false observations from userspace"
to:
 "resulted in inaccurate observations from userspace"

-- 
Med venlig hilsen / Best regards
  Jesper Brouer
  ComX Networks A/S
  Linux Network Kernel Developer
  Cand. Scient Datalog / MSc.CS
  Author of http://adsl-optimizer.dk
  LinkedIn: http://www.linkedin.com/in/brouer