Received-SPF: pass (google.com: domain of linux-kernel+bounces-119771-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) client-ip=147.75.80.249;
Sender: Ingo Molnar <mingo.kernel.org@gmail.com>
Date: Tue, 26 Mar 2024 20:12:31 +0100
From: Ingo Molnar <mingo@kernel.org>
To: Oleg Nesterov <oleg@redhat.com>
Cc: linux-kernel@vger.kernel.org, Andy Lutomirski <luto@amacapital.net>,
	Andrew Morton <akpm@linux-foundation.org>,
	Dave Hansen <dave@sr71.net>, Peter Zijlstra <peterz@infradead.org>,
	Borislav Petkov <bp@alien8.de>, "H . Peter Anvin" <hpa@zytor.com>,
	Linus Torvalds <torvalds@linux-foundation.org>,
	Thomas Gleixner <tglx@linutronix.de>,
	Uros Bizjak <ubizjak@gmail.com>
Subject: Re: [PATCH 1/1] headers/deps: x86/fpu: Make task_struct::thread
 constant size
Message-ID: <ZgMeH9p0BTnMfmOM@gmail.com>
References: <20240320131908.2708438-1-mingo@kernel.org>
 <20240320131908.2708438-2-mingo@kernel.org>
 <20240326174903.GA4539@redhat.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20240326174903.GA4539@redhat.com>


* 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