kfpu_begin() and kfpu_end() split

[lundman]

Trying out this discussion thing, hoping to get input from @AttilaFueloep @mcmilk @behlendorf

At the moment we have things like;

https://github.com/openzfs/zfs/blob/master/module/zcommon/zfs_fletcher_avx512.c#L42
https://github.com/openzfs/zfs/blob/master/module/zcommon/zfs_fletcher_avx512.c#L76

So the kfpu_being() and kfpu_end() are in separate functions. Windows sort of want to allocate some space to save it:

	NTSTATUS saveStatus = STATUS_INVALID_PARAMETER; \
	XSTATE_SAVE SaveState;
	saveStatus = KeSaveExtendedProcessorState(kfpu_state, &SaveState);

	if (NT_SUCCESS(saveStatus)) \
		KeRestoreExtendedProcessorState(&SaveState);

So it gets difficult when they are split across functions. They used to be nested even, but it seems maybe the nested calls
have been removed.

Can we make them simpler? Other solutions considered?

[mcmilk]

My personal thinking is, that all fletcher based code should move from module/zcommon into module/icp/algs/fletcher.
The standard c code functions should then go into fletcher_generic.c - and all the mixed code with intrinsics and inline asm should converted to assembly files with hand optimized code. These would then go into module/icp/asm-(aarch64|arm|ppc64|x86_64).

This would also resolve the issues with compilers, which generate sometimes bad code:

• Optimize the fletcher4 neon implementation #14219
• RFC: Generic Fletcher4 vector implementation #14234

[AttilaFueloep]

The XSAVE instruction requires memory to save the FPU state to, it's called the XSAVE area. On Linux we allocate a global buffer per CPU for it.

zfs/include/os/linux/kernel/linux/simd_x86.h

Lines 243 to 252 in 620a977

	for_each_possible_cpu(cpu) {
	struct page *page = alloc_pages_node(cpu_to_node(cpu),
	GFP_KERNEL | __GFP_ZERO, order);
	if (page == NULL) {
	kfpu_fini();
	return (-ENOMEM);
	}
	zfs_kfpu_fpregs[cpu] = page_address(page);
	}

All calls to kfpu_{begin,end}() will use this buffer. Due to this, nesting calls to kfpu_begin() would be a FPU state corrupting bug since the second call would override saved state and the second kfpu_end() call would then restore stale state.

Splitting the kfpu_{begin,end}() into different functions, while technically correct, is a bit hard to reason about. I'd say it's done this way to preserve the contents of the SIMD registers across calls to the various functions [1]. This, of course, will not work with the kfpu_{begin,end}() implementation you currently have on Windows. You'd need to allocate XSTATE_SAVE SaveState separately, like it's done on Linux. Alternatively we could make sure that kfpu_{begin,end}() calls always match in the same function, as @mcmilk suggested.

[1] kfpu_begin() disables preemption, so nothing can change the FPU state in between kfpu_{begin,end}().

[AttilaFueloep]

Looking at zfs_fletcher_avx512.c in more detail the calls to kfpu_{begin,end}() were moved from fletcher_4_avx512f_native() to fletcher_4_avx512f_{init,fini() in #14247. I see no easy way to changes this without loosing that optimization.

[lundman]

How important is the per-core thing? I can probably do like Linux on Windows, but on macOS, the cpu_number() function for M1 is Private, so we are not allowed to call it.

[ryao]

How important is the per-core thing? I can probably do like Linux on Windows, but on macOS, the cpu_number() function for M1 is Private, so we are not allowed to call it.

How does one determine whether a XNU function is private or public? I am not seeing an obvious distinction in the XNU kernel sources.

[lundman]

~/src/zfs/xnu-8792.41.9$ grep -r cpu_number config/*.exports
config/Private.arm.exports:_cpu_number
config/Private.arm64.exports:_cpu_number
config/Unsupported.x86_64.exports:_cpu_number

So, I can use it in x86_64 (but its unsupported), but can't link with it on arm/arm64.
We used to hack around and link against it anyway (rebels, wooo) , but ARM's PAC has made that hard.

[lundman]

But don't worry about macOS, we don't actually need to do anything for SIMD use in macOS - at least not yet. I want cpu_number() for the other places we use it in ZFS.

[ryao]

Would these work?

https://developer.apple.com/documentation/kernel/3609224-ml_get_cpu_number
https://developer.apple.com/documentation/kernel/3609223-ml_get_cpu_count

[AttilaFueloep]

I'd say we do this to avoid moving around cachelines. Please note that we're doing the whole kfpu dance only to account for the fact that the native Linux implementation is GPL only. So I wonder if there's no OS provided way to ensure integrity of the FPU state on Windows and macOS you could use there.

[ryao]

@lundman Windows should have a save state area called ->XStateSave in KTHREAD according to this:

https://github.com/reactos/reactos/search?q=PXSTATE_SAVE&type=code

You should be able to use that.

[ryao]

@lundman It looks like XNU already supports this. Is there anything stopping you from using fp_save() and fp_load()?

https://github.com/apple/darwin-xnu/blob/2ff845c2e033bd0ff64b5b6aa6063a1f8f65aa32/osfmk/mach/i386/fp_reg.h#L130-L134
https://github.com/apple/darwin-xnu/blob/2ff845c2e033bd0ff64b5b6aa6063a1f8f65aa32/osfmk/i386/fpu.c#L1369-#L1422

[lundman]

Yeah, not exported. Can only calls things in config/*.exports except for Private.exports.

[ryao]

That is annoying. On the bright side, we probably could share code for saving registers between Linux and XNU, provided that the region in the thread_t is similar. Also, #14555 means that we will probably be doing this on Linux for aarch64 too.

[lundman]

thread_t is opaque, can't see inside it, can't use any fields :)

[ryao]

In that case, does XNU support kernel thread local storage?

[lundman]

Nope. We did a spl-tsd.c which just matches based on thread ptr value. Luckily, TSDs arent used much in ZFS.

[lundman]

The simple solution is to just use the local stack, which is what this started as. But there aren't many places were we split the kfpu calls up, so I can certainly #ifdef around those few places.

[ryao]

The simple solution is to just use the local stack, which is what this started as. But there aren't many places were we split the kfpu calls up, so I can certainly #ifdef around those few places.

That would also work with some refactoring. I guess we could change the callbacks to take a pointer to stack memory from the prior frame and use that to store this. It would be the least messy way I can imagine to refactor this.

[behlendorf]

Using local stack when the calls aren't split up sure sounds like the way to go. It should be the most efficient thing by far. Before going to far down the TSD route for the other places I'd suggest some quick benchmarks. If you need to do any sort of lookup it might more than offset any gains from the optimized code.

[ryao]

The reason why the calls are split up is to support operating on ABDs. Passing a pointer to the init and fini callbacks that points to the memory needed by the architecture to store variables on the stack feels like the sanest way to keep the flexibility of not teaching every checksum algorithm about ABDs while being able to use stack memory to store registers.

[lundman]

Presumably we could also pick a new member in fletcher_4_ops_t for boolean_t call_kfpu; as well, since the init/fini calls are only called zfs_fletcher.c / zio_checksum.c ? Passing ptr along seems more inline with kernel style tho.

[lundman]

OK so there's currently just 4 places, and each one of them take a fletcher_4_ctx_t *ctx.

So I simply added some local vars to it:

 typedef union fletcher_4_ctx {
        zio_cksum_t scalar;
        zfs_fletcher_superscalar_t superscalar[4];

+#if defined(_WIN32) && defined(_KERNEL) /* kfpu_begin_ctx() */
+       NTSTATUS saveStatus;
+       XSTATE_SAVE SaveState;
+#endif
+
 #if defined(HAVE_SSE2) || (defined(HAVE_SSE2) && defined(HAVE_SSSE3))
        zfs_fletcher_sse_t sse[4];

and invented new kfpu_begin_ctx(CTX); and kfpu_end_ctx(CTX). I can make the FreeBSD/Linux versions simply be kfpu_begin(). etc.

It will do for now, we can still think of other things of course, just means it isn't a roadblock here.

[ryao]

That is a union, so putting it there will not work, but I like the direction you are taking with this. Maybe zio_abd_checksum_data_t would work for this.

[AttilaFueloep]

Yes, I agree that this is a good approach. zio_abd_checksum_data_t is passed to abd_fletcher_4_{init,fini}() so would be the right place to keep the data.

[AttilaFueloep]

No, wait, that won't work. fletcher_4_{sse,avx512,intel}_{init,fini}() need the SaveState since they call kfpu_{begin,end}(). And they only take a fletcher_4_ctx *.

[ryao]

kfpu_begin_ctx(CTX); and kfpu_end_ctx(CTX) would be problematic because this should be reused elsewhere in SIMD versions of other checksum algorithms, the raidz and encryption code, and we cannot have the same context there. In the case of the other checksum algorithms (blake3, sha256 and sha512), we do not even have such a nice data structure pointer being passed either, although one could consider accelerating those to be less important. In the case of GCM (AES) and raidz, we do have structures that could be used for that purpose.

It is a bit odd to me that the example is for Windows (since I was under the impression that ->XStateSave in KTHREAD could be used, but here is what I suggest:

I think we should do something like this:

typedef struct xsavestate {
       NTSTATUS saveStatus;
       XSTATE_SAVE SaveState;
} xsavestate_t;

Then xsavestate_t could be embedded at the top of zio_abd_checksum_data_t, raidz_row_t and gcm_ctx_t. Then we could have void kfpu_begin_portable(void * xsavestate); and kfpu_end_portable(void * xsavestate). You would pass in a pointer to whatever the data structure is and kfpu_begin_portable()/kfpu_end_portable() will be given a pointer to xsavestate_t. On platforms where the save state storage can go into existing per-thread memory, the pointer would still be passed, but it can be safely ignored, as we would just do #define kfpu_begin_portable(_) kfpu_begin() and #define kfpu_end_portable(_) kfpu_end()`.

[ryao]

Note that this is an alternative approach to one that makes use of ml_get_cpu_number()/ml_get_cpu_count().

[ryao]

Would something like this work on XNU?

struct xsavestate {
       ...
       int oldlevel;
} xsavestate_t;

static xsavestate_t PERCPU_DATA(xss);

void kfpu_begin() {
       xsavestate_t *xssp;
       int oldlevel = ml_set_interrupts_enabled(FALSE);
       xssp = PERCPU_GET(xss);
       kfpu_save_fsave((uint_8 *)xssp);
       xssp->oldlevel = oldlevel;
}

void kfpu_end() {
       uint8_t *xssp = PERCPU_GET(xss);
       kfpu_restore_fsave(xssp);
       ml_set_interrupts_enabled(xssp->oldlevel);
}

I assume that they have not banned the use of per cpu variables on XNU.

[lundman]

So, PERCPU won't work, no header to start with, but it boils down to current_percpu_base(void) and current_thread() and threads are opaque. Possible I can use the segment __attribute__((section("__DATA, __percpu"))) , but any address I fetch out, will not be PAC, so it would panic. (But I don't know if they need to be PAC in __DATA segment, I will have to experiment).

However, I can possibly do something with:

    uint64_t mpidr_el1;
    asm volatile("mrs %0, mpidr_el1" : "=r" (mpidr_el1));
    i = (uint8_t)(mpidr_el1 & 0xff);

seems awfully deep, but it is what it is. I'll try it next time I can reboot my M1.

[AttilaFueloep]

What would you think of this small refactoring I did (untested)?

AttilaFueloep/zfs@master...zfs-refactor-fletcher

You could then add your state to zio_abd_checksum_data_t and handle the differences in abd_fletcher_4_init() and abd_fletcher_4_fini(). Everything else would be businesses as usual.

[lundman]

Yeah that would work for me.

[lundman]

In the currently code, after it does the sse2 benchmark, which is a loop of 32, the calls to kfpu_begin_ctx() has happened 32x more times than kfpu_end_ctx(). So begin is called twice for every end. Do they not need to match one-to-one?
For example:

OpenZFS!fletcher_4_sse2_init+0x8a:
VERIFY3( 0 == kfpu_atomic ) failed (0 == 31)
OpenZFS!fletcher_4_sse2_byteswap+0xad:
VERIFY3( 0 == kfpu_atomic ) failed (0 == 32)
OpenZFS!fletcher_4_sse2_fini+0x17d:
VERIFY3( 0 == kfpu_atomic ) failed (0 == 31)

Should byteswap also call begin? or perhaps not, if _init does. Or should it call end before returning? Which style is expected?

I just threw in an atomic inc/dec in begin/end to see why it kept writing over my SaveState.

[ryao]

They should match 1 to 1. I am not sure why it would be different. The way it is supposed to work is:

• fletcher_4_sse2_init() -> kfpu_begin()
• fletcher_4_sse2_byteswap()/fletcher_4_sse2_native() as many times as necessary (the same one each time)
• fletcher_4_sse2_fini() -> kfpu_end()

[lundman]

You made me suspicious, yeah, just a bad merge - I need to double check everything, in case something else slipped through

[AttilaFueloep]

All right, #14600.