Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751487AbaABM5k (ORCPT ); Thu, 2 Jan 2014 07:57:40 -0500 Received: from mx1.redhat.com ([209.132.183.28]:65087 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750740AbaABM5i (ORCPT ); Thu, 2 Jan 2014 07:57:38 -0500 Message-ID: <52C56234.5030608@redhat.com> Date: Thu, 02 Jan 2014 07:57:24 -0500 From: Prarit Bhargava User-Agent: Mozilla/5.0 (X11; U; Linux x86_64; en-US; rv:1.9.2.17) Gecko/20110419 Red Hat/3.1.10-1.el6_0 Thunderbird/3.1.10 MIME-Version: 1.0 To: rui wang , Tony Luck , Linux Kernel Mailing List , Thomas Gleixner , Ingo Molnar , "H. Peter Anvin" , X86-ML , Michel Lespinasse , Andi Kleen , Seiji Aguchi , Yang Zhang , Paul Gortmaker , janet.morgan@intel.com, "Yu, Fenghua" , chen gong Subject: Re: [PATCH] x86: Add check for number of available vectors before CPU down [v2] References: <1387394945-5704-1-git-send-email-prarit@redhat.com> <52B336D4.8010809@redhat.com> <52BF060E.7090905@redhat.com> <52C18C6B.8090802@redhat.com> <52C33581.1030204@redhat.com> <20140102024138.GA28000@gchen.bj.intel.com> In-Reply-To: <20140102024138.GA28000@gchen.bj.intel.com> Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 2867 Lines: 75 On 01/01/2014 09:41 PM, Chen, Gong wrote: > On Tue, Dec 31, 2013 at 04:22:09PM -0500, Prarit Bhargava wrote: >> Okay, how about, >> if (irq_has_action(irq) && !irqd_is_per_cpu(data) && >> ((!cpumask_empty(&affinity_new)) && >> !cpumask_subset(&affinity_new, &online_new)) || >> cpumask_empty(&affinity_new)) >> this_count++; >> > I think it is good but a little bit complicated. How about this: > > if (irq_has_action(irq) && !irqd_is_per_cpu(data) && > /* add some commments to emphysize the importance of turn */ > (cpumask_empty(&affinity_new) || > !cpumask_subset(&affinity_new, &online_new))) Yeah :) I thought of that after I sent it. :) > >> I tried this with the following examples and AFAICT I get the correct result: >> >> 1) affinity mask = online mask = 0xf. CPU 3 (1000b) is down'd. >> >> this_count is not incremented. >> >> 2) affinity mask is a non-zero subset of the online mask (which IMO is >> the "typical" case). For example, affinity_mask = 0x9, online mask = 0xf. CPU >> 3 is again down'd. >> >> this_count is not incremented. >> >> 3) affinity_mask = 0x1, online mask = 0x3. (this is your example). CPU >> 1 is going down. >> >> this_count is incremented, as the resulting affinity mask will be 0. >> >> 4) affinity_mask = 0x0, online mask = 0x7. CPU 1 is going down. >> >> this_count is incremented, as the affinity mask is 0. >> > The 4th scenario is very tricky. If you try to set affinity from user space, > it will return failure because before kernel tried to change the affinity it > will verify it: > int __ioapic_set_affinity(...) > { > ... > if (!cpumask_intersects(mask, cpu_online_mask)) > return -EINVAL; > ... > } > > So from this point of view, affinity can't be 0. But your patch is very > special because you change it by hand: > cpu_clear(smp_processor_id(), affinity_new); > > so it is reasonable. It makes me thinking a little bit more. In fixup_irqs > we have similar logic but we don't protect it. Maybe it is because currently > the scenario 4 can't happen because we stop it in advance. But who knows > if one day we use it in other situation we will hit this subtle issue > probably. > > So, Prarit, I suggest you writing another patch to fix this potential issue > for fixup_irqs. How would you think? As you know Rui, I've been staring at that code wondering if it needs a fix. I'd like to hear Gong Chen's thoughts about it too... P. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/