Closed rostedt closed 1 year ago
Thanks for doing this; it was indeed really overdue
Build error on current release Debian 11 Bullseye:
devices/i915-gpu.cpp:` In function ‘void create_i915_gpu()’:
devices/i915-gpu.cpp:83:7: error: ‘tracefs_event_file_exists’ was not declared in this scope; did you mean ‘tracefs_file_exists’?
83 | if (!tracefs_event_file_exists(NULL, "i915", "i915_gem_ring_dispatch", "format")) {
| ^~~~~~~~~~~~~~~~~~~~~~~~~
| `tracefs_file_exists
racefs_event_file_exists()
is missing in the Debian libtracefs 1.0.2-1 package.
It can be made to compile using updates from Debian Bookworm (yet to be released, apparently expected mid 2023). Mixing packages could cause problems elsewhere because it updates lots of other packages such as libc, libnl.
After adding Bookworm to the apt configuration in /etc/apt:
apt update
apt -t bookworm install libtraceevent-dev
apt -t bookworm install libtracefs-dev
On Sat, 11 Mar 2023 04:13:24 -0800 John @.***> wrote:
Build error on Debian 11:
devices/i915-gpu.cpp:
In function ‘void create_i915_gpu()’: devices/i915-gpu.cpp:83:7: error: ‘tracefs_event_file_exists’ was not declared in this scope; did you mean ‘tracefs_file_exists’? 83 | if (!tracefs_event_file_exists(NULL, "i915", "i915_gem_ring_dispatch", "format")) { | ^~~~~~~~~ |tracefs_file_exists
tracefs_event_file_exists() was introduced in libtracefs 1.3
This may have been built with an older version. I probably should have added a minimum version in the configure file.
-- Steve
Over 10 years powertop had its own copy of libtraceevent with the assumption that it would eventually use the library when it became stable. With the following stated in the code:
The traceevent library being built in PowerTOP is really a effort by Steven Rostedt. The long term intent is for Steven to push the trace event library to distributions, and consume it externally. Right now the PowerTOP project is keeping in sync with his code posted in the Linux kernel git under tools/lib/traceevent. We will not take patches into the traceevent code directly, rather we will be re-basing against that code base.
Well, it's been a few years now that libtraceevent is out in the wild and in distributions. In fact, there's also a libtracefs that is a superset of libtraceevent that also handles managing of the tracefs file system. Namely, for what powertop is concerned with, is where tracefs may be mounted. Not all distros mount it at /sys/kernel/debug/tracing. And having that path hard coded is error prone.
This pull request is broken up into three commits.
1) Remove the copy of libtraceevent and use the libraries directly. But does not take advantage of the tracefs functions.
Note, I noticed that tracefs.h causes a warning that requires adding -fpermissive to the CFLAGS. I'll add a fix for this, but that may not land early enough to not have that flag. It's due to an static inline function using zero instead of the enum.
2) Convert add_event() to pass in system_name and event_name instead of using "system:event". This allows to use the tracefs_event_file_read() function that will read the format that can be passed to tep_parse_event(). This simplifies the code a bit.
3) Replace the hard coded paths in devices/i915-gpu.cpp and use the tracefs_event_file_exists() instead.
I'm still rather new to github workflow (I rather send email patches). But I'll give you this pull request. Do what you want with it. I ran it on my machine, but you may want to run more tests incase I messed something up.