linux-kernel - Re: E1000: e1000_update

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <47CF136A.8070202@intel.com>
Date:	Wed, 05 Mar 2008 13:40:58 -0800
From:	"Kok, Auke" <auke-jan.h.kok@...el.com>
To:	Chris Friesen <cfriesen@...tel.com>
CC:	Todd Tomaino <ttomaino@...ulet.com>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: E1000:   e1000_update_stats()

Chris Friesen wrote:
> Todd Tomaino wrote:
> 
>> Reading the comments in the code it appeared that updating
>> adapter->net_stats and adapter->stat required interrupts to be turned
>> off to prevent the e1000_adjust_tbi_stats() function from modifying
>> these structures from interrupt context.  In order to reduce the
>> amount of time spent with interrupts disabled I separated the
>> register reads from updating adapter->stat counters.  The register
>> reads take up the bulk of the 57 usec and shouldn't require
>> protection (I could be wrong).  The new operation looks like this and
>> reduces the amount of time spent with interrupts turned off by 1/2:
>>
>> 1. read registers 2. acquire spinlock 3. update adapter->stat
>> counters 4. update OS adapter->net_stats statistics 5. update phy
>> stats. 6. release spinlock.
> 
> One potential problem with this is that the stats can be further out of
> date.
> 
> Consider this case:
> 
> cpu0 reads some of the registers, then gets preempted/interrupted
> cpu1 reads all the registers, updates the stats, then dumps the data
> 
> 
> At this point cpu1 dumped information that doesn't include the registers
> that cpu0 was in the middle of updating.  Eventually cpu0 will run again
> and the stats will be accurate, but there is a window where the stats
> may be self-inconsistent and the various counts may not add up.

the update_stats function is only called every two seconds, and scheduled once. I
think that the chance that two of these functions run in contention in the way you
point out are practically zero, and if they do then we have much bigger problems
than just counters being wrong...

the lock is there to prevent an mii-tool PHY read from interfering with the
regularly scheduled PHY reads, so we might want to keep those reads locked
together as Todd suggests himself, but not the bulk MAC reads.

Auke
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/