Re: [PATCH net-next 3/5] be2net: fix erx->rx_drops_no_frags wrap around
flat view
From: Eric Dumazet <hidden>
Date: 2011-08-23 06:41:23
Le mardi 23 août 2011 à 11:11 +0530, Sathya Perla a écrit :
quoted hunk ↗ jump to hunk
The rx_drops_no_frags HW counter for RSS rings is 16bits in HW and can wraparound often. Maintain a 32-bit accumulator in the driver to prevent frequent wraparound. Also, incorporated Eric's feedback to use ACCESS_ONCE() for the accumulator write. Signed-off-by: Sathya Perla <redacted> --- drivers/net/ethernet/emulex/benet/be_main.c | 22 +++++++++++++++++++--- 1 files changed, 19 insertions(+), 3 deletions(-)diff --git a/drivers/net/ethernet/emulex/benet/be_main.c b/drivers/net/ethernet/emulex/benet/be_main.c index 2375c0c..fb2eda0 100644 --- a/drivers/net/ethernet/emulex/benet/be_main.c +++ b/drivers/net/ethernet/emulex/benet/be_main.c@@ -378,6 +378,18 @@ static void populate_lancer_stats(struct be_adapter *adapter) pport_stats->rx_drops_too_many_frags_lo; } +static void accumulate_16bit_val(u32 *acc, u16 val) +{ +#define lo(x) (x & 0xFFFF) +#define hi(x) (x & 0xFFFF0000) + bool wrapped = val < lo(*acc); + u32 newacc = hi(*acc) + val; + + if (wrapped) + newacc += 65536; + ACCESS_ONCE(*acc) = newacc; +} +
I still feel something is wrong here :
quoted hunk ↗ jump to hunk
void be_parse_stats(struct be_adapter *adapter) { struct be_erx_stats_v1 *erx = be_erx_stats_from_cmd(adapter);@@ -394,9 +406,13 @@ void be_parse_stats(struct be_adapter *adapter) } /* as erx_v1 is longer than v0, ok to use v1 defn for v0 access */ - for_all_rx_queues(adapter, rxo, i) - rx_stats(rxo)->rx_drops_no_frags = - erx->rx_drops_no_fragments[rxo->q.id];
previous code was not doing a sum_of_all_queues. It only gave the final erx->rx_drops_no_fragments[rxo->q.id], not taking into account previous rx_stats(rxo)->rx_drops_no_frags value. Your changelog is about wrap around, while the bug might have be different (No real sum) Now you say : previous value is meaningfull, and we add to it 16bits values.
quoted hunk ↗ jump to hunk
+ for_all_rx_queues(adapter, rxo, i) { + /* below erx HW counter can actually wrap around after + * 65535. Driver accumulates a 32-bit value + */ + accumulate_16bit_val(&rx_stats(rxo)->rx_drops_no_frags, + (u16)erx->rx_drops_no_fragments[rxo->q.id]); + } }
Arent multiple calls to be_parse_stats() will have wrong final
rx_drops_no_frags value ?
Or are the rx_drops_no_fragments[rxo->q.id] cleared when read ?
I am afraid that if HW maintains 16bit values, then the only way is to
also have a 16bit accumulator. You cannot detect wraparounds without
also keeping a copy of previous 16bit samples.
u16 accum = 0;
or_all_rx_queues(adapter, rxo, i) {
accum += erx->rx_drops_no_fragments[rxo->q.id];
}
rx_stats(rxo)->rx_drops_no_frags = accum;