On Thu, Nov 5, 2015 at 2:44 PM, Arnd Bergmann arnd@arndb.de wrote:
On Thursday 05 November 2015 11:13:30 Amitoj Kaur Chawla wrote:
On Thu, Nov 5, 2015 at 5:19 AM, Arnd Bergmann arnd@arndb.de wrote:
On Wednesday 04 November 2015 23:51:07 Amitoj Kaur Chawla wrote:
32 bit systems using 'struct timeval' will break in the year 2038, so we modify the code appropriately.
This patch replaces the use of struct timeval and do_gettimeofday() with 64 bit ktime_get_seconds() since we only need to find elapsed seconds.
stats_reset_time variable has been converted from a u32 to u64 type to store the 64 bit value returned by ktime_get_seconds().
Signed-off-by: Amitoj Kaur Chawla amitoj1606@gmail.com
The changelog looks good, but you seem to have introduced a new bug:
@@ -3347,9 +3345,8 @@ __bfa_cb_fcport_stats_get(void *cbarg, bfa_boolean_t complete) union bfa_fcport_stats_u *ret;
if (complete) {
struct timeval tv;
u64 timestamp = ktime_get_seconds(); if (fcport->stats_status == BFA_STATUS_OK)
do_gettimeofday(&tv); list_for_each_safe(qe, qen, &fcport->stats_pending_q) { bfa_q_deq(&fcport->stats_pending_q, &qe);The 'if' condition now applies to the list_for_each_safe(), so the behavior of the code changes significantly.
I'll remove the if condition totally since it doesn't make sense anymore to keep it.
That works, but why not keep the code as it is here? ktime_get_seconds() still adds a little overhead if we call it unconditionally, and the author of the original code apparently felt that it was worth optimizing for.
Oh okay, I sent a v2 thinking that the removal of if condition would work. But since you're saying that the optimising seems necessary. I'll change it to u64 timestamp; if (fcport->stats_status == BFA_STATUS_OK) timestamp = ktime_get_seconds();
Is this fine?
diff --git a/drivers/scsi/bfa/bfa_svc.h b/drivers/scsi/bfa/bfa_svc.h index ef07365..0c25ee3 100644 --- a/drivers/scsi/bfa/bfa_svc.h +++ b/drivers/scsi/bfa/bfa_svc.h @@ -504,7 +504,7 @@ struct bfa_fcport_s { struct list_head stats_pending_q; struct list_head statsclr_pending_q; bfa_boolean_t stats_qfull;
u32 stats_reset_time; /* stats reset time stamp */
u64 stats_reset_time; /* stats reset time stamp */ bfa_boolean_t diag_busy; /* diag busy status */ bfa_boolean_t beacon; /* port beacon status */ bfa_boolean_t link_e2e_beacon; /* link beacon status */Let's talk about types here: u64 is technically correct, but the return type of ktime_get_seconds() is 'time64_t', so if you want to stay with 64 bits just use that.
I'll use time64_t next time.
Leaving the type as u32 is also fine as I explained earlier because monotonic times do not overflow 32-bit variables in practice.
ArndOh okay, I'll leave stats_reset_time as is.
As with every patch, it doesn't matter so much which one of those two (or possibly 'unsigned long' if you like) you pick here, as long as it's clear from the changelog that you have thought about it and made a decision one way or another. This means if someone disagrees with your choice, they just need to reply to your changelog instead of having to understand all the details.
Arnd
I'll also mention in the changelog in v3 that the stats_reset_time variable was not changed to 64 bit type since this is monotonic time.
Works?
Thanks,