4.5.0+ panic when setup loop device

Quoted 2 lines · Peter Zijlstra

Could you please try? I'm not sure how this would explain your loop device bug fail, but it certainly pointed towards broken.

It definitely does not explain it. The wreckage that topo stuff causes is that it disables a cpu, but that really is not a reason for block/loop to explode.

Thanks,

tglx

Peter Zijlstra

maintainer

2016-03-17 10:26 UTC #6 lore raw

Quoted 7 lines · Thomas Gleixner

Quoted 2 lines · Peter Zijlstra
Could you please try? I'm not sure how this would explain your loop device bug fail, but it certainly pointed towards broken.
It definitely does not explain it. The wreckage that topo stuff causes is that it disables a cpu, but that really is not a reason for block/loop to explode.

Right. Sadly I could not reproduce that error on my machine. But we can at least start by fixing the 'obvious' problems and then maybe we get more clues ;-)

Thomas Gleixner

2016-03-17 11:41 UTC #7 lore raw

Quoted 6 lines · Peter Zijlstra

Quoted 5 lines · Thomas Gleixner
Quoted 2 lines · Peter Zijlstra
Could you please try? I'm not sure how this would explain your loop device bug fail, but it certainly pointed towards broken.

Quoted 6 lines

Quoted 2 lines
It definitely does not explain it. The wreckage that topo stuff causes is that it disables a cpu, but that really is not a reason for block/loop to explode.
Right. Sadly I could not reproduce that error on my machine. But we can at least start by fixing the 'obvious' problems and then maybe we get more clues ;-)

I'm able to reproduce by rejecting a cpu in that topology map function forcefully.

That stuff explodes, because the block-mq code assumes that cpu_possible_mask has no holes.

#define queue_for_each_ctx(q, ctx, i)                                   \
        for ((i) = 0; (i) < (q)->nr_queues &&                           \
             ({ ctx = per_cpu_ptr((q)->queue_ctx, (i)); 1; }); (i)++)

is what makes that assumption about a consecutive possible mask.

The cure for now is the patch below on top of PeterZ's patch.

But we have to clarify and document whether holes in cpu_possible_mask are not allowed at all or if code like the above is simply broken.

Thanks,

tglx
---

arch/x86/kernel/smpboot.c +0 -1

index 643dbdccf4bc..f2ed8a01f870 100644
@@ -345,7 +345,6 @@ static void __init smp_init_package_map(void)
 			continue;
 		pr_warn("CPU %u APICId %x disabled\n", cpu, apicid);
 		per_cpu(x86_bios_cpu_apicid, cpu) = BAD_APICID;
-		set_cpu_possible(cpu, false);
 		set_cpu_present(cpu, false);
 	}
 }

Peter Zijlstra

maintainer

2016-03-17 11:51 UTC #8 lore raw

Quoted 2 lines · Thomas Gleixner

But we have to clarify and document whether holes in cpu_possible_mask are not allowed at all or if code like the above is simply broken.

So the general rule is that cpumasks can have holes, and exempting one just muddles the water.

Therefore I'd call the code just plain broken.

Borislav Petkov

maintainer

2016-03-17 11:57 UTC #9 lore raw

Quoted 8 lines · Peter Zijlstra

Quoted 2 lines · Thomas Gleixner
But we have to clarify and document whether holes in cpu_possible_mask are not allowed at all or if code like the above is simply broken.
So the general rule is that cpumasks can have holes, and exempting one just muddles the water.
Therefore I'd call the code just plain broken.

I'll say.

Can't the code simply do:

	if (!cpu_possible(i))
		continue;

Signature · 4 lines

Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.

Thomas Gleixner

2016-03-17 12:03 UTC #10 lore raw

In reply to Thomas Gleixner

Quoted 8 lines · Peter Zijlstra

Quoted 2 lines · Thomas Gleixner
But we have to clarify and document whether holes in cpu_possible_mask are not allowed at all or if code like the above is simply broken.
So the general rule is that cpumasks can have holes, and exempting one just muddles the water.
Therefore I'd call the code just plain broken.

Agreed.

That macro is not really helping the readability of the code at all. So a simple for_each_possible_cpu() loop would have avoided that wreckage.

Thanks,

tglx

Jens Axboe

maintainer

2016-03-17 16:42 UTC #11 lore raw

Quoted 8 lines · Thomas Gleixner

Quoted 7 lines · Peter Zijlstra
Quoted 2 lines · Thomas Gleixner
But we have to clarify and document whether holes in cpu_possible_mask are not allowed at all or if code like the above is simply broken.
So the general rule is that cpumasks can have holes, and exempting one just muddles the water.

Quoted 6 lines

Quoted 1 line
Therefore I'd call the code just plain broken.
Agreed.
That macro is not really helping the readability of the code at all. So a simple for_each_possible_cpu() loop would have avoided that wreckage.

Does the attached work? The rest of blk-mq should deal with holes just fine, we found some of those issues on sparc. Not sure why this one slipped through the cracks.

Signature · 1 line

Jens Axboe

block/blk-mq.h +18 -0

index eaede8e45c9c..5a8ab6c2abe5 100644
@@ -118,4 +118,22 @@ static inline bool blk_mq_hw_queue_mapped(struct blk_mq_hw_ctx *hctx)
 	return hctx->nr_ctx && hctx->tags;
 }
 
+static inline struct blk_mq_ctx *next_ctx(struct request_queue *q, int *i)
+{
+	do {
+		(*i)++;
+		if (*i < q->nr_queues) {
+			if (cpu_possible(*i))
+				return per_cpu_ptr(q->queue_ctx, *i);
+			continue;
+		}
+		break;
+	} while (1);
+
+	return NULL;
+}
+
+#define queue_for_each_ctx(q, ctx, i)					\
+	for ((i) = 0; (ctx = next_ctx((q), &(i))) != NULL;)
+
 #endif

include/linux/blk-mq.h +0 ~~-14~~

index 7fc9296b5742..a8756cd00767 100644
@@ -261,22 +261,8 @@ static inline void *blk_mq_rq_to_pdu(struct request *rq)
 	for ((i) = 0; (i) < (q)->nr_hw_queues &&			\
 	     ({ hctx = (q)->queue_hw_ctx[i]; 1; }); (i)++)
 
-#define queue_for_each_ctx(q, ctx, i)					\
-	for ((i) = 0; (i) < (q)->nr_queues &&				\
-	     ({ ctx = per_cpu_ptr((q)->queue_ctx, (i)); 1; }); (i)++)
-
 #define hctx_for_each_ctx(hctx, ctx, i)					\
 	for ((i) = 0; (i) < (hctx)->nr_ctx &&				\
 	     ({ ctx = (hctx)->ctxs[(i)]; 1; }); (i)++)
 
-#define blk_ctx_sum(q, sum)						\
-({									\
-	struct blk_mq_ctx *__x;						\
-	unsigned int __ret = 0, __i;					\
-									\
-	queue_for_each_ctx((q), __x, __i)				\
-		__ret += sum;						\
-	__ret;								\
-})
-
 #endif

Jens Axboe

maintainer

2016-03-17 18:26 UTC #12 lore raw

Quoted 12 lines · Jens Axboe

Quoted 11 lines · Thomas Gleixner
Quoted 9 lines · Peter Zijlstra
Quoted 3 lines · Thomas Gleixner
But we have to clarify and document whether holes in cpu_possible_mask are not
allowed at all or if code like the above is simply broken.
So the general rule is that cpumasks can have holes, and exempting one just muddles the water.
Therefore I'd call the code just plain broken.

Quoted 8 lines

Quoted 4 lines
Agreed.
That macro is not really helping the readability of the code at all. So a simple for_each_possible_cpu() loop would have avoided that wreckage.
Does the attached work? The rest of blk-mq should deal with holes just fine, we found some of those issues on sparc. Not sure why this one slipped through the cracks.

This might be better, we need to start at -1 to not miss the first one... Still untested.

Signature · 1 line

Jens Axboe

block/blk-mq.h +18 -0

index eaede8e45c9c..ce68ceda652c 100644
@@ -118,4 +118,22 @@ static inline bool blk_mq_hw_queue_mapped(struct blk_mq_hw_ctx *hctx)
 	return hctx->nr_ctx && hctx->tags;
 }
 
+static inline struct blk_mq_ctx *next_ctx(struct request_queue *q, int *i)
+{
+	do {
+		(*i)++;
+		if (*i < q->nr_queues) {
+			if (cpu_possible(*i))
+				return per_cpu_ptr(q->queue_ctx, *i);
+			continue;
+		}
+		break;
+	} while (1);
+
+	return NULL;
+}
+
+#define queue_for_each_ctx(q, ctx, i)					\
+	for ((i) = -1; (ctx = next_ctx((q), &(i))) != NULL;)
+
 #endif

include/linux/blk-mq.h +0 ~~-14~~

index 7fc9296b5742..a8756cd00767 100644
@@ -261,22 +261,8 @@ static inline void *blk_mq_rq_to_pdu(struct request *rq)
 	for ((i) = 0; (i) < (q)->nr_hw_queues &&			\
 	     ({ hctx = (q)->queue_hw_ctx[i]; 1; }); (i)++)
 
-#define queue_for_each_ctx(q, ctx, i)					\
-	for ((i) = 0; (i) < (q)->nr_queues &&				\
-	     ({ ctx = per_cpu_ptr((q)->queue_ctx, (i)); 1; }); (i)++)
-
 #define hctx_for_each_ctx(hctx, ctx, i)					\
 	for ((i) = 0; (i) < (hctx)->nr_ctx &&				\
 	     ({ ctx = (hctx)->ctxs[(i)]; 1; }); (i)++)
 
-#define blk_ctx_sum(q, sum)						\
-({									\
-	struct blk_mq_ctx *__x;						\
-	unsigned int __ret = 0, __i;					\
-									\
-	queue_for_each_ctx((q), __x, __i)				\
-		__ret += sum;						\
-	__ret;								\
-})
-
 #endif

Thomas Gleixner

2016-03-17 20:22 UTC #13 lore raw

Quoted 11 lines · Jens Axboe

Quoted 10 lines · Jens Axboe
Quoted 9 lines · Thomas Gleixner
Quoted 8 lines · Peter Zijlstra
Quoted 3 lines · Thomas Gleixner
But we have to clarify and document whether holes in cpu_possible_mask are not
allowed at all or if code like the above is simply broken.
So the general rule is that cpumasks can have holes, and exempting one just muddles the water.

Quoted 8 lines

Quoted 8 lines
Quoted 6 lines
Quoted 1 line
Therefore I'd call the code just plain broken.
Agreed.
That macro is not really helping the readability of the code at all. So a simple for_each_possible_cpu() loop would have avoided that wreckage.
Does the attached work? The rest of blk-mq should deal with holes just

Bah. Attachements ...

Quoted 14 lines

Quoted 2 lines
fine, we found some of those issues on sparc. Not sure why this one slipped through the cracks.
This might be better, we need to start at -1 to not miss the first one... Still untested.
block/blk-mq.h +8 -0
+static inline struct blk_mq_ctx *next_ctx(struct request_queue *q, int *i)
+{
+	do {
+		(*i)++;
+		if (*i < q->nr_queues) {
+			if (cpu_possible(*i))
+				return per_cpu_ptr(q->queue_ctx, *i);
+			continue;

block/blk-mq.h static inline bool blk_mq_hw_queue_mapped(struct blk_mq_hw_ctx *hctx) · line 138

Quoted 10 lines

block/blk-mq.h +10 -0

+		}
+		break;
+	} while (1);
+
+	return NULL;
+}
+
+#define queue_for_each_ctx(q, ctx, i)					\
+	for ((i) = -1; (ctx = next_ctx((q), &(i))) != NULL;)
+

What's wrong with

        for_each_possible_cpu(cpu) {
       		ctx = per_cpu_ptr(q->queue_ctx, cpu);

		....
	}

instead of hiding it behind an incomprehensible macro mess?

Thanks,

tglx

Jens Axboe

maintainer

2016-03-17 20:23 UTC #14 lore raw

Quoted 14 lines · Thomas Gleixner

Quoted 13 lines · Jens Axboe
Quoted 12 lines · Jens Axboe
Quoted 11 lines · Thomas Gleixner
Quoted 9 lines · Peter Zijlstra
Quoted 3 lines · Thomas Gleixner
But we have to clarify and document whether holes in cpu_possible_mask are not
allowed at all or if code like the above is simply broken.
So the general rule is that cpumasks can have holes, and exempting one just muddles the water.
Therefore I'd call the code just plain broken.

Quoted 8 lines

Quoted 6 lines
Quoted 6 lines
Quoted 4 lines
Agreed.
That macro is not really helping the readability of the code at all. So a simple for_each_possible_cpu() loop would have avoided that wreckage.
Does the attached work? The rest of blk-mq should deal with holes just
Bah. Attachements ...

You'll live. Let's face it, all mailers suck in one way or another.

Quoted 27 lines

Quoted 14 lines
Quoted 2 lines
fine, we found some of those issues on sparc. Not sure why this one slipped through the cracks.
This might be better, we need to start at -1 to not miss the first one... Still untested.
block/blk-mq.h +8 -0
+static inline struct blk_mq_ctx *next_ctx(struct request_queue *q, int *i)
+{
+	do {
+		(*i)++;
+		if (*i < q->nr_queues) {
+			if (cpu_possible(*i))
+				return per_cpu_ptr(q->queue_ctx, *i);
+			continue;
block/blk-mq.h static inline bool blk_mq_hw_queue_mapped(struct blk_mq_hw_ctx *hctx) · line 138
Quoted 10 lines
block/blk-mq.h +10 -0
+		}
+		break;
+	} while (1);
+
+	return NULL;
+}
+
+#define queue_for_each_ctx(q, ctx, i)					\
+	for ((i) = -1; (ctx = next_ctx((q), &(i))) != NULL;)
+
What's wrong with

Quoted 7 lines

         for_each_possible_cpu(cpu) {
        		ctx = per_cpu_ptr(q->queue_ctx, cpu);

		....
	}
instead of hiding it behind an incomprehensible macro mess?

We might not have mapped all of them.

Signature · 1 line

Jens Axboe

Thomas Gleixner

2016-03-17 20:31 UTC #15 lore raw

Quoted 30 lines · Jens Axboe

Quoted 29 lines · Thomas Gleixner
Quoted 21 lines
This might be better, we need to start at -1 to not miss the first one... Still untested.
+static inline struct blk_mq_ctx *next_ctx(struct request_queue *q, int *i)
block/blk-mq.h +15 -0
+{
+	do {
+		(*i)++;
+		if (*i < q->nr_queues) {
+			if (cpu_possible(*i))
+				return per_cpu_ptr(q->queue_ctx, *i);
+			continue;
+		}
+		break;
+	} while (1);
+
+	return NULL;
+}
+
+#define queue_for_each_ctx(q, ctx, i)
\
block/blk-mq.h static inline bool blk_mq_hw_queue_mapped(struct blk_mq_hw_ctx *hctx) · line 138
Quoted 2 lines
block/blk-mq.h +2 -0
+	for ((i) = -1; (ctx = next_ctx((q), &(i))) != NULL;)
+
What's wrong with
         for_each_possible_cpu(cpu) {
        		ctx = per_cpu_ptr(q->queue_ctx, cpu);

Quoted 6 lines

Quoted 4 lines
		....
	}
instead of hiding it behind an incomprehensible macro mess?
We might not have mapped all of them.

blk_mq_init_cpu_queues() tells a different story and q->queue_ctx is a per_cpu allocation.

Thanks,

tglx

Jens Axboe

maintainer

2016-03-17 20:41 UTC #16 lore raw

Quoted 34 lines · Thomas Gleixner

Quoted 33 lines · Jens Axboe
Quoted 32 lines · Thomas Gleixner
Quoted 21 lines
This might be better, we need to start at -1 to not miss the first one... Still untested.
+static inline struct blk_mq_ctx *next_ctx(struct request_queue *q, int *i)
block/blk-mq.h +15 -0
+{
+	do {
+		(*i)++;
+		if (*i < q->nr_queues) {
+			if (cpu_possible(*i))
+				return per_cpu_ptr(q->queue_ctx, *i);
+			continue;
+		}
+		break;
+	} while (1);
+
+	return NULL;
+}
+
+#define queue_for_each_ctx(q, ctx, i)
\
block/blk-mq.h static inline bool blk_mq_hw_queue_mapped(struct blk_mq_hw_ctx *hctx) · line 138
Quoted 2 lines
block/blk-mq.h +2 -0
+	for ((i) = -1; (ctx = next_ctx((q), &(i))) != NULL;)
+
What's wrong with
          for_each_possible_cpu(cpu) {
         		ctx = per_cpu_ptr(q->queue_ctx, cpu);

		....
	}

Quoted 6 lines

Quoted 3 lines
Quoted 1 line
instead of hiding it behind an incomprehensible macro mess?
We might not have mapped all of them.
blk_mq_init_cpu_queues() tells a different story and q->queue_ctx is a per_cpu allocation.

Yeah my bad, I mistook the possible for online. So we can do the easier fix.

Signature · 1 line

Jens Axboe

Xiong Zhou

author

2016-03-18 02:31 UTC #17 lore raw

Hi,

Quoted 21 lines · Thomas Gleixner

Quoted 6 lines · Peter Zijlstra
Quoted 5 lines · Thomas Gleixner
Quoted 2 lines · Peter Zijlstra
Could you please try? I'm not sure how this would explain your loop device bug fail, but it certainly pointed towards broken.
Quoted 6 lines
Quoted 2 lines
It definitely does not explain it. The wreckage that topo stuff causes is that it disables a cpu, but that really is not a reason for block/loop to explode.
Right. Sadly I could not reproduce that error on my machine. But we can at least start by fixing the 'obvious' problems and then maybe we get more clues ;-)
I'm able to reproduce by rejecting a cpu in that topology map function forcefully.
That stuff explodes, because the block-mq code assumes that cpu_possible_mask has no holes.

Quoted 7 lines

#define queue_for_each_ctx(q, ctx, i)                                   \
        for ((i) = 0; (i) < (q)->nr_queues &&                           \
             ({ ctx = per_cpu_ptr((q)->queue_ctx, (i)); 1; }); (i)++)
is what makes that assumption about a consecutive possible mask.
The cure for now is the patch below on top of PeterZ's patch.

No panic with both Peter's patch and yours.

Thanks all.

--
Xiong

Quoted 28 lines

But we have to clarify and document whether holes in cpu_possible_mask are not allowed at all or if code like the above is simply broken.

Thanks,

tglx
---

arch/x86/kernel/smpboot.c +0 -1

index 643dbdccf4bc..f2ed8a01f870 100644
@@ -345,7 +345,6 @@ static void __init smp_init_package_map(void)
                        continue;
                pr_warn("CPU %u APICId %x disabled\n", cpu, apicid);
                per_cpu(x86_bios_cpu_apicid, cpu) = BAD_APICID;
-               set_cpu_possible(cpu, false);
                set_cpu_present(cpu, false);
        }
 }

Mike Galbraith

2016-03-18 04:12 UTC #18 lore raw

In reply to Mike Galbraith

Quoted 4 lines · Peter Zijlstra

Andreas; Borislav said to Cc you since you wrote all this. The issue is that Linux assumes:
> nr_logical_cpus = nr_cores * nr_siblings

It also seems to now assume that if SMT is possible, it's enabled.

Below is my 8 socket DL980 G7, which has SMT turned off for RT testing, booting NOPREEMPT master tuned for maximum bloat ala distro and getting confused by me telling it (as always) nr_cpus=64. Bad juju ensues.

[    0.216180] max_cores: 8, cpu_ids: 64, num_siblings: 2, coreid_bits: 5
[    0.226593] smpboot: Max logical packages: 4 <== not
[    0.233742] smpboot: APIC(0) Converting physical 0 to logical package 0
[    0.244233] smpboot: APIC(20) Converting physical 1 to logical package 1
[    0.253765] smpboot: APIC(40) Converting physical 2 to logical package 2
[    0.264081] smpboot: APIC(60) Converting physical 3 to logical package 3
[    0.274827] smpboot: APIC(80) Package 4 exceeds logical package map
[    0.284705] smpboot: CPU 32 APICId 80 disabled
[    0.292277] smpboot: APIC(a0) Package 5 exceeds logical package map
[    0.302141] smpboot: CPU 40 APICId a0 disabled
[    0.308607] smpboot: APIC(c0) Package 6 exceeds logical package map
[    0.321682] smpboot: CPU 48 APICId c0 disabled
[    0.328179] smpboot: APIC(e0) Package 7 exceeds logical package map
[    0.337902] smpboot: CPU 56 APICId e0 disabled
[    0.345695] DMAR: Host address width 40
[    0.351511] DMAR: DRHD base: 0x000000b0100000 flags: 0x0
[    0.360018] DMAR: dmar0: reg_base_addr b0100000 ver 1:0 cap c90780106f0462 ecap f0207e
[    0.373342] DMAR: DRHD base: 0x000000a8000000 flags: 0x1
[    0.383164] DMAR: dmar1: reg_base_addr a8000000 ver 1:0 cap c90780106f0462 ecap f0207e
[    0.396475] DMAR: RMRR base: 0x0000007f7ee000 end: 0x0000007f7effff
[    0.407255] DMAR: RMRR base: 0x0000007f7e7000 end: 0x0000007f7ecfff
[    0.418136] DMAR: RMRR base: 0x0000007f62e000 end: 0x0000007f62ffff
[    0.429787] DMAR: ATSR flags: 0x0
[    0.434778] DMAR: ATSR flags: 0x0
[    0.441624] DMAR-IR: IOAPIC id 10 under DRHD base  0xb0100000 IOMMU 0
[    0.452716] DMAR-IR: IOAPIC id 8 under DRHD base  0xa8000000 IOMMU 1
[    0.465782] DMAR-IR: IOAPIC id 0 under DRHD base  0xa8000000 IOMMU 1
[    0.477123] DMAR-IR: Queued invalidation will be enabled to support x2apic and Intr-remapping.
[    0.492918] DMAR-IR: Enabled IRQ remapping in x2apic mode
[    0.502549] x2apic enabled
[    0.506678] Switched APIC routing to cluster x2apic.
[    0.519955] ..TIMER: vector=0x30 apic1=0 pin1=2 apic2=-1 pin2=-1
[    0.642858] smpboot: CPU0: Intel(R) Xeon(R) CPU           X7560  @ 2.27GHz (family: 0x6, model: 0x2e, stepping: 0x6)
[    0.668111] Performance Events: PEBS fmt1+, 16-deep LBR, Nehalem events, Broken BIOS detected, complain to your hardware vendor.
[    0.694907] [Firmware Bug]: the BIOS has corrupted hw-PMU resources (MSR 38d is 330)
[    0.713186] Intel PMU driver.
[    0.719091] core: CPU erratum AAJ80 worked around
[    0.731647] core: CPUID marked event: 'bus cycles' unavailable
[    0.741499] ... version:                3
[    0.747982] ... bit width:              48
[    0.754109] ... generic registers:      4
[    0.760980] ... value mask:             0000ffffffffffff
[    0.769336] ... max period:             000000007fffffff
[    0.776913] ... fixed-purpose events:   3
[    0.783861] ... event mask:             000000070000000f
[    0.793737] x86: Booting SMP configuration:
[    0.800069] .... node  #0, CPUs:        #1  #2  #3  #4  #5  #6  #7  #8  #9 #10 #11 #12 #13 #14 #15 #16 #17 #18 #19 #20 #21 #22 #23 #24 #25 #26 #27 #28 #29 #30 #31 #33 #34 #35 #36 #37 #38 #39 #41 #42 #43 #44 #45 #46 #47 #49 #50 #51 #5>
[    4.717309] x86: Booted up 1 node, 60 CPUs
[    4.724551] smpboot: Total of 60 processors activated (271280.00 BogoMIPS)
[    5.007438] node 0 initialised, 1013474 pages in 36ms

Peter Zijlstra

maintainer

2016-03-18 07:52 UTC #19 lore raw

Quoted 8 lines · Mike Galbraith

Quoted 4 lines · Peter Zijlstra
Andreas; Borislav said to Cc you since you wrote all this. The issue is that Linux assumes:
> nr_logical_cpus = nr_cores * nr_siblings
It also seems to now assume that if SMT is possible, it's enabled.

Urgh..

What I think, with my pre wakeup brain, happens is that the CPUID topology muck still reports 2 siblings (it tends to do that).

But the BIOS only reports APIC-IDs for all your cores, so our nr_cpu_ids is reduced, while the nr_siblings count it not.

And them *boom*.

This is the same old problem that is nearly impossible to tell if HT is enabled or not -- complete and utter trainwreck :/

I need to go make wake-up-juice and ponder wth to do about this.

Peter Zijlstra

maintainer

2016-03-18 10:16 UTC #20 lore raw

Quoted 5 lines · Mike Galbraith

Quoted 3 lines · Peter Zijlstra
Andreas; Borislav said to Cc you since you wrote all this. The issue is that Linux assumes:

Quoted 7 lines

Quoted 1 line
> nr_logical_cpus = nr_cores * nr_siblings
It also seems to now assume that if SMT is possible, it's enabled.
Below is my 8 socket DL980 G7, which has SMT turned off for RT testing, booting NOPREEMPT master tuned for maximum bloat ala distro and getting confused by me telling it (as always) nr_cpus=64. Bad juju ensues.

Ah, did you actually disable HT in the BIOS, or just skip the HT enumeration by saying nr_cpus=64 (knowing that all the siblings are last)?

In any case, Thomas has a clue and I'm going to test, but 4 socket machine takes forever to boot, so might be a few minutes :/

Mike Galbraith

2016-03-18 12:39 UTC #21 lore raw

Quoted 16 lines · Peter Zijlstra

Quoted 7 lines · Mike Galbraith
Quoted 4 lines · Peter Zijlstra
Andreas; Borislav said to Cc you since you wrote all this. The issue is that Linux assumes:
> nr_logical_cpus = nr_cores * nr_siblings
Quoted 7 lines
It also seems to now assume that if SMT is possible, it's enabled.
Below is my 8 socket DL980 G7, which has SMT turned off for RT testing,
booting NOPREEMPT master tuned for maximum bloat ala distro and getting
confused by me telling it (as always) nr_cpus=64. Bad juju ensues.

Quoted 3 lines

Ah, did you actually disable HT in the BIOS, or just skip the HT enumeration by saying nr_cpus=64 (knowing that all the siblings are last)?

It's disabled in BIOS.

Quoted 2 lines

In any case, Thomas has a clue and I'm going to test, but 4 socket machine takes forever to boot, so might be a few minutes :/

His one-liner made my DL980 all better.

-Mike

Peter Zijlstra

maintainer

2016-03-18 13:32 UTC #22 lore raw

Quoted 7 lines · Mike Galbraith

Quoted 3 lines · Peter Zijlstra
Ah, did you actually disable HT in the BIOS, or just skip the HT enumeration by saying nr_cpus=64 (knowing that all the siblings are last)?
It's disabled in BIOS.

OK, so I disabled HT in the BIOS too, on my IVB-EX

Quoted 4 lines

Quoted 2 lines
In any case, Thomas has a clue and I'm going to test, but 4 socket machine takes forever to boot, so might be a few minutes :/
His one-liner made my DL980 all better.

My machine is profoundly unhappy though; and since I have no nr_cpus= nr_cpu_ids == total_cpus and his patch wouldn't do anything anyway.

[    0.286838] max_cores: 15, cpu_ids: 60, num_siblings: 2, coreid_bits: 5
[    0.293463] smpboot: Max logical packages: 2
[    0.297733] smpboot: APIC(0) Converting physical 0 to logical package 0
[    0.304346] smpboot: APIC(20) Converting physical 1 to logical package 1
[    0.311047] smpboot: APIC(40) Package 2 exceeds logical package map
[    0.317309] smpboot: CPU 30 APICId 40 disabled
[    0.321757] smpboot: APIC(60) Package 3 exceeds logical package map
[    0.328022] smpboot: CPU 45 APICId 60 disabled

This machine does exactly what I suspected yours did.

Mike Galbraith

2016-03-18 14:07 UTC #23 lore raw

Quoted 28 lines · Peter Zijlstra

Quoted 7 lines · Mike Galbraith
Quoted 3 lines · Peter Zijlstra
Ah, did you actually disable HT in the BIOS, or just skip the HT enumeration by saying nr_cpus=64 (knowing that all the siblings are last)?
It's disabled in BIOS.
OK, so I disabled HT in the BIOS too, on my IVB-EX
Quoted 4 lines
Quoted 2 lines
In any case, Thomas has a clue and I'm going to test, but 4 socket machine takes forever to boot, so might be a few minutes :/
His one-liner made my DL980 all better.
My machine is profoundly unhappy though; and since I have no nr_cpus= nr_cpu_ids == total_cpus and his patch wouldn't do anything anyway.
[    0.286838] max_cores: 15, cpu_ids: 60, num_siblings: 2, coreid_bits: 5
[    0.293463] smpboot: Max logical packages: 2
[    0.297733] smpboot: APIC(0) Converting physical 0 to logical package 0
[    0.304346] smpboot: APIC(20) Converting physical 1 to logical package 1
[    0.311047] smpboot: APIC(40) Package 2 exceeds logical package map
[    0.317309] smpboot: CPU 30 APICId 40 disabled
[    0.321757] smpboot: APIC(60) Package 3 exceeds logical package map
[    0.328022] smpboot: CPU 45 APICId 60 disabled

Quoted 1 line

This machine does exactly what I suspected yours did.

Yup, that looks very familiar. I just booted without nr_cpus=64 to make sure it's a happy camper both w/wo, and it says all is peachy.

-Mike

Thomas Gleixner

2016-03-18 11:57 UTC #24 lore raw

In reply to Mike Galbraith

Quoted 5 lines · Mike Galbraith

Quoted 3 lines · Peter Zijlstra
Andreas; Borislav said to Cc you since you wrote all this. The issue is that Linux assumes:

Quoted 7 lines

Quoted 1 line
> nr_logical_cpus = nr_cores * nr_siblings
It also seems to now assume that if SMT is possible, it's enabled.
Below is my 8 socket DL980 G7, which has SMT turned off for RT testing, booting NOPREEMPT master tuned for maximum bloat ala distro and getting confused by me telling it (as always) nr_cpus=64. Bad juju ensues.

Does the patch below fix the wreckage?

arch/x86/kernel/smpboot.c +1 -1

index 643dbdccf4bc..c5ac71276076 100644
@@ -319,7 +319,7 @@ static void __init smp_init_package_map(void)
 	 * might change in the future....
 	 */
 	ncpus = boot_cpu_data.x86_max_cores * smp_num_siblings;
-	__max_logical_packages = DIV_ROUND_UP(nr_cpu_ids, ncpus);
+	__max_logical_packages = DIV_ROUND_UP(total_cpus, ncpus);
 
 	/*
 	 * Possibly larger than what we need as the number of apic ids per

Mike Galbraith

2016-03-18 12:39 UTC #25 lore raw

Quoted 1 line · Thomas Gleixner

Does the patch below fix the wreckage?

Yup, all better.

Quoted 14 lines

arch/x86/kernel/smpboot.c +1 -1

index 643dbdccf4bc..c5ac71276076 100644
@@ -319,7 +319,7 @@ static void __init smp_init_package_map(void)
         * might change in the future....
         */
        ncpus = boot_cpu_data.x86_max_cores * smp_num_siblings;
-       __max_logical_packages = DIV_ROUND_UP(nr_cpu_ids, ncpus);
+       __max_logical_packages = DIV_ROUND_UP(total_cpus, ncpus);
 
        /*
         * Possibly larger than what we need as the number of apic

ids per

tip-bot for Thomas Gleixner

merge bot

2016-03-19 09:35 UTC #26 lore raw

[tip:x86/urgent] x86/topology: Use total_cpus not nr_cpu_ids for logical packages

In reply to Thomas Gleixner

x86/topology: Use total_cpus not nr_cpu_ids for logical packages

Commit-ID:  3e8db2246b434c6b18a6a9f09904038bddcf76c7
Gitweb:     http://git.kernel.org/tip/3e8db2246b434c6b18a6a9f09904038bddcf76c7
Author:     Thomas Gleixner <[email protected]>
AuthorDate: Fri, 18 Mar 2016 17:20:30 +0100
Committer:  Thomas Gleixner <[email protected]>
CommitDate: Sat, 19 Mar 2016 10:26:40 +0100

x86/topology: Use total_cpus not nr_cpu_ids for logical packages

nr_cpu_ids can be limited on the command line via nr_cpus=. That can break the logical package management because it results in a smaller number of packages, but the cpus to online are occupying the full package space as the hyper threads are enumerated after the physical cores typically.

total_cpus is the real possible cpu space not limited by nr_cpus command line and gives us the proper number of packages.

Reported-by Mike Galbraith <[email protected]>

Fixes 1f12e32f4cd5 ("x86/topology: Create logical package id")

Link http://lkml.kernel.org/r/alpine.DEB.2.11.1603181254330.3978@nanos open here

Cc Xiong Zhou <[email protected]>

Cc Ingo Molnar <[email protected]>

Cc Borislav Petkov <[email protected]>

Cc Andreas Herrmann <[email protected]>

---
 arch/x86/kernel/smpboot.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

arch/x86/kernel/smpboot.c +6 -1

index 81e6a43..b2c99f8 100644
@@ -325,9 +325,14 @@ static void __init smp_init_package_map(void)
 	 * By not including this we'll sometimes over-estimate the number of
 	 * logical packages by the amount of !present siblings, but this is
 	 * still better than MAX_LOCAL_APIC.
+	 *
+	 * We use total_cpus not nr_cpu_ids because nr_cpu_ids can be limited
+	 * on the command line leading to a similar issue as the HT disable
+	 * problem because the hyperthreads are usually enumerated after the
+	 * primary cores.
 	 */
 	ncpus = boot_cpu_data.x86_max_cores;
-	__max_logical_packages = DIV_ROUND_UP(nr_cpu_ids, ncpus);
+	__max_logical_packages = DIV_ROUND_UP(total_cpus, ncpus);
 
 	/*
 	 * Possibly larger than what we need as the number of apic ids per

tip-bot for Peter Zijlstra

merge bot

2016-03-29 08:49 UTC #27 lore raw

[tip:x86/urgent] x86/topology: Fix AMD core count

x86/topology: Fix AMD core count

Commit-ID:  ee6825c80e870fff1a370c718ec77022ade0889b
Gitweb:     http://git.kernel.org/tip/ee6825c80e870fff1a370c718ec77022ade0889b
Author:     Peter Zijlstra <[email protected]>
AuthorDate: Fri, 25 Mar 2016 15:52:34 +0100
Committer:  Thomas Gleixner <[email protected]>
CommitDate: Tue, 29 Mar 2016 10:45:04 +0200

x86/topology: Fix AMD core count

It turns out AMD gets x86_max_cores wrong when there are compute units.

The issue is that Linux assumes:

nr_logical_cpus = nr_cores * nr_siblings

But AMD reports its CU unit as 2 cores, but then sets num_smp_siblings to 2 as well.

Boris: fixup ras/mce_amd_inj.c too, to compute the Node Base Core properly, according to the new nomenclature.

Fixes 1f12e32f4cd5 ("x86/topology: Create logical package id")

Signed-off-by Peter Zijlstra (Intel) <[email protected]>

Signed-off-by Borislav Petkov <[email protected]>

Cc Andreas Herrmann <[email protected]>

Cc Andy Lutomirski <[email protected]>

Link http://lkml.kernel.org/r/[email protected] open here

---
 arch/x86/include/asm/smp.h | 1 +
 arch/x86/kernel/cpu/amd.c  | 8 ++++----
 arch/x86/ras/mce_amd_inj.c | 3 ++-
 3 files changed, 7 insertions(+), 5 deletions(-)

arch/x86/include/asm/smp.h +1 -0

index 20a3de5..66b0573 100644
@@ -155,6 +155,7 @@ static inline int wbinvd_on_all_cpus(void)
 	wbinvd();
 	return 0;
 }
+#define smp_num_siblings	1
 #endif /* CONFIG_SMP */
 
 extern unsigned disabled_cpus;

arch/x86/kernel/cpu/amd.c +4 -4

index 6e47e3a..4d0087f 100644
@@ -312,9 +312,9 @@ static void amd_get_topology(struct cpuinfo_x86 *c)
 		node_id = ecx & 7;
 
 		/* get compute unit information */
-		smp_num_siblings = ((ebx >> 8) & 3) + 1;
+		cores_per_cu = smp_num_siblings = ((ebx >> 8) & 3) + 1;
+		c->x86_max_cores /= smp_num_siblings;
 		c->compute_unit_id = ebx & 0xff;
-		cores_per_cu += ((ebx >> 8) & 3);
 	} else if (cpu_has(c, X86_FEATURE_NODEID_MSR)) {
 		u64 value;

@@ -329,8 +329,8 @@ static void amd_get_topology(struct cpuinfo_x86 *c)
 		u32 cus_per_node;
 
 		set_cpu_cap(c, X86_FEATURE_AMD_DCM);
-		cores_per_node = c->x86_max_cores / nodes_per_socket;
-		cus_per_node = cores_per_node / cores_per_cu;
+		cus_per_node = c->x86_max_cores / nodes_per_socket;
+		cores_per_node = cus_per_node * cores_per_cu;
 
 		/* store NodeID, use llc_shared_map to store sibling info */
 		per_cpu(cpu_llc_id, cpu) = node_id;

arch/x86/ras/mce_amd_inj.c +2 -1

index 55d38cf..9e02dca 100644
@@ -20,6 +20,7 @@
 #include <linux/pci.h>
 
 #include <asm/mce.h>
+#include <asm/smp.h>
 #include <asm/amd_nb.h>
 #include <asm/irq_vectors.h>

@@ -206,7 +207,7 @@ static u32 get_nbc_for_node(int node_id)
 	struct cpuinfo_x86 *c = &boot_cpu_data;
 	u32 cores_per_node;
 
-	cores_per_node = c->x86_max_cores / amd_get_nodes_per_socket();
+	cores_per_node = (c->x86_max_cores * smp_num_siblings) / amd_get_nodes_per_socket();
 
 	return cores_per_node * node_id;
 }

Peter Zijlstra

maintainer

2016-03-18 15:07 UTC #28 lore raw

cover x86 topology fixes

Some collected patches addressing the logical package fallout

While, strictly speaking, the 3rd patches makes the 2nd redundant, I think it still makes sense to keep things consistent etc..

Peter Zijlstra

maintainer

2016-03-18 15:08 UTC #29 lore raw

PATCH 1/3 x86/topology: Fix logical pkg mapping

That first branch testing pkg against __max_logical_packages is wrong, because if the first pkg id is larger, then the find_first_zero will find us logical package id 0. However, if the second pkg id is indeed 0, we'll again claim it without testing if it was already taken.

Also, it fails to print the mapping.

Cc Ingo Molnar <[email protected]>

Cc Borislav Petkov <[email protected]>

Cc Thomas Gleixner <[email protected]>

Cc Andreas Herrmann <[email protected]>

Fixes 1f12e32f4cd5 ("x86/topology: Create logical package id")

Signed-off-by Peter Zijlstra (Intel) <[email protected]>

Link http://lkml.kernel.org/r/[email protected] open here

---
 arch/x86/kernel/cpu/amd.c |    8 ++++----
 arch/x86/kernel/smpboot.c |   11 ++++++-----
 2 files changed, 10 insertions(+), 9 deletions(-)

arch/x86/kernel/smpboot.c +0 -5

@@ -274,11 +274,6 @@ int topology_update_package_map(unsigned
 	if (test_and_set_bit(pkg, physical_package_map))
 		goto found;
 
-	if (pkg < __max_logical_packages) {
-		set_bit(pkg, logical_package_map);
-		physical_to_logical_pkg[pkg] = pkg;
-		goto found;
-	}
 	new = find_first_zero_bit(logical_package_map, __max_logical_packages);
 	if (new >= __max_logical_packages) {
 		physical_to_logical_pkg[pkg] = -1;

tip-bot for Peter Zijlstra

merge bot

2016-03-19 09:33 UTC #30 lore raw

[tip:x86/urgent] x86/topology: Fix logical package mapping

x86/topology: Fix logical package mapping

Commit-ID:  b5d5f27d938fb6fc8d3202704e699d2694a02da6
Gitweb:     http://git.kernel.org/tip/b5d5f27d938fb6fc8d3202704e699d2694a02da6
Author:     Peter Zijlstra <[email protected]>
AuthorDate: Fri, 18 Mar 2016 16:03:46 +0100
Committer:  Thomas Gleixner <[email protected]>
CommitDate: Sat, 19 Mar 2016 10:26:40 +0100

x86/topology: Fix logical package mapping

Also, it fails to print the mapping.

Fixes 1f12e32f4cd5 ("x86/topology: Create logical package id")

Signed-off-by Peter Zijlstra (Intel) <[email protected]>

Cc Mike Galbraith <[email protected]>

Link http://lkml.kernel.org/r/[email protected] open here

---
 arch/x86/kernel/smpboot.c | 5 -----
 1 file changed, 5 deletions(-)

arch/x86/kernel/smpboot.c +0 -5

index 643dbdc..64b669d 100644
@@ -274,11 +274,6 @@ int topology_update_package_map(unsigned int apicid, unsigned int cpu)
 	if (test_and_set_bit(pkg, physical_package_map))
 		goto found;
 
-	if (pkg < __max_logical_packages) {
-		set_bit(pkg, logical_package_map);
-		physical_to_logical_pkg[pkg] = pkg;
-		goto found;
-	}
 	new = find_first_zero_bit(logical_package_map, __max_logical_packages);
 	if (new >= __max_logical_packages) {
 		physical_to_logical_pkg[pkg] = -1;

Peter Zijlstra

maintainer

2016-03-18 15:07 UTC #31 lore raw

PATCH 2/3 x86/topology: Fix AMD core count

It turns out AMD gets x86_max_cores wrong when there are compute units.

The issue is that Linux assumes:

nr_logical_cpus = nr_cores * nr_siblings

But AMD reports its CU unit as 2 cores, but then sets num_smp_siblings to 2 as well.

Cc Ingo Molnar <[email protected]>

Cc Borislav Petkov <[email protected]>

Cc Thomas Gleixner <[email protected]>

Cc Andreas Herrmann <[email protected]>

Fixes 1f12e32f4cd5 ("x86/topology: Create logical package id")

Signed-off-by Peter Zijlstra (Intel) <[email protected]>

Link http://lkml.kernel.org/r/[email protected] open here

---
 arch/x86/kernel/cpu/amd.c |    8 ++++----
 arch/x86/kernel/smpboot.c |   11 ++++++-----
 2 files changed, 10 insertions(+), 9 deletions(-)

arch/x86/kernel/cpu/amd.c +4 -4

@@ -313,9 +313,9 @@ static void amd_get_topology(struct cpui
 		node_id = ecx & 7;
 
 		/* get compute unit information */
-		smp_num_siblings = ((ebx >> 8) & 3) + 1;
+		cores_per_cu = smp_num_siblings = ((ebx >> 8) & 3) + 1;
+		c->x86_max_cores /= smp_num_siblings;
 		c->compute_unit_id = ebx & 0xff;
-		cores_per_cu += ((ebx >> 8) & 3);
 	} else if (cpu_has(c, X86_FEATURE_NODEID_MSR)) {
 		u64 value;

@@ -331,8 +331,8 @@ static void amd_get_topology(struct cpui
 		u32 cus_per_node;
 
 		set_cpu_cap(c, X86_FEATURE_AMD_DCM);
-		cores_per_node = c->x86_max_cores / nodes_per_socket;
-		cus_per_node = cores_per_node / cores_per_cu;
+		cus_per_node = c->x86_max_cores / nodes_per_socket;
+		cores_per_node = cus_per_node * cores_per_cu;
 
 		/* store NodeID, use llc_shared_map to store sibling info */
 		per_cpu(cpu_llc_id, cpu) = node_id;

Borislav Petkov

maintainer

2016-03-18 16:41 UTC #32 lore raw

Re: [PATCH 2/3] x86/topology: Fix AMD core count

Quoted 38 lines · Peter Zijlstra

It turns out AMD gets x86_max_cores wrong when there are compute units.
The issue is that Linux assumes:
nr_logical_cpus = nr_cores * nr_siblings
But AMD reports its CU unit as 2 cores, but then sets num_smp_siblings to 2 as well.
Cc Ingo Molnar <[email protected]>
Cc Borislav Petkov <[email protected]>
Cc Thomas Gleixner <[email protected]>
Cc Andreas Herrmann <[email protected]>
Reported-by Xiong Zhou <[email protected]>
Fixes 1f12e32f4cd5 ("x86/topology: Create logical package id")
Signed-off-by Peter Zijlstra (Intel) <[email protected]>
Link http://lkml.kernel.org/r/[email protected] open here
---
 arch/x86/kernel/cpu/amd.c |    8 ++++----
 arch/x86/kernel/smpboot.c |   11 ++++++-----
 2 files changed, 10 insertions(+), 9 deletions(-)
arch/x86/kernel/cpu/amd.c +2 -2
@@ -313,9 +313,9 @@ static void amd_get_topology(struct cpui
 		node_id = ecx & 7;
 
 		/* get compute unit information */
-		smp_num_siblings = ((ebx >> 8) & 3) + 1;
+		cores_per_cu = smp_num_siblings = ((ebx >> 8) & 3) + 1;
+		c->x86_max_cores /= smp_num_siblings;
 		c->compute_unit_id = ebx & 0xff;
-		cores_per_cu += ((ebx >> 8) & 3);
 	} else if (cpu_has(c, X86_FEATURE_NODEID_MSR)) {
 		u64 value;
 

arch/x86/kernel/cpu/amd.c static void amd_get_topology(struct cpui · line 338

Quoted 10 lines

arch/x86/kernel/cpu/amd.c +2 -2

@@ -331,8 +331,8 @@ static void amd_get_topology(struct cpui
 		u32 cus_per_node;
 
 		set_cpu_cap(c, X86_FEATURE_AMD_DCM);
-		cores_per_node = c->x86_max_cores / nodes_per_socket;
-		cus_per_node = cores_per_node / cores_per_cu;
+		cus_per_node = c->x86_max_cores / nodes_per_socket;
+		cores_per_node = cus_per_node * cores_per_cu;
 
 		/* store NodeID, use llc_shared_map to store sibling info */
 		per_cpu(cpu_llc_id, cpu) = node_id;

Looks ok to me, however it probably would be prudent if AMD tested it on a bunch of machines just to make sure we don't break anything else. I'm thinking F15h and F16h, something big...

Rui, can you find some time to run this one please?

Look at before/after info in /proc/cpuinfo, topology in sysfs and dmesg before and after might be useful too.

Thanks.

Signature · 4 lines

Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.

Huang Rui

2016-03-21 03:07 UTC #33 lore raw

Re: [PATCH 2/3] x86/topology: Fix AMD core count

Quoted 54 lines · Borislav Petkov

Quoted 38 lines · Peter Zijlstra
It turns out AMD gets x86_max_cores wrong when there are compute units.
The issue is that Linux assumes:
nr_logical_cpus = nr_cores * nr_siblings
But AMD reports its CU unit as 2 cores, but then sets num_smp_siblings to 2 as well.
Cc Ingo Molnar <[email protected]>
Cc Borislav Petkov <[email protected]>
Cc Thomas Gleixner <[email protected]>
Cc Andreas Herrmann <[email protected]>
Reported-by Xiong Zhou <[email protected]>
Fixes 1f12e32f4cd5 ("x86/topology: Create logical package id")
Signed-off-by Peter Zijlstra (Intel) <[email protected]>
Link http://lkml.kernel.org/r/[email protected] open here
---
 arch/x86/kernel/cpu/amd.c |    8 ++++----
 arch/x86/kernel/smpboot.c |   11 ++++++-----
 2 files changed, 10 insertions(+), 9 deletions(-)
arch/x86/kernel/cpu/amd.c +2 -2
@@ -313,9 +313,9 @@ static void amd_get_topology(struct cpui
 		node_id = ecx & 7;
 
 		/* get compute unit information */
-		smp_num_siblings = ((ebx >> 8) & 3) + 1;
+		cores_per_cu = smp_num_siblings = ((ebx >> 8) & 3) + 1;
+		c->x86_max_cores /= smp_num_siblings;
 		c->compute_unit_id = ebx & 0xff;
-		cores_per_cu += ((ebx >> 8) & 3);
 	} else if (cpu_has(c, X86_FEATURE_NODEID_MSR)) {
 		u64 value;
 
arch/x86/kernel/cpu/amd.c static void amd_get_topology(struct cpui · line 338
Quoted 10 lines
arch/x86/kernel/cpu/amd.c +2 -2
@@ -331,8 +331,8 @@ static void amd_get_topology(struct cpui
 		u32 cus_per_node;
 
 		set_cpu_cap(c, X86_FEATURE_AMD_DCM);
-		cores_per_node = c->x86_max_cores / nodes_per_socket;
-		cus_per_node = cores_per_node / cores_per_cu;
+		cus_per_node = c->x86_max_cores / nodes_per_socket;
+		cores_per_node = cus_per_node * cores_per_cu;
 
 		/* store NodeID, use llc_shared_map to store sibling info */
 		per_cpu(cpu_llc_id, cpu) = node_id;
Looks ok to me, however it probably would be prudent if AMD tested it on a bunch of machines just to make sure we don't break anything else. I'm thinking F15h and F16h, something big...

Quoted 5 lines

Rui, can you find some time to run this one please?
Look at before/after info in /proc/cpuinfo, topology in sysfs and dmesg before and after might be useful too.

OK, we will find some fam15h, fam16h platforms to verify it. Please wait for my feedback.

But I am confused with c->x86_max_cores /= smp_num_siblings, what is the real meaning of c->x86_max_cores here for AMD, the whole compute unit numbers per socket?

+ Sherry, for her awareness.

Thanks,
Rui

Huang Rui

2016-03-21 03:46 UTC #34 lore raw

Re: [PATCH 2/3] x86/topology: Fix AMD core count

Quoted 61 lines · Huang Rui

Quoted 54 lines · Borislav Petkov
Quoted 38 lines · Peter Zijlstra
It turns out AMD gets x86_max_cores wrong when there are compute units.
The issue is that Linux assumes:
nr_logical_cpus = nr_cores * nr_siblings
But AMD reports its CU unit as 2 cores, but then sets num_smp_siblings to 2 as well.
Cc Ingo Molnar <[email protected]>
Cc Borislav Petkov <[email protected]>
Cc Thomas Gleixner <[email protected]>
Cc Andreas Herrmann <[email protected]>
Reported-by Xiong Zhou <[email protected]>
Fixes 1f12e32f4cd5 ("x86/topology: Create logical package id")
Signed-off-by Peter Zijlstra (Intel) <[email protected]>
Link http://lkml.kernel.org/r/[email protected] open here
---
 arch/x86/kernel/cpu/amd.c |    8 ++++----
 arch/x86/kernel/smpboot.c |   11 ++++++-----
 2 files changed, 10 insertions(+), 9 deletions(-)
arch/x86/kernel/cpu/amd.c +2 -2
@@ -313,9 +313,9 @@ static void amd_get_topology(struct cpui
 		node_id = ecx & 7;
 
 		/* get compute unit information */
-		smp_num_siblings = ((ebx >> 8) & 3) + 1;
+		cores_per_cu = smp_num_siblings = ((ebx >> 8) & 3) + 1;
+		c->x86_max_cores /= smp_num_siblings;
 		c->compute_unit_id = ebx & 0xff;
-		cores_per_cu += ((ebx >> 8) & 3);
 	} else if (cpu_has(c, X86_FEATURE_NODEID_MSR)) {
 		u64 value;
 
arch/x86/kernel/cpu/amd.c static void amd_get_topology(struct cpui · line 338
Quoted 10 lines
arch/x86/kernel/cpu/amd.c +2 -2
@@ -331,8 +331,8 @@ static void amd_get_topology(struct cpui
 		u32 cus_per_node;
 
 		set_cpu_cap(c, X86_FEATURE_AMD_DCM);
-		cores_per_node = c->x86_max_cores / nodes_per_socket;
-		cus_per_node = cores_per_node / cores_per_cu;
+		cus_per_node = c->x86_max_cores / nodes_per_socket;
+		cores_per_node = cus_per_node * cores_per_cu;
 
 		/* store NodeID, use llc_shared_map to store sibling info */
 		per_cpu(cpu_llc_id, cpu) = node_id;
Looks ok to me, however it probably would be prudent if AMD tested it on a bunch of machines just to make sure we don't break anything else. I'm thinking F15h and F16h, something big...
Quoted 5 lines
Rui, can you find some time to run this one please?
Look at before/after info in /proc/cpuinfo, topology in sysfs and dmesg before and after might be useful too.

Quoted 9 lines

OK, we will find some fam15h, fam16h platforms to verify it. Please wait for my feedback.
But I am confused with c->x86_max_cores /= smp_num_siblings, what is the real meaning of c->x86_max_cores here for AMD, the whole compute unit numbers per socket?
+ Sherry, for her awareness.

I quickly applied this patch on tip/master with on a fam15h machine. The issue is still existed, only one core can be detected.

processor : 0
vendor_id : AuthenticAMD
cpu family : 21
model : 2
model name : AMD Opteron(tm) Processor 6386 SE
stepping : 0
microcode : 0x6000822
cpu MHz : 2792.882
cache size : 2048 KB
fpu : yes
fpu_exception : yes
cpuid level : 13
wp : yes
flags : fpu vme de pse tsc msr pae mce cx8 apic sep mtrr pge mca cmov pat pse36 clflush mmx fxsr sse sse2 ht syscall nx mmxext fxsr_opt pdpe1gb rdtscp lm constant_tsc rep_good nopl nonstop_tsc extd_apicid aperfmperf eagerfpu pni pclmulqdq monitor ssse3 fma cx16 sse4_1 sse4_2 popcnt aes xsave avx f16c lahf_lm cmp_legacy svm extapic cr8_legacy abm sse4a misalignsse 3dnowprefetch osvw ibs xop skinit wdt lwp fma4 tce nodeid_msr tbm topoext perfctr_core perfctr_nb cpb hw_pstate vmmcall bmi1 arat npt lbrv svm_lock nrip_save tsc_scale vmcb_clean flushbyasid decodeassists pausefilter pfthreshold
bugs : fxsave_leak sysret_ss_attrs
bogomips : 5585.76
TLB size : 1536 4K pages
clflush size : 64
cache_alignment : 64
address sizes : 48 bits physical, 48 bits virtual
power management: ts ttp tm 100mhzsteps hwpstate cpb eff_freq_ro

Architecture:          x86_64
CPU op-mode(s):        32-bit, 64-bit
Byte Order:            Little Endian
CPU(s):                1
On-line CPU(s) list:   0
Thread(s) per core:    1
Core(s) per socket:    1
Socket(s):             1
Vendor ID:             AuthenticAMD
CPU family:            21
Model:                 2
Stepping:              0
CPU MHz:               2792.882
BogoMIPS:              5585.76
Virtualization:        AMD-V
L1d cache:             16K
L1i cache:             64K
L2 cache:              2048K
L3 cache:              6144K

Thanks,
Rui

Borislav Petkov

maintainer

2016-03-21 08:26 UTC #35 lore raw

Re: [PATCH 2/3] x86/topology: Fix AMD core count

Quoted 2 lines · Huang Rui

I quickly applied this patch on tip/master with on a fam15h machine. The issue is still existed, only one core can be detected.

Huh, what?

So that 6386 has 16 cores, according to wikipedia, that must be 8 compute units. Correct?

Are you saying, you have only one core in /proc/cpuinfo?

Can you send full dmesg please?

Signature · 4 lines

Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.

Huang Rui

2016-03-21 09:18 UTC #36 lore raw

Re: [PATCH 2/3] x86/topology: Fix AMD core count

Quoted 9 lines · Borislav Petkov

Quoted 2 lines · Huang Rui
I quickly applied this patch on tip/master with on a fam15h machine. The issue is still existed, only one core can be detected.
Huh, what?
So that 6386 has 16 cores, according to wikipedia, that must be 8 compute units. Correct?

Yes, there are two sockets in this platform. 32 cores and 16 compute units.

Quoted 4 lines

Are you saying, you have only one core in /proc/cpuinfo?
Can you send full dmesg please?

Apology to bring a surprise. Our test machine didn't configure the CONFIG_SMP just now because it was to reproduce another build issue before...
So an incorrect "only one" core is detected, sorry. Please ignore previous result. I will do more testing on other platforms.

Now I re-build system, please see the correct message:

ray@hr-ub:~/tip$ cat /sys/devices/system/cpu/cpu2/topology/thread_siblings_list
2-3
ray@hr-ub:~/tip$ cat /sys/devices/system/cpu/cpu2/topology/core_siblings_list
0-15

processor : 31
vendor_id : AuthenticAMD
cpu family : 21
model : 2
model name : AMD Opteron(tm) Processor 6386 SE
stepping : 0
microcode : 0x6000822
cpu MHz : 1400.000
cache size : 2048 KB
physical id : 1
siblings : 16
core id : 7
cpu cores : 8
apicid : 79
initial apicid : 47
fpu : yes
fpu_exception : yes
cpuid level : 13
wp : yes
flags : fpu vme de pse tsc msr pae mce cx8 apic sep mtrr pge
mca cmov pat pse36 clflush mmx fxsr sse sse2 ht syscall nx mmxext fxsr_opt pdpe1gb rdtscp lm constant_tsc rep_good nopl nonstop_tsc extd_apicid amd_dcm aperfmperf eagerfpu pni pclmulqdq monitor ssse3 fma cx16 sse4_1 sse4_2 popcnt aes xsave avx f16c lahf_lm cmp_legacy svm extapic cr8_legacy abm sse4a misalignsse 3dnowprefetch osvw ibs xop skinit wdt lwp fma4 tce nodeid_msr tbm topoext perfctr_core perfctr_nb cpb hw_pstate vmmcall bmi1 arat npt lbrv svm_lock nrip_save tsc_scale vmcb_clean flushbyasid decodeassists pausefilter pfthreshold
bugs : fxsave_leak sysret_ss_attrs
bogomips : 5585.90
TLB size : 1536 4K pages
clflush size : 64
cache_alignment : 64
address sizes : 48 bits physical, 48 bits virtual
power management: ts ttp tm 100mhzsteps hwpstate cpb eff_freq_ro

Architecture:          x86_64
CPU op-mode(s):        32-bit, 64-bit
Byte Order:            Little Endian
CPU(s):                32
On-line CPU(s) list:   0-31
Thread(s) per core:    2
Core(s) per socket:    8
Socket(s):             2
NUMA node(s):          4
Vendor ID:             AuthenticAMD
CPU family:            21
Model:                 2
Stepping:              0
CPU MHz:               1400.000
BogoMIPS:              5585.90
Virtualization:        AMD-V
L1d cache:             16K
L1i cache:             64K
L2 cache:              2048K
L3 cache:              6144K
NUMA node0 CPU(s):     0-7
NUMA node1 CPU(s):     8-15
NUMA node2 CPU(s):     16-23
NUMA node3 CPU(s):     24-31

Thanks,
Rui

Thomas Gleixner

2016-03-21 08:58 UTC #37 lore raw

Re: [PATCH 2/3] x86/topology: Fix AMD core count

In reply to Huang Rui

Quoted 1 line · Huang Rui

I quickly applied this patch on tip/master with on a fam15h machine.

Can you use tip/x86/urgent please?

Thanks,

tglx

Peter Zijlstra

maintainer

2016-03-21 08:23 UTC #38 lore raw

Re: [PATCH 2/3] x86/topology: Fix AMD core count

In reply to Huang Rui

Quoted 10 lines · Huang Rui

Quoted 6 lines
Quoted 6 lines
The issue is that Linux assumes:
nr_logical_cpus = nr_cores * nr_siblings
But AMD reports its CU unit as 2 cores, but then sets num_smp_siblings to 2 as well.
But I am confused with c->x86_max_cores /= smp_num_siblings, what is the real meaning of c->x86_max_cores here for AMD, the whole compute unit numbers per socket?

Yes, with the whole Compute Unit being the Core, each logical CPU becomes a Thread. This is the direct consequence of using the SMT topology to model the CU thing.

Huang Rui

2016-03-21 09:46 UTC #39 lore raw

Re: [PATCH 2/3] x86/topology: Fix AMD core count

Quoted 8 lines · Peter Zijlstra

Quoted 6 lines · Huang Rui
Quoted 6 lines
Quoted 6 lines
The issue is that Linux assumes:
nr_logical_cpus = nr_cores * nr_siblings
But AMD reports its CU unit as 2 cores, but then sets num_smp_siblings to 2 as well.

Quoted 8 lines

Quoted 3 lines
But I am confused with c->x86_max_cores /= smp_num_siblings, what is the real meaning of c->x86_max_cores here for AMD, the whole compute unit numbers per socket?
Yes, with the whole Compute Unit being the Core, each logical CPU becomes a Thread. This is the direct consequence of using the SMT topology to model the CU thing.

OK, maybe, we would better add a comment to explain here. :-)

Thanks,
Rui

Borislav Petkov

maintainer

2016-03-21 13:57 UTC #40 lore raw

Re: [PATCH 2/3] x86/topology: Fix AMD core count

Quoted 1 line · Huang Rui

OK, maybe, we would better add a comment to explain here. :-)

Here's a start:

---
From: Borislav Petkov <[email protected]>
Date: Mon, 21 Mar 2016 14:53:05 +0100
Subject: [PATCH] x86/Documentation: Start documenting x86 topology

This should contain important aspects of how we represent the system topology on x86. If people have questions about it and this file doesn't answer it, then it must be updated.

Signed-off-by Borislav Petkov <[email protected]>

---
 Documentation/x86/topology.txt | 34 ++++++++++++++++++++++++++++++++++
 1 file changed, 34 insertions(+)
 create mode 100644 Documentation/x86/topology.txt

Documentation/x86/topology.txt new file +34 -0

new file mode 100644
index 000000000000..828e953869d4
@@ -0,0 +1,34 @@
+x86 Topology
+============
+
+This documents and clarifies the main aspects of x86 topology modelling
+and representation in the kernel. Update/change when doing changes to
+the respective code.
+
+Started by Borislav Petkov <[email protected]>.
+
+The main aim of the topology facilities is to present adequate
+interfaces to code which needs to know/query/use the structure of the
+running system wrt threads, cores, nodes, etc.
+
+* cpuinfo_x86.x86_max_cores: the number of cores on a node, as reported
+by CPUID. Now, in order to accomodate both Intel and AMD, this variable
+means the following:
+
+- Intel: the number of cores in a processor. A core can have 1, 2 or
+more logical threads (hyperthreaded).
+
+- AMD: the number of cores in a processor. On a system where cores are
+clustered in groups of 2, 4 or more in compute units, this variable
+denotes the number of *compute units* on the node.
+
+In both cases, the number of scheduling threads is computed by doing:
+
+	x86_max_cores * smp_num_siblings
+
+This means:
+
+* smp_num_siblings: the number of siblings in a core. On AMD with
+compute units, this number is the number of compute unit siblings,
+i.e., compute unit cores in a single compute unit, according to their
+nomenclature.

Signature · 7 lines

2.7.3

-- 
Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.

Sherry Hurwitz

2016-03-22 08:11 UTC #41 lore raw

Re: [PATCH 2/3] x86/topology: Fix AMD core count

Quoted 5 lines · Borislav Petkov

+- AMD: the number of cores in a processor. On a system where cores are
Documentation/x86/topology.txt new file +4 -0
+clustered in groups of 2, 4 or more in compute units, this variable
+denotes the number of*compute units*  on the node.
+
+In both cases, the number of scheduling threads is computed by doing:

Documentation/x86/topology.txt

Quoted 10 lines

Documentation/x86/topology.txt new file +9 -1

+
+	x86_max_cores * smp_num_siblings
+
+This means:
+
+* smp_num_siblings: the number of siblings in a core. On AMD with
+compute units, this number is the number of compute unit siblings,
+i.e., compute unit cores in a single compute unit, according to their
+nomenclature.
-- hr

Boris, this documentation will help tremendously. In just this line of code:

nr_local_cpus = nr_cores * nr_siblings

we have an Intel HT = AMD core = logical_cpu
and core = AMD compute unit.

Anybody surprised there was a bug?

The surprising thing is that running lscpu on a 32 core 2 socket 6300 Opteron system without the patches I get the same output as with the patches and it matches Ray's. I don't see an impact.

Borislav Petkov

maintainer

2016-03-22 11:22 UTC #42 lore raw

Re: [PATCH 2/3] x86/topology: Fix AMD core count

Hi Sherry,

Quoted 1 line · Sherry Hurwitz

Boris, this documentation will help tremendously.

Yeah, I'm collecting more stuff for it. If you feel like something else should be explained there, holler.

Quoted 8 lines

In just this line of
code:
nr_local_cpus = nr_cores * nr_siblings
we have an Intel HT = AMD core = logical_cpu
and core = AMD compute unit.

Quoted 6 lines

Anybody surprised there was a bug?
The surprising thing is that running lscpu on a 32 core 2 socket 6300 Opteron system without
the patches I get the same output as with the patches and it matches Ray's. I don't see an impact.

Yeah, so the logic is closer to an Intel core which can have 1 or more threads and we did set x86_max_cores to the number of Bulldozer cores (each of them in a pair forming a Compute Unit). And that's fine but then we did set smp_num_siblings to 2.

In any case, I'm not seeing any issues here too but we wanted for you guys to run those changes too before we apply them.

In any case, they will be in 4.6 and you could test tip/master from time to time and scream if there's an issue.

Thanks!

Signature · 4 lines

Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.

Borislav Petkov

maintainer

2016-03-21 08:23 UTC #43 lore raw

Re: [PATCH 2/3] x86/topology: Fix AMD core count

In reply to Huang Rui

Quoted 6 lines · Huang Rui

OK, we will find some fam15h, fam16h platforms to verify it. Please wait for my feedback.
But I am confused with c->x86_max_cores /= smp_num_siblings, what is the real meaning of c->x86_max_cores here for AMD, the whole compute unit numbers per socket?

Yes, it is the cores and each core can contain two or more logical threads. In AMD speak, that's the compute unit count. We read it in detect_ht() from CPUID(1).EBX[23:16] which is LogicalProcessorCount, i.e., CPUID(8000_0008).ECX[NC] + 1, i.e., the number of cores. And "cores" in BKDG speak is the number of all cores in a processor which are distributed across compute units....

That's why we divide by the number of siblings, i.e., the number of cores in a CU, in AMD speak.

I know, it is confusing but once we're fine with the nomenclature, it'll become as clear as day. :-)

Signature · 4 lines

Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.

Huang Rui

2016-03-21 10:05 UTC #44 lore raw

Re: [PATCH 2/3] x86/topology: Fix AMD core count

Quoted 15 lines · Borislav Petkov

Quoted 6 lines · Huang Rui
OK, we will find some fam15h, fam16h platforms to verify it. Please wait for my feedback.
But I am confused with c->x86_max_cores /= smp_num_siblings, what is the real meaning of c->x86_max_cores here for AMD, the whole compute unit numbers per socket?
Yes, it is the cores and each core can contain two or more logical threads. In AMD speak, that's the compute unit count. We read it in detect_ht() from CPUID(1).EBX[23:16] which is LogicalProcessorCount, i.e., CPUID(8000_0008).ECX[NC] + 1, i.e., the number of cores. And "cores" in BKDG speak is the number of all cores in a processor which are distributed across compute units....

Quoted 6 lines

That's why we divide by the number of siblings, i.e., the number of cores in a CU, in AMD speak.
I know, it is confusing but once we're fine with the nomenclature, it'll become as clear as day. :-)

OK, actually, there was a topology bug on Carrzio before, the thread number was detected as 1:

Before:

autotest@autotest-Gardenia88:~$ lscpu
Architecture:          x86_64
CPU op-mode(s):        32-bit, 64-bit
Byte Order:            Little Endian
CPU(s):                4
On-line CPU(s) list:   0-3
Thread(s) per core:    1
Core(s) per socket:    4
Socket(s):             1
NUMA node(s):          1
Vendor ID:             AuthenticAMD
CPU family:            21
Model:                 96
Model name:            AMD Eng Sample: ZM1810C1Y4381_34/18/12/06_9874
Stepping:              0
CPU MHz:               1400.000
CPU max MHz:           1800.0000
CPU min MHz:           1400.0000
BogoMIPS:              3592.19
Virtualization:        AMD-V
L1d cache:             32K
L1i cache:             64K
L2 cache:              1024K
NUMA node0 CPU(s):     0-3

Now:

ray@hr-ub:~/tip$ lscpu
Architecture:          x86_64
CPU op-mode(s):        32-bit, 64-bit
Byte Order:            Little Endian
CPU(s):                4
On-line CPU(s) list:   0-3
Thread(s) per core:    2
Core(s) per socket:    2
Socket(s):             1
NUMA node(s):          1
Vendor ID:             AuthenticAMD
CPU family:            21
Model:                 96
Stepping:              0
CPU MHz:               1400.000
BogoMIPS:              3592.19
Virtualization:        AMD-V
L1d cache:             32K
L1i cache:             96K
L2 cache:              1024K
NUMA node0 CPU(s):     0-3

Looks better. I will test it on fam16h machine tomorrow, if it's OK, will add my Test-by.

Thanks,
Rui

Borislav Petkov

maintainer

2016-03-21 10:23 UTC #45 lore raw

Re: [PATCH 2/3] x86/topology: Fix AMD core count

Quoted 2 lines · Huang Rui

Looks better. I will test it on fam16h machine tomorrow, if it's OK, will add my Test-by.

Sure.

Thanks!

Signature · 4 lines

Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.

Thomas Gleixner

2016-03-19 09:26 UTC #46 lore raw

Re: [PATCH 2/3] x86/topology: Fix AMD core count

In reply to Thomas Gleixner

Quoted 26 lines · Peter Zijlstra

It turns out AMD gets x86_max_cores wrong when there are compute units.
The issue is that Linux assumes:
nr_logical_cpus = nr_cores * nr_siblings
But AMD reports its CU unit as 2 cores, but then sets num_smp_siblings to 2 as well.
Cc Ingo Molnar <[email protected]>
Cc Borislav Petkov <[email protected]>
Cc Thomas Gleixner <[email protected]>
Cc Andreas Herrmann <[email protected]>
Reported-by Xiong Zhou <[email protected]>
Fixes 1f12e32f4cd5 ("x86/topology: Create logical package id")
Signed-off-by Peter Zijlstra (Intel) <[email protected]>
Link http://lkml.kernel.org/r/[email protected] open here
---
 arch/x86/kernel/cpu/amd.c |    8 ++++----
 arch/x86/kernel/smpboot.c |   11 ++++++-----
 2 files changed, 10 insertions(+), 9 deletions(-)

arch/x86/kernel/cpu/amd.c static void amd_get_topology(struct cpui · line 317

Quoted 6 lines

arch/x86/kernel/cpu/amd.c +2 -1

@@ -313,9 +313,9 @@ static void amd_get_topology(struct cpui
 		node_id = ecx & 7;
 
 		/* get compute unit information */
-		smp_num_siblings = ((ebx >> 8) & 3) + 1;
+		cores_per_cu = smp_num_siblings = ((ebx >> 8) & 3) + 1;
+		c->x86_max_cores /= smp_num_siblings;

Unfortunately that will break stuff in event/amd/core.c, ras/mce_amd_inj.c which rely on the AMD interpretation of c->x86_max_cores.

I'm seriously grumpy about pointless inconsistent representations depending on the CPU vendor. That's just lazy and sloppy hackery from the "works for me" departement.

For now we'll get away with Peters workaround for the Intel HT mess, which is a different kind of mental insanity invented by HW/BIOS people, but this topology stuff needs to be made consistent ASAP.

Thanks,

tglx

Borislav Petkov

maintainer

2016-03-19 15:57 UTC #47 lore raw

Re: [PATCH 2/3] x86/topology: Fix AMD core count

Quoted 29 lines · Thomas Gleixner

Quoted 28 lines · Peter Zijlstra
It turns out AMD gets x86_max_cores wrong when there are compute units.
The issue is that Linux assumes:
nr_logical_cpus = nr_cores * nr_siblings
But AMD reports its CU unit as 2 cores, but then sets num_smp_siblings to 2 as well.
Cc Ingo Molnar <[email protected]>
Cc Borislav Petkov <[email protected]>
Cc Thomas Gleixner <[email protected]>
Cc Andreas Herrmann <[email protected]>
Reported-by Xiong Zhou <[email protected]>
Fixes 1f12e32f4cd5 ("x86/topology: Create logical package id")
Signed-off-by Peter Zijlstra (Intel) <[email protected]>
Link http://lkml.kernel.org/r/[email protected] open here
---
 arch/x86/kernel/cpu/amd.c |    8 ++++----
 arch/x86/kernel/smpboot.c |   11 ++++++-----
 2 files changed, 10 insertions(+), 9 deletions(-)
arch/x86/kernel/cpu/amd.c +0 -0
@@ -313,9 +313,9 @@ static void amd_get_topology(struct cpui
 		node_id = ecx & 7;
 

Quoted 7 lines

arch/x86/kernel/cpu/amd.c static void amd_get_topology(struct cpui · line 317
Quoted 4 lines
arch/x86/kernel/cpu/amd.c +2 -1
 		/* get compute unit information */
-		smp_num_siblings = ((ebx >> 8) & 3) + 1;
+		cores_per_cu = smp_num_siblings = ((ebx >> 8) & 3) + 1;
+		c->x86_max_cores /= smp_num_siblings;
Unfortunately that will break stuff in event/amd/core.c, ras/mce_amd_inj.c which rely on the AMD interpretation of c->x86_max_cores.

What is the events/amd/core.c check even mean? We alloc an amd_nb per core but not per thread?

The mce_amd_inj.c thing we could fix probably like this:

---

arch/x86/ras/mce_amd_inj.c +1 -6

index 55d38cfa46c2..b5a917d943c8 100644
@@ -203,12 +203,7 @@ static void trigger_thr_int(void *info)
 
 static u32 get_nbc_for_node(int node_id)
 {
-	struct cpuinfo_x86 *c = &boot_cpu_data;
-	u32 cores_per_node;
-
-	cores_per_node = c->x86_max_cores / amd_get_nodes_per_socket();
-
-	return cores_per_node * node_id;
+	return boot_cpu_data.x86_max_cores * node_id;
 }
 
 static void toggle_nb_mca_mst_cpu(u16 nid)

---

Signature · 5 lines

Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.
--

Peter Zijlstra

maintainer

2016-03-20 10:39 UTC #48 lore raw

Re: [PATCH 2/3] x86/topology: Fix AMD core count

Quoted 2 lines · Thomas Gleixner

Unfortunately that will break stuff in event/amd/core.c, ras/mce_amd_inj.c which rely on the AMD interpretation of c->x86_max_cores.

So the AMD NB stuff in events/amd/core.c is only for Fam10, Fam15 got its own uncore driver. (Fam10 had the uncore events through the same counters as the core PMU with with 'fun' constraints).

And since Fam10 isn't affected by this change in x86_max_cores, this _should_ work out, IF that NB code knows to switch off properly when not required.

Borislav Petkov

maintainer

2016-03-20 11:05 UTC #49 lore raw

Re: [PATCH 2/3] x86/topology: Fix AMD core count

Quoted 7 lines · Peter Zijlstra

So the AMD NB stuff in events/amd/core.c is only for Fam10, Fam15 got its own uncore driver. (Fam10 had the uncore events through the same counters as the core PMU with with 'fun' constraints).
And since Fam10 isn't affected by this change in x86_max_cores, this _should_ work out, IF that NB code knows to switch off properly when not required.

But but, amd_core_pmu_init() is called on F15h too. It only gets the different F15h event constraints and MSRs...

Signature · 4 lines

Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.

Peter Zijlstra

maintainer

2016-03-20 12:33 UTC #50 lore raw

Re: [PATCH 2/3] x86/topology: Fix AMD core count

Quoted 11 lines · Borislav Petkov

Quoted 7 lines · Peter Zijlstra
So the AMD NB stuff in events/amd/core.c is only for Fam10, Fam15 got its own uncore driver. (Fam10 had the uncore events through the same counters as the core PMU with with 'fun' constraints).
And since Fam10 isn't affected by this change in x86_max_cores, this _should_ work out, IF that NB code knows to switch off properly when not required.
But but, amd_core_pmu_init() is called on F15h too. It only gets the different F15h event constraints and MSRs...

Yes, but IIRC the F15h driver doesn't use the NB constraints, as they moved all the NB events to their own set of MSRs, which has events/amd/uncore.c.

So all the NB cruft in the core pmu is only relevant to F10h.

So F15h also calling and allocating NB cruft in the core PMU driver is entirely pointless.

Peter Zijlstra

maintainer

2016-03-20 12:48 UTC #51 lore raw

Re: [PATCH 2/3] x86/topology: Fix AMD core count

Quoted 8 lines · Peter Zijlstra

Yes, but IIRC the F15h driver doesn't use the NB constraints, as they moved all the NB events to their own set of MSRs, which has events/amd/uncore.c.
So all the NB cruft in the core pmu is only relevant to F10h.
So F15h also calling and allocating NB cruft in the core PMU driver is entirely pointless.

Something like so.

---
 arch/x86/events/amd/core.c   | 21 ++++++++++++++++++---
 arch/x86/events/perf_event.h |  5 +++++
 2 files changed, 23 insertions(+), 3 deletions(-)

arch/x86/events/amd/core.c +18 -3

index 049ada8d4e9c..62026a79a930 100644
@@ -369,7 +369,7 @@ static int amd_pmu_cpu_prepare(int cpu)
 
 	WARN_ON_ONCE(cpuc->amd_nb);
 
-	if (boot_cpu_data.x86_max_cores < 2)
+	if (!x86_pmu.amd_nb)
 		return NOTIFY_OK;
 
 	cpuc->amd_nb = amd_alloc_nb(cpu);

@@ -388,7 +388,7 @@ static void amd_pmu_cpu_starting(int cpu)
 
 	cpuc->perf_ctr_virt_mask = AMD64_EVENTSEL_HOSTONLY;
 
-	if (boot_cpu_data.x86_max_cores < 2)
+	if (!x86_pmu.amd_nb)
 		return;
 
 	nb_id = amd_get_nb_id(cpu);

@@ -414,7 +414,7 @@ static void amd_pmu_cpu_dead(int cpu)
 {
 	struct cpu_hw_events *cpuhw;
 
-	if (boot_cpu_data.x86_max_cores < 2)
+	if (!x86_pmu.amd_nb)
 		return;
 
 	cpuhw = &per_cpu(cpu_hw_events, cpu);

@@ -648,6 +648,8 @@ static __initconst const struct x86_pmu amd_pmu = {
 	.cpu_prepare		= amd_pmu_cpu_prepare,
 	.cpu_starting		= amd_pmu_cpu_starting,
 	.cpu_dead		= amd_pmu_cpu_dead,
+
+	.amd_nb			= 1;
 };
 
 static int __init amd_core_pmu_init(void)

@@ -674,6 +676,11 @@ static int __init amd_core_pmu_init(void)
 	x86_pmu.eventsel	= MSR_F15H_PERF_CTL;
 	x86_pmu.perfctr		= MSR_F15H_PERF_CTR;
 	x86_pmu.num_counters	= AMD64_NUM_COUNTERS_CORE;
+	/*
+	 * AMD Core perfctr has separate MSRs for the NB events, see
+	 * the amd/uncore.c driver.
+	 */
+	x86_pmu.amd_nb		= 0;
 
 	pr_cont("core perfctr, ");
 	return 0;

@@ -693,6 +700,14 @@ __init int amd_pmu_init(void)
 	if (ret)
 		return ret;
 
+	if (num_possible_cpus() == 1) {
+		/*
+		 * No point in allocating data structures to serialize
+		 * against other CPUs, when there is only the one CPU.
+		 */
+		x86_pmu.amd_nb = 0;
+	}
+
 	/* Events are common for all AMDs */
 	memcpy(hw_cache_event_ids, amd_hw_cache_event_ids,
 	       sizeof(hw_cache_event_ids));

arch/x86/events/perf_event.h +5 -0

index ba6ef18528c9..46d2ece10a7b 100644
@@ -608,6 +608,11 @@ struct x86_pmu {
 	atomic_t	lbr_exclusive[x86_lbr_exclusive_max];
 
 	/*
+	 * AMD bits
+	 */
+	unsigned int	amd_nb	: 1;
+
+	/*
 	 * Extra registers for events
 	 */
 	struct extra_reg *extra_regs;

Borislav Petkov

maintainer

2016-03-20 13:09 UTC #52 lore raw

Re: [PATCH 2/3] x86/topology: Fix AMD core count

Quoted 11 lines · Peter Zijlstra

Quoted 8 lines · Peter Zijlstra
Yes, but IIRC the F15h driver doesn't use the NB constraints, as they moved all the NB events to their own set of MSRs, which has events/amd/uncore.c.
So all the NB cruft in the core pmu is only relevant to F10h.
So F15h also calling and allocating NB cruft in the core PMU driver is entirely pointless.
Something like so.

Perhaps. Some minor notes below.

First a question about the big picture: why is amd/core.c even dealing with NB counters? Especially if NB has its own initcall amd_uncore_init()?

IOW, what I'm trying to say is, can we untangle uncore.c from core.c or is there some dependency in the modelling I'm not aware of... which is very likely, btw.

Quoted 10 lines

---
 arch/x86/events/amd/core.c   | 21 ++++++++++++++++++---
 arch/x86/events/perf_event.h |  5 +++++
 2 files changed, 23 insertions(+), 3 deletions(-)

arch/x86/events/amd/core.c +0 -0

index 049ada8d4e9c..62026a79a930 100644

arch/x86/events/amd/core.c static int amd_pmu_cpu_prepare(int cpu) · line 372

Quoted 5 lines

arch/x86/events/amd/core.c +1 -1

@@ -369,7 +369,7 @@ static int amd_pmu_cpu_prepare(int cpu)
 
 	WARN_ON_ONCE(cpuc->amd_nb);
 
-	if (boot_cpu_data.x86_max_cores < 2)
+	if (!x86_pmu.amd_nb)

Yeah, except there's cpuc->amd_nb and now pmu->amd_nb. Can we differentiate a bit more with the naming?

Quoted 22 lines

arch/x86/events/amd/core.c +2 -2

 		return NOTIFY_OK;
 
 	cpuc->amd_nb = amd_alloc_nb(cpu);

@@ -388,7 +388,7 @@ static void amd_pmu_cpu_starting(int cpu)
 
 	cpuc->perf_ctr_virt_mask = AMD64_EVENTSEL_HOSTONLY;
 
-	if (boot_cpu_data.x86_max_cores < 2)
+	if (!x86_pmu.amd_nb)
 		return;
 
 	nb_id = amd_get_nb_id(cpu);

@@ -414,7 +414,7 @@ static void amd_pmu_cpu_dead(int cpu)
 {
 	struct cpu_hw_events *cpuhw;
 
-	if (boot_cpu_data.x86_max_cores < 2)
+	if (!x86_pmu.amd_nb)
 		return;
 
 	cpuhw = &per_cpu(cpu_hw_events, cpu);

arch/x86/events/amd/core.c static __initconst const struct x86_pmu amd_pmu = { · line 652

Quoted 5 lines

arch/x86/events/amd/core.c +2 -0

@@ -648,6 +648,8 @@ static __initconst const struct x86_pmu amd_pmu = {
 	.cpu_prepare		= amd_pmu_cpu_prepare,
 	.cpu_starting		= amd_pmu_cpu_starting,
 	.cpu_dead		= amd_pmu_cpu_dead,
+
+	.amd_nb			= 1;

s/;/,/

Quoted 47 lines

arch/x86/events/amd/core.c +13 -0

 };
 
 static int __init amd_core_pmu_init(void)

@@ -674,6 +676,11 @@ static int __init amd_core_pmu_init(void)
 	x86_pmu.eventsel	= MSR_F15H_PERF_CTL;
 	x86_pmu.perfctr		= MSR_F15H_PERF_CTR;
 	x86_pmu.num_counters	= AMD64_NUM_COUNTERS_CORE;
+	/*
+	 * AMD Core perfctr has separate MSRs for the NB events, see
+	 * the amd/uncore.c driver.
+	 */
+	x86_pmu.amd_nb		= 0;
 
 	pr_cont("core perfctr, ");
 	return 0;

@@ -693,6 +700,14 @@ __init int amd_pmu_init(void)
 	if (ret)
 		return ret;
 
+	if (num_possible_cpus() == 1) {
+		/*
+		 * No point in allocating data structures to serialize
+		 * against other CPUs, when there is only the one CPU.
+		 */
+		x86_pmu.amd_nb = 0;
+	}
+
 	/* Events are common for all AMDs */
 	memcpy(hw_cache_event_ids, amd_hw_cache_event_ids,
 	       sizeof(hw_cache_event_ids));

arch/x86/events/perf_event.h +5 -0

index ba6ef18528c9..46d2ece10a7b 100644
@@ -608,6 +608,11 @@ struct x86_pmu {
 	atomic_t	lbr_exclusive[x86_lbr_exclusive_max];
 
 	/*
+	 * AMD bits
+	 */
+	unsigned int	amd_nb	: 1;
+
+	/*
 	 * Extra registers for events
 	 */
 	struct extra_reg *extra_regs;

Signature · 4 lines

Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.

Peter Zijlstra

maintainer

2016-03-20 17:10 UTC #53 lore raw

Re: [PATCH 2/3] x86/topology: Fix AMD core count

Quoted 2 lines · Borislav Petkov

First a question about the big picture: why is amd/core.c even dealing with NB counters?

It it not, it is dealing with Fam10 NB events.

Fam10h doesn't have NB counters. Its NB events are on the same counters as all the other events. Its just that they have constraints.

Borislav Petkov

maintainer

2016-03-20 18:46 UTC #54 lore raw

Re: [PATCH 2/3] x86/topology: Fix AMD core count

Quoted 8 lines · Peter Zijlstra

Quoted 2 lines · Borislav Petkov
First a question about the big picture: why is amd/core.c even dealing with NB counters?
It it not, it is dealing with Fam10 NB events.
Fam10h doesn't have NB counters. Its NB events are on the same counters as all the other events. Its just that they have constraints.

See, and I was missing something: so there's the core x86_pmu and then more PMUs get added with perf_pmu_register(). I.e., the uncore stuff, in this case. Ok, makes sense...

Signature · 4 lines

Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.

tip-bot for Peter Zijlstra

merge bot

2016-03-29 08:50 UTC #55 lore raw

[tip:x86/urgent] perf/x86/amd: Cleanup Fam10h NB event constraints

perf/x86/amd: Cleanup Fam10h NB event constraints

Commit-ID:  32b62f446827f696cc474a6d83cea93693c5ed49
Gitweb:     http://git.kernel.org/tip/32b62f446827f696cc474a6d83cea93693c5ed49
Author:     Peter Zijlstra <[email protected]>
AuthorDate: Fri, 25 Mar 2016 15:52:35 +0100
Committer:  Thomas Gleixner <[email protected]>
CommitDate: Tue, 29 Mar 2016 10:45:04 +0200

perf/x86/amd: Cleanup Fam10h NB event constraints

Avoid allocating the AMD NB event constraints data structure when not needed. This gets rid of x86_max_cores usage and avoids allocating this on AMD Core Perfctr supporting hardware (which has separate MSRs for NB events).

Signed-off-by Peter Zijlstra (Intel) <[email protected]>

Signed-off-by Borislav Petkov <[email protected]>

Cc Rui Huang <[email protected]>

Cc Borislav Petkov <[email protected]>

Link http://lkml.kernel.org/r/[email protected] open here

---
 arch/x86/events/amd/core.c   | 21 ++++++++++++++++++---
 arch/x86/events/perf_event.h |  5 +++++
 2 files changed, 23 insertions(+), 3 deletions(-)

arch/x86/events/amd/core.c +18 -3

index 049ada8d..86a9bec 100644
@@ -369,7 +369,7 @@ static int amd_pmu_cpu_prepare(int cpu)
 
 	WARN_ON_ONCE(cpuc->amd_nb);
 
-	if (boot_cpu_data.x86_max_cores < 2)
+	if (!x86_pmu.amd_nb_constraints)
 		return NOTIFY_OK;
 
 	cpuc->amd_nb = amd_alloc_nb(cpu);

@@ -388,7 +388,7 @@ static void amd_pmu_cpu_starting(int cpu)
 
 	cpuc->perf_ctr_virt_mask = AMD64_EVENTSEL_HOSTONLY;
 
-	if (boot_cpu_data.x86_max_cores < 2)
+	if (!x86_pmu.amd_nb_constraints)
 		return;
 
 	nb_id = amd_get_nb_id(cpu);

@@ -414,7 +414,7 @@ static void amd_pmu_cpu_dead(int cpu)
 {
 	struct cpu_hw_events *cpuhw;
 
-	if (boot_cpu_data.x86_max_cores < 2)
+	if (!x86_pmu.amd_nb_constraints)
 		return;
 
 	cpuhw = &per_cpu(cpu_hw_events, cpu);

@@ -648,6 +648,8 @@ static __initconst const struct x86_pmu amd_pmu = {
 	.cpu_prepare		= amd_pmu_cpu_prepare,
 	.cpu_starting		= amd_pmu_cpu_starting,
 	.cpu_dead		= amd_pmu_cpu_dead,
+
+	.amd_nb_constraints	= 1,
 };
 
 static int __init amd_core_pmu_init(void)

@@ -674,6 +676,11 @@ static int __init amd_core_pmu_init(void)
 	x86_pmu.eventsel	= MSR_F15H_PERF_CTL;
 	x86_pmu.perfctr		= MSR_F15H_PERF_CTR;
 	x86_pmu.num_counters	= AMD64_NUM_COUNTERS_CORE;
+	/*
+	 * AMD Core perfctr has separate MSRs for the NB events, see
+	 * the amd/uncore.c driver.
+	 */
+	x86_pmu.amd_nb_constraints = 0;
 
 	pr_cont("core perfctr, ");
 	return 0;

@@ -693,6 +700,14 @@ __init int amd_pmu_init(void)
 	if (ret)
 		return ret;
 
+	if (num_possible_cpus() == 1) {
+		/*
+		 * No point in allocating data structures to serialize
+		 * against other CPUs, when there is only the one CPU.
+		 */
+		x86_pmu.amd_nb_constraints = 0;
+	}
+
 	/* Events are common for all AMDs */
 	memcpy(hw_cache_event_ids, amd_hw_cache_event_ids,
 	       sizeof(hw_cache_event_ids));

arch/x86/events/perf_event.h +5 -0

index ba6ef18..716d048 100644
@@ -608,6 +608,11 @@ struct x86_pmu {
 	atomic_t	lbr_exclusive[x86_lbr_exclusive_max];
 
 	/*
+	 * AMD bits
+	 */
+	unsigned int	amd_nb_constraints : 1;
+
+	/*
 	 * Extra registers for events
 	 */
 	struct extra_reg *extra_regs;

Huang Rui

Tested-by on v1 2/3

2016-03-22 07:56 UTC #56 lore raw

Re: [PATCH 2/3] x86/topology: Fix AMD core count

Quoted 10 lines · Peter Zijlstra

It turns out AMD gets x86_max_cores wrong when there are compute units.
The issue is that Linux assumes:
nr_logical_cpus = nr_cores * nr_siblings
But AMD reports its CU unit as 2 cores, but then sets num_smp_siblings to 2 as well.

Quoted 7 lines

Cc Ingo Molnar <[email protected]>
Cc Borislav Petkov <[email protected]>
Cc Thomas Gleixner <[email protected]>
Cc Andreas Herrmann <[email protected]>
Reported-by Xiong Zhou <[email protected]>
Fixes 1f12e32f4cd5 ("x86/topology: Create logical package id")
Signed-off-by Peter Zijlstra (Intel) <[email protected]>

I tested this patch both on fam15h and fam16h platforms, x86_max_cores can report correct compute unit numbers. And /sys/devices/system/cpu/cpuX/topology/thread_siblings_list /sys/devices/system/cpu/cpuX/topology/core_siblings_list are also right. Thanks.

Tested-by Huang Rui <[email protected]>

Quoted 33 lines

Link http://lkml.kernel.org/r/[email protected] open here

---
 arch/x86/kernel/cpu/amd.c |    8 ++++----
 arch/x86/kernel/smpboot.c |   11 ++++++-----
 2 files changed, 10 insertions(+), 9 deletions(-)

arch/x86/kernel/cpu/amd.c +4 -4

@@ -313,9 +313,9 @@ static void amd_get_topology(struct cpui
 		node_id = ecx & 7;
 
 		/* get compute unit information */
-		smp_num_siblings = ((ebx >> 8) & 3) + 1;
+		cores_per_cu = smp_num_siblings = ((ebx >> 8) & 3) + 1;
+		c->x86_max_cores /= smp_num_siblings;
 		c->compute_unit_id = ebx & 0xff;
-		cores_per_cu += ((ebx >> 8) & 3);
 	} else if (cpu_has(c, X86_FEATURE_NODEID_MSR)) {
 		u64 value;

@@ -331,8 +331,8 @@ static void amd_get_topology(struct cpui
 		u32 cus_per_node;
 
 		set_cpu_cap(c, X86_FEATURE_AMD_DCM);
-		cores_per_node = c->x86_max_cores / nodes_per_socket;
-		cus_per_node = cores_per_node / cores_per_cu;
+		cus_per_node = c->x86_max_cores / nodes_per_socket;
+		cores_per_node = cus_per_node * cores_per_cu;
 
 		/* store NodeID, use llc_shared_map to store sibling info */
 		per_cpu(cpu_llc_id, cpu) = node_id;

Peter Zijlstra

maintainer

2016-03-18 15:08 UTC #57 lore raw

PATCH 3/3 x86/topology: Fix Intel HT disable

As per the comment below; due to BIOS it is sometimes impossible to know if there actually are smp siblings until the machine is fully enumerated.

Signed-off-by Peter Zijlstra (Intel) <[email protected]>

---
 arch/x86/kernel/smpboot.c |   15 ++++++++++++++-
 1 file changed, 14 insertions(+), 1 deletion(-)

arch/x86/kernel/smpboot.c +14 -1

@@ -312,8 +312,21 @@ static void __init smp_init_package_map(
 	/*
 	 * Today neither Intel nor AMD support heterogenous systems. That
 	 * might change in the future....
+	 *
+	 * While ideally we'd want '* smp_num_siblings' in the below @ncpus
+	 * computation, this won't actually work since some Intel BIOSes
+	 * report inconsistent HT data when they disable HT.
+	 *
+	 * In particular, they reduce the APIC-IDs to only include the cores,
+	 * but leave the CPUID topology to say there are (2) siblings.
+	 * This means we don't know how many threads there will be until
+	 * after the APIC enumeration.
+	 *
+	 * By not including this we'll sometimes over-estimate the number of
+	 * logical packages by the amount of !present siblings, but this is
+	 * still better than MAX_LOCAL_APIC.
 	 */
-	ncpus = boot_cpu_data.x86_max_cores * smp_num_siblings;
+	ncpus = boot_cpu_data.x86_max_cores;
 	__max_logical_packages = DIV_ROUND_UP(nr_cpu_ids, ncpus);
 
 	/*

tip-bot for Peter Zijlstra

merge bot

2016-03-19 09:34 UTC #58 lore raw

[tip:x86/urgent] x86/topology: Fix Intel HT disable

x86/topology: Fix Intel HT disable

Commit-ID:  63d1e995be455ae9196270eb4b789de21afd42ed
Gitweb:     http://git.kernel.org/tip/63d1e995be455ae9196270eb4b789de21afd42ed
Author:     Peter Zijlstra <[email protected]>
AuthorDate: Fri, 18 Mar 2016 16:03:48 +0100
Committer:  Thomas Gleixner <[email protected]>
CommitDate: Sat, 19 Mar 2016 10:26:40 +0100

x86/topology: Fix Intel HT disable

As per the comment in the code; due to BIOS it is sometimes impossible to know if there actually are smp siblings until the machine is fully enumerated. So we rather overestimate the number of possible packages.

Fixes 1f12e32f4cd5 ("x86/topology: Create logical package id")

Signed-off-by Peter Zijlstra (Intel) <[email protected]>

Cc Mike Galbraith <[email protected]>

Link http://lkml.kernel.org/r/[email protected] open here