On 7/11/11 5:17 AM, Hillf Danton wrote:
On Sun, Jul 10, 2011 at 4:14 PM, Joe Eykholt<[email protected]> wrote:
On 7/9/11 7:46 PM, Hillf Danton wrote:
When receiving data, if CRC error encountered we have to do stats for the
event.
It is PER CPU operation, and opeartions related to preempt could be
dropped.
Any comment is appreciated.
Thanks
Signed-off-by: Hillf Danton<[email protected]>
---
drivers/scsi/libfc/fc_fcp.c | 4 ++--
1 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/scsi/libfc/fc_fcp.c b/drivers/scsi/libfc/fc_fcp.c
index 9cd2149..e14fc3b 100644
--- a/drivers/scsi/libfc/fc_fcp.c
+++ b/drivers/scsi/libfc/fc_fcp.c
@@ -495,14 +495,14 @@ static void fc_fcp_recv_data(struct fc_fcp_pkt
*fsp, struct fc_frame *fp)
if (~crc != le32_to_cpu(fr_crc(fp))) {
crc_err:
- stats = per_cpu_ptr(lport->dev_stats, get_cpu());
+ stats = per_cpu_ptr(lport->dev_stats,
smp_processor_id());
At this point you could be preempted, so ...
stats->ErrorFrames++;
could be incrementing the ErrorFrames count for the CPU you *used* to be on,
not the CPU you're actually running on, so, the get_cpu(), put_cpu() were
necessary. It's not very often that you'll have CRC errors, probably, but
even so, it's good to be correct when using per-CPU variables.
Yeah, CRC error is rare.
But the comment just above fc_fcp_recv() says it is in Soft IRQ context, then
there is no window opened for preempt, I think.
You may be right. I don't remember if there are other paths not in Soft IRQ
or not. Just because a comment says it is called in Soft IRQ, doesn't mean
that it's *only* called that way. It seems like the callback from
lport->tt.fcp_cmd_send() might want to be at thread level in some cases.
Maybe more documentation is necessary there to document the conditions.
It doesn't seem worth it to make this change, though. It does make things
a bit smaller, but no faster, and not safer.
Regards,
Joe
_______________________________________________
devel mailing list
[email protected]
https://lists.open-fcoe.org/mailman/listinfo/devel
