D27883: Register spawned applications as an independent cgroups

[David Edmundson]

This revision was automatically updated to reflect the committed changes.
Closed by commit R241:a7d91ea3f0df: Register spawned applications as an 
independent cgroups (authored by davidedmundson).

CHANGED PRIOR TO COMMIT
  https://phabricator.kde.org/D27883?vs=78288=78462#toc

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D27883?vs=78288=78462

REVISION DETAIL
  https://phabricator.kde.org/D27883

AFFECTED FILES
  src/gui/kprocessrunner.cpp
  src/gui/kprocessrunner_p.h

To: davidedmundson, dfaure
Cc: apol, kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, ngraham, 
bruns

D27883: Register spawned applications as an independent cgroups

[David Faure]

dfaure accepted this revision.
dfaure added a comment.
This revision is now accepted and ready to land.


  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.

INLINE COMMENTS

> kprocessrunner.cpp:43
> +
> +#include 
> +#include 

unused? std::call_once and std::once_flag come from 

> kprocessrunner.cpp:307
> +
> +QList pidList = {static_cast(m_process->pid())};
> +

const

REPOSITORY
  R241 KIO

BRANCH
  master

REVISION DETAIL
  https://phabricator.kde.org/D27883

To: davidedmundson, dfaure
Cc: apol, kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, ngraham, 
bruns

D27883: Register spawned applications as an independent cgroups

[David Edmundson]

davidedmundson marked 3 inline comments as done.

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D27883

To: davidedmundson, dfaure
Cc: apol, kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, ngraham, 
bruns

D27883: Register spawned applications as an independent cgroups

[David Edmundson]

davidedmundson updated this revision to Diff 78288.
davidedmundson added a comment.


  review comments

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D27883?vs=78002=78288

BRANCH
  master

REVISION DETAIL
  https://phabricator.kde.org/D27883

AFFECTED FILES
  src/gui/kprocessrunner.cpp
  src/gui/kprocessrunner_p.h

To: davidedmundson, dfaure
Cc: apol, kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, ngraham, 
bruns

D27883: Register spawned applications as an independent cgroups

[David Faure]

dfaure requested changes to this revision.
dfaure added inline comments.
This revision now requires changes to proceed.

INLINE COMMENTS

> kprocessrunner.cpp:292
> +
> +QDBusMessage message = 
> QDBusMessage::createMethodCall(QStringLiteral("org.freedesktop.systemd1"),
> +   
> QStringLiteral("/org/freedesktop/systemd1"),

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

> kprocessrunner.cpp:301
> +
> +QList pidList = {static_cast(m_process->pid())};
> +

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

> kprocessrunner.cpp:308
> +message.setArguments({name, mode, QVariant::fromValue(properties), 
> QVariant::fromValue(aux)});
> +QDBusPendingCall reply = 
> QDBusConnection::sessionBus().asyncCall(message);
> +}

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

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D27883

To: davidedmundson, dfaure
Cc: apol, kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, ngraham, 
bruns

D27883: Register spawned applications as an independent cgroups

[David Edmundson]

davidedmundson added a reviewer: dfaure.

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D27883

To: davidedmundson, dfaure
Cc: apol, kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, ngraham, 
bruns

D27883: Register spawned applications as an independent cgroups

[David Edmundson]

davidedmundson added a comment.


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

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D27883

To: davidedmundson
Cc: apol, kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, ngraham, 
bruns

D27883: Register spawned applications as an independent cgroups

[David Edmundson]

davidedmundson updated this revision to Diff 78002.
davidedmundson added a comment.


  rebase on David F's work

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D27883?vs=77071=78002

BRANCH
  master

REVISION DETAIL
  https://phabricator.kde.org/D27883

AFFECTED FILES
  src/gui/kprocessrunner.cpp
  src/gui/kprocessrunner_p.h

To: davidedmundson
Cc: apol, kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, ngraham, 
bruns

D27883: Register spawned applications as an independent cgroups

[David Edmundson]

davidedmundson marked 6 inline comments as done.

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D27883

To: davidedmundson
Cc: apol, kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, ngraham, 
bruns

D27883: Register spawned applications as an independent cgroups

[David Edmundson]

davidedmundson added inline comments.

INLINE COMMENTS

> apol wrote in kprocessrunner.cpp:281
> Who defines this environment variable?

I figured startplasma?

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

> apol wrote in kprocessrunner.cpp:302
> *signatuRe
> 
> Also I'm not sure I understand...

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.

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D27883

To: davidedmundson
Cc: apol, kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, ngraham, 
bruns

D27883: Register spawned applications as an independent cgroups

[Aleix Pol Gonzalez]

apol added inline comments.

INLINE COMMENTS

> kprocessrunner.cpp:51
> +
> +std::once_flag dbusTypesRegistered;
> +

This could be in the function scope as static.

> kprocessrunner.cpp:281
> +{
> +if (!qEnvironmentVariableIsSet("KDE_APPLICATIONS_AS_SCOPE")) {
> +return;

Who defines this environment variable?

> kprocessrunner.cpp:301
> +const QString mode = QStringLiteral("fail"); // mode defines what to do 
> in the case of a name conflict, in this case, just do nothing
> +NamedVariantList properties;
> +QList> aux; // unused, but we need the 
> signatue to be correct;

Declare and define together

> kprocessrunner.cpp:302
> +NamedVariantList properties;
> +QList> aux; // unused, but we need the 
> signatue to be correct;
> +

*signatuRe

Also I'm not sure I understand...

> kprocessrunner.cpp:308
> +message.setArguments({name, mode, QVariant::fromValue(properties), 
> QVariant::fromValue(aux)});
> +QDBusConnection::sessionBus().call(message);
> +}

Does this need to be sync?

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D27883

To: davidedmundson
Cc: apol, kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, ngraham, 
bruns

D27883: Register spawned applications as an independent cgroups

[Nathaniel Graham]

ngraham added a task: T12678: Scope applications into transient services .

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D27883

To: davidedmundson
Cc: kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, ngraham, bruns

D27883: Register spawned applications as an independent cgroups

[David Edmundson]

davidedmundson created this revision.
Herald added a project: Frameworks.
Herald added a subscriber: kde-frameworks-devel.
davidedmundson requested review of this revision.

REVISION 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

REPOSITORY
  R241 KIO

BRANCH
  systemd_start

REVISION DETAIL
  https://phabricator.kde.org/D27883

AFFECTED FILES
  src/widgets/kprocessrunner.cpp
  src/widgets/kprocessrunner_p.h

To: davidedmundson
Cc: kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, ngraham, bruns