Page MenuHomeFreeBSD

radix_trie: eliminate iteration in keydiff
ClosedPublic

Authored by dougm on Jun 17 2023, 6:20 PM.
Tags
None
Referenced Files
Unknown Object (File)
Fri, May 17, 5:57 PM
Unknown Object (File)
Fri, May 17, 5:57 PM
Unknown Object (File)
Jan 1 2024, 3:52 AM
Unknown Object (File)
Dec 31 2023, 11:01 PM
Unknown Object (File)
Dec 31 2023, 11:01 PM
Unknown Object (File)
Dec 22 2023, 11:03 PM
Unknown Object (File)
Dec 11 2023, 3:48 AM
Unknown Object (File)
Oct 27 2023, 5:45 AM

Details

Summary

The implementations of *_keydiff iterate. By using flsl(), they don't have to.

If flsl() is implemented as a loop, though, this is a loss. Of particular concern is behavior on riscv, where the standard loop implementations of ffs, fls, etc appear to be in place. The change proposed at https://reviews.freebsd.org/D40594 seems like it ought to replace the loops with __builtin_ffs and the like, but I don't see obvious benefits (the presence of ctz and clz opcodes in objdump output) from this change after building a riscv kernel.

Mark, do you have any suggestions on how to (or who could) get ffs and fls inlined on riscv?

Test Plan

Kernel boots.

Diff Detail

Repository
rG FreeBSD src repository
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

dougm requested review of this revision.Jun 17 2023, 6:20 PM
dougm created this revision.
dougm added a subscriber: markj.

The change proposed at https://reviews.freebsd.org/D40594 seems like it ought to replace the loops with __builtin_ffs and the like, but I don't see obvious benefits (the presence of ctz and clz opcodes in objdump output) from this change after building a riscv kernel.

Mark, do you have any suggestions on how to (or who could) get ffs and fls inlined on riscv?

Those instructions are not part of the base ISA, but instead belong to one of a set of ratified "bit-manipulation" extensions. There appears to be some LLVM support for them but I'm not sure how to turn it on, and presumably we don't want to do that for GENERIC kernels? @jrtc27 or @mhorne could possibly say more.

The change proposed at https://reviews.freebsd.org/D40594 seems like it ought to replace the loops with __builtin_ffs and the like, but I don't see obvious benefits (the presence of ctz and clz opcodes in objdump output) from this change after building a riscv kernel.

Mark, do you have any suggestions on how to (or who could) get ffs and fls inlined on riscv?

Those instructions are not part of the base ISA, but instead belong to one of a set of ratified "bit-manipulation" extensions. There appears to be some LLVM support for them but I'm not sure how to turn it on, and presumably we don't want to do that for GENERIC kernels? @jrtc27 or @mhorne could possibly say more.

Mark's assessment is correct. I've left a more detailed comment in D40594.

sys/vm/vm_radix.c
300–304

On 32-bit machines, a long is 32 bits, but a vm_pindex_t is 64 bits.

dougm marked an inline comment as done.

flsl -> flsll

I've left a more detailed comment in D40594.

Thank you!

sys/kern/subr_pctrie.c
56

libkern.h sorts after kernel.h.

This revision is now accepted and ready to land.Jun 19 2023, 7:32 PM
sys/vm/vm_radix.c
289

This description is wrong, This function does not return a slot. Please fix it.

dougm marked 2 inline comments as done.

Fix include order. Fix inaccurate comment. Add explanatory comment before flsl(). Do it twice.

This revision now requires review to proceed.Jun 20 2023, 5:15 AM
This revision is now accepted and ready to land.Jun 20 2023, 5:54 AM

At this point, I'll wait for Mark to re-approve.

This revision was automatically updated to reflect the committed changes.