virtio-devices: block: Fix latency counter for average read/write #5763
Conversation
The cumulative average formula [1] requires to use signed integers for proper calculations, while calculated result (e.g. cumulative average) is always positive. This patch reflects the above requirements in our code. [1] https://en.wikipedia.org/wiki/Moving_average#Cumulative_average Fixes: cloud-hypervisor#5745 Signed-off-by: Bo Chen <chen.bo@intel.com>
|
@rbradford Can you please take a look at it? I think it is a more appropriate fix to #5745. Please let me know your thoughts. Thanks. |
| + ((latency * LATENCY_SCALE) as i64 - read_avg as i64) | ||
| / (read_ops_last + read_ops.0) as i64) | ||
| .try_into() | ||
| .unwrap() | ||
| }; |
There was a problem hiding this comment.
I realised about the signedness issue in my updated version in my PR:
read_avg.saturating_add_signed(
((latency as i64)
.saturating_mul(LATENCY_SCALE as i64)
.saturating_sub(read_avg as i64))
/ (read_ops_last + read_ops.0) as i64
Do you think there is still scope for overflow?
There was a problem hiding this comment.
Not that I can think of. To overflow the calculation, you will need to have a latency lager than (u64::MAX-1)/LATENCY_SCALE ms (that is roughly 58494 years). If we really worry about silent overflow, I can replace all as i64 with try_into().unwrap(), so that any future overflows will be reported as a panic. I am fine either way.
| + ((latency * LATENCY_SCALE) - read_avg) | ||
| / (read_ops_last + read_ops.0) | ||
| // Cumulative average is guaranteed to be | ||
| // positive if being calculated properly |
There was a problem hiding this comment.
I don't think you need this comment - could add an assert...
There was a problem hiding this comment.
The comment was put to explain why the unwrap() was used here, e.g. programming error (not an error to be handled/reported).
|
test_vfio_user has been very flaky recently so this PR is ready to merge. |
The cumulative average formula [1] requires to use signed integers for proper calculations, while calculated result (e.g. cumulative average) is always positive. This patch reflects the above requirements in our code.
[1] https://en.wikipedia.org/wiki/Moving_average#Cumulative_average
Fixes: #5745