@@ -298,7 +298,6 @@ struct be_adapter {u8__iomem*csr;u8__iomem*db;/* Door Bell */-u8__iomem*pcicfg;/* PCI config space */structmutexmbox_lock;/* For serializing mbox cmds to BE card */structbe_dma_memmbox_mem;
There is a possibility of be_post_rx_frags() being called simultaneously from
both be_worker() (when rx_post_starved) and be_poll_rx() (when rxq->used is 0).
This can be avoided by posting rx buffers only when some completions have been
reaped.
Signed-off-by: Sathya Perla <redacted>
---
drivers/net/ethernet/emulex/benet/be_main.c | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
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(-)
@@ -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];+for_all_rx_queues(adapter,rxo,i){+/* below erx HW counter can actually wrap around after+*65535.Driveraccumulatesa32-bitvalue+*/+accumulate_16bit_val(&rx_stats(rxo)->rx_drops_no_frags,+(u16)erx->rx_drops_no_fragments[rxo->q.id]);+}}staticstructrtnl_link_stats64*be_get_stats64(structnet_device*netdev,
Flashing some of the PHYs can take longer thus increasing the total flash
update time to a max of 40s.
Signed-off-by: Sathya Perla <redacted>
---
drivers/net/ethernet/emulex/benet/be_cmds.c | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
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
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(-)
@@ -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
+ 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;
-----Original Message-----
From: Eric Dumazet [mailto:eric.dumazet@gmail.com]
Sent: Tuesday, August 23, 2011 12:11 PM
Le mardi 23 août 2011 à 11:11 +0530, Sathya Perla a écrit :
quoted
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
@@ -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.
Yes, the new code still *does not* do a sum of all queues. The code just
parses per-queue stats. For each queue, the 16 bit
HW stats value is taken and stored in a 32-bit *per-queue* variable.
The comment that "driver accumulates a 32-bit val" may be misleading. The
code here is not doing a sum of the per-queue stat.
+ 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]);
+ }
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
+ 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 ?
The following logic should take care of that:
u32 newacc = hi(*acc) + val;
Only the upper 16 bits of the accumulator are retained and the new-val
of the 16-bit stat is used for the lower 16 bits. So, even if I call this
routine 10 times with the same value for val, the accumulator value will not change.
Say: accumulator is 0x00010001
Say: new 16-bit hw counter value is now 2 (it was previously 1 and had wraped-aroud before that)
accumulate(acc, 2) ==> newacc = 0x00010000 + 2 = 0x00010002
any number of calls to accumulate(acc, 2) will still produce acc = 0x00010002
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.
I don't agree:
Say: accumulator is 0x0000FFFF
Say: new 16-bit hw counter value is now 0 (after a wrap-aroud)
accumulate(acc, 0) ==> newacc = 0x00000000 + 0 = 0x00000000
After the wraparound check newacc = 0x00010000
From: Eric Dumazet <hidden> Date: 2011-08-23 07:29:43
Le mardi 23 août 2011 à 00:06 -0700, Sathya.Perla@Emulex.Com a écrit :
quoted
-----Original Message-----
From: Eric Dumazet [mailto:eric.dumazet@gmail.com]
Sent: Tuesday, August 23, 2011 12:11 PM
Le mardi 23 août 2011 à 11:11 +0530, Sathya Perla a écrit :
quoted
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
@@ -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.
Yes, the new code still *does not* do a sum of all queues. The code just
parses per-queue stats. For each queue, the 16 bit
HW stats value is taken and stored in a 32-bit *per-queue* variable.
The comment that "driver accumulates a 32-bit val" may be misleading. The
code here is not doing a sum of the per-queue stat.
Ah ok, I now see the code intent, and everything seems fine now, thanks
for explaining.