Discussion:
[PATCH] procview: limit refresh rate in perf mode to refresh_time
John Kacur
2021-04-13 20:38:03 UTC
Permalink
When in perf mode (so not polling) previously the GUI would be
refreshed at the arrival of any single perf event. This may mean
that it would get refreshed all the time especially on quite loaded
systems, causing very high CPU load and unuseable GUI.
The patch limits the refresh to the minimum refresh_time (already
used for polling and configurable via -R command line by the user)
to prevent such cases. If no events arrive, the GUI will still not
be refreshed at all as before.
---
tuna/gui/procview.py | 14 ++++++++++----
tuna/tuna_gui.py | 2 +-
2 files changed, 11 insertions(+), 5 deletions(-)
diff --git a/tuna/gui/procview.py b/tuna/gui/procview.py
index c224e4b..dc2e94e 100755
--- a/tuna/gui/procview.py
+++ b/tuna/gui/procview.py
self.treeview = treeview
self.nr_cpus = procfs.cpuinfo().nr_cpus
self.gladefile = gladefile
+ self.evlist_added = True
self.evlist = None
self.perf_counter[tid] = event.sample_period
- self.show()
+ self.evlist_added = True # Mark that event arrived, so next periodic show() will refresh GUI
return True
# create the rows.
return
- row = self.tree_store.get_iter_first()
- self.update_rows(self.ps, row, None)
- self.treeview.show_all()
+
+ # If using perf only refresh if we saw at least a new event since last refresh
+ self.evlist_added = None
+
+ row = self.tree_store.get_iter_first()
+ self.update_rows(self.ps, row, None)
+ self.treeview.show_all()
new_tids = list(threads.keys())
diff --git a/tuna/tuna_gui.py b/tuna/tuna_gui.py
index 83af063..f1f2caa 100755
--- a/tuna/tuna_gui.py
+++ b/tuna/tuna_gui.py
if not self.procview.evlist: # Poll, as we don't have perf
self.ps.reload()
self.ps.reload_threads()
- self.procview.show()
+ self.procview.show()
self.irqview.refresh()
return True
--
2.26.3
I like the idea but the patch does not apply cleanly, do you want to take
a look?

Thanks

John
_______________________________________________
tuna-devel mailing list -- tuna-***@lists.fedorahosted.org
To unsubscribe send an email to tuna-devel-***@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-***@lists.fedorahosted.org
Do not reply to spam on the list, report it: https://pagure.io/fedora-i
Federico Pellegrin
2021-04-14 03:15:46 UTC
Permalink
Hi John,
Thanks for the feedback!

This patch should be applied on the top of the 9/9 of the previous bunch
(this one:
https://lists.fedorahosted.org/archives/list/tuna-***@lists.fedorahosted.org/thread/JAC5IL6A6GNFL2MI5EPS3NEPWA6PWCLU/
) that is still pending, as I was assuming that could go in too ([PATCH
9/9] tuna_gui: add command line option to explicitly disable perf usage). I
believe if you apply that too, then it should be fine.

In my opinion that 9/9 patch should be still useful, as it covers still
some use cases, depending what information one is after (since ie. when in
perf mode you don't get context switches or so), which cannot be fulfilled
if we force always the user to go (when automatically detected on the
system) in "perf mode".

Of course if you don't agree with that one (or if applying both still
creates problems), please just let me know and I'll immediately submit a
new version of this patch that doesn't rely it, so it can be merged cleanly.

Thanks!
Federico
When in perf mode (so not polling) previously the GUI would be
refreshed at the arrival of any single perf event. This may mean
that it would get refreshed all the time especially on quite loaded
systems, causing very high CPU load and unuseable GUI.
The patch limits the refresh to the minimum refresh_time (already
used for polling and configurable via -R command line by the user)
to prevent such cases. If no events arrive, the GUI will still not
be refreshed at all as before.
---
tuna/gui/procview.py | 14 ++++++++++----
tuna/tuna_gui.py | 2 +-
2 files changed, 11 insertions(+), 5 deletions(-)
diff --git a/tuna/gui/procview.py b/tuna/gui/procview.py
index c224e4b..dc2e94e 100755
--- a/tuna/gui/procview.py
+++ b/tuna/gui/procview.py
self.treeview = treeview
self.nr_cpus = procfs.cpuinfo().nr_cpus
self.gladefile = gladefile
+ self.evlist_added = True
self.evlist = None
self.perf_counter[tid] = event.sample_period
- self.show()
+ self.evlist_added = True # Mark that event arrived, so next
periodic show() will refresh GUI
return True
# create the rows.
return
- row = self.tree_store.get_iter_first()
- self.update_rows(self.ps, row, None)
- self.treeview.show_all()
+
+ # If using perf only refresh if we saw at least a new event
since last refresh
+ self.evlist_added = None
+
+ row = self.tree_store.get_iter_first()
+ self.update_rows(self.ps, row, None)
+ self.treeview.show_all()
new_tids = list(threads.keys())
diff --git a/tuna/tuna_gui.py b/tuna/tuna_gui.py
index 83af063..f1f2caa 100755
--- a/tuna/tuna_gui.py
+++ b/tuna/tuna_gui.py
if not self.procview.evlist: # Poll, as we don't have perf
self.ps.reload()
self.ps.reload_threads()
- self.procview.show()
+ self.procview.show()
self.irqview.refresh()
return True
--
2.26.3
I like the idea but the patch does not apply cleanly, do you want to take
a look?
Thanks
John
Federico Pellegrin
2021-05-10 04:55:13 UTC
Permalink
Hi John,
Sorry just a "ping" on this still pending patch (or better two patches if
you agree with what I mentioned in the previous email).

Many thanks!
Federico


Il giorno mer 14 apr 2021 alle ore 05:15 Federico Pellegrin <
Post by Federico Pellegrin
Hi John,
Thanks for the feedback!
This patch should be applied on the top of the 9/9 of the previous bunch
) that is still pending, as I was assuming that could go in too ([PATCH
9/9] tuna_gui: add command line option to explicitly disable perf usage). I
believe if you apply that too, then it should be fine.
In my opinion that 9/9 patch should be still useful, as it covers still
some use cases, depending what information one is after (since ie. when in
perf mode you don't get context switches or so), which cannot be fulfilled
if we force always the user to go (when automatically detected on the
system) in "perf mode".
Of course if you don't agree with that one (or if applying both still
creates problems), please just let me know and I'll immediately submit a
new version of this patch that doesn't rely it, so it can be merged cleanly.
Thanks!
Federico
When in perf mode (so not polling) previously the GUI would be
refreshed at the arrival of any single perf event. This may mean
that it would get refreshed all the time especially on quite loaded
systems, causing very high CPU load and unuseable GUI.
The patch limits the refresh to the minimum refresh_time (already
used for polling and configurable via -R command line by the user)
to prevent such cases. If no events arrive, the GUI will still not
be refreshed at all as before.
---
tuna/gui/procview.py | 14 ++++++++++----
tuna/tuna_gui.py | 2 +-
2 files changed, 11 insertions(+), 5 deletions(-)
diff --git a/tuna/gui/procview.py b/tuna/gui/procview.py
index c224e4b..dc2e94e 100755
--- a/tuna/gui/procview.py
+++ b/tuna/gui/procview.py
self.treeview = treeview
self.nr_cpus = procfs.cpuinfo().nr_cpus
self.gladefile = gladefile
+ self.evlist_added = True
self.evlist = None
self.perf_counter[tid] =
event.sample_period
- self.show()
+ self.evlist_added = True # Mark that event arrived, so next
periodic show() will refresh GUI
return True
# create the rows.
return
- row = self.tree_store.get_iter_first()
- self.update_rows(self.ps, row, None)
- self.treeview.show_all()
+
+ # If using perf only refresh if we saw at least a new event
since last refresh
+ self.evlist_added = None
+
+ row = self.tree_store.get_iter_first()
+ self.update_rows(self.ps, row, None)
+ self.treeview.show_all()
new_tids = list(threads.keys())
diff --git a/tuna/tuna_gui.py b/tuna/tuna_gui.py
index 83af063..f1f2caa 100755
--- a/tuna/tuna_gui.py
+++ b/tuna/tuna_gui.py
if not self.procview.evlist: # Poll, as we don't have perf
self.ps.reload()
self.ps.reload_threads()
- self.procview.show()
+ self.procview.show()
self.irqview.refresh()
return True
--
2.26.3
I like the idea but the patch does not apply cleanly, do you want to take
a look?
Thanks
John
John Kacur
2021-05-13 20:41:02 UTC
Permalink
Post by Federico Pellegrin
Hi John,
Sorry just a "ping" on this still pending patch (or better two patches if you agree with what I mentioned in the previous email).
Don't be afraid to ping me, I appreciate it. I think it would be easier
for me if you would [re]send any pending patches so that I can either
apply them, or send some feedback. This would be easier than slogging
through old mails.

Thanks

John
Post by Federico Pellegrin
Many thanks!
Federico
Hi John,
Thanks for the feedback!
too ([PATCH 9/9] tuna_gui: add command line option to explicitly disable perf usage). I believe if you apply that too, then it should be fine.
In my opinion that 9/9 patch should be still useful, as it covers still some use cases, depending what information one is after (since ie. when in perf mode you don't get context
switches or so), which cannot be fulfilled if we force always the user to go (when automatically detected on the system) in "perf mode".
Of course if you don't agree with that one (or if applying both still creates problems), please just let me know and I'll immediately submit a new version of this patch that
doesn't rely it, so it can be merged cleanly.
Thanks!
Federico
When in perf mode (so not polling) previously the GUI would be
refreshed at the arrival of any single perf event. This may mean
that it would get refreshed all the time especially on quite loaded
systems, causing very high CPU load and unuseable GUI.
The patch limits the refresh to the minimum refresh_time (already
used for polling and configurable via -R command line by the user)
to prevent such cases. If no events arrive, the GUI will still not
be refreshed at all as before.
---
  tuna/gui/procview.py | 14 ++++++++++----
  tuna/tuna_gui.py     |  2 +-
  2 files changed, 11 insertions(+), 5 deletions(-)
diff --git a/tuna/gui/procview.py b/tuna/gui/procview.py
index c224e4b..dc2e94e 100755
--- a/tuna/gui/procview.py
+++ b/tuna/gui/procview.py
          self.treeview = treeview
          self.nr_cpus = procfs.cpuinfo().nr_cpus
          self.gladefile = gladefile
+        self.evlist_added = True
 
          self.evlist = None
                              self.perf_counter[tid] = event.sample_period
 
-        self.show()
+        self.evlist_added = True  # Mark that event arrived, so next periodic show() will refresh GUI
          return True
 
          # create the rows.
              return
-        row = self.tree_store.get_iter_first()
-        self.update_rows(self.ps, row, None)
-        self.treeview.show_all()
+
+        # If using perf only refresh if we saw at least a new event since last refresh
+            self.evlist_added = None
+
+            row = self.tree_store.get_iter_first()
+            self.update_rows(self.ps, row, None)
+            self.treeview.show_all()
 
          new_tids = list(threads.keys())
diff --git a/tuna/tuna_gui.py b/tuna/tuna_gui.py
index 83af063..f1f2caa 100755
--- a/tuna/tuna_gui.py
+++ b/tuna/tuna_gui.py
          if not self.procview.evlist: # Poll, as we don't have perf
              self.ps.reload()
              self.ps.reload_threads()
-            self.procview.show()
+        self.procview.show()
          self.irqview.refresh()
          return True
 
--
2.26.3
I like the idea but the patch does not apply cleanly, do you want to take
a look?
Thanks
John
Federico Pellegrin
2021-05-14 06:59:08 UTC
Permalink
Hi John,
No problem, I've resent the patches a bit ago, so it's easier to see them,
since indeed they got quite some other mail on top in the meantime :-)

I've reversed the order of them, putting the refresh limit one (that you
already gave a look, but then wasn't applying) first so it's easier to
apply, while the new option to be able to disable perf second. I've also
updated the manpage for the second patch as I was resubmitting it, should
you decide it's a good thing to apply.

Thanks!
Federico
Post by Federico Pellegrin
Post by Federico Pellegrin
Hi John,
Sorry just a "ping" on this still pending patch (or better two patches
if you agree with what I mentioned in the previous email).
Don't be afraid to ping me, I appreciate it. I think it would be easier
for me if you would [re]send any pending patches so that I can either
apply them, or send some feedback. This would be easier than slogging
through old mails.
Thanks
John
Post by Federico Pellegrin
Many thanks!
Federico
Il giorno mer 14 apr 2021 alle ore 05:15 Federico Pellegrin <
Hi John,
Thanks for the feedback!
This patch should be applied on the top of the 9/9 of the previous bunch
) that is still pending, as I was assuming that could go in
Post by Federico Pellegrin
too ([PATCH 9/9] tuna_gui: add command line option to explicitly disable
perf usage). I believe if you apply that too, then it should be fine.
Post by Federico Pellegrin
In my opinion that 9/9 patch should be still useful, as it covers still
some use cases, depending what information one is after (since ie. when in
perf mode you don't get context
Post by Federico Pellegrin
switches or so), which cannot be fulfilled if we force always the user
to go (when automatically detected on the system) in "perf mode".
Post by Federico Pellegrin
Of course if you don't agree with that one (or if applying both still
creates problems), please just let me know and I'll immediately submit a
new version of this patch that
Post by Federico Pellegrin
doesn't rely it, so it can be merged cleanly.
Thanks!
Federico
When in perf mode (so not polling) previously the GUI would be
refreshed at the arrival of any single perf event. This may mean
that it would get refreshed all the time especially on quite
loaded
Post by Federico Pellegrin
systems, causing very high CPU load and unuseable GUI.
The patch limits the refresh to the minimum refresh_time (already
used for polling and configurable via -R command line by the
user)
Post by Federico Pellegrin
to prevent such cases. If no events arrive, the GUI will still
not
Post by Federico Pellegrin
be refreshed at all as before.
---
tuna/gui/procview.py | 14 ++++++++++----
tuna/tuna_gui.py | 2 +-
2 files changed, 11 insertions(+), 5 deletions(-)
diff --git a/tuna/gui/procview.py b/tuna/gui/procview.py
index c224e4b..dc2e94e 100755
--- a/tuna/gui/procview.py
+++ b/tuna/gui/procview.py
self.treeview = treeview
self.nr_cpus = procfs.cpuinfo().nr_cpus
self.gladefile = gladefile
+ self.evlist_added = True
self.evlist = None
self.perf_counter[tid] =
event.sample_period
Post by Federico Pellegrin
- self.show()
+ self.evlist_added = True # Mark that event arrived, so
next periodic show() will refresh GUI
Post by Federico Pellegrin
return True
# create the rows.
return
- row = self.tree_store.get_iter_first()
- self.update_rows(self.ps, row, None)
- self.treeview.show_all()
+
+ # If using perf only refresh if we saw at least a new
event since last refresh
Post by Federico Pellegrin
+ self.evlist_added = None
+
+ row = self.tree_store.get_iter_first()
+ self.update_rows(self.ps, row, None)
+ self.treeview.show_all()
new_tids = list(threads.keys())
diff --git a/tuna/tuna_gui.py b/tuna/tuna_gui.py
index 83af063..f1f2caa 100755
--- a/tuna/tuna_gui.py
+++ b/tuna/tuna_gui.py
if not self.procview.evlist: # Poll, as we don't have
perf
Post by Federico Pellegrin
self.ps.reload()
self.ps.reload_threads()
- self.procview.show()
+ self.procview.show()
self.irqview.refresh()
return True
--
2.26.3
I like the idea but the patch does not apply cleanly, do you want
to take
Post by Federico Pellegrin
a look?
Thanks
John
Loading...