Re: [Xen-devel] [PATCHv1] x86: don't schedule when handling #NM exception

[David Vrabel <david.vrabel@xxxxxxxxxx>]

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.

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