Re: [Xen-devel] [PATCHv1] x86: don't schedule when handling #NM exception
From: Konrad Rzeszutek Wilk
Date: Wed Mar 19 2014 - 09:22:31 EST
On Mon, Mar 17, 2014 at 01:29:03PM +0000, David Vrabel wrote:
> On 17/03/14 03:43, H. Peter Anvin wrote:
> > On 03/16/2014 08:35 PM, Sarah Newman wrote:
> >> Can you please review my patch first? It's only enabled when absolutely required.
> >
> > It doesn't help. It means you're running on Xen, and you will have
> > processes subjected to random SIGKILL because they happen to touch the
> > FPU when the atomic pool is low.
> >
> > However, there is probably a happy medium: you don't actually need eager
> > FPU restore, you just need eager FPU *allocation*. We have been
> > intending to allocate the FPU state at task creation time for eagerfpu,
> > and Suresh Siddha has already produced such a patch; it just needs some
> > minor fixups due to an __init failure.
> >
> > http://lkml.kernel.org/r/1391325599.6481.5.camel@europa
> >
> > In the Xen case we could turn on eager allocation but not eager fpu. In
> > fact, it might be justified to *always* do eager allocation...
>
> The following patch does the always eager allocation. It's a fixup of
> Suresh's original patch.
>
Hey Peter,
I think this is the solution you were looking for?
Or are there some other subtle issues that you think lurk around?
Thanks!
> v2:
> - Allocate initial fpu state after xsave_init().
> - Only allocate initial FPU state on boot cpu.
>
> 8<-----------------------
> x86, fpu: remove the logic of non-eager fpu mem allocation at the first usage
>
> From: Suresh Siddha <sbsiddha@xxxxxxxxx>
>
> For non-eager fpu mode, thread's fpu state is allocated during the first
> fpu usage (in the context of device not available exception). This can be
> a blocking call and hence we enable interrupts (which were originally
> disabled when the exception happened), allocate memory and disable
> interrupts etc. While this saves 512 bytes or so per-thread, there
> are some issues in general.
>
> a. Most of the future cases will be anyway using eager
> FPU (because of processor features like xsaveopt, LWP, MPX etc) and
> they do the allocation at the thread creation itself. Nice to have
> one common mechanism as all the state save/restore code is
> shared. Avoids the confusion and minimizes the subtle bugs
> in the core piece involved with context-switch.
>
> b. If a parent thread uses FPU, during fork() we allocate
> the FPU state in the child and copy the state etc. Shortly after this,
> during exec() we free it up, so that we can later allocate during
> the first usage of FPU. So this free/allocate might be slower
> for some workloads.
>
> c. math_state_restore() is called from multiple places
> and it is error pone if the caller expects interrupts to be disabled
> throughout the execution of math_state_restore(). Can lead to subtle
> bugs like Ubuntu bug #1265841.
>
> Memory savings will be small anyways and the code complexity
> introducing subtle bugs is not worth it. So remove
> the logic of non-eager fpu mem allocation at the first usage.
>
> Signed-off-by: Suresh Siddha <sbsiddha@xxxxxxxxx>
> Signed-off-by: David Vrabel <david.vrabel@xxxxxxxxxx>
> ---
> arch/x86/kernel/i387.c | 22 +++++++++++++---------
> arch/x86/kernel/process.c | 6 ------
> arch/x86/kernel/traps.c | 16 ++--------------
> arch/x86/kernel/xsave.c | 2 --
> 4 files changed, 15 insertions(+), 31 deletions(-)
>
> diff --git a/arch/x86/kernel/i387.c b/arch/x86/kernel/i387.c
> index e8368c6..05aeae2 100644
> --- a/arch/x86/kernel/i387.c
> +++ b/arch/x86/kernel/i387.c
> @@ -5,6 +5,7 @@
> * General FPU state handling cleanups
> * Gareth Hughes <gareth@xxxxxxxxxxx>, May 2000
> */
> +#include <linux/bootmem.h>
> #include <linux/module.h>
> #include <linux/regset.h>
> #include <linux/sched.h>
> @@ -153,8 +154,15 @@ static void init_thread_xstate(void)
> * into all processes.
> */
>
> +static void __init fpu_init_boot_cpu(void)
> +{
> + current->thread.fpu.state =
> + alloc_bootmem_align(xstate_size, __alignof__(struct xsave_struct));
> +}
> +
> void fpu_init(void)
> {
> + static __refdata void (*boot_func)(void) = fpu_init_boot_cpu;
> unsigned long cr0;
> unsigned long cr4_mask = 0;
>
> @@ -189,6 +197,11 @@ void fpu_init(void)
> mxcsr_feature_mask_init();
> xsave_init();
> eager_fpu_init();
> +
> + if (boot_func) {
> + boot_func();
> + boot_func = NULL;
> + }
> }
>
> void fpu_finit(struct fpu *fpu)
> @@ -219,8 +232,6 @@ EXPORT_SYMBOL_GPL(fpu_finit);
> */
> int init_fpu(struct task_struct *tsk)
> {
> - int ret;
> -
> if (tsk_used_math(tsk)) {
> if (cpu_has_fpu && tsk == current)
> unlazy_fpu(tsk);
> @@ -228,13 +239,6 @@ int init_fpu(struct task_struct *tsk)
> return 0;
> }
>
> - /*
> - * Memory allocation at the first usage of the FPU and other state.
> - */
> - ret = fpu_alloc(&tsk->thread.fpu);
> - if (ret)
> - return ret;
> -
> fpu_finit(&tsk->thread.fpu);
>
> set_stopped_child_used_math(tsk);
> diff --git a/arch/x86/kernel/process.c b/arch/x86/kernel/process.c
> index 3fb8d95..cd9c190 100644
> --- a/arch/x86/kernel/process.c
> +++ b/arch/x86/kernel/process.c
> @@ -128,12 +128,6 @@ void flush_thread(void)
> flush_ptrace_hw_breakpoint(tsk);
> memset(tsk->thread.tls_array, 0, sizeof(tsk->thread.tls_array));
> drop_init_fpu(tsk);
> - /*
> - * Free the FPU state for non xsave platforms. They get reallocated
> - * lazily at the first use.
> - */
> - if (!use_eager_fpu())
> - free_thread_xstate(tsk);
> }
>
> static void hard_disable_TSC(void)
> diff --git a/arch/x86/kernel/traps.c b/arch/x86/kernel/traps.c
> index 57409f6..3265429 100644
> --- a/arch/x86/kernel/traps.c
> +++ b/arch/x86/kernel/traps.c
> @@ -623,20 +623,8 @@ void math_state_restore(void)
> {
> struct task_struct *tsk = current;
>
> - if (!tsk_used_math(tsk)) {
> - local_irq_enable();
> - /*
> - * does a slab alloc which can sleep
> - */
> - if (init_fpu(tsk)) {
> - /*
> - * ran out of memory!
> - */
> - do_group_exit(SIGKILL);
> - return;
> - }
> - local_irq_disable();
> - }
> + if (!tsk_used_math(tsk))
> + init_fpu(tsk);
>
> __thread_fpu_begin(tsk);
>
> diff --git a/arch/x86/kernel/xsave.c b/arch/x86/kernel/xsave.c
> index a4b451c..46f6266 100644
> --- a/arch/x86/kernel/xsave.c
> +++ b/arch/x86/kernel/xsave.c
> @@ -598,8 +598,6 @@ void xsave_init(void)
>
> static inline void __init eager_fpu_init_bp(void)
> {
> - current->thread.fpu.state =
> - alloc_bootmem_align(xstate_size, __alignof__(struct xsave_struct));
> if (!init_xstate_buf)
> setup_init_fpu_buf();
> }
> --
> 1.7.2.5
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@xxxxxxxxxxxxx
> http://lists.xen.org/xen-devel
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/