Skip to contentSkip to conversation

igb: update adapter stats when reading /proc/net/dev.

PATCH In review Jesper Dangaard Brouer 6 participants 21 messages Oct 5, 2010 – Oct 12, 2010

Subsystems (2)
Networking Network drivers
lore
Share & tools
lore

Jesper Dangaard Brouer

author
#0 lore raw

PATCH 1/1 igb: update adapter stats when reading /proc/net/dev.

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 <[email protected]>
---
 drivers/net/igb/igb_main.c |   12 +++++++++---
 1 files changed, 9 insertions(+), 3 deletions(-)
drivers/net/igb/igb_main.c +9 -3
index 55edcb7..6cec297 100644
@@ -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;
#1 lore raw
Quoted 10 lines · 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 <[email protected]>

Unfortunately this is racy with igb_watchdog_task()

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

author
#2 lore raw
Quoted 10 lines · Eric Dumazet
Quoted 9 lines · 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.

Quoted 5 lines
Quoted 1 line
Signed-off-by Jesper Dangaard Brouer <[email protected]>

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);

Signature · 7 lines
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
#3 lore raw
Quoted 6 lines · Jesper Dangaard Brouer

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 ;)

#4 lore raw
Quoted 5 lines · Eric Dumazet
Quoted 3 lines · Jesper Dangaard Brouer

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

Quoted 8 lines
Quoted 3 lines

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 ;)

#5 lore raw
Quoted 10 lines · Eric Dumazet
Quoted 9 lines · Eric Dumazet
Quoted 6 lines · Jesper Dangaard Brouer

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);

Quoted 6 lines
Quoted 2 lines

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()?

Quoted 1 line

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
-------------------------------------------------------------------

#6 lore raw
Quoted 20 lines · Jesper Dangaard Brouer
Quoted 11 lines · Eric Dumazet
Quoted 10 lines · Eric Dumazet
Quoted 6 lines · Jesper Dangaard Brouer

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);

Quoted 6 lines
Quoted 2 lines

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).

Quoted 7 lines

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

Quoted 1 line

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.

Signature · 2 lines
Cheers,
Jeff
#7 lore raw
Quoted 24 lines · Jeff Kirsher
Quoted 23 lines · Jesper Dangaard Brouer
Quoted 11 lines · Eric Dumazet
Quoted 10 lines · Eric Dumazet
Quoted 6 lines · Jesper Dangaard Brouer

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);

Quoted 6 lines
Quoted 2 lines

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()?

Quoted 8 lines
Quoted 4 lines
Quoted 1 line

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.

#8 lore raw

PATCH 1/1 igb: fix stats handling

Quoted 8 lines · Eric Dumazet

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.

Here is the net-next-2.6 patch, I am currently enable to test it, the dev machine with IGB NIC cannot be restarted until tomorrow, my son Nicolas is currently using it ;)

Could you and/or Jesper test it, possibly on 32 and 64 bit kernels ?

Thanks !

[PATCH net-next] igb: fix stats handling

There are currently some problems with igb.

- On 32bit arches, maintaining 64bit counters without proper synchronization between writers and readers.

- Stats updated every two seconds, as reported by Jesper. (Jesper provided a patch for this)

- Potential problem between worker thread and ethtool -S

This patch uses u64_stats_sync, and convert everything to be 64bit safe, SMP safe, even on 32bit arches.

Signed-off-by Eric Dumazet <[email protected]>
---
 drivers/net/igb/igb.h         |    7 +-
 drivers/net/igb/igb_ethtool.c |   10 +-
 drivers/net/igb/igb_main.c    |  111 +++++++++++++++++++++++---------
 3 files changed, 94 insertions(+), 34 deletions(-)
drivers/net/igb/igb.h +6 -1
index 44e0ff1..a1b9584 100644
@@ -159,6 +159,7 @@ struct igb_tx_queue_stats {
 	u64 packets;
 	u64 bytes;
 	u64 restart_queue;
+	struct u64_stats_sync syncp;
 };
 
 struct igb_rx_queue_stats {

@@ -167,6 +168,7 @@ struct igb_rx_queue_stats {
 	u64 drops;
 	u64 csum_err;
 	u64 alloc_failed;
+	struct u64_stats_sync syncp;
 };
 
 struct igb_q_vector {

@@ -288,6 +290,9 @@ struct igb_adapter {
 	struct timecompare compare;
 	struct hwtstamp_config hwtstamp_config;
 
+	spinlock_t stats64_lock;
+	struct rtnl_link_stats64 stats64;
+
 	/* structs defined in e1000_hw.h */
 	struct e1000_hw hw;
 	struct e1000_hw_stats stats;

@@ -357,7 +362,7 @@ extern netdev_tx_t igb_xmit_frame_ring_adv(struct sk_buff *, struct igb_ring *);
 extern void igb_unmap_and_free_tx_resource(struct igb_ring *,
 					   struct igb_buffer *);
 extern void igb_alloc_rx_buffers_adv(struct igb_ring *, int);
-extern void igb_update_stats(struct igb_adapter *);
+extern void igb_update_stats(struct igb_adapter *, struct rtnl_link_stats64 *);
 extern bool igb_has_link(struct igb_adapter *adapter);
 extern void igb_set_ethtool_ops(struct net_device *);
 extern void igb_power_up_link(struct igb_adapter *);
drivers/net/igb/igb_ethtool.c +6 -4
index 26bf6a1..e51c233 100644
@@ -90,8 +90,8 @@ static const struct igb_stats igb_gstrings_stats[] = {
 
 #define IGB_NETDEV_STAT(_net_stat) { \
 	.stat_string = __stringify(_net_stat), \
-	.sizeof_stat = FIELD_SIZEOF(struct net_device_stats, _net_stat), \
-	.stat_offset = offsetof(struct net_device_stats, _net_stat) \
+	.sizeof_stat = FIELD_SIZEOF(struct rtnl_link_stats64, _net_stat), \
+	.stat_offset = offsetof(struct rtnl_link_stats64, _net_stat) \
 }
 static const struct igb_stats igb_gstrings_net_stats[] = {
 	IGB_NETDEV_STAT(rx_errors),

@@ -2070,12 +2070,13 @@ static void igb_get_ethtool_stats(struct net_device *netdev,
 				  struct ethtool_stats *stats, u64 *data)
 {
 	struct igb_adapter *adapter = netdev_priv(netdev);
-	struct net_device_stats *net_stats = &netdev->stats;
+	struct rtnl_link_stats64 *net_stats = &adapter->stats64;
 	u64 *queue_stat;
 	int i, j, k;
 	char *p;
 
-	igb_update_stats(adapter);
+	spin_lock(&adapter->stats64_lock);
+	igb_update_stats(adapter, net_stats);
 
 	for (i = 0; i < IGB_GLOBAL_STATS_LEN; i++) {
 		p = (char *)adapter + igb_gstrings_stats[i].stat_offset;

@@ -2097,6 +2098,7 @@ static void igb_get_ethtool_stats(struct net_device *netdev,
 		for (k = 0; k < IGB_RX_QUEUE_STATS_LEN; k++, i++)
 			data[i] = queue_stat[k];
 	}
+	spin_unlock(&adapter->stats64_lock);
 }
 
 static void igb_get_strings(struct net_device *netdev, u32 stringset, u8 *data)
drivers/net/igb/igb_main.c +82 -29

263 lines not on this page · raw message

#9 lore raw

Re: [PATCH net-next] igb: fix stats handling

Quoted 34 lines · Eric Dumazet
Quoted 8 lines · Eric Dumazet

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.

Here is the net-next-2.6 patch, I am currently enable to test it, the dev machine with IGB NIC cannot be restarted until tomorrow, my son Nicolas is currently using it ;)

Could you and/or Jesper test it, possibly on 32 and 64 bit kernels ?

Thanks !

[PATCH net-next] igb: fix stats handling

There are currently some problems with igb.

- On 32bit arches, maintaining 64bit counters without proper synchronization between writers and readers.

- Stats updated every two seconds, as reported by Jesper. (Jesper provided a patch for this)

- Potential problem between worker thread and ethtool -S

This patch uses u64_stats_sync, and convert everything to be 64bit safe, SMP safe, even on 32bit arches.

Quoted 7 lines
Signed-off-by Eric Dumazet <[email protected]>

---
 drivers/net/igb/igb.h         |    7 +- drivers/net/igb/igb_ethtool.c |   10 +- drivers/net/igb/igb_main.c    |  111 +++++++++++++++++++++++---------

 3 files changed, 94 insertions(+), 34 deletions(-)

Thanks again Eric! I will add your patch to my queue.

Signature · 2 lines
Cheers,
Jeff
author Acked-by Tested-by on v1 1/1
#10 lore raw

Re: [PATCH net-next] igb: fix stats handling

In reply to Eric Dumazet
Quoted 10 lines · Eric Dumazet
Quoted 8 lines · Eric Dumazet

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.

You have already integrated my patch, so you get my:

Acked-by Jesper Dangaard Brouer <[email protected]>
Quoted 5 lines

Here is the net-next-2.6 patch, I am currently enable to test it, the dev machine with IGB NIC cannot be restarted until tomorrow, my son Nicolas is currently using it ;)

Could you and/or Jesper test it, possibly on 32 and 64 bit kernels ?

I could not resist, I have tested the patch, on a 64 bit kernel, it works :-). Did some simple test with two while loops running ethtool -S and ifconfig, didn't see any wrong packet counts.

Tested-by Jesper Dangaard Brouer <[email protected]>

Now back to moving office ;-)

Signature · 7 lines
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
#11 lore raw

RE: [PATCH net-next] igb: fix stats handling

In reply to Eric Dumazet
Quoted 35 lines · Eric Dumazet
Quoted 8 lines · Eric Dumazet

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.

Here is the net-next-2.6 patch, I am currently enable to test it, the dev machine with IGB NIC cannot be restarted until tomorrow, my son Nicolas is currently using it ;)

Could you and/or Jesper test it, possibly on 32 and 64 bit kernels ?

Thanks !

[PATCH net-next] igb: fix stats handling

There are currently some problems with igb.

- On 32bit arches, maintaining 64bit counters without proper synchronization between writers and readers.

- Stats updated every two seconds, as reported by Jesper. (Jesper provided a patch for this)

- Potential problem between worker thread and ethtool -S

This patch uses u64_stats_sync, and convert everything to be 64bit safe,
SMP safe, even on 32bit arches.

Quoted 6 lines
Signed-off-by Eric Dumazet <[email protected]>
---
 drivers/net/igb/igb.h         |    7 +-
 drivers/net/igb/igb_ethtool.c |   10 +-
 drivers/net/igb/igb_main.c    |  111 +++++++++++++++++++++++---------
 3 files changed, 94 insertions(+), 34 deletions(-)

This patch is causing a hang when testing with 2 sessions in a while loop reading /proc/net/dev/ and ethtool -S. I think even just reading /proc/net/dev/ is sufficient, but have not confirmed it yet. I have seen the hang somewhere between 15 min to an hour. Without the patch same test ran 24+ hours without issues.

There was no trace on the screen, I got this with magic sysrq:

[15388.393579] SysRq : Show Regs 
[15388.397341] Modules linked in: igb [last unloaded: scsi_wait_scan] [15388.404846]  
[15388.406889] Pid: 16218, comm: kworker/4:1 Not tainted 2.6.36-rc3-net-next-igb-100810+ #2 S5520HC/S5520HC 
[15388.418393] EIP: 0060:[<c13fead2>] EFLAGS: 00000297 CPU: 4 
[15388.424908] EIP is at _raw_spin_lock+0x13/0x19 
[15388.430257] EAX: f6eab55c EBX: f6eab380 ECX: 00000001 EDX: 00004e4a [15388.437629] ESI: f6eab000 EDI: f6eab41c EBP: f3d9bf4c ESP: f3d9bf4c [15388.445011]  DS: 007b ES: 007b FS: 00d8 GS: 0000 SS: 0068 
[15388.451422] Process kworker/4:1 (pid: 16218, ti=f3d9a000 task=f5ce0ed0 task.ti=f3d9a000) 
[15388.461173] Stack: 
[15388.463796]  f3d9bf64 f8586c57 f3d9bf5c f6eab41c f6901e00 c8703cc0 f3d9bf90 c1041379 
[15388.473116] <0> 00000004 00000000 f8586b16 00000000 c8707b05 c8707b00 f6901e00 c8703cc4 
[15388.483379] <0> c8703cc0 f3d9bfb8 c1042879 c16bb640 c8703cc4 00000c54 c16bb640 f6901e10 
[15388.494219] Call Trace: 
[15388.497336]  [<f8586c57>] ? igb_watchdog_task+0x141/0x21a [igb] 
[15388.504336]  [<c1041379>] ? process_one_work+0x18e/0x265 
[15388.510643]  [<f8586b16>] ? igb_watchdog_task+0x0/0x21a [igb] 
[15388.517455]  [<c1042879>] ? worker_thread+0xf3/0x1ef 
[15388.523384]  [<c1042786>] ? worker_thread+0x0/0x1ef 
[15388.529222]  [<c104506b>] ? kthread+0x62/0x67 
[15388.534475]  [<c1045009>] ? kthread+0x0/0x67 
[15388.539623]  [<c1002d36>] ? kernel_thread_helper+0x6/0x10 
[15388.546034] Code: 00 75 05 f0 66 0f b1 0a 0f 94 c1 0f b6 c1 85 c0 0f 95 c0 0f b6 c0 5d c3 55 ba 00 01 00 00 89 e5 f0 66 0f c1 10 38 f2 74 06 f3 90 <8a> 10 eb f6 5d c3 55 89 e5 9c 59 fa ba 00 01 00 00 f0 66 0f c1

Thanks,
Emil

#12 lore raw

PATCH v2 1/1 igb: fix stats handling

Quoted 69 lines · Emil S
Quoted 35 lines · Eric Dumazet
Quoted 8 lines · Eric Dumazet

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.

Here is the net-next-2.6 patch, I am currently enable to test it, the dev machine with IGB NIC cannot be restarted until tomorrow, my son Nicolas is currently using it ;)

Could you and/or Jesper test it, possibly on 32 and 64 bit kernels ?

Thanks !

[PATCH net-next] igb: fix stats handling

There are currently some problems with igb.

- On 32bit arches, maintaining 64bit counters without proper synchronization between writers and readers.

- Stats updated every two seconds, as reported by Jesper. (Jesper provided a patch for this)

- Potential problem between worker thread and ethtool -S

This patch uses u64_stats_sync, and convert everything to be 64bit safe,
SMP safe, even on 32bit arches.

Quoted 6 lines
Signed-off-by Eric Dumazet <[email protected]>
---
 drivers/net/igb/igb.h         |    7 +-
 drivers/net/igb/igb_ethtool.c |   10 +-
 drivers/net/igb/igb_main.c    |  111 +++++++++++++++++++++++---------
 3 files changed, 94 insertions(+), 34 deletions(-)

This patch is causing a hang when testing with 2 sessions in a while loop reading /proc/net/dev/ and ethtool -S. I think even just reading /proc/net/dev/ is sufficient, but have not confirmed it yet. I have seen the hang somewhere between 15 min to an hour. Without the patch same test ran 24+ hours without issues.

There was no trace on the screen, I got this with magic sysrq:

[15388.393579] SysRq : Show Regs 
[15388.397341] Modules linked in: igb [last unloaded: scsi_wait_scan] [15388.404846]  
[15388.406889] Pid: 16218, comm: kworker/4:1 Not tainted 2.6.36-rc3-net-next-igb-100810+ #2 S5520HC/S5520HC 
[15388.418393] EIP: 0060:[<c13fead2>] EFLAGS: 00000297 CPU: 4 
[15388.424908] EIP is at _raw_spin_lock+0x13/0x19 
[15388.430257] EAX: f6eab55c EBX: f6eab380 ECX: 00000001 EDX: 00004e4a [15388.437629] ESI: f6eab000 EDI: f6eab41c EBP: f3d9bf4c ESP: f3d9bf4c [15388.445011]  DS: 007b ES: 007b FS: 00d8 GS: 0000 SS: 0068 
[15388.451422] Process kworker/4:1 (pid: 16218, ti=f3d9a000 task=f5ce0ed0 task.ti=f3d9a000) 
[15388.461173] Stack: 
[15388.463796]  f3d9bf64 f8586c57 f3d9bf5c f6eab41c f6901e00 c8703cc0 f3d9bf90 c1041379 
[15388.473116] <0> 00000004 00000000 f8586b16 00000000 c8707b05 c8707b00 f6901e00 c8703cc4 
[15388.483379] <0> c8703cc0 f3d9bfb8 c1042879 c16bb640 c8703cc4 00000c54 c16bb640 f6901e10 
[15388.494219] Call Trace: 
[15388.497336]  [<f8586c57>] ? igb_watchdog_task+0x141/0x21a [igb] 
[15388.504336]  [<c1041379>] ? process_one_work+0x18e/0x265 
[15388.510643]  [<f8586b16>] ? igb_watchdog_task+0x0/0x21a [igb] 
[15388.517455]  [<c1042879>] ? worker_thread+0xf3/0x1ef 
[15388.523384]  [<c1042786>] ? worker_thread+0x0/0x1ef 
[15388.529222]  [<c104506b>] ? kthread+0x62/0x67 
[15388.534475]  [<c1045009>] ? kthread+0x0/0x67 
[15388.539623]  [<c1002d36>] ? kernel_thread_helper+0x6/0x10 
[15388.546034] Code: 00 75 05 f0 66 0f b1 0a 0f 94 c1 0f b6 c1 85 c0 0f 95 c0 0f b6 c0 5d c3 55 ba 00 01 00 00 89 e5 f0 66 0f c1 10 38 f2 74 06 f3 90 <8a> 10 eb f6 5d c3 55 89 e5 9c 59 fa ba 00 01 00 00 f0 66 0f c1
Quoted 3 lines

Thanks,
Emil

Hi Emil, thanks for testing.

It seems one one the u64_stats_sync invariant is not met.

I believe problem comes from "restart_queue"

This one can be updated in // by two cpus.

So we can lose one of the u64_stats_update_begin() / u64_stats_update_end() increment and freeze a reader.

So igb had a previous bug here, un-noticed :)

restart_queue can be updated by start_xmit() (so only one cpu can be there, because of txqueue lock serialization), but it also can be updated by the igb_clean_tx_irq() function (one cpu there too).

One solution to this problem is to use two separate counters, with two separate syncp.

[PATCH net-next V2] igb: fix stats handling

There are currently some problems with igb.

- On 32bit arches, maintaining 64bit counters without proper synchronization between writers and readers.

- Stats updated every two seconds, as reported by Jesper. (Jesper provided a patch for this)

- Potential problem between worker thread and ethtool -S

This patch uses u64_stats_sync, and convert everything to be 64bit safe, SMP safe, even on 32bit arches. It integrates Jesper idea of providing accurate stats at the time user reads them.

Signed-off-by Eric Dumazet <[email protected]>

---
V2: Add a second restart_queue field, with separate syncp, because original restart_queue was potentially udpated by two cpus.
Corrected igb_get_ethtool_stats() to also use appropriate syncp, and do the sum of two restart_queue fields.

 drivers/net/igb/igb.h         |    9 ++
 drivers/net/igb/igb_ethtool.c |   52 ++++++++++----
 drivers/net/igb/igb_main.c    |  113 +++++++++++++++++++++++---------
 3 files changed, 129 insertions(+), 45 deletions(-)
drivers/net/igb/igb.h +8 -1
index 44e0ff1..edab9c4 100644
@@ -159,6 +159,7 @@ struct igb_tx_queue_stats {
 	u64 packets;
 	u64 bytes;
 	u64 restart_queue;
+	u64 restart_queue2;
 };
 
 struct igb_rx_queue_stats {

@@ -210,11 +211,14 @@ struct igb_ring {
 		/* TX */
 		struct {
 			struct igb_tx_queue_stats tx_stats;
+			struct u64_stats_sync tx_syncp;
+			struct u64_stats_sync tx_syncp2;
 			bool detect_tx_hung;
 		};
 		/* RX */
 		struct {
 			struct igb_rx_queue_stats rx_stats;
+			struct u64_stats_sync rx_syncp;
 			u32 rx_buffer_len;
 		};
 	};

@@ -288,6 +292,9 @@ struct igb_adapter {
 	struct timecompare compare;
 	struct hwtstamp_config hwtstamp_config;
 
+	spinlock_t stats64_lock;
+	struct rtnl_link_stats64 stats64;
+
 	/* structs defined in e1000_hw.h */
 	struct e1000_hw hw;
 	struct e1000_hw_stats stats;

@@ -357,7 +364,7 @@ extern netdev_tx_t igb_xmit_frame_ring_adv(struct sk_buff *, struct igb_ring *);
 extern void igb_unmap_and_free_tx_resource(struct igb_ring *,
 					   struct igb_buffer *);
 extern void igb_alloc_rx_buffers_adv(struct igb_ring *, int);
-extern void igb_update_stats(struct igb_adapter *);
+extern void igb_update_stats(struct igb_adapter *, struct rtnl_link_stats64 *);
 extern bool igb_has_link(struct igb_adapter *adapter);
 extern void igb_set_ethtool_ops(struct net_device *);
 extern void igb_power_up_link(struct igb_adapter *);
drivers/net/igb/igb_ethtool.c +38 -14

81 lines not on this page · raw message

drivers/net/igb/igb_main.c +83 -30

264 lines not on this page · raw message

#13 lore raw

Re: [PATCH net-next V2] igb: fix stats handling

Quoted 117 lines · Eric Dumazet
Quoted 69 lines · Emil S
Quoted 35 lines · Eric Dumazet
Quoted 8 lines · Eric Dumazet

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.

Here is the net-next-2.6 patch, I am currently enable to test it, the dev machine with IGB NIC cannot be restarted until tomorrow, my son Nicolas is currently using it ;)

Could you and/or Jesper test it, possibly on 32 and 64 bit kernels ?

Thanks !

[PATCH net-next] igb: fix stats handling

There are currently some problems with igb.

- On 32bit arches, maintaining 64bit counters without proper synchronization between writers and readers.

- Stats updated every two seconds, as reported by Jesper. (Jesper provided a patch for this)

- Potential problem between worker thread and ethtool -S

This patch uses u64_stats_sync, and convert everything to be 64bit safe,
SMP safe, even on 32bit arches.

Quoted 6 lines
Signed-off-by Eric Dumazet <[email protected]>

---
 drivers/net/igb/igb.h         |    7 +- drivers/net/igb/igb_ethtool.c |   10 +- drivers/net/igb/igb_main.c    |  111 +++++++++++++++++++++++---------

 3 files changed, 94 insertions(+), 34 deletions(-)

This patch is causing a hang when testing with 2 sessions in a while loop reading /proc/net/dev/ and ethtool -S. I think even just reading /proc/net/dev/ is sufficient, but have not confirmed it yet. I have seen the hang somewhere between 15 min to an hour. Without the patch same test ran 24+ hours without issues.

There was no trace on the screen, I got this with magic sysrq:

[15388.393579] SysRq : Show Regs
[15388.397341] Modules linked in: igb [last unloaded: scsi_wait_scan] [15388.404846]
[15388.406889] Pid: 16218, comm: kworker/4:1 Not tainted 2.6.36-rc3-net-next-igb-100810+ #2 S5520HC/S5520HC
[15388.418393] EIP: 0060:[<c13fead2>] EFLAGS: 00000297 CPU: 4
[15388.424908] EIP is at _raw_spin_lock+0x13/0x19
[15388.430257] EAX: f6eab55c EBX: f6eab380 ECX: 00000001 EDX: 00004e4a [15388.437629] ESI: f6eab000 EDI: f6eab41c EBP: f3d9bf4c ESP: f3d9bf4c [15388.445011]  DS: 007b ES: 007b FS: 00d8 GS: 0000 SS: 0068
[15388.451422] Process kworker/4:1 (pid: 16218, ti=f3d9a000 task=f5ce0ed0 task.ti=f3d9a000)
[15388.461173] Stack:
[15388.463796]  f3d9bf64 f8586c57 f3d9bf5c f6eab41c f6901e00 c8703cc0 f3d9bf90 c1041379
[15388.473116] <0> 00000004 00000000 f8586b16 00000000 c8707b05 c8707b00 f6901e00 c8703cc4
[15388.483379] <0> c8703cc0 f3d9bfb8 c1042879 c16bb640 c8703cc4 00000c54 c16bb640 f6901e10
[15388.494219] Call Trace:
[15388.497336]  [<f8586c57>] ? igb_watchdog_task+0x141/0x21a [igb]
[15388.504336]  [<c1041379>] ? process_one_work+0x18e/0x265
[15388.510643]  [<f8586b16>] ? igb_watchdog_task+0x0/0x21a [igb]
[15388.517455]  [<c1042879>] ? worker_thread+0xf3/0x1ef
[15388.523384]  [<c1042786>] ? worker_thread+0x0/0x1ef
[15388.529222]  [<c104506b>] ? kthread+0x62/0x67
[15388.534475]  [<c1045009>] ? kthread+0x0/0x67
[15388.539623]  [<c1002d36>] ? kernel_thread_helper+0x6/0x10
[15388.546034] Code: 00 75 05 f0 66 0f b1 0a 0f 94 c1 0f b6 c1 85 c0 0f 95 c0 0f b6 c0 5d c3 55 ba 00 01 00 00 89 e5 f0 66 0f c1 10 38 f2 74 06 f3 90 <8a> 10 eb f6 5d c3 55 89 e5 9c 59 fa ba 00 01 00 00 f0 66 0f c1
Quoted 3 lines

Thanks,
Emil

Hi Emil, thanks for testing.

It seems one one the u64_stats_sync invariant is not met.

I believe problem comes from "restart_queue"

This one can be updated in // by two cpus.

So we can lose one of the u64_stats_update_begin() / u64_stats_update_end() increment and freeze a reader.

So igb had a previous bug here, un-noticed :)

restart_queue can be updated by start_xmit() (so only one cpu can be there, because of txqueue lock serialization), but it also can be updated by the igb_clean_tx_irq() function (one cpu there too).

One solution to this problem is to use two separate counters, with two separate syncp.

[PATCH net-next V2] igb: fix stats handling

There are currently some problems with igb.

- On 32bit arches, maintaining 64bit counters without proper synchronization between writers and readers.

- Stats updated every two seconds, as reported by Jesper. (Jesper provided a patch for this)

- Potential problem between worker thread and ethtool -S

This patch uses u64_stats_sync, and convert everything to be 64bit safe, SMP safe, even on 32bit arches. It integrates Jesper idea of providing accurate stats at the time user reads them.

Signed-off-by Eric Dumazet <[email protected]>

---
V2: Add a second restart_queue field, with separate syncp, because original restart_queue was potentially udpated by two cpus. Corrected igb_get_ethtool_stats() to also use appropriate syncp, and do the sum of two restart_queue fields.

Quoted 5 lines

 drivers/net/igb/igb.h         |    9 ++ drivers/net/igb/igb_ethtool.c |   52 ++++++++++---- drivers/net/igb/igb_main.c    |  113 +++++++++++++++++++++++---------

 3 files changed, 129 insertions(+), 45 deletions(-)

Thanks Eric, I have added the updated patch to my queue.

Signature · 2 lines
Cheers,
Jeff
#14 lore raw
In reply to Eric Dumazet
Quoted 37 lines · Eric Dumazet
Quoted 24 lines · Jeff Kirsher
Quoted 23 lines · Jesper Dangaard Brouer
Quoted 11 lines · Eric Dumazet
Quoted 10 lines · Eric Dumazet
Quoted 6 lines · Jesper Dangaard Brouer

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);

Quoted 6 lines
Quoted 2 lines

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()?

Quoted 8 lines
Quoted 4 lines
Quoted 1 line

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

Quoted 10 lines

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.

Signature · 2 lines
Cheers,
Jeff
maintainer
#15 lore raw
In reply to Jesper Dangaard Brouer

From: Jesper Dangaard Brouer <[email protected]>
Date: Tue, 05 Oct 2010 16:18:33 +0200

Quoted 10 lines

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 <[email protected]>

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

Thanks.

#16 lore raw
Quoted 8 lines · David Miller

From: Jesper Dangaard Brouer <[email protected]>
Date: Tue, 05 Oct 2010 16:18:33 +0200

Quoted 5 lines

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.

Quoted 9 lines
Quoted 5 lines

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 <[email protected]>

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.

maintainer
#17 lore raw

From: Eric Dumazet <[email protected]>
Date: Thu, 07 Oct 2010 08:44:08 +0200

Quoted 13 lines · David Miller
Quoted 12 lines

From: Jesper Dangaard Brouer <[email protected]>
Date: Tue, 05 Oct 2010 16:18:33 +0200

Quoted 9 lines

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.

Quoted 7 lines
Quoted 5 lines
Quoted 1 line
Signed-off-by Jesper Dangaard Brouer <[email protected]>

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.

author
#18 lore raw
Quoted 16 lines · David Miller

From: Eric Dumazet <[email protected]>
Date: Thu, 07 Oct 2010 08:44:08 +0200

Quoted 13 lines · David Miller
Quoted 12 lines

From: Jesper Dangaard Brouer <[email protected]>
Date: Tue, 05 Oct 2010 16:18:33 +0200

Quoted 9 lines

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.

Quoted 8 lines
Quoted 6 lines
Quoted 4 lines
Quoted 1 line
Signed-off-by Jesper Dangaard Brouer <[email protected]>

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.

Signature · 7 lines
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
#19 lore raw
Quoted 29 lines · Jesper Dangaard Brouer
Quoted 16 lines · David Miller

From: Eric Dumazet <[email protected]>
Date: Thu, 07 Oct 2010 08:44:08 +0200

Quoted 13 lines · David Miller
Quoted 12 lines

From: Jesper Dangaard Brouer <[email protected]>
Date: Tue, 05 Oct 2010 16:18:33 +0200

Quoted 9 lines

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.

Quoted 8 lines
Quoted 6 lines
Quoted 4 lines
Quoted 1 line
Signed-off-by Jesper Dangaard Brouer <[email protected]>

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.

Quoted 8 lines

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.

author
#20 lore raw
Quoted 39 lines · Eric Dumazet
Quoted 29 lines · Jesper Dangaard Brouer
Quoted 16 lines · David Miller

From: Eric Dumazet <[email protected]>
Date: Thu, 07 Oct 2010 08:44:08 +0200

Quoted 13 lines · David Miller
Quoted 12 lines

From: Jesper Dangaard Brouer <[email protected]>
Date: Tue, 05 Oct 2010 16:18:33 +0200

Quoted 9 lines

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.

Quoted 8 lines
Quoted 6 lines
Quoted 4 lines
Quoted 1 line
Signed-off-by Jesper Dangaard Brouer <[email protected]>

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.

Quoted 8 lines

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.

Quoted 8 lines

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".

Quoted 6 lines

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.

Quoted 2 lines

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"

Signature · 7 lines
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