<div dir="ltr"> <div class="gmail_extra"> <div class="gmail_quote">On Fri, Feb 10, 2017 at 3:22 AM, John Ferlan <<a href="mailto:jferlan@redhat.com" target="_blank">jferlan@redhat.com</a>> wrote: <blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> 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 <<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> | 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/<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 1857168..50a6bdb 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> > @@ -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? </blockquote><div> maj+min is not always exactly the same as page faults. Sometimes there is a small offset </div><div>value. </div><div> Eg: perf record -a --event={page-faults,major-faults,minor-faults} 47 page-faults 0 major-faults 46 minor-faults </div><div>Offset by 1 </div><div>Eg: virsh domstats --perf Domain: 'Fedora' perf.page_faults=890 perf.page_faults_min=890 perf.page_faults_maj=0 </div><div>Here maj+min=page_faults </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> Thus are all necessary? </blockquote><div>I am not sure on this part. Probably yes as we dont want </div><div>the user to add min+maj to get (approx)total page faults. </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> > </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 <div><div class="gmail-h5"> > </description> > </change> > <change> > diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng > 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 page_faults_min > * perf event. > + * "perf.page_faults_maj" - The count of major page faults as unsigned long > + * long. It is produced by the page_faults_maj > + * 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 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, where > + the page was not present in the page cache, and therefore > + had to be fetched from storage, by applications > + running on the platform </div></div>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... </blockquote><div>Okay. Lets have a longer description in API and terse decription in virsh output. </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> John <div class="gmail-HOEnZb"><div class="gmail-h5">> > B<Note>: The statistics can be retrieved using the B<domstats> command using > the I<--perf> flag. > </div></div></blockquote><div> </div><div>Thanks for all the review comments. </div></div> </div></div>