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

Ingo Molnar
2015-07-18T03:40:31Z

* H. Peter Anvin <hpa@zytor.com> wrote:

> On 07/16/2015 12:14 PM, Dave Hansen 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?
>
> 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.

Note that we have 3 related 'task state' data structures with overlapping purpose:

task_struct
thread_struct
thread_info

where thread_struct is embedded in task_struct currently.

So to turn it all into a single structure we'd have to merge thread_info into
thread_struct. thread_info was put on the kernel stack due to the ESP trick we
played long ago - but that is moot these days.

So I think what we want is not some common structure, but to actually merge all of
thread_info into thread_struct for arch details and into task_struct for generic
fields, and only have:

task_struct /* generic fields */
thread_struct /* arch details */

this can be done gradually, field by field, and in the end thread_info can be
eliminated altogether.

The only real complication is that it affects every architecture. The good news is
that most of the thread_info layout details are wrapped in various constructs like
test_ti_thread_flag() and task_thread_info().

While at it we might as well rename 'thread_struct' to 'arch_task_struct':

task_struct /* generic fields */
arch_task_struct /* arch details */

to make it really clear and easy to understand at a glance - as the current naming
is has become ambiguous and slightly confusing the moment we introduced threading.

Thanks,

Ingo