Received-SPF: pass (google.com: domain of linux-kernel+bounces-207985-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:4601:e00::3 as permitted sender) client-ip=2604:1380:4601:e00::3;
Date: Mon, 10 Jun 2024 12:01:34 +0200
From: Oleg Nesterov <oleg@redhat.com>
To: Ingo Molnar <mingo@kernel.org>
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>, Brian Gerst <brgerst@gmail.com>,
	"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 6/9] x86/fpu: Make sure x86_task_fpu() doesn't get called
 for PF_KTHREAD tasks during exit
Message-ID: <20240610100133.GA20640@redhat.com>
References: <20240608073134.264210-1-mingo@kernel.org>
 <20240608073134.264210-7-mingo@kernel.org>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20240608073134.264210-7-mingo@kernel.org>
User-Agent: Mutt/1.5.24 (2015-08-30)

The whole series looks good to me, and afaics 7/9 allows more
cleanups / improvements.

But let me ask a stupid question about fpu__drop(), I know nothing
about fpu asm.

fpu__drop() does

	/* Ignore delayed exceptions from user space */
	asm volatile("1: fwait\n"

and this comment predates the git history. Could someone explain
why exactly the exiting user-space thread needs fwait ?

And if it is needed, suppose that a kernel thread exits right
after kernel_fpu_end(), can this lead to the delayed exception?


And otoh, perhaps fpu__drop() can set TIF_NEED_FPU_LOAD to avoid
switch_fpu_prepare()->save_fpregs_to_fpstate() on its path to the
final schedule?


On 06/08, Ingo Molnar wrote:
>
>  void fpu__drop(struct task_struct *tsk)
>  {
> -	struct fpu *fpu = x86_task_fpu(tsk);
> +	struct fpu *fpu;
> +
> +	/* PF_KTHREAD tasks do not use the FPU context area: */
> +	if (tsk->flags & PF_KTHREAD)
> +		return;

I think it can already do

	if (tsk->flags & (PF_KTHREAD | PF_USER_WORKER))
		return;

This matches other similar checks. But I won't insist, and I
think all these checks need some cleanups anyway.

Oleg.