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

[Ingo Molnar]

* Oleg Nesterov <oleg@xxxxxxxxxx> 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