Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

PPD-to-TuneD API translation daemon #579

Merged
merged 3 commits into from
Feb 7, 2024

Conversation

zacikpa
Copy link
Contributor

@zacikpa zacikpa commented Dec 7, 2023

This PR (currently draft) will introduce a PPD-to-TuneD API translation daemon. Feel free to comment on the implementation during the development process.

@smallorange
Copy link

Hi @zacikpa ,

Thank you for working on this.
I would like to run the patches and test it with GNOME. How could I get it running?

@zacikpa
Copy link
Contributor Author

zacikpa commented Dec 19, 2023

Hi @smallorange,
this is not yet ready for testing. I'll tag you here when it is.

@zacikpa zacikpa force-pushed the ppd-daemon branch 5 times, most recently from a6db80b to 359053c Compare December 19, 2023 17:13
@zacikpa
Copy link
Contributor Author

zacikpa commented Dec 19, 2023

@smallorange This is now in a state where it supports profile switching, but it still has some issues.

You can try the following:

  1. Disable PPD:
systemctl disable --now power-profiles-daemon
systemctl mask power-profiles-daemon
  1. In a directory with cloned TuneD, run TuneD manually from the command line:
sudo ./tuned.py
  1. Let TuneD run, open another command line, and run the translation daemon:
sudo ./tuned-ppd.py

So far, I tested this with gnome-control-center. Profile switching works fine, but the daemon does not yet support signalling (and therefore gnome-control-center does not always show the active profile). I haven't yet tested the hold functionality, either.

Once you're done playing with it, don't forget to unmask and enable PPD (if you use it).

@smallorange
Copy link

Hi @zacikpa,

I've tested it. It works great! I can switch the profile through gnome control center.

Thank you :)

@zacikpa zacikpa force-pushed the ppd-daemon branch 2 times, most recently from 7e30a9a to 954d3c1 Compare January 5, 2024 08:50
@zacikpa zacikpa requested a review from yarda January 9, 2024 15:33
@zacikpa
Copy link
Contributor Author

zacikpa commented Jan 9, 2024

At this point, the daemon supports all capabilities of PPD except for the "trickle charging" action:

  • setting and querying the active profile using the ActiveProfile property,
  • holding and releasing profiles,
  • querying the current list of holds,
  • the PerformanceDegraded property, which is set to True when the Intel P-State's no_turbo option is on or when a Thinkpad device is in lap mode. This is currently implemented via trivial polling of sysfs options.

@zacikpa
Copy link
Contributor Author

zacikpa commented Jan 16, 2024

In the spec file, I now added the daemon as a subpackage of TuneD called tuned-ppd. The daemon exports the same DBus object and interface as PPD, so it conflicts with PPD (though I'm not entirely sure that such conflicts are allowed in Fedora).

This way, applications that currently use PPD can transition to TuneD seamlessly. An alternative option would be to provide a different object (e.g. com.redhat.tuned.PowerProfiles) which would export PPD's interface. That would, however, require changes in those applications.

@zacikpa zacikpa marked this pull request as ready for review January 17, 2024 13:36
@KyleGospo
Copy link

@zacikpa You may also want this:
KyleGospo@f55f1bd

I can say this is working great in my testing!

tuned.spec Outdated
Requires: %{name} = %{version}
# The compatibility daemon conflicts with PPD,
# since it exports the same DBus object.
Conflicts: power-profiles-daemon

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is fine, but we probably want to adjust both power-profiles-daemon and tuned-ppd packaging to Provide+Conflicts so that it becomes swappable. Maybe something like:

Provides: ppd-service
Conflicts: ppd-service

And we'll need to get consumers of power-profiles-daemon to require ppd-service instead.

@yarda
Copy link
Contributor

yarda commented Jan 18, 2024

@zacikpa, now when the development of the feature is nearly completed, could you please squash the commits? It would be easier to handle/review and possibly backport.

@zacikpa
Copy link
Contributor Author

zacikpa commented Jan 22, 2024

@zacikpa You may also want this: KyleGospo@f55f1bd

I can say this is working great in my testing!

Thanks for the patch Kyle, now the daemon can be started as a dependency, e.g., by gnome on startup.

@zacikpa
Copy link
Contributor Author

zacikpa commented Jan 22, 2024

@zacikpa, now when the development of the feature is nearly completed, could you please squash the commits? It would be easier to handle/review and possibly backport.

I now squashed the changes into 3 commits for review, I can then squash into a single one when approved.

@yarda
Copy link
Contributor

yarda commented Jan 23, 2024

@zacikpa, now when the development of the feature is nearly completed, could you please squash the commits? It would be easier to handle/review and possibly backport.

I now squashed the changes into 3 commits for review, I can then squash into a single one when approved.

I think it's OK now, thanks.

Copy link
Contributor

@yarda yarda left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, good job. It mostly LGTM. I have few minor comments inline and one RFE (configuration file).

I also ran simple tests and it seems it works great.

I noticed it returns high-operating-temperature reason as the degraded perf in case turbo is disabled. In fact turbo can be disabled for various reasons, even manually, i.e. it needn't be due to the high platform temperature. But from the PPD source code, it seems it behaves exactly the same way, i.e. it's probably PPD bug and we should emulate it this (even buggy) way.

tuned/ppd/tuned-ppd.service Show resolved Hide resolved
tuned-ppd.py Outdated Show resolved Hide resolved
tuned/ppd/controller.py Outdated Show resolved Hide resolved
LAP_MODE_PATH = "/sys/bus/platform/devices/thinkpad_acpi/dytc_lapmode"


class PPDProfile(StrEnum):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you use here configuration file with profiles mapping? I think it would be more flexible. The configuration file could be e.g. /etc/tuned/ppd.conf and could e.g. be the INI with e.g. the following format:

[main]
base=balanced

[hold]
min=power-saver
max=performance

[profiles]
# PPD = TuneD
power-saver = powersave
balanced = balanced
performance = throughput-performance

Then the users could customize it or even add more profiles.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, I can do that. What exactly do you propose that the hold section does? Currently, the daemon allows holding only power-saver and performance profiles, similar to PPD.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, I can do that. What exactly do you propose that the hold section does? Currently, the daemon allows holding only power-saver and performance profiles, similar to PPD.

I wanted to map min performance and max performance hold profiles to PPD profile names, but because the mapping to TuneD profiles would be already under the [profiles] section, this is probably redundant abstraction. Let's don't over-engineer it and say power-saver and performance profiles mapping have to be always defined in the [profiles] section.

Otherwise I think it shouldn't be hard change, just one more INI parser and I think it's worth the benefit of greater flexibility.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right, I agree, let's keep it simple.

tuned/ppd/controller.py Outdated Show resolved Hide resolved
@zacikpa
Copy link
Contributor Author

zacikpa commented Jan 25, 2024

I think it could shutdown without traceback on the first CTRL+C.

@yarda, I just fixed this using the same method as TuneD does it. Once the profile mapping is moved to a configuration file, I suppose it also makes sense to handle the SIGHUP signal to reload the config.

@zacikpa
Copy link
Contributor Author

zacikpa commented Jan 25, 2024

Had few reports in our tests about extremely long shutdown times. I was able to reproduce it by reaching the desktop, switching to the performance option in GNOME/KDE, and then shutting down - where it'll hang waiting for dbus to end.

Thanks for the heads up. So far I haven't been able to reproduce on my system, but I'd be happy to investigate once you've got the logs.

@yarda
Copy link
Contributor

yarda commented Jan 25, 2024

I think it could shutdown without traceback on the first CTRL+C.

@yarda, I just fixed this using the same method as TuneD does it. Once the profile mapping is moved to a configuration file, I suppose it also makes sense to handle the SIGHUP signal to reload the config.

SIGHUP would be nice :) but it can be also added later.

Do not hardcode Polkit namespace into DBusExporter,
provide it as a parameter in the constructor.
@zacikpa zacikpa force-pushed the ppd-daemon branch 4 times, most recently from 7a17c71 to 0ddf1eb Compare February 6, 2024 16:51
@zacikpa
Copy link
Contributor Author

zacikpa commented Feb 6, 2024

As requested, I changed the implementation to load the profile mapping from a configuration file, which must fulfill three conditions:

  • the profile mapping must be one-to-one (so that we can translate both ways)
  • the two PPD profiles that can be held, power-saver and performance, must be present
  • the default profile must be present in the profile mapping.

@zacikpa
Copy link
Contributor Author

zacikpa commented Feb 6, 2024

Let me know if I should add any more checks (i.e., warnings when the config file contains unknown options).

@zacikpa zacikpa requested a review from yarda February 6, 2024 16:55
Copy link
Contributor

@yarda yarda left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If TuneD is not running it ends with a traceback. I think it could write some legible error message instead:

Traceback (most recent call last):
  File "/usr/lib64/python3.12/site-packages/dbus/bus.py", line 173, in activate_name_owner
    return self.get_name_owner(bus_name)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/usr/lib64/python3.12/site-packages/dbus/bus.py", line 348, in get_name_owner
    return self.call_blocking(BUS_DAEMON_NAME, BUS_DAEMON_PATH,
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/usr/lib64/python3.12/site-packages/dbus/connection.py", line 634, in call_blocking
    reply_message = self.send_message_with_reply_and_block(
                    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
dbus.exceptions.DBusException: org.freedesktop.DBus.Error.NameHasNoOwner: The name does not have an owner

During handling of the above exception, another exception occurred:

Traceback (most recent call last):
  File "/home/yarda/tuned-private/./tuned-ppd.py", line 25, in <module>
    tuned_object = bus.get_object(consts.DBUS_BUS, consts.DBUS_OBJECT)
                   ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/usr/lib64/python3.12/site-packages/dbus/bus.py", line 237, in get_object
    return self.ProxyObjectClass(self, bus_name, object_path,
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/usr/lib64/python3.12/site-packages/dbus/proxies.py", line 250, in __init__
    self._named_service = conn.activate_name_owner(bus_name)
                          ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/usr/lib64/python3.12/site-packages/dbus/bus.py", line 178, in activate_name_owner
    self.start_service_by_name(bus_name)
  File "/usr/lib64/python3.12/site-packages/dbus/bus.py", line 273, in start_service_by_name
    return (True, self.call_blocking(BUS_DAEMON_NAME, BUS_DAEMON_PATH,
                  ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/usr/lib64/python3.12/site-packages/dbus/connection.py", line 634, in call_blocking
    reply_message = self.send_message_with_reply_and_block(
                    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
dbus.exceptions.DBusException: org.freedesktop.DBus.Error.ServiceUnknown: The name is not activatable

Also I don't see the 'ActiveProfile` property in the introspection. E.g. compare introspections of the tuned-ppd and original ppd:

...
+ '    <property type="s" name="ActiveProfile" access="readwrite">\n'
+ '    </property>\n'
...

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not to name it ppd.conf?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This could be just dbus.conf.

@yarda
Copy link
Contributor

yarda commented Feb 7, 2024

Let me know if I should add any more checks (i.e., warnings when the config file contains unknown options).

I think the current checks are OK, let's be sane :)

@zacikpa
Copy link
Contributor Author

zacikpa commented Feb 7, 2024

If TuneD is not running it ends with a traceback. I think it could write some legible error message instead:

Good idea, will fix.

Also I don't see the 'ActiveProfile` property in the introspection. E.g. compare introspections of the tuned-ppd and original ppd.

Yes, python-dbus unfortunately does not support exporting properties via the introspection interface (see discussion in this ancient pull request, TL;DR: the maintainer is against supporting it due to backward compatibility and argues that introspection should only be used for debugging).

tuned-ppd.py Fixed Show resolved Hide resolved
@yarda
Copy link
Contributor

yarda commented Feb 7, 2024

Also I don't see the 'ActiveProfile` property in the introspection. E.g. compare introspections of the tuned-ppd and original ppd.

Yes, python-dbus unfortunately does not support exporting properties via the introspection interface (see discussion in this ancient pull request, TL;DR: the maintainer is against supporting it due to backward compatibility and argues that introspection should only be used for debugging).

OK, thanks for the info. Nobody has complained so far, so let's go without it and we will see.

@yarda
Copy link
Contributor

yarda commented Feb 7, 2024

LGTM (not counting the unused import found by CodeQL).

The daemon translates DBus API calls
to power-profiles-daemon into TuneD API calls.
This includes the configuration of DBus, systemd, Polkit, and a new
TuneD Fedora/RHEL subpackage.
@zacikpa
Copy link
Contributor Author

zacikpa commented Feb 7, 2024

Fixed and squashed.

@yarda yarda merged commit 11c6c57 into redhat-performance:master Feb 7, 2024
13 of 14 checks passed
@yarda
Copy link
Contributor

yarda commented Feb 7, 2024

Thanks, merged.

Jiri-Koten added a commit to Jiri-Koten/content-resolver-input that referenced this pull request Feb 12, 2024
power-profiles-daemon will be replaced by tuned-ppd, see redhat-performance/tuned#579.
yselkowitz pushed a commit to Jiri-Koten/content-resolver-input that referenced this pull request Feb 12, 2024
power-profiles-daemon will be replaced by tuned-ppd, see redhat-performance/tuned#579.
yselkowitz pushed a commit to minimization/content-resolver-input that referenced this pull request Feb 12, 2024
power-profiles-daemon will be replaced by tuned-ppd, see redhat-performance/tuned#579.
notroj added a commit to notroj/content-resolver-input that referenced this pull request Feb 13, 2024
yselkowitz pushed a commit to notroj/content-resolver-input that referenced this pull request Feb 15, 2024
yselkowitz pushed a commit to minimization/content-resolver-input that referenced this pull request Feb 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants