From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754723AbbCFPBV (ORCPT ); Fri, 6 Mar 2015 10:01:21 -0500 Received: from smtp.citrix.com ([66.165.176.89]:61533 "EHLO SMTP.CITRIX.COM" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753027AbbCFPBU (ORCPT ); Fri, 6 Mar 2015 10:01:20 -0500 X-IronPort-AV: E=Sophos;i="5.11,353,1422921600"; d="scan'208";a="240930557" Message-ID: <54F9C112.3010604@citrix.com> Date: Fri, 6 Mar 2015 15:00:34 +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 , Ingo Molnar CC: 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> In-Reply-To: <20150306140154.GA22811@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 14:01, Oleg Nesterov wrote: > On 03/06, Ingo Molnar wrote: >> >> * Oleg Nesterov wrote: >> >>> On 03/06, Ingo Molnar wrote: >>>> >>>> * Oleg Nesterov wrote: >>>> >>>>> [...] The patch above looks "obviously safe", but perhaps I am >>>>> paranoid too much... >>>> >>>> IMHO your hack above isn't really acceptable, even for a backport. >>>> So lets test the patch below (assuming it's the right thing to do) >>>> and move forward? >>> >>> OK, but please note that this patch is not beckportable. If you think >>> that -stable doesn't need this fix, then I agree. >>> >>> If the caller is do_device_not_available(), then we can not enable >>> irqs before __thread_fpu_begin() + restore_fpu_checking(). >>> >>> 1. Preemption in between can destroy ->fpu.state initialized by >>> fpu_finit(), __switch_to() will save the live (wrong) FPU state >>> again. >>> >>> 2. kernel_fpu_begin() from irq right after __thread_fpu_begin() is >>> not nice too. It will do __save_init_fpu() and this overwrites >>> ->fpu.state too. >>> >>> Starting from v4.0 it does kernel_fpu_disable(), but the older kernels >>> do not. >>> >>> Ingo, this code is really horrible and fragile. We need to cleanup it >>> step-by-step, imho. >> >> How about the patch from David Vrabel? That seems to solve the >> irq-disable problem too, right? > > I wasn't cc'ed, I guess you mean > > [PATCHv4] x86, fpu: remove the logic of non-eager fpu mem allocation at the first usage > http://marc.info/?l=linux-kernel&m=142564237705311&w=2 This patch is from Suresh, and was originally against 3.10, so... > 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? David