<div dir="ltr"> <div class="gmail_extra"> <div class="gmail_quote">On Fri, Feb 10, 2017 at 3:10 AM, John Ferlan <<a href="mailto:jferlan@redhat.com" target="_blank">jferlan@redhat.com</a>> wrote: <blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="HOEnZb"><div class="h5"> 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 <<a href="mailto:nitkon12@linux.vnet.ibm.com">nitkon12@linux.vnet.ibm.com</a>> > --- > docs/<a href="http://formatdomain.html.in" rel="noreferrer" target="_blank">formatdomain.html.in</a> | 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/<a href="http://formatdomain.html.in" rel="noreferrer" target="_blank">formatdomain.html.in</a> b/docs/<a href="http://formatdomain.html.in" rel="noreferrer" target="_blank">formatdomain.html.in</a> > index 3f7f875..e44e758 100644 > --- a/docs/<a href="http://formatdomain.html.in" rel="noreferrer" target="_blank">formatdomain.html.in</a> > +++ b/docs/<a href="http://formatdomain.html.in" rel="noreferrer" target="_blank">formatdomain.html.in</a> > @@ -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> </div></div>"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 </blockquote><div>Lets keep the virsh.pod and libvirt-domain.c explanantion terse and let formatdomain have detailed explanation of the events. Do you agree? </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> > > <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. </blockquote><div>Noted. </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> <div><div class="h5"> > 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 </div></div>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. </blockquote><div>Noted. </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> 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. </blockquote><div>Sure. </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> John <div class="HOEnZb"><div class="h5">> > B<Note>: The statistics can be retrieved using the B<domstats> command using > the I<--perf> flag. > </div></div></blockquote><div>Thanks. </div></div> </div></div>