From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751963AbbCFQPn (ORCPT ); Fri, 6 Mar 2015 11:15:43 -0500 Received: from smtp02.citrix.com ([66.165.176.63]:2556 "EHLO SMTP02.CITRIX.COM" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751660AbbCFQPl (ORCPT ); Fri, 6 Mar 2015 11:15:41 -0500 X-IronPort-AV: E=Sophos;i="5.11,353,1422921600"; d="scan'208";a="242527108" Message-ID: <54F9D2A9.8030401@citrix.com> Date: Fri, 6 Mar 2015 16:15:37 +0000 From: David Vrabel User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Icedove/31.4.0 MIME-Version: 1.0 To: Oleg Nesterov CC: Ingo Molnar , Dave Hansen , Borislav Petkov , Andy Lutomirski , Linus Torvalds , Pekka Riikonen , Rik van Riel , Suresh Siddha , LKML , "Yu, Fenghua" , Quentin Casasnovas Subject: Re: [PATCH 1/1] x86/fpu: math_state_restore() should not blindly disable irqs References: <54F74F59.5070107@intel.com> <20150305195127.GA12657@redhat.com> <20150305195149.GB12657@redhat.com> <20150305201101.GA21571@gmail.com> <20150305212532.GA16890@redhat.com> <20150306075833.GA623@gmail.com> <20150306132634.GA20693@redhat.com> <20150306134601.GA11718@gmail.com> <20150306140154.GA22811@redhat.com> <54F9C112.3010604@citrix.com> <20150306153622.GA26939@redhat.com> In-Reply-To: <20150306153622.GA26939@redhat.com> Content-Type: text/plain; charset="windows-1252" Content-Transfer-Encoding: 7bit X-DLP: MIA1 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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. >