Re: [PATCH v3 3/3] Add support for RAPL MSRs in KVM/Qemu

["Anthony Harivel" <aharivel@redhat.com>]

Hi Daniel,

> > +
> > +        /* Retrieve all packages power plane energy counter */
> > +        for (int i = 0; i <= maxpkgs; i++) {
> > +            for (int j = 0; j < num_threads; j++) {
> > +                /*
> > +                 * Use the first thread we found that ran on the CPU
> > +                 * of the package to read the packages energy counter
> > +                 */
>
> This says we're using a thread ID
>
> > +                if (thd_stat[j].numa_node_id == i) {
> > +                    pkg_stat[i].e_start =
> > +                    vmsr_read_msr(MSR_PKG_ENERGY_STATUS, i, pid,
>
> but here we're using a pid ID, which is the thread ID of the initial
> thread.
>
> > +                                  s->msr_energy.socket_path);
> > +                    break;
> > +                }
> > +            }
> > +        }
>
> This API design for vmsr_read_msr() is incredibly inefficient.
> We're making (maxpkgs * num_threads) calls to vmsr_read_msr(),
> and every one of those is opening and closing the socket.
>
> Why isn't QEMU opening the socket once and then sending all
> the requests over the same socket ?
>


The usage of pid here is a mistake, thanks for pointing this out.

However, I'm more sceptical about the fact that the loop is inefficient. 
The confusion could definitely be because of the poor variable naming, 
and I apologize about that.
Let me try to explain what it's supposed to do:
Imagine we are running on machine that has i packages. QEMU has 
j threads running on whichever packages. We need to get the current 
packages energy of each packages that are used by the QEMU threads. 
(could be all i packages, only 1, 2.. we don't know what we need yet) 
So it loops first on the packages "0", and look if any thread has run 
on this packages. 
If no, test the next thread. 
if yes, we need the value, we call the vmsr_read_msr() then break and 
now loop for the next package, i.e package "1". And this until all 
packages has been tested.

So in the end, we 'only' have maximum "maxpkgs" calls of vmsr_read_msr().

Hope that's ok and that clear up the confusion!

Regards,
Anthony