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.