diff mbox series

[V3,04/11] um: Improve panic notifiers consistency and ordering

Message ID 20220819221731.480795-5-gpiccoli@igalia.com
State Accepted
Headers show
Series The panic notifiers refactor - fixes/clean-ups (V3) | expand

Commit Message

Guilherme G. Piccoli Aug. 19, 2022, 10:17 p.m. UTC
Currently the panic notifiers from user mode linux don't follow
the convention for most of the other notifiers present in the
kernel (indentation, priority setting, numeric return).
More important, the priorities could be improved, since it's a
special case (userspace), hence we could run the notifiers earlier;
user mode linux shouldn't care much with other panic notifiers but
the ordering among the mconsole and arch notifier is important,
given that the arch one effectively triggers a core dump.

Fix that by running the mconsole notifier as the first panic
notifier, followed by the architecture one (that coredumps).

Cc: Anton Ivanov <anton.ivanov@cambridgegreys.com>
Cc: Johannes Berg <johannes@sipsolutions.net>
Cc: Richard Weinberger <richard@nod.at>
Signed-off-by: Guilherme G. Piccoli <gpiccoli@igalia.com>

V3:
- No changes.

V2:
- Kept the notifier header to avoid implicit usage - thanks
Johannes for the suggestion!
---
 arch/um/drivers/mconsole_kern.c | 7 +++----
 arch/um/kernel/um_arch.c        | 8 ++++----
 2 files changed, 7 insertions(+), 8 deletions(-)

Comments

Guilherme G. Piccoli Sept. 18, 2022, 2 p.m. UTC | #1
On 19/08/2022 19:17, Guilherme G. Piccoli wrote:
> Currently the panic notifiers from user mode linux don't follow
> the convention for most of the other notifiers present in the
> kernel (indentation, priority setting, numeric return).
> More important, the priorities could be improved, since it's a
> special case (userspace), hence we could run the notifiers earlier;
> user mode linux shouldn't care much with other panic notifiers but
> the ordering among the mconsole and arch notifier is important,
> given that the arch one effectively triggers a core dump.
> 
> Fix that by running the mconsole notifier as the first panic
> notifier, followed by the architecture one (that coredumps).
> 
> Cc: Anton Ivanov <anton.ivanov@cambridgegreys.com>
> Cc: Johannes Berg <johannes@sipsolutions.net>
> Cc: Richard Weinberger <richard@nod.at>
> Signed-off-by: Guilherme G. Piccoli <gpiccoli@igalia.com>
> 
> V3:
> - No changes.
> [...]

Hi Johannes, sorry for the ping. Do you think you could pick this one?
Or if you prefer, I can resend it alone (not in the series) - let me
know your preference.

Thanks,


Guilherme
Richard Weinberger Sept. 18, 2022, 9:19 p.m. UTC | #2
----- Ursprüngliche Mail -----
> Von: "Guilherme G. Piccoli" <gpiccoli@igalia.com>
> On 19/08/2022 19:17, Guilherme G. Piccoli wrote:
>> Currently the panic notifiers from user mode linux don't follow
>> the convention for most of the other notifiers present in the
>> kernel (indentation, priority setting, numeric return).
>> More important, the priorities could be improved, since it's a
>> special case (userspace), hence we could run the notifiers earlier;
>> user mode linux shouldn't care much with other panic notifiers but
>> the ordering among the mconsole and arch notifier is important,
>> given that the arch one effectively triggers a core dump.
>> 
>> Fix that by running the mconsole notifier as the first panic
>> notifier, followed by the architecture one (that coredumps).
>> 
>> Cc: Anton Ivanov <anton.ivanov@cambridgegreys.com>
>> Cc: Johannes Berg <johannes@sipsolutions.net>
>> Cc: Richard Weinberger <richard@nod.at>
>> Signed-off-by: Guilherme G. Piccoli <gpiccoli@igalia.com>
>> 
>> V3:
>> - No changes.
>> [...]
> 
> Hi Johannes, sorry for the ping. Do you think you could pick this one?
> Or if you prefer, I can resend it alone (not in the series) - let me
> know your preference.

This patch is on my TODO list.
Just had no chance to test it.

Thanks,
//richard
Guilherme G. Piccoli Sept. 19, 2022, 11:44 a.m. UTC | #3
On 18/09/2022 18:19, Richard Weinberger wrote:
> ----- Ursprüngliche Mail -----
>> Von: "Guilherme G. Piccoli" <gpiccoli@igalia.com>
>> On 19/08/2022 19:17, Guilherme G. Piccoli wrote:
>>> Currently the panic notifiers from user mode linux don't follow
>>> the convention for most of the other notifiers present in the
>>> kernel (indentation, priority setting, numeric return).
>>> More important, the priorities could be improved, since it's a
>>> special case (userspace), hence we could run the notifiers earlier;
>>> user mode linux shouldn't care much with other panic notifiers but
>>> the ordering among the mconsole and arch notifier is important,
>>> given that the arch one effectively triggers a core dump.
>>>
>>> Fix that by running the mconsole notifier as the first panic
>>> notifier, followed by the architecture one (that coredumps).
>>>
>>> Cc: Anton Ivanov <anton.ivanov@cambridgegreys.com>
>>> Cc: Johannes Berg <johannes@sipsolutions.net>
>>> Cc: Richard Weinberger <richard@nod.at>
>>> Signed-off-by: Guilherme G. Piccoli <gpiccoli@igalia.com>
>>>
>>> V3:
>>> - No changes.
>>> [...]
>>
>> Hi Johannes, sorry for the ping. Do you think you could pick this one?
>> Or if you prefer, I can resend it alone (not in the series) - let me
>> know your preference.
> 
> This patch is on my TODO list.
> Just had no chance to test it.
> 
> Thanks,
> //richard

Thanks a lot Richard!
Guilherme G. Piccoli Oct. 17, 2022, 2:01 p.m. UTC | #4
On 18/09/2022 18:19, Richard Weinberger wrote:
> ----- Ursprüngliche Mail -----
>> Von: "Guilherme G. Piccoli" <gpiccoli@igalia.com>
>> On 19/08/2022 19:17, Guilherme G. Piccoli wrote:
>>> Currently the panic notifiers from user mode linux don't follow
>>> the convention for most of the other notifiers present in the
>>> kernel (indentation, priority setting, numeric return).
>>> More important, the priorities could be improved, since it's a
>>> special case (userspace), hence we could run the notifiers earlier;
>>> user mode linux shouldn't care much with other panic notifiers but
>>> the ordering among the mconsole and arch notifier is important,
>>> given that the arch one effectively triggers a core dump.
>>>
>>> Fix that by running the mconsole notifier as the first panic
>>> notifier, followed by the architecture one (that coredumps).
>>>
>>> Cc: Anton Ivanov <anton.ivanov@cambridgegreys.com>
>>> Cc: Johannes Berg <johannes@sipsolutions.net>
>>> Cc: Richard Weinberger <richard@nod.at>
>>> Signed-off-by: Guilherme G. Piccoli <gpiccoli@igalia.com>
>>>
>>> V3:
>>> - No changes.
>>> [...]
>>
>> Hi Johannes, sorry for the ping. Do you think you could pick this one?
>> Or if you prefer, I can resend it alone (not in the series) - let me
>> know your preference.
> 
> This patch is on my TODO list.
> Just had no chance to test it.
> 
> Thanks,
> //richard

Hi Richard / Johannes, is there any news on this one?
Thanks in advance,


Guilherme
Richard Weinberger Oct. 17, 2022, 2:10 p.m. UTC | #5
----- Ursprüngliche Mail -----
> Von: "Guilherme G. Piccoli" <gpiccoli@igalia.com>
> Hi Richard / Johannes, is there any news on this one?
> Thanks in advance,

It's upstream:
git.kernel.org/linus/758dfdb9185cf94160f20e85bbe05583e3cd4ff4

Thanks,
//richard
Guilherme G. Piccoli Oct. 17, 2022, 2:22 p.m. UTC | #6
On 17/10/2022 11:10, Richard Weinberger wrote:
> ----- Ursprüngliche Mail -----
>> Von: "Guilherme G. Piccoli" <gpiccoli@igalia.com>
>> Hi Richard / Johannes, is there any news on this one?
>> Thanks in advance,
> 
> It's upstream:
> git.kernel.org/linus/758dfdb9185cf94160f20e85bbe05583e3cd4ff4
> 
> Thanks,
> //richard

Wow, thanks! I am sorry, I didn't notice.
Cheers,


Guilherme
diff mbox series

Patch

diff --git a/arch/um/drivers/mconsole_kern.c b/arch/um/drivers/mconsole_kern.c
index 8ca67a692683..69af3ce8407a 100644
--- a/arch/um/drivers/mconsole_kern.c
+++ b/arch/um/drivers/mconsole_kern.c
@@ -846,13 +846,12 @@  static int notify_panic(struct notifier_block *self, unsigned long unused1,
 
 	mconsole_notify(notify_socket, MCONSOLE_PANIC, message,
 			strlen(message) + 1);
-	return 0;
+	return NOTIFY_DONE;
 }
 
 static struct notifier_block panic_exit_notifier = {
-	.notifier_call 		= notify_panic,
-	.next 			= NULL,
-	.priority 		= 1
+	.notifier_call	= notify_panic,
+	.priority	= INT_MAX, /* run as soon as possible */
 };
 
 static int add_notifier(void)
diff --git a/arch/um/kernel/um_arch.c b/arch/um/kernel/um_arch.c
index e0de60e503b9..ae272878f692 100644
--- a/arch/um/kernel/um_arch.c
+++ b/arch/um/kernel/um_arch.c
@@ -247,13 +247,13 @@  static int panic_exit(struct notifier_block *self, unsigned long unused1,
 	bust_spinlocks(0);
 	uml_exitcode = 1;
 	os_dump_core();
-	return 0;
+
+	return NOTIFY_DONE;
 }
 
 static struct notifier_block panic_exit_notifier = {
-	.notifier_call 		= panic_exit,
-	.next 			= NULL,
-	.priority 		= 0
+	.notifier_call	= panic_exit,
+	.priority	= INT_MAX - 1, /* run as 2nd notifier, won't return */
 };
 
 void uml_finishsetup(void)