Ensure alignment of allocation for FPU initial state

[vangyzen]

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.

[jhb]

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

[kib]

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.

[vangyzen]

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).

[kib]

In D25098#552808, @vangyzen wrote:

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.

[cem]

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

[vangyzen]

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).

[cem]

In D25098#553021, @vangyzen wrote:

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.

[vangyzen]

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.

[vangyzen]

• Do potentially blocking operations before disabling interrupts.
• Since I'm creating the uma zone first, allocate the initial state from it.

[vangyzen]

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.

[kib]

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.