Hi John,
Thanks for the feedback! I agree adding new command lines (especially
as we have already plenty!) is always to be carefully considered.
So I did some prototyping and I'm sending after this email one more
patch in that direction. The patch basically sets a minimum GUI
refresh time also when using perf events (using the same refresh_time
that can be already set via -R command line option) by adding the
periodic call to show() from the Gtk timer also in perf mode, but
adding there a filter so we are not doing any refreshes if we didn't
get at least one perf event. We will of course keep track of this with
a simple toggle variable in the perf event callback. So as before if
no perf events arrived then we do not touch the GUI, if any arrived
then we will refresh it but with a configurable minimum threshold
refresh time.
This I think is good as it resolves the CPU extreme usage due to huge
number of refresh when we are receiving many events and were
generating immediate consecutive redraws which were useless. Of course
one can always change the -R parameter to get more or less responsive
refreshes.
On the other side I think the new command line proposed in this quoted
patch should be still very useful, as when perf mode is used some data
(ie. for sure as I checked context switches counters and process
priorities, I guess also cgroups data and maybe other) are not
updated, since we do not get perf events for those (we currently have
perf.SAMPLE_CPU | perf.SAMPLE_TID as events to report). I'm not really
expert of perf to judge if we could still get all the data by hooking
to other events (from what I saw probably context switches by looking
at cs event? for processes hooking sched:sched_pi_setprio?) and how
difficult/efficient would then become compared to the "ps polling" in
that case.
But as it is in perf mode basically you have a bunch of data that is
just filled on process/thread start (using /proc data, as in polling)
and then outdated afterwards on GUI. So if you need those, excluding
some other modifications, having the new command line would be very
helpful.
Let me know what do you think!
Thanks!
Cheers,
Federico
Il giorno mar 23 mar 2021 alle ore 17:39 John Kacur
<jkacur(a)redhat.com> ha scritto:
>
>
>
> On Mon, 22 Mar 2021, Federico Pellegrin wrote:
>
> > Hi John, all,
> > Was just wondering since this last patch (add option to disable perf)
> > is still the only one pending: does it look like something that is
> > worth merging or would you prefeer an alternative solution?
> >
> > This has proven to be pretty important for my use case since on
> > systems that are very heavily loaded the refresh induced by the perf
> > events triggers a very high CPU usage of the tuna gui due to the
> > continuos GUI refreshes. So CPU load gets high and GUI extremely
> > sluggish.
> >
> > An alternative solution I've thought of (but not sure if it is better
> > / worse, and didn't try it yet out ;-) ) would be to give the
> > possibility also when using perf events to limit the maximum refresh
> > of the GUI so ie. if perf events are triggered more often than a
> > configurable time we will not refresh the gui, but just update the
> > data, assuming it will be refreshed later when the time from last
> > refresh is big enough.
>
> The reason that I didn't apply it was that I don't want to jump too
> quickly on patches that add new command line options, unless I am sure
> that they are necessary.
>
> The alternative of updating at a regularly scheduled time sounds
> interesting to me. Would you like to try and prototype that up?
>
> Thanks
>
> John
>
> >
> > What do you think?
> >
> > Many thanks!
> > Federico
> >
> > Il giorno gio 18 mar 2021 alle ore 06:05 Federico Pellegrin
> > <fede(a)evolware.org> ha scritto:
> > >
> > > Add command line option (-d, --disable_perf) to explicitly disable
> > > usage of perf facility for procview. Previously if the perf attach
> > > succeeds, it will always be used. Now if -d is passed it will be not
> > > tried.
> > >
> > > Default behaviour is kept as before.
> > >
> > > Rationale for adding this option:
> > > -) On very loaded systems (mostly with processes / threads varing very
> > > often) perf may end up being triggered all the time, causing a
> > > continuous widget refresh which will cause 100% CPU usage of tuna
> > > and therefore a very sluggish and hardly usable GUI
> > > -) As it is now (maybe we could change?) perf doesn't deliver the
> > > same data as polling /proc does, for example perf will not update
> > > context switch data. So having the possibility for the user to go
> > > anyway for acquiring data from /proc (as ugly as polling is) to have
> > > also that data displayed, if needed, sounds like a reasonable
> > > approach.
> > >
> > > Signed-off-by: Federico Pellegrin <fede(a)evolware.org>
> > > ---
> > > tuna-cmd.py | 12 +++++++++---
> > > tuna/gui/procview.py | 11 ++++++-----
> > > tuna/tuna_gui.py | 4 ++--
> > > 3 files changed, 17 insertions(+), 10 deletions(-)
> > >
> > > diff --git a/tuna-cmd.py b/tuna-cmd.py
> > > index c27abd1..ecfa15e 100755
> > > --- a/tuna-cmd.py
> > > +++ b/tuna-cmd.py
> > > @@ -59,6 +59,8 @@ def usage():
> > > {"cpulist": _('CPU-LIST')}))
> > > print(fmt % ('-C, --affect_children',
> > > _('Operation will affect children threads')))
> > > + print(fmt % ('-d, --disable_perf',
> > > + _('Explicitly disable usage of perf in GUI for
process view')))
> > > print(fmt % ('-f, --filter',
> > > _('Display filter the selected entities')))
> > > print(fmt % ('-i, --isolate', _('Move all
threads away from %(cpulist)s') %
> > > @@ -495,14 +497,14 @@ def main():
> > >
> > > i18n_init()
> > > try:
> > > - short = "a:c:CfgGhiIKlmNp:PQq:r:R:s:S:t:UvWx"
> > > + short = "a:c:dCfgGhiIKlmNp:PQq:r:R:s:S:t:UvWx"
> > > long = ["cpus=", "affect_children",
"filter", "gui", "help",
> > > "isolate", "include",
"no_kthreads", "move", "nohz_full",
> > > "show_sockets", "priority=",
"show_threads",
> > > "show_irqs", "irqs=",
> > > "save=", "sockets=",
"threads=", "no_uthreads",
> > > "version", "what_is",
"spread", "cgroup", "config_file_apply=",
> > > - "config_file_list=", "run=",
"refresh="]
> > > + "config_file_list=", "run=",
"refresh=", "disable_perf"]
> > > if have_inet_diag:
> > > short += "n"
> > > long.append("show_sockets")
> > > @@ -528,6 +530,7 @@ def main():
> > > show_sockets = False
> > > p_waiting_action = False
> > > gui_refresh = 2500
> > > + disable_perf = False
> > >
> > > for o, a in opts:
> > > if o in ("-h", "--help"):
> > > @@ -588,6 +591,9 @@ def main():
> > > except Exception as err:
> > > print("tuna: --refresh %s" % err)
> > > sys.exit(2)
> > > + elif o in ("-d", "--disable_perf"):
> > > + run_gui = True
> > > + disable_perf = True
> > > elif o in ("-i", "--isolate"):
> > > if not cpu_list:
> > > print("tuna: --isolate " + _("requires a
cpu list!"))
> > > @@ -747,7 +753,7 @@ def main():
> > >
> > > try:
> > > cpus_filtered = filter and cpu_list or []
> > > - app = tuna_gui.main_gui(kthreads, uthreads, cpus_filtered,
gui_refresh)
> > > + app = tuna_gui.main_gui(kthreads, uthreads, cpus_filtered,
gui_refresh, disable_perf)
> > > app.run()
> > > except KeyboardInterrupt:
> > > pass
> > > diff --git a/tuna/gui/procview.py b/tuna/gui/procview.py
> > > index 199a4f1..c5b5f1f 100755
> > > --- a/tuna/gui/procview.py
> > > +++ b/tuna/gui/procview.py
> > > @@ -214,17 +214,18 @@ class procview:
> > > gui.list_store_column(_("Command Line"),
GObject.TYPE_STRING))
> > >
> > > def __init__(self, treeview, ps, show_kthreads, show_uthreads,
> > > - cpus_filtered, gladefile):
> > > + cpus_filtered, gladefile, disable_perf):
> > > self.ps = ps
> > > self.treeview = treeview
> > > self.nr_cpus = procfs.cpuinfo().nr_cpus
> > > self.gladefile = gladefile
> > >
> > > self.evlist = None
> > > - try:
> > > - self.perf_init()
> > > - except: # No perf, poll /proc baby, poll
> > > - pass
> > > + if not disable_perf:
> > > + try:
> > > + self.perf_init()
> > > + except: # No perf, poll /proc baby, poll
> > > + pass
> > >
> > > if "voluntary_ctxt_switches" not in
ps[1]["status"]:
> > > self.nr_columns = 5
> > > diff --git a/tuna/tuna_gui.py b/tuna/tuna_gui.py
> > > index 1296beb..83af063 100755
> > > --- a/tuna/tuna_gui.py
> > > +++ b/tuna/tuna_gui.py
> > > @@ -23,7 +23,7 @@ tuna_glade = None
> > >
> > > class main_gui:
> > >
> > > - def __init__(self, show_kthreads=True, show_uthreads=True,
cpus_filtered=[], refresh_time=2500):
> > > + def __init__(self, show_kthreads=True, show_uthreads=True,
cpus_filtered=[], refresh_time=2500, disable_perf=False):
> > > global tuna_glade
> > >
> > > (app, localedir) = ('tuna', '/usr/share/locale')
> > > @@ -46,7 +46,7 @@ class main_gui:
> > > self.procview = procview(
> > > self.wtree.get_object("processlist"),
> > > self.ps, show_kthreads, show_uthreads,
> > > - cpus_filtered, tuna_glade)
> > > + cpus_filtered, tuna_glade, disable_perf)
> > > self.irqview = irqview(
> > > self.wtree.get_object("irqlist"),
> > > self.irqs, self.ps, cpus_filtered,
> > > --
> > > 2.26.3
> > >
> > _______________________________________________
> > tuna-devel mailing list -- tuna-devel(a)lists.fedorahosted.org
> > To unsubscribe send an email to tuna-devel-leave(a)lists.fedorahosted.org
> > Fedora Code of Conduct:
https://docs.fedoraproject.org/en-US/project/code-of-conduct/
> > List Guidelines:
https://fedoraproject.org/wiki/Mailing_list_guidelines
> > List Archives:
https://lists.fedorahosted.org/archives/list/tuna-devel@lists.fedorahoste...
> > Do not reply to spam on the list, report it:
https://pagure.io/fedora-infrastructure
> >
> _______________________________________________
> tuna-devel mailing list -- tuna-devel(a)lists.fedorahosted.org
> To unsubscribe send an email to tuna-devel-leave(a)lists.fedorahosted.org
> Fedora Code of Conduct:
https://docs.fedoraproject.org/en-US/project/code-of-conduct/
> List Guidelines:
https://fedoraproject.org/wiki/Mailing_list_guidelines
> List Archives:
https://lists.fedorahosted.org/archives/list/tuna-devel@lists.fedorahoste...
> Do not reply to spam on the list, report it:
https://pagure.io/fedora-infrastructure