This patchset removes the use of struct timeval because struct timeval will break in 32 bit systems in the year 2038.
Amitoj Kaur Chawla(2): scsi: bfa: bfa_svc: Use ktime_get_real_seconds() scsi: bfa: bfa_svc: Remove use of struct timeval
drivers/scsi/bfa/bfa_svc.c | 30 ++++++++---------------------- drivers/scsi/bfa/bfa_svc.h | 2 +- 2 files changed, 9 insertions(+), 23 deletions(-)
32 bit systems using 'struct timeval' will break in the year 2038, so we modify the code appropriately.
This patch removes the function bfa_get_log_time() which is only used to get current time in seconds. bfa_get_log_time() is replaced with ktime_get_real_seconds() which returns a 64 bit value.
Signed-off-by: Amitoj Kaur Chawla amitoj1606@gmail.com --- drivers/scsi/bfa/bfa_svc.c | 21 +++++---------------- 1 file changed, 5 insertions(+), 16 deletions(-)
diff --git a/drivers/scsi/bfa/bfa_svc.c b/drivers/scsi/bfa/bfa_svc.c index 625225f..b3668e9 100644 --- a/drivers/scsi/bfa/bfa_svc.c +++ b/drivers/scsi/bfa/bfa_svc.c @@ -15,6 +15,7 @@ * General Public License for more details. */
+#include <linux/ktime.h> #include "bfad_drv.h" #include "bfad_im.h" #include "bfa_plog.h" @@ -303,18 +304,6 @@ plkd_validate_logrec(struct bfa_plog_rec_s *pl_rec) return 0; }
-static u64 -bfa_get_log_time(void) -{ - u64 system_time = 0; - struct timeval tv; - do_gettimeofday(&tv); - - /* We are interested in seconds only. */ - system_time = tv.tv_sec; - return system_time; -} - static void bfa_plog_add(struct bfa_plog_s *plog, struct bfa_plog_rec_s *pl_rec) { @@ -335,7 +324,7 @@ bfa_plog_add(struct bfa_plog_s *plog, struct bfa_plog_rec_s *pl_rec)
memcpy(pl_recp, pl_rec, sizeof(struct bfa_plog_rec_s));
- pl_recp->tv = bfa_get_log_time(); + pl_recp->tv = ktime_get_real_seconds(); BFA_PL_LOG_REC_INCR(plog->tail);
if (plog->head == plog->tail) @@ -6278,7 +6267,7 @@ bfa_fcdiag_lb_is_running(struct bfa_s *bfa) * D-port */ #define bfa_dport_result_start(__dport, __mode) do { \ - (__dport)->result.start_time = bfa_get_log_time(); \ + (__dport)->result.start_time = ktime_get_real_seconds();\ (__dport)->result.status = DPORT_TEST_ST_INPRG; \ (__dport)->result.mode = (__mode); \ (__dport)->result.rp_pwwn = (__dport)->rp_pwwn; \ @@ -6717,7 +6706,7 @@ bfa_dport_scn(struct bfa_dport_s *dport, struct bfi_diag_dport_scn_s *msg)
switch (dport->i2hmsg.scn.state) { case BFI_DPORT_SCN_TESTCOMP: - dport->result.end_time = bfa_get_log_time(); + dport->result.end_time = ktime_get_real_seconds(); bfa_trc(dport->bfa, dport->result.end_time);
dport->result.status = msg->info.testcomp.status; @@ -6764,7 +6753,7 @@ bfa_dport_scn(struct bfa_dport_s *dport, struct bfi_diag_dport_scn_s *msg) case BFI_DPORT_SCN_SUBTESTSTART: subtesttype = msg->info.teststart.type; dport->result.subtest[subtesttype].start_time = - bfa_get_log_time(); + ktime_get_real_seconds(); dport->result.subtest[subtesttype].status = DPORT_TEST_ST_INPRG;
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 --- drivers/scsi/bfa/bfa_svc.c | 9 +++------ drivers/scsi/bfa/bfa_svc.h | 2 +- 2 files changed, 4 insertions(+), 7 deletions(-)
diff --git a/drivers/scsi/bfa/bfa_svc.c b/drivers/scsi/bfa/bfa_svc.c index b3668e9..de18153 100644 --- a/drivers/scsi/bfa/bfa_svc.c +++ b/drivers/scsi/bfa/bfa_svc.c @@ -3081,7 +3081,6 @@ bfa_fcport_attach(struct bfa_s *bfa, void *bfad, struct bfa_iocfc_cfg_s *cfg, struct bfa_fcport_s *fcport = BFA_FCPORT_MOD(bfa); struct bfa_port_cfg_s *port_cfg = &fcport->cfg; struct bfa_fcport_ln_s *ln = &fcport->ln; - struct timeval tv;
fcport->bfa = bfa; ln->fcport = fcport; @@ -3094,8 +3093,7 @@ bfa_fcport_attach(struct bfa_s *bfa, void *bfad, struct bfa_iocfc_cfg_s *cfg, /* * initialize time stamp for stats reset */ - do_gettimeofday(&tv); - fcport->stats_reset_time = tv.tv_sec; + fcport->stats_reset_time = ktime_get_seconds(); fcport->stats_dma_ready = BFA_FALSE;
/* @@ -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); @@ -3364,7 +3361,7 @@ __bfa_cb_fcport_stats_get(void *cbarg, bfa_boolean_t complete) bfa_fcport_fcoe_stats_swap(&ret->fcoe, &fcport->stats->fcoe); ret->fcoe.secs_reset = - tv.tv_sec - fcport->stats_reset_time; + timestamp - fcport->stats_reset_time; } } bfa_cb_queue_status(fcport->bfa, &cb->hcb_qe, 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 */
On Wednesday 04 November 2015 23:47:03 Amitoj Kaur Chawla wrote:
32 bit systems using 'struct timeval' will break in the year 2038, so we modify the code appropriately.
This patch removes the function bfa_get_log_time() which is only used to get current time in seconds. bfa_get_log_time() is replaced with ktime_get_real_seconds() which returns a 64 bit value.
Signed-off-by: Amitoj Kaur Chawla amitoj1606@gmail.com
Looks good. Please remember to put the outreachy mailing list on Cc as well for those patches.
You can post this patch to the maintainers for the driver now.
arnd
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();
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.
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.
Leaving the type as u32 is also fine as I explained earlier because monotonic times do not overflow 32-bit variables in practice.
Arnd
On Thu, Nov 5, 2015 at 5:11 AM, Arnd Bergmann arnd@arndb.de wrote:
On Wednesday 04 November 2015 23:47:03 Amitoj Kaur Chawla wrote:
32 bit systems using 'struct timeval' will break in the year 2038, so we modify the code appropriately.
This patch removes the function bfa_get_log_time() which is only used to get current time in seconds. bfa_get_log_time() is replaced with ktime_get_real_seconds() which returns a 64 bit value.
Signed-off-by: Amitoj Kaur Chawla amitoj1606@gmail.com
Looks good. Please remember to put the outreachy mailing list on Cc as well for those patches.
You can post this patch to the maintainers for the driver now.
arnd
Since Julia and Greg both mentioned not sending patches on the outreachy mailing list anymore I didn't send these there.
If you think I should, I will take care of it next time.
Also I'll correct the second patch in this series and send the series to the maintainer after you feel the second is correct since the second patch is changing the same file.
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.
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.
Arnd
Oh okay, I'll leave stats_reset_time as is.
On Thu, 5 Nov 2015, Amitoj Kaur Chawla wrote:
On Thu, Nov 5, 2015 at 5:11 AM, Arnd Bergmann arnd@arndb.de wrote:
On Wednesday 04 November 2015 23:47:03 Amitoj Kaur Chawla wrote:
32 bit systems using 'struct timeval' will break in the year 2038, so we modify the code appropriately.
This patch removes the function bfa_get_log_time() which is only used to get current time in seconds. bfa_get_log_time() is replaced with ktime_get_real_seconds() which returns a 64 bit value.
Signed-off-by: Amitoj Kaur Chawla amitoj1606@gmail.com
Looks good. Please remember to put the outreachy mailing list on Cc as well for those patches.
You can post this patch to the maintainers for the driver now.
arndSince Julia and Greg both mentioned not sending patches on the outreachy mailing list anymore I didn't send these there.
If you think I should, I will take care of it next time.
No, you did the right thing.
julia
Also I'll correct the second patch in this series and send the series to the maintainer after you feel the second is correct since the second patch is changing the same file.
-- Amitoj _______________________________________________ Y2038 mailing list Y2038@lists.linaro.org https://lists.linaro.org/mailman/listinfo/y2038
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.
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
On Thursday 05 November 2015 08:19:49 Julia Lawall wrote:
On Thu, 5 Nov 2015, Amitoj Kaur Chawla wrote:
On Thu, Nov 5, 2015 at 5:11 AM, Arnd Bergmann arnd@arndb.de wrote:
On Wednesday 04 November 2015 23:47:03 Amitoj Kaur Chawla wrote:
32 bit systems using 'struct timeval' will break in the year 2038, so we modify the code appropriately.
This patch removes the function bfa_get_log_time() which is only used to get current time in seconds. bfa_get_log_time() is replaced with ktime_get_real_seconds() which returns a 64 bit value.
Signed-off-by: Amitoj Kaur Chawla amitoj1606@gmail.com
Looks good. Please remember to put the outreachy mailing list on Cc as well for those patches.
You can post this patch to the maintainers for the driver now.
arndSince Julia and Greg both mentioned not sending patches on the outreachy mailing list anymore I didn't send these there.
If you think I should, I will take care of it next time.
No, you did the right thing.
Ok, my mistake, sorry about this.
Arnd
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,
On Thursday 05 November 2015 19:00:36 Amitoj Kaur Chawla wrote:
On Thu, Nov 5, 2015 at 2:44 PM, Arnd Bergmann arnd@arndb.de wrote:
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?
Yes, that looks ok. I'd probably use 'time64_t' instead of 'u64', and 'long' would also work just as well here because it is monotonic time.
Regarding the optimization, I think it is not an important one and it could indeed be removed, but only if you can prove that the code without the optimization is as efficient in practice, and explain that in the changelog text. It's usually easier not to change it then, so you don't have to spend the time figuring out whether that is an improvement or not.
Arnd
On Thu, Nov 5, 2015 at 8:31 PM, Arnd Bergmann arnd@arndb.de wrote:
On Thursday 05 November 2015 19:00:36 Amitoj Kaur Chawla wrote:
On Thu, Nov 5, 2015 at 2:44 PM, Arnd Bergmann arnd@arndb.de wrote:
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?
Yes, that looks ok. I'd probably use 'time64_t' instead of 'u64', and 'long' would also work just as well here because it is monotonic time.
Regarding the optimization, I think it is not an important one and it could indeed be removed, but only if you can prove that the code without the optimization is as efficient in practice, and explain that in the changelog text. It's usually easier not to change it then, so you don't have to spend the time figuring out whether that is an improvement or not.
Arnd
Now, it gives me a warning since the if condition makes it a possibility that timestamp may be uninitialised for the later operation in the code.
Not understanding, shouldn't it have given a warning when declared also, as 'struct timeval tv' since do_gettimeofday() was also intialising 'tv' only when the if condition held true.
On Thursday 05 November 2015 20:43:02 Amitoj Kaur Chawla wrote:
On Thu, Nov 5, 2015 at 8:31 PM, Arnd Bergmann arnd@arndb.de wrote:
On Thursday 05 November 2015 19:00:36 Amitoj Kaur Chawla wrote:
On Thu, Nov 5, 2015 at 2:44 PM, Arnd Bergmann arnd@arndb.de wrote:
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?
Yes, that looks ok. I'd probably use 'time64_t' instead of 'u64', and 'long' would also work just as well here because it is monotonic time.
Regarding the optimization, I think it is not an important one and it could indeed be removed, but only if you can prove that the code without the optimization is as efficient in practice, and explain that in the changelog text. It's usually easier not to change it then, so you don't have to spend the time figuring out whether that is an improvement or not.
Now, it gives me a warning since the if condition makes it a possibility that timestamp may be uninitialised for the later operation in the code.
Not understanding, shouldn't it have given a warning when declared also, as 'struct timeval tv' since do_gettimeofday() was also intialising 'tv' only when the if condition held true.
This is an unreliable warning from gcc, and it sometimes gets things wrong here for no clear reason. It probably cannot prove that fcport->stats_status has not been modified between the two checks.
Make sure that the code is actually correct. You can probably avoid the warning by introducing a local variable like
bool status_ok = (fcport->stats_status == BFA_STATUS_OK); if (status_ok) timestamp = ktime_get_seconds(); ...
Arnd
On Thu, Nov 5, 2015 at 8:52 PM, Arnd Bergmann arnd@arndb.de wrote:
On Thursday 05 November 2015 20:43:02 Amitoj Kaur Chawla wrote:
On Thu, Nov 5, 2015 at 8:31 PM, Arnd Bergmann arnd@arndb.de wrote:
On Thursday 05 November 2015 19:00:36 Amitoj Kaur Chawla wrote:
On Thu, Nov 5, 2015 at 2:44 PM, Arnd Bergmann arnd@arndb.de wrote:
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?
Yes, that looks ok. I'd probably use 'time64_t' instead of 'u64', and 'long' would also work just as well here because it is monotonic time.
Regarding the optimization, I think it is not an important one and it could indeed be removed, but only if you can prove that the code without the optimization is as efficient in practice, and explain that in the changelog text. It's usually easier not to change it then, so you don't have to spend the time figuring out whether that is an improvement or not.
Now, it gives me a warning since the if condition makes it a possibility that timestamp may be uninitialised for the later operation in the code.
Not understanding, shouldn't it have given a warning when declared also, as 'struct timeval tv' since do_gettimeofday() was also intialising 'tv' only when the if condition held true.
This is an unreliable warning from gcc, and it sometimes gets things wrong here for no clear reason. It probably cannot prove that fcport->stats_status has not been modified between the two checks.
Make sure that the code is actually correct. You can probably avoid the warning by introducing a local variable like
bool status_ok = (fcport->stats_status == BFA_STATUS_OK); if (status_ok) timestamp = ktime_get_seconds(); ... Arnd
Oh okay.
Thanks,
On Thu, Nov 5, 2015 at 8:52 PM, Arnd Bergmann arnd@arndb.de wrote:
On Thursday 05 November 2015 20:43:02 Amitoj Kaur Chawla wrote:
On Thu, Nov 5, 2015 at 8:31 PM, Arnd Bergmann arnd@arndb.de wrote:
On Thursday 05 November 2015 19:00:36 Amitoj Kaur Chawla wrote:
On Thu, Nov 5, 2015 at 2:44 PM, Arnd Bergmann arnd@arndb.de wrote:
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?
Yes, that looks ok. I'd probably use 'time64_t' instead of 'u64', and 'long' would also work just as well here because it is monotonic time.
Regarding the optimization, I think it is not an important one and it could indeed be removed, but only if you can prove that the code without the optimization is as efficient in practice, and explain that in the changelog text. It's usually easier not to change it then, so you don't have to spend the time figuring out whether that is an improvement or not.
Now, it gives me a warning since the if condition makes it a possibility that timestamp may be uninitialised for the later operation in the code.
Not understanding, shouldn't it have given a warning when declared also, as 'struct timeval tv' since do_gettimeofday() was also intialising 'tv' only when the if condition held true.
This is an unreliable warning from gcc, and it sometimes gets things wrong here for no clear reason. It probably cannot prove that fcport->stats_status has not been modified between the two checks.
Make sure that the code is actually correct. You can probably avoid the warning by introducing a local variable like
bool status_ok = (fcport->stats_status == BFA_STATUS_OK); if (status_ok) timestamp = ktime_get_seconds(); ... Arnd
I just tried the introduction of the new variable and it doesn't remove the warning so I'll skip the new variable and send the patch without it, since the warning is unreliable.
On Thursday 05 November 2015 21:00:22 Amitoj Kaur Chawla wrote:
Make sure that the code is actually correct. You can probably avoid the warning by introducing a local variable like
bool status_ok = (fcport->stats_status == BFA_STATUS_OK); if (status_ok) timestamp = ktime_get_seconds(); ... ArndI just tried the introduction of the new variable and it doesn't remove the warning so I'll skip the new variable and send the patch without it, since the warning is unreliable.
Just to clarify: you have to check that local variable in both places for gcc to know that this is the same one as before.
If that doesn't work, you may have to unconditionally initialize the timestamp to zero before the check. We don't like to get those warnings even if they are unreliable. If you have to do this, please mention in the changelog that it is done to avoid a harmless warning from gcc (and which version of gcc caused it).
Arnd
-
Amitoj Kaur Chawla -
Arnd Bergmann -
Julia Lawall