[libvirt] [PATCH 7/9] perf: add page_faults_maj software perf event support

[John Ferlan]

On 02/16/2017 07:57 AM, Nitesh Konkar wrote:
> 
> 
> On Mon, Feb 13, 2017 at 8:14 PM, John Ferlan <jferlan at redhat.com
> <mailto:jferlan at 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 at redhat.com <mailto:jferlan at redhat.com>
>     > <mailto:jferlan at redhat.com <mailto:jferlan at 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 at linux.vnet.ibm.com
>     <mailto:nitkon12 at linux.vnet.ibm.com>
>     <mailto:nitkon12 at linux.vnet.ibm.com
>     <mailto:nitkon12 at linux.vnet.ibm.com>>>
>     >     > ---
>     >     >  docs/formatdomain.html.in <http://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> <http://formatdomain.html.in>
>     >     b/docs/formatdomain.html.in <http://formatdomain.html.in>
>     <http://formatdomain.html.in>
>     >     > index 1857168..50a6bdb 100644
>     >     > --- a/docs/formatdomain.html.in
>     <http://formatdomain.html.in> <http://formatdomain.html.in>
>     >     > +++ b/docs/formatdomain.html.in
>     <http://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.

Thanks for digging in and getting the answer... Something to document in
the description for the fields...

> 
> Also, shd i follow the comment pattern as shown
> in ur patch under review, in /virsh.pod ?

I think it would be better, but then again my patch hasn't yet been ACK'd...

John