Menu
▾
▴
dle-develop
|
From: Seiji A. <sei...@hd...> - 2011-10-14 20:53:36
|
Hi, As Don mentioned in following thread, it would be nice for pstore/kmsg_dump to serialize panic path and have one cpu running because they can log messages reliably. https://lkml.org/lkml/2011/10/13/427 For realizing this idea, we have to move kmsg_dump below smp_send_stop() and bust some locks of kmsg_dump/pstore in panic path. This patch does followings. - moving kmsg_dump(KMSG_DUMP_PANIC) below smp_send_stop. - busting logbuf_lock of kmsg_dump() in panic path for avoiding deadlock. - busting psinfo->buf_lock of pstore_dump() in panic path for avoiding deadlock. Any comments are welcome. Signed-off-by: Seiji Aguchi <sei...@hd...> --- fs/pstore/platform.c | 22 ++++++++++------------ kernel/panic.c | 4 ++-- kernel/printk.c | 7 +++++++ 3 files changed, 19 insertions(+), 14 deletions(-) diff --git a/fs/pstore/platform.c b/fs/pstore/platform.c index 2bd620f..e73d940 100644 --- a/fs/pstore/platform.c +++ b/fs/pstore/platform.c @@ -90,19 +90,21 @@ static void pstore_dump(struct kmsg_dumper *dumper, int hsize, ret; unsigned int part = 1; unsigned long flags = 0; - int is_locked = 0; if (reason < ARRAY_SIZE(reason_str)) why = reason_str[reason]; else why = "Unknown"; - if (in_nmi()) { - is_locked = spin_trylock(&psinfo->buf_lock); - if (!is_locked) - pr_err("pstore dump routine blocked in NMI, may corrupt error record\n"); - } else - spin_lock_irqsave(&psinfo->buf_lock, flags); + /* + * pstore_dump() is called after smp_send_stop() in panic path. + * So, spin_lock should be bust for avoiding deadlock. + */ + if (reason == KMSG_DUMP_PANIC) + spin_lock_init(&psinfo->buf_lock); + + spin_lock_irqsave(&psinfo->buf_lock, flags); + oopscount++; while (total < kmsg_bytes) { dst = psinfo->buf; @@ -131,11 +133,7 @@ static void pstore_dump(struct kmsg_dumper *dumper, total += l1_cpy + l2_cpy; part++; } - if (in_nmi()) { - if (is_locked) - spin_unlock(&psinfo->buf_lock); - } else - spin_unlock_irqrestore(&psinfo->buf_lock, flags); + spin_unlock_irqrestore(&psinfo->buf_lock, flags); } static struct kmsg_dumper pstore_dumper = { diff --git a/kernel/panic.c b/kernel/panic.c index d7bb697..41bf6ad 100644 --- a/kernel/panic.c +++ b/kernel/panic.c @@ -88,8 +88,6 @@ NORET_TYPE void panic(const char * fmt, ...) */ crash_kexec(NULL); - kmsg_dump(KMSG_DUMP_PANIC); - /* * Note smp_send_stop is the usual smp shutdown function, which * unfortunately means it may not be hardened to work in a panic @@ -97,6 +95,8 @@ NORET_TYPE void panic(const char * fmt, ...) */ smp_send_stop(); + kmsg_dump(KMSG_DUMP_PANIC); + atomic_notifier_call_chain(&panic_notifier_list, 0, buf); bust_spinlocks(0); diff --git a/kernel/printk.c b/kernel/printk.c index 1455a0d..e1e57db 100644 --- a/kernel/printk.c +++ b/kernel/printk.c @@ -1732,6 +1732,13 @@ void kmsg_dump(enum kmsg_dump_reason reason) unsigned long l1, l2; unsigned long flags; + /* + * kmsg_dump() is called after smp_send_stop() in panic path. + * So, spin_lock should be bust for avoiding deadlock. + */ + if (reason == KMSG_DUMP_PANIC) + raw_spin_lock_init(&logbuf_lock); + /* Theoretically, the log could move on after we do this, but there's not a lot we can do about that. The new messages will overwrite the start of what we dump. */ -- 1.7.1 |
|
From: Chen G. <gon...@li...> - 2011-10-17 06:21:50
|
于 2011/10/15 4:53, Seiji Aguchi 写道: > Hi, > > As Don mentioned in following thread, it would be nice for pstore/kmsg_dump to serialize > panic path and have one cpu running because they can log messages reliably. > > https://lkml.org/lkml/2011/10/13/427 > > For realizing this idea, we have to move kmsg_dump below smp_send_stop() and bust some locks > of kmsg_dump/pstore in panic path. > > This patch does followings. > > - moving kmsg_dump(KMSG_DUMP_PANIC) below smp_send_stop. > - busting logbuf_lock of kmsg_dump() in panic path for avoiding deadlock. > - busting psinfo->buf_lock of pstore_dump() in panic path for avoiding deadlock. > > Any comments are welcome. > Hi, Seiji I have a stupid question: since you have serialized the process procedure via smp_send_stop, why still using spin_lock_xxx? Maybe preempt_disable/enable is enough? |
|
From: Seiji A. <sei...@hd...> - 2011-10-17 14:11:11
|
Hi,
Thank you for giving me a comment.
>I have a stupid question: since you have serialized the process procedure via
>smp_send_stop, why still using spin_lock_xxx? Maybe preempt_disable/enable is
>enough?
I added spin_lock_init() in panic path for sharing code with other triggers
such as oops/reboot/emergency_restart because they still need spin_locks.
Do you suggest following code?
<snip>
If(!panic)
spin_lock_irqsave();
.
.
If(!panic)
spin_unlock_restore();
<snip>
I don't stick to current patch.
So I will resend a patch above if you request.
Regarding as preempt_disable/enable, we don't need to call them in panic path because they are
called at the beginning of panic().
<snip>
60 NORET_TYPE void panic(const char * fmt, ...)
61 {
62 static char buf[1024];
63 va_list args;
64 long i, i_next = 0;
65 int state = 0;
66
67 /*
68 * It's possible to come here directly from a panic-assertion and
69 * not have preempt disabled. Some functions called from here want
70 * preempt to be disabled. No point enabling it later though...
71 */
72 preempt_disable();
<snip>
Seiji
|
|
From: Chen G. <gon...@li...> - 2011-10-18 07:28:33
|
于 2011/10/17 22:10, Seiji Aguchi 写道: > Hi, > > Thank you for giving me a comment. > >> I have a stupid question: since you have serialized the process procedure via >> smp_send_stop, why still using spin_lock_xxx? Maybe preempt_disable/enable is >> enough? > > > I added spin_lock_init() in panic path for sharing code with other triggers > such as oops/reboot/emergency_restart because they still need spin_locks. > > Do you suggest following code? > > <snip> > If(!panic) > spin_lock_irqsave(); > > . > . > If(!panic) > spin_unlock_restore(); > <snip> > > I don't stick to current patch. > So I will resend a patch above if you request. Yes, it looks more readable. + if (reason == KMSG_DUMP_PANIC) + raw_spin_lock_init(&logbuf_lock); above codes look a little bit confused |
|
From: Don Z. <dz...@re...> - 2011-10-17 16:09:46
|
On Fri, Oct 14, 2011 at 04:53:05PM -0400, Seiji Aguchi wrote: > Hi, > > As Don mentioned in following thread, it would be nice for pstore/kmsg_dump to serialize > panic path and have one cpu running because they can log messages reliably. > > https://lkml.org/lkml/2011/10/13/427 > > For realizing this idea, we have to move kmsg_dump below smp_send_stop() and bust some locks > of kmsg_dump/pstore in panic path. > > This patch does followings. > > - moving kmsg_dump(KMSG_DUMP_PANIC) below smp_send_stop. > - busting logbuf_lock of kmsg_dump() in panic path for avoiding deadlock. > - busting psinfo->buf_lock of pstore_dump() in panic path for avoiding deadlock. > > Any comments are welcome. I think I am ok with this, just some comments below. > > Signed-off-by: Seiji Aguchi <sei...@hd...> > > --- > fs/pstore/platform.c | 22 ++++++++++------------ > kernel/panic.c | 4 ++-- > kernel/printk.c | 7 +++++++ > 3 files changed, 19 insertions(+), 14 deletions(-) > > diff --git a/fs/pstore/platform.c b/fs/pstore/platform.c > index 2bd620f..e73d940 100644 > --- a/fs/pstore/platform.c > +++ b/fs/pstore/platform.c > @@ -90,19 +90,21 @@ static void pstore_dump(struct kmsg_dumper *dumper, > int hsize, ret; > unsigned int part = 1; > unsigned long flags = 0; > - int is_locked = 0; > > if (reason < ARRAY_SIZE(reason_str)) > why = reason_str[reason]; > else > why = "Unknown"; > > - if (in_nmi()) { > - is_locked = spin_trylock(&psinfo->buf_lock); > - if (!is_locked) > - pr_err("pstore dump routine blocked in NMI, may corrupt error record\n"); > - } else > - spin_lock_irqsave(&psinfo->buf_lock, flags); > + /* > + * pstore_dump() is called after smp_send_stop() in panic path. > + * So, spin_lock should be bust for avoiding deadlock. > + */ > + if (reason == KMSG_DUMP_PANIC) > + spin_lock_init(&psinfo->buf_lock); > + > + spin_lock_irqsave(&psinfo->buf_lock, flags); > + > oopscount++; > while (total < kmsg_bytes) { > dst = psinfo->buf; > @@ -131,11 +133,7 @@ static void pstore_dump(struct kmsg_dumper *dumper, > total += l1_cpy + l2_cpy; > part++; > } > - if (in_nmi()) { > - if (is_locked) > - spin_unlock(&psinfo->buf_lock); > - } else > - spin_unlock_irqrestore(&psinfo->buf_lock, flags); > + spin_unlock_irqrestore(&psinfo->buf_lock, flags); > } My original problem was the NMI path was calling panic and thus causing these locking problems. With the change below, which allows the change above, I believe the NMI case is covered (because this path is only called by NMI during panic). This makes my previous patch (accepted by Tony) probably unnecessary if we go with this patch. As for the need for using spin_lock_init vs just skipping the locking, I don't have a preference, nor know what the correct usage is for these things. I suppose this patch does the proper thing. > > static struct kmsg_dumper pstore_dumper = { > diff --git a/kernel/panic.c b/kernel/panic.c > index d7bb697..41bf6ad 100644 > --- a/kernel/panic.c > +++ b/kernel/panic.c > @@ -88,8 +88,6 @@ NORET_TYPE void panic(const char * fmt, ...) > */ > crash_kexec(NULL); > > - kmsg_dump(KMSG_DUMP_PANIC); > - > /* > * Note smp_send_stop is the usual smp shutdown function, which > * unfortunately means it may not be hardened to work in a panic > @@ -97,6 +95,8 @@ NORET_TYPE void panic(const char * fmt, ...) > */ > smp_send_stop(); > > + kmsg_dump(KMSG_DUMP_PANIC); > + > atomic_notifier_call_chain(&panic_notifier_list, 0, buf); > > bust_spinlocks(0); Yes, this is what Seiji and I talked about previously based on Vivek's suggestion. This can only work on x86 if we pick up my 'using NMI in stop cpu path' patch from last week. https://lkml.org/lkml/2011/10/13/426 Otherwise, there is no guarantee all the cpus stop and we can still end up in data corruption depending on the backend being used. > diff --git a/kernel/printk.c b/kernel/printk.c > index 1455a0d..e1e57db 100644 > --- a/kernel/printk.c > +++ b/kernel/printk.c > @@ -1732,6 +1732,13 @@ void kmsg_dump(enum kmsg_dump_reason reason) > unsigned long l1, l2; > unsigned long flags; > > + /* > + * kmsg_dump() is called after smp_send_stop() in panic path. > + * So, spin_lock should be bust for avoiding deadlock. > + */ > + if (reason == KMSG_DUMP_PANIC) > + raw_spin_lock_init(&logbuf_lock); > + > /* Theoretically, the log could move on after we do this, but > there's not a lot we can do about that. The new messages > will overwrite the start of what we dump. */ > -- The just adds more lock busting based on the movement of kmsg_dump() in the panic path. There is still more lock busting to do in the various backends, but this patch provides the necessary first steps. All this lock busting probably isn't pretty and causes one to reflect what is going on here. But as long as we are going to keep the kmsg_dump design, changes like this seem necessary to make sure the locking stays sane(r) for now. Acked-by: Don Zickus <dz...@re...> |
|
From: Luck, T. <ton...@in...> - 2011-10-17 16:57:15
|
> All this lock busting probably isn't pretty and causes one to reflect what > is going on here. But as long as we are going to keep the kmsg_dump > design, changes like this seem necessary to make sure the locking stays > sane(r) for now. So should we let the back-end know that locks have been busted? I worry that we'll get through the pstore layer by blasting away at any locks that get in our way - but then the backend (which may well have been written on the assumption that pstore serialized calls to it) will not do anything useful (and may cause its own hang). -Tony |
|
From: Don Z. <dz...@re...> - 2011-10-17 17:23:06
|
On Mon, Oct 17, 2011 at 09:56:50AM -0700, Luck, Tony wrote: > > All this lock busting probably isn't pretty and causes one to reflect what > > is going on here. But as long as we are going to keep the kmsg_dump > > design, changes like this seem necessary to make sure the locking stays > > sane(r) for now. > > So should we let the back-end know that locks have been busted? I worry > that we'll get through the pstore layer by blasting away at any locks that > get in our way - but then the backend (which may well have been written on > the assumption that pstore serialized calls to it) will not do anything > useful (and may cause its own hang). I was kinda alluding to something like that, when I said we probably need follow on patches to clean up the backend. But maybe it would be smarter to set a flag in pstore to let the backend know we busted the locks instead of letting them do panic checks themselves. I agree with your concerns. I really can't think of a good overall solution other than slowly step-by-step untangle things by making subtle changes, this round being the serialization of kmsg_dump(). My brain is to small to figure this out. I keep thinking about making an ascii art diagram to see all the various paths and their contexts, but then I keep finding more interesting things to do. :-) Perhaps we should do that and make some rules about what the back-end can and can not do when plugging into kmsg_dump. Cheers, Don |
|
From: Seiji A. <sei...@hd...> - 2011-10-17 17:52:03
|
Hi, I agree with Tony's concern. But as Don told, the best way to go proceed is fixing backend drivers step by step. Seiji >-----Original Message----- >From: Don Zickus [mailto:dz...@re...] >Sent: Monday, October 17, 2011 1:22 PM >To: Luck, Tony >Cc: Seiji Aguchi; lin...@vg...; Matthew Garrett; Vivek Goyal; Chen, Gong; Andrew Morton; Brown, Len; Huang, >Ying; ak...@li...; hu...@ch...; mi...@el...; jm...@na...; a.p...@ch...; >nam...@gm...; dle...@li...; Satoru Moriya >Subject: Re: [RFC][PATCH -next] make pstore/kmsg_dump run after stopping other cpus in panic path > >On Mon, Oct 17, 2011 at 09:56:50AM -0700, Luck, Tony wrote: >> > All this lock busting probably isn't pretty and causes one to reflect what >> > is going on here. But as long as we are going to keep the kmsg_dump >> > design, changes like this seem necessary to make sure the locking stays >> > sane(r) for now. >> >> So should we let the back-end know that locks have been busted? I worry >> that we'll get through the pstore layer by blasting away at any locks that >> get in our way - but then the backend (which may well have been written on >> the assumption that pstore serialized calls to it) will not do anything >> useful (and may cause its own hang). > >I was kinda alluding to something like that, when I said we probably need >follow on patches to clean up the backend. But maybe it would be smarter >to set a flag in pstore to let the backend know we busted the locks >instead of letting them do panic checks themselves. I agree with your >concerns. > >I really can't think of a good overall solution other than slowly >step-by-step untangle things by making subtle changes, this round being >the serialization of kmsg_dump(). > >My brain is to small to figure this out. I keep thinking about making an >ascii art diagram to see all the various paths and their contexts, but >then I keep finding more interesting things to do. :-) > >Perhaps we should do that and make some rules about what the back-end can >and can not do when plugging into kmsg_dump. > >Cheers, >Don |
|
From: Andrew M. <ak...@li...> - 2011-10-17 23:47:28
|
On Fri, 14 Oct 2011 16:53:05 -0400 Seiji Aguchi <sei...@hd...> wrote: > Hi, > > As Don mentioned in following thread, it would be nice for pstore/kmsg_dump to serialize > panic path and have one cpu running because they can log messages reliably. > > https://lkml.org/lkml/2011/10/13/427 > > For realizing this idea, we have to move kmsg_dump below smp_send_stop() and bust some locks > of kmsg_dump/pstore in panic path. > > This patch does followings. > > - moving kmsg_dump(KMSG_DUMP_PANIC) below smp_send_stop. > - busting logbuf_lock of kmsg_dump() in panic path for avoiding deadlock. > - busting psinfo->buf_lock of pstore_dump() in panic path for avoiding deadlock. > > Any comments are welcome. > > ... > > --- a/fs/pstore/platform.c > +++ b/fs/pstore/platform.c > @@ -90,19 +90,21 @@ static void pstore_dump(struct kmsg_dumper *dumper, > int hsize, ret; > unsigned int part = 1; > unsigned long flags = 0; > - int is_locked = 0; > > if (reason < ARRAY_SIZE(reason_str)) > why = reason_str[reason]; > else > why = "Unknown"; > > - if (in_nmi()) { > - is_locked = spin_trylock(&psinfo->buf_lock); > - if (!is_locked) > - pr_err("pstore dump routine blocked in NMI, may corrupt error record\n"); > - } else > - spin_lock_irqsave(&psinfo->buf_lock, flags); > + /* > + * pstore_dump() is called after smp_send_stop() in panic path. > + * So, spin_lock should be bust for avoiding deadlock. > + */ > + if (reason == KMSG_DUMP_PANIC) > + spin_lock_init(&psinfo->buf_lock); > + > + spin_lock_irqsave(&psinfo->buf_lock, flags); > + > oopscount++; > while (total < kmsg_bytes) { > dst = psinfo->buf; > @@ -131,11 +133,7 @@ static void pstore_dump(struct kmsg_dumper *dumper, > total += l1_cpy + l2_cpy; > part++; > } > - if (in_nmi()) { > - if (is_locked) > - spin_unlock(&psinfo->buf_lock); > - } else > - spin_unlock_irqrestore(&psinfo->buf_lock, flags); > + spin_unlock_irqrestore(&psinfo->buf_lock, flags); > } afacit this assumes that (reason == KMSG_DUMP_PANIC) if in_nmi(). Is that always the case, and will it always be the case in the future? I felt that the spin_trylock() approach was less horrid than this. I assume that the new approach will cause lockdep to go berzerk? > --- a/kernel/printk.c > +++ b/kernel/printk.c > @@ -1732,6 +1732,13 @@ void kmsg_dump(enum kmsg_dump_reason reason) > unsigned long l1, l2; > unsigned long flags; > > + /* > + * kmsg_dump() is called after smp_send_stop() in panic path. > + * So, spin_lock should be bust for avoiding deadlock. > + */ > + if (reason == KMSG_DUMP_PANIC) > + raw_spin_lock_init(&logbuf_lock); > + > /* Theoretically, the log could move on after we do this, but > there's not a lot we can do about that. The new messages > will overwrite the start of what we dump. */ I suggest you do some research into bust_spinlocks() and how it has changed over time. I think that code used to fiddle with log levels and once upon a time it might have fiddled with logbuf_lock. |
|
From: Don Z. <dz...@re...> - 2011-10-18 12:55:24
|
On Mon, Oct 17, 2011 at 04:47:15PM -0700, Andrew Morton wrote:
> On Fri, 14 Oct 2011 16:53:05 -0400
> > @@ -131,11 +133,7 @@ static void pstore_dump(struct kmsg_dumper *dumper,
> > total += l1_cpy + l2_cpy;
> > part++;
> > }
> > - if (in_nmi()) {
> > - if (is_locked)
> > - spin_unlock(&psinfo->buf_lock);
> > - } else
> > - spin_unlock_irqrestore(&psinfo->buf_lock, flags);
> > + spin_unlock_irqrestore(&psinfo->buf_lock, flags);
> > }
>
> afacit this assumes that (reason == KMSG_DUMP_PANIC) if in_nmi(). Is
> that always the case, and will it always be the case in the future?
I see your point. For now yes. The common case for which I think pstore
was designed for was the APEI/GHES case. Normally when GHES hits an NMI
it stays at the APEI/GHES layer and either uses an irq_workqueue for
recoverable errors or panics on non-recoverable errors.
So currently the only time it reaches the pstore layer is in the panic
case. Unfortunately, I can't vouch for all the backends that can hook
into pstore.
Perhaps a 'BUG_ON(in_nmi() && reason != KMSG_DUMP_PANIC)'?
>
> I felt that the spin_trylock() approach was less horrid than this. I
> assume that the new approach will cause lockdep to go berzerk?
Heh. Good point. That is probably a good test case. Though finding a
working GHES implementation in the firmware isn't easy these days, making
it hard to test. :-/
Cheers,
Don
|
|
From: Seiji A. <sei...@hd...> - 2011-10-18 14:53:13
|
Hi,
>> afacit this assumes that (reason == KMSG_DUMP_PANIC) if in_nmi(). Is
>> that always the case, and will it always be the case in the future?
>
Currently, when kernel is in nmi context and kmsg_dump() are called, its reason is always "KMSG_DUMP_PANIC".
>
>Perhaps a 'BUG_ON(in_nmi() && reason != KMSG_DUMP_PANIC)'?
I don't think BUG_ON() is needed.
If someone would like to log messages in the case of "in_nmi() && reason != KMSG_DUMP_PANIC",
he/she will add a new trigger as follows.
We can discuss implementation of kmsg_dump() at that time.
But I can't see any use case now.
371 static notrace __kprobes void
372 unknown_nmi_error(unsigned char reason, struct pt_regs *regs)
373 {
374 if (notify_die(DIE_NMIUNKNOWN, "nmi", regs, reason, 2, SIGINT) ==
375 NOTIFY_STOP)
376 return;
377 #ifdef CONFIG_MCA
378 /*
379 * Might actually be able to figure out what the guilty party
380 * is:
381 */
382 if (MCA_bus) {
383 mca_handle_nmi();
384 return;
385 }
386 #endif
387 pr_emerg("Uhhuh. NMI received for unknown reason %02x on CPU %d.\n",
388 reason, smp_processor_id());
389
390 pr_emerg("Do you have a strange power saving mode enabled?\n");
391 if (unknown_nmi_panic || panic_on_unrecovered_nmi)
392 panic("NMI: Not continuing");
393
394 pr_emerg("Dazed and confused, but trying to continue\n");
*kmsg_dump(NMI);*
395 }
>>
>> I felt that the spin_trylock() approach was less horrid than this. I
>> assume that the new approach will cause lockdep to go berzerk?
>
>Heh. Good point. That is probably a good test case. Though finding a
>working GHES implementation in the firmware isn't easy these days, making
>it hard to test. :-/
Thank you for supporting my patch.
But I can resend another patch below in accordance with Chen's request.
We can avoid potential deplock issue with this patch.
And this is simpler than trylock approach.
> <snip>
> If(!panic)
> spin_lock_irqsave();
>
> .
> .
> If(!panic)
> spin_unlock_restore();
> <snip>
Seiji
|
|
From: Don Z. <dz...@re...> - 2011-10-18 15:11:34
|
On Tue, Oct 18, 2011 at 10:52:22AM -0400, Seiji Aguchi wrote: > Hi, > > >> afacit this assumes that (reason == KMSG_DUMP_PANIC) if in_nmi(). Is > >> that always the case, and will it always be the case in the future? > > > > Currently, when kernel is in nmi context and kmsg_dump() are called, its reason is always "KMSG_DUMP_PANIC". > > > > >Perhaps a 'BUG_ON(in_nmi() && reason != KMSG_DUMP_PANIC)'? > > I don't think BUG_ON() is needed. > > If someone would like to log messages in the case of "in_nmi() && reason != KMSG_DUMP_PANIC", > he/she will add a new trigger as follows. The point I was trying to make with the BUG_ON is to catch future uses of NMI and kmsg_dump that were implemented without understanding the restriction we place (you have to be in a panic path for NMI to use pstore/kmsg_dump). That is part of the wider problem with kmsg_dump that Vivek talks about with me, is that it is just a giant hook in the panic path with limited auditing. So we need to explicit set our expectations with BUG_ONs/WARNs otherwise we might get bit later by them. Cheers, Don |
|
From: Seiji A. <sei...@hd...> - 2011-10-20 16:18:01
|
Don, >That is part of the wider problem with kmsg_dump that Vivek talks about >with me, is that it is just a giant hook in the panic path with limited >auditing. So we need to explicit set our expectations with BUG_ONs/WARNs >otherwise we might get bit later by them. I found an issue while developing a patch v2. We can't call BUG_ON(in_nmi() && reason != KMSG_DUMP_PANIC) because kmsg_dump(KMSG_DUMP_EMERG) is called in NMI context if "panic" kernel parameter is set. I will keep doing research bust_spinlock() or vprintk() so that lockdep checking works and we can avoid any deadlocks. Seiji |
|
From: Don Z. <dz...@re...> - 2011-10-20 17:48:53
|
On Thu, Oct 20, 2011 at 11:13:26AM -0400, Seiji Aguchi wrote: > Don, > > >That is part of the wider problem with kmsg_dump that Vivek talks about > >with me, is that it is just a giant hook in the panic path with limited > >auditing. So we need to explicit set our expectations with BUG_ONs/WARNs > >otherwise we might get bit later by them. > > I found an issue while developing a patch v2. > > We can't call BUG_ON(in_nmi() && reason != KMSG_DUMP_PANIC) > because kmsg_dump(KMSG_DUMP_EMERG) is called in NMI context if "panic" kernel parameter is set. I am confused. Did you mean 'panic' kernel parameter is _not_ set? Actually, BUG_ON is probably overkill, perhaps a WARN_ON would be more appropriate. > > I will keep doing research bust_spinlock() or vprintk() so that lockdep checking works and we can avoid any deadlocks. I wonder if it would be smarter to split your patch in half. The part where you move kmsg_dump to below stop_cpus is probably non-controversial. That might make it sooner than the more controversial bust_spinlock pieces. Cheers, Don |
|
From: Seiji A. <sei...@hd...> - 2011-10-20 18:40:16
|
Hi,
>I am confused. Did you mean 'panic' kernel parameter is _not_ set?
>
I meant that if panic parameter _is_ set (like panic=1),
emergency_restart() will be called in panic path.
And then, kmsg_dump(KMSG_DUMP_EMERG) will be called.
<snip>
60 NORET_TYPE void panic(const char * fmt, ...)
61 {
.
.
123 if (panic_timeout != 0) {
124 /*
125 * This will not be a clean reboot, with everything
126 * shutting down. But if there is a chance of
127 * rebooting the system it will be rebooted.
128 */
129 emergency_restart();
130 }
<snip>
>Actually, BUG_ON is probably overkill, perhaps a WARN_ON would be more
>appropriate.
I don't think both BUG_ON and WARN_ON are appropriate.
kmsg_dump(KMSG_DUMP_EMERG) should be called successfully in panic path.
>I wonder if it would be smarter to split your patch in half. The part
>where you move kmsg_dump to below stop_cpus is probably non-controversial.
>That might make it sooner than the more controversial bust_spinlock
>pieces.
Thank you for your suggestion. I will split my patch.
Seiji
>On Thu, Oct 20, 2011 at 11:13:26AM -0400, Seiji Aguchi wrote:
>> Don,
>>
>> >That is part of the wider problem with kmsg_dump that Vivek talks about
>> >with me, is that it is just a giant hook in the panic path with limited
>> >auditing. So we need to explicit set our expectations with BUG_ONs/WARNs
>> >otherwise we might get bit later by them.
>>
>> I found an issue while developing a patch v2.
>>
>> We can't call BUG_ON(in_nmi() && reason != KMSG_DUMP_PANIC)
>> because kmsg_dump(KMSG_DUMP_EMERG) is called in NMI context if "panic" kernel parameter is set.
>
>I am confused. Did you mean 'panic' kernel parameter is _not_ set?
>
>Actually, BUG_ON is probably overkill, perhaps a WARN_ON would be more
>appropriate.
>
>>
>> I will keep doing research bust_spinlock() or vprintk() so that lockdep checking works and we can avoid any deadlocks.
>
>I wonder if it would be smarter to split your patch in half. The part
>where you move kmsg_dump to below stop_cpus is probably non-controversial.
>That might make it sooner than the more controversial bust_spinlock
>pieces.
>
>Cheers,
>Don
|
