linux-kernel - Re: [PATCH 6/6] cpupower tools: Fix warning and a bug with the cpu package count

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-Id: <201208091207.36846.trenn@suse.de>
Date:	Thu, 9 Aug 2012 12:07:36 +0200
From:	Thomas Renninger <trenn@...e.de>
To:	Palmer Cox <p@...rcox.com>
Cc:	Dominik Brodowski <linux@...inikbrodowski.net>,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH 6/6] cpupower tools: Fix warning and a bug with the cpu package count

On Tuesday 07 August 2012 04:24:48 Palmer Cox wrote:
> The pkgs member of cpupower_topology is being used as the number of
> cpu packages. As the comment in get_cpu_topology notes, the package ids
> are not guaranteed to be contiguous. So, simply setting pkgs to the value
> of the highest physical_package_id doesn't actually provide a count of
> the number of cpu packages. Instead, calculate pkgs by setting it to
> the number of distinct physical_packge_id values which is pretty easy
> to do after the core_info structs are sorted. Calculating pkgs this
> way also has the nice benefit of getting rid of a sign comparison warning
> that GCC 4.6 was reporting.
> ---
>  tools/power/cpupower/utils/helpers/topology.c |   18 +++++++++++++-----
>  1 file changed, 13 insertions(+), 5 deletions(-)
> 
> diff --git a/tools/power/cpupower/utils/helpers/topology.c b/tools/power/cpupower/utils/helpers/topology.c
> index 4e2b583..fd3cc4d 100644
> --- a/tools/power/cpupower/utils/helpers/topology.c
> +++ b/tools/power/cpupower/utils/helpers/topology.c
> @@ -64,7 +64,7 @@ static int __compare(const void *t1, const void *t2)
>   */
>  int get_cpu_topology(struct cpupower_topology *cpu_top)
>  {
> -	int cpu, cpus = sysconf(_SC_NPROCESSORS_CONF);
> +	int cpu, last_pkg, cpus = sysconf(_SC_NPROCESSORS_CONF);
>  
>  	cpu_top->core_info = malloc(sizeof(struct cpuid_core_info) * cpus);
>  	if (cpu_top->core_info == NULL)
> @@ -78,20 +78,28 @@ int get_cpu_topology(struct cpupower_topology *cpu_top)
>  			"physical_package_id",
>  			&(cpu_top->core_info[cpu].pkg)) < 0)
>  			return -1;
> -		if ((int)cpu_top->core_info[cpu].pkg != -1 &&
> -		    cpu_top->core_info[cpu].pkg > cpu_top->pkgs)
> -			cpu_top->pkgs = cpu_top->core_info[cpu].pkg;
>  		if(sysfs_topology_read_file(
>  			cpu,
>  			"core_id",
>  			&(cpu_top->core_info[cpu].core)) < 0)
>  			return -1;
>  	}
> -	cpu_top->pkgs++;
>  
>  	qsort(cpu_top->core_info, cpus, sizeof(struct cpuid_core_info),
>  	      __compare);
>  
> +	/* Count the number of distinct pkgs values. This works
> +	   becuase the primary sort of of the core_info structs was just
becuase ... of of ... struct instead of structs

Otherwise the first 4 patches look like rather nice and straight forward
cleanups/fixes.
Feel free to add a Reviewed-by: Thomas Renninger <trenn@...e.de>

Let me have a closer look at patch 5 and 6. I had problems getting rid of
the compiler warning, looks like you found an easy way to clean this up.
I will be back and have time for this in the beginning of next week.

On which platforms (topology) did you test this?

   Thomas
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/