Re: [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  > wrote:
> 
> 
> 
> On 02/13/2017 01:49 AM, Nitesh Konkar wrote:
> >
> >
> >
> > On Fri, Feb 10, 2017 at 3:22 AM, John Ferlan  
> > >> 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  
>  >>
> > > ---
> > >  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
> > >  ...
> > >  
> > > @@ -2052,6 +2053,12 @@
> > >platform
> > >perf.page_faults_min
> > >  
> > > +
> > > +  page_faults_maj
> > > +  the count of major page faults by applications
> running on the
> > > +  platform
> > > +  perf.page_faults_maj
> > > +
> >
> > 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 

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

[Nitesh Konkar]

On Mon, Feb 13, 2017 at 8:14 PM, John Ferlan  wrote:

>
>
> On 02/13/2017 01:49 AM, Nitesh Konkar wrote:
> >
> >
> >
> > On Fri, Feb 10, 2017 at 3:22 AM, John Ferlan  > > 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 >
> > > ---
> > >  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 <
> http://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
> > >  ...
> > >  
> > > @@ -2052,6 +2053,12 @@
> > >platform
> > >perf.page_faults_min
> > >  
> > > +
> > > +  page_faults_maj
> > > +  the count of major page faults by applications running
> on the
> > > +  platform
> > > +  perf.page_faults_maj
> > > +
> >
> > 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
>
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

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

[John Ferlan]

On 02/13/2017 01:49 AM, Nitesh Konkar wrote:
> 
> 
> 
> On Fri, Feb 10, 2017 at 3:22 AM, John Ferlan  > 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  >
> > ---
> >  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
> >  ...
> >  
> > @@ -2052,6 +2053,12 @@
> >platform
> >perf.page_faults_min
> >  
> > +
> > +  page_faults_maj
> > +  the count of major page faults by applications running on the
> > +  platform
> > +  perf.page_faults_maj
> > +
> 
> 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.

John

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

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

[Nitesh Konkar]

On Fri, Feb 10, 2017 at 3:22 AM, John Ferlan  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 
> > ---
> >  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
> >  ...
> >  
> > @@ -2052,6 +2053,12 @@
> >platform
> >perf.page_faults_min
> >  
> > +
> > +  page_faults_maj
> > +  the count of major page faults by applications running on the
> > +  platform
> > +  perf.page_faults_maj
> > +
>
> 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.


>
> >
> >
> >  Devices
> > 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
>
> >  
> >
> >
> > 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 @@
> >context_switches
> >cpu_migrations
> >page_faults_min
> > +  page_faults_maj
> >  
> >
> >
> > 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
> > + *

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

[John Ferlan]

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 
> ---
>  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
>  ...
>  
> @@ -2052,6 +2053,12 @@
>platform
>perf.page_faults_min
>  
> +
> +  page_faults_maj
> +  the count of major page faults by applications running on the
> +  platform
> +  perf.page_faults_maj
> +

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

Thus are all necessary?

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

>  
>
>
> 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 @@
>context_switches
>cpu_migrations
>page_faults_min
> +  page_faults_maj
>  
>
>
> 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,
>