Register spawned applications as an independent cgroups
ClosedPublic

Authored by davidedmundson on Mar 6 2020, 9:02 AM.

Details

Summary

Having each application in it's own scope brings numerous advantages:

  • PIDs are very yesteryear, a modern application has a tonne of

processes. We want some sort of logical grouping in ksysguard.
We have a working version of this.

It also can resolve the case of correctly matching the
audio indicator to the correct application in the task manager.
Something that has been proven to not work reliably by tracking parent
PIDs.

  • We can use the cgroup freezer controller on plasma-mobile to suspend

background processes.

  • We want to put things into the correct slice.

Future systemd will split user.slice into 3 subslices, background
services, apps and chrome (with chrome being plasmashell and kwin in our
case). Putting applications in the correct slice will bring pre-bumped
niceness levels.

  • We also can expose cgroup resources limits
  • Things get cleaned up logout without relying on xsmp.

This is analogous to Gnome's recent change:
https://gitlab.gnome.org/GNOME/gnome-shell/-/merge_requests/863/diffs

It was also discussed in person at a very-mini meeting at FOSDEM with
both systemd and gnome people.

Task T12678

If a relevant cgroup controller (systemd) is not running, this simply
no-ops without issue.

In addition things are guarded by an environment variable so we don't
introduce any behavioural changes to released Plasma's.


This change is incredibly minimal by launching as normal and then
tagging afterwards. It's a bit of a chicken and egg scenario as we merge
the intended usages of this change.

After things are established we will want to move spawning the
application to be responsiblity of the cgroup controller as transient
services rather than transient scopes. It will be more "technically
correct" and allow even more features such as improved logging.

Test Plan

Ran app from kickoff, started systemd-cgls
I've been using some variant of this on a private work project for months
Also ran against pending ksysguard modifications that showed applications grouped nicely

Diff Detail

Repository
R241 KIO
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
davidedmundson created this revision.Mar 6 2020, 9:02 AM
Restricted Application added a project: Frameworks. · View Herald TranscriptMar 6 2020, 9:02 AM
Restricted Application added a subscriber: kde-frameworks-devel. · View Herald Transcript
davidedmundson requested review of this revision.Mar 6 2020, 9:02 AM
apol added a subscriber: apol.Mar 9 2020, 1:56 AM
apol added inline comments.
src/widgets/kprocessrunner.cpp
51 ↗(On Diff #77071)

This could be in the function scope as static.

281 ↗(On Diff #77071)

Who defines this environment variable?

301 ↗(On Diff #77071)

Declare and define together

302 ↗(On Diff #77071)

*signatuRe

Also I'm not sure I understand...

308 ↗(On Diff #77071)

Does this need to be sync?

src/widgets/kprocessrunner.cpp
281 ↗(On Diff #77071)

I figured startplasma?

It's just so we don't change existing code with new frameworks

302 ↗(On Diff #77071)

Maybe unused wasn't the right term.

I was saying this list will be empty. Maybe that's an unhelpful comment as you can see it's empty from the code.

davidedmundson marked 6 inline comments as done.Mar 19 2020, 2:13 PM

rebase on David F's work

Relevant docs on the spec we're moving towards: https://github.com/systemd/systemd/pull/14846

dfaure requested changes to this revision.Mar 21 2020, 10:59 AM
dfaure added inline comments.
src/gui/kprocessrunner.cpp
299

The code in this method should be in some #ifdef UNIX and not MAC, right?
Pretty pointless to make this DBus call on Windows ;)

308

(this is what made me think about Windows, where pid is a 64bit value).

315

And then? if we don't care about the async reply, maybe you should remove the left part of the equal sign?

This revision now requires changes to proceed.Mar 21 2020, 10:59 AM

review comments

davidedmundson marked 3 inline comments as done.Mar 23 2020, 1:13 PM
dfaure accepted this revision.Mar 24 2020, 9:04 PM

It would be good to add a link to the spec as a code comment -- but I see that the docu isn't merged yet, so can't be done yet.

src/gui/kprocessrunner.cpp
43

unused? std::call_once and std::once_flag come from <mutex>

312

const

This revision is now accepted and ready to land.Mar 24 2020, 9:04 PM
This revision was automatically updated to reflect the committed changes.