Light Display Manager

Merge lp:~townsend/lightdm/fix-plymouthd-spin into lp:~mir-team/lightdm/unity

fix-plymouthd-spin
Merge into unity

Proposed by Christopher Townsend on 2013-07-15

Status:

Merged

Approved by:

Robert Ancell on 2013-07-19

Approved revision:

1635

Merged at revision:

1636

Proposed branch:

lp:~townsend/lightdm/fix-plymouthd-spin

Merge into:

lp:~mir-team/lightdm/unity

Diff against target:

90 lines (+12/-25)

4 files modified

src/plymouth.c (+2/-22)
src/plymouth.h (+1/-1)
src/seat-unity.c (+8/-1)
src/xserver-local.c (+1/-1)

To merge this branch:

bzr merge lp:~townsend/lightdm/fix-plymouthd-spin

Related bugs:

Bug #1192051: plymouthd spinning at 100% CPU after I log in

High

Fix Released

Link a bug report

Reviewer	Review Type	Date Requested	Status
Robert Ancell			Approve on 2013-07-19
Daniel van Vugt		2013-07-15	Needs Information on 2013-07-16
PS Jenkins bot (community)	continuous-integration		Approve on 2013-07-15
Review via email: mp+174874@code.launchpad.net

Commit message

* Revert the workaround introduced in revno. 1619.
* Add in handling of "plymouth quit" when using the compositor to send the plymouth commands to deactivate and quit. (LP: #1192051)

Description of the change

In Xmir, the compositor start up is supposed to handle the interaction with plymouth unlike X only in which xserver-local does this. However, it is lacking a call to plymouth_quit(), so plymouthd will spin forever since nothing told it to quit.

This fix uses the stopping_plymouth boolean to set when to run plymouth_quit(). I think this is a bug in upstream lightdm as well since nothing ever sets that boolean to TRUE, rendering this a useless variable.

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2013-07-15:

PASSED: Continuous integration, rev:1635
http://jenkins.qa.ubuntu.com/job/mir-team-lightdm-unity-ci/34/
Executed test runs:
SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-team-lightdm-unity-saucy-amd64-ci/15

Click here to trigger a rebuild:
http://s-jenkins:8080/job/mir-team-lightdm-unity-ci/34/rebuild

review: Approve (continuous-integration)

Revision history for this message

Daniel van Vugt (vanvugt) wrote on 2013-07-16:

I thought the absence of plymouth_quit wass correct. Lightdm is not meant to quit plymouth, only deactivate it...

I thought you should not want plymouth quit because we needed a faster and smoother transition on startup, which deactivate gives you(?). But I also can't see any reason why plymouthd should keep running, so quit might make sense.

review: Needs Information

Revision history for this message

Daniel van Vugt (vanvugt) wrote on 2013-07-16:

See also: /etc/init/plymouth-stop.conf
which is intended to quit plymouth. But apparently not with lightdm...

Revision history for this message

Christopher Townsend (townsend) wrote on 2013-07-16:

@Daniel,

Thanks for looking over this. I also thought deactivate was a more graceful way of shutting down plymouth, but I've looked through the plymouth code and basically all deactivate does is stop displaying the splash screen. There is no code in the deactivate path to kill the plymouthd process.

Also, when using the X only setup, "plymouth quit" is indeed called by xserver-local in lightdm and this is what kills off the plymouthd process. So what I did is took how xserver-local handles that and apply it to when the compositor is handling the plymouth duties. I think this is a bug in upstream lightdm as well, but it was undiscovered since xserver-local has been handling these duties until now.

The reason your workaround appeared to work is that is actually caused plymouthd to segfault. After the first deactivate is done, some pointers are NULLed out and the subsequent call to deactivate dereferences the null pointer. I think that is a bug as well, but not this bug we are trying to fix:)

I hope this helps explain my findings and rationale for this MP. I also put a bit more detail in the bug itself about this.

Revision history for this message

Robert Ancell (robert-ancell) wrote on 2013-07-19:

Thanks Christopher! Looking into it you're completely right - the Unity seat was not behaving like the X seat. I've pushed this to trunk with a regression test that confirms and checks the behaviour (lp:~robert-ancell/lightdm/unity-plymouth-fix).

review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk

Subscribers

People subscribed via source and target branches

to all changes:

Christopher Townsend

Mir development team

Robert Ancell

1	=== modified file 'src/plymouth.c'
2	--- src/plymouth.c 2013-07-01 03:40:17 +0000
3	+++ src/plymouth.c 2013-07-15 21:50:44 +0000
4	@@ -81,30 +81,10 @@
5	81	}	81	}
6	82		82
7	83	void	83	void
9	84	plymouth_deactivate (gboolean wait)	84	plymouth_deactivate (void)
10	85	{	85	{
11	86	gint attempt = 1;
12	87	is_active = FALSE;	86	is_active = FALSE;
33	88		87	plymouth_run_command ("deactivate", NULL);
14	89	/*
15	90	* The "plymouth" command has no option like --wait-for-deactivation so
16	91	* we have to wait in a loop.
17	92	*/
18	93	do
19	94	{
20	95	g_debug("Deactivating Plymouth (attempt #%d)", attempt);
21	96	attempt++;
22	97	plymouth_run_command ("deactivate", NULL);
23	98
24	99	if (!wait)
25	100	return;
26	101
27	102	sleep (1);
28	103	have_checked_active_vt = FALSE;
29	104	} while (plymouth_has_active_vt () && attempt <= 60);
30	105
31	106	if (has_active_vt)
32	107	g_debug("Failed to deactivate Plymouth.");
34	108	}	88	}
35	109		89
36	110	void	90	void
37	111		91
38	=== modified file 'src/plymouth.h'
39	--- src/plymouth.h 2013-07-01 03:40:17 +0000
40	+++ src/plymouth.h 2013-07-15 21:50:44 +0000
41	@@ -22,7 +22,7 @@
42	22		22
43	23	gboolean plymouth_has_active_vt (void);	23	gboolean plymouth_has_active_vt (void);
44	24		24
46	25	void plymouth_deactivate (gboolean wait);	25	void plymouth_deactivate (void);
47	26		26
48	27	void plymouth_quit (gboolean retain_splash);	27	void plymouth_quit (gboolean retain_splash);
49	28		28
50	29		29
51	=== modified file 'src/seat-unity.c'
52	--- src/seat-unity.c 2013-07-01 05:10:33 +0000
53	+++ src/seat-unity.c 2013-07-15 21:50:44 +0000
54	@@ -146,6 +146,12 @@
55	146	close (fd);	146	close (fd);
56	147	}	147	}
57	148	}	148	}
58			149
59			150	if(seat->priv->stopping_plymouth)
60			151	{
61			152	seat->priv->stopping_plymouth = FALSE;
62			153	plymouth_quit(TRUE);
63			154	}
64	149	}	155	}
65	150		156
66	151	static void	157	static void
67	@@ -309,8 +315,9 @@
68	309	if (active_vt >= vt_get_min ())	315	if (active_vt >= vt_get_min ())
69	310	{	316	{
70	311	g_debug ("Compositor will replace Plymouth");	317	g_debug ("Compositor will replace Plymouth");
71			318	SEAT_UNITY (seat)->priv->stopping_plymouth = TRUE;
72	312	SEAT_UNITY (seat)->priv->vt = active_vt;	319	SEAT_UNITY (seat)->priv->vt = active_vt;
74	313	plymouth_deactivate (TRUE);	320	plymouth_deactivate ();
75	314	}	321	}
76	315	else	322	else
77	316	g_debug ("Plymouth is running on VT %d, but this is less than the configured minimum of %d so not replacing it", active_vt, vt_get_min ());	323	g_debug ("Plymouth is running on VT %d, but this is less than the configured minimum of %d so not replacing it", active_vt, vt_get_min ());
78	317		324
79	=== modified file 'src/xserver-local.c'
80	--- src/xserver-local.c 2013-07-01 03:40:17 +0000
81	+++ src/xserver-local.c 2013-07-15 21:50:44 +0000
82	@@ -170,7 +170,7 @@
83	170	g_debug ("X server %s will replace Plymouth", xserver_get_address (XSERVER (self)));	170	g_debug ("X server %s will replace Plymouth", xserver_get_address (XSERVER (self)));
84	171	self->priv->replacing_plymouth = TRUE;	171	self->priv->replacing_plymouth = TRUE;
85	172	self->priv->vt = active_vt;	172	self->priv->vt = active_vt;
87	173	plymouth_deactivate (FALSE);	173	plymouth_deactivate ();
88	174	}	174	}
89	175	else	175	else
90	176	g_debug ("Plymouth is running on VT %d, but this is less than the configured minimum of %d so not replacing it", active_vt, vt_get_min ());	176	g_debug ("Plymouth is running on VT %d, but this is less than the configured minimum of %d so not replacing it", active_vt, vt_get_min ());