Avoid 64bit division in multilist index functions. #12288
Conversation
There was a problem hiding this comment.
I could potentially see this intentional 64-bit to 32-bit overflow causing some static analyzer warnings. It looks like by default both cppcheck and coverity have these checkers disabled, so we're not seeing any warnings today, but in the future we might. I'd suggest we extend to comments to make it clear why the casts are here and that they are intentional. Otherwise, this optimization might get removed if the default behavior for one of the static analyzers changes to complain about this.
The change itself though looks good to me.
Number of sublists in multilists equal to number of CPUs is typically small even or even power of 2. It makes almost no sense to use full slow 64bit division. Signed-off-by: Alexander Motin <[email protected]> Sponsored-By: iXsystems, Inc.
|
I am not sure why would static analyzer complained about explicit type change. And I believe we have commit messages for the reasons why the code is what it is. But I've added/extended comments to explain it explicitly. |
mmaybee
added
Status: Accepted
The number of sublists in a multilist is relatively small. We dont need 64 bits to calculate an index. 32 bits is sufficient and makes the code more efficient. Reviewed-by: Matthew Ahrens <[email protected]> Reviewed-by: Brian Behlendorf <[email protected]> Reviewed-by: Mark Maybee <[email protected]> Signed-off-by: Alexander Motin <[email protected]> Sponsored-By: iXsystems, Inc. Closes openzfs#12288
Number of sublists in multilists equal to number of CPUs is typically
small even or even power of 2. It makes almost no sense to use full
slow 64bit division.
How Has This Been Tested?
On 80-thread FreeBSD system doing ~630K mostly uncached 4KB ZVOLs reads profiler shows reduction of CPU time spent in arc_state_multilist_index_func() from 3.7% to 3.3% of ARC eviction thread, bottle-necking this test.
Types of changes
Checklist:
Signed-off-by.