On Tue, 23 Jul 2024 at 15:17, Paolo Bonzini <pbonz...@redhat.com> wrote:
>
> From: Anthony Harivel <ahari...@redhat.com>
>
> Starting with the "Sandy Bridge" generation, Intel CPUs provide a RAPL
> interface (Running Average Power Limit) for advertising the accumulated
> energy consumption of various power domains (e.g. CPU packages, DRAM,
> etc.).
>
> The consumption is reported via MSRs (model specific registers) like
> MSR_PKG_ENERGY_STATUS for the CPU package power domain. These MSRs are
> 64 bits registers that represent the accumulated energy consumption in
> micro Joules. They are updated by microcode every ~1ms.
Hi; Coverity points out a couple of issues with this commit
(details below).
> +static void *kvm_msr_energy_thread(void *data)
> +{
> + KVMState *s = data;
> + struct KVMMsrEnergy *vmsr = &s->msr_energy;
> +
> + g_autofree vmsr_package_energy_stat *pkg_stat = NULL;
> + g_autofree vmsr_thread_stat *thd_stat = NULL;
> + g_autofree CPUState *cpu = NULL;
> + g_autofree unsigned int *vpkgs_energy_stat = NULL;
> + unsigned int num_threads = 0;
> +
> + X86CPUTopoIDs topo_ids;
> +
> + rcu_register_thread();
> +
> + /* Allocate memory for each package energy status */
> + pkg_stat = g_new0(vmsr_package_energy_stat, vmsr->host_topo.maxpkgs);
> +
> + /* Allocate memory for thread stats */
> + thd_stat = g_new0(vmsr_thread_stat, 1);
> +
> + /* Allocate memory for holding virtual package energy counter */
> + vpkgs_energy_stat = g_new0(unsigned int, vmsr->guest_vsockets);
> +
> + /* Populate the max tick of each packages */
> + for (int i = 0; i < vmsr->host_topo.maxpkgs; i++) {
> + /*
> + * Max numbers of ticks per package
> + * Time in second * Number of ticks/second * Number of cores/package
> + * ex: 100 ticks/second/CPU, 12 CPUs per Package gives 1200 ticks max
> + */
> + vmsr->host_topo.maxticks[i] = (MSR_ENERGY_THREAD_SLEEP_US / 1000000)
> + * sysconf(_SC_CLK_TCK)
> + * vmsr->host_topo.pkg_cpu_count[i];
> + }
> +
> + while (true) {
> + /* Get all qemu threads id */
> + g_autofree pid_t *thread_ids =
> + thread_ids = vmsr_get_thread_ids(vmsr->pid, &num_threads);
Here you are assigning the same value to the thread_ids variable
twice. What was the intention here ? (CID 1558553)
> +
> + if (thread_ids == NULL) {
> + goto clean;
> + }
> +/* Read the scheduled time for a given thread of a give pid */
> +void vmsr_read_thread_stat(pid_t pid,
> + unsigned int thread_id,
> + unsigned long long *utime,
> + unsigned long long *stime,
> + unsigned int *cpu_id)
> +{
> + g_autofree char *path = NULL;
> + g_autofree char *path_name = NULL;
> +
> + path_name = g_strdup_printf("/proc/%u/task/%d/stat", pid, thread_id);
> +
> + path = g_build_filename(path_name, NULL);
> +
> + FILE *file = fopen(path, "r");
> + if (file == NULL) {
> + pid = -1;
> + return;
> + }
> +
> + if (fscanf(file, "%*d (%*[^)]) %*c %*d %*d %*d %*d %*d %*u %*u %*u %*u
> %*u"
> + " %llu %llu %*d %*d %*d %*d %*d %*d %*u %*u %*d %*u %*u"
> + " %*u %*u %*u %*u %*u %*u %*u %*u %*u %*d %*u %*u %u",
> + utime, stime, cpu_id) != 3)
> + {
> + pid = -1;
> + return;
In this error-exit path we leak 'file', because we opened it but
didn't close it. (CID 1558557)
Also, both here and in the previous error-exit check we set pid = -1,
which is pointless because it's a function-local variable and nothing
is going to see the new value.
> + }
> +
> + fclose(file);
> + return;
> +}
thanks
-- PMM
