On 02/16/2017 07:57 AM, Nitesh Konkar wrote: > > > On Mon, Feb 13, 2017 at 8:14 PM, John Ferlan <jferlan@xxxxxxxxxx > <mailto:jferlan@xxxxxxxxxx>> wrote: > > > > On 02/13/2017 01:49 AM, Nitesh Konkar wrote: > > > > > > > > On Fri, Feb 10, 2017 at 3:22 AM, John Ferlan <jferlan@xxxxxxxxxx <mailto:jferlan@xxxxxxxxxx> > > <mailto:jferlan@xxxxxxxxxx <mailto:jferlan@xxxxxxxxxx>>> 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@xxxxxxxxxxxxxxxxxx > <mailto:nitkon12@xxxxxxxxxxxxxxxxxx> > <mailto:nitkon12@xxxxxxxxxxxxxxxxxx > <mailto:nitkon12@xxxxxxxxxxxxxxxxxx>>> > > > --- > > > 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 -- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list
