On Fri, Feb 10, 2017 at 3:10 AM, John Ferlan <jfer...@redhat.com> wrote:
>
>
> On 01/27/2017 06:01 AM, Nitesh Konkar wrote:
> > This patch adds support and documentation for
> > the cpu_clock perf event.
> >
> > Signed-off-by: Nitesh Konkar <nitko...@linux.vnet.ibm.com>
> > ---
> > docs/formatdomain.html.in | 6 ++++++
> > docs/news.xml | 4 ++--
> > docs/schemas/domaincommon.rng | 1 +
> > include/libvirt/libvirt-domain.h | 10 ++++++++++
> > src/libvirt-domain.c | 2 ++
> > src/qemu/qemu_driver.c | 1 +
> > src/util/virperf.c | 6 +++++-
> > src/util/virperf.h | 1 +
> > tests/genericxml2xmlindata/generic-perf.xml | 1 +
> > tools/virsh.pod | 3 +++
> > 10 files changed, 32 insertions(+), 3 deletions(-)
> >
> > diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in
> > index 3f7f875..e44e758 100644
> > --- a/docs/formatdomain.html.in
> > +++ b/docs/formatdomain.html.in
> > @@ -1937,6 +1937,7 @@
> > <event name='stalled_cycles_frontend' enabled='no'/>
> > <event name='stalled_cycles_backend' enabled='no'/>
> > <event name='ref_cpu_cycles' enabled='no'/>
> > + <event name='cpu_clock' enabled='no'/>
> > </perf>
> > ...
> > </pre>
> > @@ -2015,6 +2016,11 @@
> > by applications running on the platform</td>
> > <td><code>perf.ref_cpu_cycles</code></td>
> > </tr>
> > + <tr>
> > + <td><code>cpu_clock</code></td>
> > + <td>the count of cpu clock by applications running on the
> platform</td>
> > + <td><code>perf.cpu_clock</code></td>
> > + </tr>
> > </table>
>
> "the count of cpu clock by applications..."
>
> doesn't convey enough meaning. Is that cycles? Time? something else?
>
> The virsh.pod description seems to give the best hint...
>
> Some subsequent patches had longer descriptions in the virsh.pod, which
> got me thinking - where is the best place to have the longer
> description. I would think either in formatdomain or the
> libvirt-domain.{c|} description. At least that way you don't need to
> mess with the 80 column formatting that is "nice" to have in virsh.pod
> output
>
Lets keep the virsh.pod and libvirt-domain.c explanantion terse and let
formatdomain have detailed explanation of the events. Do you agree?
>
>
> >
> > <h3><a name="elementsDevices">Devices</a></h3>
> > diff --git a/docs/news.xml b/docs/news.xml
> > index ef7e2db..ec113ab 100644
> > --- a/docs/news.xml
> > +++ b/docs/news.xml
> > @@ -136,8 +136,8 @@
> > <description>
> > Add support to get the count of branch instructions
> > executed, branch misses, bus cycles, stalled frontend
> > - cpu cycles, stalled backend cpu cycles, and ref cpu
> > - cycles by applications running on the platform.
> > + cpu cycles, stalled backend cpu cycles, ref cpu cycles
> > + and cpu clock by applications running on the platform.
> > </description>
> > </change>
> > <change>
>
> So the latest decree is to not integrate news.xml into the patches with
> the code. So just make one patch at the end that summarizes all the
> adjustments.
>
> Also this patch is wrong because it describes the additions in the 3.0.0
> section while this code would be available in 3.1.0.
>
Noted.
>
> > diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.
> rng
> > index 4d76315..86a39c8 100644
> > --- a/docs/schemas/domaincommon.rng
> > +++ b/docs/schemas/domaincommon.rng
> > @@ -433,6 +433,7 @@
> > <value>stalled_cycles_frontend</value>
> > <value>stalled_cycles_backend</value>
> > <value>ref_cpu_cycles</value>
> > + <value>cpu_clock</value>
> > </choice>
> > </attribute>
> > <attribute name="enabled">
> > diff --git a/include/libvirt/libvirt-domain.h b/include/libvirt/libvirt-
> domain.h
> > index e303140..bffe955 100644
> > --- a/include/libvirt/libvirt-domain.h
> > +++ b/include/libvirt/libvirt-domain.h
> > @@ -2188,6 +2188,16 @@ void
> > virDomainStatsRecordListFree(virDomainStatsRecordPtr
> *stats);
> > */
> > # define VIR_PERF_PARAM_REF_CPU_CYCLES "ref_cpu_cycles"
> >
> > +/**
> > + * VIR_PERF_PARAM_CPU_CLOCK:
> > + *
> > + * Macro for typed parameter name that represents cpu_clock
> > + * perf event which can be used to measure the count of cpu
> > + * clock by applications running on the platform. It corresponds
> > + * to the "perf.cpu_clock" field in the *Stats APIs.
> > + */
> > +# define VIR_PERF_PARAM_CPU_CLOCK "cpu_clock"
> > +
> > int virDomainGetPerfEvents(virDomainPtr dom,
> > virTypedParameterPtr *params,
> > int *nparams,
> > diff --git a/src/libvirt-domain.c b/src/libvirt-domain.c
> > index 5b3e842..001cdd7 100644
> > --- a/src/libvirt-domain.c
> > +++ b/src/libvirt-domain.c
> > @@ -11250,6 +11250,8 @@ virConnectGetDomainCapabilities(virConnectPtr
> conn,
> > * CPU frequency scaling by applications
> running
> > * as unsigned long long. It is produced by
> the
> > * ref_cpu_cycles perf event.
> > + * "perf.cpu_clock" - The count of cpu clock as unsigned long long.
> It is
> > + * produced by the cpu_clock perf event.
> > *
> > * Note that entire stats groups or individual stat fields may be
> missing from
> > * the output in case they are not supported by the given hypervisor,
> are not
> > diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> > index 516a851..ff4803c 100644
> > --- a/src/qemu/qemu_driver.c
> > +++ b/src/qemu/qemu_driver.c
> > @@ -9549,6 +9549,7 @@ qemuDomainSetPerfEvents(virDomainPtr dom,
> > VIR_PERF_PARAM_STALLED_CYCLES_FRONTEND,
> VIR_TYPED_PARAM_BOOLEAN,
> > VIR_PERF_PARAM_STALLED_CYCLES_BACKEND,
> VIR_TYPED_PARAM_BOOLEAN,
> > VIR_PERF_PARAM_REF_CPU_CYCLES,
> VIR_TYPED_PARAM_BOOLEAN,
> > + VIR_PERF_PARAM_CPU_CLOCK,
> VIR_TYPED_PARAM_BOOLEAN,
> > NULL) < 0)
> > return -1;
> >
> > diff --git a/src/util/virperf.c b/src/util/virperf.c
> > index f64692b..3629e5b 100644
> > --- a/src/util/virperf.c
> > +++ b/src/util/virperf.c
> > @@ -43,7 +43,8 @@ VIR_ENUM_IMPL(virPerfEvent, VIR_PERF_EVENT_LAST,
> > "cache_references", "cache_misses",
> > "branch_instructions", "branch_misses",
> > "bus_cycles", "stalled_cycles_frontend",
> > - "stalled_cycles_backend", "ref_cpu_cycles");
> > + "stalled_cycles_backend", "ref_cpu_cycles",
> > + "cpu_clock");
> >
> > struct virPerfEvent {
> > int type;
> > @@ -112,6 +113,9 @@ static struct virPerfEventAttr attrs[] = {
> > .attrConfig = 0,
> > # endif
> > },
> > + {.type = VIR_PERF_EVENT_CPU_CLOCK,
> > + .attrType = PERF_TYPE_SOFTWARE,
> > + .attrConfig = PERF_COUNT_SW_CPU_CLOCK},
> > };
> > typedef struct virPerfEventAttr *virPerfEventAttrPtr;
> >
> > diff --git a/src/util/virperf.h b/src/util/virperf.h
> > index 1f43c92..81f1e20 100644
> > --- a/src/util/virperf.h
> > +++ b/src/util/virperf.h
> > @@ -47,6 +47,7 @@ typedef enum {
> > the backend of the
> instruction
> > processor pipeline */
> > VIR_PERF_EVENT_REF_CPU_CYCLES, /* Count of ref cpu cycles */
> > + VIR_PERF_EVENT_CPU_CLOCK, /* Count of cpu clock */
> >
> > VIR_PERF_EVENT_LAST
> > } virPerfEventType;
> > diff --git a/tests/genericxml2xmlindata/generic-perf.xml
> b/tests/genericxml2xmlindata/generic-perf.xml
> > index 437cd65..3e7834e 100644
> > --- a/tests/genericxml2xmlindata/generic-perf.xml
> > +++ b/tests/genericxml2xmlindata/generic-perf.xml
> > @@ -26,6 +26,7 @@
> > <event name='stalled_cycles_frontend' enabled='yes'/>
> > <event name='stalled_cycles_backend' enabled='yes'/>
> > <event name='ref_cpu_cycles' enabled='yes'/>
> > + <event name='cpu_clock' enabled='yes'/>
> > </perf>
> > <devices>
> > </devices>
> > diff --git a/tools/virsh.pod b/tools/virsh.pod
> > index 290f508..2f0bf36 100644
> > --- a/tools/virsh.pod
> > +++ b/tools/virsh.pod
> > @@ -946,6 +946,7 @@ I<--perf> returns the statistics of all enabled perf
> events:
> > "perf.stalled_cycles_frontend" - the count of stalled frontend cpu
> cycles,
> > "perf.stalled_cycles_backend" - the count of stalled backend cpu cycles,
> > "perf.ref_cpu_cycles" - the count of ref cpu cycles
> > +"perf.cpu_clock" - the count of cpu clock
>
>
> If you do a 'man tools/virsh.1' using an 80 column wide terminal, you'll
> note that starting with perf.stalled_cycles_frontend things go a bit
> haywire.
>
> I sent a patch to "fix" the existing format issues and make things a bit
> more consistent in this space.
>
Noted.
>
> If that's ACK'd then this will be affected. Still the text here still
> doesn't convey enough meaning.
>
> >
> > See the B<perf> command for more details about each event.
> >
> > @@ -2310,6 +2311,8 @@ B<Valid perf event names>
> > ref_cpu_cycles - Provides the count of total cpu cycles
> > not affected by CPU frequency scaling by
> > applications running on the platform.
> > + cpu_clock - Provides the cpu clock time consumed by applications
> > + running on the platform.
>
> Now this is a little bit better w/r/t meaning, but needs to be fit in
> else where as well.
>
Sure.
>
> John
> >
> > B<Note>: The statistics can be retrieved using the B<domstats> command
> using
> > the I<--perf> flag.
> >
>
Thanks.
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
