| Message ID | 20220719195325.402745-7-gpiccoli@igalia.com (mailing list archive) |
|---|---|
| State | Not Applicable |
| Headers | show |
| Series | The panic notifiers refactor strikes back - fixes/clean-ups | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Not a local patch, async |
On 19/07/2022 16:53, 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> > > --- > > 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(-) > [...] Hi Johannes, do you feel this one is good now, after your last review? Thanks in advance, Guilherme
On Sun, 2022-08-07 at 12:40 -0300, Guilherme G. Piccoli wrote: > On 19/07/2022 16:53, 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> > > > > --- > > > > 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(-) > > [...] > > Hi Johannes, do you feel this one is good now, after your last review? > Thanks in advance, > Yeah, no objections, my previous comment was just a minor almost style issue anyway. johannes
On 09/08/2022 15:09, Johannes Berg wrote: > [...] >>> 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(-) >>> [...] >> >> Hi Johannes, do you feel this one is good now, after your last review? >> Thanks in advance, >> > > Yeah, no objections, my previous comment was just a minor almost style > issue anyway. > > johannes Perfect, thank you! Let me take the opportunity to ask you something I'm asking all the maintainers involved here - do you prefer taking the patch through your tree, or to get it landed with the whole series, at once, from some maintainer? Cheers!
On Tue, 2022-08-09 at 16:03 -0300, Guilherme G. Piccoli wrote: > On 09/08/2022 15:09, Johannes Berg wrote: > > [...] > > > > 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(-) > > > > [...] > > > > > > Hi Johannes, do you feel this one is good now, after your last review? > > > Thanks in advance, > > > > > > > Yeah, no objections, my previous comment was just a minor almost style > > issue anyway. > > > > johannes > > Perfect, thank you! Let me take the opportunity to ask you something I'm > asking all the maintainers involved here - do you prefer taking the > patch through your tree, or to get it landed with the whole series, at > once, from some maintainer? > Hm. I don't think we'd really care, but so far I was thinking - since it's a series - it'd go through some appropriate tree all together. If you think it should be applied separately, let us know. johannes
On 09/08/2022 16:08, Johannes Berg wrote: > [...] >> Perfect, thank you! Let me take the opportunity to ask you something I'm >> asking all the maintainers involved here - do you prefer taking the >> patch through your tree, or to get it landed with the whole series, at >> once, from some maintainer? >> > Hm. I don't think we'd really care, but so far I was thinking - since > it's a series - it'd go through some appropriate tree all together. If > you think it should be applied separately, let us know. > > johannes For me, it would be easier if maintainers pick the patches into their -next/-fixes trees when they think the patch is good enough, but some maintainers complained that prefer the whole series approach (and some others are already taking the patches into their trees). Given that, in case you do have a linux-um tree and feel OK with that, I appreciate if you merge it, so I can remove the patch in next iteration. If you prefer the whole series approach, OK as well, your call =) Thanks, Guilherme
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 9838967d0b2f..970fdccc2f94 100644 --- a/arch/um/kernel/um_arch.c +++ b/arch/um/kernel/um_arch.c @@ -246,13 +246,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)
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> --- 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(-)