Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1694169 > unrolled thread

Re: [PATCH v2 1/2] x86/amd: Refactor topology extension related code

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.

Contents

  Re: [PATCH v2 1/2] x86/amd: Refactor topology extension related code Borislav Petkov <bp@suse.de> - 2017-07-22 18:20 +0200
    Re: [PATCH v2 1/2] x86/amd: Refactor topology extension related code Borislav Petkov <bp@suse.de> - 2017-07-24 09:20 +0200

#1694169 — 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 <suravee.suthikulpanit@amd.com>
> ---
>  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(0x8000001e, &eax, &ebx, &ecx, &edx);
>  
> -		cpuid(0x8000001e, &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)
-- 

[toc] | [next] | [standalone]

#1694491

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(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;
+	}
+
+	/*
+	 * 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) {
+			/*
+			 * 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(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;
-		}
-
-		/*
-		 * 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) {
-				/*
-				 * 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)
-- 

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web