From owner-svn-src-head@FreeBSD.ORG Mon Apr 9 17:59:16 2012 Return-Path: Delivered-To: svn-src-head@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [69.147.83.52]) by hub.freebsd.org (Postfix) with ESMTP id EE3151065672; Mon, 9 Apr 2012 17:59:16 +0000 (UTC) (envelope-from asmrookie@gmail.com) Received: from mail-lpp01m010-f54.google.com (mail-lpp01m010-f54.google.com [209.85.215.54]) by mx1.freebsd.org (Postfix) with ESMTP id 651F58FC08; Mon, 9 Apr 2012 17:59:15 +0000 (UTC) Received: by lagv3 with SMTP id v3so5068454lag.13 for ; Mon, 09 Apr 2012 10:59:14 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20120113; h=mime-version:sender:in-reply-to:references:date :x-google-sender-auth:message-id:subject:from:to:cc:content-type :content-transfer-encoding; bh=Zwpz0DI3BX5TsxzNVEYjj7X16I7UgVnvYCXcjsSBIL8=; b=INoPeIwP2t25k6hPGq3W20VjeXD5Dg7uXpzN6xpwK+17KZpbh8i6BkHmGkI+lddnPU KsB2mCl6xD4FrROtJyK8ECIyAk/l1B+RJt7HNfxUXvSR100BiJkhjW6WaDbfLvXY5wfA 5tj/9u6zqdagrUXNI98O/vo60k3Bh/pntLguDrvlhVTS25G1BHAzT+9KLoqiT0BEkplN KrK/s5SDgmr/fu8E+eMe9JQ/SaBqq4w66lkS7Q96sxrd70/ICU56jsvjARIFGsSv3CHi T3YWyyFxpN5839DsgZY58+6L3nPva623K+DrtkDQqmQMWNlEmNPg0xCIm3maFGV7giHk sbkA== MIME-Version: 1.0 Received: by 10.112.37.132 with SMTP id y4mr93593lbj.8.1333994354009; Mon, 09 Apr 2012 10:59:14 -0700 (PDT) Sender: asmrookie@gmail.com Received: by 10.112.93.138 with HTTP; Mon, 9 Apr 2012 10:59:13 -0700 (PDT) In-Reply-To: <201204091314.20775.jhb@freebsd.org> References: <201204062119.q36LJTKR026564@svn.freebsd.org> <201204091234.43106.jhb@freebsd.org> <201204091314.20775.jhb@freebsd.org> Date: Mon, 9 Apr 2012 18:59:13 +0100 X-Google-Sender-Auth: ZAgkR9QrWAo3URVTpKFZMW9Xmzk Message-ID: From: Attilio Rao To: John Baldwin Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable Cc: svn-src-head@freebsd.org, Jaakko Heinonen , svn-src-all@freebsd.org, "Justin T. Gibbs" , src-committers@freebsd.org Subject: Re: svn commit: r233961 - head/sys/x86/x86 X-BeenThere: svn-src-head@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: SVN commit messages for the src tree for head/-current List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 09 Apr 2012 17:59:17 -0000 Il 09 aprile 2012 18:14, John Baldwin ha scritto: > On Monday, April 09, 2012 12:58:07 pm Attilio Rao wrote: >> Il 09 aprile 2012 17:34, John Baldwin ha scritto: >> > On Monday, April 09, 2012 11:45:11 am Jaakko Heinonen wrote: >> >> >> >> Hi, >> >> >> >> On 2012-04-06, Justin T. Gibbs wrote: >> >> > =C2=A0 Fix interrupt load balancing regression, introduced in revis= ion >> >> > =C2=A0 222813, that left all un-pinned interrupts assigned to CPU 0= . >> >> > >> >> > =C2=A0 sys/x86/x86/intr_machdep.c: >> >> > =C2=A0 =C2=A0 In intr_shuffle_irqs(), remove CPU_SETOF() call that = initialized >> >> > =C2=A0 =C2=A0 the "intr_cpus" cpuset to only contain CPU0. >> >> > >> >> > =C2=A0 =C2=A0 This initialization is too late and nullifies the res= ults of calls >> >> > =C2=A0 =C2=A0 the intr_add_cpu() that occur much earlier in the boo= t process. >> >> > =C2=A0 =C2=A0 Since "intr_cpus" is statically initialized to the em= pty set, and >> >> > =C2=A0 =C2=A0 all processors, including the BSP, already add themse= lves to >> >> > =C2=A0 =C2=A0 "intr_cpus" no special initialization for the BSP is = necessary. >> >> >> >> My Pentium 4 system hangs on boot after this commit. These are the la= st >> >> lines from a verbose boot: >> >> >> >> SMP: AP CPU #1 Launched! >> >> cpu1 AP: >> >> =C2=A0 =C2=A0 =C2=A0ID: 0x01000000 =C2=A0 VER: 0x00050014 LDR: 0x0000= 0000 DFR: 0xffffffff >> >> =C2=A0 lint0: 0x00010700 lint1: 0x00000400 TPR: 0x00000000 SVR: 0x000= 001ff >> >> =C2=A0 timer: 0x000100ef therm: 0x00010000 err: 0x000000f0 pmc: 0x000= 10400 >> >> >> >> The system boots with r233960. >> >> >> >> Some information: >> >> >> >> CPU: Intel(R) Pentium(R) 4 CPU 2.60GHz (2605.96-MHz 686-class CPU) >> >> =C2=A0 Origin =3D "GenuineIntel" =C2=A0Id =3D 0xf29 =C2=A0Family =3D = f =C2=A0Model =3D 2 =C2=A0Stepping =3D >> >> 9 >> >> >> > Features=3D0xbfebfbff >> >> =C2=A0 Features2=3D0x4400 >> >> real memory =C2=A0=3D 2147483648 (2048 MB) >> >> avail memory =3D 2085228544 (1988 MB) >> >> Event timer "LAPIC" quality 400 >> >> ACPI APIC Table: >> >> FreeBSD/SMP: Multiprocessor System Detected: 2 CPUs >> >> FreeBSD/SMP: 1 package(s) x 1 core(s) x 2 HTT threads >> >> =C2=A0cpu0 (BSP): APIC ID: =C2=A00 >> >> =C2=A0cpu1 (AP/HT): APIC ID: =C2=A01 >> > >> > I suspect in your case intr_add_cpu() is never called. =C2=A0I think A= ttilio is not >> > correct in that it is not called for the BSP. >> > >> > Yes, it is not called for the BSP in set_interrupt_apic_ids(). =C2=A0T= his used to >> > work because bit 0 was assigned statically. =C2=A0Also, in a UP machin= e >> > set_interrupt_apic_ids() is not called at all. >> >> But why there is a front-end check for the BSP in set_interrupt_apic_ids= ()? >> >> Anyway, I think a better fix would be like the attached patch. > > This would be fine. =C2=A0What I would really prefer is to not need the s= ysinit at > all and be able to do something like the original pre-cpuset code: > > static cpuset_t intr_cpus =3D CPU_INITIAILIZER(0); This is more difficult to do because it would require static array initializations and it would pollute too much the code with compile time, MAXCPU-dependant details. > Also, with the cpuset variant, I think we could remove the special case c= heck > for the BSP from set_apic_interrupt_ids() as it doesn't hurt to set it > multiple times. =C2=A0 IIRC, the pre-cpuset code kept a separate count wh= ich is > why that would have been harmful. I'm not sure I follow, a separate count for what? So do you consider the following patch as a real commit candidate? Thanks, Attilio Index: sys/i386/i386/mp_machdep.c =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D --- sys/i386/i386/mp_machdep.c (revisione 234064) +++ sys/i386/i386/mp_machdep.c (copia locale) @@ -819,8 +819,6 @@ init_secondary(void) * We tell the I/O APIC code about all the CPUs we want to receive * interrupts. If we don't want certain CPUs to receive IRQs we * can simply not tell the I/O APIC code about them in this function. - * We also do not tell it about the BSP since it tells itself about - * the BSP internally to work with UP kernels and on UP machines. */ static void set_interrupt_apic_ids(void) @@ -831,8 +829,6 @@ set_interrupt_apic_ids(void) apic_id =3D cpu_apic_ids[i]; if (apic_id =3D=3D -1) continue; - if (cpu_info[apic_id].cpu_bsp) - continue; if (cpu_info[apic_id].cpu_disabled) continue; Index: sys/i386/i386/machdep.c =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D --- sys/i386/i386/machdep.c (revisione 234064) +++ sys/i386/i386/machdep.c (copia locale) @@ -336,6 +336,11 @@ cpu_startup(dummy) #ifndef XEN cpu_setregs(); #endif + + /* + * Add BSP interrupt bitmask. + */ + intr_add_cpu(0); } /* Index: sys/amd64/amd64/mp_machdep.c =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D --- sys/amd64/amd64/mp_machdep.c (revisione 234064) +++ sys/amd64/amd64/mp_machdep.c (copia locale) @@ -785,8 +785,6 @@ init_secondary(void) * We tell the I/O APIC code about all the CPUs we want to receive * interrupts. If we don't want certain CPUs to receive IRQs we * can simply not tell the I/O APIC code about them in this function. - * We also do not tell it about the BSP since it tells itself about - * the BSP internally to work with UP kernels and on UP machines. */ static void set_interrupt_apic_ids(void) @@ -797,8 +795,6 @@ set_interrupt_apic_ids(void) apic_id =3D cpu_apic_ids[i]; if (apic_id =3D=3D -1) continue; - if (cpu_info[apic_id].cpu_bsp) - continue; if (cpu_info[apic_id].cpu_disabled) continue; Index: sys/amd64/amd64/machdep.c =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D --- sys/amd64/amd64/machdep.c (revisione 234064) +++ sys/amd64/amd64/machdep.c (copia locale) @@ -295,6 +295,11 @@ cpu_startup(dummy) vm_pager_bufferinit(); cpu_setregs(); + + /* + * Add BSP interrupt bitmask. + */ + intr_add_cpu(0); } /*