The linuxkpi log2.h file includes functions besides ilog2 that are generally useful. This change moves some of them into libkern.h.
Details
Details
- Reviewers
alc markj - Group Reviewers
bhyve - Commits
- rG5dbf886104b4: x86: use order_base_2
rG0839258c5690: ipfw: Use roundup_pow_of_two
rGdc048255b37e: mlx5: use roundup_pow_of_two
rG87177ce3aa98: irdma: Use round{up,down}_pow_of_two
rG5fc42387d704: cxgbe: use order_base_2
rG7bb73f731538: cxgb: use rounddown_pow_of_two
rG4bbdabc2bd76: aic7xxx: use rounddown_pow_of_two
rGc8b0c33b03ac: log2: move log2 functions from linuxkpi to libkern
rGa94ed493b507: dev/mana: replace power2 function
Diff Detail
Diff Detail
- Repository
- rG FreeBSD src repository
- Lint
Lint Not Applicable - Unit
Tests Not Applicable
Event Timeline
| sys/dev/drm2/drm_os_freebsd.h | ||
|---|---|---|
| 236 ↗ | (On Diff #139650) | Is this line needed? I don't know. None of my test builds seemed to care about the files I'm removing here. |
| sys/dev/qlnx/qlnxe/bcm_osal.h | ||
| 104 ↗ | (On Diff #139650) | Is this line needed? I don't know. None of the test builds I did seemed to care whether or not the functions below were defined. |
| sys/sys/libkern.h | ||
| 39 ↗ | (On Diff #139650) | Should this go before line 36? No, not if I want KASSERT to be defined. |
| sys/sys/log2.h | ||
| 35 ↗ | (On Diff #139650) | Why are these defined? Well, this fixes the failure of buildworld on i386. To be clear, it fails already. This is just added so that I can compete that buildworld. Also, it might let someone include log2.h without libkern.h. |
| sys/dev/drm2/drm_os_freebsd.h | ||
|---|---|---|
| 236 ↗ | (On Diff #139650) | The one kernel config which uses this code is arm/conf/TEGRA124. Does your test build cover it? |
| sys/sys/log2.h | ||
| 149 ↗ | (On Diff #139650) | Aside from order_base_2(), these are aliases of rounddown2(), roundup2(), and powerof2(). Which versions are kernel developers supposed to use? These aliases are generally used in drivers which are ported from Linux; can we not keep them in the linuxkpi? |
Comment Actions
Move is_power_of_2 back to the linuxkpi version.
Restore the orignal 2013 behavior of the constant-handling code for invalid constant log parameters, which might stop coverity from complaining so much.
| sys/dev/drm2/drm_os_freebsd.h | ||
|---|---|---|
| 236 ↗ | (On Diff #139650) | make buildkernel TARGET=arm KERNCONF=TEGRA124 completes successfully. |
| sys/sys/log2.h | ||
| 149 ↗ | (On Diff #139650) | I agree that is_power_of_two is the same as powerof2 in param.h, and that this definiton could be left in linuxkpi. However rounddown_pow_of_two is not the same as rounddown2. They take a different number of arguments. One rounds down to the next power of two, while the other rounds down to the next multiple of a given power of two. Same thing for the up versions. |
| sys/sys/log2.h | ||
|---|---|---|
| 149 ↗ | (On Diff #139650) | If we keep them here, I'd prefer to use names that complement the existing param.h macros. It's not really ideal to have, e.g., roundup2() and roundup_pow_of_2() side-by-side. Drivers which use Linux interfaces, like mana, can pull in LinuxKPI includes if they need to. I'm not sure what the names should be, but this patch also doesn't introduce any non-LinuxKPI consumers of these macros. |
| sys/sys/log2.h | ||
|---|---|---|
| 149 ↗ | (On Diff #139650) | There are three places where this patch removes definitions of roundup_pow_of_two so that this new, common definition can be used. Those three drivers would be non-LinuxKPI consumers of that macro. This patch is a response to this @alc comment in D45494: It's not clear to me I can make both of you happy here. |
| sys/sys/log2.h | ||
|---|---|---|
| 149 ↗ | (On Diff #139650) | I don't have any better names to suggest, so can't really argue against using the Linux names. :) |
Comment Actions
I'm not convinced that we should be creating an ilog2.h header file. I would leave the definitions in the existing libkern.h header file.
| sys/sys/log2.h | ||
|---|---|---|
| 149 ↗ | (On Diff #139650) | I think that rounddown_powerof2() would be more consistent with param.h. However, in the end, I also think that being consistent with Linux will probably save time for more people. Thoughts? |
| sys/sys/log2.h | ||
|---|---|---|
| 149 ↗ | (On Diff #139650) | If someone is familiar with Linux, they can quickly look up the LinuxKPI definition and see the "native" name for it (presumably we'd have #define roundup_pow_of_two roundup_powerof2 or similar), so it's not obvious to me that they'd waste a lot of time. I'm not doing the work here so don't want to argue very much. I'll just say that I tend to find code easier to read if it's consistently following one of native FreeBSD style or Linux style, and code which mixes the two is a bit jarring. This is just about a couple of macros, though, so either approach is fine with me. |
Comment Actions
Use order_base_2 in place of fls in the code modified in 9ff1462976fce4f4389be9a3357eadd22d04d30
Comment Actions
Please modify consumers (drivers, etc.) in separate commits from the actual libkern.h change.
| sys/sys/libkern.h | ||
|---|---|---|
| 228 ↗ | (On Diff #139861) | Why four underscores? Using cdefs.h macros you could write extern int ____ilog2_NaN(void) __pure2 __dead2; instead. |
| sys/sys/libkern.h | ||
|---|---|---|
| 228 ↗ | (On Diff #139861) | The log2 implementation comes from sys/ofed/include/linux/log2. The last commit to that file (before it was removed and the functionality was moved to linuxKPI dropped ____ilog2_NaN() and replaced it with -1. The commit by HPS just says: Minor rework of ilog2() function. The second commit to that file introduced the 66-line const handling ilog2 lines with no comment, but with the four underscores. This web link: https://lwn.net/Articles/203596/ Ultimately, it comes from 2006 Red Hat code by David Howells. Only he knows why there are four underscores. I'll make the pure2 dead change. |
Comment Actions
Replace spaces with tabs in #defines. Avoid problems with argument side-effects in is_power_of_2 and order_base_2.
| sys/sys/libkern.h | ||
|---|---|---|
| 228 ↗ | (On Diff #139861) | Linux got rid of ____ilog2_NaN about 8 years ago. |
| sys/sys/libkern.h | ||
|---|---|---|
| 228 ↗ | (On Diff #139861) | They did, because the gcc of that time couldn't optimize well in its presence. Also, since the code was copied from Linux to FreeBSD, they redefined is_power_of_2 so that 0 was not a power of 2, where the current implementation we have produces an undefined result in that case. Also, they got rid of the long list of const log values here: I'll update with all those considerations in mind. |
Comment Actions
Stick to our more common usage of typeof (with two underscores before and after).
Change is_power_of_2 to match the latest linux definition (where 0 is not a power of 2).
Get rid of the giant table for const log2 values, since linux made that change years ago, writing here:
https://code.googlesource.com/linux/torvalds/linux/+/2f78788b55baa3410b1ec91a576286abe1ad4d6a
It can be expressed using builtin_clzll builtin which is supported by
GCC 3.4 and later and when used only in the builtin_constant_p guarded
code it ought to always fold back to a constant. Other compilers support
the same builtin for many years too.
| sys/compat/linuxkpi/common/include/linux/log2.h | ||
|---|---|---|
| 38 ↗ | (On Diff #140136) | |
Comment Actions
Change ilog2_const to not have a special case for n <= 0.
Testing reveals that for compile-time constants, ilog2(0) won't compile, which is not the weird link-time error from the old code.