Differential D8356

Don't use the global KConfig object on the OpenGL freeze detection thread
ClosedPublic
Actions

Authored by jpalecek on Oct 18 2017, 1:02 PM.

Details

Reviewers
graesslin
Group Reviewers
KWin
Commits
R108:a7117e430454: Don't use the global KConfig object on the OpenGL freeze detection thread
Summary

This is meant to address Bug 372114 in KWin. The problem here is that the KConfig object (and its derivatives), that the freeze detection thread needs to record the freeze, are not thread safe. When it happens that the main thread is in fact not frozen, it is possible that the two stomp on each other's KConfig object.

The solution applied here is to use the KSharedConfig::openConfig function, which is thread safe, on the freeze detection thread. As was mentioned by Martin Flöser in the discussion, the thread needs to obey the name of the main config file of KWin, which can change in the future. Please read the whole discussion for details.

As a secondary issue, this patch also turns off KCrash reporting for aborts due to a freeze being detected. IMO it is not very user friendly to still show a crash report to the user, even after this bug is fixed, for the deliberate SIGABRT. Maybe a less intrusive notification could be used to tell the user why effects are suddenly disabled?

I've been using kwin with this change for several weeks now and it makes the restarts of kwin due to freezes unobtrusive. However, most (I would say almost all) of these freezes are actually instances where the system is being slow after eg. screen resolution is changed.

The patch applies to kwin 5.10.5 from Debian.

BUG: 372114

Diff Detail

Repository
R108 KWin
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
jpalecek created this revision.Oct 18 2017, 1:02 PM
Restricted Application added a project: KWin. · View Herald TranscriptOct 18 2017, 1:02 PM
Restricted Application added a subscriber: kwin. · View Herald Transcript
cfeck added a subscriber: cfeck.Oct 18 2017, 1:20 PM
cfeck added inline comments.
kwin-5.8.7/plugins/platforms/x11/standalone/CMakeLists.txt
17 ↗(On Diff #20942)

REQUIRED?

anthonyfieroni added a subscriber: anthonyfieroni.Oct 18 2017, 1:27 PM
anthonyfieroni added inline comments.
kwin-5.8.7/plugins/platforms/x11/standalone/x11_platform.cpp
217 ↗(On Diff #20942)

const

226 ↗(On Diff #20942)

Just replace qFatal with qCritical and do not stop DrKonqi.

jpalecek added inline comments.Oct 18 2017, 3:19 PM
kwin-5.8.7/plugins/platforms/x11/standalone/CMakeLists.txt
17 ↗(On Diff #20942)

Indeed. However I wasn't sure if it's really needed because it's already required at the top level CMakeLists.txt.

kwin-5.8.7/plugins/platforms/x11/standalone/x11_platform.cpp
226 ↗(On Diff #20942)

But that wouldn't terminate kwin (unless some environment variable is set...), would it? We really need that.

graesslin added a subscriber: graesslin.Oct 18 2017, 3:49 PM

Thanks for putting this on phab! Looks good to me, I like the idea with disabling KCrash.

kwin-5.8.7/plugins/platforms/x11/standalone/CMakeLists.txt
17 ↗(On Diff #20942)

no, you don't need it. It is found in root toplevel.

kwin-5.8.7/plugins/platforms/x11/standalone/x11_platform.cpp
225 ↗(On Diff #20942)

did you verify that this still restarts KWin?

226 ↗(On Diff #20942)

Yes it needs to be qfatal. That's used to abort the app, which is what we want here and trigger the required restart.

jpalecek added a comment.Oct 18 2017, 5:43 PM
In D8356#156920, @graesslin wrote:

Thanks for putting this on phab! Looks good to me, I like the idea with disabling KCrash.

You're welcome. But what happens next? Should I delete the find_package(...), add the const, check it builds and upload through "Update Diff"?

kwin-5.8.7/plugins/platforms/x11/standalone/x11_platform.cpp
225 ↗(On Diff #20942)

Yes, and disables compositing as well. I've had that opportunity several times.

KWin uses the emergency save handler for its restart, that works still with drKonqi enabled. Actually, drKonqiEnabled only causes that the default handler doesn't spawn drKonqi, however it still runs, and executes the emergency save function and terminates the application.

graesslin requested changes to this revision.Oct 18 2017, 6:56 PM
In D8356#156961, @jpalecek wrote:
In D8356#156920, @graesslin wrote:

Thanks for putting this on phab! Looks good to me, I like the idea with disabling KCrash.

You're welcome. But what happens next? Should I delete the find_package(...), add the const, check it builds and upload through "Update Diff"?

Yes please. Let's make it super correct: I marked "Request Changes"

This revision now requires changes to proceed.Oct 18 2017, 6:56 PM
jpalecek updated this revision to Diff 20968.Oct 18 2017, 11:47 PM

By reviewers' demand:

  • removed superfluous find_package for KCrash
  • made the configName variable const
graesslin accepted this revision.Oct 19 2017, 3:44 PM

Do you have commit rights? If not I'm going to push to 5.11 branch.

This revision is now accepted and ready to land.Oct 19 2017, 3:44 PM
ngraham edited the summary of this revision. (Show Details)Oct 19 2017, 3:47 PM
cfeck added a comment.Oct 19 2017, 3:51 PM

Nate, the BUG: entry needs to be on its own line.

ngraham edited the summary of this revision. (Show Details)Oct 19 2017, 3:54 PM
graesslin added a comment.Oct 21 2017, 7:35 PM

Could you please upload the patch in git-format against a git branch? I just wanted to pull it in,but it failed with:

error: kwin-5.10.5.orig/plugins/platforms/x11/standalone/x11_platform.cpp: does not exist in index

 Patch Failed! 
Usage Exception: Unable to apply patch!
jpalecek edited the summary of this revision. (Show Details)Oct 22 2017, 3:40 PM
jpalecek added a comment.Oct 22 2017, 3:46 PM
In D8356#157952, @graesslin wrote:

Could you please upload the patch in git-format against a git branch? I just wanted to pull it in,but it failed with:

error: kwin-5.10.5.orig/plugins/platforms/x11/standalone/x11_platform.cpp: does not exist in index

 Patch Failed! 
Usage Exception: Unable to apply patch!

Hmm. Although the patch I was submitting was a -p1 patch (produced by quilt in Debian packaging), it seems that the upload added a/ and b/ to the paths which made the diff -p2. Anyway, git apply -p2 worked as a charm atop 5.10. The patches are applied here: https://github.com/jpalecek/kwin/tree/Plasma/5.11, https://github.com/jpalecek/kwin/tree/Plasma/5.10.

jpalecek updated this revision to Diff 21329.Oct 25 2017, 6:21 PM

Recreated the diff usign arc (on branch Plasma/5.10)

Closed by commit R108:a7117e430454: Don't use the global KConfig object on the OpenGL freeze detection thread (authored by graesslin). · Explain WhyNov 5 2017, 9:45 AM
This revision was automatically updated to reflect the committed changes.

Revision Contents
Changeset List

Diff 21897

View Options

plugins/platforms/x11/standalone/CMakeLists.txt

Loading...
View Options

plugins/platforms/x11/standalone/x11_platform.cpp

Loading...