x86, fpu: dynamically allocate 'struct fpu'

The FPU rewrite removed the dynamic allocations of 'struct fpu'. But, this potentially wastes massive amounts of memory (2k per task on systems that do not have AVX-512 for instance).

Instead of having a separate slab, this patch just appends the space that we need to the 'task_struct' which we dynamically allocate already. This saves from doing an extra slab allocation at fork(). The only real downside here is that we have to stick everything and the end of the task_struct. But, I think the BUILD_BUG_ON()s I stuck in there should keep that from being too fragile.

This survives a quick build and boot in a VM. Does anyone see any real downsides to this?

arch/x86/kernel/fpu/init.c static void __init fpu__init_system_gene · line 144

Quoted 6 lines · Dave Hansen

arch/x86/kernel/fpu/init.c renamed from arch/x86/kernel/fpu/init.c~dynamically-allocate-struct-fpu +6 -0

+#define CHECK_MEMBER_AT_END_OF(TYPE, MEMBER)   \
+       BUILD_BUG_ON((sizeof(TYPE) -                    \
+                       offsetof(TYPE, MEMBER) -        \
+                       sizeof(((TYPE *)0)->MEMBER)) >  \
+                       0)                              \
+

You could save a bit of typing by using offsetofend here. Something along the lines of BUILD_BUG_ON(sizeof(TYPE) != offsetofend(TYPE, MEMBER));

Quoted 7 lines

#endif /* _ASM_X86_FPU_H */
diff -puN arch/x86/kernel/process.c~dynamically-allocate-struct-fpu arch/x86/kernel/process.c

arch/x86/kernel/process.c 2015-07-16 12:00:59.204808551 -0700 renamed from arch/x86/kernel/process.c~dynamically-allocate-struct-fpu 2015-07-16 10:50:42.360571875 -0700 +0 -0

@@ -81,7 +81,7 @@ EXPORT_SYMBOL_GPL(idle_notifier_unregist
  */
 int arch_dup_task_struct(struct task_struct *dst, struct task_struct *src)

arch/x86/kernel/process.c 2015-07-16 12:00:59.204808551 -0700 EXPORT_SYMBOL_GPL(idle_notifier_unregist · line 84

Quoted 3 lines

arch/x86/kernel/process.c 2015-07-16 12:00:59.204808551 -0700 renamed from arch/x86/kernel/process.c~dynamically-allocate-struct-fpu 2015-07-16 10:50:42.360571875 -0700 +1 -1

 {
-       *dst = *src;
+       memcpy(dst, src, arch_task_struct_size());

This is actually vaguely performance-critical, which makes me thing that using some kind of inline or other real way (config macro, ifdef, etc) to detect whether there's an arch override would be better than a weak function.

Quoted 7 lines

Quoted 7 lines

#endif /* _ASM_X86_FPU_H */
diff -puN arch/x86/kernel/process.c~dynamically-allocate-struct-fpu arch/x86/kernel/process.c

arch/x86/kernel/process.c 2015-07-16 12:00:59.204808551 -0700 renamed from arch/x86/kernel/process.c~dynamically-allocate-struct-fpu 2015-07-16 10:50:42.360571875 -0700 +0 -0

@@ -81,7 +81,7 @@ EXPORT_SYMBOL_GPL(idle_notifier_unregist
  */
 int arch_dup_task_struct(struct task_struct *dst, struct task_struct *src)

Quoted 8 lines

arch/x86/kernel/process.c 2015-07-16 12:00:59.204808551 -0700 EXPORT_SYMBOL_GPL(idle_notifier_unregist · line 84

Quoted 3 lines

arch/x86/kernel/process.c 2015-07-16 12:00:59.204808551 -0700 renamed from arch/x86/kernel/process.c~dynamically-allocate-struct-fpu 2015-07-16 10:50:42.360571875 -0700 +1 -1

 {
-       *dst = *src;
+       memcpy(dst, src, arch_task_struct_size());

This is actually vaguely performance-critical, which makes me thing that using some kind of inline or other real way (config macro, ifdef, etc) to detect whether there's an arch override would be better than a weak function.

Fair enough. I'll send out another version in a bit if there are no more comments.

Quoted 16 lines · Dave Hansen

Quoted 15 lines

arch/x86/kernel/process.c 2015-07-16 12:00:59.204808551 -0700 EXPORT_SYMBOL_GPL(idle_notifier_unregist · line 84

Quoted 10 lines

#endif /* _ASM_X86_FPU_H */
diff -puN arch/x86/kernel/process.c~dynamically-allocate-struct-fpu arch/x86/kernel/process.c

arch/x86/kernel/process.c 2015-07-16 12:00:59.204808551 -0700 renamed from arch/x86/kernel/process.c~dynamically-allocate-struct-fpu 2015-07-16 10:50:42.360571875 -0700 +1 -1

@@ -81,7 +81,7 @@ EXPORT_SYMBOL_GPL(idle_notifier_unregist
  */
 int arch_dup_task_struct(struct task_struct *dst, struct task_struct *src)
 {
-       *dst = *src;
+       memcpy(dst, src, arch_task_struct_size());

This is actually vaguely performance-critical, which makes me thing that using some kind of inline or other real way (config macro, ifdef, etc) to detect whether there's an arch override would be better than a weak function.

Quoted 2 lines

Fair enough. I'll send out another version in a bit if there are no more comments.

Beyond making it a build time switch for other architectures, I'd also suggest introducing a __read_mostly task_struct_size variable on x86, so that we can write:

	memcpy(dst, src, task_struct_size);

From ab5da866792e4297de2308b42121633419f6d06d Mon Sep 17 00:00:00 2001

+++ b/arch/x86/kernel/fpu/init.c 2015-07-16 12:02:15.284280976 -0700

arch/x86/kernel/fpu/init.c renamed from arch/x86/kernel/fpu/init.c~dynamically-allocate-struct-fpu +28 -0

@@ -136,6 +136,45 @@ static void __init fpu__init_system_gene
 unsigned int xstate_size;
 EXPORT_SYMBOL_GPL(xstate_size);
 
+#define CHECK_MEMBER_AT_END_OF(TYPE, MEMBER)	\
+	BUILD_BUG_ON((sizeof(TYPE) -			\
+			offsetof(TYPE, MEMBER) -	\
+			sizeof(((TYPE *)0)->MEMBER)) > 	\
+			0)				\
+
+/*
+ * We append the 'struct fpu' to the task_struct.
+ */
+int __weak arch_task_struct_size(void)
+{
+	int task_size = sizeof(struct task_struct);
+
+	/*
+	 * Subtract off the static size of the register state.
+	 * It potentially has a bunch of padding.
+	 */
+	task_size -= sizeof(((struct task_struct *)0)->thread.fpu.state);
+
+	/*
+	 * Add back the dynamically-calculated register state
+	 * size.
+	 */
+	task_size += xstate_size;
+
+	/*
+	 * We dynamically size 'struct fpu', so we require that
+	 * it be at the end of 'thread_struct' and that

Quoted 39 lines · Dave Hansen

+++ b/arch/x86/kernel/fpu/init.c 2015-07-16 12:02:15.284280976 -0700

arch/x86/kernel/fpu/init.c renamed from arch/x86/kernel/fpu/init.c~dynamically-allocate-struct-fpu +34 -0

@@ -136,6 +136,45 @@ static void __init fpu__init_system_gene
 unsigned int xstate_size;
 EXPORT_SYMBOL_GPL(xstate_size);
 
+#define CHECK_MEMBER_AT_END_OF(TYPE, MEMBER)	\
+	BUILD_BUG_ON((sizeof(TYPE) -			\
+			offsetof(TYPE, MEMBER) -	\
+			sizeof(((TYPE *)0)->MEMBER)) > 	\
+			0)				\
+
+/*
+ * We append the 'struct fpu' to the task_struct.
+ */
+int __weak arch_task_struct_size(void)
+{
+	int task_size = sizeof(struct task_struct);
+
+	/*
+	 * Subtract off the static size of the register state.
+	 * It potentially has a bunch of padding.
+	 */
+	task_size -= sizeof(((struct task_struct *)0)->thread.fpu.state);
+
+	/*
+	 * Add back the dynamically-calculated register state
+	 * size.
+	 */
+	task_size += xstate_size;
+
+	/*
+	 * We dynamically size 'struct fpu', so we require that
+	 * it be at the end of 'thread_struct' and that
+	 * 'thread_struct' be at the end of 'task_struct'.  If
+	 * you hit a compile error here, check the structure to
+	 * see if something got added to the end.
+	 */
+	CHECK_MEMBER_AT_END_OF(struct fpu, state);
+	CHECK_MEMBER_AT_END_OF(struct thread_struct, fpu);

arch/x86/kernel/fpu/init.c static void __init fpu__init_system_gene · line 176

Quoted 4 lines

arch/x86/kernel/fpu/init.c renamed from arch/x86/kernel/fpu/init.c~dynamically-allocate-struct-fpu +4 -0

+	CHECK_MEMBER_AT_END_OF(struct task_struct, thread);
+
+	return task_size;
+}

Since you want these invariants true at all times, maybe put the BUILD_BUG_ON() in generic code instead of x86 specific? That way people poking at other archs are less likely to accidentally break your stuff.

Quoted 40 lines · Peter Zijlstra

Quoted 39 lines · Dave Hansen

+++ b/arch/x86/kernel/fpu/init.c 2015-07-16 12:02:15.284280976 -0700

(quoted diff) +34 -0

@@ -136,6 +136,45 @@ static void __init fpu__init_system_gene
 unsigned int xstate_size;
 EXPORT_SYMBOL_GPL(xstate_size);
 
+#define CHECK_MEMBER_AT_END_OF(TYPE, MEMBER)	\
+	BUILD_BUG_ON((sizeof(TYPE) -			\
+			offsetof(TYPE, MEMBER) -	\
+			sizeof(((TYPE *)0)->MEMBER)) > 	\
+			0)				\
+
+/*
+ * We append the 'struct fpu' to the task_struct.
+ */
+int __weak arch_task_struct_size(void)
+{
+	int task_size = sizeof(struct task_struct);
+
+	/*
+	 * Subtract off the static size of the register state.
+	 * It potentially has a bunch of padding.
+	 */
+	task_size -= sizeof(((struct task_struct *)0)->thread.fpu.state);
+
+	/*
+	 * Add back the dynamically-calculated register state
+	 * size.
+	 */
+	task_size += xstate_size;
+
+	/*
+	 * We dynamically size 'struct fpu', so we require that
+	 * it be at the end of 'thread_struct' and that
+	 * 'thread_struct' be at the end of 'task_struct'.  If
+	 * you hit a compile error here, check the structure to
+	 * see if something got added to the end.
+	 */
+	CHECK_MEMBER_AT_END_OF(struct fpu, state);
+	CHECK_MEMBER_AT_END_OF(struct thread_struct, fpu);

Quoted 8 lines

Review static void __init fpu__init_system_gene · line 176

Quoted 4 lines

(quoted diff) +4 -0

+	CHECK_MEMBER_AT_END_OF(struct task_struct, thread);
+
+	return task_size;
+}

Since you want these invariants true at all times, maybe put the BUILD_BUG_ON() in generic code instead of x86 specific? That way people poking at other archs are less likely to accidentally break your stuff.

Yeah.

The FPU rewrite removed the dynamic allocations of 'struct fpu'. But, this potentially wastes massive amounts of memory (2k per task on systems that do not have AVX-512 for instance).

Instead of having a separate slab, this patch just appends the space that we need to the 'task_struct' which we dynamically allocate already. This saves from doing an extra slab allocation at fork(). The only real downside here is that we have to stick everything and the end of the task_struct. But, I think the BUILD_BUG_ON()s I stuck in there should keep that from being too fragile.

This survives a quick build and boot in a VM. Does anyone see any real downsides to this?

Quoted 12 lines · Dave Hansen

The FPU rewrite removed the dynamic allocations of 'struct fpu'. But, this potentially wastes massive amounts of memory (2k per task on systems that do not have AVX-512 for instance).

Instead of having a separate slab, this patch just appends the space that we need to the 'task_struct' which we dynamically allocate already. This saves from doing an extra slab allocation at fork(). The only real downside here is that we have to stick everything and the end of the task_struct. But, I think the BUILD_BUG_ON()s I stuck in there should keep that from being too fragile.

Quoted 3 lines

This survives a quick build and boot in a VM. Does anyone see any real downsides to this?

No. I have also long advocated for merging task_struct and thread_info into a common structure and get it off the stack; it would improve security and avoid weird corner cases in the irqstack handling.

Quoted 12 lines · Dave Hansen

The FPU rewrite removed the dynamic allocations of 'struct fpu'. But, this potentially wastes massive amounts of memory (2k per task on systems that do not have AVX-512 for instance).

Instead of having a separate slab, this patch just appends the space that we need to the 'task_struct' which we dynamically allocate already. This saves from doing an extra slab allocation at fork(). The only real downside here is that we have to stick everything and the end of the task_struct. But, I think the BUILD_BUG_ON()s I stuck in there should keep that from being too fragile.

Quoted 2 lines

This survives a quick build and boot in a VM. Does anyone see any real downsides to this?

No. I have also long advocated for merging task_struct and thread_info into a common structure and get it off the stack; it would improve security and avoid weird corner cases in the irqstack handling.

The FPU rewrite removed the dynamic allocations of 'struct fpu'. But, this potentially wastes massive amounts of memory (2k per task on systems that do not have AVX-512 for instance).

Instead of having a separate slab, this patch just appends the space that we need to the 'task_struct' which we dynamically allocate already. This saves from doing an extra slab allocation at fork(). The only real downside here is that we have to stick everything and the end of the task_struct. But, I think the BUILD_BUG_ON()s I stuck in there should keep that from being too fragile.

This survives a quick build and boot in a VM. Does anyone see any real downsides to this?

arch/x86/kernel/fpu/init.c renamed from arch/x86/kernel/fpu/init.c~dynamically-allocate-struct-fpu +0 -0

@@ -136,6 +136,45 @@ static void __init fpu__init_system_gene
 unsigned int xstate_size;
 EXPORT_SYMBOL_GPL(xstate_size);