[PATCH v1 00/19] tools/kvm_stat: More misc patches

[PATCH v1 00/19] tools/kvm_stat: More misc patches

[Stefan Raspl]

There were leftovers from the previous patch series as suggested by Radim,
so I set out to fix these. Then I got a bit carried away...


Stefan Raspl (19):
  tools/kvm_stat: fix typo
  tools/kvm_stat: fix event counts display for interrupted intervals
  tools/kvm_stat: fix undue use of initial sleeptime
  tools/kvm_stat: remove unnecessary header redraws
  tools/kvm_stat: simplify line print logic
  tools/kvm_stat: removed unused function
  tools/kvm_stat: remove extra statement
  tools/kvm_stat: simplify initializers
  tools/kvm_stat: move functions to corresponding classes
  tools/kvm_stat: show cursor in selection screens
  tools/kvm_stat: display message indicating lack of events
  tools/kvm_stat: make heading look a bit more like 'top'
  tools/kvm_stat: rename 'Current' column to 'CurAvg/s'
  tools/kvm_stat: add new interactive command 'h'
  tools/kvm_stat: add new interactive command 's'
  tools/kvm_stat: add new interactive command 't'
  tools/kvm_stat: add new interactive command 'o'
  tools/kvm_stat: add new interactive command 'b'
  tools/kvm_stat: display guest list in pid/guest selection screens

 tools/kvm/kvm_stat/kvm_stat     | 652 +++++++++++++++++++++++++---------------
 tools/kvm/kvm_stat/kvm_stat.txt |  10 +
 2 files changed, 422 insertions(+), 240 deletions(-)

-- 
2.11.2

[PATCH v1 01/19] tools/kvm_stat: fix typo

[Stefan Raspl]

Signed-off-by: Stefan Raspl <raspl@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 8f74ed8e7237..904eb6214602 100755
--- a/tools/kvm/kvm_stat/kvm_stat
+++ b/tools/kvm/kvm_stat/kvm_stat
@@ -929,7 +929,7 @@ class Tui(object):
         return self
 
     def __exit__(self, *exception):
-        """Resets the terminal to its normal state.  Based on curses.wrappre
+        """Resets the terminal to its normal state.  Based on curses.wrapper
            implementation from the Python standard library."""
         if self.screen:
             self.screen.keypad(0)
-- 
2.11.2

[PATCH v1 02/19] tools/kvm_stat: fix event counts display for interrupted intervals

[Stefan Raspl]

When an update interval is interrupted via key press (e.g. space), the
'Current' column value is calculated using the full interval length
instead of the elapsed time, which leads to lower than actual numbers.
Furthermore, the value should be rounded, not truncated.
This is fixed by using the actual elapsed time for the calculation.

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

diff --git a/tools/kvm/kvm_stat/kvm_stat b/tools/kvm/kvm_stat/kvm_stat
index 904eb6214602..b571584419ae 100755
--- a/tools/kvm/kvm_stat/kvm_stat
+++ b/tools/kvm/kvm_stat/kvm_stat
@@ -1009,7 +1009,8 @@ class Tui(object):
             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,))
+                self.screen.addstr(row, col, '%8d' %
+                                   round(values[1] / sleeptime))
             row += 1
         self.screen.refresh()
 
@@ -1130,9 +1131,11 @@ class Tui(object):
         """Refreshes the screen and processes user input."""
         sleeptime = DELAY_INITIAL
         self.refresh_header()
+        start = 0.0  # result based on init value never appears on screen
         while True:
-            self.refresh_body(sleeptime)
+            self.refresh_body(time.time() - start)
             curses.halfdelay(int(sleeptime * 10))
+            start = time.time()
             sleeptime = DELAY_REGULAR
             try:
                 char = self.screen.getkey()
-- 
2.11.2

[PATCH v1 03/19] tools/kvm_stat: fix undue use of initial sleeptime

[Stefan Raspl]

We should not use the initial sleeptime for any key press that does not
switch to a different screen, as that introduces an unaesthetic flicker due
to two updates in quick succession.

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

diff --git a/tools/kvm/kvm_stat/kvm_stat b/tools/kvm/kvm_stat/kvm_stat
index b571584419ae..6e29e5b072ab 100755
--- a/tools/kvm/kvm_stat/kvm_stat
+++ b/tools/kvm/kvm_stat/kvm_stat
@@ -1142,14 +1142,12 @@ class Tui(object):
                 if char == 'x':
                     self.refresh_header()
                     self.update_drilldown()
-                    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
@@ -1162,7 +1160,6 @@ class Tui(object):
                 if char == 'r':
                     self.refresh_header()
                     self.stats.reset()
-                    sleeptime = DELAY_INITIAL
             except KeyboardInterrupt:
                 break
             except curses.error:
-- 
2.11.2

[PATCH v1 04/19] tools/kvm_stat: remove unnecessary header redraws

[Stefan Raspl]

Certain interactive commands will not modify any information displayed in
the header, hence we can skip them.

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

diff --git a/tools/kvm/kvm_stat/kvm_stat b/tools/kvm/kvm_stat/kvm_stat
index 6e29e5b072ab..d2526b698db4 100755
--- a/tools/kvm/kvm_stat/kvm_stat
+++ b/tools/kvm/kvm_stat/kvm_stat
@@ -1140,7 +1140,6 @@ class Tui(object):
             try:
                 char = self.screen.getkey()
                 if char == 'x':
-                    self.refresh_header()
                     self.update_drilldown()
                 if char == 'q':
                     break
@@ -1158,7 +1157,6 @@ class Tui(object):
                     self.show_vm_selection_by_pid()
                     sleeptime = DELAY_INITIAL
                 if char == 'r':
-                    self.refresh_header()
                     self.stats.reset()
             except KeyboardInterrupt:
                 break
-- 
2.11.2

[PATCH v1 05/19] tools/kvm_stat: simplify line print logic

[Stefan Raspl]

Simplify line print logic for header and data lines in interactive mode
as previously suggested by Radim.
While at it, add a space between the first two columns to avoid the
total bleeding into the event name.
Furthermore, for column 'Current', differentiate between no events being
reported (empty 'Current' column) vs the case where events were reported
but the average was rounded down to zero ('0' in 'Current column), for
the folks who appreciate the difference.
Finally: Only skip events which were not reported at all yet, instead of
events that don't have a value in the current interval.
Considered using constants for the field widths in the format strings.
However, that would make things a bit more complicated, and considering
that there are only two places where output happens, I figured it isn't
worth the trouble.

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

diff --git a/tools/kvm/kvm_stat/kvm_stat b/tools/kvm/kvm_stat/kvm_stat
index d2526b698db4..a527b2fc6685 100755
--- a/tools/kvm/kvm_stat/kvm_stat
+++ b/tools/kvm/kvm_stat/kvm_stat
@@ -887,8 +887,6 @@ class Stats(object):
                 self.values[key] = (newval, newdelta)
         return self.values
 
-LABEL_WIDTH = 40
-NUMBER_WIDTH = 10
 DELAY_INITIAL = 0.25
 DELAY_REGULAR = 3.0
 MAX_GUEST_NAME_LEN = 48
@@ -970,13 +968,8 @@ class Tui(object):
             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')
-        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(2, 1, '%-40s %10s%7s %7s' %
+                           ('Event', 'Total', '%Total', 'Current'))
         self.screen.addstr(4, 1, 'Collecting data...')
         self.screen.refresh()
 
@@ -1001,16 +994,11 @@ class Tui(object):
             values = stats[key]
             if not values[0] and not values[1]:
                 break
-            col = 1
-            self.screen.addstr(row, col, key)
-            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' %
-                                   round(values[1] / sleeptime))
+            if values[0] is not None:
+                cur = int(round(values[1] / sleeptime)) if values[1] else ''
+                self.screen.addstr(row, 1, '%-40s %10d%7.1f %7s' %
+                                   (key, values[0], values[0] * 100 / total,
+                                    cur))
             row += 1
         self.screen.refresh()
 
-- 
2.11.2

[PATCH v1 06/19] tools/kvm_stat: removed unused function

[Stefan Raspl]

Function available_fields() is not used in any place.

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 | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/tools/kvm/kvm_stat/kvm_stat b/tools/kvm/kvm_stat/kvm_stat
index a527b2fc6685..1b8626b42e22 100755
--- a/tools/kvm/kvm_stat/kvm_stat
+++ b/tools/kvm/kvm_stat/kvm_stat
@@ -671,9 +671,6 @@ class TracepointProvider(object):
 
             self.group_leaders.append(group)
 
-    def available_fields(self):
-        return self.get_available_fields()
-
     @property
     def fields(self):
         return self._fields
-- 
2.11.2

[PATCH v1 07/19] tools/kvm_stat: remove extra statement

[Stefan Raspl]

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

diff --git a/tools/kvm/kvm_stat/kvm_stat b/tools/kvm/kvm_stat/kvm_stat
index 1b8626b42e22..e38791ddb37a 100755
--- a/tools/kvm/kvm_stat/kvm_stat
+++ b/tools/kvm/kvm_stat/kvm_stat
@@ -1066,7 +1066,6 @@ class Tui(object):
 
             except ValueError:
                 msg = '"' + str(pid) + '": Not a valid pid'
-                continue
 
     def show_vm_selection_by_guest_name(self):
         """Draws guest selection mask.
-- 
2.11.2

[PATCH v1 08/19] tools/kvm_stat: simplify initializers

[Stefan Raspl]

Simplify a couple of initialization routines:
* TracepointProvider and DebugfsProvider: Pass pid into __init__() instead
  of switching to the requested value in an extra call after initializing
  to the default first.
* Pass a single options object into Stats.__init__(), delaying options
  evaluation accordingly, instead of evaluating options first and passing
  several parts of the options object to Stats.__init__() individually.
* Eliminate Stats.update_provider_pid(), since this 2-line function is now
  used in a single place only.
* Remove extra call to update_drilldown() in Tui.__init__() by getting the
  value of options.fields right initially when parsing options.
* Simplify get_providers() logic.
* Avoid duplicate fields initialization by handling it once in the
  providers' __init__() methods.

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

diff --git a/tools/kvm/kvm_stat/kvm_stat b/tools/kvm/kvm_stat/kvm_stat
index e38791ddb37a..b8522d2ddb0a 100755
--- a/tools/kvm/kvm_stat/kvm_stat
+++ b/tools/kvm/kvm_stat/kvm_stat
@@ -295,6 +295,13 @@ class ArchS390(Arch):
 ARCH = Arch.get_arch()
 
 
+def is_field_wanted(fields_filter, field):
+    """Indicate whether field is valid according to fields_filter."""
+    if not fields_filter:
+        return True
+    return re.match(fields_filter, field) is not None
+
+
 def walkdir(path):
     """Returns os.walk() data for specified directory.
 
@@ -581,11 +588,11 @@ class TracepointProvider(object):
     Manages the events/groups from which it acquires its data.
 
     """
-    def __init__(self):
+    def __init__(self, pid, fields_filter):
         self.group_leaders = []
         self.filters = get_filters()
-        self._fields = self.get_available_fields()
-        self._pid = 0
+        self.update_fields(fields_filter)
+        self.pid = pid
 
     def get_available_fields(self):
         """Returns a list of available event's of format 'event name(filter
@@ -613,6 +620,11 @@ class TracepointProvider(object):
         fields += extra
         return fields
 
+    def update_fields(self, fields_filter):
+        """Refresh fields, applying fields_filter"""
+        self._fields = [field for field in self.get_available_fields()
+                        if is_field_wanted(fields_filter, field)]
+
     def setup_traces(self):
         """Creates all event and group objects needed to be able to retrieve
         data."""
@@ -723,13 +735,12 @@ class TracepointProvider(object):
 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()
+    def __init__(self, pid, fields_filter):
+        self.update_fields(fields_filter)
         self._baseline = {}
-        self._pid = 0
         self.do_read = True
         self.paths = []
-        self.reset()
+        self.pid = pid
 
     def get_available_fields(self):
         """"Returns a list of available fields.
@@ -739,6 +750,11 @@ class DebugfsProvider(object):
         """
         return walkdir(PATH_DEBUGFS_KVM)[2]
 
+    def update_fields(self, fields_filter):
+        """Refresh fields, applying fields_filter"""
+        self._fields = [field for field in self.get_available_fields()
+                        if is_field_wanted(fields_filter, field)]
+
     @property
     def fields(self):
         return self._fields
@@ -754,9 +770,8 @@ class DebugfsProvider(object):
 
     @pid.setter
     def pid(self, pid):
+        self._pid = pid
         if pid != 0:
-            self._pid = pid
-
             vms = walkdir(PATH_DEBUGFS_KVM)[1]
             if len(vms) == 0:
                 self.do_read = False
@@ -818,33 +833,19 @@ class Stats(object):
     provider data.
 
     """
-    def __init__(self, providers, pid, fields=None):
-        self.providers = providers
-        self._pid_filter = pid
-        self._fields_filter = fields
+    def __init__(self, options):
+        self.providers = get_providers(options)
+        self._pid_filter = options.pid
+        self._fields_filter = options.fields
         self.values = {}
-        self.update_provider_pid()
-        self.update_provider_filters()
 
     def update_provider_filters(self):
         """Propagates fields filters to providers."""
-        def wanted(key):
-            if not self._fields_filter:
-                return True
-            return re.match(self._fields_filter, key) is not None
-
         # As we reset the counters when updating the fields we can
         # also clear the cache of old values.
         self.values = {}
         for provider in self.providers:
-            provider_fields = [key for key in provider.get_available_fields()
-                               if wanted(key)]
-            provider.fields = provider_fields
-
-    def update_provider_pid(self):
-        """Propagates pid filters to providers."""
-        for provider in self.providers:
-            provider.pid = self._pid_filter
+            provider.update_fields(self._fields_filter)
 
     def reset(self):
         self.values = {}
@@ -870,7 +871,8 @@ class Stats(object):
         if pid != self._pid_filter:
             self._pid_filter = pid
             self.values = {}
-            self.update_provider_pid()
+            for provider in self.providers:
+                provider.pid = self._pid_filter
 
     def get(self):
         """Returns a dict with field -> (value, delta to last value) of all
@@ -896,7 +898,6 @@ class Tui(object):
     def __init__(self, stats):
         self.stats = stats
         self.screen = None
-        self.update_drilldown()
 
     def __enter__(self):
         """Initialises curses for later use.  Based on curses.wrapper
@@ -1270,7 +1271,7 @@ Press any other key to refresh statistics immediately.
                          )
     optparser.add_option('-f', '--fields',
                          action='store',
-                         default=None,
+                         default=DEFAULT_REGEX,
                          dest='fields',
                          help='fields to display (regex)',
                          )
@@ -1297,12 +1298,10 @@ def get_providers(options):
     """Returns a list of data providers depending on the passed options."""
     providers = []
 
-    if options.tracepoints:
-        providers.append(TracepointProvider())
     if options.debugfs:
-        providers.append(DebugfsProvider())
-    if len(providers) == 0:
-        providers.append(TracepointProvider())
+        providers.append(DebugfsProvider(options.pid, options.fields))
+    if options.tracepoints or not providers:
+        providers.append(TracepointProvider(options.pid, options.fields))
 
     return providers
 
@@ -1347,8 +1346,7 @@ def main():
         sys.stderr.write('Did you use a (unsupported) tid instead of a pid?\n')
         sys.exit('Specified pid does not exist.')
 
-    providers = get_providers(options)
-    stats = Stats(providers, options.pid, fields=options.fields)
+    stats = Stats(options)
 
     if options.log:
         log(stats)
-- 
2.11.2

[PATCH v1 09/19] tools/kvm_stat: move functions to corresponding classes

[Stefan Raspl]

Quite a few of the functions are used only in a single class. Moving
functions accordingly to improve the overall structure.
Furthermore, introduce a base class for the providers, which might also
come handy for future extensions.

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

diff --git a/tools/kvm/kvm_stat/kvm_stat b/tools/kvm/kvm_stat/kvm_stat
index b8522d2ddb0a..f81ed208307f 100755
--- a/tools/kvm/kvm_stat/kvm_stat
+++ b/tools/kvm/kvm_stat/kvm_stat
@@ -295,121 +295,6 @@ class ArchS390(Arch):
 ARCH = Arch.get_arch()
 
 
-def is_field_wanted(fields_filter, field):
-    """Indicate whether field is valid according to fields_filter."""
-    if not fields_filter:
-        return True
-    return re.match(fields_filter, field) is not None
-
-
-def walkdir(path):
-    """Returns os.walk() data for specified directory.
-
-    As it is only a wrapper it returns the same 3-tuple of (dirpath,
-    dirnames, filenames).
-    """
-    return next(os.walk(path))
-
-
-def parse_int_list(list_string):
-    """Returns an int list from a string of comma separated integers and
-    integer ranges."""
-    integers = []
-    members = list_string.split(',')
-
-    for member in members:
-        if '-' not in member:
-            integers.append(int(member))
-        else:
-            int_range = member.split('-')
-            integers.extend(range(int(int_range[0]),
-                                  int(int_range[1]) + 1))
-
-    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.
-
-    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:
-        cpu_string = cpu_list.readline()
-        return parse_int_list(cpu_string)
-
-
-def get_filters():
-    """Returns a dict of trace events, their filter ids and
-    the values that can be filtered.
-
-    Trace events can be filtered for special values by setting a
-    filter string via an ioctl. The string normally has the format
-    identifier==value. For each filter a new event will be created, to
-    be able to distinguish the events.
-
-    """
-    filters = {}
-    filters['kvm_userspace_exit'] = ('reason', USERSPACE_EXIT_REASONS)
-    if ARCH.exit_reasons:
-        filters['kvm_exit'] = ('exit_reason', ARCH.exit_reasons)
-    return filters
-
-libc = ctypes.CDLL('libc.so.6', use_errno=True)
-syscall = libc.syscall
-
-
 class perf_event_attr(ctypes.Structure):
     """Struct that holds the necessary data to set up a trace event.
 
@@ -439,25 +324,6 @@ class perf_event_attr(ctypes.Structure):
         self.read_format = PERF_FORMAT_GROUP
 
 
-def perf_event_open(attr, pid, cpu, group_fd, flags):
-    """Wrapper for the sys_perf_evt_open() syscall.
-
-    Used to set up performance events, returns a file descriptor or -1
-    on error.
-
-    Attributes are:
-    - syscall number
-    - struct perf_event_attr *
-    - pid or -1 to monitor all pids
-    - cpu number or -1 to monitor all cpus
-    - The file descriptor of the group leader or -1 to create a group.
-    - flags
-
-    """
-    return syscall(ARCH.sc_perf_evt_open, ctypes.pointer(attr),
-                   ctypes.c_int(pid), ctypes.c_int(cpu),
-                   ctypes.c_int(group_fd), ctypes.c_long(flags))
-
 PERF_TYPE_TRACEPOINT = 2
 PERF_FORMAT_GROUP = 1 << 3
 
@@ -502,6 +368,8 @@ class Event(object):
     """Represents a performance event and manages its life cycle."""
     def __init__(self, name, group, trace_cpu, trace_pid, trace_point,
                  trace_filter, trace_set='kvm'):
+        self.libc = ctypes.CDLL('libc.so.6', use_errno=True)
+        self.syscall = self.libc.syscall
         self.name = name
         self.fd = None
         self.setup_event(group, trace_cpu, trace_pid, trace_point,
@@ -518,6 +386,25 @@ class Event(object):
         if self.fd:
             os.close(self.fd)
 
+    def perf_event_open(self, attr, pid, cpu, group_fd, flags):
+        """Wrapper for the sys_perf_evt_open() syscall.
+
+        Used to set up performance events, returns a file descriptor or -1
+        on error.
+
+        Attributes are:
+        - syscall number
+        - struct perf_event_attr *
+        - pid or -1 to monitor all pids
+        - cpu number or -1 to monitor all cpus
+        - The file descriptor of the group leader or -1 to create a group.
+        - flags
+
+        """
+        return self.syscall(ARCH.sc_perf_evt_open, ctypes.pointer(attr),
+                            ctypes.c_int(pid), ctypes.c_int(cpu),
+                            ctypes.c_int(group_fd), ctypes.c_long(flags))
+
     def setup_event_attribute(self, trace_set, trace_point):
         """Returns an initialized ctype perf_event_attr struct."""
 
@@ -546,8 +433,8 @@ class Event(object):
         if group.events:
             group_leader = group.events[0].fd
 
-        fd = perf_event_open(event_attr, trace_pid,
-                             trace_cpu, group_leader, 0)
+        fd = self.perf_event_open(event_attr, trace_pid,
+                                  trace_cpu, group_leader, 0)
         if fd == -1:
             err = ctypes.get_errno()
             raise OSError(err, os.strerror(err),
@@ -582,7 +469,26 @@ class Event(object):
         fcntl.ioctl(self.fd, ARCH.ioctl_numbers['RESET'], 0)
 
 
-class TracepointProvider(object):
+class Provider(object):
+    """Encapsulates functionalities used by all providers."""
+    @staticmethod
+    def is_field_wanted(fields_filter, field):
+        """Indicate whether field is valid according to fields_filter."""
+        if not fields_filter:
+            return True
+        return re.match(fields_filter, field) is not None
+
+    @staticmethod
+    def walkdir(path):
+        """Returns os.walk() data for specified directory.
+
+        As it is only a wrapper it returns the same 3-tuple of (dirpath,
+        dirnames, filenames).
+        """
+        return next(os.walk(path))
+
+
+class TracepointProvider(Provider):
     """Data provider for the stats class.
 
     Manages the events/groups from which it acquires its data.
@@ -590,10 +496,27 @@ class TracepointProvider(object):
     """
     def __init__(self, pid, fields_filter):
         self.group_leaders = []
-        self.filters = get_filters()
+        self.filters = self.get_filters()
         self.update_fields(fields_filter)
         self.pid = pid
 
+    @staticmethod
+    def get_filters():
+        """Returns a dict of trace events, their filter ids and
+        the values that can be filtered.
+
+        Trace events can be filtered for special values by setting a
+        filter string via an ioctl. The string normally has the format
+        identifier==value. For each filter a new event will be created, to
+        be able to distinguish the events.
+
+        """
+        filters = {}
+        filters['kvm_userspace_exit'] = ('reason', USERSPACE_EXIT_REASONS)
+        if ARCH.exit_reasons:
+            filters['kvm_exit'] = ('exit_reason', ARCH.exit_reasons)
+        return filters
+
     def get_available_fields(self):
         """Returns a list of available event's of format 'event name(filter
         name)'.
@@ -610,7 +533,7 @@ class TracepointProvider(object):
 
         """
         path = os.path.join(PATH_DEBUGFS_TRACING, 'events', 'kvm')
-        fields = walkdir(path)[1]
+        fields = self.walkdir(path)[1]
         extra = []
         for field in fields:
             if field in self.filters:
@@ -623,7 +546,30 @@ class TracepointProvider(object):
     def update_fields(self, fields_filter):
         """Refresh fields, applying fields_filter"""
         self._fields = [field for field in self.get_available_fields()
-                        if is_field_wanted(fields_filter, field)]
+                        if self.is_field_wanted(fields_filter, field)]
+
+    @staticmethod
+    def get_online_cpus():
+        """Returns a list of cpu id integers."""
+        def parse_int_list(list_string):
+            """Returns an int list from a string of comma separated integers and
+            integer ranges."""
+            integers = []
+            members = list_string.split(',')
+
+            for member in members:
+                if '-' not in member:
+                    integers.append(int(member))
+                else:
+                    int_range = member.split('-')
+                    integers.extend(range(int(int_range[0]),
+                                          int(int_range[1]) + 1))
+
+            return integers
+
+        with open('/sys/devices/system/cpu/online') as cpu_list:
+            cpu_string = cpu_list.readline()
+            return parse_int_list(cpu_string)
 
     def setup_traces(self):
         """Creates all event and group objects needed to be able to retrieve
@@ -633,9 +579,9 @@ class TracepointProvider(object):
             # Fetch list of all threads of the monitored pid, as qemu
             # starts a thread for each vcpu.
             path = os.path.join('/proc', str(self._pid), 'task')
-            groupids = walkdir(path)[1]
+            groupids = self.walkdir(path)[1]
         else:
-            groupids = get_online_cpus()
+            groupids = self.get_online_cpus()
 
         # The constant is needed as a buffer for python libs, std
         # streams and other files that the script opens.
@@ -732,7 +678,7 @@ class TracepointProvider(object):
                 event.reset()
 
 
-class DebugfsProvider(object):
+class DebugfsProvider(Provider):
     """Provides data from the files that KVM creates in the kvm debugfs
     folder."""
     def __init__(self, pid, fields_filter):
@@ -748,12 +694,12 @@ class DebugfsProvider(object):
         The fields are all available KVM debugfs files
 
         """
-        return walkdir(PATH_DEBUGFS_KVM)[2]
+        return self.walkdir(PATH_DEBUGFS_KVM)[2]
 
     def update_fields(self, fields_filter):
         """Refresh fields, applying fields_filter"""
         self._fields = [field for field in self.get_available_fields()
-                        if is_field_wanted(fields_filter, field)]
+                        if self.is_field_wanted(fields_filter, field)]
 
     @property
     def fields(self):
@@ -772,7 +718,7 @@ class DebugfsProvider(object):
     def pid(self, pid):
         self._pid = pid
         if pid != 0:
-            vms = walkdir(PATH_DEBUGFS_KVM)[1]
+            vms = self.walkdir(PATH_DEBUGFS_KVM)[1]
             if len(vms) == 0:
                 self.do_read = False
 
@@ -834,11 +780,23 @@ class Stats(object):
 
     """
     def __init__(self, options):
-        self.providers = get_providers(options)
+        self.providers = self.get_providers(options)
         self._pid_filter = options.pid
         self._fields_filter = options.fields
         self.values = {}
 
+    @staticmethod
+    def get_providers(options):
+        """Returns a list of data providers depending on the passed options."""
+        providers = []
+
+        if options.debugfs:
+            providers.append(DebugfsProvider(options.pid, options.fields))
+        if options.tracepoints or not providers:
+            providers.append(TracepointProvider(options.pid, options.fields))
+
+        return providers
+
     def update_provider_filters(self):
         """Propagates fields filters to providers."""
         # As we reset the counters when updating the fields we can
@@ -933,6 +891,63 @@ class Tui(object):
             curses.nocbreak()
             curses.endwin()
 
+    @staticmethod
+    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 == self.get_gname_from_pid(line[0])):
+                pids.append(int(line[0]))
+        child.stdout.close()
+
+        return pids
+
+    @staticmethod
+    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 update_drilldown(self):
         """Sets or removes a filter that only allows fields without braces."""
         if not self.stats.fields_filter:
@@ -950,7 +965,7 @@ class Tui(object):
         if pid is None:
             pid = self.stats.pid_filter
         self.screen.erase()
-        gname = get_gname_from_pid(pid)
+        gname = self.get_gname_from_pid(pid)
         if gname:
             gname = ('({})'.format(gname[:MAX_GUEST_NAME_LEN] + '...'
                                    if len(gname) > MAX_GUEST_NAME_LEN
@@ -1096,7 +1111,7 @@ class Tui(object):
             else:
                 pids = []
                 try:
-                    pids = get_pid_from_gname(gname)
+                    pids = self.get_pid_from_gname(gname)
                 except:
                     msg = '"' + gname + '": Internal error while searching, ' \
                           'use pid filter instead'
@@ -1229,7 +1244,7 @@ Press any other key to refresh statistics immediately.
 
     def cb_guest_to_pid(option, opt, val, parser):
         try:
-            pids = get_pid_from_gname(val)
+            pids = Tui.get_pid_from_gname(val)
         except:
             raise optparse.OptionValueError('Error while searching for guest '
                                             '"{}", use "-p" to specify a pid '
@@ -1294,18 +1309,6 @@ Press any other key to refresh statistics immediately.
     return options
 
 
-def get_providers(options):
-    """Returns a list of data providers depending on the passed options."""
-    providers = []
-
-    if options.debugfs:
-        providers.append(DebugfsProvider(options.pid, options.fields))
-    if options.tracepoints or not providers:
-        providers.append(TracepointProvider(options.pid, options.fields))
-
-    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'):
-- 
2.11.2

[PATCH v1 10/19] tools/kvm_stat: show cursor in selection screens

[Stefan Raspl]

Show the cursor in the interactive screens to specify pid, filter or guest
name as an orientation for the user.

Signed-off-by: Stefan Raspl <raspl@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 f81ed208307f..53dcd406e9d6 100755
--- a/tools/kvm/kvm_stat/kvm_stat
+++ b/tools/kvm/kvm_stat/kvm_stat
@@ -1148,13 +1148,19 @@ class Tui(object):
                     self.refresh_header(0)
                     self.update_pid(0)
                 if char == 'f':
+                    curses.curs_set(1)
                     self.show_filter_selection()
+                    curses.curs_set(0)
                     sleeptime = DELAY_INITIAL
                 if char == 'g':
+                    curses.curs_set(1)
                     self.show_vm_selection_by_guest_name()
+                    curses.curs_set(0)
                     sleeptime = DELAY_INITIAL
                 if char == 'p':
+                    curses.curs_set(1)
                     self.show_vm_selection_by_pid()
+                    curses.curs_set(0)
                     sleeptime = DELAY_INITIAL
                 if char == 'r':
                     self.stats.reset()
-- 
2.11.2

[PATCH v1 11/19] tools/kvm_stat: display message indicating lack of events

[Stefan Raspl]

Give users some indication on the reason why no data is displayed on the
screen yet.

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

diff --git a/tools/kvm/kvm_stat/kvm_stat b/tools/kvm/kvm_stat/kvm_stat
index 53dcd406e9d6..790fbce95bd6 100755
--- a/tools/kvm/kvm_stat/kvm_stat
+++ b/tools/kvm/kvm_stat/kvm_stat
@@ -1013,6 +1013,8 @@ class Tui(object):
                                    (key, values[0], values[0] * 100 / total,
                                     cur))
             row += 1
+        if row == 3:
+            self.screen.addstr(4, 1, 'No matching events reported yet')
         self.screen.refresh()
 
     def show_filter_selection(self):
-- 
2.11.2

[PATCH v1 12/19] tools/kvm_stat: make heading look a bit more like 'top'

[Stefan Raspl]

Print header in standout font just like the 'top' command does.

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

diff --git a/tools/kvm/kvm_stat/kvm_stat b/tools/kvm/kvm_stat/kvm_stat
index 790fbce95bd6..35147e4877ae 100755
--- a/tools/kvm/kvm_stat/kvm_stat
+++ b/tools/kvm/kvm_stat/kvm_stat
@@ -982,7 +982,8 @@ class Tui(object):
                 regex = regex[:MAX_REGEX_LEN] + '...'
             self.screen.addstr(1, 17, 'regex filter: {0}'.format(regex))
         self.screen.addstr(2, 1, '%-40s %10s%7s %7s' %
-                           ('Event', 'Total', '%Total', 'Current'))
+                           ('Event', 'Total', '%Total', 'Current'),
+                           curses.A_STANDOUT)
         self.screen.addstr(4, 1, 'Collecting data...')
         self.screen.refresh()
 
-- 
2.11.2

[PATCH v1 13/19] tools/kvm_stat: rename 'Current' column to 'CurAvg/s'

[Stefan Raspl]

'Current' can be misleading as it doesn't tell whether this is the amount
of events in the last interval or the current average per second.
Note that this necessitates widening the respective column by one more
character.

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

diff --git a/tools/kvm/kvm_stat/kvm_stat b/tools/kvm/kvm_stat/kvm_stat
index 35147e4877ae..a9e7ea612e7f 100755
--- a/tools/kvm/kvm_stat/kvm_stat
+++ b/tools/kvm/kvm_stat/kvm_stat
@@ -981,8 +981,8 @@ class Tui(object):
             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, '%-40s %10s%7s %7s' %
-                           ('Event', 'Total', '%Total', 'Current'),
+        self.screen.addstr(2, 1, '%-40s %10s%7s %8s' %
+                           ('Event', 'Total', '%Total', 'CurAvg/s'),
                            curses.A_STANDOUT)
         self.screen.addstr(4, 1, 'Collecting data...')
         self.screen.refresh()
@@ -1010,7 +1010,7 @@ class Tui(object):
                 break
             if values[0] is not None:
                 cur = int(round(values[1] / sleeptime)) if values[1] else ''
-                self.screen.addstr(row, 1, '%-40s %10d%7.1f %7s' %
+                self.screen.addstr(row, 1, '%-40s %10d%7.1f %8s' %
                                    (key, values[0], values[0] * 100 / total,
                                     cur))
             row += 1
-- 
2.11.2

[PATCH v1 14/19] tools/kvm_stat: add new interactive command 'h'

[Stefan Raspl]

Display interactive commands reference on 'h'.
While at it, sort interactive commands alphabetically in various places.

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

diff --git a/tools/kvm/kvm_stat/kvm_stat b/tools/kvm/kvm_stat/kvm_stat
index a9e7ea612e7f..6838de38ecb5 100755
--- a/tools/kvm/kvm_stat/kvm_stat
+++ b/tools/kvm/kvm_stat/kvm_stat
@@ -1018,6 +1018,30 @@ class Tui(object):
             self.screen.addstr(4, 1, 'No matching events reported yet')
         self.screen.refresh()
 
+    def show_help_interactive(self):
+        """Display help with list of interactive commands"""
+        msg = ('   c     clear filter',
+               '   f     filter by regular expression',
+               '   g     filter by guest name',
+               '   h     display interactive commands reference',
+               '   p     filter by PID',
+               '   q     quit',
+               '   r     reset stats',
+               '   x     toggle reporting of stats for individual child trace'
+               ' events',
+               'Any other key refreshes statistics immediately')
+        curses.cbreak()
+        self.screen.erase()
+        self.screen.addstr(0, 0, "Interactive commands reference",
+                           curses.A_BOLD)
+        self.screen.addstr(2, 0, "Press any key to exit", curses.A_STANDOUT)
+        row = 4
+        for line in msg:
+            self.screen.addstr(row, 0, line)
+            row += 1
+        self.screen.getkey()
+        self.refresh_header()
+
     def show_filter_selection(self):
         """Draws filter selection mask.
 
@@ -1142,10 +1166,6 @@ class Tui(object):
             sleeptime = DELAY_REGULAR
             try:
                 char = self.screen.getkey()
-                if char == 'x':
-                    self.update_drilldown()
-                if char == 'q':
-                    break
                 if char == 'c':
                     self.stats.fields_filter = DEFAULT_REGEX
                     self.refresh_header(0)
@@ -1160,13 +1180,19 @@ class Tui(object):
                     self.show_vm_selection_by_guest_name()
                     curses.curs_set(0)
                     sleeptime = DELAY_INITIAL
+                if char == 'h':
+                    self.show_help_interactive()
                 if char == 'p':
                     curses.curs_set(1)
                     self.show_vm_selection_by_pid()
                     curses.curs_set(0)
                     sleeptime = DELAY_INITIAL
+                if char == 'q':
+                    break
                 if char == 'r':
                     self.stats.reset()
+                if char == 'x':
+                    self.update_drilldown()
             except KeyboardInterrupt:
                 break
             except curses.error:
@@ -1237,10 +1263,11 @@ Interactive Commands:
    c     clear filter
    f     filter by regular expression
    g     filter by guest name
+   h     display interactive commands reference
    p     filter by PID
    q     quit
-   x     toggle reporting of stats for individual child trace events
    r     reset stats
+   x     toggle reporting of stats for individual child trace events
 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 109431bdc63c..2bad6f22183b 100644
--- a/tools/kvm/kvm_stat/kvm_stat.txt
+++ b/tools/kvm/kvm_stat/kvm_stat.txt
@@ -35,6 +35,8 @@ INTERACTIVE COMMANDS
 
 *g*::	filter by guest name
 
+*h*::	display interactive commands reference
+
 *p*::	filter by PID
 
 *q*::	quit
-- 
2.11.2

[PATCH v1 15/19] tools/kvm_stat: add new interactive command 's'

[Stefan Raspl]

Add new command 's' to modify the update interval. Limited to a maximum of
25.5 sec and a minimum of 0.1 sec, since curses cannot handle longer
and shorter delays respectively.

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

diff --git a/tools/kvm/kvm_stat/kvm_stat b/tools/kvm/kvm_stat/kvm_stat
index 6838de38ecb5..1276b88937c0 100755
--- a/tools/kvm/kvm_stat/kvm_stat
+++ b/tools/kvm/kvm_stat/kvm_stat
@@ -844,8 +844,7 @@ class Stats(object):
                 self.values[key] = (newval, newdelta)
         return self.values
 
-DELAY_INITIAL = 0.25
-DELAY_REGULAR = 3.0
+DELAY_DEFAULT = 3.0
 MAX_GUEST_NAME_LEN = 48
 MAX_REGEX_LEN = 44
 DEFAULT_REGEX = r'^[^\(]*$'
@@ -856,6 +855,8 @@ class Tui(object):
     def __init__(self, stats):
         self.stats = stats
         self.screen = None
+        self._delay_initial = 0.25
+        self._delay_regular = DELAY_DEFAULT
 
     def __enter__(self):
         """Initialises curses for later use.  Based on curses.wrapper
@@ -1027,6 +1028,7 @@ class Tui(object):
                '   p     filter by PID',
                '   q     quit',
                '   r     reset stats',
+               '   s     set update interval',
                '   x     toggle reporting of stats for individual child trace'
                ' events',
                'Any other key refreshes statistics immediately')
@@ -1106,10 +1108,41 @@ class Tui(object):
                 self.refresh_header(pid)
                 self.update_pid(pid)
                 break
-
             except ValueError:
                 msg = '"' + str(pid) + '": Not a valid pid'
 
+    def show_set_update_interval(self):
+        """Draws update interval selection mask."""
+        msg = ''
+        while True:
+            self.screen.erase()
+            self.screen.addstr(0, 0, 'Set update interval (defaults to %fs).' %
+                               DELAY_DEFAULT, curses.A_BOLD)
+            self.screen.addstr(4, 0, msg)
+            self.screen.addstr(2, 0, 'Change delay from %.1fs to ' %
+                               self._delay_regular)
+            curses.echo()
+            val = self.screen.getstr()
+            curses.noecho()
+
+            try:
+                if len(val) > 0:
+                    delay = float(val)
+                    if delay < 0.1:
+                        msg = '"' + str(val) + '": Value must be >=0.1'
+                        continue
+                    if delay > 25.5:
+                        msg = '"' + str(val) + '": Value must be <=25.5'
+                        continue
+                else:
+                    delay = DELAY_DEFAULT
+                self._delay_regular = delay
+                break
+
+            except ValueError:
+                msg = '"' + str(val) + '": Invalid value'
+        self.refresh_header()
+
     def show_vm_selection_by_guest_name(self):
         """Draws guest selection mask.
 
@@ -1156,14 +1189,14 @@ class Tui(object):
 
     def show_stats(self):
         """Refreshes the screen and processes user input."""
-        sleeptime = DELAY_INITIAL
+        sleeptime = self._delay_initial
         self.refresh_header()
         start = 0.0  # result based on init value never appears on screen
         while True:
             self.refresh_body(time.time() - start)
             curses.halfdelay(int(sleeptime * 10))
             start = time.time()
-            sleeptime = DELAY_REGULAR
+            sleeptime = self._delay_regular
             try:
                 char = self.screen.getkey()
                 if char == 'c':
@@ -1174,23 +1207,28 @@ class Tui(object):
                     curses.curs_set(1)
                     self.show_filter_selection()
                     curses.curs_set(0)
-                    sleeptime = DELAY_INITIAL
+                    sleeptime = self._delay_initial
                 if char == 'g':
                     curses.curs_set(1)
                     self.show_vm_selection_by_guest_name()
                     curses.curs_set(0)
-                    sleeptime = DELAY_INITIAL
+                    sleeptime = self._delay_initial
                 if char == 'h':
                     self.show_help_interactive()
                 if char == 'p':
                     curses.curs_set(1)
                     self.show_vm_selection_by_pid()
                     curses.curs_set(0)
-                    sleeptime = DELAY_INITIAL
+                    sleeptime = self._delay_initial
                 if char == 'q':
                     break
                 if char == 'r':
                     self.stats.reset()
+                if char == 's':
+                    curses.curs_set(1)
+                    self.show_set_update_interval()
+                    curses.curs_set(0)
+                    sleeptime = self._delay_initial
                 if char == 'x':
                     self.update_drilldown()
             except KeyboardInterrupt:
@@ -1267,6 +1305,7 @@ Interactive Commands:
    p     filter by PID
    q     quit
    r     reset stats
+   s     set update interval
    x     toggle reporting of stats for individual child trace events
 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 2bad6f22183b..cc019b09e0f5 100644
--- a/tools/kvm/kvm_stat/kvm_stat.txt
+++ b/tools/kvm/kvm_stat/kvm_stat.txt
@@ -43,6 +43,8 @@ INTERACTIVE COMMANDS
 
 *r*::	reset stats
 
+*s*::   set update interval
+
 *x*::	toggle reporting of stats for child trace events
 
 Press any other key to refresh statistics immediately.
-- 
2.11.2

[PATCH v1 16/19] tools/kvm_stat: add new interactive command 't'

[Stefan Raspl]

It might be handy to display the full history of event stats in some cases.
Since we have that available for debugfs, we offer a respective command to
display what's available.
Note that this command does not toggle - one merely has to reset the stats
via 'r' to do the next best thing to reverting the effect of 't'.

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

diff --git a/tools/kvm/kvm_stat/kvm_stat b/tools/kvm/kvm_stat/kvm_stat
index 1276b88937c0..169f6601a90c 100755
--- a/tools/kvm/kvm_stat/kvm_stat
+++ b/tools/kvm/kvm_stat/kvm_stat
@@ -677,6 +677,10 @@ class TracepointProvider(Provider):
             for event in group.events:
                 event.reset()
 
+    def restore(self):
+        """No historic data available that we could restore"""
+        pass
+
 
 class DebugfsProvider(Provider):
     """Provides data from the files that KVM creates in the kvm debugfs
@@ -730,7 +734,14 @@ class DebugfsProvider(Provider):
         self.reset()
 
     def read(self, reset=0):
-        """Returns a dict with format:'file name / field -> current value'."""
+        """Returns a dict with format:'file name / field -> current value'.
+
+        Parameter 'reset':
+          0   plain read
+          1   reset field counts to 0
+          2   restore the original field counts
+
+        """
         results = {}
 
         # If no debugfs filtering support is available, then don't read.
@@ -747,8 +758,10 @@ class DebugfsProvider(Provider):
             for field in self._fields:
                 value = self.read_field(field, path)
                 key = path + field
-                if reset:
+                if reset == 1:
                     self._baseline[key] = value
+                if reset == 2:
+                    self._baseline[key] = 0
                 if self._baseline.get(key, -1) == -1:
                     self._baseline[key] = value
                 results[field] = (results.get(field, 0) + value -
@@ -771,6 +784,11 @@ class DebugfsProvider(Provider):
         self._baseline = {}
         self.read(1)
 
+    def restore(self):
+        """Reset field counters"""
+        self._baseline = {}
+        self.read(2)
+
 
 class Stats(object):
     """Manages the data providers and the data they provide.
@@ -806,10 +824,20 @@ class Stats(object):
             provider.update_fields(self._fields_filter)
 
     def reset(self):
+        """Reset providers' field counters"""
         self.values = {}
         for provider in self.providers:
             provider.reset()
 
+    def restore(self):
+        """Restore providers' field counters"""
+        self.values = {}
+        for provider in self.providers:
+            provider.restore()
+        # Updates oldval (see get()) for all fields to prevent the totals from
+        # being displayed in the 'CurAvg/s' column on next refresh
+        self.get()
+
     @property
     def fields_filter(self):
         return self._fields_filter
@@ -1029,6 +1057,7 @@ class Tui(object):
                '   q     quit',
                '   r     reset stats',
                '   s     set update interval',
+               '   t     show total available stats (debugfs only)',
                '   x     toggle reporting of stats for individual child trace'
                ' events',
                'Any other key refreshes statistics immediately')
@@ -1229,8 +1258,12 @@ class Tui(object):
                     self.show_set_update_interval()
                     curses.curs_set(0)
                     sleeptime = self._delay_initial
+                if char == 't':
+                    self.stats.restore()
                 if char == 'x':
                     self.update_drilldown()
+                    # prevents display of current values on next refresh
+                    self.stats.get()
             except KeyboardInterrupt:
                 break
             except curses.error:
@@ -1306,6 +1339,7 @@ Interactive Commands:
    q     quit
    r     reset stats
    s     set update interval
+   t     show total available stats (debugfs only)
    x     toggle reporting of stats for individual child trace events
 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 cc019b09e0f5..53478ff6eded 100644
--- a/tools/kvm/kvm_stat/kvm_stat.txt
+++ b/tools/kvm/kvm_stat/kvm_stat.txt
@@ -45,6 +45,8 @@ INTERACTIVE COMMANDS
 
 *s*::   set update interval
 
+*t*::   show total available stats (debugfs only)
+
 *x*::	toggle reporting of stats for child trace events
 
 Press any other key to refresh statistics immediately.
-- 
2.11.2

[PATCH v1 17/19] tools/kvm_stat: add new interactive command 'o'

[Stefan Raspl]

Add new interactive command 'o' to toggle sorting by 'CurAvg/s' (default)
and 'Total' columns.

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

diff --git a/tools/kvm/kvm_stat/kvm_stat b/tools/kvm/kvm_stat/kvm_stat
index 169f6601a90c..c9f53832af2d 100755
--- a/tools/kvm/kvm_stat/kvm_stat
+++ b/tools/kvm/kvm_stat/kvm_stat
@@ -876,6 +876,7 @@ DELAY_DEFAULT = 3.0
 MAX_GUEST_NAME_LEN = 48
 MAX_REGEX_LEN = 44
 DEFAULT_REGEX = r'^[^\(]*$'
+SORT_DEFAULT = 0
 
 
 class Tui(object):
@@ -885,6 +886,7 @@ class Tui(object):
         self.screen = None
         self._delay_initial = 0.25
         self._delay_regular = DELAY_DEFAULT
+        self._sorting = SORT_DEFAULT
 
     def __enter__(self):
         """Initialises curses for later use.  Based on curses.wrapper
@@ -1022,14 +1024,23 @@ class Tui(object):
         self.screen.clrtobot()
         stats = self.stats.get()
 
-        def sortkey(x):
+        def sortCurAvg(x):
+            # sort by current events if available
             if stats[x][1]:
                 return (-stats[x][1], -stats[x][0])
             else:
                 return (0, -stats[x][0])
+
+        def sortTotal(x):
+            # sort by totals
+            return (0, -stats[x][0])
         total = 0.
         for val in stats.values():
             total += val[0]
+        if self._sorting == SORT_DEFAULT:
+            sortkey = sortCurAvg
+        else:
+            sortkey = sortTotal
         for key in sorted(stats.keys(), key=sortkey):
 
             if row >= self.screen.getmaxyx()[0]:
@@ -1053,6 +1064,7 @@ class Tui(object):
                '   f     filter by regular expression',
                '   g     filter by guest name',
                '   h     display interactive commands reference',
+               '   o     toggle sorting order (Total vs CurAvg/s)',
                '   p     filter by PID',
                '   q     quit',
                '   r     reset stats',
@@ -1244,6 +1256,8 @@ class Tui(object):
                     sleeptime = self._delay_initial
                 if char == 'h':
                     self.show_help_interactive()
+                if char == 'o':
+                    self._sorting = not self._sorting
                 if char == 'p':
                     curses.curs_set(1)
                     self.show_vm_selection_by_pid()
@@ -1335,6 +1349,7 @@ Interactive Commands:
    f     filter by regular expression
    g     filter by guest name
    h     display interactive commands reference
+   o     toggle sorting order (Total vs CurAvg/s)
    p     filter by PID
    q     quit
    r     reset stats
diff --git a/tools/kvm/kvm_stat/kvm_stat.txt b/tools/kvm/kvm_stat/kvm_stat.txt
index 53478ff6eded..bea768ffbd0d 100644
--- a/tools/kvm/kvm_stat/kvm_stat.txt
+++ b/tools/kvm/kvm_stat/kvm_stat.txt
@@ -37,6 +37,8 @@ INTERACTIVE COMMANDS
 
 *h*::	display interactive commands reference
 
+*o*::   toggle sorting order (Total vs CurAvg/s)
+
 *p*::	filter by PID
 
 *q*::	quit
-- 
2.11.2

[PATCH v1 18/19] tools/kvm_stat: add new interactive command 'b'

[Stefan Raspl]

Toggle display total number of events by guest (debugfs only).
If both, tracepoints and debugfs, are in use, a mixture of events (from
tracepoints) and guests (from debugfs) is displayed. Furthermore, when
switching to display of events by guest, field filters remain active.
I.e. the number of events per guest reported considers only events
matching the filters. Likewise with pid/guest filtering.
Note that when switching to display of events by guest, DebugfsProvider
remains to collect data for events as it did before, but the read()
method summarizes the values by pid.

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

diff --git a/tools/kvm/kvm_stat/kvm_stat b/tools/kvm/kvm_stat/kvm_stat
index c9f53832af2d..f1cec0103757 100755
--- a/tools/kvm/kvm_stat/kvm_stat
+++ b/tools/kvm/kvm_stat/kvm_stat
@@ -662,7 +662,7 @@ class TracepointProvider(Provider):
         self.setup_traces()
         self.fields = self._fields
 
-    def read(self):
+    def read(self, by_guest=0):
         """Returns 'event name: current value' for all enabled events."""
         ret = defaultdict(int)
         for group in self.group_leaders:
@@ -677,7 +677,7 @@ class TracepointProvider(Provider):
             for event in group.events:
                 event.reset()
 
-    def restore(self):
+    def restore(self, by_guest):
         """No historic data available that we could restore"""
         pass
 
@@ -733,7 +733,7 @@ class DebugfsProvider(Provider):
             self.do_read = True
         self.reset()
 
-    def read(self, reset=0):
+    def read(self, reset=0, by_guest=0):
         """Returns a dict with format:'file name / field -> current value'.
 
         Parameter 'reset':
@@ -764,8 +764,16 @@ class DebugfsProvider(Provider):
                     self._baseline[key] = 0
                 if self._baseline.get(key, -1) == -1:
                     self._baseline[key] = value
-                results[field] = (results.get(field, 0) + value -
-                                  self._baseline.get(key, 0))
+                increment = (results.get(field, 0) + value -
+                             self._baseline.get(key, 0))
+                if by_guest:
+                    pid = key.split('-')[0]
+                    if pid in results:
+                        results[pid] += increment
+                    else:
+                        results[pid] = increment
+                else:
+                    results[field] = increment
 
         return results
 
@@ -784,10 +792,10 @@ class DebugfsProvider(Provider):
         self._baseline = {}
         self.read(1)
 
-    def restore(self):
+    def restore(self, by_guest):
         """Reset field counters"""
         self._baseline = {}
-        self.read(2)
+        self.read(2, by_guest)
 
 
 class Stats(object):
@@ -829,14 +837,14 @@ class Stats(object):
         for provider in self.providers:
             provider.reset()
 
-    def restore(self):
+    def restore(self, by_guests):
         """Restore providers' field counters"""
         self.values = {}
         for provider in self.providers:
-            provider.restore()
+            provider.restore(by_guests)
         # Updates oldval (see get()) for all fields to prevent the totals from
         # being displayed in the 'CurAvg/s' column on next refresh
-        self.get()
+        self.get(by_guests)
 
     @property
     def fields_filter(self):
@@ -860,18 +868,41 @@ class Stats(object):
             for provider in self.providers:
                 provider.pid = self._pid_filter
 
-    def get(self):
+    def get(self, by_guest=0):
         """Returns a dict with field -> (value, delta to last value) of all
         provider data."""
         for provider in self.providers:
-            new = provider.read()
-            for key in provider.fields:
+            new = provider.read(by_guest=by_guest)
+            for key in new if by_guest else provider.fields:
                 oldval = self.values.get(key, (0, 0))[0]
                 newval = new.get(key, 0)
                 newdelta = newval - oldval
                 self.values[key] = (newval, newdelta)
         return self.values
 
+    def toggle_display_guests(self, to_pid):
+        """Toggle between collection of stats by individual event and by
+        guest pid
+
+        Events reported by DebugfsProvider change when switching to/from
+        reading by guest values. Hence we have to remove the excess event
+        names from self.values.
+
+        """
+        if to_pid:
+            for provider in self.providers:
+                if isinstance(provider, DebugfsProvider):
+                    for key in provider.fields:
+                        if key in self.values:
+                            del self.values[key]
+        else:
+            oldvals = self.values.copy()
+            for key in oldvals:
+                if key.isdigit():
+                    del self.values[key]
+        # Update oldval (see get())
+        self.get(to_pid)
+
 DELAY_DEFAULT = 3.0
 MAX_GUEST_NAME_LEN = 48
 MAX_REGEX_LEN = 44
@@ -887,6 +918,7 @@ class Tui(object):
         self._delay_initial = 0.25
         self._delay_regular = DELAY_DEFAULT
         self._sorting = SORT_DEFAULT
+        self._display_guests = 0
 
     def __enter__(self):
         """Initialises curses for later use.  Based on curses.wrapper
@@ -1022,7 +1054,7 @@ class Tui(object):
         row = 3
         self.screen.move(row, 0)
         self.screen.clrtobot()
-        stats = self.stats.get()
+        stats = self.stats.get(self._display_guests)
 
         def sortCurAvg(x):
             # sort by current events if available
@@ -1050,6 +1082,8 @@ class Tui(object):
                 break
             if values[0] is not None:
                 cur = int(round(values[1] / sleeptime)) if values[1] else ''
+                if self._display_guests and key.isdigit():
+                    key = self.get_gname_from_pid(key)
                 self.screen.addstr(row, 1, '%-40s %10d%7.1f %8s' %
                                    (key, values[0], values[0] * 100 / total,
                                     cur))
@@ -1060,7 +1094,9 @@ class Tui(object):
 
     def show_help_interactive(self):
         """Display help with list of interactive commands"""
-        msg = ('   c     clear filter',
+        msg = ('   b     toggle events by guests (debugfs only, honors'
+               ' filters)',
+               '   c     clear filter',
                '   f     filter by regular expression',
                '   g     filter by guest name',
                '   h     display interactive commands reference',
@@ -1240,6 +1276,9 @@ class Tui(object):
             sleeptime = self._delay_regular
             try:
                 char = self.screen.getkey()
+                if char == 'b':
+                    self._display_guests = not self._display_guests
+                    self.stats.toggle_display_guests(self._display_guests)
                 if char == 'c':
                     self.stats.fields_filter = DEFAULT_REGEX
                     self.refresh_header(0)
@@ -1273,7 +1312,7 @@ class Tui(object):
                     curses.curs_set(0)
                     sleeptime = self._delay_initial
                 if char == 't':
-                    self.stats.restore()
+                    self.stats.restore(self._display_guests)
                 if char == 'x':
                     self.update_drilldown()
                     # prevents display of current values on next refresh
@@ -1345,6 +1384,7 @@ Requirements:
   the large number of files that are possibly opened.
 
 Interactive Commands:
+   b     toggle events by guests (debugfs only, honors filters)
    c     clear filter
    f     filter by regular expression
    g     filter by guest name
diff --git a/tools/kvm/kvm_stat/kvm_stat.txt b/tools/kvm/kvm_stat/kvm_stat.txt
index bea768ffbd0d..e8ec6147fc05 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]
+*b*::	toggle events by guests (debugfs only, honors filters)
+
 *c*::	clear filter
 
 *f*::	filter by regular expression
-- 
2.11.2

[PATCH v1 19/19] tools/kvm_stat: display guest list in pid/guest selection screens

[Stefan Raspl]

Display a (possibly inaccurate) list of all running guests. Note that we
leave a bit of extra room above the list for potential error messages.
Furthermore, we deliberately do not reject pids or guest names that are
not in our list, as we cannot rule out that our fuzzy approach might be
in error somehow.

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

diff --git a/tools/kvm/kvm_stat/kvm_stat b/tools/kvm/kvm_stat/kvm_stat
index f1cec0103757..8237d9bb8547 100755
--- a/tools/kvm/kvm_stat/kvm_stat
+++ b/tools/kvm/kvm_stat/kvm_stat
@@ -954,15 +954,9 @@ class Tui(object):
             curses.nocbreak()
             curses.endwin()
 
-    @staticmethod
-    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 = []
+    def get_all_gnames(self):
+        """Returns a list of (pid, gname) tuples of all running guests"""
+        res = []
         try:
             child = subprocess.Popen(['ps', '-A', '--format', 'pid,args'],
                                      stdout=subprocess.PIPE)
@@ -972,11 +966,40 @@ class Tui(object):
             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 == self.get_gname_from_pid(line[0])):
-                pids.append(int(line[0]))
+            if ' -name ' in line[1]:
+                res.append((line[0], self.get_gname_from_pid(line[0])))
         child.stdout.close()
 
+        return res
+
+    def print_all_gnames(self, row):
+        """Print a list of all running guests along with their pids."""
+        self.screen.addstr(row, 2, '%8s  %-60s' %
+                           ('Pid', 'Guest Name (fuzzy list, might be '
+                            'inaccurate!)'),
+                           curses.A_UNDERLINE)
+        row += 1
+        try:
+            for line in self.get_all_gnames():
+                self.screen.addstr(row, 2, '%8s  %-60s' % (line[0], line[1]))
+                row += 1
+                if row >= self.screen.getmaxyx()[0]:
+                    break
+        except Exception:
+            self.screen.addstr(row + 1, 2, 'Not available')
+
+    def get_pid_from_gname(self, 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 = []
+        for line in self.get_all_gnames():
+            if gname == line[1]:
+                pids.append(int(line[0]))
+
         return pids
 
     @staticmethod
@@ -1167,6 +1190,7 @@ class Tui(object):
                                'This might limit the shown data to the trace '
                                'statistics.')
             self.screen.addstr(5, 0, msg)
+            self.print_all_gnames(7)
 
             curses.echo()
             self.screen.addstr(3, 0, "Pid [0 or pid]: ")
@@ -1236,6 +1260,7 @@ class Tui(object):
                                'This might limit the shown data to the trace '
                                'statistics.')
             self.screen.addstr(5, 0, msg)
+            self.print_all_gnames()
             curses.echo()
             self.screen.addstr(3, 0, "Guest [ENTER or guest]: ")
             gname = self.screen.getstr()
-- 
2.11.2

Re: [PATCH v1 14/19] tools/kvm_stat: add new interactive command 'h'

[Paolo Bonzini]

On 07/06/2017 21:08, Stefan Raspl wrote:
> +        """Display help with list of interactive commands"""
> +        msg = ('   c     clear filter',
> +               '   f     filter by regular expression',
> +               '   g     filter by guest name',
> +               '   h     display interactive commands reference',
> +               '   p     filter by PID',
> +               '   q     quit',
> +               '   r     reset stats',
> +               '   x     toggle reporting of stats for individual child trace'
> +               ' events',
> +               'Any other key refreshes statistics immediately')

> @@ -1237,10 +1263,11 @@ description_text = """
>     c     clear filter
>     f     filter by regular expression
>     g     filter by guest name
> +   h     display interactive commands reference
>     p     filter by PID
>     q     quit
> -   x     toggle reporting of stats for individual child trace events
>     r     reset stats
> +   x     toggle reporting of stats for individual child trace events
>  Press any other key to refresh statistics immediately.
>  """

Some duplication here... want to eliminate it as a follow-up patch, by
creating a global string or array for this part of description_text?

Paolo

Re: [PATCH v1 16/19] tools/kvm_stat: add new interactive command 't'

[Paolo Bonzini]

On 07/06/2017 21:08, Stefan Raspl wrote:
> It might be handy to display the full history of event stats in some cases.
> Since we have that available for debugfs, we offer a respective command to
> display what's available.
> Note that this command does not toggle - one merely has to reset the stats
> via 'r' to do the next best thing to reverting the effect of 't'.

It's not clear to me what this does...  Can you explain it better?

Paolo

Re: [PATCH v1 18/19] tools/kvm_stat: add new interactive command 'b'

[Paolo Bonzini]

On 07/06/2017 21:08, Stefan Raspl wrote:
> Toggle display total number of events by guest (debugfs only).
> If both, tracepoints and debugfs, are in use, a mixture of events (from
> tracepoints) and guests (from debugfs) is displayed. Furthermore, when
> switching to display of events by guest, field filters remain active.
> I.e. the number of events per guest reported considers only events
> matching the filters. Likewise with pid/guest filtering.
> Note that when switching to display of events by guest, DebugfsProvider
> remains to collect data for events as it did before, but the read()
> method summarizes the values by pid.

Maybe do not enable this command if tracepoints are in use?

Paolo

Re: [PATCH v1 00/19] tools/kvm_stat: More misc patches

[Paolo Bonzini]

On 07/06/2017 21:08, Stefan Raspl wrote:
> There were leftovers from the previous patch series as suggested by Radim,
> so I set out to fix these. Then I got a bit carried away...

You did! :)  Looks pretty good though.

I'm leaving out patch 16 and 18, the only two that I wasn't really sure
about (and conveniently, patch 18 conflicted with 16...) but I'm
applying the rest.

Thanks,

Paolo

Re: [PATCH v1 18/19] tools/kvm_stat: add new interactive command 'b'

[Stefan Raspl]

On 08.06.2017 18:24, Paolo Bonzini wrote:
> 
> 
> On 07/06/2017 21:08, Stefan Raspl wrote:
>> Toggle display total number of events by guest (debugfs only).
>> If both, tracepoints and debugfs, are in use, a mixture of events (from
>> tracepoints) and guests (from debugfs) is displayed. Furthermore, when
>> switching to display of events by guest, field filters remain active.
>> I.e. the number of events per guest reported considers only events
>> matching the filters. Likewise with pid/guest filtering.
>> Note that when switching to display of events by guest, DebugfsProvider
>> remains to collect data for events as it did before, but the read()
>> method summarizes the values by pid.
> 
> Maybe do not enable this command if tracepoints are in use?

The idea was to give the user all options available. *I* can't
imagine how a mixture of guests and events would be useful, but that
doesn't mean that there might be folks out there who can make use of
that, still.
Plus if we would prevent this command from doing anything when
tracepoints are in use, then we should probably give the user some
kind of feedback on why nothing changed upon his keypress - but I
wouldn't know a place in kvm_stats GUI to do that. So if we leave as
is, at least the user gets some immediate feedback and hopefully
figure what's going on, and that he might want to rather restart
with debugfs only.

Ciao,
Stefan

Re: [PATCH v1 16/19] tools/kvm_stat: add new interactive command 't'

[Stefan Raspl]

On 08.06.2017 18:21, Paolo Bonzini wrote:
> 
> 
> On 07/06/2017 21:08, Stefan Raspl wrote:
>> It might be handy to display the full history of event stats in some cases.
>> Since we have that available for debugfs, we offer a respective command to
>> display what's available.
>> Note that this command does not toggle - one merely has to reset the stats
>> via 'r' to do the next best thing to reverting the effect of 't'.
> 
> It's not clear to me what this does...  Can you explain it better?

Sure:
While debugfs provides the total number of events since system
start, kvm_stat only displays the number of events since kvm_stat
was started. Press 't' to display the number of events since the
system was started.
Note that this command does not toggle, i.e. pressing it again will
not restore the number of events since kvm_stat was started. One
merely has to reset the stats via 'r' to do the next best thing to
reverting the effect of 't'.

Is that more clear?

Ciao,
Stefan

Re: [PATCH v1 14/19] tools/kvm_stat: add new interactive command 'h'

[Stefan Raspl]

On 08.06.2017 18:19, Paolo Bonzini wrote:
> 
> 
> On 07/06/2017 21:08, Stefan Raspl wrote:
>> +        """Display help with list of interactive commands"""
>> +        msg = ('   c     clear filter',
>> +               '   f     filter by regular expression',
>> +               '   g     filter by guest name',
>> +               '   h     display interactive commands reference',
>> +               '   p     filter by PID',
>> +               '   q     quit',
>> +               '   r     reset stats',
>> +               '   x     toggle reporting of stats for individual child trace'
>> +               ' events',
>> +               'Any other key refreshes statistics immediately')
> 
>> @@ -1237,10 +1263,11 @@ description_text = """
>>     c     clear filter
>>     f     filter by regular expression
>>     g     filter by guest name
>> +   h     display interactive commands reference
>>     p     filter by PID
>>     q     quit
>> -   x     toggle reporting of stats for individual child trace events
>>     r     reset stats
>> +   x     toggle reporting of stats for individual child trace events
>>  Press any other key to refresh statistics immediately.
>>  """
> 
> Some duplication here... want to eliminate it as a follow-up patch, by
> creating a global string or array for this part of description_text?

I was annoyed by this, too. Couldn't figure an obvious way
immediately to re-use the string, will give it some more thought.

Ciao,
Stefan

Re: [PATCH v1 18/19] tools/kvm_stat: add new interactive command 'b'

[Paolo Bonzini]

On 20/06/2017 11:10, Stefan Raspl wrote:
> Plus if we would prevent this command from doing anything when
> tracepoints are in use, then we should probably give the user some
> kind of feedback on why nothing changed upon his keypress - but I
> wouldn't know a place in kvm_stats GUI to do that. So if we leave as
> is, at least the user gets some immediate feedback and hopefully
> figure what's going on, and that he might want to rather restart
> with debugfs only.

What about writing a message in the last line?

Paolo

Re: [PATCH v1 16/19] tools/kvm_stat: add new interactive command 't'

[Paolo Bonzini]

On 20/06/2017 11:10, Stefan Raspl wrote:
> On 08.06.2017 18:21, Paolo Bonzini wrote:
>>
>>
>> On 07/06/2017 21:08, Stefan Raspl wrote:
>>> It might be handy to display the full history of event stats in some cases.
>>> Since we have that available for debugfs, we offer a respective command to
>>> display what's available.
>>> Note that this command does not toggle - one merely has to reset the stats
>>> via 'r' to do the next best thing to reverting the effect of 't'.
>>
>> It's not clear to me what this does...  Can you explain it better?
> 
> Sure:
> While debugfs provides the total number of events since system
> start, kvm_stat only displays the number of events since kvm_stat
> was started. Press 't' to display the number of events since the
> system was started.
> Note that this command does not toggle, i.e. pressing it again will
> not restore the number of events since kvm_stat was started. One
> merely has to reset the stats via 'r' to do the next best thing to
> reverting the effect of 't'.
> 
> Is that more clear?

Sorry for the stupid question, when can this be useful? :)

Paolo

Re: [PATCH v1 14/19] tools/kvm_stat: add new interactive command 'h'

[Paolo Bonzini]

On 20/06/2017 11:10, Stefan Raspl wrote:
> On 08.06.2017 18:19, Paolo Bonzini wrote:
>>
>>
>> On 07/06/2017 21:08, Stefan Raspl wrote:
>>> +        """Display help with list of interactive commands"""
>>> +        msg = ('   c     clear filter',
>>> +               '   f     filter by regular expression',
>>> +               '   g     filter by guest name',
>>> +               '   h     display interactive commands reference',
>>> +               '   p     filter by PID',
>>> +               '   q     quit',
>>> +               '   r     reset stats',
>>> +               '   x     toggle reporting of stats for individual child trace'
>>> +               ' events',
>>> +               'Any other key refreshes statistics immediately')
>>
>>> @@ -1237,10 +1263,11 @@ description_text = """
>>>     c     clear filter
>>>     f     filter by regular expression
>>>     g     filter by guest name
>>> +   h     display interactive commands reference
>>>     p     filter by PID
>>>     q     quit
>>> -   x     toggle reporting of stats for individual child trace events
>>>     r     reset stats
>>> +   x     toggle reporting of stats for individual child trace events
>>>  Press any other key to refresh statistics immediately.
>>>  """
>>
>> Some duplication here... want to eliminate it as a follow-up patch, by
>> creating a global string or array for this part of description_text?
> 
> I was annoyed by this, too. Couldn't figure an obvious way
> immediately to re-use the string, will give it some more thought.

Ok, I'll give it a shot then.

Paolo

Re: [PATCH v1 18/19] tools/kvm_stat: add new interactive command 'b'

[Stefan Raspl]

On 20.06.2017 14:33, Paolo Bonzini wrote:
> 
> 
> On 20/06/2017 11:10, Stefan Raspl wrote:
>> Plus if we would prevent this command from doing anything when
>> tracepoints are in use, then we should probably give the user some
>> kind of feedback on why nothing changed upon his keypress - but I
>> wouldn't know a place in kvm_stats GUI to do that. So if we leave as
>> is, at least the user gets some immediate feedback and hopefully
>> figure what's going on, and that he might want to rather restart
>> with debugfs only.
> 
> What about writing a message in the last line?

kvm_stat fills the entire screen till the last line. We'd have to
make any message stick out, probably by erasing the line before and
after, too - and then make sure that the message remains visible for
at least 3 seconds, i.e. re-apply the change after refreshing the
body till 3 sec expired (since users could set the refresh interval
to e.g. 0.5 sec via 's', whereby it would be really hard to spot the
message within the 0.5 sec that it is visible on screen). So that
would require some kind of time measuring logic. Which also would
need to consider add'l key presses before those 3 secs expire.
In short: We're talking quite a bit of more messy code for little
benefit.
Then again: How about simply drawing a separate screen a la the help
(see 'h') with whatever error message we need, and which is exited
upon a key press? In a generic way, of course, so we could re-use
for future likewise cases.

Ciao,
Stefan

Re: [PATCH v1 16/19] tools/kvm_stat: add new interactive command 't'

[Stefan Raspl]

On 20.06.2017 14:34, Paolo Bonzini wrote:
> 
> 
> On 20/06/2017 11:10, Stefan Raspl wrote:
>> On 08.06.2017 18:21, Paolo Bonzini wrote:
>>>
>>>
>>> On 07/06/2017 21:08, Stefan Raspl wrote:
>>>> It might be handy to display the full history of event stats in some cases.
>>>> Since we have that available for debugfs, we offer a respective command to
>>>> display what's available.
>>>> Note that this command does not toggle - one merely has to reset the stats
>>>> via 'r' to do the next best thing to reverting the effect of 't'.
>>>
>>> It's not clear to me what this does...  Can you explain it better?
>>
>> Sure:
>> While debugfs provides the total number of events since system
>> start, kvm_stat only displays the number of events since kvm_stat
>> was started. Press 't' to display the number of events since the
>> system was started.
>> Note that this command does not toggle, i.e. pressing it again will
>> not restore the number of events since kvm_stat was started. One
>> merely has to reset the stats via 'r' to do the next best thing to
>> reverting the effect of 't'.
>>
>> Is that more clear?
> 
> Sorry for the stupid question, when can this be useful? :)

When you have several guests running for weeks, and want to have a
quick check whether any of the events was triggered in an excessive
manner while you weren't looking (via kvm_stat or similar)?

Ciao,
Stefan

Re: [PATCH v1 18/19] tools/kvm_stat: add new interactive command 'b'

[Paolo Bonzini]

On 20/06/2017 15:07, Stefan Raspl wrote:
> On 20.06.2017 14:33, Paolo Bonzini wrote:
>>
>>
>> On 20/06/2017 11:10, Stefan Raspl wrote:
>>> Plus if we would prevent this command from doing anything when
>>> tracepoints are in use, then we should probably give the user some
>>> kind of feedback on why nothing changed upon his keypress - but I
>>> wouldn't know a place in kvm_stats GUI to do that. So if we leave as
>>> is, at least the user gets some immediate feedback and hopefully
>>> figure what's going on, and that he might want to rather restart
>>> with debugfs only.
>>
>> What about writing a message in the last line?
> 
> kvm_stat fills the entire screen till the last line. We'd have to
> make any message stick out, probably by erasing the line before and
> after, too - and then make sure that the message remains visible for
> at least 3 seconds, i.e. re-apply the change after refreshing the
> body till 3 sec expired (since users could set the refresh interval
> to e.g. 0.5 sec via 's', whereby it would be really hard to spot the
> message within the 0.5 sec that it is visible on screen). So that
> would require some kind of time measuring logic. Which also would
> need to consider add'l key presses before those 3 secs expire.
> In short: We're talking quite a bit of more messy code for little
> benefit.
> Then again: How about simply drawing a separate screen a la the help
> (see 'h') with whatever error message we need, and which is exited
> upon a key press? In a generic way, of course, so we could re-use
> for future likewise cases.

That would work.  Alternatively, make kvm_stat not use the last line,
and draw it inverted.  The last error can be printed in the last line,
again inverted, and we can skip clearing the last line when we redraw.

Paolo

Re: [PATCH v1 16/19] tools/kvm_stat: add new interactive command 't'

[Paolo Bonzini]

On 20/06/2017 15:09, Stefan Raspl wrote:
>> Sorry for the stupid question, when can this be useful? :)
>
> When you have several guests running for weeks, and want to have a
> quick check whether any of the events was triggered in an excessive
> manner while you weren't looking (via kvm_stat or similar)?

Maybe kvm_stat could take an option for "do not reset debugfs on
startup"?  Is there a case where kvm_stat would be invoked for a while
and _then_ you press 't'?

Paolo

Re: [PATCH v1 16/19] tools/kvm_stat: add new interactive command 't'

[Stefan Raspl]

On 20.06.2017 15:17, Paolo Bonzini wrote:
> 
> 
> On 20/06/2017 15:09, Stefan Raspl wrote:
>>> Sorry for the stupid question, when can this be useful? :)
>>
>> When you have several guests running for weeks, and want to have a
>> quick check whether any of the events was triggered in an excessive
>> manner while you weren't looking (via kvm_stat or similar)?
> 
> Maybe kvm_stat could take an option for "do not reset debugfs on
> startup"?  Is there a case where kvm_stat would be invoked for a while
> and _then_ you press 't'?

Maybe when one watches the output for a while and wonders whether
the current (strange) distribution of events has been like that ever
since?

Re: [PATCH v1 18/19] tools/kvm_stat: add new interactive command 'b'

[Stefan Raspl]

On 20.06.2017 15:10, Paolo Bonzini wrote:
> 
> 
> On 20/06/2017 15:07, Stefan Raspl wrote:
>> On 20.06.2017 14:33, Paolo Bonzini wrote:
>>>
>>>
>>> On 20/06/2017 11:10, Stefan Raspl wrote:
>>>> Plus if we would prevent this command from doing anything when
>>>> tracepoints are in use, then we should probably give the user some
>>>> kind of feedback on why nothing changed upon his keypress - but I
>>>> wouldn't know a place in kvm_stats GUI to do that. So if we leave as
>>>> is, at least the user gets some immediate feedback and hopefully
>>>> figure what's going on, and that he might want to rather restart
>>>> with debugfs only.
>>>
>>> What about writing a message in the last line?
>>
>> kvm_stat fills the entire screen till the last line. We'd have to
>> make any message stick out, probably by erasing the line before and
>> after, too - and then make sure that the message remains visible for
>> at least 3 seconds, i.e. re-apply the change after refreshing the
>> body till 3 sec expired (since users could set the refresh interval
>> to e.g. 0.5 sec via 's', whereby it would be really hard to spot the
>> message within the 0.5 sec that it is visible on screen). So that
>> would require some kind of time measuring logic. Which also would
>> need to consider add'l key presses before those 3 secs expire.
>> In short: We're talking quite a bit of more messy code for little
>> benefit.
>> Then again: How about simply drawing a separate screen a la the help
>> (see 'h') with whatever error message we need, and which is exited
>> upon a key press? In a generic way, of course, so we could re-use
>> for future likewise cases.
> 
> That would work.  Alternatively, make kvm_stat not use the last line,
> and draw it inverted.  The last error can be printed in the last line,
> again inverted, and we can skip clearing the last line when we redraw.

There's a specific call to clear all remaining lines on the screen
till bottom that we're using - would need to look into it how to
prevent the final line from getting drawn. In the worst case, we
could manually clear all affected lines except the final one.
Still: I bet usability experts would argue that printing "important"
messages at the bottom of the screen, even if highlighted, might not
be the most obvious way to present such information. Plus if we put
it on a separate page, we'd get a bit more space in case we'd have a
future message with multiple lines or similar.

Ciao,
Stefan

Re: [PATCH v1 16/19] tools/kvm_stat: add new interactive command 't'

[Paolo Bonzini]

On 20/06/2017 16:17, Stefan Raspl wrote:
>> Maybe kvm_stat could take an option for "do not reset debugfs on
>> startup"?  Is there a case where kvm_stat would be invoked for a while
>> and _then_ you press 't'?
> Maybe when one watches the output for a while and wonders whether
> the current (strange) distribution of events has been like that ever
> since?

That would probably be served better by firing _another_ separate
kvm_stat and comparing the two, since there's no reset option.

Paolo

Re: [PATCH v1 18/19] tools/kvm_stat: add new interactive command 'b'

[Paolo Bonzini]

On 20/06/2017 16:41, Stefan Raspl wrote:
> There's a specific call to clear all remaining lines on the screen
> till bottom that we're using - would need to look into it how to
> prevent the final line from getting drawn. In the worst case, we
> could manually clear all affected lines except the final one.
> Still: I bet usability experts would argue that printing "important"
> messages at the bottom of the screen, even if highlighted, might not
> be the most obvious way to present such information. Plus if we put
> it on a separate page, we'd get a bit more space in case we'd have a
> future message with multiple lines or similar.

If you write the patch, I'm not going to complain. :)

Paolo

Re: [PATCH v1 18/19] tools/kvm_stat: add new interactive command 'b'

[Stefan Raspl]

On 20.06.2017 16:47, Paolo Bonzini wrote:
> 
> 
> On 20/06/2017 16:41, Stefan Raspl wrote:
>> There's a specific call to clear all remaining lines on the screen
>> till bottom that we're using - would need to look into it how to
>> prevent the final line from getting drawn. In the worst case, we
>> could manually clear all affected lines except the final one.
>> Still: I bet usability experts would argue that printing "important"
>> messages at the bottom of the screen, even if highlighted, might not
>> be the most obvious way to present such information. Plus if we put
>> it on a separate page, we'd get a bit more space in case we'd have a
>> future message with multiple lines or similar.
> 
> If you write the patch, I'm not going to complain. :)

I'll update this patch accordingly and repost. Might take a moment.

Ciao,
Stefan

Re: [PATCH v1 16/19] tools/kvm_stat: add new interactive command 't'

[Stefan Raspl]

On 20.06.2017 16:47, Paolo Bonzini wrote:
> 
> 
> On 20/06/2017 16:17, Stefan Raspl wrote:
>>> Maybe kvm_stat could take an option for "do not reset debugfs on
>>> startup"?  Is there a case where kvm_stat would be invoked for a while
>>> and _then_ you press 't'?
>> Maybe when one watches the output for a while and wonders whether
>> the current (strange) distribution of events has been like that ever
>> since?
> 
> That would probably be served better by firing _another_ separate
> kvm_stat and comparing the two, since there's no reset option.

Yeah, but that will only work from point X onwards. If you just
happen to realize at point X that things look a bit strange and only
then realize that the "historic" data might be of interest for
comparison, then 't' is your friend.
The thing is that debugfs was actually reporting "historic" data
exclusively up to the point where
9f114a03c6854f49065dd036c17e1b4bb947f842 was committed. In that
patch, as a side effect I changed things so that both, debugfs and
tracepoints, would consistently report number of events from
kvm_stat startup onwards, which made a hell of a lot more sense to
me especially when starting kvmstat with '-td'. Still, the previous
behavior of debugfs probably had its merits, so this patch is an
attempt to get the best of both worlds.

Ciao,
Stefan

Re: [PATCH v1 16/19] tools/kvm_stat: add new interactive command 't'

[Paolo Bonzini]

On 21/06/2017 08:54, Stefan Raspl wrote:
>>>> Maybe kvm_stat could take an option for "do not reset debugfs on
>>>> startup"?  Is there a case where kvm_stat would be invoked for a while
>>>> and _then_ you press 't'?
>>> Maybe when one watches the output for a while and wonders whether
>>> the current (strange) distribution of events has been like that ever
>>> since?
>> That would probably be served better by firing _another_ separate
>> kvm_stat and comparing the two, since there's no reset option.
> Yeah, but that will only work from point X onwards. If you just
> happen to realize at point X that things look a bit strange and only
> then realize that the "historic" data might be of interest for
> comparison, then 't' is your friend.

But wouldn't historic data always available from a fresh invocation of
kvm_stat (with a --debugfs-include-past or similar) option?

> The thing is that debugfs was actually reporting "historic" data
> exclusively up to the point where
> 9f114a03c6854f49065dd036c17e1b4bb947f842 was committed. In that
> patch, as a side effect I changed things so that both, debugfs and
> tracepoints, would consistently report number of events from
> kvm_stat startup onwards, which made a hell of a lot more sense to
> me especially when starting kvmstat with '-td'. Still, the previous
> behavior of debugfs probably had its merits, so this patch is an
> attempt to get the best of both worlds.

I understand, and I agree.  I'm just not sure that the interactive
command is the best way to achieve it.

Paolo

Re: [PATCH v1 16/19] tools/kvm_stat: add new interactive command 't'

[Stefan Raspl]

On 21.06.2017 12:37, Paolo Bonzini wrote:
> 
> 
> On 21/06/2017 08:54, Stefan Raspl wrote:
>>>>> Maybe kvm_stat could take an option for "do not reset debugfs on
>>>>> startup"?  Is there a case where kvm_stat would be invoked for a while
>>>>> and _then_ you press 't'?
>>>> Maybe when one watches the output for a while and wonders whether
>>>> the current (strange) distribution of events has been like that ever
>>>> since?
>>> That would probably be served better by firing _another_ separate
>>> kvm_stat and comparing the two, since there's no reset option.
>> Yeah, but that will only work from point X onwards. If you just
>> happen to realize at point X that things look a bit strange and only
>> then realize that the "historic" data might be of interest for
>> comparison, then 't' is your friend.
> 
> But wouldn't historic data always available from a fresh invocation of
> kvm_stat (with a --debugfs-include-past or similar) option?

Only when we add that option as a new feature.

>> The thing is that debugfs was actually reporting "historic" data
>> exclusively up to the point where
>> 9f114a03c6854f49065dd036c17e1b4bb947f842 was committed. In that
>> patch, as a side effect I changed things so that both, debugfs and
>> tracepoints, would consistently report number of events from
>> kvm_stat startup onwards, which made a hell of a lot more sense to
>> me especially when starting kvmstat with '-td'. Still, the previous
>> behavior of debugfs probably had its merits, so this patch is an
>> attempt to get the best of both worlds.
> 
> I understand, and I agree.  I'm just not sure that the interactive
> command is the best way to achieve it.

OK, so that sounds like we're in agreement that this is a useful
feature. The question is whether to do it via an interactive command
or a command line switch, or both.
I guess my vote would be to do both, i.e. add
'--debugfs-include-past' as a new command line switch to the patch,
but leave 't' in place.
Thoughts?

Ciao,
Stefan

Re: [PATCH v1 16/19] tools/kvm_stat: add new interactive command 't'

[Paolo Bonzini]

On 22/06/2017 09:46, Stefan Raspl wrote:
> OK, so that sounds like we're in agreement that this is a useful
> feature. The question is whether to do it via an interactive command
> or a command line switch, or both.
> I guess my vote would be to do both, i.e. add
> '--debugfs-include-past' as a new command line switch to the patch,
> but leave 't' in place.

I would prefer to add only the new option for now.

Paolo

Re: [PATCH v1 16/19] tools/kvm_stat: add new interactive command 't'

[Stefan Raspl]

On 22.06.2017 09:52, Paolo Bonzini wrote:
> 
> 
> On 22/06/2017 09:46, Stefan Raspl wrote:
>> OK, so that sounds like we're in agreement that this is a useful
>> feature. The question is whether to do it via an interactive command
>> or a command line switch, or both.
>> I guess my vote would be to do both, i.e. add
>> '--debugfs-include-past' as a new command line switch to the patch,
>> but leave 't' in place.
> 
> I would prefer to add only the new option for now.

OK, I'll update the patch accordingly.

Ciao,
Stefan