[PATCH 00/17] tools/kvm_stat: Misc Patches

[PATCH 00/17] tools/kvm_stat: Misc Patches

[Stefan Raspl]

Assortment of various fixes and improvements for the kvm_stat utility.
Patches 1-5 are bugfixes.
Patch 6 provides full PEP8 compliance by inserting the required empty
lines. It is included as an extra for easy disposal, since that kind of
patch might be a bit controversial.
There are some subtle aspects regarding patch 7, as its effects might
differ per platform. However, I didn't notice any negative effects, only
improvements.
Patch 8 is a simple doc update.
Patches 9-13 provide usability improvements.
Patches 14-17 add new features.

Signed-off-by: Stefan Raspl <raspl@linux.vnet.ibm.com>

---

Stefan Raspl (17):
  tools/kvm_stat: hide cursor
  tools/kvm_stat: catch curses exceptions only
  tools/kvm_stat: handle SIGINT in log and batch modes
  tools/kvm_stat: fix misc glitches
  tools/kvm_stat: fix trace setup glitch on field updates in
    TracepointProvider
  tools/kvm_stat: full PEP8 compliance
  tools/kvm_stat: reduce perceived idle time on filter updates
  tools/kvm_stat: document list of interactive commands
  tools/kvm_stat: display guest name when using pid filter
  tools/kvm_stat: remove pid filter on empty input
  tools/kvm_stat: print error messages on faulty pid filter input
  tools/kvm_stat: display regex when set to non-default
  tools/kvm_stat: remove regex filter on empty input
  tools/kvm_stat: add option '--guest'
  tools/kvm_stat: add interactive command 'c'
  tools/kvm_stat: add interactive command 'r'
  tools/kvm_stat: add '%Total' column

 tools/kvm/kvm_stat/kvm_stat     | 383 +++++++++++++++++++++++++++++++++-------
 tools/kvm/kvm_stat/kvm_stat.txt |  26 +++
 2 files changed, 343 insertions(+), 66 deletions(-)

[PATCH 01/17] tools/kvm_stat: hide cursor

[Stefan Raspl]

When running kvm_stat in interactive mode, the cursor appears at the lower
left corner, which looks a bit distracting.
This patch hides the cursor by turning it invisible.

Signed-off-by: Stefan Raspl <raspl@linux.vnet.ibm.com>
Reviewed-By: Sascha Silbe <silbe@linux.vnet.ibm.com>
Reviewed-by: Marc Hartmayer <mhartmay@linux.vnet.ibm.com>
---
 tools/kvm/kvm_stat/kvm_stat | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/tools/kvm/kvm_stat/kvm_stat b/tools/kvm/kvm_stat/kvm_stat
index 581278c..8cc83a30 100755
--- a/tools/kvm/kvm_stat/kvm_stat
+++ b/tools/kvm/kvm_stat/kvm_stat
@@ -812,6 +812,13 @@ class Tui(object):
         except:
             pass
 
+        # Hide cursor in extra statement as some monochrome terminals
+        # might support hiding but not colors.
+        try:
+            curses.curs_set(0)
+        except curses.error:
+            pass
+
         curses.use_default_colors()
         return self
 
-- 
2.8.4

[PATCH 02/17] tools/kvm_stat: catch curses exceptions only

[Stefan Raspl]

The previous version was catching all exceptions, including SIGINT.
We only want to catch the curses exceptions here.

Signed-off-by: Stefan Raspl <raspl@linux.vnet.ibm.com>
Reviewed-by: Janosch Frank <frankja@linux.vnet.ibm.com>
Reviewed-by: Sascha Silbe <silbe@linux.vnet.ibm.com>
Reviewed-by: Marc Hartmayer <mhartmay@linux.vnet.ibm.com>
---
 tools/kvm/kvm_stat/kvm_stat | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/kvm/kvm_stat/kvm_stat b/tools/kvm/kvm_stat/kvm_stat
index 8cc83a30..ef47ad7 100755
--- a/tools/kvm/kvm_stat/kvm_stat
+++ b/tools/kvm/kvm_stat/kvm_stat
@@ -809,7 +809,7 @@ class Tui(object):
         # return from C start_color() is ignorable.
         try:
             curses.start_color()
-        except:
+        except curses.error:
             pass
 
         # Hide cursor in extra statement as some monochrome terminals
-- 
2.8.4

[PATCH 03/17] tools/kvm_stat: handle SIGINT in log and batch modes

[Stefan Raspl]

SIGINT causes ugly unhandled exceptions in log and batch mode, which we
prevent by catching the exceptions accordingly.

Signed-off-by: Stefan Raspl <raspl@linux.vnet.ibm.com>
Reviewed-by: Marc Hartmayer <mhartmay@linux.vnet.ibm.com>
---
 tools/kvm/kvm_stat/kvm_stat | 28 +++++++++++++++++-----------
 1 file changed, 17 insertions(+), 11 deletions(-)

diff --git a/tools/kvm/kvm_stat/kvm_stat b/tools/kvm/kvm_stat/kvm_stat
index ef47ad7..14536c0 100755
--- a/tools/kvm/kvm_stat/kvm_stat
+++ b/tools/kvm/kvm_stat/kvm_stat
@@ -969,12 +969,15 @@ class Tui(object):
 
 def batch(stats):
     """Prints statistics in a key, value format."""
-    s = stats.get()
-    time.sleep(1)
-    s = stats.get()
-    for key in sorted(s.keys()):
-        values = s[key]
-        print '%-42s%10d%10d' % (key, values[0], values[1])
+    try:
+        s = stats.get()
+        time.sleep(1)
+        s = stats.get()
+        for key in sorted(s.keys()):
+            values = s[key]
+            print '%-42s%10d%10d' % (key, values[0], values[1])
+    except KeyboardInterrupt:
+        pass
 
 def log(stats):
     """Prints statistics as reiterating key block, multiple value blocks."""
@@ -991,11 +994,14 @@ def log(stats):
     line = 0
     banner_repeat = 20
     while True:
-        time.sleep(1)
-        if line % banner_repeat == 0:
-            banner()
-        statline()
-        line += 1
+        try:
+            time.sleep(1)
+            if line % banner_repeat == 0:
+                banner()
+            statline()
+            line += 1
+        except KeyboardInterrupt:
+            break
 
 def get_options():
     """Returns processed program arguments."""
-- 
2.8.4

[PATCH 04/17] tools/kvm_stat: fix misc glitches

[Stefan Raspl]

Addresses
- eliminate extra import
- missing variable initialization
- type redefinition from int to float
- passing of int type argument instead of string
- a couple of PEP8-reported indentation/formatting glitches
- remove unused variable drilldown in class Tui

Signed-off-by: Stefan Raspl <raspl@linux.vnet.ibm.com>
Reviewed-by: Marc Hartmayer <mhartmay@linux.vnet.ibm.com>
---
 tools/kvm/kvm_stat/kvm_stat | 27 ++++++++++++---------------
 1 file changed, 12 insertions(+), 15 deletions(-)

diff --git a/tools/kvm/kvm_stat/kvm_stat b/tools/kvm/kvm_stat/kvm_stat
index 14536c0..a09179e 100755
--- a/tools/kvm/kvm_stat/kvm_stat
+++ b/tools/kvm/kvm_stat/kvm_stat
@@ -31,7 +31,6 @@ import resource
 import struct
 import re
 from collections import defaultdict
-from time import sleep
 
 VMX_EXIT_REASONS = {
     'EXCEPTION_NMI':        0,
@@ -339,8 +338,7 @@ def get_filters():
         filters['kvm_exit'] = ('exit_reason', ARCH.exit_reasons)
     return filters
 
-libc = ctypes.CDLL('libc.so.6', use_errno=True)
-syscall = libc.syscall
+syscall = ctypes.CDLL('libc.so.6', use_errno=True).syscall
 
 class perf_event_attr(ctypes.Structure):
     """Struct that holds the necessary data to set up a trace event.
@@ -657,6 +655,7 @@ class DebugfsProvider(object):
         self._fields = self.get_available_fields()
         self._pid = 0
         self.do_read = True
+        self.paths = []
 
     def get_available_fields(self):
         """"Returns a list of available fields.
@@ -794,7 +793,6 @@ class Tui(object):
     def __init__(self, stats):
         self.stats = stats
         self.screen = None
-        self.drilldown = False
         self.update_drilldown()
 
     def __enter__(self):
@@ -950,11 +948,10 @@ class Tui(object):
         while True:
             self.refresh(sleeptime)
             curses.halfdelay(int(sleeptime * 10))
-            sleeptime = 3
+            sleeptime = 3.
             try:
                 char = self.screen.getkey()
                 if char == 'x':
-                    self.drilldown = not self.drilldown
                     self.update_drilldown()
                 if char == 'q':
                     break
@@ -1064,12 +1061,12 @@ Requirements:
                          help='fields to display (regex)',
                          )
     optparser.add_option('-p', '--pid',
-                        action='store',
-                        default=0,
-                        type=int,
-                        dest='pid',
-                        help='restrict statistics to pid',
-                        )
+                         action='store',
+                         default=0,
+                         type='int',
+                         dest='pid',
+                         help='restrict statistics to pid',
+                         )
     (options, _) = optparser.parse_args(sys.argv)
     return options
 
@@ -1099,8 +1096,8 @@ def check_access(options):
                          "Also ensure, that the kvm modules are loaded.\n")
         sys.exit(1)
 
-    if not os.path.exists(PATH_DEBUGFS_TRACING) and (options.tracepoints
-                                                     or not options.debugfs):
+    if not os.path.exists(PATH_DEBUGFS_TRACING) and (options.tracepoints or
+                                                     not options.debugfs):
         sys.stderr.write("Please enable CONFIG_TRACING in your kernel "
                          "when using the option -t (default).\n"
                          "If it is enabled, make {0} readable by the "
@@ -1111,7 +1108,7 @@ def check_access(options):
 
         sys.stderr.write("Falling back to debugfs statistics!\n")
         options.debugfs = True
-        sleep(5)
+        time.sleep(5)
 
     return options
 
-- 
2.8.4

[PATCH 05/17] tools/kvm_stat: fix trace setup glitch on field updates in TracepointProvider

[Stefan Raspl]

Updating the fields of the TracepointProvider does not propagate changes to the
tracepoints. This shows when a pid filter is enabled, whereby subsequent
extensions of the fields of the Tracepoint provider (e.g. by toggling
drilldown) will not modify the tracepoints as required.
To reproduce, select a specific process via interactive command 'p', and
enable drilldown via 'x' - none of the fields with the braces will appear
although they should.
The fix will always leave all available fields in the TracepointProvider
enabled.

Signed-off-by: Stefan Raspl <raspl@linux.vnet.ibm.com>
Based-on-text-by: Janosch Frank <frankja@linux.vnet.ibm.com>
---
 tools/kvm/kvm_stat/kvm_stat | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/tools/kvm/kvm_stat/kvm_stat b/tools/kvm/kvm_stat/kvm_stat
index a09179e..a1fa86e 100755
--- a/tools/kvm/kvm_stat/kvm_stat
+++ b/tools/kvm/kvm_stat/kvm_stat
@@ -549,6 +549,7 @@ class TracepointProvider(object):
     def setup_traces(self):
         """Creates all event and group objects needed to be able to retrieve
         data."""
+        fields = self.get_available_fields()
         if self._pid > 0:
             # Fetch list of all threads of the monitored pid, as qemu
             # starts a thread for each vcpu.
@@ -559,7 +560,7 @@ class TracepointProvider(object):
 
         # The constant is needed as a buffer for python libs, std
         # streams and other files that the script opens.
-        newlim = len(groupids) * len(self._fields) + 50
+        newlim = len(groupids) * len(fields) + 50
         try:
             softlim_, hardlim = resource.getrlimit(resource.RLIMIT_NOFILE)
 
@@ -575,7 +576,7 @@ class TracepointProvider(object):
 
         for groupid in groupids:
             group = Group()
-            for name in self._fields:
+            for name in fields:
                 tracepoint = name
                 tracefilter = None
                 match = re.match(r'(.*)\((.*)\)', name)
-- 
2.8.4

[PATCH 06/17] tools/kvm_stat: full PEP8 compliance

[Stefan Raspl]

Provides all missing empty lines as required for full PEP compliance.

Signed-off-by: Stefan Raspl <raspl@linux.vnet.ibm.com>
Reviewed-by: Marc Hartmayer <mhartmay@linux.vnet.ibm.com>
---
 tools/kvm/kvm_stat/kvm_stat | 22 ++++++++++++++++++++++
 1 file changed, 22 insertions(+)

diff --git a/tools/kvm/kvm_stat/kvm_stat b/tools/kvm/kvm_stat/kvm_stat
index a1fa86e..e0503c8 100755
--- a/tools/kvm/kvm_stat/kvm_stat
+++ b/tools/kvm/kvm_stat/kvm_stat
@@ -224,6 +224,7 @@ IOCTL_NUMBERS = {
     'RESET':       0x00002403,
 }
 
+
 class Arch(object):
     """Encapsulates global architecture specific data.
 
@@ -254,12 +255,14 @@ class Arch(object):
                     return ArchX86(SVM_EXIT_REASONS)
                 return
 
+
 class ArchX86(Arch):
     def __init__(self, exit_reasons):
         self.sc_perf_evt_open = 298
         self.ioctl_numbers = IOCTL_NUMBERS
         self.exit_reasons = exit_reasons
 
+
 class ArchPPC(Arch):
     def __init__(self):
         self.sc_perf_evt_open = 319
@@ -274,12 +277,14 @@ class ArchPPC(Arch):
         self.ioctl_numbers['SET_FILTER'] = 0x80002406 | char_ptr_size << 16
         self.exit_reasons = {}
 
+
 class ArchA64(Arch):
     def __init__(self):
         self.sc_perf_evt_open = 241
         self.ioctl_numbers = IOCTL_NUMBERS
         self.exit_reasons = AARCH64_EXIT_REASONS
 
+
 class ArchS390(Arch):
     def __init__(self):
         self.sc_perf_evt_open = 331
@@ -340,6 +345,7 @@ def get_filters():
 
 syscall = ctypes.CDLL('libc.so.6', use_errno=True).syscall
 
+
 class perf_event_attr(ctypes.Structure):
     """Struct that holds the necessary data to set up a trace event.
 
@@ -368,6 +374,7 @@ class perf_event_attr(ctypes.Structure):
         self.size = ctypes.sizeof(self)
         self.read_format = PERF_FORMAT_GROUP
 
+
 def perf_event_open(attr, pid, cpu, group_fd, flags):
     """Wrapper for the sys_perf_evt_open() syscall.
 
@@ -393,6 +400,7 @@ PERF_FORMAT_GROUP = 1 << 3
 PATH_DEBUGFS_TRACING = '/sys/kernel/debug/tracing'
 PATH_DEBUGFS_KVM = '/sys/kernel/debug/kvm'
 
+
 class Group(object):
     """Represents a perf event group."""
 
@@ -425,6 +433,7 @@ class Group(object):
                         struct.unpack(read_format,
                                       os.read(self.events[0].fd, length))))
 
+
 class Event(object):
     """Represents a performance event and manages its life cycle."""
     def __init__(self, name, group, trace_cpu, trace_pid, trace_point,
@@ -508,6 +517,7 @@ class Event(object):
         """Resets the count of the trace event in the kernel."""
         fcntl.ioctl(self.fd, ARCH.ioctl_numbers['RESET'], 0)
 
+
 class TracepointProvider(object):
     """Data provider for the stats class.
 
@@ -649,6 +659,7 @@ class TracepointProvider(object):
                     ret[name] += val
         return ret
 
+
 class DebugfsProvider(object):
     """Provides data from the files that KVM creates in the kvm debugfs
     folder."""
@@ -718,6 +729,7 @@ class DebugfsProvider(object):
         except IOError:
             return 0
 
+
 class Stats(object):
     """Manages the data providers and the data they provide.
 
@@ -789,6 +801,7 @@ class Stats(object):
 LABEL_WIDTH = 40
 NUMBER_WIDTH = 10
 
+
 class Tui(object):
     """Instruments curses to draw a nice text ui."""
     def __init__(self, stats):
@@ -858,6 +871,7 @@ class Tui(object):
                            len('Current'), 'Current')
         row = 3
         stats = self.stats.get()
+
         def sortkey(x):
             if stats[x][1]:
                 return (-stats[x][1], -stats[x][0])
@@ -965,6 +979,7 @@ class Tui(object):
             except curses.error:
                 continue
 
+
 def batch(stats):
     """Prints statistics in a key, value format."""
     try:
@@ -977,13 +992,16 @@ def batch(stats):
     except KeyboardInterrupt:
         pass
 
+
 def log(stats):
     """Prints statistics as reiterating key block, multiple value blocks."""
     keys = sorted(stats.get().iterkeys())
+
     def banner():
         for k in keys:
             print '%s' % k,
         print
+
     def statline():
         s = stats.get()
         for k in keys:
@@ -1001,6 +1019,7 @@ def log(stats):
         except KeyboardInterrupt:
             break
 
+
 def get_options():
     """Returns processed program arguments."""
     description_text = """
@@ -1071,6 +1090,7 @@ Requirements:
     (options, _) = optparser.parse_args(sys.argv)
     return options
 
+
 def get_providers(options):
     """Returns a list of data providers depending on the passed options."""
     providers = []
@@ -1084,6 +1104,7 @@ def get_providers(options):
 
     return providers
 
+
 def check_access(options):
     """Exits if the current user can't access all needed directories."""
     if not os.path.exists('/sys/kernel/debug'):
@@ -1113,6 +1134,7 @@ def check_access(options):
 
     return options
 
+
 def main():
     options = get_options()
     options = check_access(options)
-- 
2.8.4

[PATCH 07/17] tools/kvm_stat: reduce perceived idle time on filter updates

[Stefan Raspl]

Whenever a user adds a filter, we
* redraw the header immediately for a snappy response
* print a message indicating to the user that we're busy while the
  noticeable delay induced by updating all of the stats objects takes place
* update the statistics ASAP (i.e. after 0.25s instead of 3s) to be
  consistent with behavior on startup
To do so, we split the Tui's refresh() method to allow for drawing header
and stats separately, and trigger a header refresh whenever we are about
to do something that takes a while - like updating filters.

Signed-off-by: Stefan Raspl <raspl@linux.vnet.ibm.com>
---
 tools/kvm/kvm_stat/kvm_stat | 48 ++++++++++++++++++++++++++++-----------------
 1 file changed, 30 insertions(+), 18 deletions(-)

diff --git a/tools/kvm/kvm_stat/kvm_stat b/tools/kvm/kvm_stat/kvm_stat
index e0503c8..eeb5fe5 100755
--- a/tools/kvm/kvm_stat/kvm_stat
+++ b/tools/kvm/kvm_stat/kvm_stat
@@ -800,6 +800,8 @@ class Stats(object):
 
 LABEL_WIDTH = 40
 NUMBER_WIDTH = 10
+DELAY_INITIAL = 0.25
+DELAY_REGULAR = 3.
 
 
 class Tui(object):
@@ -855,13 +857,14 @@ class Tui(object):
         """Propagates pid selection to stats object."""
         self.stats.pid_filter = pid
 
-    def refresh(self, sleeptime):
-        """Refreshes on-screen data."""
+    def refresh_header(self, pid=None):
+        """Refreshes the header."""
+        if pid is None:
+            pid = self.stats.pid_filter
         self.screen.erase()
-        if self.stats.pid_filter > 0:
+        if pid > 0:
             self.screen.addstr(0, 0, 'kvm statistics - pid {0}'
-                               .format(self.stats.pid_filter),
-                               curses.A_BOLD)
+                               .format(pid), curses.A_BOLD)
         else:
             self.screen.addstr(0, 0, 'kvm statistics - summary', curses.A_BOLD)
         self.screen.addstr(2, 1, 'Event')
@@ -869,7 +872,13 @@ class Tui(object):
                            len('Total'), 'Total')
         self.screen.addstr(2, 1 + LABEL_WIDTH + NUMBER_WIDTH + 8 -
                            len('Current'), 'Current')
+        self.screen.addstr(4, 1, 'Collecting data...')
+        self.screen.refresh()
+
+    def refresh_body(self, sleeptime):
         row = 3
+        self.screen.move(row, 0)
+        self.screen.clrtobot()
         stats = self.stats.get()
 
         def sortkey(x):
@@ -913,10 +922,12 @@ class Tui(object):
             regex = self.screen.getstr()
             curses.noecho()
             if len(regex) == 0:
+                self.refresh_header()
                 return
             try:
                 re.compile(regex)
                 self.stats.fields_filter = regex
+                self.refresh_header()
                 return
             except re.error:
                 continue
@@ -943,37 +954,38 @@ class Tui(object):
 
             try:
                 pid = int(pid)
-
-                if pid == 0:
-                    self.update_pid(pid)
-                    break
-                else:
-                    if not os.path.isdir(os.path.join('/proc/', str(pid))):
-                        continue
-                    else:
-                        self.update_pid(pid)
-                        break
+                if pid != 0 and not os.path.isdir(os.path.join('/proc/',
+                                                               str(pid))):
+                    continue
+                self.refresh_header(pid)
+                self.update_pid(pid)
+                break
 
             except ValueError:
                 continue
 
     def show_stats(self):
         """Refreshes the screen and processes user input."""
-        sleeptime = 0.25
+        sleeptime = DELAY_INITIAL
+        self.refresh_header()
         while True:
-            self.refresh(sleeptime)
+            self.refresh_body(sleeptime)
             curses.halfdelay(int(sleeptime * 10))
-            sleeptime = 3.
+            sleeptime = DELAY_REGULAR
             try:
                 char = self.screen.getkey()
                 if char == 'x':
+                    self.refresh_header()
                     self.update_drilldown()
+                    sleeptime = DELAY_INITIAL
                 if char == 'q':
                     break
                 if char == 'f':
                     self.show_filter_selection()
+                    sleeptime = DELAY_INITIAL
                 if char == 'p':
                     self.show_vm_selection()
+                    sleeptime = DELAY_INITIAL
             except KeyboardInterrupt:
                 break
             except curses.error:
-- 
2.8.4

[PATCH 08/17] tools/kvm_stat: document list of interactive commands

[Stefan Raspl]

Apart from the source code, there does not seem to be a place that documents
the interactive capabilities of kvm_stat yet.

Signed-off-by: Stefan Raspl <raspl@linux.vnet.ibm.com>
---
 tools/kvm/kvm_stat/kvm_stat     |  7 +++++++
 tools/kvm/kvm_stat/kvm_stat.txt | 16 ++++++++++++++++
 2 files changed, 23 insertions(+)

diff --git a/tools/kvm/kvm_stat/kvm_stat b/tools/kvm/kvm_stat/kvm_stat
index eeb5fe5..1db7f38 100755
--- a/tools/kvm/kvm_stat/kvm_stat
+++ b/tools/kvm/kvm_stat/kvm_stat
@@ -1051,6 +1051,13 @@ Requirements:
   CAP_SYS_ADMIN and perf events are used.
 - CAP_SYS_RESOURCE if the hard limit is not high enough to allow
   the large number of files that are possibly opened.
+
+Interactive Commands:
+   f     filter by regular expression
+   p     filter by PID
+   q     quit
+   x     toggle reporting of stats for individual child trace events
+Press any other key to refresh statistics immediately.
 """
 
     class PlainHelpFormatter(optparse.IndentedHelpFormatter):
diff --git a/tools/kvm/kvm_stat/kvm_stat.txt b/tools/kvm/kvm_stat/kvm_stat.txt
index b92a153..077bcc7 100644
--- a/tools/kvm/kvm_stat/kvm_stat.txt
+++ b/tools/kvm/kvm_stat/kvm_stat.txt
@@ -18,11 +18,27 @@ state transitions such as guest mode entry and exit.
 This tool is useful for observing guest behavior from the host perspective.
 Often conclusions about performance or buggy behavior can be drawn from the
 output.
+While running in regular mode, use any of the keys listed in section
+'Interactive Commands' below.
+Use batch and logging modes for scripting purposes.
 
 The set of KVM kernel module trace events may be specific to the kernel version
 or architecture.  It is best to check the KVM kernel module source code for the
 meaning of events.
 
+INTERACTIVE COMMANDS
+--------------------
+[horizontal]
+*f*::	filter by regular expression
+
+*p*::	filter by PID
+
+*q*::	quit
+
+*x*::	toggle reporting of stats for child trace events
+
+Press any other key to refresh statistics immediately.
+
 OPTIONS
 -------
 -1::
-- 
2.8.4

[PATCH 09/17] tools/kvm_stat: display guest name when using pid filter

[Stefan Raspl]

When running kvm_stat with option '-p' to filter per process, display
the QEMU guest name next to the pid, if available.

Signed-off-by: Stefan Raspl <raspl@linux.vnet.ibm.com>
Reviewed-By: Janosch Frank <frankja@linux.vnet.ibm.com>
---
 tools/kvm/kvm_stat/kvm_stat | 41 +++++++++++++++++++++++++++++++++++++++--
 1 file changed, 39 insertions(+), 2 deletions(-)

diff --git a/tools/kvm/kvm_stat/kvm_stat b/tools/kvm/kvm_stat/kvm_stat
index 1db7f38..2ef7308 100755
--- a/tools/kvm/kvm_stat/kvm_stat
+++ b/tools/kvm/kvm_stat/kvm_stat
@@ -320,6 +320,37 @@ def parse_int_list(list_string):
     return integers
 
 
+def get_gname_from_pid(pid):
+    """Returns the guest name for a QEMU process pid.
+
+    Extracts the guest name from the QEMU comma line by processing the '-name'
+    option. Will also handle names specified out of sequence.
+
+    """
+    name = ''
+    try:
+        line = open('/proc/{}/cmdline'.format(pid), 'rb').read().split('\0')
+        parms = line[line.index('-name') + 1].split(',')
+        while '' in parms:
+            # commas are escaped (i.e. ',,'), hence e.g. 'foo,bar' results in
+            # ['foo', '', 'bar'], which we revert here
+            idx = parms.index('')
+            parms[idx - 1] += ',' + parms[idx + 1]
+            del parms[idx:idx+2]
+        # the '-name' switch allows for two ways to specify the guest name,
+        # where the plain name overrides the name specified via 'guest='
+        for arg in parms:
+            if '=' not in arg:
+                name = arg
+                break
+            if arg[:6] == 'guest=':
+                name = arg[6:]
+    except (ValueError, IOError, IndexError):
+        pass
+
+    return name
+
+
 def get_online_cpus():
     """Returns a list of cpu id integers."""
     with open('/sys/devices/system/cpu/online') as cpu_list:
@@ -802,6 +833,7 @@ LABEL_WIDTH = 40
 NUMBER_WIDTH = 10
 DELAY_INITIAL = 0.25
 DELAY_REGULAR = 3.
+MAX_GUEST_NAME_LEN = 48
 
 
 class Tui(object):
@@ -862,9 +894,14 @@ class Tui(object):
         if pid is None:
             pid = self.stats.pid_filter
         self.screen.erase()
+        gname = get_gname_from_pid(pid)
+        if gname:
+            gname = ('({})'.format(gname[:MAX_GUEST_NAME_LEN] + '...'
+                                   if len(gname) > MAX_GUEST_NAME_LEN
+                                   else gname))
         if pid > 0:
-            self.screen.addstr(0, 0, 'kvm statistics - pid {0}'
-                               .format(pid), curses.A_BOLD)
+            self.screen.addstr(0, 0, 'kvm statistics - pid {0} {1}'
+                               .format(pid, gname), curses.A_BOLD)
         else:
             self.screen.addstr(0, 0, 'kvm statistics - summary', curses.A_BOLD)
         self.screen.addstr(2, 1, 'Event')
-- 
2.8.4

[PATCH 10/17] tools/kvm_stat: remove pid filter on empty input

[Stefan Raspl]

Improve consistency in the interactive dialogue for pid filtering by
removing any filters on empty input (in addition to entering 0).

Signed-off-by: Stefan Raspl <raspl@linux.vnet.ibm.com>
Reviewed-by: Janosch Frank <frankja@linux.vnet.ibm.com>
Reviewed-by: Marc Hartmayer <mhartmay@linux.vnet.ibm.com>
---
 tools/kvm/kvm_stat/kvm_stat | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

diff --git a/tools/kvm/kvm_stat/kvm_stat b/tools/kvm/kvm_stat/kvm_stat
index 2ef7308..8271b41 100755
--- a/tools/kvm/kvm_stat/kvm_stat
+++ b/tools/kvm/kvm_stat/kvm_stat
@@ -990,10 +990,13 @@ class Tui(object):
             curses.noecho()
 
             try:
-                pid = int(pid)
-                if pid != 0 and not os.path.isdir(os.path.join('/proc/',
-                                                               str(pid))):
-                    continue
+                if len(pid) > 0:
+                    pid = int(pid)
+                    if pid != 0 and not os.path.isdir(os.path.join('/proc/',
+                                                                   str(pid))):
+                        continue
+                else:
+                    pid = 0
                 self.refresh_header(pid)
                 self.update_pid(pid)
                 break
-- 
2.8.4

[PATCH 11/17] tools/kvm_stat: print error messages on faulty pid filter input

[Stefan Raspl]

Print helpful messages in case users enter invalid input or invalid pids in
the interactive pid filter dialogue.

Signed-off-by: Stefan Raspl <raspl@linux.vnet.ibm.com>
Reviewed-by: Marc Hartmayer <mhartmay@linux.vnet.ibm.com>
---
 tools/kvm/kvm_stat/kvm_stat | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/tools/kvm/kvm_stat/kvm_stat b/tools/kvm/kvm_stat/kvm_stat
index 8271b41..db6a624 100755
--- a/tools/kvm/kvm_stat/kvm_stat
+++ b/tools/kvm/kvm_stat/kvm_stat
@@ -975,6 +975,7 @@ class Tui(object):
         Asks for a pid until a valid pid or 0 has been entered.
 
         """
+        msg = ''
         while True:
             self.screen.erase()
             self.screen.addstr(0, 0,
@@ -983,6 +984,7 @@ class Tui(object):
             self.screen.addstr(1, 0,
                                'This might limit the shown data to the trace '
                                'statistics.')
+            self.screen.addstr(5, 0, msg)
 
             curses.echo()
             self.screen.addstr(3, 0, "Pid [0 or pid]: ")
@@ -994,6 +996,7 @@ class Tui(object):
                     pid = int(pid)
                     if pid != 0 and not os.path.isdir(os.path.join('/proc/',
                                                                    str(pid))):
+                        msg = '"' + str(pid) + '": Not a running process'
                         continue
                 else:
                     pid = 0
@@ -1002,6 +1005,7 @@ class Tui(object):
                 break
 
             except ValueError:
+                msg = '"' + str(pid) + '": Not a valid pid'
                 continue
 
     def show_stats(self):
-- 
2.8.4

[PATCH 12/17] tools/kvm_stat: display regex when set to non-default

[Stefan Raspl]

If a user defines a regex filter through the interactive command, display
the active regex in the header's second line.

Signed-off-by: Stefan Raspl <raspl@linux.vnet.ibm.com>
Reviewed-by: Marc Hartmayer <mhartmay@linux.vnet.ibm.com>
---
 tools/kvm/kvm_stat/kvm_stat | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/tools/kvm/kvm_stat/kvm_stat b/tools/kvm/kvm_stat/kvm_stat
index db6a624..da7bd0b 100755
--- a/tools/kvm/kvm_stat/kvm_stat
+++ b/tools/kvm/kvm_stat/kvm_stat
@@ -834,6 +834,7 @@ NUMBER_WIDTH = 10
 DELAY_INITIAL = 0.25
 DELAY_REGULAR = 3.
 MAX_GUEST_NAME_LEN = 48
+MAX_REGEX_LEN = 44
 
 
 class Tui(object):
@@ -904,6 +905,11 @@ class Tui(object):
                                .format(pid, gname), curses.A_BOLD)
         else:
             self.screen.addstr(0, 0, 'kvm statistics - summary', curses.A_BOLD)
+        if self.stats.fields_filter and self.stats.fields_filter != '^[^\(]*$':
+            regex = self.stats.fields_filter
+            if len(regex) > MAX_REGEX_LEN:
+                regex = regex[:MAX_REGEX_LEN] + '...'
+            self.screen.addstr(1, 17, 'regex filter: {0}'.format(regex))
         self.screen.addstr(2, 1, 'Event')
         self.screen.addstr(2, 1 + LABEL_WIDTH + NUMBER_WIDTH -
                            len('Total'), 'Total')
-- 
2.8.4

[PATCH 13/17] tools/kvm_stat: remove regex filter on empty input

[Stefan Raspl]

Behavior on empty/0 input for regex and pid filtering was inconsistent, as
the former would keep the current filter, while the latter would (naturally)
remove any pid filtering.
Make things consistent by falling back to the default filter on empty input
for the regex filter dialogue.

Signed-off-by: Stefan Raspl <raspl@linux.vnet.ibm.com>
Reviewed-by: Marc Hartmayer <mhartmay@linux.vnet.ibm.com>
---
 tools/kvm/kvm_stat/kvm_stat | 1 +
 1 file changed, 1 insertion(+)

diff --git a/tools/kvm/kvm_stat/kvm_stat b/tools/kvm/kvm_stat/kvm_stat
index da7bd0b..17d2eff 100755
--- a/tools/kvm/kvm_stat/kvm_stat
+++ b/tools/kvm/kvm_stat/kvm_stat
@@ -965,6 +965,7 @@ class Tui(object):
             regex = self.screen.getstr()
             curses.noecho()
             if len(regex) == 0:
+                self.stats.fields_filter = r'^[^\(]*$'
                 self.refresh_header()
                 return
             try:
-- 
2.8.4

[PATCH 14/17] tools/kvm_stat: add option '--guest'

[Stefan Raspl]

Add a new option '-g'/'--guest' to select a particular process by providing
the QEMU guest name.
Notes:
- The logic to figure out the pid corresponding to the guest name might look
  scary, but works pretty reliably in practice; in the unlikely event that it
  returns add'l flukes, it will bail out and hint at using '-p' instead, no
  harm done.
- Mixing '-g' and '-p' is possible, and the final instance specified on the
  command line is the significant one. This is consistent with current
  behavior for '-p' which, if specified multiple times, also regards the final
  instance as the significant one.

Signed-off-by: Stefan Raspl <raspl@linux.vnet.ibm.com>
---
 tools/kvm/kvm_stat/kvm_stat     | 101 +++++++++++++++++++++++++++++++++++++++-
 tools/kvm/kvm_stat/kvm_stat.txt |   6 +++
 2 files changed, 105 insertions(+), 2 deletions(-)

diff --git a/tools/kvm/kvm_stat/kvm_stat b/tools/kvm/kvm_stat/kvm_stat
index 17d2eff..ec77aac 100755
--- a/tools/kvm/kvm_stat/kvm_stat
+++ b/tools/kvm/kvm_stat/kvm_stat
@@ -30,6 +30,7 @@ import fcntl
 import resource
 import struct
 import re
+import subprocess
 from collections import defaultdict
 
 VMX_EXIT_REASONS = {
@@ -320,6 +321,30 @@ def parse_int_list(list_string):
     return integers
 
 
+def get_pid_from_gname(gname):
+    """Fuzzy function to convert guest name to QEMU process pid.
+
+    Returns a list of potential pids, can be empty if no match found.
+    Throws an exception on processing errors.
+
+    """
+    pids = []
+    try:
+        child = subprocess.Popen(['ps', '-A', '--format', 'pid,args'],
+                                 stdout=subprocess.PIPE)
+    except:
+        raise Exception
+    for line in child.stdout:
+        line = line.lstrip().split(' ', 1)
+        # perform a sanity check before calling the more expensive
+        # function to possibly extract the guest name
+        if ' -name ' in line[1] and gname == get_gname_from_pid(line[0]):
+            pids.append(int(line[0]))
+    child.stdout.close()
+
+    return pids
+
+
 def get_gname_from_pid(pid):
     """Returns the guest name for a QEMU process pid.
 
@@ -976,7 +1001,7 @@ class Tui(object):
             except re.error:
                 continue
 
-    def show_vm_selection(self):
+    def show_vm_selection_by_pid(self):
         """Draws PID selection mask.
 
         Asks for a pid until a valid pid or 0 has been entered.
@@ -1015,6 +1040,50 @@ class Tui(object):
                 msg = '"' + str(pid) + '": Not a valid pid'
                 continue
 
+    def show_vm_selection_by_guest_name(self):
+        """Draws guest selection mask.
+
+        Asks for a guest name until a valid guest name or '' is entered.
+
+        """
+        msg = ''
+        while True:
+            self.screen.erase()
+            self.screen.addstr(0, 0,
+                               'Show statistics for specific guest.',
+                               curses.A_BOLD)
+            self.screen.addstr(1, 0,
+                               'This might limit the shown data to the trace '
+                               'statistics.')
+            self.screen.addstr(5, 0, msg)
+            curses.echo()
+            self.screen.addstr(3, 0, "Guest [ENTER or guest]: ")
+            gname = self.screen.getstr()
+            curses.noecho()
+
+            if not gname:
+                self.refresh_header(0)
+                self.update_pid(0)
+                break
+            else:
+                pids = []
+                try:
+                    pids = get_pid_from_gname(gname)
+                except:
+                    msg = '"' + gname + '": Internal error while searching, ' \
+                          'use pid filter instead'
+                    continue
+                if len(pids) == 0:
+                    msg = '"' + gname + '": Not an active guest'
+                    continue
+                if len(pids) > 1:
+                    msg = '"' + gname + '": Multiple matches found, use pid ' \
+                          'filter instead'
+                    continue
+                self.refresh_header(pids[0])
+                self.update_pid(pids[0])
+                break
+
     def show_stats(self):
         """Refreshes the screen and processes user input."""
         sleeptime = DELAY_INITIAL
@@ -1034,8 +1103,11 @@ class Tui(object):
                 if char == 'f':
                     self.show_filter_selection()
                     sleeptime = DELAY_INITIAL
+                if char == 'g':
+                    self.show_vm_selection_by_guest_name()
+                    sleeptime = DELAY_INITIAL
                 if char == 'p':
-                    self.show_vm_selection()
+                    self.show_vm_selection_by_pid()
                     sleeptime = DELAY_INITIAL
             except KeyboardInterrupt:
                 break
@@ -1105,6 +1177,7 @@ Requirements:
 
 Interactive Commands:
    f     filter by regular expression
+   g     filter by guest name
    p     filter by PID
    q     quit
    x     toggle reporting of stats for individual child trace events
@@ -1118,6 +1191,22 @@ Press any other key to refresh statistics immediately.
             else:
                 return ""
 
+    def cb_guest_to_pid(option, opt, val, parser):
+        try:
+            pids = get_pid_from_gname(val)
+        except:
+            raise optparse.OptionValueError('Error while searching for guest '
+                                            '"{}", use "-p" to specify a pid '
+                                            'instead'.format(val))
+        if len(pids) == 0:
+            raise optparse.OptionValueError('No guest by the name "{}" '
+                                            'found'.format(val))
+        if len(pids) > 1:
+            raise optparse.OptionValueError('Multiple processes found (pids: '
+                                            '{}) - use "-p" to specify a pid '
+                                            'instead'.format(" ".join(pids)))
+        parser.values.pid = pids[0]
+
     optparser = optparse.OptionParser(description=description_text,
                                       formatter=PlainHelpFormatter())
     optparser.add_option('-1', '--once', '--batch',
@@ -1157,6 +1246,14 @@ Press any other key to refresh statistics immediately.
                          dest='pid',
                          help='restrict statistics to pid',
                          )
+    optparser.add_option('-g', '--guest',
+                         action='callback',
+                         type='string',
+                         dest='pid',
+                         metavar='GUEST',
+                         help='restrict statistics to guest by name',
+                         callback=cb_guest_to_pid,
+                         )
     (options, _) = optparser.parse_args(sys.argv)
     return options
 
diff --git a/tools/kvm/kvm_stat/kvm_stat.txt b/tools/kvm/kvm_stat/kvm_stat.txt
index 077bcc7..35587c3 100644
--- a/tools/kvm/kvm_stat/kvm_stat.txt
+++ b/tools/kvm/kvm_stat/kvm_stat.txt
@@ -31,6 +31,8 @@ INTERACTIVE COMMANDS
 [horizontal]
 *f*::	filter by regular expression
 
+*g*::	filter by guest name
+
 *p*::	filter by PID
 
 *q*::	quit
@@ -62,6 +64,10 @@ OPTIONS
 --pid=<pid>::
 	limit statistics to one virtual machine (pid)
 
+-g<guest>::
+--guest=<guest_name>::
+	limit statistics to one virtual machine (guest name)
+
 -f<fields>::
 --fields=<fields>::
 	fields to display (regex)
-- 
2.8.4

[PATCH 15/17] tools/kvm_stat: add interactive command 'c'

[Stefan Raspl]

Provide a real simple way to erase any active filter.

Signed-off-by: Stefan Raspl <raspl@linux.vnet.ibm.com>
Reviewed-by: Marc Hartmayer <mhartmay@linux.vnet.ibm.com>
---
 tools/kvm/kvm_stat/kvm_stat     | 16 ++++++++++++----
 tools/kvm/kvm_stat/kvm_stat.txt |  2 ++
 2 files changed, 14 insertions(+), 4 deletions(-)

diff --git a/tools/kvm/kvm_stat/kvm_stat b/tools/kvm/kvm_stat/kvm_stat
index ec77aac..e0f28e1 100755
--- a/tools/kvm/kvm_stat/kvm_stat
+++ b/tools/kvm/kvm_stat/kvm_stat
@@ -860,6 +860,7 @@ DELAY_INITIAL = 0.25
 DELAY_REGULAR = 3.
 MAX_GUEST_NAME_LEN = 48
 MAX_REGEX_LEN = 44
+DEFAULT_REGEX = r'^[^\(]*$'
 
 
 class Tui(object):
@@ -906,9 +907,9 @@ class Tui(object):
     def update_drilldown(self):
         """Sets or removes a filter that only allows fields without braces."""
         if not self.stats.fields_filter:
-            self.stats.fields_filter = r'^[^\(]*$'
+            self.stats.fields_filter = DEFAULT_REGEX
 
-        elif self.stats.fields_filter == r'^[^\(]*$':
+        elif self.stats.fields_filter == DEFAULT_REGEX:
             self.stats.fields_filter = None
 
     def update_pid(self, pid):
@@ -930,7 +931,8 @@ class Tui(object):
                                .format(pid, gname), curses.A_BOLD)
         else:
             self.screen.addstr(0, 0, 'kvm statistics - summary', curses.A_BOLD)
-        if self.stats.fields_filter and self.stats.fields_filter != '^[^\(]*$':
+        if self.stats.fields_filter and self.stats.fields_filter \
+           != DEFAULT_REGEX:
             regex = self.stats.fields_filter
             if len(regex) > MAX_REGEX_LEN:
                 regex = regex[:MAX_REGEX_LEN] + '...'
@@ -990,7 +992,7 @@ class Tui(object):
             regex = self.screen.getstr()
             curses.noecho()
             if len(regex) == 0:
-                self.stats.fields_filter = r'^[^\(]*$'
+                self.stats.fields_filter = DEFAULT_REGEX
                 self.refresh_header()
                 return
             try:
@@ -1100,6 +1102,11 @@ class Tui(object):
                     sleeptime = DELAY_INITIAL
                 if char == 'q':
                     break
+                if char == 'c':
+                    self.stats.fields_filter = DEFAULT_REGEX
+                    self.refresh_header(0)
+                    self.update_pid(0)
+                    sleeptime = DELAY_INITIAL
                 if char == 'f':
                     self.show_filter_selection()
                     sleeptime = DELAY_INITIAL
@@ -1176,6 +1183,7 @@ Requirements:
   the large number of files that are possibly opened.
 
 Interactive Commands:
+   c     clear filter
    f     filter by regular expression
    g     filter by guest name
    p     filter by PID
diff --git a/tools/kvm/kvm_stat/kvm_stat.txt b/tools/kvm/kvm_stat/kvm_stat.txt
index 35587c3..c3ab6a2 100644
--- a/tools/kvm/kvm_stat/kvm_stat.txt
+++ b/tools/kvm/kvm_stat/kvm_stat.txt
@@ -29,6 +29,8 @@ meaning of events.
 INTERACTIVE COMMANDS
 --------------------
 [horizontal]
+*c*::	clear filter
+
 *f*::	filter by regular expression
 
 *g*::	filter by guest name
-- 
2.8.4

[PATCH 16/17] tools/kvm_stat: add interactive command 'r'

[Stefan Raspl]

Provide an interactive command to reset the tracepoint statistics.
Requires some extra work for debugfs, as the counters cannot be reset.
On the up side, offers us the opportunity to have debugfs values reset
on startup and whenever a filter is modified, becoming consistent with
the tracepoint provider. As a bonus, 'kvmstat -dt' will now provide
useful output, instead of mixing values in totally different orders of
magnitude.
Furthermore, we avoid unnecessary resets when any of the filters is
"changed" interactively to the previous value.

Signed-off-by: Stefan Raspl <raspl@linux.vnet.ibm.com>
---
 tools/kvm/kvm_stat/kvm_stat     | 66 +++++++++++++++++++++++++++++++----------
 tools/kvm/kvm_stat/kvm_stat.txt |  2 ++
 2 files changed, 53 insertions(+), 15 deletions(-)

diff --git a/tools/kvm/kvm_stat/kvm_stat b/tools/kvm/kvm_stat/kvm_stat
index e0f28e1..f9f653a 100755
--- a/tools/kvm/kvm_stat/kvm_stat
+++ b/tools/kvm/kvm_stat/kvm_stat
@@ -715,15 +715,23 @@ class TracepointProvider(object):
                     ret[name] += val
         return ret
 
+    def reset(self):
+        """Reset all field counters"""
+        for group in self.group_leaders:
+            for event in group.events:
+                event.reset()
+
 
 class DebugfsProvider(object):
     """Provides data from the files that KVM creates in the kvm debugfs
     folder."""
     def __init__(self):
         self._fields = self.get_available_fields()
+        self._baseline = {}
         self._pid = 0
         self.do_read = True
         self.paths = []
+        self.reset()
 
     def get_available_fields(self):
         """"Returns a list of available fields.
@@ -740,6 +748,7 @@ class DebugfsProvider(object):
     @fields.setter
     def fields(self, fields):
         self._fields = fields
+        self.reset()
 
     @property
     def pid(self):
@@ -757,10 +766,11 @@ class DebugfsProvider(object):
             self.paths = filter(lambda x: "{}-".format(pid) in x, vms)
 
         else:
-            self.paths = ['']
+            self.paths = []
             self.do_read = True
+        self.reset()
 
-    def read(self):
+    def read(self, reset=0):
         """Returns a dict with format:'file name / field -> current value'."""
         results = {}
 
@@ -768,10 +778,22 @@ class DebugfsProvider(object):
         if not self.do_read:
             return results
 
-        for path in self.paths:
+        paths = self.paths
+        if self._pid == 0:
+            paths = []
+            for entry in os.walk(PATH_DEBUGFS_KVM):
+                for dir in entry[1]:
+                    paths.append(dir)
+        for path in paths:
             for field in self._fields:
-                results[field] = results.get(field, 0) \
-                                 + self.read_field(field, path)
+                value = self.read_field(field, path)
+                key = path + field
+                if reset:
+                    self._baseline[key] = value
+                if self._baseline.get(key, -1) == -1:
+                    self._baseline[key] = value
+                results[field] = (results.get(field, 0) + value -
+                                  self._baseline.get(key, 0))
 
         return results
 
@@ -785,6 +807,11 @@ class DebugfsProvider(object):
         except IOError:
             return 0
 
+    def reset(self):
+        """Reset field counters"""
+        self._baseline = {}
+        self.read(1)
+
 
 class Stats(object):
     """Manages the data providers and the data they provide.
@@ -821,14 +848,20 @@ class Stats(object):
         for provider in self.providers:
             provider.pid = self._pid_filter
 
+    def reset(self):
+        self.values = {}
+        for provider in self.providers:
+            provider.reset()
+
     @property
     def fields_filter(self):
         return self._fields_filter
 
     @fields_filter.setter
     def fields_filter(self, fields_filter):
-        self._fields_filter = fields_filter
-        self.update_provider_filters()
+        if fields_filter != self._fields_filter:
+            self._fields_filter = fields_filter
+            self.update_provider_filters()
 
     @property
     def pid_filter(self):
@@ -836,9 +869,10 @@ class Stats(object):
 
     @pid_filter.setter
     def pid_filter(self, pid):
-        self._pid_filter = pid
-        self.values = {}
-        self.update_provider_pid()
+        if pid != self._pid_filter:
+            self._pid_filter = pid
+            self.values = {}
+            self.update_provider_pid()
 
     def get(self):
         """Returns a dict with field -> (value, delta to last value) of all
@@ -846,11 +880,9 @@ class Stats(object):
         for provider in self.providers:
             new = provider.read()
             for key in provider.fields:
-                oldval = self.values.get(key, (0, 0))
+                oldval = self.values.get(key, (0, 0))[0]
                 newval = new.get(key, 0)
-                newdelta = None
-                if oldval is not None:
-                    newdelta = newval - oldval[0]
+                newdelta = newval - oldval
                 self.values[key] = (newval, newdelta)
         return self.values
 
@@ -1116,6 +1148,10 @@ class Tui(object):
                 if char == 'p':
                     self.show_vm_selection_by_pid()
                     sleeptime = DELAY_INITIAL
+                if char == 'r':
+                    self.refresh_header()
+                    self.stats.reset()
+                    sleeptime = DELAY_INITIAL
             except KeyboardInterrupt:
                 break
             except curses.error:
@@ -1188,7 +1224,7 @@ Interactive Commands:
    g     filter by guest name
    p     filter by PID
    q     quit
-   x     toggle reporting of stats for individual child trace events
+   r     reset stats
 Press any other key to refresh statistics immediately.
 """
 
diff --git a/tools/kvm/kvm_stat/kvm_stat.txt b/tools/kvm/kvm_stat/kvm_stat.txt
index c3ab6a2..109431b 100644
--- a/tools/kvm/kvm_stat/kvm_stat.txt
+++ b/tools/kvm/kvm_stat/kvm_stat.txt
@@ -39,6 +39,8 @@ INTERACTIVE COMMANDS
 
 *q*::	quit
 
+*r*::	reset stats
+
 *x*::	toggle reporting of stats for child trace events
 
 Press any other key to refresh statistics immediately.
-- 
2.8.4

[PATCH 17/17] tools/kvm_stat: add '%Total' column

[Stefan Raspl]

Add column '%Total' next to 'Total' for easier comparison of numbers between
hosts.

Signed-off-by: Stefan Raspl <raspl@linux.vnet.ibm.com>
Marc Hartmayer <mhartmay@linux.vnet.ibm.com>
---
 tools/kvm/kvm_stat/kvm_stat | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/tools/kvm/kvm_stat/kvm_stat b/tools/kvm/kvm_stat/kvm_stat
index f9f653a..9e40c8a 100755
--- a/tools/kvm/kvm_stat/kvm_stat
+++ b/tools/kvm/kvm_stat/kvm_stat
@@ -972,7 +972,9 @@ class Tui(object):
         self.screen.addstr(2, 1, 'Event')
         self.screen.addstr(2, 1 + LABEL_WIDTH + NUMBER_WIDTH -
                            len('Total'), 'Total')
-        self.screen.addstr(2, 1 + LABEL_WIDTH + NUMBER_WIDTH + 8 -
+        self.screen.addstr(2, 1 + LABEL_WIDTH + NUMBER_WIDTH + 7 -
+                           len('%Total'), '%Total')
+        self.screen.addstr(2, 1 + LABEL_WIDTH + NUMBER_WIDTH + 7 + 8 -
                            len('Current'), 'Current')
         self.screen.addstr(4, 1, 'Collecting data...')
         self.screen.refresh()
@@ -988,6 +990,9 @@ class Tui(object):
                 return (-stats[x][1], -stats[x][0])
             else:
                 return (0, -stats[x][0])
+        total = 0.
+        for val in stats.values():
+            total += val[0]
         for key in sorted(stats.keys(), key=sortkey):
 
             if row >= self.screen.getmaxyx()[0]:
@@ -1000,6 +1005,8 @@ class Tui(object):
             col += LABEL_WIDTH
             self.screen.addstr(row, col, '%10d' % (values[0],))
             col += NUMBER_WIDTH
+            self.screen.addstr(row, col, '%7.1f' % (values[0] * 100 / total,))
+            col += 7
             if values[1] is not None:
                 self.screen.addstr(row, col, '%8d' % (values[1] / sleeptime,))
             row += 1
-- 
2.8.4

Re: [PATCH 00/17] tools/kvm_stat: Misc Patches

[Stefan Raspl]

*ping*


On 20.02.2017 16:41, Stefan Raspl wrote:
> Assortment of various fixes and improvements for the kvm_stat utility.
> Patches 1-5 are bugfixes.
> Patch 6 provides full PEP8 compliance by inserting the required empty
> lines. It is included as an extra for easy disposal, since that kind of
> patch might be a bit controversial.
> There are some subtle aspects regarding patch 7, as its effects might
> differ per platform. However, I didn't notice any negative effects, only
> improvements.
> Patch 8 is a simple doc update.
> Patches 9-13 provide usability improvements.
> Patches 14-17 add new features.
> 
> Signed-off-by: Stefan Raspl <raspl@linux.vnet.ibm.com>
> 
> ---
> 
> Stefan Raspl (17):
>   tools/kvm_stat: hide cursor
>   tools/kvm_stat: catch curses exceptions only
>   tools/kvm_stat: handle SIGINT in log and batch modes
>   tools/kvm_stat: fix misc glitches
>   tools/kvm_stat: fix trace setup glitch on field updates in
>     TracepointProvider
>   tools/kvm_stat: full PEP8 compliance
>   tools/kvm_stat: reduce perceived idle time on filter updates
>   tools/kvm_stat: document list of interactive commands
>   tools/kvm_stat: display guest name when using pid filter
>   tools/kvm_stat: remove pid filter on empty input
>   tools/kvm_stat: print error messages on faulty pid filter input
>   tools/kvm_stat: display regex when set to non-default
>   tools/kvm_stat: remove regex filter on empty input
>   tools/kvm_stat: add option '--guest'
>   tools/kvm_stat: add interactive command 'c'
>   tools/kvm_stat: add interactive command 'r'
>   tools/kvm_stat: add '%Total' column
> 
>  tools/kvm/kvm_stat/kvm_stat     | 383 +++++++++++++++++++++++++++++++++-------
>  tools/kvm/kvm_stat/kvm_stat.txt |  26 +++
>  2 files changed, 343 insertions(+), 66 deletions(-)
> 

Re: [PATCH 00/17] tools/kvm_stat: Misc Patches

[Paolo Bonzini]

On 06/03/2017 09:08, Stefan Raspl wrote:
> *ping*

It's on the to-do list, but typically only very urgent patches get
reviewed during the merge window.

Paolo

> 
> On 20.02.2017 16:41, Stefan Raspl wrote:
>> Assortment of various fixes and improvements for the kvm_stat utility.
>> Patches 1-5 are bugfixes.
>> Patch 6 provides full PEP8 compliance by inserting the required empty
>> lines. It is included as an extra for easy disposal, since that kind of
>> patch might be a bit controversial.
>> There are some subtle aspects regarding patch 7, as its effects might
>> differ per platform. However, I didn't notice any negative effects, only
>> improvements.
>> Patch 8 is a simple doc update.
>> Patches 9-13 provide usability improvements.
>> Patches 14-17 add new features.
>>
>> Signed-off-by: Stefan Raspl <raspl@linux.vnet.ibm.com>
>>
>> ---
>>
>> Stefan Raspl (17):
>>   tools/kvm_stat: hide cursor
>>   tools/kvm_stat: catch curses exceptions only
>>   tools/kvm_stat: handle SIGINT in log and batch modes
>>   tools/kvm_stat: fix misc glitches
>>   tools/kvm_stat: fix trace setup glitch on field updates in
>>     TracepointProvider
>>   tools/kvm_stat: full PEP8 compliance
>>   tools/kvm_stat: reduce perceived idle time on filter updates
>>   tools/kvm_stat: document list of interactive commands
>>   tools/kvm_stat: display guest name when using pid filter
>>   tools/kvm_stat: remove pid filter on empty input
>>   tools/kvm_stat: print error messages on faulty pid filter input
>>   tools/kvm_stat: display regex when set to non-default
>>   tools/kvm_stat: remove regex filter on empty input
>>   tools/kvm_stat: add option '--guest'
>>   tools/kvm_stat: add interactive command 'c'
>>   tools/kvm_stat: add interactive command 'r'
>>   tools/kvm_stat: add '%Total' column
>>
>>  tools/kvm/kvm_stat/kvm_stat     | 383 +++++++++++++++++++++++++++++++++-------
>>  tools/kvm/kvm_stat/kvm_stat.txt |  26 +++
>>  2 files changed, 343 insertions(+), 66 deletions(-)
>>
> 

Re: [PATCH 04/17] tools/kvm_stat: fix misc glitches

[Paolo Bonzini]

On 20/02/2017 16:41, Stefan Raspl wrote:
> @@ -339,8 +338,7 @@ def get_filters():
>          filters['kvm_exit'] = ('exit_reason', ARCH.exit_reasons)
>      return filters
>  
> -libc = ctypes.CDLL('libc.so.6', use_errno=True)
> -syscall = libc.syscall
> +syscall = ctypes.CDLL('libc.so.6', use_errno=True).syscall
>  
>  class perf_event_attr(ctypes.Structure):
>      """Struct that holds the necessary data to set up a trace event.
> @@ -950,11 +948,10 @@ class Tui(object):
>          while True:
>              self.refresh(sleeptime)
>              curses.halfdelay(int(sleeptime * 10))
> -            sleeptime = 3
> +            sleeptime = 3.
>              try:
>                  char = self.screen.getkey()
>                  if char == 'x':
> -                    self.drilldown = not self.drilldown
>                      self.update_drilldown()
>                  if char == 'q':
>                      break

I'm not sure I understand the point of these; the rest is fine.

Paolo

Re: [PATCH 07/17] tools/kvm_stat: reduce perceived idle time on filter updates

[Paolo Bonzini]

On 20/02/2017 16:42, Stefan Raspl wrote:
> Whenever a user adds a filter, we
> * redraw the header immediately for a snappy response
> * print a message indicating to the user that we're busy while the
>   noticeable delay induced by updating all of the stats objects takes place
> * update the statistics ASAP (i.e. after 0.25s instead of 3s) to be
>   consistent with behavior on startup
> To do so, we split the Tui's refresh() method to allow for drawing header
> and stats separately, and trigger a header refresh whenever we are about
> to do something that takes a while - like updating filters.
> 
> Signed-off-by: Stefan Raspl <raspl@linux.vnet.ibm.com>
> ---
>  tools/kvm/kvm_stat/kvm_stat | 48 ++++++++++++++++++++++++++++-----------------
>  1 file changed, 30 insertions(+), 18 deletions(-)
> 
> diff --git a/tools/kvm/kvm_stat/kvm_stat b/tools/kvm/kvm_stat/kvm_stat
> index e0503c8..eeb5fe5 100755
> --- a/tools/kvm/kvm_stat/kvm_stat
> +++ b/tools/kvm/kvm_stat/kvm_stat
> @@ -800,6 +800,8 @@ class Stats(object):
>  
>  LABEL_WIDTH = 40
>  NUMBER_WIDTH = 10
> +DELAY_INITIAL = 0.25
> +DELAY_REGULAR = 3.

Same here, the float constant isn't strictly necessary and the trailing
dot is somewhat distracting.

Nothing to complain about the change, which is a nice touch.

Paolo

>  
>  class Tui(object):

Re: [PATCH 09/17] tools/kvm_stat: display guest name when using pid filter

[Paolo Bonzini]

On 20/02/2017 16:42, Stefan Raspl wrote:
> +def get_gname_from_pid(pid):
> +    """Returns the guest name for a QEMU process pid.
> +
> +    Extracts the guest name from the QEMU comma line by processing the '-name'
> +    option. Will also handle names specified out of sequence.
> +
> +    """
> +    name = ''
> +    try:
> +        line = open('/proc/{}/cmdline'.format(pid), 'rb').read().split('\0')
> +        parms = line[line.index('-name') + 1].split(',')
> +        while '' in parms:
> +            # commas are escaped (i.e. ',,'), hence e.g. 'foo,bar' results in
> +            # ['foo', '', 'bar'], which we revert here
> +            idx = parms.index('')
> +            parms[idx - 1] += ',' + parms[idx + 1]
> +            del parms[idx:idx+2]
> +        # the '-name' switch allows for two ways to specify the guest name,
> +        # where the plain name overrides the name specified via 'guest='
> +        for arg in parms:
> +            if '=' not in arg:
> +                name = arg
> +                break
> +            if arg[:6] == 'guest=':
> +                name = arg[6:]
> +    except (ValueError, IOError, IndexError):
> +        pass
> +
> +    return name
> +
> +

That's as hideous as it is useful! :)

Paolo

Re: [PATCH 00/17] tools/kvm_stat: Misc Patches

[Paolo Bonzini]

On 20/02/2017 16:41, Stefan Raspl wrote:
> Assortment of various fixes and improvements for the kvm_stat utility.
> Patches 1-5 are bugfixes.
> Patch 6 provides full PEP8 compliance by inserting the required empty
> lines. It is included as an extra for easy disposal, since that kind of
> patch might be a bit controversial.
> There are some subtle aspects regarding patch 7, as its effects might
> differ per platform. However, I didn't notice any negative effects, only
> improvements.
> Patch 8 is a simple doc update.
> Patches 9-13 provide usability improvements.
> Patches 14-17 add new features.
> 
> Signed-off-by: Stefan Raspl <raspl@linux.vnet.ibm.com>

Really nice -- just two very small comments.

The hard-coded knowledge of QEMU is not the nicest thing (after all
kvm_stat is part of the Linux repository), but it is useful so I'm not
going to complain too loudly.

Paolo

Re: [PATCH 04/17] tools/kvm_stat: fix misc glitches

[Stefan Raspl]

On 09.03.2017 17:51, Paolo Bonzini wrote:
> 
> 
> On 20/02/2017 16:41, Stefan Raspl wrote:
>> @@ -339,8 +338,7 @@ def get_filters():
>>          filters['kvm_exit'] = ('exit_reason', ARCH.exit_reasons)
>>      return filters
>>  
>> -libc = ctypes.CDLL('libc.so.6', use_errno=True)
>> -syscall = libc.syscall
>> +syscall = ctypes.CDLL('libc.so.6', use_errno=True).syscall
>>  
>>  class perf_event_attr(ctypes.Structure):
>>      """Struct that holds the necessary data to set up a trace event.
>> @@ -950,11 +948,10 @@ class Tui(object):
>>          while True:
>>              self.refresh(sleeptime)
>>              curses.halfdelay(int(sleeptime * 10))
>> -            sleeptime = 3
>> +            sleeptime = 3.
>>              try:
>>                  char = self.screen.getkey()
>>                  if char == 'x':
>> -                    self.drilldown = not self.drilldown
>>                      self.update_drilldown()
>>                  if char == 'q':
>>                      break
> 
> I'm not sure I understand the point of these; the rest is fine.

'sleeptime' starts out as a float (sleeptime = 0.25), but is here
re-defined to an int - so we make it float all the way.
The variable 'drilldown' is never used, so we remove its
initialization in __init__() and the sole place where it is ever
used, which is the line above.

Ciao,
Stefan

Re: [PATCH 09/17] tools/kvm_stat: display guest name when using pid filter

[Stefan Raspl]

On 09.03.2017 17:54, Paolo Bonzini wrote:
> 
> 
> On 20/02/2017 16:42, Stefan Raspl wrote:
>> +def get_gname_from_pid(pid):
>> +    """Returns the guest name for a QEMU process pid.
>> +
>> +    Extracts the guest name from the QEMU comma line by processing the '-name'
>> +    option. Will also handle names specified out of sequence.
>> +
>> +    """
>> +    name = ''
>> +    try:
>> +        line = open('/proc/{}/cmdline'.format(pid), 'rb').read().split('\0')
>> +        parms = line[line.index('-name') + 1].split(',')
>> +        while '' in parms:
>> +            # commas are escaped (i.e. ',,'), hence e.g. 'foo,bar' results in
>> +            # ['foo', '', 'bar'], which we revert here
>> +            idx = parms.index('')
>> +            parms[idx - 1] += ',' + parms[idx + 1]
>> +            del parms[idx:idx+2]
>> +        # the '-name' switch allows for two ways to specify the guest name,
>> +        # where the plain name overrides the name specified via 'guest='
>> +        for arg in parms:
>> +            if '=' not in arg:
>> +                name = arg
>> +                break
>> +            if arg[:6] == 'guest=':
>> +                name = arg[6:]
>> +    except (ValueError, IOError, IndexError):
>> +        pass
>> +
>> +    return name
>> +
>> +
> 
> That's as hideous as it is useful! :)

I'm not going to argue with that!

Ciao,
Stefan

Re: [PATCH 00/17] tools/kvm_stat: Misc Patches

[Stefan Raspl]

On 09.03.2017 18:00, Paolo Bonzini wrote:
> 
> 
> On 20/02/2017 16:41, Stefan Raspl wrote:
>> Assortment of various fixes and improvements for the kvm_stat utility.
>> Patches 1-5 are bugfixes.
>> Patch 6 provides full PEP8 compliance by inserting the required empty
>> lines. It is included as an extra for easy disposal, since that kind of
>> patch might be a bit controversial.
>> There are some subtle aspects regarding patch 7, as its effects might
>> differ per platform. However, I didn't notice any negative effects, only
>> improvements.
>> Patch 8 is a simple doc update.
>> Patches 9-13 provide usability improvements.
>> Patches 14-17 add new features.
>>
>> Signed-off-by: Stefan Raspl <raspl@linux.vnet.ibm.com>
> 
> Really nice -- just two very small comments.
> 
> The hard-coded knowledge of QEMU is not the nicest thing (after all
> kvm_stat is part of the Linux repository), but it is useful so I'm not
> going to complain too loudly.

Thx for the credit :)
Have to admit that I totally missed the QEMU-only aspect. Still, if
somebody
is running something else, he'll get no result/a proper hint, so we
should be
fine.

Ciao,
Stefan

Re: [PATCH 04/17] tools/kvm_stat: fix misc glitches

[Paolo Bonzini]

----- Original Message -----
> From: "Stefan Raspl" <raspl@linux.vnet.ibm.com>
> To: "Paolo Bonzini" <pbonzini@redhat.com>, kvm@vger.kernel.org
> Cc: rkrcmar@redhat.com, frankja@linux.vnet.ibm.com
> Sent: Friday, March 10, 2017 7:04:46 AM
> Subject: Re: [PATCH 04/17] tools/kvm_stat: fix misc glitches
> 
> On 09.03.2017 17:51, Paolo Bonzini wrote:
> > 
> > 
> > On 20/02/2017 16:41, Stefan Raspl wrote:
> >> @@ -339,8 +338,7 @@ def get_filters():
> >>          filters['kvm_exit'] = ('exit_reason', ARCH.exit_reasons)
> >>      return filters
> >>  
> >> -libc = ctypes.CDLL('libc.so.6', use_errno=True)
> >> -syscall = libc.syscall
> >> +syscall = ctypes.CDLL('libc.so.6', use_errno=True).syscall
> >>  
> >>  class perf_event_attr(ctypes.Structure):
> >>      """Struct that holds the necessary data to set up a trace event.
> >> @@ -950,11 +948,10 @@ class Tui(object):
> >>          while True:
> >>              self.refresh(sleeptime)
> >>              curses.halfdelay(int(sleeptime * 10))
> >> -            sleeptime = 3
> >> +            sleeptime = 3.
> >>              try:
> >>                  char = self.screen.getkey()
> >>                  if char == 'x':
> >> -                    self.drilldown = not self.drilldown
> >>                      self.update_drilldown()
> >>                  if char == 'q':
> >>                      break
> > 
> > I'm not sure I understand the point of these; the rest is fine.
> 
> 'sleeptime' starts out as a float (sleeptime = 0.25), but is here
> re-defined to an int - so we make it float all the way.
> The variable 'drilldown' is never used, so we remove its
> initialization in __init__() and the sole place where it is ever
> used, which is the line above.

Yes, I was referring to libc and sleeptime.  I don't like the
"3.", especially since Python 3 has "3/2" return a float.  Is
this a PEP8 complaint?  Does it still complain if you do
"from __future__ import division"?

Paolo

Re: [PATCH 04/17] tools/kvm_stat: fix misc glitches

[Stefan Raspl]

On 10.03.2017 09:14, Paolo Bonzini wrote:
> 
> 
> ----- Original Message -----
>> From: "Stefan Raspl" <raspl@linux.vnet.ibm.com>
>> To: "Paolo Bonzini" <pbonzini@redhat.com>, kvm@vger.kernel.org
>> Cc: rkrcmar@redhat.com, frankja@linux.vnet.ibm.com
>> Sent: Friday, March 10, 2017 7:04:46 AM
>> Subject: Re: [PATCH 04/17] tools/kvm_stat: fix misc glitches
>>
>> On 09.03.2017 17:51, Paolo Bonzini wrote:
>>>
>>>
>>> On 20/02/2017 16:41, Stefan Raspl wrote:
>>>> @@ -339,8 +338,7 @@ def get_filters():
>>>>          filters['kvm_exit'] = ('exit_reason', ARCH.exit_reasons)
>>>>      return filters
>>>>  
>>>> -libc = ctypes.CDLL('libc.so.6', use_errno=True)
>>>> -syscall = libc.syscall
>>>> +syscall = ctypes.CDLL('libc.so.6', use_errno=True).syscall
>>>>  
>>>>  class perf_event_attr(ctypes.Structure):
>>>>      """Struct that holds the necessary data to set up a trace event.
>>>> @@ -950,11 +948,10 @@ class Tui(object):
>>>>          while True:
>>>>              self.refresh(sleeptime)
>>>>              curses.halfdelay(int(sleeptime * 10))
>>>> -            sleeptime = 3
>>>> +            sleeptime = 3.
>>>>              try:
>>>>                  char = self.screen.getkey()
>>>>                  if char == 'x':
>>>> -                    self.drilldown = not self.drilldown
>>>>                      self.update_drilldown()
>>>>                  if char == 'q':
>>>>                      break
>>>
>>> I'm not sure I understand the point of these; the rest is fine.
>>
>> 'sleeptime' starts out as a float (sleeptime = 0.25), but is here
>> re-defined to an int - so we make it float all the way.
>> The variable 'drilldown' is never used, so we remove its
>> initialization in __init__() and the sole place where it is ever
>> used, which is the line above.
> 
> Yes, I was referring to libc and sleeptime.  I don't like the
> "3.", especially since Python 3 has "3/2" return a float.  Is
> this a PEP8 complaint?  Does it still complain if you do
> "from __future__ import division"?


Ah, sorry, missed the first chunk: That was to eliminate unused
variable 'libc'.
As for the "3.": I noticed through pylint, PEP8 is fine. Seemed
valid to me, but in the grand scheme of things it certainly doesn't
matter (and wasn't aware it becomes moot in Python 3), so I'd take
it out if you want.
Sidenote: I only ever hear the complaint about the '3.' from kernel
folks, seemed quite common to me elsewhere - maybe 'cause there's
usually no floats in the kernel?! But as I said, I'd rather take it
out altogether in here instead of adding more imports, and fix 07/17
to redefine to 3 (instead of 3.), OK?

Ciao,
Stefan

Re: [PATCH 04/17] tools/kvm_stat: fix misc glitches

[Paolo Bonzini]

On 10/03/2017 09:33, Stefan Raspl wrote:
> On 10.03.2017 09:14, Paolo Bonzini wrote:
>>
>>
>> ----- Original Message -----
>>> From: "Stefan Raspl" <raspl@linux.vnet.ibm.com>
>>> To: "Paolo Bonzini" <pbonzini@redhat.com>, kvm@vger.kernel.org
>>> Cc: rkrcmar@redhat.com, frankja@linux.vnet.ibm.com
>>> Sent: Friday, March 10, 2017 7:04:46 AM
>>> Subject: Re: [PATCH 04/17] tools/kvm_stat: fix misc glitches
>>>
>>> On 09.03.2017 17:51, Paolo Bonzini wrote:
>>>>
>>>>
>>>> On 20/02/2017 16:41, Stefan Raspl wrote:
>>>>> @@ -339,8 +338,7 @@ def get_filters():
>>>>>          filters['kvm_exit'] = ('exit_reason', ARCH.exit_reasons)
>>>>>      return filters
>>>>>  
>>>>> -libc = ctypes.CDLL('libc.so.6', use_errno=True)
>>>>> -syscall = libc.syscall
>>>>> +syscall = ctypes.CDLL('libc.so.6', use_errno=True).syscall
>>>>>  
>>>>>  class perf_event_attr(ctypes.Structure):
>>>>>      """Struct that holds the necessary data to set up a trace event.
>>>>> @@ -950,11 +948,10 @@ class Tui(object):
>>>>>          while True:
>>>>>              self.refresh(sleeptime)
>>>>>              curses.halfdelay(int(sleeptime * 10))
>>>>> -            sleeptime = 3
>>>>> +            sleeptime = 3.
>>>>>              try:
>>>>>                  char = self.screen.getkey()
>>>>>                  if char == 'x':
>>>>> -                    self.drilldown = not self.drilldown
>>>>>                      self.update_drilldown()
>>>>>                  if char == 'q':
>>>>>                      break
>>>>
>>>> I'm not sure I understand the point of these; the rest is fine.
>>>
>>> 'sleeptime' starts out as a float (sleeptime = 0.25), but is here
>>> re-defined to an int - so we make it float all the way.
>>> The variable 'drilldown' is never used, so we remove its
>>> initialization in __init__() and the sole place where it is ever
>>> used, which is the line above.
>>
>> Yes, I was referring to libc and sleeptime.  I don't like the
>> "3.", especially since Python 3 has "3/2" return a float.  Is
>> this a PEP8 complaint?  Does it still complain if you do
>> "from __future__ import division"?
> 
> 
> Ah, sorry, missed the first chunk: That was to eliminate unused
> variable 'libc'.

It's not unused, it's used on the following line. :)

> As for the "3.": I noticed through pylint, PEP8 is fine. Seemed
> valid to me, but in the grand scheme of things it certainly doesn't
> matter (and wasn't aware it becomes moot in Python 3), so I'd take
> it out if you want.

I think it's a valid complaint with Python 2 division.  If pylint
complaints even with "from __future__ import division", it would be a
pylint bug in my opinion.

Maybe it's just me... I wouldn't have complained about "3.0" for example. :)

Paolo

> Sidenote: I only ever hear the complaint about the '3.' from kernel
> folks, seemed quite common to me elsewhere - maybe 'cause there's
> usually no floats in the kernel?! But as I said, I'd rather take it
> out altogether in here instead of adding more imports, and fix 07/17
> to redefine to 3 (instead of 3.), OK?

Re: [PATCH 04/17] tools/kvm_stat: fix misc glitches

[Stefan Raspl]

On 10.03.2017 09:38, Paolo Bonzini wrote:
> 
> 
> On 10/03/2017 09:33, Stefan Raspl wrote:
>> On 10.03.2017 09:14, Paolo Bonzini wrote:
>>>
>>>
>>> ----- Original Message -----
>>>> From: "Stefan Raspl" <raspl@linux.vnet.ibm.com>
>>>> To: "Paolo Bonzini" <pbonzini@redhat.com>, kvm@vger.kernel.org
>>>> Cc: rkrcmar@redhat.com, frankja@linux.vnet.ibm.com
>>>> Sent: Friday, March 10, 2017 7:04:46 AM
>>>> Subject: Re: [PATCH 04/17] tools/kvm_stat: fix misc glitches
>>>>
>>>> On 09.03.2017 17:51, Paolo Bonzini wrote:
>>>>>
>>>>>
>>>>> On 20/02/2017 16:41, Stefan Raspl wrote:
>>>>>> @@ -339,8 +338,7 @@ def get_filters():
>>>>>>          filters['kvm_exit'] = ('exit_reason', ARCH.exit_reasons)
>>>>>>      return filters
>>>>>>  
>>>>>> -libc = ctypes.CDLL('libc.so.6', use_errno=True)
>>>>>> -syscall = libc.syscall
>>>>>> +syscall = ctypes.CDLL('libc.so.6', use_errno=True).syscall
>>>>>>  
>>>>>>  class perf_event_attr(ctypes.Structure):
>>>>>>      """Struct that holds the necessary data to set up a trace event.
>>>>>> @@ -950,11 +948,10 @@ class Tui(object):
>>>>>>          while True:
>>>>>>              self.refresh(sleeptime)
>>>>>>              curses.halfdelay(int(sleeptime * 10))
>>>>>> -            sleeptime = 3
>>>>>> +            sleeptime = 3.
>>>>>>              try:
>>>>>>                  char = self.screen.getkey()
>>>>>>                  if char == 'x':
>>>>>> -                    self.drilldown = not self.drilldown
>>>>>>                      self.update_drilldown()
>>>>>>                  if char == 'q':
>>>>>>                      break
>>>>>
>>>>> I'm not sure I understand the point of these; the rest is fine.
>>>>
>>>> 'sleeptime' starts out as a float (sleeptime = 0.25), but is here
>>>> re-defined to an int - so we make it float all the way.
>>>> The variable 'drilldown' is never used, so we remove its
>>>> initialization in __init__() and the sole place where it is ever
>>>> used, which is the line above.
>>>
>>> Yes, I was referring to libc and sleeptime.  I don't like the
>>> "3.", especially since Python 3 has "3/2" return a float.  Is
>>> this a PEP8 complaint?  Does it still complain if you do
>>> "from __future__ import division"?
>>
>>
>> Ah, sorry, missed the first chunk: That was to eliminate unused
>> variable 'libc'.
> 
> It's not unused, it's used on the following line. :)

Sorry, don't want to get annoying: Yeah, right, so I was eliminating
that variable - do you want me to remove the chunk from the patch?

>> As for the "3.": I noticed through pylint, PEP8 is fine. Seemed
>> valid to me, but in the grand scheme of things it certainly doesn't
>> matter (and wasn't aware it becomes moot in Python 3), so I'd take
>> it out if you want.
> 
> I think it's a valid complaint with Python 2 division.  If pylint
> complaints even with "from __future__ import division", it would be a
> pylint bug in my opinion.
> 
> Maybe it's just me... I wouldn't have complained about "3.0" for example. :)

Tried the import, pylint still complains :O
So: Just switch to 3.0 here and in 07/17, and repost?

Ciao,
Stefan

Re: [PATCH 04/17] tools/kvm_stat: fix misc glitches

[Paolo Bonzini]

On 10/03/2017 10:42, Stefan Raspl wrote:
>>> Ah, sorry, missed the first chunk: That was to eliminate unused
>>> variable 'libc'.
>> It's not unused, it's used on the following line. :)
> Sorry, don't want to get annoying: Yeah, right, so I was eliminating
> that variable - do you want me to remove the chunk from the patch?

Yes, please.

>>> As for the "3.": I noticed through pylint, PEP8 is fine. Seemed
>>> valid to me, but in the grand scheme of things it certainly doesn't
>>> matter (and wasn't aware it becomes moot in Python 3), so I'd take
>>> it out if you want.
>> I think it's a valid complaint with Python 2 division.  If pylint
>> complaints even with "from __future__ import division", it would be a
>> pylint bug in my opinion.
>>
>> Maybe it's just me... I wouldn't have complained about "3.0" for example. :)
> Tried the import, pylint still complains :O
> So: Just switch to 3.0 here and in 07/17, and repost?

And this too.

Thanks,

Paolo

Re: [PATCH 16/17] tools/kvm_stat: add interactive command 'r'

[Janosch Frank]

[-- Attachment #1.1: Type: text/plain, Size: 3188 bytes --]

On 20.02.2017 16:42, Stefan Raspl wrote:
> Provide an interactive command to reset the tracepoint statistics.
> Requires some extra work for debugfs, as the counters cannot be reset.
> On the up side, offers us the opportunity to have debugfs values reset
                  ^
                  add "this"

> on startup and whenever a filter is modified, becoming consistent with
> the tracepoint provider. As a bonus, 'kvmstat -dt' will now provide
> useful output, instead of mixing values in totally different orders of
> magnitude.
> Furthermore, we avoid unnecessary resets when any of the filters is
> "changed" interactively to the previous value.

Acked-by: Janosch Frank <frankja@linux.vnet.ibm.com>

> 
> Signed-off-by: Stefan Raspl <raspl@linux.vnet.ibm.com>
> ---
>  tools/kvm/kvm_stat/kvm_stat     | 66 +++++++++++++++++++++++++++++++----------
>  tools/kvm/kvm_stat/kvm_stat.txt |  2 ++
>  2 files changed, 53 insertions(+), 15 deletions(-)
> 
> diff --git a/tools/kvm/kvm_stat/kvm_stat b/tools/kvm/kvm_stat/kvm_stat
> index e0f28e1..f9f653a 100755
> --- a/tools/kvm/kvm_stat/kvm_stat
> +++ b/tools/kvm/kvm_stat/kvm_stat
> @@ -715,15 +715,23 @@ class TracepointProvider(object):
>                      ret[name] += val
>          return ret
> 
> +    def reset(self):
> +        """Reset all field counters"""
> +        for group in self.group_leaders:
> +            for event in group.events:
> +                event.reset()
> +
> 
>  class DebugfsProvider(object):
>      """Provides data from the files that KVM creates in the kvm debugfs
>      folder."""
>      def __init__(self):
>          self._fields = self.get_available_fields()
> +        self._baseline = {}
>          self._pid = 0
>          self.do_read = True
>          self.paths = []
> +        self.reset()
> 
>      def get_available_fields(self):
>          """"Returns a list of available fields.
> @@ -740,6 +748,7 @@ class DebugfsProvider(object):
>      @fields.setter
>      def fields(self, fields):
>          self._fields = fields
> +        self.reset()
> 
>      @property
>      def pid(self):
> @@ -757,10 +766,11 @@ class DebugfsProvider(object):
>              self.paths = filter(lambda x: "{}-".format(pid) in x, vms)
> 
>          else:
> -            self.paths = ['']
> +            self.paths = []

This is more a cleanup than new code, isn't it?

[...]
> @@ -1188,7 +1224,7 @@ Interactive Commands:
>     g     filter by guest name
>     p     filter by PID
>     q     quit
> -   x     toggle reporting of stats for individual child trace events
> +   r     reset stats
>  Press any other key to refresh statistics immediately.
>  """

This is inconsistent to the part below.

> diff --git a/tools/kvm/kvm_stat/kvm_stat.txt b/tools/kvm/kvm_stat/kvm_stat.txt
> index c3ab6a2..109431b 100644
> --- a/tools/kvm/kvm_stat/kvm_stat.txt
> +++ b/tools/kvm/kvm_stat/kvm_stat.txt
> @@ -39,6 +39,8 @@ INTERACTIVE COMMANDS
> 
>  *q*::	quit
> 
> +*r*::	reset stats
> +
>  *x*::	toggle reporting of stats for child trace events
> 
>  Press any other key to refresh statistics immediately.
> 



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH 14/17] tools/kvm_stat: add option '--guest'

[Janosch Frank]

[-- Attachment #1.1: Type: text/plain, Size: 1055 bytes --]

On 20.02.2017 16:42, Stefan Raspl wrote:
> Add a new option '-g'/'--guest' to select a particular process by providing
> the QEMU guest name.
> Notes:
> - The logic to figure out the pid corresponding to the guest name might look
>   scary, but works pretty reliably in practice; in the unlikely event that it
>   returns add'l flukes, it will bail out and hint at using '-p' instead, no
>   harm done.
> - Mixing '-g' and '-p' is possible, and the final instance specified on the
>   command line is the significant one. This is consistent with current
>   behavior for '-p' which, if specified multiple times, also regards the final
>   instance as the significant one.
> 
> Signed-off-by: Stefan Raspl <raspl@linux.vnet.ibm.com>

This is really helpful, no more extra terminals for searching guest pids
anymore. And I hope qemu will not change the name passing for a very
long time, I want to avoid having to find solutions to such terrible
string parsing problems again.

Reviewed-by: Janosch Frank <frankja@linux.vnet.ibm.com>


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH 16/17] tools/kvm_stat: add interactive command 'r'

[Stefan Raspl]

On 10.03.2017 12:37, Janosch Frank wrote:
>> @@ -757,10 +766,11 @@ class DebugfsProvider(object):
>>              self.paths = filter(lambda x: "{}-".format(pid) in x, vms)
>>
>>          else:
>> -            self.paths = ['']
>> +            self.paths = []
> 
> This is more a cleanup than new code, isn't it?

Not quite: The former holds an element, while the latter doesn't.
Therefore a subsequent 'for path in self.paths:' would execute with
the former version, but won't with the latter.

> [...]
>> @@ -1188,7 +1224,7 @@ Interactive Commands:
>>     g     filter by guest name
>>     p     filter by PID
>>     q     quit
>> -   x     toggle reporting of stats for individual child trace events
>> +   r     reset stats
>>  Press any other key to refresh statistics immediately.
>>  """
> 
> This is inconsistent to the part below.

Yup, accidental deletion - good catch, thanks!

Ciao,
Stefan