Messages in this thread | | | Date | Tue, 11 Mar 2008 13:57:39 -0700 | From | Suresh Siddha <> | Subject | Re: [patch 2/2] x86, fpu: lazy allocation of FPU area - v5 |
| |
On Tue, Mar 11, 2008 at 10:08:16AM +0100, Ingo Molnar wrote: > > * Suresh Siddha <suresh.b.siddha@intel.com> wrote: > > > asmlinkage void math_state_restore(void) > > { > > struct task_struct *me = current; > > - clts(); /* Allow maths ops (or we recurse) */ > > > > - if (!used_math()) > > - init_fpu(me); > > + if (!used_math()) { > > + local_irq_enable(); > > + /* > > + * does a slab alloc which can sleep > > + */ > > + if (init_fpu(me)) { > > + /* > > + * ran out of memory! > > + */ > > + do_group_exit(SIGKILL); > > + return; > > + } > > + local_irq_disable(); > > + } > > + > > + clts(); /* Allow maths ops (or we recurse) */ > > restore_fpu_checking(&me->thread.xstate->fxsave); > > task_thread_info(me)->status |= TS_USEDFPU; > > me->fpu_counter++; > > hm, three things: > > firstly, the clts is now done _after_ fpu_init() - are you sure that's > OK? We do it in this order so that FINIT [on older cpus] does not fault.
init_fpu() is getting called only if !used_math() and in this case, we don't do any FP operations in init_fpu()
> secondly, while i know you were responding to review feedback from > others, but the do_group_exit(SIGKILL) looks quite bad. It's totally > undebuggable to the user - not even a coredump will be generated AFAICS > - and the user has no idea that this all happened due to out-of-memory. > A (forced) SIGBUS is our usual answer to out-of-memory situations. [such > as when a pagetable allocation fails]
AFAICS, fault handler is doing do_group_exit(SIGKILL); under out-of-memory conditions while handling page fault.
Just want to make sure that the user doesn't see this signal.
force_sig() with SIGKILL/SIGBUS along with printk("out of memory! killing process") is fair enough, right?
> If you get review feedback that > suggests a crappy solution then please resist it! :-)
:) Didn't feel SIGKILL was completely crappy..
> > thirdly, the irq enable/disable worries me. Can it ever trigger in > kernel code that has irqs off? If it happens when kernel uses the FPU in > irqs-off sections (to do SSE optimized routines, etc.) then enabling > irqs is dangerous - the original callsite had it disabled for a reason.
Good point. But math_state_restore() should never happen between the kernel_fpu_begin() and end() sections. Otherwise, it will corrupt the user's FPU data.
Today, we make sure that we don't get device not available (DNA) exceptions in kernel_fpu_begin() by explicitly doing clts()
> At minimum we should add a debug check to math_state_restore(), > something like: > > WARN_ON_ONCE(!(regs->flags & X86_EFLAGS_IF)) > > (this means we need to pass regs to math_state_restore())
Based on above, do you think this is still needed? Even if it is needed, the check should be
BUG_ON(!user_mode(regs))
thanks, suresh
| |