LinuxLists.cc - [PATCH v2] x86/apic: Move pending intr check code into it's own function

2018-02-23 03:37:36

Subject: [PATCH v2] x86/apic: Move pending intr check code into it's own function

the pending interrupt check code is mixed with the local APIC setup code,
that looks messy.

Extract the related code, move it into a new function named
apic_pending_intr_clear().

bonus cleanups from Andy Shevchenko's suggestions:

- for() -> for_each_set_bit()
- printk() -> pr_err()

Signed-off-by: Dou Liyang <[email protected]>
---
changlog:
v1 --> v2:
-do some cleanup suggested by Andy
-rebase the patch on the tip/master tree

arch/x86/kernel/apic/apic.c | 99 ++++++++++++++++++++++++---------------------
1 file changed, 53 insertions(+), 46 deletions(-)

diff --git a/arch/x86/kernel/apic/apic.c b/arch/x86/kernel/apic/apic.c
index 84b244ec49ed..687457d9df41 100644
--- a/arch/x86/kernel/apic/apic.c
+++ b/arch/x86/kernel/apic/apic.c
@@ -1408,6 +1408,57 @@ static void lapic_setup_esr(void)
oldvalue, value);
}

+static void apic_pending_intr_clear(void)
+{
+ long long max_loops = cpu_khz ? cpu_khz : 1000000;
+ unsigned long long tsc = 0, ntsc;
+ unsigned int queued;
+ unsigned long value;
+ int i, j, acked = 0;
+
+ if (boot_cpu_has(X86_FEATURE_TSC))
+ tsc = rdtsc();
+ /*
+ * After a crash, we no longer service the interrupts and a pending
+ * interrupt from previous kernel might still have ISR bit set.
+ *
+ * Most probably by now CPU has serviced that pending interrupt and
+ * it might not have done the ack_APIC_irq() because it thought,
+ * interrupt came from i8259 as ExtInt. LAPIC did not get EOI so it
+ * does not clear the ISR bit and cpu thinks it has already serivced
+ * the interrupt. Hence a vector might get locked. It was noticed
+ * for timer irq (vector 0x31). Issue an extra EOI to clear ISR.
+ */
+ do {
+ queued = 0;
+ for (i = APIC_ISR_NR - 1; i >= 0; i--)
+ queued |= apic_read(APIC_IRR + i*0x10);
+
+ for (i = APIC_ISR_NR - 1; i >= 0; i--) {
+ value = apic_read(APIC_ISR + i*0x10);
+ for_each_set_bit(j, &value, 32) {
+ if (j) {
+ ack_APIC_irq();
+ acked++;
+ }
+ }
+ }
+ if (acked > 256) {
+ pr_err("LAPIC pending interrupts after %d EOI\n",
+ acked);
+ break;
+ }
+ if (queued) {
+ if (boot_cpu_has(X86_FEATURE_TSC) && cpu_khz) {
+ ntsc = rdtsc();
+ max_loops = (cpu_khz << 10) - (ntsc - tsc);
+ } else
+ max_loops--;
+ }
+ } while (queued && max_loops > 0);
+ WARN_ON(max_loops <= 0);
+}
+
/**
* setup_local_APIC - setup the local APIC
*
@@ -1417,13 +1468,7 @@ static void lapic_setup_esr(void)
static void setup_local_APIC(void)
{
int cpu = smp_processor_id();
- unsigned int value, queued;
- int i, j, acked = 0;
- unsigned long long tsc = 0, ntsc;
- long long max_loops = cpu_khz ? cpu_khz : 1000000;
-
- if (boot_cpu_has(X86_FEATURE_TSC))
- tsc = rdtsc();
+ unsigned int value;

if (disable_apic) {
disable_ioapic_support();
@@ -1475,45 +1520,7 @@ static void setup_local_APIC(void)
value &= ~APIC_TPRI_MASK;
apic_write(APIC_TASKPRI, value);

- /*
- * After a crash, we no longer service the interrupts and a pending
- * interrupt from previous kernel might still have ISR bit set.
- *
- * Most probably by now CPU has serviced that pending interrupt and
- * it might not have done the ack_APIC_irq() because it thought,
- * interrupt came from i8259 as ExtInt. LAPIC did not get EOI so it
- * does not clear the ISR bit and cpu thinks it has already serivced
- * the interrupt. Hence a vector might get locked. It was noticed
- * for timer irq (vector 0x31). Issue an extra EOI to clear ISR.
- */
- do {
- queued = 0;
- for (i = APIC_ISR_NR - 1; i >= 0; i--)
- queued |= apic_read(APIC_IRR + i*0x10);
-
- for (i = APIC_ISR_NR - 1; i >= 0; i--) {
- value = apic_read(APIC_ISR + i*0x10);
- for (j = 31; j >= 0; j--) {
- if (value & (1<<j)) {
- ack_APIC_irq();
- acked++;
- }
- }
- }
- if (acked > 256) {
- printk(KERN_ERR "LAPIC pending interrupts after %d EOI\n",
- acked);
- break;
- }
- if (queued) {
- if (boot_cpu_has(X86_FEATURE_TSC) && cpu_khz) {
- ntsc = rdtsc();
- max_loops = (cpu_khz << 10) - (ntsc - tsc);
- } else
- max_loops--;
- }
- } while (queued && max_loops > 0);
- WARN_ON(max_loops <= 0);
+ apic_pending_intr_clear();

/*
* Now that we are all set up, enable the APIC
--
2.14.3

2018-02-23 07:10:20

by Ingo Molnar

[permalink] [raw]

Subject: Re: [PATCH v2] x86/apic: Move pending intr check code into it's own function

* Dou Liyang <[email protected]> wrote:

> the pending interrupt check code is mixed with the local APIC setup code,
> that looks messy.
>
> Extract the related code, move it into a new function named
> apic_pending_intr_clear().
>
> bonus cleanups from Andy Shevchenko's suggestions:
>
> - for() -> for_each_set_bit()
> - printk() -> pr_err()

Please split the cleanups (and the cleanups suggested further below) into a
separate patch, so that there's a pure 'code movement' patch plus another patch
that is easy to review.

> + /*
> + * After a crash, we no longer service the interrupts and a pending
> + * interrupt from previous kernel might still have ISR bit set.
> + *
> + * Most probably by now CPU has serviced that pending interrupt and
> + * it might not have done the ack_APIC_irq() because it thought,
> + * interrupt came from i8259 as ExtInt. LAPIC did not get EOI so it
> + * does not clear the ISR bit and cpu thinks it has already serivced
> + * the interrupt. Hence a vector might get locked. It was noticed
> + * for timer irq (vector 0x31). Issue an extra EOI to clear ISR.
> + */
> + do {
> + queued = 0;
> + for (i = APIC_ISR_NR - 1; i >= 0; i--)
> + queued |= apic_read(APIC_IRR + i*0x10);
> +
> + for (i = APIC_ISR_NR - 1; i >= 0; i--) {
> + value = apic_read(APIC_ISR + i*0x10);
> + for_each_set_bit(j, &value, 32) {
> + if (j) {
> + ack_APIC_irq();
> + acked++;
> + }
> + }
> + }
> + if (acked > 256) {
> + pr_err("LAPIC pending interrupts after %d EOI\n",
> + acked);

Please don't
break the line of
printk's.

> + if (queued) {
> + if (boot_cpu_has(X86_FEATURE_TSC) && cpu_khz) {
> + ntsc = rdtsc();
> + max_loops = (cpu_khz << 10) - (ntsc - tsc);
> + } else
> + max_loops--;

unbalanced curly braces.

Thanks,

Ingo

2018-02-23 17:08:56

by kernel test robot

[permalink] [raw]

Subject: Re: [PATCH v2] x86/apic: Move pending intr check code into it's own function

Hi Dou,

Thank you for the patch! Yet something to
[auto build test ERROR on tip/auto-latest]
[also build test ERROR on v4.16-rc2 next-20180223]
[if your patch is applied to the wrong git
url: /> tree, please drop us a note to help improve the system]
commits/Dou-Liyang/x86-apic-Move-pending-intr-check-code-into-it-s-own-function/20180224-004030">https://github.com/0day-ci/linux/commits/Dou-Liyang/x86-apic-Move-pending-intr-check-code-into-it-s-own-function/20180224-004030
as .config)
build tree
'setup_local_APIC':
error: 'i' undeclared (first use in this function)
cpu);
each undeclared identifier is reported only once for each function it appears in
2018-02-23 1461
2008-01-30 1462 /**
2008-01-30 1463 * setup_local_APIC - setup the local APIC
2010-12-09 1464 *
2017-02-07 1465 * Used to setup local APIC while initializing BSP or bringing up APs.
2010-12-09 1466 * Always called with preemption disabled.
2008-01-30 1467 */
2018-02-14 1468 static void setup_local_APIC(void)
2005-04-16 1469 {
2010-12-09 1470 int cpu = smp_processor_id();
2018-02-23 1471 unsigned int value;
2005-04-16 1472
2009-01-14 1473 if (disable_apic) {
Kretzschmar 2011-02-22 1474 disable_ioapic_support();
2009-01-14 1475 return;
2009-01-14 1476 }
2009-01-14 1477
2008-08-24 1478 #ifdef CONFIG_X86_32
2008-08-24 1479 /* Pound the ESR really hard over the head with a big hammer - mbligh */
2009-01-28 1480 if (lapic_is_integrated() && apic->disable_esr) {
2008-08-24 1481 apic_write(APIC_ESR, 0);
2008-08-24 1482 apic_write(APIC_ESR, 0);
2008-08-24 1483 apic_write(APIC_ESR, 0);
2008-08-24 1484 apic_write(APIC_ESR, 0);
2008-08-24 1485 }
2008-08-24 1486 #endif
2009-09-21 1487 perf_events_lapic_init();
2008-08-24 1488
2005-04-16 1489 /*
2005-04-16 1490 * Double-check whether this APIC is really registered.
2005-04-16 1491 * This is meaningless in clustered apic mode, so we skip it.
2005-04-16 1492 */
2009-09-12 1493 BUG_ON(!apic->apic_id_registered());
2005-04-16 1494
2005-04-16 1495 /*
2005-04-16 1496 * Intel recommends to set DFR, LDR and TPR before enabling
2005-04-16 1497 * an APIC. See e.g. "AP-388 82489DX User's Manual" (Intel
2005-04-16 1498 * document number 292116). So here it goes...
2005-04-16 1499 */
2009-01-28 1500 apic->init_apic_ldr();
2005-04-16 1501
2011-01-23 1502 #ifdef CONFIG_X86_32
2011-01-23 1503 /*
2011-01-23 1504 * APIC LDR is initialized. If logical_apicid mapping was
2011-01-23 1505 * initialized during get_smp_config(), make sure it matches the
2011-01-23 1506 * actual value.
2011-01-23 1507 */
2011-01-23 @1508 i = early_per_cpu(x86_cpu_to_logical_apicid, cpu);
2011-01-23 1509 WARN_ON(i != BAD_APICID && i != logical_smp_processor_id());
2011-01-23 1510 /* always use the value from LDR */
2011-01-23 1511 early_per_cpu(x86_cpu_to_logical_apicid, cpu) =
2011-01-23 1512 logical_smp_processor_id();
2011-01-23 1513 #endif
2011-01-23 1514
2005-04-16 1515 /*
2005-04-16 1516 * Set Task Priority to 'accept all'. We never change this
2005-04-16 1517 * later on.
2005-04-16 1518 */
2005-04-16 1519 value = apic_read(APIC_TASKPRI);
2005-04-16 1520 value &= ~APIC_TPRI_MASK;
2006-01-11 1521 apic_write(APIC_TASKPRI, value);
2005-04-16 1522
2018-02-23 1523 apic_pending_intr_clear();
2006-03-25 1524
2006-03-25 1525 /*
2005-04-16 1526 * Now that we are all set up, enable the APIC
2005-04-16 1527 */
2005-04-16 1528 value = apic_read(APIC_SPIV);
2005-04-16 1529 value &= ~APIC_VECTOR_MASK;
2005-04-16 1530 /*
2005-04-16 1531 * Enable APIC
2005-04-16 1532 */
2005-04-16 1533 value |= APIC_SPIV_APIC_ENABLED;
2005-04-16 1534
by commit
x86: Add apic->x86_32_early_logical_apicid()
ection#d2f4bea6e9a6b892b9b7a0bcb7befcbda0b5f4b5a6e9"><[email protected]>
otection#37115b430c5a5e59505877525b4352195f421150430c"><[email protected]>
Open Source Technology Center
">https://lists.01.org/pipermail/kbuild-all Intel Corporation

Attachments:

(No filename) (8.59 kB)
.config.gz (33.37 kB)
Download all attachments

2018-02-23 17:22:54

by kernel test robot

[permalink] [raw]

Subject: Re: [PATCH v2] x86/apic: Move pending intr check code into it's own function

Hi Dou,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on tip/auto-latest]
[also build test ERROR on v4.16-rc2 next-20180223]
[cannot apply to tip/x86/core]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url: https://github.com/0day-ci/linux/commits/Dou-Liyang/x86-apic-Move-pending-intr-check-code-into-it-s-own-function/20180224-004030
config: i386-randconfig-x079-201807 (attached as .config)
compiler: gcc-7 (Debian 7.3.0-1) 7.3.0
reproduce:
# save the attached .config to linux build tree
make ARCH=i386

All errors (new ones prefixed by >>):

arch/x86/kernel/apic/apic.c: In function 'setup_local_APIC':
>> arch/x86/kernel/apic/apic.c:1508:2: error: 'i' undeclared (first use in this function); did you mean 'if'?
i = early_per_cpu(x86_cpu_to_logical_apicid, cpu);
^
if
arch/x86/kernel/apic/apic.c:1508:2: note: each undeclared identifier is reported only once for each function it appears in

vim +1508 arch/x86/kernel/apic/apic.c

0d827285 arch/x86/kernel/apic/apic.c Dou Liyang 2018-02-23 1461
0e078e2f arch/x86/kernel/apic_64.c Thomas Gleixner 2008-01-30 1462 /**
0e078e2f arch/x86/kernel/apic_64.c Thomas Gleixner 2008-01-30 1463 * setup_local_APIC - setup the local APIC
0aa002fe arch/x86/kernel/apic/apic.c Tejun Heo 2010-12-09 1464 *
543113d2 arch/x86/kernel/apic/apic.c Dou Liyang 2017-02-07 1465 * Used to setup local APIC while initializing BSP or bringing up APs.
0aa002fe arch/x86/kernel/apic/apic.c Tejun Heo 2010-12-09 1466 * Always called with preemption disabled.
0e078e2f arch/x86/kernel/apic_64.c Thomas Gleixner 2008-01-30 1467 */
b753a2b7 arch/x86/kernel/apic/apic.c Dou Liyang 2018-02-14 1468 static void setup_local_APIC(void)
^1da177e arch/x86_64/kernel/apic.c Linus Torvalds 2005-04-16 1469 {
0aa002fe arch/x86/kernel/apic/apic.c Tejun Heo 2010-12-09 1470 int cpu = smp_processor_id();
0d827285 arch/x86/kernel/apic/apic.c Dou Liyang 2018-02-23 1471 unsigned int value;
^1da177e arch/x86_64/kernel/apic.c Linus Torvalds 2005-04-16 1472
f1182638 arch/x86/kernel/apic.c Jan Beulich 2009-01-14 1473 if (disable_apic) {
7167d08e arch/x86/kernel/apic/apic.c Henrik Kretzschmar 2011-02-22 1474 disable_ioapic_support();
f1182638 arch/x86/kernel/apic.c Jan Beulich 2009-01-14 1475 return;
f1182638 arch/x86/kernel/apic.c Jan Beulich 2009-01-14 1476 }
f1182638 arch/x86/kernel/apic.c Jan Beulich 2009-01-14 1477
89c38c28 arch/x86/kernel/apic_64.c Cyrill Gorcunov 2008-08-24 1478 #ifdef CONFIG_X86_32
89c38c28 arch/x86/kernel/apic_64.c Cyrill Gorcunov 2008-08-24 1479 /* Pound the ESR really hard over the head with a big hammer - mbligh */
08125d3e arch/x86/kernel/apic.c Ingo Molnar 2009-01-28 1480 if (lapic_is_integrated() && apic->disable_esr) {
89c38c28 arch/x86/kernel/apic_64.c Cyrill Gorcunov 2008-08-24 1481 apic_write(APIC_ESR, 0);
89c38c28 arch/x86/kernel/apic_64.c Cyrill Gorcunov 2008-08-24 1482 apic_write(APIC_ESR, 0);
89c38c28 arch/x86/kernel/apic_64.c Cyrill Gorcunov 2008-08-24 1483 apic_write(APIC_ESR, 0);
89c38c28 arch/x86/kernel/apic_64.c Cyrill Gorcunov 2008-08-24 1484 apic_write(APIC_ESR, 0);
89c38c28 arch/x86/kernel/apic_64.c Cyrill Gorcunov 2008-08-24 1485 }
89c38c28 arch/x86/kernel/apic_64.c Cyrill Gorcunov 2008-08-24 1486 #endif
cdd6c482 arch/x86/kernel/apic/apic.c Ingo Molnar 2009-09-21 1487 perf_events_lapic_init();
89c38c28 arch/x86/kernel/apic_64.c Cyrill Gorcunov 2008-08-24 1488
^1da177e arch/x86_64/kernel/apic.c Linus Torvalds 2005-04-16 1489 /*
^1da177e arch/x86_64/kernel/apic.c Linus Torvalds 2005-04-16 1490 * Double-check whether this APIC is really registered.
^1da177e arch/x86_64/kernel/apic.c Linus Torvalds 2005-04-16 1491 * This is meaningless in clustered apic mode, so we skip it.
^1da177e arch/x86_64/kernel/apic.c Linus Torvalds 2005-04-16 1492 */
c2777f98 arch/x86/kernel/apic/apic.c Daniel Walker 2009-09-12 1493 BUG_ON(!apic->apic_id_registered());
^1da177e arch/x86_64/kernel/apic.c Linus Torvalds 2005-04-16 1494
^1da177e arch/x86_64/kernel/apic.c Linus Torvalds 2005-04-16 1495 /*
^1da177e arch/x86_64/kernel/apic.c Linus Torvalds 2005-04-16 1496 * Intel recommends to set DFR, LDR and TPR before enabling
^1da177e arch/x86_64/kernel/apic.c Linus Torvalds 2005-04-16 1497 * an APIC. See e.g. "AP-388 82489DX User's Manual" (Intel
^1da177e arch/x86_64/kernel/apic.c Linus Torvalds 2005-04-16 1498 * document number 292116). So here it goes...
^1da177e arch/x86_64/kernel/apic.c Linus Torvalds 2005-04-16 1499 */
a5c43296 arch/x86/kernel/apic.c Ingo Molnar 2009-01-28 1500 apic->init_apic_ldr();
^1da177e arch/x86_64/kernel/apic.c Linus Torvalds 2005-04-16 1501
6f802c4b arch/x86/kernel/apic/apic.c Tejun Heo 2011-01-23 1502 #ifdef CONFIG_X86_32
6f802c4b arch/x86/kernel/apic/apic.c Tejun Heo 2011-01-23 1503 /*
acb8bc09 arch/x86/kernel/apic/apic.c Tejun Heo 2011-01-23 1504 * APIC LDR is initialized. If logical_apicid mapping was
acb8bc09 arch/x86/kernel/apic/apic.c Tejun Heo 2011-01-23 1505 * initialized during get_smp_config(), make sure it matches the
acb8bc09 arch/x86/kernel/apic/apic.c Tejun Heo 2011-01-23 1506 * actual value.
6f802c4b arch/x86/kernel/apic/apic.c Tejun Heo 2011-01-23 1507 */
acb8bc09 arch/x86/kernel/apic/apic.c Tejun Heo 2011-01-23 @1508 i = early_per_cpu(x86_cpu_to_logical_apicid, cpu);
acb8bc09 arch/x86/kernel/apic/apic.c Tejun Heo 2011-01-23 1509 WARN_ON(i != BAD_APICID && i != logical_smp_processor_id());
acb8bc09 arch/x86/kernel/apic/apic.c Tejun Heo 2011-01-23 1510 /* always use the value from LDR */
6f802c4b arch/x86/kernel/apic/apic.c Tejun Heo 2011-01-23 1511 early_per_cpu(x86_cpu_to_logical_apicid, cpu) =
6f802c4b arch/x86/kernel/apic/apic.c Tejun Heo 2011-01-23 1512 logical_smp_processor_id();
6f802c4b arch/x86/kernel/apic/apic.c Tejun Heo 2011-01-23 1513 #endif
6f802c4b arch/x86/kernel/apic/apic.c Tejun Heo 2011-01-23 1514
^1da177e arch/x86_64/kernel/apic.c Linus Torvalds 2005-04-16 1515 /*
^1da177e arch/x86_64/kernel/apic.c Linus Torvalds 2005-04-16 1516 * Set Task Priority to 'accept all'. We never change this
^1da177e arch/x86_64/kernel/apic.c Linus Torvalds 2005-04-16 1517 * later on.
^1da177e arch/x86_64/kernel/apic.c Linus Torvalds 2005-04-16 1518 */
^1da177e arch/x86_64/kernel/apic.c Linus Torvalds 2005-04-16 1519 value = apic_read(APIC_TASKPRI);
^1da177e arch/x86_64/kernel/apic.c Linus Torvalds 2005-04-16 1520 value &= ~APIC_TPRI_MASK;
11a8e778 arch/x86_64/kernel/apic.c Andi Kleen 2006-01-11 1521 apic_write(APIC_TASKPRI, value);
^1da177e arch/x86_64/kernel/apic.c Linus Torvalds 2005-04-16 1522
0d827285 arch/x86/kernel/apic/apic.c Dou Liyang 2018-02-23 1523 apic_pending_intr_clear();
da7ed9f9 arch/x86_64/kernel/apic.c Vivek Goyal 2006-03-25 1524
da7ed9f9 arch/x86_64/kernel/apic.c Vivek Goyal 2006-03-25 1525 /*
^1da177e arch/x86_64/kernel/apic.c Linus Torvalds 2005-04-16 1526 * Now that we are all set up, enable the APIC
^1da177e arch/x86_64/kernel/apic.c Linus Torvalds 2005-04-16 1527 */
^1da177e arch/x86_64/kernel/apic.c Linus Torvalds 2005-04-16 1528 value = apic_read(APIC_SPIV);
^1da177e arch/x86_64/kernel/apic.c Linus Torvalds 2005-04-16 1529 value &= ~APIC_VECTOR_MASK;
^1da177e arch/x86_64/kernel/apic.c Linus Torvalds 2005-04-16 1530 /*
^1da177e arch/x86_64/kernel/apic.c Linus Torvalds 2005-04-16 1531 * Enable APIC
^1da177e arch/x86_64/kernel/apic.c Linus Torvalds 2005-04-16 1532 */
^1da177e arch/x86_64/kernel/apic.c Linus Torvalds 2005-04-16 1533 value |= APIC_SPIV_APIC_ENABLED;
^1da177e arch/x86_64/kernel/apic.c Linus Torvalds 2005-04-16 1534

:::::: The code at line 1508 was first introduced by commit
:::::: acb8bc09c6185e4d3d582d0076aaa6a89f19d8c5 x86: Add apic->x86_32_early_logical_apicid()

:::::: TO: Tejun Heo <[email protected]>
:::::: CC: Ingo Molnar <[email protected]>

---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation

Attachments:

(No filename) (8.62 kB)
.config.gz (29.42 kB)
Download all attachments