Re: [PATCH 1/1] x86/fpu: math_state_restore() should not blindly disable irqs

[David Vrabel]

On 06/03/15 15:36, Oleg Nesterov wrote:
> On 03/06, David Vrabel wrote:
>>
>> On 06/03/15 14:01, Oleg Nesterov wrote:
>>
>>> Not sure I understand it correctly after the first quick look, but
>>>
>>> 1. It conflicts with the recent changes in tip/x86/fpu
>>>
>>> 2. fpu_ini() initializes current->thread.fpu.state. This looks unneeded,
>>>    the kernel threads no longer have FPU context and do not abuse CPU.
>>>
>>> 3. I can be easily wrong, but it looks buggy... Note that
>>>    arch_dup_task_struct() doesn't allocate child->fpu.state if
>>>    !tsk_used_math(parent).
>>
>> ...yes. It's bit-rotted a bit.
>>
>>> No, I do not think this patch is a good idea. Perhaps I am wrong, but I
>>> think we need other changes. And they should start from init_fpu().
>>
>> But the general principle of avoiding the allocation in the #NM handler
>> and hence avoiding the need to re-enable IRQs is still a good idea, yes?
> 
> This needs more discussion, but in short so far I think that fpu_alloc()
> from #NM exception is fine if user_mode(regs) == T.

I think a memory allocation here, where the only behaviour on a failure
is to kill the task, is (and has always been) a crazy idea.

Additionally, in a Xen PV guest the #NM handler is called with TS
already cleared by the hypervisor so the handler must not enable
interrupts (and thus potentially schedule another task) until after the
current task's fpu state has been restored.  If a task was scheduled
before restoring the FPU state, TS would be clear and that task will use
fpu state from a previous task.

> Just do_device_not_available() should simply do conditional_sti(), I think.
> Perhaps it can even enable irqs unconditionally, but we need to verify that
> this is 100% correct.
> 
> And I agree that "if (!tsk_used_math(tsk))" code in math_state_restore()
> should be removed. But not to avoid the allocation, and this needs other
> changes.
> 
> Oleg.
> 

--
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/