Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]
Groups > linux.kernel > #1694647 > unrolled thread
| Started by | Borislav Petkov <bp@suse.de> |
|---|---|
| First post | 2017-07-24 13:20 +0200 |
| Last post | 2017-07-28 11:50 +0200 |
| Articles | 5 — 2 participants |
Back to article view | Back to linux.kernel
This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by
below is the oldest one visible, not the original post.
Re: [PATCH v4 2/2] x86/amd: Fixup cpu_core_id for family17h downcore configuration Borislav Petkov <bp@suse.de> - 2017-07-24 13:20 +0200
Re: [PATCH v4 2/2] x86/amd: Fixup cpu_core_id for family17h downcore configuration Borislav Petkov <bp@suse.de> - 2017-07-24 16:50 +0200
Re: [PATCH v4 2/2] x86/amd: Fixup cpu_core_id for family17h downcore configuration Borislav Petkov <bp@suse.de> - 2017-07-25 08:00 +0200
Re: [PATCH v4 2/2] x86/amd: Fixup cpu_core_id for family17h downcore configuration Borislav Petkov <bp@suse.de> - 2017-07-25 10:10 +0200
Re: [PATCH v4 2/2] x86/amd: Fixup cpu_core_id for family17h downcore configuration Andreas Herrmann <aherrmann@suse.com> - 2017-07-28 11:50 +0200
| From | Borislav Petkov <bp@suse.de> |
|---|---|
| Date | 2017-07-24 13:20 +0200 |
| Subject | Re: [PATCH v4 2/2] x86/amd: Fixup cpu_core_id for family17h downcore configuration |
| Message-ID | <u6Jz3-42s-9@gated-at.bofh.it> |
On Mon, Jul 24, 2017 at 04:22:45AM -0500, Suravee Suthikulpanit wrote:
> For family17h, current cpu_core_id is directly taken from the value
> CPUID_Fn8000001E_EBX[7:0] (CoreId), which is the physical ID of the
> core within a die. However, on system with downcore configuration
> (where not all physical cores within a die are available), this could
> result in the case where cpu_core_id > (cores_per_node - 1).
And that is a problem because...?
> Fix up the cpu_core_id by breaking down the bitfields of CoreId,
> and calculate relative ID using available topology information.
>
> Signed-off-by: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
> ---
> arch/x86/kernel/cpu/amd.c | 74 ++++++++++++++++++++++++++++++++++-------------
> 1 file changed, 54 insertions(+), 20 deletions(-)
Btw, I have to say, it is much easier to review now.
> diff --git a/arch/x86/kernel/cpu/amd.c b/arch/x86/kernel/cpu/amd.c
> index a2a52b5..b5ea28f 100644
> --- a/arch/x86/kernel/cpu/amd.c
> +++ b/arch/x86/kernel/cpu/amd.c
> @@ -305,37 +305,71 @@ static void __get_topoext(struct cpuinfo_x86 *c, int cpu)
> {
> u32 eax, ebx, ecx, edx;
> u8 node_id;
> + u16 l3_nshared = 0;
> +
> + if (cpuid_edx(0x80000006)) {
Ok, so Janakarajan did some L3 detection:
https://lkml.kernel.org/r/cover.1497452002.git.Janakarajan.Natarajan@amd.com
and now that you need it too, I'd like you to refactor and unify that
L3 detection code and put it in arch/x86/kernel/cpu/intel_cacheinfo.c
(yes, we will rename that file one fine day :-)) along with accessors
for other users to call. init_amd_cacheinfo() looks good to me right
now.
> + cpuid_count(0x8000001d, 3, &eax, &ebx, &ecx, &edx);
> + l3_nshared = ((eax >> 14) & 0xfff) + 1;
> + }
>
> cpuid(0x8000001e, &eax, &ebx, &ecx, &edx);
>
> node_id = ecx & 0xff;
> smp_num_siblings = ((ebx >> 8) & 0xff) + 1;
>
> - if (c->x86 == 0x15)
> - c->cu_id = ebx & 0xff;
> -
> - if (c->x86 >= 0x17) {
> - c->cpu_core_id = ebx & 0xff;
> -
> - if (smp_num_siblings > 1)
> - c->x86_max_cores /= smp_num_siblings;
> - }
> + switch (c->x86) {
> + case 0x17: {
> + u32 tmp, ccx_offset, cpu_offset;
>
> - /*
> - * We may have multiple LLCs if L3 caches exist, so check if we
> - * have an L3 cache by looking at the L3 cache CPUID leaf.
> - */
> - if (cpuid_edx(0x80000006)) {
> - if (c->x86 == 0x17) {
> + /*
> + * In family 17h, the CPUID_Fn8000001E_EBX[7:0] (CoreId)
> + * is non-contiguous in downcore and non-SMT cases.
> + * Fixup the cpu_core_id to be contiguous for cores within
> + * the die.
Why do we need it to be contiguous? It is not contiguous on Intel too.
> + */
> + tmp = ebx & 0xff;
> + if (smp_num_siblings == 1) {
> /*
> - * LLC is at the core complex level.
> - * Core complex id is ApicId[3].
> + * CoreId bit-encoding for SMT-disabled
> + * [7:4] : die
> + * [3] : ccx
> + * [2:0] : core
> */
> - per_cpu(cpu_llc_id, cpu) = c->apicid >> 3;
> + ccx_offset = ((tmp >> 3) & 1) * l3_nshared;
> + cpu_offset = tmp & 7;
> } else {
> - /* LLC is at the node level. */
> - per_cpu(cpu_llc_id, cpu) = node_id;
> + /*
> + * CoreId bit-encoding for SMT-enabled
> + * [7:3] : die
> + * [2] : ccx
> + * [1:0] : core
> + */
> + ccx_offset = ((tmp >> 2) & 1) * l3_nshared /
> + smp_num_siblings;
> + cpu_offset = tmp & 3;
> + c->x86_max_cores /= smp_num_siblings;
> +
> }
> + c->cpu_core_id = ccx_offset + cpu_offset;
> +
> + /*
> + * Family17h L3 cache (LLC) is at Core Complex (CCX).
> + * There could be multiple CCXs in a node.
> + * CCX ID is ApicId[3].
> + */
> + per_cpu(cpu_llc_id, cpu) = c->apicid >> 3;
> +
> + pr_debug("Fixup coreid:%#x to cpu_core_id:%#x\n",
> + tmp, c->cpu_core_id);
> + break;
> + }
> + case 0x15:
> + c->cu_id = ebx & 0xff;
> + /* Follow through */
> + default:
> + /* LLC is default to L3, which generally per-node */
> + if (l3_nshared > 0)
> + per_cpu(cpu_llc_id, cpu) = node_id;
If this needs to be executed unconditionally, just move it out of the
switch-case.
--
Regards/Gruss,
Boris.
SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nürnberg)
--
[toc] | [next] | [standalone]
| From | Borislav Petkov <bp@suse.de> |
|---|---|
| Date | 2017-07-24 16:50 +0200 |
| Message-ID | <u6MQj-6cH-41@gated-at.bofh.it> |
| In reply to | #1694647 |
On Mon, Jul 24, 2017 at 09:14:18PM +0700, Suravee Suthikulpanit wrote:
> Actually, this is not totally accurate. My apology. This patch is
> mainly fix to incorrect core ID in /proc/cpuinfo.
So you're "fixing" only some numbering thing. Because core_id doesn't
have any influence on anything. Here's on an Intel box I have here:
processor : 0 physical id : 0 core id : 0
processor : 1 physical id : 1 core id : 0
processor : 2 physical id : 2 core id : 0
processor : 3 physical id : 3 core id : 0
processor : 4 physical id : 0 core id : 8
processor : 5 physical id : 1 core id : 8
processor : 6 physical id : 2 core id : 8
processor : 7 physical id : 3 core id : 8
processor : 8 physical id : 0 core id : 2
processor : 9 physical id : 1 core id : 2
processor : 10 physical id : 2 core id : 2
processor : 11 physical id : 3 core id : 2
processor : 12 physical id : 0 core id : 10
processor : 13 physical id : 1 core id : 10
processor : 14 physical id : 2 core id : 10
processor : 15 physical id : 3 core id : 10
processor : 16 physical id : 0 core id : 1
processor : 17 physical id : 1 core id : 1
processor : 18 physical id : 2 core id : 1
processor : 19 physical id : 3 core id : 1
processor : 20 physical id : 0 core id : 9
processor : 21 physical id : 1 core id : 9
processor : 22 physical id : 2 core id : 9
processor : 23 physical id : 3 core id : 9
processor : 24 physical id : 0 core id : 3
processor : 25 physical id : 1 core id : 3
processor : 26 physical id : 2 core id : 3
processor : 27 physical id : 3 core id : 3
processor : 28 physical id : 0 core id : 11
processor : 29 physical id : 1 core id : 11
processor : 30 physical id : 2 core id : 11
processor : 31 physical id : 3 core id : 11
processor : 32 physical id : 0 core id : 0
processor : 33 physical id : 1 core id : 0
processor : 34 physical id : 2 core id : 0
processor : 35 physical id : 3 core id : 0
processor : 36 physical id : 0 core id : 8
processor : 37 physical id : 1 core id : 8
processor : 38 physical id : 2 core id : 8
processor : 39 physical id : 3 core id : 8
processor : 40 physical id : 0 core id : 2
processor : 41 physical id : 1 core id : 2
processor : 42 physical id : 2 core id : 2
processor : 43 physical id : 3 core id : 2
processor : 44 physical id : 0 core id : 10
processor : 45 physical id : 1 core id : 10
processor : 46 physical id : 2 core id : 10
processor : 47 physical id : 3 core id : 10
processor : 48 physical id : 0 core id : 1
processor : 49 physical id : 1 core id : 1
processor : 50 physical id : 2 core id : 1
processor : 51 physical id : 3 core id : 1
processor : 52 physical id : 0 core id : 9
processor : 53 physical id : 1 core id : 9
processor : 54 physical id : 2 core id : 9
processor : 55 physical id : 3 core id : 9
processor : 56 physical id : 0 core id : 3
processor : 57 physical id : 1 core id : 3
processor : 58 physical id : 2 core id : 3
processor : 59 physical id : 3 core id : 3
processor : 60 physical id : 0 core id : 11
processor : 61 physical id : 1 core id : 11
processor : 62 physical id : 2 core id : 11
processor : 63 physical id : 3 core id : 11
So those core id numbers can be anything as long as the cpumasks used by
the scheduler are correct.
> This is due to the cpu_core_id fixup in amd_get_topology() below:
>
> /* fixup multi-node processor information */
> if (nodes_per_socket > 1) {
> u32 cus_per_node;
>
> set_cpu_cap(c, X86_FEATURE_AMD_DCM);
> cus_per_node = c->x86_max_cores / nodes_per_socket;
>
> /* core id has to be in the [0 .. cores_per_node - 1] range */
> c->cpu_core_id %= cus_per_node;
> }
AFAICT, Andreas did this for MC at the time:
4a376ec3a259 ("x86: Fix CPU llc_shared_map information for AMD Magny-Cours")
but I don't think we need to care about core_ids fitting into the node
range anymore. For the above reason - topology doesn't use core ids.
So you can just as well let ->cpu_core_id be derived from the
->initial_apicid as it is being done now in amd_detect_cmp().
In order not to cause any more confusion, you can limit the above fixup
to anything below F17h so that we don't upset existing users and add a
big fat comment as to why we're doing this. But if it is only a silly
numbering thing, I don't see the need for doing that jumping through
hoops.
--
Regards/Gruss,
Boris.
SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nürnberg)
--
[toc] | [prev] | [next] | [standalone]
| From | Borislav Petkov <bp@suse.de> |
|---|---|
| Date | 2017-07-25 08:00 +0200 |
| Message-ID | <u712W-7ke-5@gated-at.bofh.it> |
| In reply to | #1694774 |
On Tue, Jul 25, 2017 at 12:51:53PM +0700, Suravee Suthikulpanit wrote:
> Ok. Sure, it doesn't need be contiguous. But at least the cpu_core_id should
> represent an ID that make some sense since it is used in the
> arch/x86/kernel/smpboot.c: match_smt() and some other places. So, if it's
> invalid for the downcore configuration (i.e. duplicated where it should not
> be), we should at least clean this up.
Ah right, we do use it for the SMT siblings. So yes, it should be
correct for them. And I'm pretty sure the numbers we derive from the
initial APIC ID are already good enough for that.
> I will update the patch to only limit the fixup to pre-family17h.
Yeah, give that a try. Make sure to check the topology masks are
correct.
Thanks.
--
Regards/Gruss,
Boris.
SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nürnberg)
--
[toc] | [prev] | [next] | [standalone]
| From | Borislav Petkov <bp@suse.de> |
|---|---|
| Date | 2017-07-25 10:10 +0200 |
| Message-ID | <u734L-iG-35@gated-at.bofh.it> |
| In reply to | #1695403 |
On Tue, Jul 25, 2017 at 01:02:49PM +0700, Suravee Suthikulpanit wrote:
> Actually, this commit change how we derive the cpu_core_id fro family17h to
> use CPUID_Fn8000001E_EBX instead of from APIC ID for family17h and later.
Is the CPUID info correct also for downcored, MCM configurations? If so,
I guess you could exit early from amd_get_topology() on F17h...
--
Regards/Gruss,
Boris.
SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nürnberg)
--
[toc] | [prev] | [next] | [standalone]
| From | Andreas Herrmann <aherrmann@suse.com> |
|---|---|
| Date | 2017-07-28 11:50 +0200 |
| Message-ID | <u8a49-20x-7@gated-at.bofh.it> |
| In reply to | #1694774 |
On Mon, Jul 24, 2017 at 04:44:45PM +0200, Borislav Petkov wrote:
> On Mon, Jul 24, 2017 at 09:14:18PM +0700, Suravee Suthikulpanit wrote:
> > Actually, this is not totally accurate. My apology. This patch is
> > mainly fix to incorrect core ID in /proc/cpuinfo.
>
> So you're "fixing" only some numbering thing. Because core_id doesn't
> have any influence on anything. Here's on an Intel box I have here:
>
> processor : 0 physical id : 0 core id : 0
> processor : 1 physical id : 1 core id : 0
---8<---
> processor : 62 physical id : 2 core id : 11
> processor : 63 physical id : 3 core id : 11
>
> So those core id numbers can be anything as long as the cpumasks used by
> the scheduler are correct.
And as long as the user is able to find consistent topology
information. (Ie. via /proc/cpuinfo and/or
/sys/bus/cpu/devices/cpuX/topology/)
> > This is due to the cpu_core_id fixup in amd_get_topology() below:
> >
> > /* fixup multi-node processor information */
> > if (nodes_per_socket > 1) {
> > u32 cus_per_node;
> >
> > set_cpu_cap(c, X86_FEATURE_AMD_DCM);
> > cus_per_node = c->x86_max_cores / nodes_per_socket;
> >
> > /* core id has to be in the [0 .. cores_per_node - 1] range */
> > c->cpu_core_id %= cus_per_node;
> > }
>
> AFAICT, Andreas did this for MC at the time:
>
> 4a376ec3a259 ("x86: Fix CPU llc_shared_map information for AMD Magny-Cours")
>
> but I don't think we need to care about core_ids fitting into the node
> range anymore. For the above reason - topology doesn't use core ids.
IIRC this was the way to fix llc_shared_map as CPUID functions could
not be used to figure which cores belong to the same compute unit.
> So you can just as well let ->cpu_core_id be derived from the
> ->initial_apicid as it is being done now in amd_detect_cmp().
>
> In order not to cause any more confusion, you can limit the above fixup
> to anything below F17h so that we don't upset existing users and add a
> big fat comment as to why we're doing this. But if it is only a silly
> numbering thing, I don't see the need for doing that jumping through
> hoops.
>
> --
> Regards/Gruss,
> Boris.
Regards,
Andreas
[toc] | [prev] | [standalone]
Back to top | Article view | linux.kernel
csiph-web