libkern: add ilog2 helpers

[dougm]

Summary

The fls* bit-finding functions have to include zero-checks. In a case where the caller knows that the argument is not zero, these new functions provide a way to avoid those zero-checks.

Test Plan

Built kernels for several architectures.

[dougm]

Others have used the ilog2 name for the same function. Drop those other definitions.

[jhibbits]

sys/powerpc/booke/pmap_64.c

753

There's another ilog2 in pmap_32.c, too, and looks like yet another declaration in pmap.c. Also, now you'll need to change all uses in pmap.c to ilog2l() instead of ilog2(), since ilog2() now operates on an int, and we need a long here (vm_size_t).

[dougm]

Change ilog2 to ilog2l where appropriate. Drop bslr, since it's just another name for ilog2l.

[jhibbits]

PPC LGTM. If you haven't, test a buildkernel with MPC85XX (powerpc) and QORIQ64 (powerpc64) just to be safe.

[dougm]

In D45170#1030344, @jhibbits wrote:

PPC LGTM. If you haven't, test a buildkernel with MPC85XX (powerpc) and QORIQ64 (powerpc64) just to be safe.

With or without these changes in place
make -j 24 buildkernel TARGET=powerpc KERNCONF=QORIQ64
fails:
./machine/reg.h:80:41: error: declaration of 'struct reg32' will not be visible outside of this function [-Werror,-Wvisibility]

80 | int     fill_regs32(struct thread *, struct reg32 *);

begins a stream of errors.

With these changes in place
make -j 24 buildkernel TARGET=powerpc KERNCONF=MPC85XX
completes successfully.

[jhibbits]

In D45170#1030347, @dougm wrote:

In D45170#1030344, @jhibbits wrote:

PPC LGTM. If you haven't, test a buildkernel with MPC85XX (powerpc) and QORIQ64 (powerpc64) just to be safe.

With or without these changes in place
make -j 24 buildkernel TARGET=powerpc KERNCONF=QORIQ64
fails:
./machine/reg.h:80:41: error: declaration of 'struct reg32' will not be visible outside of this function [-Werror,-Wvisibility]

80 | int     fill_regs32(struct thread *, struct reg32 *);

begins a stream of errors.

powerpc64 is a trickier beast. You need to 'make TARGET=powerpc TARGET_ARCH=powerpc64 ...' (explicitly specify TARGET_ARCH)

[dougm]

powerpc64 is a trickier beast. You need to 'make TARGET=powerpc TARGET_ARCH=powerpc64 ...' (explicitly specify TARGET_ARCH)

Then it builds fine with these changes.

[dougm]

@whu: This change would drop the definition of ilog2 that you included in sys/dev/mana/gdma_util.h. However, that function, called ilog2(x), actually returns x + log2(x), because it initializes 'log' to x, and not to 0.

Is this change, as currently formulated, fixing an error in the dev/mana code, or introducing one?

[mhorne]

sys/powerpc/booke/pmap_32.c

933–944

I would think the consumers within this file should be changed to use ilog2l(), no?

sys/powerpc/booke/pmap_64.c

753

Same question as pmap_32.c above.

[dougm]

sys/powerpc/booke/pmap_32.c

933–944

Yes, but there are none.

sys/powerpc/booke/pmap_64.c

753

Same answer as above.

[mhorne]

LGTM

[markj]

sys/arm64/iommu/smmu.c

938

I think these uses can be ilog2(), even though the removed function implemented ilog2l().

sys/sys/libkern.h

206

I wonder if we want to start using _Generic to reduce the need for callers to be careful about picking the right variant. That is, provide a macro ilog2() which uses _Generic to dispatch to a specific variant. We've been using _Generic in atomic_common.h for some time now without problems.

[dougm]

sys/sys/libkern.h

206

It is used conditionally in atomic_common.h. I believe I would need to use Generic unconditionally, if I used it at all. Is unconditional use going to be a problem?

[dougm]

sys/sys/libkern.h

206

Additionally, defining ilog2 as a _Generic clashes with the #define of ilog2 in sys/compat/linuxkpi/common/include/linux/log2.h, so a #define of ilog2 in libkern will have to include that functionality as well.

[markj]

sys/sys/libkern.h

206

I think unconditional use is ok so long as the uses are restricted to the kernel. atomic_common.h is also used by userspace, where we can't control the compiler.

This header (libkern.h) is exposed to userspace (there are no _KERNEL guards), but I think it's most likely ok to assume that it's only going to be used by the kernel.