Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S932267AbcCMH0b (ORCPT ); Sun, 13 Mar 2016 03:26:31 -0400 Received: from mail-oi0-f66.google.com ([209.85.218.66]:35746 "EHLO mail-oi0-f66.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752145AbcCMH0W (ORCPT ); Sun, 13 Mar 2016 03:26:22 -0400 MIME-Version: 1.0 In-Reply-To: References: From: Jianyu Zhan Date: Sun, 13 Mar 2016 15:25:41 +0800 Message-ID: Subject: Re: [PATCH 3/3] x86/irq: update first_system_vector only when X86_LOCAL_PIC is on To: Thomas Gleixner Cc: mingo@redhat.com, "H. Peter Anvin" , Aravind.Gopalakrishnan@amd.com, brgerst@gmail.com, bp@suse.de, feng.wu@intel.com, jiang.liu@linux.intel.com, Tejun Heo , dvlasenk@redhat.com, penberg@cs.helsinki.fi, Yinghai Lu , andi@firstfloor.org, Andy Lutomirski , ajm@sgi.com, Yinghai Lu , Akinobu Mita , x86@kernel.org, LKML Content-Type: text/plain; charset=UTF-8 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 3168 Lines: 111 On Sun, Mar 13, 2016 at 2:35 PM, Thomas Gleixner wrote: >> On Sun, Mar 13, 2016 at 4:08 AM, Thomas Gleixner wrote: >> > This is pointless, because it's only called when local apic is enabled as all >> > call sites of alloc_intr_gate() depend on CONFIG_X86_LOCAL_APIC .... >> >> Not exactly, currently at least smp_intr_init() DOES NOT depend on >> CONFIG_X86_LOCAL_APIC: >> >> static void __init smp_intr_init(void) >> { >> #ifdef CONFIG_SMP > > It does, because CONFIG_SMP enables CONFIG_X86_LOCAL_APIC > It is. Once CONFIG_SMP is on, CONFIG_X86_LOCAL_APIC will be turned on. So the situation should be described as: first_system_vector will be updated on system vector init, on SMP case(which indirectly implies CONFIG_X86_LOCAL_APIC) and on explicit CONFIG_X86_LOCAL_APIC(X86_UP_APIC). So initial code: #ifndef CONFIG_X86_LOCAL_APIC #define first_system_vector NR_VECTORS #endif is actually not needed, becaused it won't ever change on ! CONFIG_X86_LOCAL_APIC case. I will clarify this in v2. >> I know this is weird, because modern SMP machines implies Local APIC. >> But currently we have CONFIG_SMP detangle from CONFIG_X86_LOCAL_APIC, >> which I think is fine. > > Do you actually understand how all that works together? > So the dependency should be reversed, and it should be like this: Currently we have CONFIG_X86_LOCAL_APIC detangle from CONFIG_SMP (we can enable CONFIG_X86_LOCAL_APIC on 32-bit uniprocessor). >> Another place which is weird is CONFIG_IRQ_WORK. Technically, it >> does not depend >> on SMP, nor even necessary Local APIC. Actually, it is just a base >> configuration selected >> by others. But currently we have the > > Have you tried to enable it independent from CONFIG_X86_LOCAL_APIC? > I did. I can turn off SMP and did not use UP_APIC. But IRQ_WORK is still able be turn on(by selected by other subsystem). My purpose of posting this patch set is trying to make the system vector layout reveal this fact. we have SMP system vector defined first( these may rely on or not rely on CONFIG_X86_LOCAL_APIC), then comes the CONFIG_X86_LOCAL_APIC dependent vector definition. then comes the rest vector definition that not only depends on CONFIG_X86_LOCAL_APIC, but also others(like MCE, virtualization). Among these stands out IRQ_WORK, which neither depends on SMP nor CONFIG_X86_LOCAL_APIC. So the new init funciton is look like(from [2/3]): static void __init system_intr_init(void) { smp_intr_init(); #ifdef CONFIG_IRQ_WORK alloc_intr_gate(IRQ_WORK_VECTOR, irq_work_interrupt); #endif apic_intr_init(); } >> >> i = FIRST_EXTERNAL_VECTOR; >> >> -#ifndef CONFIG_X86_LOCAL_APIC >> >> -#define first_system_vector NR_VECTORS >> >> -#endif >> >> for_each_clear_bit_from(i, used_vectors, first_system_vector) { >> > >> > And how exactly is this here supposed to compile when CONFIG_X86_LOCAL_APIC=n? >> >> Dunno. I guess this code on !CONFIG_X86_LOCAL_APIC case hasn't been >> tested yet ? > > It's your job to at least compile test your patches not the job of others. > Will do in next round. Regards, Jianyu Zhan