Re: [PATCH v2 1/2] x86/amd: Refactor topology extension related code
On Mon, Jul 24, 2017 at 10:28:34AM +0700, Suravee Suthikulpanit wrote: > Are you referring to the part that I moved the "node_id = ecx & 0xff" ... Yes, that's what I'm referring to. Btw, I went and did it myself, now look at my diff below. Now that diff is pretty straight-forward - stuff is picked from one place and put somewhere else. Exactly like it should be done. Your diff is intermixing the new and old function because, *since* you're touching stuff in there, the diff algorithm can't really detect the move. Or maybe because you're using an old git but it doesn't look like it because your patch applied gives the same "intermixed" diff here. Also, note that I've made the new function: +static void __get_topoext(struct cpuinfo_x86 *c, int cpu) so that you don't have to do smp_processor_id() twice. Now, after you get it this way, you can start doing your cleanups and improvements ontop because a diff like that is very easy to review. Thanks. diff --git a/arch/x86/kernel/cpu/amd.c b/arch/x86/kernel/cpu/amd.c index 110ca5d2bb87..ee55f811b8c6 100644 --- a/arch/x86/kernel/cpu/amd.c +++ b/arch/x86/kernel/cpu/amd.c @@ -297,13 +297,55 @@ static int nearby_node(int apicid) } #endif +#ifdef CONFIG_SMP + +/* + * Get topology information via X86_FEATURE_TOPOEXT. + */ +static void __get_topoext(struct cpuinfo_x86 *c, int cpu) +{ + u32 eax, ebx, ecx, edx; + u8 node_id; + + cpuid(0x801e, &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; + } + + /* +* 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(0x8006)) { + if (c->x86 == 0x17) { + /* +* LLC is at the core complex level. +* Core complex id is ApicId[3]. +*/ + per_cpu(cpu_llc_id, cpu) = c->apicid >> 3; + } else { + /* LLC is at the node level. */ + per_cpu(cpu_llc_id, cpu) = node_id; + } + } +} + /* * Fixup core topology information for * (1) AMD multi-node processors * Assumption: Number of cores in each internal node is the same. * (2) AMD processors supporting compute units */ -#ifdef CONFIG_SMP static void amd_get_topology(struct cpuinfo_x86 *c) { u8 node_id; @@ -311,39 +353,7 @@ static void amd_get_topology(struct cpuinfo_x86 *c) /* get information required for multi-node processors */ if (boot_cpu_has(X86_FEATURE_TOPOEXT)) { - u32 eax, ebx, ecx, edx; - - cpuid(0x801e, &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; - } - - /* -* 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(0x8006)) { - if (c->x86 == 0x17) { - /* -* LLC is at the core complex level. -* Core complex id is ApicId[3]. -*/ - per_cpu(cpu_llc_id, cpu) = c->apicid >> 3; - } else { - /* LLC is at the node level. */ - per_cpu(cpu_llc_id, cpu) = node_id; - } - } + __get_topoext(c, cpu); } else if (cpu_has(c, X86_FEATURE_NODEID_MSR)) { u64 value; -- Regards/Gruss, Boris. SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nürnberg) --
Re: [PATCH v2 1/2] x86/amd: Refactor topology extension related code
Boris, On 7/22/17 23:12, Borislav Petkov wrote: On Fri, Jul 21, 2017 at 09:00:38PM -0500, Suravee Suthikulpanit wrote: Refactoring in preparation for subsequent changes. There is no functional change. Signed-off-by: Suravee Suthikulpanit --- arch/x86/kernel/cpu/amd.c | 79 ++- 1 file changed, 44 insertions(+), 35 deletions(-) diff --git a/arch/x86/kernel/cpu/amd.c b/arch/x86/kernel/cpu/amd.c index bb5abe8..74d8d7c 100644 --- a/arch/x86/kernel/cpu/amd.c +++ b/arch/x86/kernel/cpu/amd.c @@ -297,54 +297,63 @@ static int nearby_node(int apicid) #endif /* - * Fixup core topology information for - * (1) AMD multi-node processors - * Assumption: Number of cores in each internal node is the same. - * (2) AMD processors supporting compute units + * Get topology information via X86_FEATURE_TOPOEXT */ -#ifdef CONFIG_SMP -static void amd_get_topology(struct cpuinfo_x86 *c) +static void __get_topoext(struct cpuinfo_x86 *c) { - u8 node_id; + u32 eax, ebx, ecx, edx; int cpu = smp_processor_id(); - /* get information required for multi-node processors */ - if (boot_cpu_has(X86_FEATURE_TOPOEXT)) { - u32 eax, ebx, ecx, edx; + cpuid(0x801e, &eax, &ebx, &ecx, &edx); - cpuid(0x801e, &eax, &ebx, &ecx, &edx); + smp_num_siblings = ((ebx >> 8) & 0xff) + 1; - node_id = ecx & 0xff; When reviewers ask you about a preparatory cleanup patch, you don't sneak in changes in it - you *only* *move* the code so that the change is *absolutely* comprehensible. Ontop you do changes. Don't tell me you didn't know that! I know that we should not sneak in change. I might have missed something here. Are you referring to the part that I moved the "node_id = ecx & 0xff" from the top level of the function to inside the "if/else" logic where it is the only place that used within this new refactored __get_topoext() and there is nothing changed functionally? If that's really the case, I'll fix it. Thanks, Suravee
Re: [PATCH v2 1/2] x86/amd: Refactor topology extension related code
Hi Suravee, [auto build test ERROR on tip/x86/core] [also build test ERROR on v4.13-rc1 next-20170721] [if your patch is applied to the wrong git tree, please drop us a note to help improve the system] url: https://github.com/0day-ci/linux/commits/Suravee-Suthikulpanit/x86-amd-Refactor-topology-extension-related-code/20170724-024344 config: x86_64-randconfig-x007-201730 (attached as .config) compiler: gcc-6 (Debian 6.2.0-3) 6.2.0 20160901 reproduce: # save the attached .config to linux build tree make ARCH=x86_64 All errors (new ones prefixed by >>): arch/x86/kernel/cpu/amd.c: In function '__get_topoext': >> arch/x86/kernel/cpu/amd.c:309:19: error: lvalue required as left operand of >> assignment smp_num_siblings = ((ebx >> 8) & 0xff) + 1; ^ At top level: arch/x86/kernel/cpu/amd.c:302:13: warning: '__get_topoext' defined but not used [-Wunused-function] static void __get_topoext(struct cpuinfo_x86 *c) ^ vim +309 arch/x86/kernel/cpu/amd.c ^1da177e arch/i386/kernel/cpu/amd.c Linus Torvalds2005-04-16 298 11fdd252 arch/x86/kernel/cpu/amd.c Yinghai Lu2008-09-07 299 /* 1c2d1f62 arch/x86/kernel/cpu/amd.c Suravee Suthikulpanit 2017-07-21 300 * Get topology information via X86_FEATURE_TOPOEXT 4a376ec3 arch/x86/kernel/cpu/amd.c Andreas Herrmann 2009-09-03 301 */ 1c2d1f62 arch/x86/kernel/cpu/amd.c Suravee Suthikulpanit 2017-07-21 302 static void __get_topoext(struct cpuinfo_x86 *c) 4a376ec3 arch/x86/kernel/cpu/amd.c Andreas Herrmann 2009-09-03 303 { 79a8b9aa arch/x86/kernel/cpu/amd.c Borislav Petkov 2017-02-05 304 u32 eax, ebx, ecx, edx; 1c2d1f62 arch/x86/kernel/cpu/amd.c Suravee Suthikulpanit 2017-07-21 305 int cpu = smp_processor_id(); 6057b4d3 arch/x86/kernel/cpu/amd.c Andreas Herrmann 2010-09-30 306 79a8b9aa arch/x86/kernel/cpu/amd.c Borislav Petkov 2017-02-05 307 cpuid(0x801e, &eax, &ebx, &ecx, &edx); 79a8b9aa arch/x86/kernel/cpu/amd.c Borislav Petkov 2017-02-05 308 79a8b9aa arch/x86/kernel/cpu/amd.c Borislav Petkov 2017-02-05 @309 smp_num_siblings = ((ebx >> 8) & 0xff) + 1; 79a8b9aa arch/x86/kernel/cpu/amd.c Borislav Petkov 2017-02-05 310 79a8b9aa arch/x86/kernel/cpu/amd.c Borislav Petkov 2017-02-05 311 if (c->x86 == 0x15) 79a8b9aa arch/x86/kernel/cpu/amd.c Borislav Petkov 2017-02-05 312 c->cu_id = ebx & 0xff; b6a50cdd arch/x86/kernel/cpu/amd.c Yazen Ghannam 2016-11-08 313 08b25963 arch/x86/kernel/cpu/amd.c Yazen Ghannam 2017-02-05 314 if (c->x86 >= 0x17) { 08b25963 arch/x86/kernel/cpu/amd.c Yazen Ghannam 2017-02-05 315 c->cpu_core_id = ebx & 0xff; 08b25963 arch/x86/kernel/cpu/amd.c Yazen Ghannam 2017-02-05 316 08b25963 arch/x86/kernel/cpu/amd.c Yazen Ghannam 2017-02-05 317 if (smp_num_siblings > 1) 08b25963 arch/x86/kernel/cpu/amd.c Yazen Ghannam 2017-02-05 318 c->x86_max_cores /= smp_num_siblings; 08b25963 arch/x86/kernel/cpu/amd.c Yazen Ghannam 2017-02-05 319 } 08b25963 arch/x86/kernel/cpu/amd.c Yazen Ghannam 2017-02-05 320 b6a50cdd arch/x86/kernel/cpu/amd.c Yazen Ghannam 2016-11-08 321 /* b6a50cdd arch/x86/kernel/cpu/amd.c Yazen Ghannam 2016-11-08 322 * We may have multiple LLCs if L3 caches exist, so check if we b6a50cdd arch/x86/kernel/cpu/amd.c Yazen Ghannam 2016-11-08 323 * have an L3 cache by looking at the L3 cache CPUID leaf. b6a50cdd arch/x86/kernel/cpu/amd.c Yazen Ghannam 2016-11-08 324 */ b6a50cdd arch/x86/kernel/cpu/amd.c Yazen Ghannam 2016-11-08 325 if (cpuid_edx(0x8006)) { b6a50cdd arch/x86/kernel/cpu/amd.c Yazen Ghannam 2016-11-08 326 if (c->x86 == 0x17) { b6a50cdd arch/x86/kernel/cpu/amd.c Yazen Ghannam 2016-11-08 327 /* b6a50cdd arch/x86/kernel/cpu/amd.c Yazen Ghannam 2016-11-08 328 * LLC is at the core complex level. b6a50cdd arch/x86/kernel/cpu/amd.c Yazen Ghannam 2016-11-08 329 * Core complex id is ApicId[3]. b6a50cdd arch/x86/kernel/cpu/amd.c Yazen Ghannam 2016-11-08 330 */ b6a50cdd arch/x86/kernel/cpu/amd.c Yazen Ghannam 2016-11-08 331 per_cpu(cpu_llc_id, cpu) = c->apicid >> 3; b6a50cdd arch/x86/kernel/cpu/amd.c Yazen Ghannam 2016-11-08 332 } else { b6a50cdd arch/x86/kernel/cpu/amd.c Yazen Ghannam 2016-11-08 333 /* LLC is at the node level. */ 1c2d1f62 arch/x86/kernel/cpu/amd.c Suravee Suthikulpanit 2017-07-21 334 u8 node_id = ecx & 0xff; 1c2d1f62 arch/x86/kernel/cpu/amd.c Suravee Suthikulpanit 2017-07-21
Re: [PATCH v2 1/2] x86/amd: Refactor topology extension related code
On Fri, Jul 21, 2017 at 09:00:38PM -0500, Suravee Suthikulpanit wrote: > Refactoring in preparation for subsequent changes. > There is no functional change. > > Signed-off-by: Suravee Suthikulpanit > --- > arch/x86/kernel/cpu/amd.c | 79 > ++- > 1 file changed, 44 insertions(+), 35 deletions(-) > > diff --git a/arch/x86/kernel/cpu/amd.c b/arch/x86/kernel/cpu/amd.c > index bb5abe8..74d8d7c 100644 > --- a/arch/x86/kernel/cpu/amd.c > +++ b/arch/x86/kernel/cpu/amd.c > @@ -297,54 +297,63 @@ static int nearby_node(int apicid) > #endif > > /* > - * Fixup core topology information for > - * (1) AMD multi-node processors > - * Assumption: Number of cores in each internal node is the same. > - * (2) AMD processors supporting compute units > + * Get topology information via X86_FEATURE_TOPOEXT > */ > -#ifdef CONFIG_SMP > -static void amd_get_topology(struct cpuinfo_x86 *c) > +static void __get_topoext(struct cpuinfo_x86 *c) > { > - u8 node_id; > + u32 eax, ebx, ecx, edx; > int cpu = smp_processor_id(); > > - /* get information required for multi-node processors */ > - if (boot_cpu_has(X86_FEATURE_TOPOEXT)) { > - u32 eax, ebx, ecx, edx; > + cpuid(0x801e, &eax, &ebx, &ecx, &edx); > > - cpuid(0x801e, &eax, &ebx, &ecx, &edx); > + smp_num_siblings = ((ebx >> 8) & 0xff) + 1; > > - node_id = ecx & 0xff; When reviewers ask you about a preparatory cleanup patch, you don't sneak in changes in it - you *only* *move* the code so that the change is *absolutely* comprehensible. Ontop you do changes. Don't tell me you didn't know that! Try again. -- Regards/Gruss, Boris. SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nürnberg) --
[PATCH v2 1/2] x86/amd: Refactor topology extension related code
Refactoring in preparation for subsequent changes. There is no functional change. Signed-off-by: Suravee Suthikulpanit --- arch/x86/kernel/cpu/amd.c | 79 ++- 1 file changed, 44 insertions(+), 35 deletions(-) diff --git a/arch/x86/kernel/cpu/amd.c b/arch/x86/kernel/cpu/amd.c index bb5abe8..74d8d7c 100644 --- a/arch/x86/kernel/cpu/amd.c +++ b/arch/x86/kernel/cpu/amd.c @@ -297,54 +297,63 @@ static int nearby_node(int apicid) #endif /* - * Fixup core topology information for - * (1) AMD multi-node processors - * Assumption: Number of cores in each internal node is the same. - * (2) AMD processors supporting compute units + * Get topology information via X86_FEATURE_TOPOEXT */ -#ifdef CONFIG_SMP -static void amd_get_topology(struct cpuinfo_x86 *c) +static void __get_topoext(struct cpuinfo_x86 *c) { - u8 node_id; + u32 eax, ebx, ecx, edx; int cpu = smp_processor_id(); - /* get information required for multi-node processors */ - if (boot_cpu_has(X86_FEATURE_TOPOEXT)) { - u32 eax, ebx, ecx, edx; + cpuid(0x801e, &eax, &ebx, &ecx, &edx); - cpuid(0x801e, &eax, &ebx, &ecx, &edx); + smp_num_siblings = ((ebx >> 8) & 0xff) + 1; - node_id = ecx & 0xff; - smp_num_siblings = ((ebx >> 8) & 0xff) + 1; + if (c->x86 == 0x15) + c->cu_id = ebx & 0xff; - if (c->x86 == 0x15) - c->cu_id = ebx & 0xff; + if (c->x86 >= 0x17) { + c->cpu_core_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; + } - if (smp_num_siblings > 1) - c->x86_max_cores /= smp_num_siblings; - } + /* +* 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(0x8006)) { + if (c->x86 == 0x17) { + /* +* LLC is at the core complex level. +* Core complex id is ApicId[3]. +*/ + per_cpu(cpu_llc_id, cpu) = c->apicid >> 3; + } else { + /* LLC is at the node level. */ + u8 node_id = ecx & 0xff; - /* -* 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(0x8006)) { - if (c->x86 == 0x17) { - /* -* LLC is at the core complex level. -* Core complex id is ApicId[3]. -*/ - per_cpu(cpu_llc_id, cpu) = c->apicid >> 3; - } else { - /* LLC is at the node level. */ - per_cpu(cpu_llc_id, cpu) = node_id; - } + per_cpu(cpu_llc_id, cpu) = node_id; } + } +} + +/* + * Fixup core topology information for + * (1) AMD multi-node processors + * Assumption: Number of cores in each internal node is the same. + * (2) AMD processors supporting compute units + */ +#ifdef CONFIG_SMP +static void amd_get_topology(struct cpuinfo_x86 *c) +{ + /* get information required for multi-node processors */ + if (boot_cpu_has(X86_FEATURE_TOPOEXT)) { + __get_topoext(c); } else if (cpu_has(c, X86_FEATURE_NODEID_MSR)) { + u8 node_id; u64 value; + int cpu = smp_processor_id(); rdmsrl(MSR_FAM10H_NODE_ID, value); node_id = value & 7; -- 2.7.4