Re: [PATCH 1/1] headers/deps: x86/fpu: Make task_struct::thread constant size

Ingo Molnar
2024-03-26T19:12:36Z

* Oleg Nesterov <oleg@redhat.com> wrote:

> On 03/20, Ingo Molnar wrote:
> >
> > --- a/arch/x86/kernel/fpu/init.c
> > +++ b/arch/x86/kernel/fpu/init.c
> > @@ -38,7 +38,7 @@ static void fpu__init_cpu_generic(void)
> > /* Flush out any pending x87 state: */
> > #ifdef CONFIG_MATH_EMULATION
> > if (!boot_cpu_has(X86_FEATURE_FPU))
> > - fpstate_init_soft(&current->thread.fpu.fpstate->regs.soft);
> > + fpstate_init_soft(current->thread.fpu->fpstate->regs.soft);
> ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> Typo? it should be
>
> &current->thread.fpu->fpstate->regs.soft

Thx, fixed.

>
> > +static struct fpu x86_init_fpu __read_mostly;
> > +
> > static void __init fpu__init_system_early_generic(void)
> > {
> > + {
> > + int this_cpu = smp_processor_id();
> > +
> > + fpstate_reset(&x86_init_fpu);
> > + current->thread.fpu = &x86_init_fpu;
> > + per_cpu(fpu_fpregs_owner_ctx, this_cpu) = &x86_init_fpu;
> > + x86_init_fpu.last_cpu = this_cpu;
> > + }
>
> Can't x86_init_fpu be declared inside the block above?

As a function-local static? I think globals are better defined in a visible
fashion, not hidden among local variables where they are easy to overlook.

But the extra block is unnecessary in any case.

> > void __init fpu__init_system(void)
> > {
> > - fpstate_reset(&current->thread.fpu);
> > fpu__init_system_early_generic();
> > + fpstate_reset(current->thread.fpu);
>
> It seems that fpstate_reset(current->thread.fpu) is not needed after the
> change in fpu__init_system_early_generic() above.

Yeah. Something like the delta patch below?

Thanks,

Ingo

=================>

arch/x86/kernel/fpu/init.c | 15 ++++++---------
1 file changed, 6 insertions(+), 9 deletions(-)

diff --git a/arch/x86/kernel/fpu/init.c b/arch/x86/kernel/fpu/init.c
index f9412d012191..de618ec509aa 100644
--- a/arch/x86/kernel/fpu/init.c
+++ b/arch/x86/kernel/fpu/init.c
@@ -38,7 +38,7 @@ static void fpu__init_cpu_generic(void)
/* Flush out any pending x87 state: */
#ifdef CONFIG_MATH_EMULATION
if (!boot_cpu_has(X86_FEATURE_FPU))
- fpstate_init_soft(current->thread.fpu->fpstate->regs.soft);
+ fpstate_init_soft(&current->thread.fpu->fpstate->regs.soft);
else
#endif
asm volatile ("fninit");
@@ -75,14 +75,12 @@ static struct fpu x86_init_fpu __read_mostly;

static void __init fpu__init_system_early_generic(void)
{
- {
- int this_cpu = smp_processor_id();
+ int this_cpu = smp_processor_id();

- fpstate_reset(&x86_init_fpu);
- current->thread.fpu = &x86_init_fpu;
- per_cpu(fpu_fpregs_owner_ctx, this_cpu) = &x86_init_fpu;
- x86_init_fpu.last_cpu = this_cpu;
- }
+ fpstate_reset(&x86_init_fpu);
+ current->thread.fpu = &x86_init_fpu;
+ per_cpu(fpu_fpregs_owner_ctx, this_cpu) = &x86_init_fpu;
+ x86_init_fpu.last_cpu = this_cpu;

if (!boot_cpu_has(X86_FEATURE_CPUID) &&
!test_bit(X86_FEATURE_FPU, (unsigned long *)cpu_caps_cleared)) {
@@ -222,7 +220,6 @@ static void __init fpu__init_system_xstate_size_legacy(void)
void __init fpu__init_system(void)
{
fpu__init_system_early_generic();
- fpstate_reset(current->thread.fpu);

/*
* The FPU has to be operational for some of the