Page MenuHomeFreeBSD

Restrict supported alignment for malloc_domainset_aligned(9) to PAGE_SIZE.
ClosedPublic

Authored by kib on Jan 18 2021, 9:05 AM.

Details

Summary

UMA page_alloc() does not take an alignment, so UMA can only handle alignment less then page size.

Noted by: alc

Diff Detail

Repository
R10 FreeBSD src repository
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

kib requested review of this revision.Jan 18 2021, 9:05 AM
kib created this revision.

Sorry for not catching this.

sys/kern/kern_malloc.c
1176

Shouldn't we also formally request page alignment for zones with size > PAGE_SIZE?

kib marked an inline comment as done.Jan 18 2021, 3:33 PM
kib added inline comments.
sys/kern/kern_malloc.c
1176

I am not sure that this usage makes much sense, since at least busdma code wouldn't do it. Anyway, I changed it.

kib marked an inline comment as done.

Align larger zone on PAGE_SIZE.

This revision is now accepted and ready to land.Jan 18 2021, 6:24 PM
sys/kern/kern_malloc.c
780

This also needs change to size = roundup2(align), I think.

Also use the right zone for allocation, by bumping size to the multiple of align.

Reported by: melifaro

This revision now requires review to proceed.Jan 18 2021, 9:21 PM
kib marked an inline comment as done.Jan 18 2021, 9:21 PM

Unconditionally roundup.

jrtc27 added inline comments.
sys/kern/kern_malloc.c
1176

MIN from param.h?

This revision is now accepted and ready to land.Jan 18 2021, 9:56 PM
sys/kern/kern_malloc.c
1176

I believe that an allocation request for 384 bytes with 128 byte alignment will still go to the malloc-384 zone, yes? But, we aren't requiring better than UMA_ALIGN_PTR alignment.

kib marked 2 inline comments as done.Jan 19 2021, 8:14 AM
kib added inline comments.
sys/kern/kern_malloc.c
1176

bde@ did not liked the macro AFAIR.

kib marked an inline comment as done.

roundup alloc size to next power of two

This revision now requires review to proceed.Jan 19 2021, 8:16 AM
This revision is now accepted and ready to land.Jan 20 2021, 2:13 AM
alc added inline comments.
sys/kern/kern_malloc.c
1176

That's also my recollection. He preferred the inline functions from libkern.h.

sys/kern/kern_malloc.c
781

Isn't this going to be slow on architectures without that instruction? What are the actual requirements here? This could at least do with a comment; flsl is a rather obscure function that's non-obvious unless you've seen it before.

sys/kern/kern_malloc.c
781

A "count leading zeroes" instruction is available on every widely used architecture, including all of the architectures supported by FreeBSD. That said, the last time that I looked at the various cpufunc.h implementations, only 32-bit and 64-bit arm and x86 used the builtins that provide access to these instructions.

The objective is to round up to the next power of 2, because we can only guarantee alignment for power-of-2-sized allocations. I agree that a comment to this effect would help.

Add comment.
Bump size up to align (restore this part).

This revision now requires review to proceed.Jan 20 2021, 8:42 AM
kib added a subscriber: pho.

Restore -1 for zone align calculation.

Reported by: pho

sys/kern/kern_malloc.c
781

Ok, which then confirms my suspicion that this is wrong: if size is a power of two it doubles the allocation size. https://godbolt.org/z/a6dhEq (using the arm64 implementation since it's a simple one in terms of the builtin).

sys/kern/kern_malloc.c
781

(you want to subtract 1 before passing to flsl, and be careful about 0 - 1 if that's possible)

I ran tests with D28219.82616.diff on amd64 for two hours. No problems seen.

Do not unnecessary double alloc size.

I ran tests with D28219.82645.diff for 6 hours. No problems seen.

sys/kern/kern_malloc.c
782

I think that this sentence would be clearer without "at size".

783
784
785
kib marked 8 inline comments as done.

Edits for the comment.

alc added inline comments.
sys/kern/kern_malloc.c
785

Drop the first "the" at the beginning of the line and add "their" at the end.

This revision is now accepted and ready to land.Jan 21 2021, 7:08 PM
sys/kern/kern_malloc.c
788–790

Isn't the following one line sufficient?

asize = size < align ? align : 1UL << flsl(size - 1);
kib marked 2 inline comments as done.

Simplify calculation. Edit comment.

This revision now requires review to proceed.Jan 21 2021, 8:25 PM
This revision was not accepted when it landed; it landed in state Needs Review.Jan 21 2021, 9:34 PM
This revision was automatically updated to reflect the committed changes.