From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753239AbdAZPFb (ORCPT ); Thu, 26 Jan 2017 10:05:31 -0500 Received: from mail-wm0-f66.google.com ([74.125.82.66]:32850 "EHLO mail-wm0-f66.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752641AbdAZPFa (ORCPT ); Thu, 26 Jan 2017 10:05:30 -0500 Date: Thu, 26 Jan 2017 16:05:25 +0100 From: Ingo Molnar To: Rik van Riel Cc: linux-kernel@vger.kernel.org, Andrew Morton , Andy Lutomirski , Borislav Petkov , Dave Hansen , Fenghua Yu , "H . Peter Anvin" , Linus Torvalds , Oleg Nesterov , Peter Zijlstra , Thomas Gleixner , Yu-cheng Yu Subject: [PATCH] x86/fpu: Unify the naming of the FPU register cache validity flags Message-ID: <20170126150525.GA31833@gmail.com> References: <1485429989-23340-1-git-send-email-mingo@kernel.org> <1485429989-23340-2-git-send-email-mingo@kernel.org> <1485440636.15964.47.camel@redhat.com> <20170126145349.GA24644@gmail.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20170126145349.GA24644@gmail.com> User-Agent: Mutt/1.5.24 (2015-08-30) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org * Ingo Molnar wrote: > What we could do is to unify the naming to explain all this a bit better - right > now there's very little indication that ->fpregs_cached is closely related to > fpu_fpregs_owner_ctx. > > For example we could rename them to: > > ->fpregs_cached => ->fpregs_owner [bool] > fpu_fpregs_owner_ctx => fpregs_owner_ctx [ptr] > > ? > > Clearing ->fpregs_owner or setting fpregs_owner_ctx to NULL invalidates the > cache and it's clear from the naming that the two values are closely related. Something like the patch below - only minimally tested. Thanks, Ingo ================> >>From d5b99e1e25f86d4880bf85588eb4a4769610dd47 Mon Sep 17 00:00:00 2001 From: Ingo Molnar Date: Thu, 26 Jan 2017 16:01:00 +0100 Subject: [PATCH] x86/fpu: Unify the naming of the FPU register cache validity flags Rik pointed out that the fpu_fpregs_owner_ctx and the ->fpregs_cached flags are still described in a confusing way. Clarify this some more by renaming them to: ->fpregs_cached => ->fpregs_owner [bool] fpu_fpregs_owner_ctx => fpregs_owner_ctx [ptr] ... which better expresses that they are a cache validity flag split into two parts, where the cache can be invalidated if any of the flags is cleared. Also describe this relationship more accurately in the fpu/types.h header. No change in functionality. Reported-by: Rik van Riel Cc: Andy Lutomirski Cc: Borislav Petkov Cc: Dave Hansen Cc: Fenghua Yu Cc: H. Peter Anvin Cc: Linus Torvalds Cc: Oleg Nesterov Cc: Thomas Gleixner Cc: Yu-cheng Yu Cc: Fenghua Yu Signed-off-by: Ingo Molnar --- arch/x86/include/asm/fpu/internal.h | 22 +++++++++++----------- arch/x86/include/asm/fpu/types.h | 12 ++++++++++-- arch/x86/include/asm/switch_to.h | 2 +- arch/x86/kernel/fpu/core.c | 4 ++-- arch/x86/kernel/smpboot.c | 2 +- 5 files changed, 25 insertions(+), 17 deletions(-) diff --git a/arch/x86/include/asm/fpu/internal.h b/arch/x86/include/asm/fpu/internal.h index e62eee2e989e..bbee00aac864 100644 --- a/arch/x86/include/asm/fpu/internal.h +++ b/arch/x86/include/asm/fpu/internal.h @@ -486,11 +486,11 @@ extern int copy_fpstate_to_sigframe(void __user *buf, void __user *fp, int size) * FPU context switch related helper methods: */ -DECLARE_PER_CPU(struct fpu *, fpu_fpregs_owner_ctx); +DECLARE_PER_CPU(struct fpu *, fpregs_owner_ctx); /* * The in-register FPU state for an FPU context on a CPU is assumed to be - * valid if fpu->fpregs_cached is still set, and if the fpu_fpregs_owner_ctx + * valid if fpu->fpregs_owner is still set, and if the fpregs_owner_ctx * matches the FPU. * * If the FPU register state is valid, the kernel can skip restoring the @@ -507,17 +507,17 @@ DECLARE_PER_CPU(struct fpu *, fpu_fpregs_owner_ctx); */ static inline void __cpu_invalidate_fpregs_state(void) { - __this_cpu_write(fpu_fpregs_owner_ctx, NULL); + __this_cpu_write(fpregs_owner_ctx, NULL); } static inline void __fpu_invalidate_fpregs_state(struct fpu *fpu) { - fpu->fpregs_cached = 0; + fpu->fpregs_owner = 0; } static inline int fpregs_state_valid(struct fpu *fpu, unsigned int cpu) { - return fpu == this_cpu_read_stable(fpu_fpregs_owner_ctx) && fpu->fpregs_cached; + return fpu == this_cpu_read_stable(fpregs_owner_ctx) && fpu->fpregs_owner; } /* @@ -526,13 +526,13 @@ static inline int fpregs_state_valid(struct fpu *fpu, unsigned int cpu) */ static inline void fpregs_deactivate(struct fpu *fpu) { - this_cpu_write(fpu_fpregs_owner_ctx, NULL); + this_cpu_write(fpregs_owner_ctx, NULL); trace_x86_fpu_regs_deactivated(fpu); } static inline void fpregs_activate(struct fpu *fpu) { - this_cpu_write(fpu_fpregs_owner_ctx, fpu); + this_cpu_write(fpregs_owner_ctx, fpu); trace_x86_fpu_regs_activated(fpu); } @@ -552,14 +552,14 @@ switch_fpu_prepare(struct fpu *old_fpu, int cpu) { if (old_fpu->fpstate_active) { if (!copy_fpregs_to_fpstate(old_fpu)) - old_fpu->fpregs_cached = 0; + old_fpu->fpregs_owner = 0; else - old_fpu->fpregs_cached = 1; + old_fpu->fpregs_owner = 1; - /* But leave fpu_fpregs_owner_ctx! */ + /* But leave fpregs_owner_ctx! */ trace_x86_fpu_regs_deactivated(old_fpu); } else { - old_fpu->fpregs_cached = 0; + old_fpu->fpregs_owner = 0; } } diff --git a/arch/x86/include/asm/fpu/types.h b/arch/x86/include/asm/fpu/types.h index 07452fbd7867..d15cbfe0e8c4 100644 --- a/arch/x86/include/asm/fpu/types.h +++ b/arch/x86/include/asm/fpu/types.h @@ -285,14 +285,22 @@ struct fpu { unsigned char fpstate_active; /* - * @fpregs_cached: + * @fpregs_owner: * * This flag tells us whether this context is loaded into a CPU * right now. * * This is set to 0 if a task is migrated to another CPU. + * + * NOTE: the fpregs_owner_ctx percpu pointer also has to point to + * this FPU context for the register cache to be valid. If any + * of these two flags is cleared then the cache is invalid. + * Some internals can access the context-flag more easily, + * others have easier access to the percpu variable. The + * FPU context-switching code has access to both so there's + * very little cost of having the cache indexed in two ways: */ - unsigned char fpregs_cached; + unsigned char fpregs_owner; /* * @state: diff --git a/arch/x86/include/asm/switch_to.h b/arch/x86/include/asm/switch_to.h index a7146dadb31d..7a4915dd0547 100644 --- a/arch/x86/include/asm/switch_to.h +++ b/arch/x86/include/asm/switch_to.h @@ -78,7 +78,7 @@ do { \ */ static inline void arch_task_migrate(struct task_struct *p) { - p->thread.fpu.fpregs_cached = 0; + p->thread.fpu.fpregs_owner = 0; } #define arch_task_migrate arch_task_migrate diff --git a/arch/x86/kernel/fpu/core.c b/arch/x86/kernel/fpu/core.c index 217e37029585..1b3bf98072fe 100644 --- a/arch/x86/kernel/fpu/core.c +++ b/arch/x86/kernel/fpu/core.c @@ -39,7 +39,7 @@ static DEFINE_PER_CPU(bool, in_kernel_fpu); /* * Track which context is using the FPU on the CPU: */ -DEFINE_PER_CPU(struct fpu *, fpu_fpregs_owner_ctx); +DEFINE_PER_CPU(struct fpu *, fpregs_owner_ctx); static void kernel_fpu_disable(void) { @@ -189,7 +189,7 @@ EXPORT_SYMBOL_GPL(fpstate_init); int fpu__copy(struct fpu *dst_fpu, struct fpu *src_fpu) { - dst_fpu->fpregs_cached = 0; + dst_fpu->fpregs_owner = 0; if (!src_fpu->fpstate_active || !static_cpu_has(X86_FEATURE_FPU)) return 0; diff --git a/arch/x86/kernel/smpboot.c b/arch/x86/kernel/smpboot.c index 46732dc3b73c..8bf24aa01878 100644 --- a/arch/x86/kernel/smpboot.c +++ b/arch/x86/kernel/smpboot.c @@ -1118,7 +1118,7 @@ int native_cpu_up(unsigned int cpu, struct task_struct *tidle) return err; /* the FPU context is blank, nobody can own it */ - per_cpu(fpu_fpregs_owner_ctx, cpu) = NULL; + per_cpu(fpregs_owner_ctx, cpu) = NULL; common_cpu_up(cpu, tidle);