Re: E1000: e1000_update_stats()

[Chris Friesen]

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.

Chris

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