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 <nitkon12@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

<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.

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. 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. John

B<Note>: The statistics can be retrieved using the B<domstats> command using the I<--perf> flag.

On 02/13/2017 03:01 AM, Nitesh Konkar wrote:

On Fri, Feb 10, 2017 at 3:10 AM, John Ferlan <jferlan@redhat.com <mailto:jferlan@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 <nitkon12@linux.vnet.ibm.com <mailto:nitkon12@linux.vnet.ibm.com>> > --- > docs/formatdomain.html.in <http://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 <http://formatdomain.html.in> b/docs/formatdomain.html.in <http://formatdomain.html.in> > index 3f7f875..e44e758 100644 > --- a/docs/formatdomain.html.in <http://formatdomain.html.in> > +++ b/docs/formatdomain.html.in <http://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?

I'm fine with that as long as the terse description can provide some amount of context. John

On 01/27/2017 06:01 AM, Nitesh Konkar wrote:

This patch adds support and documentation for the task_clock perf event.

Signed-off-by: Nitesh Konkar <nitkon12@linux.vnet.ibm.com> --- docs/formatdomain.html.in | 6 ++++++ docs/news.xml | 5 +++-- 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 | 5 ++++- 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 e44e758..1ed5db7 100644 --- a/docs/formatdomain.html.in +++ b/docs/formatdomain.html.in @@ -1938,6 +1938,7 @@ <event name='stalled_cycles_backend' enabled='no'/> <event name='ref_cpu_cycles' enabled='no'/> <event name='cpu_clock' enabled='no'/> + <event name='task_clock' enabled='no'/> </perf> ... </pre> @@ -2021,6 +2022,11 @@ <td>the count of cpu clock by applications running on the platform</td> <td><code>perf.cpu_clock</code></td> </tr> + <tr> + <td><code>task_clock</code></td> + <td>the count of task clock by applications running on the platform</td> + <td><code>perf.task_clock</code></td>

Similar to 1/9 - the count of task clock by applications..." doesn't convey enough meaning... Need a bit more description...

+ </tr> </table>

<h3><a name="elementsDevices">Devices</a></h3> diff --git a/docs/news.xml b/docs/news.xml index ec113ab..38373c0 100644 --- a/docs/news.xml +++ b/docs/news.xml @@ -136,8 +136,9 @@ <description> Add support to get the count of branch instructions executed, branch misses, bus cycles, stalled frontend - cpu cycles, stalled backend cpu cycles, ref cpu cycles - and cpu clock by applications running on the platform. + cpu cycles, stalled backend cpu cycles, ref cpu cycles, + cpu clock and task clock by applications running on the + platform.

same issue w/ news as last patch...

</description> </change> <change> diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng index 86a39c8..6ee2c6f 100644 --- a/docs/schemas/domaincommon.rng +++ b/docs/schemas/domaincommon.rng @@ -434,6 +434,7 @@ <value>stalled_cycles_backend</value> <value>ref_cpu_cycles</value> <value>cpu_clock</value> + <value>task_clock</value> </choice> </attribute> <attribute name="enabled"> diff --git a/include/libvirt/libvirt-domain.h b/include/libvirt/libvirt-domain.h index bffe955..f4f1c74 100644 --- a/include/libvirt/libvirt-domain.h +++ b/include/libvirt/libvirt-domain.h @@ -2198,6 +2198,16 @@ void virDomainStatsRecordListFree(virDomainStatsRecordPtr *stats); */ # define VIR_PERF_PARAM_CPU_CLOCK "cpu_clock"

+/** + * VIR_PERF_PARAM_TASK_CLOCK: + * + * Macro for typed parameter name that represents task_clock + * perf event which can be used to measure the count of task + * clock by applications running on the platform. It corresponds + * to the "perf.task_clock" field in the *Stats APIs. + */ +# define VIR_PERF_PARAM_TASK_CLOCK "task_clock" + int virDomainGetPerfEvents(virDomainPtr dom, virTypedParameterPtr *params, int *nparams, diff --git a/src/libvirt-domain.c b/src/libvirt-domain.c index 001cdd7..6875848 100644 --- a/src/libvirt-domain.c +++ b/src/libvirt-domain.c @@ -11252,6 +11252,8 @@ virConnectGetDomainCapabilities(virConnectPtr conn, * 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. + * "perf.task_clock" - The count of task clock as unsigned long long. It is + * produced by the task_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 ff4803c..285156d 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -9550,6 +9550,7 @@ qemuDomainSetPerfEvents(virDomainPtr dom, 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, + VIR_PERF_PARAM_TASK_CLOCK, VIR_TYPED_PARAM_BOOLEAN, NULL) < 0) return -1;

diff --git a/src/util/virperf.c b/src/util/virperf.c index 3629e5b..e19cfea 100644 --- a/src/util/virperf.c +++ b/src/util/virperf.c @@ -44,7 +44,7 @@ VIR_ENUM_IMPL(virPerfEvent, VIR_PERF_EVENT_LAST, "branch_instructions", "branch_misses", "bus_cycles", "stalled_cycles_frontend", "stalled_cycles_backend", "ref_cpu_cycles", - "cpu_clock"); + "cpu_clock", "task_clock");

struct virPerfEvent { int type; @@ -116,6 +116,9 @@ static struct virPerfEventAttr attrs[] = { {.type = VIR_PERF_EVENT_CPU_CLOCK, .attrType = PERF_TYPE_SOFTWARE, .attrConfig = PERF_COUNT_SW_CPU_CLOCK}, + {.type = VIR_PERF_EVENT_TASK_CLOCK, + .attrType = PERF_TYPE_SOFTWARE, + .attrConfig = PERF_COUNT_SW_TASK_CLOCK}

Need to add the "," after the } here not in next patch.

}; typedef struct virPerfEventAttr *virPerfEventAttrPtr;

diff --git a/src/util/virperf.h b/src/util/virperf.h index 81f1e20..b9e8a75 100644 --- a/src/util/virperf.h +++ b/src/util/virperf.h @@ -48,6 +48,7 @@ typedef enum { 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_TASK_CLOCK, /* Count of task clock */

VIR_PERF_EVENT_LAST } virPerfEventType; diff --git a/tests/genericxml2xmlindata/generic-perf.xml b/tests/genericxml2xmlindata/generic-perf.xml index 3e7834e..5ebc0a2 100644 --- a/tests/genericxml2xmlindata/generic-perf.xml +++ b/tests/genericxml2xmlindata/generic-perf.xml @@ -27,6 +27,7 @@ <event name='stalled_cycles_backend' enabled='yes'/> <event name='ref_cpu_cycles' enabled='yes'/> <event name='cpu_clock' enabled='yes'/> + <event name='task_clock' enabled='yes'/> </perf> <devices> </devices> diff --git a/tools/virsh.pod b/tools/virsh.pod index 2f0bf36..958e473 100644 --- a/tools/virsh.pod +++ b/tools/virsh.pod @@ -947,6 +947,7 @@ I<--perf> returns the statistics of all enabled perf events: "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 +"perf.task_clock" - the count of task clock

See the B<perf> command for more details about each event.

@@ -2313,6 +2314,8 @@ B<Valid perf event names> applications running on the platform. cpu_clock - Provides the cpu clock time consumed by applications running on the platform. + task_clock - Provides the task clock time consumed by applications + running on the platform

Again a bit more descriptive here which needs to be propagated elsewhere. John

B<Note>: The statistics can be retrieved using the B<domstats> command using the I<--perf> flag.

On 01/27/2017 06:01 AM, Nitesh Konkar wrote:

This patch adds support and documentation for the page_faults perf event.

Signed-off-by: Nitesh Konkar <nitkon12@linux.vnet.ibm.com> --- docs/formatdomain.html.in | 6 ++++++ docs/news.xml | 4 ++-- docs/schemas/domaincommon.rng | 1 + include/libvirt/libvirt-domain.h | 11 +++++++++++ src/libvirt-domain.c | 2 ++ src/qemu/qemu_driver.c | 1 + src/util/virperf.c | 7 +++++-- src/util/virperf.h | 1 + tests/genericxml2xmlindata/generic-perf.xml | 1 + tools/virsh.pod | 3 +++ 10 files changed, 33 insertions(+), 4 deletions(-)

diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in index 1ed5db7..2071eea 100644 --- a/docs/formatdomain.html.in +++ b/docs/formatdomain.html.in @@ -1939,6 +1939,7 @@ <event name='ref_cpu_cycles' enabled='no'/> <event name='cpu_clock' enabled='no'/> <event name='task_clock' enabled='no'/> + <event name='page_faults' enabled='no'/> </perf> ... </pre> @@ -2027,6 +2028,11 @@ <td>the count of task clock by applications running on the platform</td> <td><code>perf.task_clock</code></td> </tr> + <tr> + <td><code>page_faults</code></td> + <td>the count of page faults by applications running on the platform</td> + <td><code>perf.page_faults</code></td> + </tr>

Curious why "page_faults" and "page_faults_min" & "page_faults_maj" is "min+maj" equal to this value? A nit, I've seen wording like "page faults incurred" used too.. Just a bit more descriptive

</table>

<h3><a name="elementsDevices">Devices</a></h3> diff --git a/docs/news.xml b/docs/news.xml index 38373c0..14beed6 100644 --- a/docs/news.xml +++ b/docs/news.xml @@ -137,8 +137,8 @@ Add support to get the count of branch instructions executed, branch misses, bus cycles, stalled frontend cpu cycles, stalled backend cpu cycles, ref cpu cycles, - cpu clock and task clock by applications running on the - platform. + cpu clock, task clock and page faults by applications + running on the platform.

Similar with the news.xml to previous...

</description> </change> <change> diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng index 6ee2c6f..a7727e7 100644 --- a/docs/schemas/domaincommon.rng +++ b/docs/schemas/domaincommon.rng @@ -435,6 +435,7 @@ <value>ref_cpu_cycles</value> <value>cpu_clock</value> <value>task_clock</value> + <value>page_faults</value> </choice> </attribute> <attribute name="enabled"> diff --git a/include/libvirt/libvirt-domain.h b/include/libvirt/libvirt-domain.h index f4f1c74..36619b5 100644 --- a/include/libvirt/libvirt-domain.h +++ b/include/libvirt/libvirt-domain.h @@ -2208,6 +2208,17 @@ void virDomainStatsRecordListFree(virDomainStatsRecordPtr *stats); */ # define VIR_PERF_PARAM_TASK_CLOCK "task_clock"

+/** +* VIR_PERF_PARAM_PAGE_FAULTS: +* +* Macro for typed parameter name that represents page_faults +* perf event which can be used to measure the count of page +* faults by applications running on the platform. It corresponds +* to the "perf.page_faults" field in the *Stats APIs. +*/ +# define VIR_PERF_PARAM_PAGE_FAULTS "page_faults" + + int virDomainGetPerfEvents(virDomainPtr dom, virTypedParameterPtr *params, int *nparams, diff --git a/src/libvirt-domain.c b/src/libvirt-domain.c index 6875848..a923acc 100644 --- a/src/libvirt-domain.c +++ b/src/libvirt-domain.c @@ -11254,6 +11254,8 @@ virConnectGetDomainCapabilities(virConnectPtr conn, * produced by the cpu_clock perf event. * "perf.task_clock" - The count of task clock as unsigned long long. It is * produced by the task_clock perf event. + * "perf.page_faults" - The count of page faults as unsigned long long. It is + * produced by the page_faults 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 285156d..88f649e 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -9551,6 +9551,7 @@ qemuDomainSetPerfEvents(virDomainPtr dom, VIR_PERF_PARAM_REF_CPU_CYCLES, VIR_TYPED_PARAM_BOOLEAN, VIR_PERF_PARAM_CPU_CLOCK, VIR_TYPED_PARAM_BOOLEAN, VIR_PERF_PARAM_TASK_CLOCK, VIR_TYPED_PARAM_BOOLEAN, + VIR_PERF_PARAM_PAGE_FAULTS, VIR_TYPED_PARAM_BOOLEAN, NULL) < 0) return -1;

diff --git a/src/util/virperf.c b/src/util/virperf.c index e19cfea..3282aca 100644 --- a/src/util/virperf.c +++ b/src/util/virperf.c @@ -44,7 +44,7 @@ VIR_ENUM_IMPL(virPerfEvent, VIR_PERF_EVENT_LAST, "branch_instructions", "branch_misses", "bus_cycles", "stalled_cycles_frontend", "stalled_cycles_backend", "ref_cpu_cycles", - "cpu_clock", "task_clock"); + "cpu_clock", "task_clock", "page_faults");

struct virPerfEvent { int type; @@ -118,7 +118,10 @@ static struct virPerfEventAttr attrs[] = { .attrConfig = PERF_COUNT_SW_CPU_CLOCK}, {.type = VIR_PERF_EVENT_TASK_CLOCK, .attrType = PERF_TYPE_SOFTWARE, - .attrConfig = PERF_COUNT_SW_TASK_CLOCK} + .attrConfig = PERF_COUNT_SW_TASK_CLOCK}, + {.type = VIR_PERF_EVENT_PAGE_FAULTS, + .attrType = PERF_TYPE_SOFTWARE, + .attrConfig = PERF_COUNT_SW_PAGE_FAULTS}, }; typedef struct virPerfEventAttr *virPerfEventAttrPtr;

diff --git a/src/util/virperf.h b/src/util/virperf.h index b9e8a75..fbc5ae3 100644 --- a/src/util/virperf.h +++ b/src/util/virperf.h @@ -49,6 +49,7 @@ typedef enum { VIR_PERF_EVENT_REF_CPU_CYCLES, /* Count of ref cpu cycles */ VIR_PERF_EVENT_CPU_CLOCK, /* Count of cpu clock */ VIR_PERF_EVENT_TASK_CLOCK, /* Count of task clock */ + VIR_PERF_EVENT_PAGE_FAULTS, /* Count of page faults */

VIR_PERF_EVENT_LAST } virPerfEventType; diff --git a/tests/genericxml2xmlindata/generic-perf.xml b/tests/genericxml2xmlindata/generic-perf.xml index 5ebc0a2..3f27887 100644 --- a/tests/genericxml2xmlindata/generic-perf.xml +++ b/tests/genericxml2xmlindata/generic-perf.xml @@ -28,6 +28,7 @@ <event name='ref_cpu_cycles' enabled='yes'/> <event name='cpu_clock' enabled='yes'/> <event name='task_clock' enabled='yes'/> + <event name='page_faults' enabled='yes'/> </perf> <devices> </devices> diff --git a/tools/virsh.pod b/tools/virsh.pod index 958e473..f7be74e 100644 --- a/tools/virsh.pod +++ b/tools/virsh.pod @@ -948,6 +948,7 @@ I<--perf> returns the statistics of all enabled perf events: "perf.ref_cpu_cycles" - the count of ref cpu cycles "perf.cpu_clock" - the count of cpu clock "perf.task_clock" - the count of task clock +"perf.page_faults" - the count of page faults

See the B<perf> command for more details about each event.

@@ -2316,6 +2317,8 @@ B<Valid perf event names> running on the platform. task_clock - Provides the task clock time consumed by applications running on the platform + page_faults - Provides the count of page faults by applications + running on the platform

Interesting that the maj and min descriptions are quite wordy, but this one isn't... I'm assuming to a degree the count the same thing, right? John

B<Note>: The statistics can be retrieved using the B<domstats> command using the I<--perf> flag.

On 01/27/2017 06:01 AM, Nitesh Konkar wrote:

This patch adds support and documentation for the context_switches perf event.

Signed-off-by: Nitesh Konkar <nitkon12@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 | 3 +++ 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, 33 insertions(+), 3 deletions(-)

diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in index 2071eea..4ed6e02 100644 --- a/docs/formatdomain.html.in +++ b/docs/formatdomain.html.in @@ -1940,6 +1940,7 @@ <event name='cpu_clock' enabled='no'/> <event name='task_clock' enabled='no'/> <event name='page_faults' enabled='no'/> + <event name='context_switches' enabled='no'/> </perf> ... </pre> @@ -2033,6 +2034,11 @@ <td>the count of page faults by applications running on the platform</td> <td><code>perf.page_faults</code></td> </tr> + <tr> + <td><code>context_switches</code></td> + <td>the count of context switches by applications running on the platform</td> + <td><code>perf.context_switches</code></td> + </tr> </table>

<h3><a name="elementsDevices">Devices</a></h3> diff --git a/docs/news.xml b/docs/news.xml index 14beed6..fb872f5 100644 --- a/docs/news.xml +++ b/docs/news.xml @@ -137,8 +137,8 @@ Add support to get the count of branch instructions executed, branch misses, bus cycles, stalled frontend cpu cycles, stalled backend cpu cycles, ref cpu cycles, - cpu clock, task clock and page faults by applications - running on the platform. + cpu clock, task clock, page faults and context switches + by applications running on the platform.

Remove news.xml from this patch... like previous...

</description> </change> <change> diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng index a7727e7..e4e1358 100644 --- a/docs/schemas/domaincommon.rng +++ b/docs/schemas/domaincommon.rng @@ -436,6 +436,7 @@ <value>cpu_clock</value> <value>task_clock</value> <value>page_faults</value> + <value>context_switches</value> </choice> </attribute> <attribute name="enabled"> diff --git a/include/libvirt/libvirt-domain.h b/include/libvirt/libvirt-domain.h index 36619b5..e71bfa7 100644 --- a/include/libvirt/libvirt-domain.h +++ b/include/libvirt/libvirt-domain.h @@ -2218,6 +2218,16 @@ void virDomainStatsRecordListFree(virDomainStatsRecordPtr *stats); */ # define VIR_PERF_PARAM_PAGE_FAULTS "page_faults"

+/** + * VIR_PERF_PARAM_CONTEXT_SWITCHES: + * + * Macro for typed parameter name that represents context_switches + * perf event which can be used to measure the count of context + * switches by applications running on the platform. It corresponds + * to the "perf.context_switches" field in the *Stats APIs. + */ +# define VIR_PERF_PARAM_CONTEXT_SWITCHES "context_switches" +

int virDomainGetPerfEvents(virDomainPtr dom, virTypedParameterPtr *params, diff --git a/src/libvirt-domain.c b/src/libvirt-domain.c index a923acc..42720f1 100644 --- a/src/libvirt-domain.c +++ b/src/libvirt-domain.c @@ -11256,6 +11256,9 @@ virConnectGetDomainCapabilities(virConnectPtr conn, * produced by the task_clock perf event. * "perf.page_faults" - The count of page faults as unsigned long long. It is * produced by the page_faults perf event. + * "perf.context_switches" - The count of context switches as unsigned long + * long. It is produced by the context_switches + * 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 88f649e..c579b61 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -9552,6 +9552,7 @@ qemuDomainSetPerfEvents(virDomainPtr dom, VIR_PERF_PARAM_CPU_CLOCK, VIR_TYPED_PARAM_BOOLEAN, VIR_PERF_PARAM_TASK_CLOCK, VIR_TYPED_PARAM_BOOLEAN, VIR_PERF_PARAM_PAGE_FAULTS, VIR_TYPED_PARAM_BOOLEAN, + VIR_PERF_PARAM_CONTEXT_SWITCHES, VIR_TYPED_PARAM_BOOLEAN, NULL) < 0) return -1;

diff --git a/src/util/virperf.c b/src/util/virperf.c index 3282aca..c004061 100644 --- a/src/util/virperf.c +++ b/src/util/virperf.c @@ -44,7 +44,8 @@ VIR_ENUM_IMPL(virPerfEvent, VIR_PERF_EVENT_LAST, "branch_instructions", "branch_misses", "bus_cycles", "stalled_cycles_frontend", "stalled_cycles_backend", "ref_cpu_cycles", - "cpu_clock", "task_clock", "page_faults"); + "cpu_clock", "task_clock", "page_faults", + "context_switches");

struct virPerfEvent { int type; @@ -122,6 +123,9 @@ static struct virPerfEventAttr attrs[] = { {.type = VIR_PERF_EVENT_PAGE_FAULTS, .attrType = PERF_TYPE_SOFTWARE, .attrConfig = PERF_COUNT_SW_PAGE_FAULTS}, + {.type = VIR_PERF_EVENT_CONTEXT_SWITCHES, + .attrType = PERF_TYPE_SOFTWARE, + .attrConfig = PERF_COUNT_SW_CONTEXT_SWITCHES}, }; typedef struct virPerfEventAttr *virPerfEventAttrPtr;

diff --git a/src/util/virperf.h b/src/util/virperf.h index fbc5ae3..b937d9a 100644 --- a/src/util/virperf.h +++ b/src/util/virperf.h @@ -50,6 +50,7 @@ typedef enum { VIR_PERF_EVENT_CPU_CLOCK, /* Count of cpu clock */ VIR_PERF_EVENT_TASK_CLOCK, /* Count of task clock */ VIR_PERF_EVENT_PAGE_FAULTS, /* Count of page faults */ + VIR_PERF_EVENT_CONTEXT_SWITCHES, /* Count of context switches */

VIR_PERF_EVENT_LAST } virPerfEventType; diff --git a/tests/genericxml2xmlindata/generic-perf.xml b/tests/genericxml2xmlindata/generic-perf.xml index 3f27887..29a2273 100644 --- a/tests/genericxml2xmlindata/generic-perf.xml +++ b/tests/genericxml2xmlindata/generic-perf.xml @@ -29,6 +29,7 @@ <event name='cpu_clock' enabled='yes'/> <event name='task_clock' enabled='yes'/> <event name='page_faults' enabled='yes'/> + <event name='context_switches' enabled='yes'/> </perf> <devices> </devices> diff --git a/tools/virsh.pod b/tools/virsh.pod index f7be74e..d0bf016 100644 --- a/tools/virsh.pod +++ b/tools/virsh.pod @@ -949,6 +949,7 @@ I<--perf> returns the statistics of all enabled perf events: "perf.cpu_clock" - the count of cpu clock "perf.task_clock" - the count of task clock "perf.page_faults" - the count of page faults +"perf.context_switches" - the count of context switches

See the B<perf> command for more details about each event.

@@ -2319,6 +2320,8 @@ B<Valid perf event names> running on the platform page_faults - Provides the count of page faults by applications running on the platform + context_switches - Provides the count of context switches by applications + running on the platform

This description needs to have it's formatting fixed a bit - to not go past the 80 column windows.. After building use man tools/virsh.1 with an 80 column wide terminal. John

B<Note>: The statistics can be retrieved using the B<domstats> command using the I<--perf> flag.

On 01/27/2017 06:01 AM, Nitesh Konkar wrote:

This patch adds support and documentation for the cpu_migrations perf event.

Signed-off-by: Nitesh Konkar <nitkon12@linux.vnet.ibm.com> --- docs/formatdomain.html.in | 6 ++++++ docs/news.xml | 4 ++-- docs/schemas/domaincommon.rng | 1 + include/libvirt/libvirt-domain.h | 9 +++++++++ src/libvirt-domain.c | 5 ++++- src/qemu/qemu_driver.c | 1 + src/util/virperf.c | 5 ++++- src/util/virperf.h | 1 + tests/genericxml2xmlindata/generic-perf.xml | 1 + tools/virsh.pod | 4 ++++ 10 files changed, 33 insertions(+), 4 deletions(-)

diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in index 4ed6e02..f0efab1 100644 --- a/docs/formatdomain.html.in +++ b/docs/formatdomain.html.in @@ -1941,6 +1941,7 @@ <event name='task_clock' enabled='no'/> <event name='page_faults' enabled='no'/> <event name='context_switches' enabled='no'/> + <event name='cpu_migrations' enabled='no'/> </perf> ... </pre> @@ -2039,6 +2040,11 @@ <td>the count of context switches by applications running on the platform</td> <td><code>perf.context_switches</code></td> </tr> + <tr> + <td><code>cpu_migrations</code></td> + <td>the count of cpu migrations by applications running on the platform</td> + <td><code>perf.cpu_migrations</code></td> + </tr>

Not sure "count of cpu migrations by applictions" conveys enough meaning. Is this something applications can control? Is this one of those things where applications may want to "pin" to [a] specific cpu[s]?

</table>

<h3><a name="elementsDevices">Devices</a></h3> diff --git a/docs/news.xml b/docs/news.xml index fb872f5..0489c42 100644 --- a/docs/news.xml +++ b/docs/news.xml @@ -137,8 +137,8 @@ Add support to get the count of branch instructions executed, branch misses, bus cycles, stalled frontend cpu cycles, stalled backend cpu cycles, ref cpu cycles, - cpu clock, task clock, page faults and context switches - by applications running on the platform. + cpu clock, task clock, page faults, context switches and + cpu migrations by applications running on the platform.

Remove news.xml from here.

</description> </change> <change> diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng index e4e1358..5bd06cb 100644 --- a/docs/schemas/domaincommon.rng +++ b/docs/schemas/domaincommon.rng @@ -437,6 +437,7 @@ <value>task_clock</value> <value>page_faults</value> <value>context_switches</value> + <value>cpu_migrations</value> </choice> </attribute> <attribute name="enabled"> diff --git a/include/libvirt/libvirt-domain.h b/include/libvirt/libvirt-domain.h index e71bfa7..b4abdb0 100644 --- a/include/libvirt/libvirt-domain.h +++ b/include/libvirt/libvirt-domain.h @@ -2228,6 +2228,15 @@ void virDomainStatsRecordListFree(virDomainStatsRecordPtr *stats); */ # define VIR_PERF_PARAM_CONTEXT_SWITCHES "context_switches"

+/** + * VIR_PERF_PARAM_CPU_MIGRATIONS: + * + * Macro for typed parameter name that represents cpu_migrations + * perf event which can be used to measure the count of cpu + * migrations by applications running on the platform. It corresponds + * to the "perf.cpu_migrations" field in the *Stats APIs. + */ +# define VIR_PERF_PARAM_CPU_MIGRATIONS "cpu_migrations"

int virDomainGetPerfEvents(virDomainPtr dom, virTypedParameterPtr *params, diff --git a/src/libvirt-domain.c b/src/libvirt-domain.c index 42720f1..d063414 100644 --- a/src/libvirt-domain.c +++ b/src/libvirt-domain.c @@ -11255,10 +11255,13 @@ virConnectGetDomainCapabilities(virConnectPtr conn, * "perf.task_clock" - The count of task clock as unsigned long long. It is * produced by the task_clock perf event. * "perf.page_faults" - The count of page faults as unsigned long long. It is - * produced by the page_faults perf event. + * produced by the page_faults perf event. * "perf.context_switches" - The count of context switches as unsigned long * long. It is produced by the context_switches * perf event. + * "perf.cpu_migrations" - The count of cpu migrations as unsigned long + * long. It is produced by the cpu_migrations + * 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 c579b61..3fb7032 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -9553,6 +9553,7 @@ qemuDomainSetPerfEvents(virDomainPtr dom, VIR_PERF_PARAM_TASK_CLOCK, VIR_TYPED_PARAM_BOOLEAN, VIR_PERF_PARAM_PAGE_FAULTS, VIR_TYPED_PARAM_BOOLEAN, VIR_PERF_PARAM_CONTEXT_SWITCHES, VIR_TYPED_PARAM_BOOLEAN, + VIR_PERF_PARAM_CPU_MIGRATIONS, VIR_TYPED_PARAM_BOOLEAN, NULL) < 0) return -1;

diff --git a/src/util/virperf.c b/src/util/virperf.c index c004061..4559845 100644 --- a/src/util/virperf.c +++ b/src/util/virperf.c @@ -45,7 +45,7 @@ VIR_ENUM_IMPL(virPerfEvent, VIR_PERF_EVENT_LAST, "bus_cycles", "stalled_cycles_frontend", "stalled_cycles_backend", "ref_cpu_cycles", "cpu_clock", "task_clock", "page_faults", - "context_switches"); + "context_switches", "cpu_migrations");

struct virPerfEvent { int type; @@ -126,6 +126,9 @@ static struct virPerfEventAttr attrs[] = { {.type = VIR_PERF_EVENT_CONTEXT_SWITCHES, .attrType = PERF_TYPE_SOFTWARE, .attrConfig = PERF_COUNT_SW_CONTEXT_SWITCHES}, + {.type = VIR_PERF_EVENT_CPU_MIGRATIONS, + .attrType = PERF_TYPE_SOFTWARE, + .attrConfig = PERF_COUNT_SW_CPU_MIGRATIONS}, }; typedef struct virPerfEventAttr *virPerfEventAttrPtr;

diff --git a/src/util/virperf.h b/src/util/virperf.h index b937d9a..b769ebc 100644 --- a/src/util/virperf.h +++ b/src/util/virperf.h @@ -51,6 +51,7 @@ typedef enum { VIR_PERF_EVENT_TASK_CLOCK, /* Count of task clock */ VIR_PERF_EVENT_PAGE_FAULTS, /* Count of page faults */ VIR_PERF_EVENT_CONTEXT_SWITCHES, /* Count of context switches */ + VIR_PERF_EVENT_CPU_MIGRATIONS, /* Count of cpu migrations */

VIR_PERF_EVENT_LAST } virPerfEventType; diff --git a/tests/genericxml2xmlindata/generic-perf.xml b/tests/genericxml2xmlindata/generic-perf.xml index 29a2273..f856318 100644 --- a/tests/genericxml2xmlindata/generic-perf.xml +++ b/tests/genericxml2xmlindata/generic-perf.xml @@ -30,6 +30,7 @@ <event name='task_clock' enabled='yes'/> <event name='page_faults' enabled='yes'/> <event name='context_switches' enabled='yes'/> + <event name='cpu_migrations' enabled='yes'/> </perf> <devices> </devices> diff --git a/tools/virsh.pod b/tools/virsh.pod index d0bf016..a5e8ccd 100644 --- a/tools/virsh.pod +++ b/tools/virsh.pod @@ -950,6 +950,7 @@ I<--perf> returns the statistics of all enabled perf events: "perf.task_clock" - the count of task clock "perf.page_faults" - the count of page faults "perf.context_switches" - the count of context switches +"perf.cpu_migrations" - the count of cpu migrations

See the B<perf> command for more details about each event.

@@ -2322,6 +2323,9 @@ B<Valid perf event names> running on the platform context_switches - Provides the count of context switches by applications running on the platform + cpu_migrations - Provides the count of cpu migrations, that is, where the + process moved from one logical processor to another, by + applications running on the platform

and this too goes beyond the 80 column... John

B<Note>: The statistics can be retrieved using the B<domstats> command using the I<--perf> flag.

On Fri, Feb 10, 2017 at 3:22 AM, John Ferlan <jferlan@redhat.com> wrote:

On 01/27/2017 06:01 AM, Nitesh Konkar wrote:

...

This patch adds support and documentation for the page_faults_maj perf event.

Signed-off-by: Nitesh Konkar <nitkon12@linux.vnet.ibm.com> --- docs/formatdomain.html.in | 7 +++++++ docs/news.xml | 4 ++-- docs/schemas/domaincommon.rng | 1 + include/libvirt/libvirt-domain.h | 10 ++++++++++ src/libvirt-domain.c | 3 +++ src/qemu/qemu_driver.c | 1 + src/util/virperf.c | 5 ++++- src/util/virperf.h | 1 + tests/genericxml2xmlindata/generic-perf.xml | 1 + tools/virsh.pod | 5 +++++ 10 files changed, 35 insertions(+), 3 deletions(-)

NB: Similar comments from the page_faults_min...

...

diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in index 1857168..50a6bdb 100644 --- a/docs/formatdomain.html.in +++ b/docs/formatdomain.html.in @@ -1943,6 +1943,7 @@ <event name='context_switches' enabled='no'/> <event name='cpu_migrations' enabled='no'/> <event name='page_faults_min' enabled='no'/> + <event name='page_faults_maj' enabled='no'/> </perf> ... </pre> @@ -2052,6 +2053,12 @@ platform</td> <td><code>perf.page_faults_min</code></td> </tr> + <tr> + <td><code>page_faults_maj</code></td> + <td>the count of major page faults by applications running on the + platform</td> + <td><code>perf.page_faults_maj</code></td> + </tr>

As already noted in patch 3... is maj+min the same as what patch 3 provides?

maj+min is not always exactly the same as page faults. Sometimes there is a small offset value. Eg: perf record -a --event={page-faults,major-faults,minor-faults} 47 page-faults 0 major-faults 46 minor-faults Offset by 1 Eg: virsh domstats --perf Domain: 'Fedora' perf.page_faults=890 perf.page_faults_min=890 perf.page_faults_maj=0 Here maj+min=page_faults Thus are all necessary?

I am not sure on this part. Probably yes as we dont want the user to add min+maj to get (approx)total page faults.

...

</table>

<h3><a name="elementsDevices">Devices</a></h3> diff --git a/docs/news.xml b/docs/news.xml index 0e7c0d2..fe533f2 100644 --- a/docs/news.xml +++ b/docs/news.xml @@ -138,8 +138,8 @@ executed, branch misses, bus cycles, stalled frontend cpu cycles, stalled backend cpu cycles, ref cpu cycles, cpu clock, task clock, page faults, context switches, - cpu migrations and page faults min by applications - running on the platform. + cpu migrations, page faults min and page faults maj by + applications running on the platform.

Remove news.xml

...

</description> </change> <change> diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.

...

index f30ec0d..5f986d6 100644 --- a/docs/schemas/domaincommon.rng +++ b/docs/schemas/domaincommon.rng @@ -439,6 +439,7 @@ <value>context_switches</value> <value>cpu_migrations</value> <value>page_faults_min</value> + <value>page_faults_maj</value> </choice> </attribute> <attribute name="enabled"> diff --git a/include/libvirt/libvirt-domain.h b/include/libvirt/libvirt- domain.h index 108591a..d16200f 100644 --- a/include/libvirt/libvirt-domain.h +++ b/include/libvirt/libvirt-domain.h @@ -2248,6 +2248,16 @@ void virDomainStatsRecordListFree(virDomainStatsRecordPtr *stats); */ # define VIR_PERF_PARAM_PAGE_FAULTS_MIN "page_faults_min"

+/** + * VIR_PERF_PARAM_PAGE_FAULTS_MAJ: + * + * Macro for typed parameter name that represents page_faults_maj + * perf event which can be used to measure the count of major page + * faults by applications running on the platform. It corresponds + * to the "perf.page_faults_maj" field in the *Stats APIs. + */ +# define VIR_PERF_PARAM_PAGE_FAULTS_MAJ "page_faults_maj" + int virDomainGetPerfEvents(virDomainPtr dom, virTypedParameterPtr *params, int *nparams, diff --git a/src/libvirt-domain.c b/src/libvirt-domain.c index 1be385e..1d7c181 100644 --- a/src/libvirt-domain.c +++ b/src/libvirt-domain.c @@ -11265,6 +11265,9 @@ virConnectGetDomainCapabilities(virConnectPtr conn, * "perf.page_faults_min" - The count of minor page faults as unsigned long * long. It is produced by the

...

* perf event. + * "perf.page_faults_maj" - The count of major page faults as unsigned long + * long. It is produced by the

...

+ * 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 4f24fa6..c88bf5a 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -9555,6 +9555,7 @@ qemuDomainSetPerfEvents(virDomainPtr dom, VIR_PERF_PARAM_CONTEXT_SWITCHES, VIR_TYPED_PARAM_BOOLEAN, VIR_PERF_PARAM_CPU_MIGRATIONS, VIR_TYPED_PARAM_BOOLEAN, VIR_PERF_PARAM_PAGE_FAULTS_MIN, VIR_TYPED_PARAM_BOOLEAN, + VIR_PERF_PARAM_PAGE_FAULTS_MAJ, VIR_TYPED_PARAM_BOOLEAN, NULL) < 0) return -1;

diff --git a/src/util/virperf.c b/src/util/virperf.c index c839b04..37f61e2 100644 --- a/src/util/virperf.c +++ b/src/util/virperf.c @@ -46,7 +46,7 @@ VIR_ENUM_IMPL(virPerfEvent, VIR_PERF_EVENT_LAST, "stalled_cycles_backend", "ref_cpu_cycles", "cpu_clock", "task_clock", "page_faults", "context_switches", "cpu_migrations", - "page_faults_min"); + "page_faults_min", "page_faults_maj");

struct virPerfEvent { int type; @@ -133,6 +133,9 @@ static struct virPerfEventAttr attrs[] = { {.type = VIR_PERF_EVENT_PAGE_FAULTS_MIN, .attrType = PERF_TYPE_SOFTWARE, .attrConfig = PERF_COUNT_SW_PAGE_FAULTS_MIN}, + {.type = VIR_PERF_EVENT_PAGE_FAULTS_MAJ, + .attrType = PERF_TYPE_SOFTWARE, + .attrConfig = PERF_COUNT_SW_PAGE_FAULTS_MAJ}, }; typedef struct virPerfEventAttr *virPerfEventAttrPtr;

diff --git a/src/util/virperf.h b/src/util/virperf.h index aec352b..e99c870 100644 --- a/src/util/virperf.h +++ b/src/util/virperf.h @@ -53,6 +53,7 @@ typedef enum { VIR_PERF_EVENT_CONTEXT_SWITCHES, /* Count of context switches */ VIR_PERF_EVENT_CPU_MIGRATIONS, /* Count of cpu migrations */ VIR_PERF_EVENT_PAGE_FAULTS_MIN, /* Count of minor page faults */ + VIR_PERF_EVENT_PAGE_FAULTS_MAJ, /* Count of major page faults */

VIR_PERF_EVENT_LAST } virPerfEventType; diff --git a/tests/genericxml2xmlindata/generic-perf.xml b/tests/genericxml2xmlindata/generic-perf.xml index 62ad973..a5b6dfb 100644 --- a/tests/genericxml2xmlindata/generic-perf.xml +++ b/tests/genericxml2xmlindata/generic-perf.xml @@ -32,6 +32,7 @@ <event name='context_switches' enabled='yes'/> <event name='cpu_migrations' enabled='yes'/> <event name='page_faults_min' enabled='yes'/> + <event name='page_faults_maj' enabled='yes'/> </perf> <devices> </devices> diff --git a/tools/virsh.pod b/tools/virsh.pod index 4eb689e..d6bec4d 100644 --- a/tools/virsh.pod +++ b/tools/virsh.pod @@ -952,6 +952,7 @@ I<--perf> returns the statistics of all enabled perf events: "perf.context_switches" - the count of context switches "perf.cpu_migrations" - the count of cpu migrations "perf.page_faults_min" - the count of minor page faults +"perf.page_faults_maj" - the count of major page faults

See the B<perf> command for more details about each event.

@@ -2331,6 +2332,10 @@ B<Valid perf event names> the page was present in the page cache, and

rng page_faults_min page_faults_maj therefore the

...

fault avoided loading it from storage, by

applications

...

running on the platform + page_faults_maj - Provides the count of major page faults, that is,

...

+ the page was not present in the page cache, and

where therefore

...

+ had to be fetched from storage, by applications + running on the platform

The min/maj descriptions here are a bit more wordy. It does help, but I wonder now is this the wrong place. That is should the longer description be in the API so that the rendered API docs get a nice long description and the virsh output remains somewhat terse.

It really is up to you to decide, but I'm leaning towards better API doc descriptions...

Okay. Lets have a longer description in API and terse decription in virsh output.

John

...

B<Note>: The statistics can be retrieved using the B<domstats> command

using

...

the I<--perf> flag.

Thanks for all the review comments.

On Mon, Feb 13, 2017 at 8:14 PM, John Ferlan <jferlan@redhat.com> wrote:

On 02/13/2017 01:49 AM, Nitesh Konkar wrote:

...

On Fri, Feb 10, 2017 at 3:22 AM, John Ferlan <jferlan@redhat.com <mailto:jferlan@redhat.com>> wrote:

On 01/27/2017 06:01 AM, Nitesh Konkar wrote: > This patch adds support and documentation > for the page_faults_maj perf event. > > Signed-off-by: Nitesh Konkar <nitkon12@linux.vnet.ibm.com <mailto:

nitkon12@linux.vnet.ibm.com>>

...

> --- > docs/formatdomain.html.in <http://formatdomain.html.in> | 7 +++++++ > docs/news.xml | 4 ++-- > docs/schemas/domaincommon.rng | 1 + > include/libvirt/libvirt-domain.h | 10 ++++++++++ > src/libvirt-domain.c | 3 +++ > src/qemu/qemu_driver.c | 1 + > src/util/virperf.c | 5 ++++- > src/util/virperf.h | 1 + > tests/genericxml2xmlindata/generic-perf.xml | 1 + > tools/virsh.pod | 5 +++++ > 10 files changed, 35 insertions(+), 3 deletions(-) >

NB: Similar comments from the page_faults_min...

> diff --git a/docs/formatdomain.html.in <

http://formatdomain.html.in>

...

b/docs/formatdomain.html.in <http://formatdomain.html.in> > index 1857168..50a6bdb 100644 > --- a/docs/formatdomain.html.in <http://formatdomain.html.in> > +++ b/docs/formatdomain.html.in <http://formatdomain.html.in> > @@ -1943,6 +1943,7 @@ > <event name='context_switches' enabled='no'/> > <event name='cpu_migrations' enabled='no'/> > <event name='page_faults_min' enabled='no'/> > + <event name='page_faults_maj' enabled='no'/> > </perf> > ... > </pre> > @@ -2052,6 +2053,12 @@ > platform</td> > <td><code>perf.page_faults_min</code></td> > </tr> > + <tr> > + <td><code>page_faults_maj</code></td> > + <td>the count of major page faults by applications running

on the

...

> + platform</td> > + <td><code>perf.page_faults_maj</code></td> > + </tr>

As already noted in patch 3... is maj+min the same as what patch 3 provides?

maj+min is not always exactly the same as page faults. Sometimes there is a small offset value.

Eg: perf record -a --event={page-faults,major-faults,minor-faults} 47 page-faults

0 major-faults

46 minor-faults Offset by 1

Eg: virsh domstats --perf Domain: 'Fedora' perf.page_faults=890 perf.page_faults_min=890 perf.page_faults_maj=0 Here maj+min=page_faults

Thus are all necessary?

I am not sure on this part. Probably yes as we dont want the user to add min+maj to get (approx)total page faults.

I don't mind all 3 being present... still if I'm going to ask the question, then someone getting the stats will ask the question... they may also wonder why maj+min != total.

Perhaps something you could dig deeper on with the kernel code descriptions that are setting the value.

My assumption is it's the "time" of the sample. That is a total page fault could have been counted even though it hadn't been counted as a maj or min page fault.

I looked into the kernel code in /arch/x86/mm/fault.c and also confirmed from the #perf IRC that maj+min != total is valid. This is because not all page faults fall in maj/min category. Some maybe invalid page faults(invalid address generated) whereas some pagefaults after occuring are not serviced due to lock contention so as to avoid a deadlock at that instance, thus being counted in total but not in min/maj faults. Also, shd i follow the comment pattern as shown in ur patch under review, in /virsh.pod ? Thnx. John