Re: [RFC][PATCH] x86, fpu: dynamically allocate 'struct fpu'

Andy Lutomirski
2015-07-16T19:25:36Z

On Thu, Jul 16, 2015 at 12:14 PM, Dave Hansen <dave@sr71.net> wrote:
>
> 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?

Looks generally sensible. Minor nitpicking below.

> +#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));

> #endif /* _ASM_X86_FPU_H */
> diff -puN arch/x86/kernel/process.c~dynamically-allocate-struct-fpu arch/x86/kernel/process.c
> --- a/arch/x86/kernel/process.c~dynamically-allocate-struct-fpu 2015-07-16 10:50:42.360571875 -0700
> +++ b/arch/x86/kernel/process.c 2015-07-16 12:00:59.204808551 -0700
> @@ -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.

--Andy