Re: E1000: e1000_update_stats()

["Kok, Auke" <auke-jan.h.kok@intel.com>]

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