Page MenuHomeFreeBSD

Ensure alignment of allocation for FPU initial state
ClosedPublic

Authored by vangyzen on Jun 1 2020, 9:09 PM.

Details

Summary

The FPU state must be 64-byte aligned. This happens naturally
when all malloc buckets are powers of two. However, this change
is necessary on some systems when certain non-power-of-two
(and non-multiple of 64) malloc buckets are defined.

Diff Detail

Repository
rS 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

Ok with me. Would be nice if we had some kind of malloc(9)-level analog of posix_memalign() perhaps.

This revision is now accepted and ready to land.Jun 1 2020, 10:12 PM

I suggest to create the FPU save area as the first thing in the fpuinitstate(), and then allocate from the zone instead of manually realigning.

In D25098#552799, @kib wrote:

I suggest to create the FPU save area as the first thing in the fpuinitstate(), and then allocate from the zone instead of manually realigning.

That was my first revision, until I realized it would waste a lot more memory (if the system does not use vmm, the only current consumer of that zone).

In D25098#552799, @kib wrote:

I suggest to create the FPU save area as the first thing in the fpuinitstate(), and then allocate from the zone instead of manually realigning.

That was my first revision, until I realized it would waste a lot more memory (if the system does not use vmm, the only current consumer of that zone).

So the overhead would be an unused portion of single page after the XSAVE area. I do not insist but would not bother calling this an overhead, esp. for modern AVX512 machines.

Doesn't malloc(9) align to at least sizeof(allocation) already? So perhaps malloc(max(cpu_max_ext_state_size, XSAVE_AREA_ALIGN))?

The language in the manual is even stricter than that, and seems to be violated if this change is needed:

The malloc(), realloc(), and reallocf() functions return a kernel virtual
address that is suitably aligned for storage of any type of object

Doesn't malloc(9) align to at least sizeof(allocation) already?

I think so, but maybe that's because all the malloc zones are powers of two. cpu_max_ext_state_size is 2696 bytes (on this CPU), which falls into Isilon's 3296 malloc zone, and neither of those is a multiple of 64 (the required alignment).

The language in the manual is even stricter than that, and seems to be violated if this change is needed:

The malloc(), realloc(), and reallocf() functions return a kernel virtual
address that is suitably aligned for storage of any type of object

I think "object" here means any fundamental type. In this case, XSAVE needs 64-byte alignment. malloc can't possibly know or guarantee that (except that power-of-two zones naturally align all power-of-two allocations).

I think "object" here means any fundamental type. In this case, XSAVE needs 64-byte alignment. malloc can't possibly know or guarantee that (except that power-of-two zones naturally align all power-of-two allocations).

Hm, in some sense SSE/AVX vectors are fundamental types. 64-byte alignment is surprisingly large to me; do you know if that is that a cache-line constraint, or the size of an AVX-512 vector, or just its own special constraint? I'm just curious.

Hm, in some sense SSE/AVX vectors are fundamental types. 64-byte alignment is surprisingly large to me; do you know if that is that a cache-line constraint, or the size of an AVX-512 vector, or just its own special constraint? I'm just curious.

The Instruction Set Reference simply says this about the XSAVE instruction:

Use of a destination operand not aligned to 64-byte boundary (in either 64-bit or 32-bit modes) results in a general-protection (#GP) exception.

I doesn't go into detail about why.

  • Do potentially blocking operations before disabling interrupts.
  • Since I'm creating the uma zone first, allocate the initial state from it.
This revision now requires review to proceed.Jun 12 2020, 2:31 PM

I was doing some more debugging in this area (on Isilon OneFS). I ran into an issue where uma_zcreate blocked (due to sx lock contention when creating sysctl oids) and bad things happened because interrupts were disabled.

This revision is now accepted and ready to land.Jun 12 2020, 4:09 PM
kib added inline comments.
sys/amd64/amd64/fpu.c
162 ↗(On Diff #73023)

Unrelated, please commit separately.

382 ↗(On Diff #73023)

I do not know why do we still disable interrupts there. As in other places, critical_enter() should be enough.