diff mbox

Lucid SRU: UBUNTU: SAUCE: igb: Protect stats update

Message ID 4E79E9EB.2060302@canonical.com
State New
Headers show

Commit Message

Tim Gardner Sept. 21, 2011, 1:43 p.m. UTC 
This patch is a partial backport of upstream 
12dcd86b75d571772512676ab301279952efc0b0. Its purpose is twofold; 1) 
Protect simultaneous readers and writers, 2) refresh statistics when 
read from ethtool.

rtg

Comments

Stefan Bader Sept. 21, 2011, 2:29 p.m. UTC | #1 
On 21.09.2011 15:43, Tim Gardner wrote:
> This patch is a partial backport of upstream
> 12dcd86b75d571772512676ab301279952efc0b0. Its purpose is twofold; 1) Protect
> simultaneous readers and writers, 2) refresh statistics when read from ethtool.
> 
> rtg
> 
Wondering whether there is any chance of a running user task holding the
spinlock by that watchdog work...

Otherwise I guess it should ensure more regular task updates and the fencing of
consumers...
Tim Gardner Sept. 21, 2011, 2:43 p.m. UTC | #2 
On 09/21/2011 08:29 AM, Stefan Bader wrote:
> On 21.09.2011 15:43, Tim Gardner wrote:
>> This patch is a partial backport of upstream
>> 12dcd86b75d571772512676ab301279952efc0b0. Its purpose is twofold; 1) Protect
>> simultaneous readers and writers, 2) refresh statistics when read from ethtool.
>>
>> rtg
>>
> Wondering whether there is any chance of a running user task holding the
> spinlock by that watchdog work...
>

If by 'running user task' you mean the ethtool interface, then yes the 
lock _should_ be held whilst the statistics update is in progress.

> Otherwise I guess it should ensure more regular task updates and the fencing of
> consumers...

rtg
Stefan Bader Sept. 21, 2011, 2:48 p.m. UTC | #3 
On 21.09.2011 16:43, Tim Gardner wrote:
> On 09/21/2011 08:29 AM, Stefan Bader wrote:
>> On 21.09.2011 15:43, Tim Gardner wrote:
>>> This patch is a partial backport of upstream
>>> 12dcd86b75d571772512676ab301279952efc0b0. Its purpose is twofold; 1) Protect
>>> simultaneous readers and writers, 2) refresh statistics when read from ethtool.
>>>
>>> rtg
>>>
>> Wondering whether there is any chance of a running user task holding the
>> spinlock by that watchdog work...
>>
> 
> If by 'running user task' you mean the ethtool interface, then yes the lock
> _should_ be held whilst the statistics update is in progress.
>
Yeah, no (if that makes any sense). I was more wondering whether the lock may be
tried to be taken from the watchdog task (scheduled_work) while a user-space
process does something like ethtool on the same cpu...

>> Otherwise I guess it should ensure more regular task updates and the fencing of
>> consumers...
> 
> rtg
Tim Gardner Sept. 21, 2011, 2:56 p.m. UTC | #4 
On 09/21/2011 08:48 AM, Stefan Bader wrote:
> On 21.09.2011 16:43, Tim Gardner wrote:
>> On 09/21/2011 08:29 AM, Stefan Bader wrote:
>>> On 21.09.2011 15:43, Tim Gardner wrote:
>>>> This patch is a partial backport of upstream
>>>> 12dcd86b75d571772512676ab301279952efc0b0. Its purpose is twofold; 1) Protect
>>>> simultaneous readers and writers, 2) refresh statistics when read from ethtool.
>>>>
>>>> rtg
>>>>
>>> Wondering whether there is any chance of a running user task holding the
>>> spinlock by that watchdog work...
>>>
>>
>> If by 'running user task' you mean the ethtool interface, then yes the lock
>> _should_ be held whilst the statistics update is in progress.
>>
> Yeah, no (if that makes any sense). I was more wondering whether the lock may be
> tried to be taken from the watchdog task (scheduled_work) while a user-space
> process does something like ethtool on the same cpu...
>

That could only happen if CONFIG_PREEMPT=y, in which case the spin lock 
logic should do the right thing.

rtg
John Johansen Sept. 23, 2011, 11:17 a.m. UTC | #5 
On 09/21/2011 07:56 AM, Tim Gardner wrote:
> On 09/21/2011 08:48 AM, Stefan Bader wrote:
>> On 21.09.2011 16:43, Tim Gardner wrote:
>>> On 09/21/2011 08:29 AM, Stefan Bader wrote:
>>>> On 21.09.2011 15:43, Tim Gardner wrote:
>>>>> This patch is a partial backport of upstream
>>>>> 12dcd86b75d571772512676ab301279952efc0b0. Its purpose is twofold; 1) Protect
>>>>> simultaneous readers and writers, 2) refresh statistics when read from ethtool.
>>>>>
>>>>> rtg
>>>>>
>>>> Wondering whether there is any chance of a running user task holding the
>>>> spinlock by that watchdog work...
>>>>
>>>
>>> If by 'running user task' you mean the ethtool interface, then yes the lock
>>> _should_ be held whilst the statistics update is in progress.
>>>
>> Yeah, no (if that makes any sense). I was more wondering whether the lock may be
>> tried to be taken from the watchdog task (scheduled_work) while a user-space
>> process does something like ethtool on the same cpu...
>>
> 
> That could only happen if CONFIG_PREEMPT=y, in which case the spin lock logic should do the right thing.
> 
Yep it looks good.

Acked-by: John Johansen <john.johansen@canonical.com>
Stefan Bader Sept. 23, 2011, 12:36 p.m. UTC | #6 
On 21.09.2011 16:56, Tim Gardner wrote:
> On 09/21/2011 08:48 AM, Stefan Bader wrote:
>> On 21.09.2011 16:43, Tim Gardner wrote:
>>> On 09/21/2011 08:29 AM, Stefan Bader wrote:
>>>> On 21.09.2011 15:43, Tim Gardner wrote:
>>>>> This patch is a partial backport of upstream
>>>>> 12dcd86b75d571772512676ab301279952efc0b0. Its purpose is twofold; 1) Protect
>>>>> simultaneous readers and writers, 2) refresh statistics when read from
>>>>> ethtool.
>>>>>
>>>>> rtg
>>>>>
>>>> Wondering whether there is any chance of a running user task holding the
>>>> spinlock by that watchdog work...
>>>>
>>>
>>> If by 'running user task' you mean the ethtool interface, then yes the lock
>>> _should_ be held whilst the statistics update is in progress.
>>>
>> Yeah, no (if that makes any sense). I was more wondering whether the lock may be
>> tried to be taken from the watchdog task (scheduled_work) while a user-space
>> process does something like ethtool on the same cpu...
>>
> 
> That could only happen if CONFIG_PREEMPT=y, in which case the spin lock logic
> should do the right thing.
> 
> rtg

With this mornigns discussion I' ack it as well. Just seems already applied
Tim Gardner Sept. 23, 2011, 12:44 p.m. UTC | #7 
On 09/23/2011 06:36 AM, Stefan Bader wrote:
> On 21.09.2011 16:56, Tim Gardner wrote:
>> On 09/21/2011 08:48 AM, Stefan Bader wrote:
>>> On 21.09.2011 16:43, Tim Gardner wrote:
>>>> On 09/21/2011 08:29 AM, Stefan Bader wrote:
>>>>> On 21.09.2011 15:43, Tim Gardner wrote:
>>>>>> This patch is a partial backport of upstream
>>>>>> 12dcd86b75d571772512676ab301279952efc0b0. Its purpose is twofold; 1) Protect
>>>>>> simultaneous readers and writers, 2) refresh statistics when read from
>>>>>> ethtool.
>>>>>>
>>>>>> rtg
>>>>>>
>>>>> Wondering whether there is any chance of a running user task holding the
>>>>> spinlock by that watchdog work...
>>>>>
>>>>
>>>> If by 'running user task' you mean the ethtool interface, then yes the lock
>>>> _should_ be held whilst the statistics update is in progress.
>>>>
>>> Yeah, no (if that makes any sense). I was more wondering whether the lock may be
>>> tried to be taken from the watchdog task (scheduled_work) while a user-space
>>> process does something like ethtool on the same cpu...
>>>
>>
>> That could only happen if CONFIG_PREEMPT=y, in which case the spin lock logic
>> should do the right thing.
>>
>> rtg
>
> With this mornigns discussion I' ack it as well. Just seems already applied
>

It got applied by accident. Now reapplied with proper acknowledgements.
diff mbox

Patch

From 805de5028e59d721c2f6058a16399db1e8c434f5 Mon Sep 17 00:00:00 2001
From: Tim Gardner <tim.gardner@canonical.com>
Date: Tue, 20 Sep 2011 14:06:12 -0600
Subject: [PATCH] UBUNTU: SAUCE: igb: Protect stats update

BugLink: http://bugs.launchpad.net/bugs/829566

Partial backport from upstream 12dcd86b75d571772512676ab301279952efc0b0

The stats structure was improperly protected against simultaneous
reads and writes. Also update stats at the time the user reads
them.

Signed-off-by: Tim Gardner <tim.gardner@canonical.com>
---
 drivers/net/igb/igb.h         |    2 ++
 drivers/net/igb/igb_ethtool.c |    2 ++
 drivers/net/igb/igb_main.c    |   16 ++++++++++++++--
 3 files changed, 18 insertions(+), 2 deletions(-)

diff --git a/drivers/net/igb/igb.h b/drivers/net/igb/igb.h
index b1c1eb8..ce42798 100644
--- a/drivers/net/igb/igb.h
+++ b/drivers/net/igb/igb.h
@@ -284,6 +284,8 @@  struct igb_adapter {
 	struct timecompare compare;
 	struct hwtstamp_config hwtstamp_config;
 
+	spinlock_t stats_lock;
+
 	/* structs defined in e1000_hw.h */
 	struct e1000_hw hw;
 	struct e1000_hw_stats stats;
diff --git a/drivers/net/igb/igb_ethtool.c b/drivers/net/igb/igb_ethtool.c
index 550ad93..5bda682 100644
--- a/drivers/net/igb/igb_ethtool.c
+++ b/drivers/net/igb/igb_ethtool.c
@@ -1992,6 +1992,7 @@  static void igb_get_ethtool_stats(struct net_device *netdev,
 	int i, j, k;
 	char *p;
 
+	spin_lock(&adapter->stats_lock);
 	igb_update_stats(adapter);
 
 	for (i = 0; i < IGB_GLOBAL_STATS_LEN; i++) {
@@ -2014,6 +2015,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->stats_lock);
 }
 
 static void igb_get_strings(struct net_device *netdev, u32 stringset, u8 *data)
diff --git a/drivers/net/igb/igb_main.c b/drivers/net/igb/igb_main.c
index 4370842..c159b93 100644
--- a/drivers/net/igb/igb_main.c
+++ b/drivers/net/igb/igb_main.c
@@ -1191,7 +1191,9 @@  void igb_down(struct igb_adapter *adapter)
 	netif_carrier_off(netdev);
 
 	/* record the stats before reset*/
+	spin_lock(&adapter->stats_lock);
 	igb_update_stats(adapter);
+	spin_unlock(&adapter->stats_lock);
 
 	adapter->link_speed = 0;
 	adapter->link_duplex = 0;
@@ -1534,6 +1536,8 @@  static int __devinit igb_probe(struct pci_dev *pdev,
 		goto err_eeprom;
 	}
 
+	spin_lock_init(&adapter->stats_lock);
+
 	setup_timer(&adapter->watchdog_timer, &igb_watchdog,
 	            (unsigned long) adapter);
 	setup_timer(&adapter->phy_info_timer, &igb_update_phy_info,
@@ -3120,7 +3124,10 @@  static void igb_watchdog_task(struct work_struct *work)
 		}
 	}
 
+	spin_lock(&adapter->stats_lock);
 	igb_update_stats(adapter);
+	spin_unlock(&adapter->stats_lock);
+
 	igb_update_adaptive(hw);
 
 	for (i = 0; i < adapter->num_tx_queues; i++) {
@@ -3856,7 +3863,12 @@  static void igb_reset_task(struct work_struct *work)
  **/
 static struct net_device_stats *igb_get_stats(struct net_device *netdev)
 {
-	/* only return the current stats */
+	struct igb_adapter *adapter = netdev_priv(netdev);
+
+	spin_lock(&adapter->stats_lock);
+	igb_update_stats(adapter);
+	spin_unlock(&adapter->stats_lock);
+
 	return &netdev->stats;
 }
 
@@ -3930,7 +3942,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;
-- 
1.7.0.4