Re: Stop scheduler on panic
On 12/4/11 5:11 PM, Andriy Gapon wrote:
on 02/12/2011 17:30 m...@freebsd.org said the following:
On Fri, Dec 2, 2011 at 2:05 AM, Andriy Gapon wrote:
on 02/12/2011 06:36 John Baldwin said the following:
Ah, ok (I had thought SCHEDULER_STOPPED was going to always be true when kdb was
active). But I think these two changes should cover critical_exit() ok.
I attempted to start a discussion about this a few times already :-)
Should we treat kdb context the same as SCHEDULER_STOPPED context (in the
current definition) ? That is, skip all locks in the same fashion?
There are pros and contras.
Does kdb pause all CPUs with an interrupt (NMI or regular interrupt, I
can no longer remember...) when it enters? If so, then I'd say
whether it enters via sysctl or panic doesn't matter. It's in a
special environment where nothing else is running, which is what is
needed for proper exploration of the machine (via breakpoint, for
debugging a hang, etc).
Maybe the question is, why wouldn't SCHEDULER_STOPPED be true
regardless of how kdb is entered?
I think that the discussion that followed has clarified this point a bit.
SCHEDULER_STOPPED perhaps needs a better name :-) Currently it, the name,
reflects the state of the scheduler, but not why the scheduler is stopped and
not the greater state of the system ("in panic"), nor how we should handle that
state ("bypass locking"). So I'd love something like BYPASS_LOCKING_BECAUSE
_SCHEDULER_IS_STOPPED_IN_PANIC haven't it be so unwieldy :)
Oh, hmm. Yes, being in the debugger should not potentially corrupt lock
state, so in that sense it is a weaker stop.
--
John Baldwin
___
freebsd-current@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-current
To unsubscribe, send any mail to "freebsd-current-unsubscr...@freebsd.org"
Re: Stop scheduler on panic
2011/12/7 Andriy Gapon :
> on 07/12/2011 00:11 Attilio Rao said the following:
>> I'd just change this check on panicstr:
>> @@ -606,9 +603,13 @@ kdb_trap(int type, int code, struct trapframe *tf)
>> intr = intr_disable();
>>
>> #ifdef SMP
>> - other_cpus = all_cpus;
>> - CPU_CLR(PCPU_GET(cpuid), &other_cpus);
>> - stop_cpus_hard(other_cpus);
>> + if (panicstr == NULL) {
>> + other_cpus = all_cpus;
>> + CPU_CLR(PCPU_GET(cpuid), &other_cpus);
>> + stop_cpus_hard(other_cpus);
>> + did_stop_cpus = 1;
>> + } else
>> + did_stop_cpus = 0;
>>
>> to be SCHEDULER_STOPPED().
>
> Makes sense. I will do this.
>
>> If you agree I can fix the kern_mutex, kern_sx and kern_rwlock parts
>> and it should be done.
>
> Since I am not very familiar with the details of that code, I can not be
> against
> such a proposal :-) What Kostik did seemed quite reasonable to me, but if
> that
> can be further improved, then I am all for it.
The following patch is a further add-on on Kostik's:
http://www.freebsd.org/~attilio/scheduler_stopped.patch
- Rework of mutex, rwlock and sxlock for a correct dealing of hard and
fast paths
- Protection of LOCK_PROFILING bits (missed also in my review)
- Protection of WITNESS_SAVE/RESTORE because of Giant handling (missed
also in my review)
- Removal of gratuitous whitelines
- Usage of SCHEDULER_STOPPED() in kdb check
What do you think about it?
I just test-compiled it with several combinations of LOCK_PROFILING
and LOCK_DEBUG, but I didn't change the bulk of it thus it should be
perfectly fine.
If you like it I'd say to go for the commit asap.
I wonder if someone tried to simulate a livelock and panic and thus
verify that stoppcbs is correctly populated as expected (to be honest,
this is one of the best features I'm interested into for this patch).
Thanks,
Attilio
--
Peace can only be achieved by understanding - A. Einstein
___
freebsd-current@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-current
To unsubscribe, send any mail to "freebsd-current-unsubscr...@freebsd.org"
Re: Stop scheduler on panic
on 07/12/2011 00:11 Attilio Rao said the following:
> I'd just change this check on panicstr:
> @@ -606,9 +603,13 @@ kdb_trap(int type, int code, struct trapframe *tf)
> intr = intr_disable();
>
> #ifdef SMP
> - other_cpus = all_cpus;
> - CPU_CLR(PCPU_GET(cpuid), &other_cpus);
> - stop_cpus_hard(other_cpus);
> + if (panicstr == NULL) {
> + other_cpus = all_cpus;
> + CPU_CLR(PCPU_GET(cpuid), &other_cpus);
> + stop_cpus_hard(other_cpus);
> + did_stop_cpus = 1;
> + } else
> + did_stop_cpus = 0;
>
> to be SCHEDULER_STOPPED().
Makes sense. I will do this.
> If you agree I can fix the kern_mutex, kern_sx and kern_rwlock parts
> and it should be done.
Since I am not very familiar with the details of that code, I can not be against
such a proposal :-) What Kostik did seemed quite reasonable to me, but if that
can be further improved, then I am all for it.
--
Andriy Gapon
___
freebsd-current@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-current
To unsubscribe, send any mail to "freebsd-current-unsubscr...@freebsd.org"
Re: Stop scheduler on panic
2011/12/6 Andriy Gapon :
> on 06/12/2011 20:34 Attilio Rao said the following:
> [snip]
>> - I'm not entirely sure, why we want to disable interrupts at this
>> moment (before to stop other CPUs)?:
>
> Because I believe that stop_cpus_hard() should run in a context with
> interrupts
> and preemption disabled. Also, I believe that the whole panic handling code
> should run in the same context. So it was only natural for me to enter that
> context at this point.
I'm not against that, I don't see anything wrong with having
interrupts disabled at that point.
>> @@ -547,13 +555,18 @@ panic(const char *fmt, ...)
>> {
>> #ifdef SMP
>> static volatile u_int panic_cpu = NOCPU;
>> + cpuset_t other_cpus;
>> #endif
>> struct thread *td = curthread;
>> int bootopt, newpanic;
>> va_list ap;
>> static char buf[256];
>>
>> - critical_enter();
>> + if (stop_scheduler_on_panic)
>> + spinlock_enter();
>> + else
>> + critical_enter();
>> +
>>
>> - In this chunk I don't entirely understand the kdb_active check:
>> @@ -566,11 +579,18 @@ panic(const char *fmt, ...)
>> PCPU_GET(cpuid)) == 0)
>> while (panic_cpu != NOCPU)
>> ; /* nothing */
>> + if (stop_scheduler_on_panic) {
>> + if (panicstr == NULL && !kdb_active) {
>> + other_cpus = all_cpus;
>> + CPU_CLR(PCPU_GET(cpuid), &other_cpus);
>> + stop_cpus_hard(other_cpus);
>> + }
>> + }
>> #endif
>>
>> bootopt = RB_AUTOBOOT;
>> newpanic = 0;
>> - if (panicstr)
>> + if (panicstr != NULL)
>> bootopt |= RB_NOSYNC;
>> else {
>> bootopt |= RB_DUMP;
>>
>> Is it for avoiding to pass an empty mask to stop_cpus() in kdb_trap()
>> (I saw you changed the policy there)?
>
> Yes. You know my position about elimination of the cpuset parameter to
> stop_cpus_* and my intention to do so. This is related to that. Right now
> that
> check is not strictly necessary, but it doesn't do any harm either. We know
> that all other CPUs are already stopped when kdb_active (ditto for panicstr !=
> NULL).
I see there could be races with disabiling interrupts and having 2
different stopping mechanisms that want to stop cpus, even using
stop_cpus_hard(), on architectures that don't use a privileged channel
(NMI) as mostly of our !x86.
They are mostly very rare races (requiring kdb_trap() and panic() to
happen in parallel on different CPUs).
>> Maybe we can find a better integration among the two.
>
> What kind of integration?
> Right now I have simple model - both stop all other CPUs.
Well, there is no synchronization atm between panic stopping cpus and
the kdb_trap(). When kdb_trap() stop cpus it has interrupts disabled
and if panic already started they will both deadlock because IPI_STOP
won't be properly delivered.
However, I see all this as a problem with other arches not having/not
implementing a real dedicated channel for cpu_stop_hard(), so we
should not think about it now.
I think we may need some sort of control as panic already does with
panic_cpu before to disable interrupts, but don't worry about it now.
>> I'd also move the setting of stop_scheduler variable in the "if", it
>> seems a bug to me to have it set otherwise.
>
> Can you please explain what bug do you suspect here?
> I do not see any.
I just see more natural to move it within the above if (panicstr ==
NULL ...) condition.
> [snip]
>> - I'm not sure I like to change the policies on cpu stopping for KDB
>> with this patchset.
>
> I am not sure what exactly you mean by change in policies. I do not see any
> such change, entering kdb always stops all other CPUs, with or without the
> patch.
Yes, I was confused by older code did just stopped CPUs before
kdb_trap() manually, I think what kdb_trap() does now is ok (and you
just retain it).
I'd just change this check on panicstr:
@@ -606,9 +603,13 @@ kdb_trap(int type, int code, struct trapframe *tf)
intr = intr_disable();
#ifdef SMP
- other_cpus = all_cpus;
- CPU_CLR(PCPU_GET(cpuid), &other_cpus);
- stop_cpus_hard(other_cpus);
+ if (panicstr == NULL) {
+ other_cpus = all_cpus;
+ CPU_CLR(PCPU_GET(cpuid), &other_cpus);
+ stop_cpus_hard(other_cpus);
+ did_stop_cpus = 1;
+ } else
+ did_stop_cpus = 0;
to be SCHEDULER_STOPPED().
If you agree I can fix the kern_mutex, kern_sx and kern_rwlock parts
and it should be done.
Attilio
--
Peace can only be achieved by understanding - A. Einstein
___
freebsd-current@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-current
To unsubscribe, send any mail to "freebsd-current-unsubscr...@freebsd.org"
Re: Stop scheduler on panic
on 06/12/2011 20:34 Attilio Rao said the following:
[snip]
> - I'm not entirely sure, why we want to disable interrupts at this
> moment (before to stop other CPUs)?:
Because I believe that stop_cpus_hard() should run in a context with interrupts
and preemption disabled. Also, I believe that the whole panic handling code
should run in the same context. So it was only natural for me to enter that
context at this point.
> @@ -547,13 +555,18 @@ panic(const char *fmt, ...)
> {
> #ifdef SMP
> static volatile u_int panic_cpu = NOCPU;
> + cpuset_t other_cpus;
> #endif
> struct thread *td = curthread;
> int bootopt, newpanic;
> va_list ap;
> static char buf[256];
>
> - critical_enter();
> + if (stop_scheduler_on_panic)
> + spinlock_enter();
> + else
> + critical_enter();
> +
>
> - In this chunk I don't entirely understand the kdb_active check:
> @@ -566,11 +579,18 @@ panic(const char *fmt, ...)
> PCPU_GET(cpuid)) == 0)
> while (panic_cpu != NOCPU)
> ; /* nothing */
> + if (stop_scheduler_on_panic) {
> + if (panicstr == NULL && !kdb_active) {
> + other_cpus = all_cpus;
> + CPU_CLR(PCPU_GET(cpuid), &other_cpus);
> + stop_cpus_hard(other_cpus);
> + }
> + }
> #endif
>
> bootopt = RB_AUTOBOOT;
> newpanic = 0;
> - if (panicstr)
> + if (panicstr != NULL)
> bootopt |= RB_NOSYNC;
> else {
> bootopt |= RB_DUMP;
>
> Is it for avoiding to pass an empty mask to stop_cpus() in kdb_trap()
> (I saw you changed the policy there)?
Yes. You know my position about elimination of the cpuset parameter to
stop_cpus_* and my intention to do so. This is related to that. Right now that
check is not strictly necessary, but it doesn't do any harm either. We know
that all other CPUs are already stopped when kdb_active (ditto for panicstr !=
NULL).
> Maybe we can find a better integration among the two.
What kind of integration?
Right now I have simple model - both stop all other CPUs.
> I'd also move the setting of stop_scheduler variable in the "if", it
> seems a bug to me to have it set otherwise.
Can you please explain what bug do you suspect here?
I do not see any.
[snip]
> - I'm not sure I like to change the policies on cpu stopping for KDB
> with this patchset.
I am not sure what exactly you mean by change in policies. I do not see any
such change, entering kdb always stops all other CPUs, with or without the
patch.
> I think we should discuss this furthermore, in
> particular in terms of reviewing what accesses points KDB has and if
> they are all covered.
Please review the code and see if anything is left to discuss :-)
My review shows that all access points are covered. Essentially there is only
one access point - kdb_trap. It's called either directly from a known trap
context or indirectly (via a debug trap) using kdb_enter.
> If someone can summary this up (and has already made the analysis) and
> would please share his findings I'd be happy about it, otherwise we
> should not commit the stop_cpu approach in kdb_trap() IMHO.
I hope that the above answers somewhat clear your concerns. Just in case, if
you can point out any clear specific problems with this approach, then that can
block me from committing. A fuzzy feeling that something might be wrong won't
do that :-)
--
Andriy Gapon
___
freebsd-current@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-current
To unsubscribe, send any mail to "freebsd-current-unsubscr...@freebsd.org"
Re: Stop scheduler on panic
2011/11/13 Kostik Belousov :
> I was tricked into finishing the work by Andrey Gapon, who developed
> the patch to reliably stop other processors on panic. The patch
> greatly improves the chances of getting dump on panic on SMP host.
> Several people already saw the patchset, and I remember that Andrey
> posted it to some lists.
>
> The change stops other (*) processors early upon the panic. This way,
> no parallel manipulation of the kernel memory is performed by CPUs.
> In particular, the kernel memory map is static. Patch prevents the
> panic thread from blocking and switching out.
>
> * - in the context of the description, other means not current.
>
> Since other threads are not run anymore, lock owner cannot release a
> lock which is required by panic thread. Due to this, we need to fake
> a lock acquisition after the panic, which adds minimal overhead to the
> locking cost. The patch tries to not add any overhead on the fast path
> of the lock acquire. The check for the after-panic condition was
> reduced to single memory access, done only when the quick cas lock
> attempt failed, and braced with __unlikely compiler hint.
>
> For now, the new mode of operation is disabled by default, since some
> further USB changes are needed to make USB keyboard usable in that
> environment.
>
> With the patch, getting a dump from the machine without debugger
> compiled in is much more realistic. Please comment, I will commit the
> change in 2 weeks unless strong reasons not to are given.
>
> http://people.freebsd.org/~kib/misc/stop_cpus_on_panic.1.patch
Below are reported my notes on the patch:
- Just by looking at kern_mutex.c chunk I see that you have added
SCHEDULER_STOPPED() checks on very specific places, usually after
sanity checks by WITNESS (and similar) and sometimes in odd places (as
the chunk involving _mtx_lock_spin_flags). I think that we should just
skip all the checks along with the hard locking operation. Ideall we
should also skip the fast path, IMHO, but it is impossible (without
polluting it), but at least skip the vast majority of operations for
the hard one, so that we get, for example:
%svn diff -x -p kern/kern_mutex.c | less
Index: kern/kern_mutex.c
===
--- kern/kern_mutex.c (revision 228308)
+++ kern/kern_mutex.c (working copy)
@@ -232,6 +232,8 @@ void
_mtx_lock_spin_flags(struct mtx *m, int opts, const char *file, int line)
{
+ if (SCHEDULER_STOPPED())
+ return;
MPASS(curthread != NULL);
KASSERT(m->mtx_lock != MTX_DESTROYED,
("mtx_lock_spin() of destroyed mutex @ %s:%d", file, line));
In this optic I'd patch directly the hard functions rather than
waiting them to hit the smallest possible common point (which are
_mtx_lock_sleep() and _mtx_lock_spin()). That will make the patch more
verbose but more precise and more correct too.
- This chunk is unneeded now:
@@ -577,6 +589,7 @@ retry:
m->mtx_recurse++;
break;
}
+
lock_profile_obtain_lock_failed(&m->lock_object,
&contested, &waittime);
/* Give interrupts a chance while we spin. */
- I'm not entirely sure, why we want to disable interrupts at this
moment (before to stop other CPUs)?:
@@ -547,13 +555,18 @@ panic(const char *fmt, ...)
{
#ifdef SMP
static volatile u_int panic_cpu = NOCPU;
+ cpuset_t other_cpus;
#endif
struct thread *td = curthread;
int bootopt, newpanic;
va_list ap;
static char buf[256];
- critical_enter();
+ if (stop_scheduler_on_panic)
+ spinlock_enter();
+ else
+ critical_enter();
+
- In this chunk I don't entirely understand the kdb_active check:
@@ -566,11 +579,18 @@ panic(const char *fmt, ...)
PCPU_GET(cpuid)) == 0)
while (panic_cpu != NOCPU)
; /* nothing */
+ if (stop_scheduler_on_panic) {
+ if (panicstr == NULL && !kdb_active) {
+ other_cpus = all_cpus;
+ CPU_CLR(PCPU_GET(cpuid), &other_cpus);
+ stop_cpus_hard(other_cpus);
+ }
+ }
#endif
bootopt = RB_AUTOBOOT;
newpanic = 0;
- if (panicstr)
+ if (panicstr != NULL)
bootopt |= RB_NOSYNC;
else {
bootopt |= RB_DUMP;
Is it for avoiding to pass an empty mask to stop_cpus() in kdb_trap()
(I saw you changed the policy there)?
Maybe we can find a better integration among the two.
I'd also move the setting of stop_scheduler variable in the "if", it
seems a bug to me to have it set otherwise.
- The same reservations expressed about the hard path on mutex also
applies to rwlock and sxlock.
- I'm not sure I like to change the policies on cpu stopping
Re: Stop scheduler on panic
2011/12/4 Andriy Gapon : > on 02/12/2011 19:18 Attilio Rao said the following: >> BTW, I'm waiting for the details to settle (including the patch we >> have been discussing internally about binding to CPU0 during ACPI >> shutdown) > > I do not see strong interdependency between that patch and the panic patch. > BTW, I think that your patch is good to go. I agree, we can get back to this once the stop_scheduler patch is in. Attilio -- Peace can only be achieved by understanding - A. Einstein ___ freebsd-current@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/freebsd-current To unsubscribe, send any mail to "freebsd-current-unsubscr...@freebsd.org"
Re: Stop scheduler on panic
2011/12/4 Andriy Gapon : > on 21/11/2011 18:58 Attilio Rao said the following: >> I would be very in favor about having a 'thread trampoline for KDB', >> thus that it can use locks. > > I keep hearing the suggestion to add this trampoline, but I admit that I do > not > understand its technical meaning in this context. And also how it helps with > the locking. So I will appreciate an explanation! Thanks! kdb_trap() now runs in interrupt context, my suggestion was to just to give KDB its own context (a new kernel thread) and yield its execution when KDB needs to be entered, this way it is possible to use locking and avoid functions duplications. In theory, this avoids constructing complicate algorithms to be lockless when implementing primitives KDB should use. However, I now realize a problem: if we want to stop CPUs we don't really want to acquire locks anyway because of CPU restart. Likely, the KDB trampoline is not a good option for this reason and we should work instead on getting KDB functions to be totally lockless. Another thing I'm considering is, however, the entrypoint for KDB. When I looked into it months ago I thought there is a mismatch between kdb_enter() (which should disable CPUs) and other ways to enter KDB (maybe some paths calling directly kdb_trap()?). We must offer an unified policy and entrypoint, being likely to disable CPUs when entering it. Attilio -- Peace can only be achieved by understanding - A. Einstein ___ freebsd-current@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/freebsd-current To unsubscribe, send any mail to "freebsd-current-unsubscr...@freebsd.org"
Re: Stop scheduler on panic
2011/12/2 Andriy Gapon : > on 02/12/2011 20:40 John Baldwin said the following: >> On 12/2/11 12:18 PM, Attilio Rao wrote: >>> 2011/12/2 John Baldwin: On 12/2/11 5:05 AM, Andriy Gapon wrote: > > on 02/12/2011 06:36 John Baldwin said the following: >> >> Ah, ok (I had thought SCHEDULER_STOPPED was going to always be true when >> kdb was >> active). But I think these two changes should cover critical_exit() ok. >> > > I attempted to start a discussion about this a few times already :-) > Should we treat kdb context the same as SCHEDULER_STOPPED context (in the > current definition) ? That is, skip all locks in the same fashion? > There are pros and contras. kdb should not block on locks, no. Most debugger commands should not go near locks anyway unless they are intended to carefully modify the existing system in a safe manner (such as the 'kill' command which should only be using try locks and fail if it cannot safely post the signal). >>> >>> The biggest problem to KDB as the same as panic is that doing proper >>> 'continue' is impossible. >>> One of the features of the 'skip-locking' path is that it doesn't take >>> into account fast locking paths, where sometimes the lock can succeed >>> and other fails and you don't know about them. Also the restarted CPUs >>> can find corrupted datas (as they can be arbitrarely updated), I'm >>> sure it is too much panic prone. >> >> Yes, my thought is that kdb commands, etc. should be using dedicated routines >> that do not use locks whenever possible. The problem of a user >> calling an arbitrary routine is not solvable (so I don't think we should try >> to >> solve that, you use 'call' at your own risk), but built-in commands should >> explicitly either 1) not use locking, or 2) only use try locks and fail out >> cleanly (including dropping any try locks acquired) if a try fails. Now, >> that's >> an ideal view, I don't know how close we are to that in practice or if it is >> a >> realistically attainable goal. >> > > > I agree with what Attilio and you say. Initially it was tempting for me to > apply the same SCHEDULER_STOPPED stopped medicine to the kdb_active context, > but > after trying to deal with kdb_active x SCHEDULER_STOPPED x ukbd situation I > really changed my mind. > > > I would classify the code that can be called in kdb_active context as follows: > o debugger code proper (kdb, ddb, gdb stub, etc) - this obviously must not > (doesn't have to) use any locking > > o code that can be invoked via 'call' command - this is essentially any code > and > I don't think that it can/should do anything special for the kdb_active > context [*] > > o debugger helper routines - those that do something trivial should not > acquire > any locks; those that access shared resources should try the relevant locks > and > bail out if a resource can be in inconsistent state, or should be equipped to > deal correctly with such a state; this is the same as what you say above > > o common code that the debuggers have to use - most obviously this is console > code and drivers that serve a particular console; on one hand those drivers > can > have a non-trivial state that must be lock protected during normal operation, > on > the other hand the debugger must disregard those locks and grab its console; > this is the most complex case in my opinion. Thanks for summarizing this up. However, please note that code in 2 and 4 entries may have the same issues or being the same thing, in practice. Anyway, I'm thinking now that if we really want to stop CPUs when entering KDB (and I think we do) functions at 2 and 4 should basically just be totally lockless or in general being totally re-entrant because when we restart CPUs we don't really want them finding datas to be corrupted. Also, skipping locking, is totally broken for this very specific reason. Functions at point 2 and 4 should be totally lockless then and possibly just work on read mode. For point 2, specifically, I think we need an explicit KPI to define functions within the subsystem themselves (something like DB_SHOW_COMMAND()) which marks undoublty functions to be called within ddb (the only KDB backend we implement right now) and likely for functions at point 4 we need to find a way to stress their belonging to the KDB area. Attilio -- Peace can only be achieved by understanding - A. Einstein ___ freebsd-current@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/freebsd-current To unsubscribe, send any mail to "freebsd-current-unsubscr...@freebsd.org"
Re: Stop scheduler on panic
on 02/12/2011 19:18 Attilio Rao said the following: > BTW, I'm waiting for the details to settle (including the patch we > have been discussing internally about binding to CPU0 during ACPI > shutdown) I do not see strong interdependency between that patch and the panic patch. BTW, I think that your patch is good to go. > before to read the whole thread and start a proper review, > would it be possible to have an almost-final version of the patch? It is still the same patch that Kostik posted a few weeks ago. I think that it is long overdue to be committed. -- Andriy Gapon ___ freebsd-current@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/freebsd-current To unsubscribe, send any mail to "freebsd-current-unsubscr...@freebsd.org"
Re: Stop scheduler on panic
on 21/11/2011 18:58 Attilio Rao said the following: > I would be very in favor about having a 'thread trampoline for KDB', > thus that it can use locks. I keep hearing the suggestion to add this trampoline, but I admit that I do not understand its technical meaning in this context. And also how it helps with the locking. So I will appreciate an explanation! Thanks! -- Andriy Gapon ___ freebsd-current@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/freebsd-current To unsubscribe, send any mail to "freebsd-current-unsubscr...@freebsd.org"
Re: Stop scheduler on panic
on 02/12/2011 17:30 m...@freebsd.org said the following:
> On Fri, Dec 2, 2011 at 2:05 AM, Andriy Gapon wrote:
>> on 02/12/2011 06:36 John Baldwin said the following:
>>> Ah, ok (I had thought SCHEDULER_STOPPED was going to always be true when
>>> kdb was
>>> active). But I think these two changes should cover critical_exit() ok.
>>>
>>
>> I attempted to start a discussion about this a few times already :-)
>> Should we treat kdb context the same as SCHEDULER_STOPPED context (in the
>> current definition) ? That is, skip all locks in the same fashion?
>> There are pros and contras.
>
> Does kdb pause all CPUs with an interrupt (NMI or regular interrupt, I
> can no longer remember...) when it enters? If so, then I'd say
> whether it enters via sysctl or panic doesn't matter. It's in a
> special environment where nothing else is running, which is what is
> needed for proper exploration of the machine (via breakpoint, for
> debugging a hang, etc).
>
> Maybe the question is, why wouldn't SCHEDULER_STOPPED be true
> regardless of how kdb is entered?
I think that the discussion that followed has clarified this point a bit.
SCHEDULER_STOPPED perhaps needs a better name :-) Currently it, the name,
reflects the state of the scheduler, but not why the scheduler is stopped and
not the greater state of the system ("in panic"), nor how we should handle that
state ("bypass locking"). So I'd love something like BYPASS_LOCKING_BECAUSE
_SCHEDULER_IS_STOPPED_IN_PANIC haven't it be so unwieldy :)
When the kdb_active is true the scheduler is stopped, true. But it is a
different kind of system state from which we potentially may want to continue
normal operation. So a lot more care is needed and simply bypassing locks is
probably not a solution. I guess that some day in the distant future we might
decide that a built-in debugger is for critical/abnormal situations only...
--
Andriy Gapon
___
freebsd-current@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-current
To unsubscribe, send any mail to "freebsd-current-unsubscr...@freebsd.org"
Re: Stop scheduler on panic
on 02/12/2011 20:40 John Baldwin said the following: > On 12/2/11 12:18 PM, Attilio Rao wrote: >> 2011/12/2 John Baldwin: >>> On 12/2/11 5:05 AM, Andriy Gapon wrote: on 02/12/2011 06:36 John Baldwin said the following: > > Ah, ok (I had thought SCHEDULER_STOPPED was going to always be true when > kdb was > active). But I think these two changes should cover critical_exit() ok. > I attempted to start a discussion about this a few times already :-) Should we treat kdb context the same as SCHEDULER_STOPPED context (in the current definition) ? That is, skip all locks in the same fashion? There are pros and contras. >>> >>> >>> kdb should not block on locks, no. Most debugger commands should not go >>> near locks anyway unless they are intended to carefully modify the existing >>> system in a safe manner (such as the 'kill' command which should only be >>> using try locks and fail if it cannot safely post the signal). >> >> The biggest problem to KDB as the same as panic is that doing proper >> 'continue' is impossible. >> One of the features of the 'skip-locking' path is that it doesn't take >> into account fast locking paths, where sometimes the lock can succeed >> and other fails and you don't know about them. Also the restarted CPUs >> can find corrupted datas (as they can be arbitrarely updated), I'm >> sure it is too much panic prone. > > Yes, my thought is that kdb commands, etc. should be using dedicated routines > that do not use locks whenever possible. The problem of a user > calling an arbitrary routine is not solvable (so I don't think we should try > to > solve that, you use 'call' at your own risk), but built-in commands should > explicitly either 1) not use locking, or 2) only use try locks and fail out > cleanly (including dropping any try locks acquired) if a try fails. Now, > that's > an ideal view, I don't know how close we are to that in practice or if it is a > realistically attainable goal. > I agree with what Attilio and you say. Initially it was tempting for me to apply the same SCHEDULER_STOPPED stopped medicine to the kdb_active context, but after trying to deal with kdb_active x SCHEDULER_STOPPED x ukbd situation I really changed my mind. I would classify the code that can be called in kdb_active context as follows: o debugger code proper (kdb, ddb, gdb stub, etc) - this obviously must not (doesn't have to) use any locking o code that can be invoked via 'call' command - this is essentially any code and I don't think that it can/should do anything special for the kdb_active context [*] o debugger helper routines - those that do something trivial should not acquire any locks; those that access shared resources should try the relevant locks and bail out if a resource can be in inconsistent state, or should be equipped to deal correctly with such a state; this is the same as what you say above o common code that the debuggers have to use - most obviously this is console code and drivers that serve a particular console; on one hand those drivers can have a non-trivial state that must be lock protected during normal operation, on the other hand the debugger must disregard those locks and grab its console; this is the most complex case in my opinion. Dealing with panics is much simpler, because it's a one way road to a system reset. Possibility to enter and exit debugger implies additional complications. So it doesn't look like SCHEDULER_STOPPED can be used equivalently for panic and for kdb_active. kdb_active requires more elaborate handling. [*] - but currently we depend on some "general purpose" routines to be 'callable' from debugger where we should really have a debugger command; the most popular example is 'call doadump'. -- Andriy Gapon ___ freebsd-current@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/freebsd-current To unsubscribe, send any mail to "freebsd-current-unsubscr...@freebsd.org"
Re: Stop scheduler on panic
2011/12/2 John Baldwin : > On 12/2/11 12:18 PM, Attilio Rao wrote: >> >> 2011/12/2 John Baldwin: >>> >>> On 12/2/11 5:05 AM, Andriy Gapon wrote: on 02/12/2011 06:36 John Baldwin said the following: > > > Ah, ok (I had thought SCHEDULER_STOPPED was going to always be true > when > kdb was > active). But I think these two changes should cover critical_exit() > ok. > I attempted to start a discussion about this a few times already :-) Should we treat kdb context the same as SCHEDULER_STOPPED context (in the current definition) ? That is, skip all locks in the same fashion? There are pros and contras. >>> >>> >>> >>> kdb should not block on locks, no. Most debugger commands should not go >>> near locks anyway unless they are intended to carefully modify the >>> existing >>> system in a safe manner (such as the 'kill' command which should only be >>> using try locks and fail if it cannot safely post the signal). >> >> >> The biggest problem to KDB as the same as panic is that doing proper >> 'continue' is impossible. >> One of the features of the 'skip-locking' path is that it doesn't take >> into account fast locking paths, where sometimes the lock can succeed >> and other fails and you don't know about them. Also the restarted CPUs >> can find corrupted datas (as they can be arbitrarely updated), I'm >> sure it is too much panic prone. > > > Yes, my thought is that kdb commands, etc. should be using dedicated > routines that do not use locks whenever possible. The problem of a user > calling an arbitrary routine is not solvable (so I don't think we should try > to solve that, you use 'call' at your own risk), but built-in commands > should explicitly either 1) not use locking, or 2) only use try locks and > fail out cleanly (including dropping any try locks acquired) if a try fails. > Now, that's an ideal view, I don't know how close we are to that in > practice or if it is a realistically attainable goal. So you are not in favor of giving KDB its own context? There are some fallbacks (like, for example, bugs involving the scheduler or switching mechanism but for that we can make a facility like KDB_LITE if you want to debug a scheduler problem), but in general that would avoid replicating code to avoid the locking. If you don't want to give KDB its own context, we should work on a KPI (or similar) that defines the command to serve as KDB commands, that tries to keep things under control, etc. Attilio -- Peace can only be achieved by understanding - A. Einstein ___ freebsd-current@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/freebsd-current To unsubscribe, send any mail to "freebsd-current-unsubscr...@freebsd.org"
Re: Stop scheduler on panic
On 12/2/11 12:18 PM, Attilio Rao wrote: 2011/12/2 John Baldwin: On 12/2/11 5:05 AM, Andriy Gapon wrote: on 02/12/2011 06:36 John Baldwin said the following: Ah, ok (I had thought SCHEDULER_STOPPED was going to always be true when kdb was active). But I think these two changes should cover critical_exit() ok. I attempted to start a discussion about this a few times already :-) Should we treat kdb context the same as SCHEDULER_STOPPED context (in the current definition) ? That is, skip all locks in the same fashion? There are pros and contras. kdb should not block on locks, no. Most debugger commands should not go near locks anyway unless they are intended to carefully modify the existing system in a safe manner (such as the 'kill' command which should only be using try locks and fail if it cannot safely post the signal). The biggest problem to KDB as the same as panic is that doing proper 'continue' is impossible. One of the features of the 'skip-locking' path is that it doesn't take into account fast locking paths, where sometimes the lock can succeed and other fails and you don't know about them. Also the restarted CPUs can find corrupted datas (as they can be arbitrarely updated), I'm sure it is too much panic prone. Yes, my thought is that kdb commands, etc. should be using dedicated routines that do not use locks whenever possible. The problem of a user calling an arbitrary routine is not solvable (so I don't think we should try to solve that, you use 'call' at your own risk), but built-in commands should explicitly either 1) not use locking, or 2) only use try locks and fail out cleanly (including dropping any try locks acquired) if a try fails. Now, that's an ideal view, I don't know how close we are to that in practice or if it is a realistically attainable goal. -- John Baldwin ___ freebsd-current@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/freebsd-current To unsubscribe, send any mail to "freebsd-current-unsubscr...@freebsd.org"
Re: Stop scheduler on panic
2011/12/2 John Baldwin : > On 12/2/11 5:05 AM, Andriy Gapon wrote: >> >> on 02/12/2011 06:36 John Baldwin said the following: >>> >>> Ah, ok (I had thought SCHEDULER_STOPPED was going to always be true when >>> kdb was >>> active). But I think these two changes should cover critical_exit() ok. >>> >> >> I attempted to start a discussion about this a few times already :-) >> Should we treat kdb context the same as SCHEDULER_STOPPED context (in the >> current definition) ? That is, skip all locks in the same fashion? >> There are pros and contras. > > > kdb should not block on locks, no. Most debugger commands should not go > near locks anyway unless they are intended to carefully modify the existing > system in a safe manner (such as the 'kill' command which should only be > using try locks and fail if it cannot safely post the signal). The biggest problem to KDB as the same as panic is that doing proper 'continue' is impossible. One of the features of the 'skip-locking' path is that it doesn't take into account fast locking paths, where sometimes the lock can succeed and other fails and you don't know about them. Also the restarted CPUs can find corrupted datas (as they can be arbitrarely updated), I'm sure it is too much panic prone. BTW, I'm waiting for the details to settle (including the patch we have been discussing internally about binding to CPU0 during ACPI shutdown) before to read the whole thread and start a proper review, would it be possible to have an almost-final version of the patch? Attilio -- Peace can only be achieved by understanding - A. Einstein ___ freebsd-current@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/freebsd-current To unsubscribe, send any mail to "freebsd-current-unsubscr...@freebsd.org"
Re: Stop scheduler on panic
On 12/2/11 5:05 AM, Andriy Gapon wrote: on 02/12/2011 06:36 John Baldwin said the following: Ah, ok (I had thought SCHEDULER_STOPPED was going to always be true when kdb was active). But I think these two changes should cover critical_exit() ok. I attempted to start a discussion about this a few times already :-) Should we treat kdb context the same as SCHEDULER_STOPPED context (in the current definition) ? That is, skip all locks in the same fashion? There are pros and contras. kdb should not block on locks, no. Most debugger commands should not go near locks anyway unless they are intended to carefully modify the existing system in a safe manner (such as the 'kill' command which should only be using try locks and fail if it cannot safely post the signal). -- John Baldwin ___ freebsd-current@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/freebsd-current To unsubscribe, send any mail to "freebsd-current-unsubscr...@freebsd.org"
Re: Stop scheduler on panic
On Fri, Dec 2, 2011 at 2:05 AM, Andriy Gapon wrote: > on 02/12/2011 06:36 John Baldwin said the following: >> Ah, ok (I had thought SCHEDULER_STOPPED was going to always be true when kdb >> was >> active). But I think these two changes should cover critical_exit() ok. >> > > I attempted to start a discussion about this a few times already :-) > Should we treat kdb context the same as SCHEDULER_STOPPED context (in the > current definition) ? That is, skip all locks in the same fashion? > There are pros and contras. Does kdb pause all CPUs with an interrupt (NMI or regular interrupt, I can no longer remember...) when it enters? If so, then I'd say whether it enters via sysctl or panic doesn't matter. It's in a special environment where nothing else is running, which is what is needed for proper exploration of the machine (via breakpoint, for debugging a hang, etc). Maybe the question is, why wouldn't SCHEDULER_STOPPED be true regardless of how kdb is entered? Thanks, matthew ___ freebsd-current@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/freebsd-current To unsubscribe, send any mail to "freebsd-current-unsubscr...@freebsd.org"
Re: Stop scheduler on panic
on 02/12/2011 06:36 John Baldwin said the following: > Ah, ok (I had thought SCHEDULER_STOPPED was going to always be true when kdb > was > active). But I think these two changes should cover critical_exit() ok. > I attempted to start a discussion about this a few times already :-) Should we treat kdb context the same as SCHEDULER_STOPPED context (in the current definition) ? That is, skip all locks in the same fashion? There are pros and contras. -- Andriy Gapon ___ freebsd-current@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/freebsd-current To unsubscribe, send any mail to "freebsd-current-unsubscr...@freebsd.org"
Re: Stop scheduler on panic
On 12/1/11 4:42 PM, Andriy Gapon wrote:
on 01/12/2011 22:53 John Baldwin said the following:
On Thursday, December 01, 2011 3:42:24 pm Andriy Gapon wrote:
Returning to critical_exit, what do you think about the following patch?
I guess that it could be committed independently of / before the
SCHEDULER_STOPPED thing.
commit ee3d1a04985e86911a68d854439ae8c5429b7bd5
Author: Andriy Gapon
Date: Thu Dec 1 18:53:36 2011 +0200
critical_exit: ignore td_owepreempt if kdb_active
calling mi_switch in such a context result in a recursion via
kdb_switch
diff --git a/sys/kern/kern_switch.c b/sys/kern/kern_switch.c
index 93cbf7b..885dc22 100644
--- a/sys/kern/kern_switch.c
+++ b/sys/kern/kern_switch.c
@@ -200,7 +200,7 @@ critical_exit(void)
if (td->td_critnest == 1) {
td->td_critnest = 0;
- if (td->td_owepreempt) {
+ if (td->td_owepreempt&& !kdb_active) {
td->td_critnest = 1;
thread_lock(td);
td->td_critnest--;
I think this is fine, but I'd probably change this to SCHEDULER_STOPPED()
in the SCHEDULER_STOPPED() patch.
I don't understand why... What if kdb is entered for some other reason, not
because of panic? In that case SCHEDULER_STOPPED() would be false, but it is
still possible to find a way into mi_switch.
The SCHEDULER_STOPPED patch adds this:
@@ -428,6 +428,8 @@ mi_switch(int flags, struct thread *newtd)
*/
if (kdb_active)
kdb_switch();
+ if (SCHEDULER_STOPPED())
+ return;
if (flags& SW_VOL) {
td->td_ru.ru_nvcsw++;
td->td_swvoltick = ticks;
Ah, ok (I had thought SCHEDULER_STOPPED was going to always be true when
kdb was active). But I think these two changes should cover
critical_exit() ok.
--
John Baldwin
___
freebsd-current@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-current
To unsubscribe, send any mail to "freebsd-current-unsubscr...@freebsd.org"
Re: Stop scheduler on panic
on 01/12/2011 22:53 John Baldwin said the following:
> On Thursday, December 01, 2011 3:42:24 pm Andriy Gapon wrote:
>> Returning to critical_exit, what do you think about the following patch?
>> I guess that it could be committed independently of / before the
>> SCHEDULER_STOPPED thing.
>>
>> commit ee3d1a04985e86911a68d854439ae8c5429b7bd5
>> Author: Andriy Gapon
>> Date: Thu Dec 1 18:53:36 2011 +0200
>>
>> critical_exit: ignore td_owepreempt if kdb_active
>>
>> calling mi_switch in such a context result in a recursion via
>> kdb_switch
>>
>> diff --git a/sys/kern/kern_switch.c b/sys/kern/kern_switch.c
>> index 93cbf7b..885dc22 100644
>> --- a/sys/kern/kern_switch.c
>> +++ b/sys/kern/kern_switch.c
>> @@ -200,7 +200,7 @@ critical_exit(void)
>>
>> if (td->td_critnest == 1) {
>> td->td_critnest = 0;
>> -if (td->td_owepreempt) {
>> +if (td->td_owepreempt && !kdb_active) {
>> td->td_critnest = 1;
>> thread_lock(td);
>> td->td_critnest--;
>
> I think this is fine, but I'd probably change this to SCHEDULER_STOPPED()
> in the SCHEDULER_STOPPED() patch.
I don't understand why... What if kdb is entered for some other reason, not
because of panic? In that case SCHEDULER_STOPPED() would be false, but it is
still possible to find a way into mi_switch.
The SCHEDULER_STOPPED patch adds this:
@@ -428,6 +428,8 @@ mi_switch(int flags, struct thread *newtd)
*/
if (kdb_active)
kdb_switch();
+ if (SCHEDULER_STOPPED())
+ return;
if (flags & SW_VOL) {
td->td_ru.ru_nvcsw++;
td->td_swvoltick = ticks;
--
Andriy Gapon
___
freebsd-current@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-current
To unsubscribe, send any mail to "freebsd-current-unsubscr...@freebsd.org"
Re: Stop scheduler on panic
On Thursday, December 01, 2011 3:42:24 pm Andriy Gapon wrote:
> on 01/12/2011 20:49 John Baldwin said the following:
> > On Thursday, December 01, 2011 11:59:10 am Andriy Gapon wrote:
> >>
> >> [cc list trimmed]
> >>
> >> on 21/11/2011 18:32 John Baldwin said the following:
> >>> On Friday, November 18, 2011 4:59:32 pm Andriy Gapon wrote:
> on 17/11/2011 23:38 John Baldwin said the following:
> > On Thursday, November 17, 2011 4:35:07 pm John Baldwin wrote:
> >> Hmmm, you could also make critical_exit() not perform deferred
> >> preemptions
> >> if SCHEDULER_STOPPED? That would fix the recursion and still let the
> >> preemption "work" when resuming from the debugger?
> >>
> >>
> >> Just to clarify, probably you are actually suggesting to not perform
> >> deferred
> >> preemptions if kdb_active == TRUE. Because that's where we get the
> >> recursion (via
> >> kdb_switch).
> >>
> >> I think that if we get into the mi_switch in a state where !kdb_active &&
> >> SCHEDULER_STOPPED(), then we probably should just - I don't know - panic
> >> again?
> >>
> >> [the following is preserved for context]
> >
> > Hmmm. I'd be tempted to just ignore pending preemptions anytime
> > SCHEDULER_STOPPED() is true. If it's stopped for a reason other than being
> > in the debugger (e.g. panic), I'd rather make a best effort at getting a
> > dump
> > than panic again.
>
> Yep, me too. It's just that I assumed that ending up at mi_switch in the
> panic
> thread/context meant that something had gone very wrong already. But I am not
> sure if this was a valid assumption.
>
> Returning to critical_exit, what do you think about the following patch?
> I guess that it could be committed independently of / before the
> SCHEDULER_STOPPED thing.
>
> commit ee3d1a04985e86911a68d854439ae8c5429b7bd5
> Author: Andriy Gapon
> Date: Thu Dec 1 18:53:36 2011 +0200
>
> critical_exit: ignore td_owepreempt if kdb_active
>
> calling mi_switch in such a context result in a recursion via
> kdb_switch
>
> diff --git a/sys/kern/kern_switch.c b/sys/kern/kern_switch.c
> index 93cbf7b..885dc22 100644
> --- a/sys/kern/kern_switch.c
> +++ b/sys/kern/kern_switch.c
> @@ -200,7 +200,7 @@ critical_exit(void)
>
> if (td->td_critnest == 1) {
> td->td_critnest = 0;
> - if (td->td_owepreempt) {
> + if (td->td_owepreempt && !kdb_active) {
> td->td_critnest = 1;
> thread_lock(td);
> td->td_critnest--;
I think this is fine, but I'd probably change this to SCHEDULER_STOPPED()
in the SCHEDULER_STOPPED() patch.
> Would it make sense wrap kdb_active check with __predict_false?
I don't think so.
--
John Baldwin
___
freebsd-current@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-current
To unsubscribe, send any mail to "freebsd-current-unsubscr...@freebsd.org"
Re: Stop scheduler on panic
on 01/12/2011 20:49 John Baldwin said the following:
> On Thursday, December 01, 2011 11:59:10 am Andriy Gapon wrote:
>>
>> [cc list trimmed]
>>
>> on 21/11/2011 18:32 John Baldwin said the following:
>>> On Friday, November 18, 2011 4:59:32 pm Andriy Gapon wrote:
on 17/11/2011 23:38 John Baldwin said the following:
> On Thursday, November 17, 2011 4:35:07 pm John Baldwin wrote:
>> Hmmm, you could also make critical_exit() not perform deferred
>> preemptions
>> if SCHEDULER_STOPPED? That would fix the recursion and still let the
>> preemption "work" when resuming from the debugger?
>>
>>
>> Just to clarify, probably you are actually suggesting to not perform deferred
>> preemptions if kdb_active == TRUE. Because that's where we get the
>> recursion (via
>> kdb_switch).
>>
>> I think that if we get into the mi_switch in a state where !kdb_active &&
>> SCHEDULER_STOPPED(), then we probably should just - I don't know - panic
>> again?
>>
>> [the following is preserved for context]
>
> Hmmm. I'd be tempted to just ignore pending preemptions anytime
> SCHEDULER_STOPPED() is true. If it's stopped for a reason other than being
> in the debugger (e.g. panic), I'd rather make a best effort at getting a dump
> than panic again.
Yep, me too. It's just that I assumed that ending up at mi_switch in the panic
thread/context meant that something had gone very wrong already. But I am not
sure if this was a valid assumption.
Returning to critical_exit, what do you think about the following patch?
I guess that it could be committed independently of / before the
SCHEDULER_STOPPED thing.
commit ee3d1a04985e86911a68d854439ae8c5429b7bd5
Author: Andriy Gapon
Date: Thu Dec 1 18:53:36 2011 +0200
critical_exit: ignore td_owepreempt if kdb_active
calling mi_switch in such a context result in a recursion via
kdb_switch
diff --git a/sys/kern/kern_switch.c b/sys/kern/kern_switch.c
index 93cbf7b..885dc22 100644
--- a/sys/kern/kern_switch.c
+++ b/sys/kern/kern_switch.c
@@ -200,7 +200,7 @@ critical_exit(void)
if (td->td_critnest == 1) {
td->td_critnest = 0;
- if (td->td_owepreempt) {
+ if (td->td_owepreempt && !kdb_active) {
td->td_critnest = 1;
thread_lock(td);
td->td_critnest--;
Would it make sense wrap kdb_active check with __predict_false?
--
Andriy Gapon
___
freebsd-current@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-current
To unsubscribe, send any mail to "freebsd-current-unsubscr...@freebsd.org"
Re: Stop scheduler on panic
On Thursday, December 01, 2011 11:59:10 am Andriy Gapon wrote: > > [cc list trimmed] > > on 21/11/2011 18:32 John Baldwin said the following: > > On Friday, November 18, 2011 4:59:32 pm Andriy Gapon wrote: > >> on 17/11/2011 23:38 John Baldwin said the following: > >>> On Thursday, November 17, 2011 4:35:07 pm John Baldwin wrote: > Hmmm, you could also make critical_exit() not perform deferred > preemptions > if SCHEDULER_STOPPED? That would fix the recursion and still let the > preemption "work" when resuming from the debugger? > > > Just to clarify, probably you are actually suggesting to not perform deferred > preemptions if kdb_active == TRUE. Because that's where we get the recursion > (via > kdb_switch). > > I think that if we get into the mi_switch in a state where !kdb_active && > SCHEDULER_STOPPED(), then we probably should just - I don't know - panic > again? > > [the following is preserved for context] Hmmm. I'd be tempted to just ignore pending preemptions anytime SCHEDULER_STOPPED() is true. If it's stopped for a reason other than being in the debugger (e.g. panic), I'd rather make a best effort at getting a dump than panic again. -- John Baldwin ___ freebsd-current@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/freebsd-current To unsubscribe, send any mail to "freebsd-current-unsubscr...@freebsd.org"
Re: Stop scheduler on panic
[cc list trimmed] on 21/11/2011 18:32 John Baldwin said the following: > On Friday, November 18, 2011 4:59:32 pm Andriy Gapon wrote: >> on 17/11/2011 23:38 John Baldwin said the following: >>> On Thursday, November 17, 2011 4:35:07 pm John Baldwin wrote: Hmmm, you could also make critical_exit() not perform deferred preemptions if SCHEDULER_STOPPED? That would fix the recursion and still let the preemption "work" when resuming from the debugger? Just to clarify, probably you are actually suggesting to not perform deferred preemptions if kdb_active == TRUE. Because that's where we get the recursion (via kdb_switch). I think that if we get into the mi_switch in a state where !kdb_active && SCHEDULER_STOPPED(), then we probably should just - I don't know - panic again? [the following is preserved for context] >> Yes, that's a good solution, I think. I just didn't want to touch such a >> "low >> level" code, but I guess there is no rational reason for that. >> >>> Or you could let the debugger run in a permament critical section (though >>> perhaps that breaks too many other things like debugger routines that try >>> to use locks like the 'kill' command (which is useful!)). Effectively what >>> you >>> are trying to do is having the debugger run in a critical section until the >>> debugger is exited. It would be cleanest to let it run that way explicitly >>> if possible since then you don't have to catch as many edge cases. >> >> I like this idea, but likely it would take some effort to get done. > > Yes, it would take some effort, so checking SCHEDULER_STOPPED in > critical_exit() is probably good for the short term. Would be nice to fix > it properly some day. > >> Related to this is something that I attempted to discuss before. I think >> that >> because the debugger acts on a frozen system image (the debugger is a sole >> actor >> and observer), the debugger should try to minimize its interaction with the >> debugged system. In this vein I think that the debugger should also bypass >> any >> locks just like with SCHEDULER_STOPPED. The debugger should also be careful >> to >> note a state of any subsystems that it uses (e.g. the keyboard) and return >> them >> to the initial state if it had to be changed. But that's a bit different >> story. >> And I really get beyond my knowledge zone when I try to think about things >> like >> handling 'call func_' in the debugger where func_ may want to acquire >> some locks or noticeably change some state within a system. > > I think to some extent, I think if a user calls a function from the debugger > they better know what they are doing. However, I think it can also be useful > to have the debugger modify the system in some cases if it can safely do so > (e.g. the 'kill' command from DDB can be very useful, and IIRC, it is careful > to only use try locks and just fail if it can't acquire the needed locks). > >> But to continue about the locks... I have this idea to re-implement >> SCHEDULER_STOPPED as some more general check that could be abstractly >> denoted as >> LOCKING_POLICY_CHECK(context). Where the context could be defined by flags >> like >> normal, in-panic, in-debugger, etc. And the locking policies could be: >> normal, >> bypass, warn, panic, etc. >> >> However, I am not sure if this could be useful (and doable properly) in >> practice. I am just concerned with the interaction between the debugger and >> the >> locks. It still seems to me inconsistent that we are going with >> SCHEDULER_STOPPED for panic, but we are continuing to use "if (!kdb_active)" >> around some locks that could be problematic under kdb (e.g. in USB). In my >> opinion the amount of code shared between normal context and kdb context is >> about the same as amount of code shared between normal context and panic >> context. But I haven't really quantified those. > > I think you need to keep the 'kill' case in mind. In that case you don't want > to ignore locks, but the code is carefully written to use try locks instead > (or > should be). > -- Andriy Gapon ___ freebsd-current@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/freebsd-current To unsubscribe, send any mail to "freebsd-current-unsubscr...@freebsd.org"
Re: Stop scheduler on panic
2011/11/21 John Baldwin : > On Friday, November 18, 2011 4:59:32 pm Andriy Gapon wrote: >> on 17/11/2011 23:38 John Baldwin said the following: >> > On Thursday, November 17, 2011 4:35:07 pm John Baldwin wrote: >> >> Hmmm, you could also make critical_exit() not perform deferred preemptions >> >> if SCHEDULER_STOPPED? That would fix the recursion and still let the >> >> preemption "work" when resuming from the debugger? >> >> Yes, that's a good solution, I think. I just didn't want to touch such a >> "low >> level" code, but I guess there is no rational reason for that. >> >> > Or you could let the debugger run in a permament critical section (though >> > perhaps that breaks too many other things like debugger routines that try >> > to use locks like the 'kill' command (which is useful!)). Effectively >> > what you >> > are trying to do is having the debugger run in a critical section until the >> > debugger is exited. It would be cleanest to let it run that way explicitly >> > if possible since then you don't have to catch as many edge cases. >> >> I like this idea, but likely it would take some effort to get done. > > Yes, it would take some effort, so checking SCHEDULER_STOPPED in > critical_exit() is probably good for the short term. Would be nice to fix > it properly some day. > >> Related to this is something that I attempted to discuss before. I think >> that >> because the debugger acts on a frozen system image (the debugger is a sole >> actor >> and observer), the debugger should try to minimize its interaction with the >> debugged system. In this vein I think that the debugger should also bypass >> any >> locks just like with SCHEDULER_STOPPED. The debugger should also be careful >> to >> note a state of any subsystems that it uses (e.g. the keyboard) and return >> them >> to the initial state if it had to be changed. But that's a bit different >> story. >> And I really get beyond my knowledge zone when I try to think about things >> like >> handling 'call func_' in the debugger where func_ may want to acquire >> some locks or noticeably change some state within a system. > > I think to some extent, I think if a user calls a function from the debugger > they better know what they are doing. However, I think it can also be useful > to have the debugger modify the system in some cases if it can safely do so > (e.g. the 'kill' command from DDB can be very useful, and IIRC, it is careful > to only use try locks and just fail if it can't acquire the needed locks). I would be very in favor about having a 'thread trampoline for KDB', thus that it can use locks. The only downside is that it assume an healthy state of the switching infrastructure, so maybe it would be fine to wrapper this under a proper compile-time option (KDB_LITE -> it will run in interrupt context and the users will better know what they do). Attilio -- Peace can only be achieved by understanding - A. Einstein ___ freebsd-current@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/freebsd-current To unsubscribe, send any mail to "freebsd-current-unsubscr...@freebsd.org"
Re: Stop scheduler on panic
On Friday, November 18, 2011 4:59:32 pm Andriy Gapon wrote: > on 17/11/2011 23:38 John Baldwin said the following: > > On Thursday, November 17, 2011 4:35:07 pm John Baldwin wrote: > >> Hmmm, you could also make critical_exit() not perform deferred preemptions > >> if SCHEDULER_STOPPED? That would fix the recursion and still let the > >> preemption "work" when resuming from the debugger? > > Yes, that's a good solution, I think. I just didn't want to touch such a "low > level" code, but I guess there is no rational reason for that. > > > Or you could let the debugger run in a permament critical section (though > > perhaps that breaks too many other things like debugger routines that try > > to use locks like the 'kill' command (which is useful!)). Effectively what > > you > > are trying to do is having the debugger run in a critical section until the > > debugger is exited. It would be cleanest to let it run that way explicitly > > if possible since then you don't have to catch as many edge cases. > > I like this idea, but likely it would take some effort to get done. Yes, it would take some effort, so checking SCHEDULER_STOPPED in critical_exit() is probably good for the short term. Would be nice to fix it properly some day. > Related to this is something that I attempted to discuss before. I think that > because the debugger acts on a frozen system image (the debugger is a sole > actor > and observer), the debugger should try to minimize its interaction with the > debugged system. In this vein I think that the debugger should also bypass > any > locks just like with SCHEDULER_STOPPED. The debugger should also be careful > to > note a state of any subsystems that it uses (e.g. the keyboard) and return > them > to the initial state if it had to be changed. But that's a bit different > story. > And I really get beyond my knowledge zone when I try to think about things > like > handling 'call func_' in the debugger where func_ may want to acquire > some locks or noticeably change some state within a system. I think to some extent, I think if a user calls a function from the debugger they better know what they are doing. However, I think it can also be useful to have the debugger modify the system in some cases if it can safely do so (e.g. the 'kill' command from DDB can be very useful, and IIRC, it is careful to only use try locks and just fail if it can't acquire the needed locks). > But to continue about the locks... I have this idea to re-implement > SCHEDULER_STOPPED as some more general check that could be abstractly denoted > as > LOCKING_POLICY_CHECK(context). Where the context could be defined by flags > like > normal, in-panic, in-debugger, etc. And the locking policies could be: > normal, > bypass, warn, panic, etc. > > However, I am not sure if this could be useful (and doable properly) in > practice. I am just concerned with the interaction between the debugger and > the > locks. It still seems to me inconsistent that we are going with > SCHEDULER_STOPPED for panic, but we are continuing to use "if (!kdb_active)" > around some locks that could be problematic under kdb (e.g. in USB). In my > opinion the amount of code shared between normal context and kdb context is > about the same as amount of code shared between normal context and panic > context. But I haven't really quantified those. I think you need to keep the 'kill' case in mind. In that case you don't want to ignore locks, but the code is carefully written to use try locks instead (or should be). -- John Baldwin ___ freebsd-current@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/freebsd-current To unsubscribe, send any mail to "freebsd-current-unsubscr...@freebsd.org"
Re: Stop scheduler on panic
on 17/11/2011 23:38 John Baldwin said the following: > On Thursday, November 17, 2011 4:35:07 pm John Baldwin wrote: >> Hmmm, you could also make critical_exit() not perform deferred preemptions >> if SCHEDULER_STOPPED? That would fix the recursion and still let the >> preemption "work" when resuming from the debugger? Yes, that's a good solution, I think. I just didn't want to touch such a "low level" code, but I guess there is no rational reason for that. > Or you could let the debugger run in a permament critical section (though > perhaps that breaks too many other things like debugger routines that try > to use locks like the 'kill' command (which is useful!)). Effectively what > you > are trying to do is having the debugger run in a critical section until the > debugger is exited. It would be cleanest to let it run that way explicitly > if possible since then you don't have to catch as many edge cases. I like this idea, but likely it would take some effort to get done. Related to this is something that I attempted to discuss before. I think that because the debugger acts on a frozen system image (the debugger is a sole actor and observer), the debugger should try to minimize its interaction with the debugged system. In this vein I think that the debugger should also bypass any locks just like with SCHEDULER_STOPPED. The debugger should also be careful to note a state of any subsystems that it uses (e.g. the keyboard) and return them to the initial state if it had to be changed. But that's a bit different story. And I really get beyond my knowledge zone when I try to think about things like handling 'call func_' in the debugger where func_ may want to acquire some locks or noticeably change some state within a system. But to continue about the locks... I have this idea to re-implement SCHEDULER_STOPPED as some more general check that could be abstractly denoted as LOCKING_POLICY_CHECK(context). Where the context could be defined by flags like normal, in-panic, in-debugger, etc. And the locking policies could be: normal, bypass, warn, panic, etc. However, I am not sure if this could be useful (and doable properly) in practice. I am just concerned with the interaction between the debugger and the locks. It still seems to me inconsistent that we are going with SCHEDULER_STOPPED for panic, but we are continuing to use "if (!kdb_active)" around some locks that could be problematic under kdb (e.g. in USB). In my opinion the amount of code shared between normal context and kdb context is about the same as amount of code shared between normal context and panic context. But I haven't really quantified those. -- Andriy Gapon ___ freebsd-current@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/freebsd-current To unsubscribe, send any mail to "freebsd-current-unsubscr...@freebsd.org"
Re: Stop scheduler on panic
Alexander Motin wrote: > On 11/17/11 13:14, Kostik Belousov wrote: > > On Thu, Nov 17, 2011 at 11:29:02AM +0200, Alexander Motin wrote: > >> On 11/17/11 11:06, Kostik Belousov wrote: > >>> On Thu, Nov 17, 2011 at 10:40:58AM +0200, Alexander Motin wrote: > On 11/17/11 10:15, Kostik Belousov wrote: > > On Thu, Nov 17, 2011 at 01:07:38AM +0200, Alexander Motin wrote: > >> On 17.11.2011 00:21, Andriy Gapon wrote: > >>> on 16/11/2011 21:27 Fabian Keil said the following: > Kostik Belousov wrote: > > > I was tricked into finishing the work by Andrey Gapon, who developed > > the patch to reliably stop other processors on panic. The patch > > greatly improves the chances of getting dump on panic on SMP host. > > I tested the patch trying to get a dump (from the debugger) for > kern/162036, which currently results in the double fault reported in: > http://lists.freebsd.org/pipermail/freebsd-current/2011-September/027766.html > > It didn't help, but also didn't make anything worse. > >>> The mi_switch recursion looks very familiar to me: > >>> mi_switch() at mi_switch+0x270 > >>> critical_exit() at critical_exit+0x9b > >>> spinlock_exit() at spinlock_exit+0x17 > >>> mi_switch() at mi_switch+0x275 > >>> critical_exit() at critical_exit+0x9b > >>> spinlock_exit() at spinlock_exit+0x17 > >>> [several pages of the previous three lines skipped] > >>> mi_switch() at mi_switch+0x275 > >>> critical_exit() at critical_exit+0x9b > >>> spinlock_exit() at spinlock_exit+0x17 > >>> intr_even_schedule_thread() at intr_event_schedule_thread+0xbb > >>> ahci_end_transaction() at ahci_end_transaction+0x398 > >>> ahci_ch_intr() at ahci_ch_intr+0x2b5 > >>> ahcipoll() at ahcipoll+0x15 > >>> xpt_polled_action() at xpt_polled_action+0xf7 > >>> > >>> In fact I once discussed with jhb this recursion triggered from a > >>> different > >>> place. To quote myself: > >>> spinlock_exit -> critical_exit -> mi_switch -> kdb_switch > >>> -> > >>> thread_unlock -> spinlock_exit -> critical_exit -> mi_switch -> > >>> ... > >>> in the kdb context > >>> this issue seems to be triggered by td_owepreempt being true > >>> at > >>> the time > >>> kdb is entered > >>> and there of course has to be an initial spinlock_exit call > >>> somewhere > >>> in my case it's because of usb keyboard > >>> I wonder if it would make sense to clear td_owepreempt right > >>> before > >>> calling kdb_switch in mi_switch > >>> instead of in sched_switch() > >>> clearing td_owepreempt seems like a scheduler-independent > >>> operation to me > >>> or is it better to just skip locking in usb when kdb_active > >>> is set > >>> ? > >>> > >>> The workaround described above should work in this case. > >>> Another possibility is to pessimize mtx_unlock_spin() implementations > >>> to > >>> check > >>> SCHEDULER_STOPPED() and to bypass any further actions in that case. > >>> But > >>> that > >>> would add unnecessary overhead to the sunny day code paths. > >>> > >>> Going further up the stack one can come up with the following > >>> proposals: > >>> - check SCHEDULER_STOPPED() swi_sched() and return early > >>> - do not call swi_sched() from xpt_done() if we somehow know that we > >>> are > >>> in a > >>> polling mode > >> > >> There is no flag in CAM now to indicate polling mode, but if needed, > >> it > >> should not be difficult to add one and not call swi_sched(). > > > > I have the following change for eons on my test boxes. Without it, > > I simply cannot get _any_ dump. > That should be OK for kernel dumping. I was thinking about CAM abusing > polling not only for dumping. But looking on cases where it does it now, > may be it is better to rewrite them instead. > >>> > >>> So, should I interpret your response as 'Reviewed by" ? > >> > >> It feels somehow dirty to me. I don't like these global variables. If > >> you consider it is fine, proceed, I see no much harm. But if not, I can > >> add polling flag to the CAM. Flip a coin for me. :) > > You promised to add the polling at summer' meeting in Kiev. Will you do > > it now ? > > Sorry, I've probably forgot. The patch is attached. After rebasing on r227637 dumping core from the debugger works and the backtrace is at least partly usable. PR updated. Thanks a lot. Fabian signature.asc Description: PGP signature
Re: Stop scheduler on panic
On Thursday, November 17, 2011 4:35:07 pm John Baldwin wrote:
> On Thursday, November 17, 2011 2:42:51 pm Andriy Gapon wrote:
> > on 17/11/2011 21:09 John Baldwin said the following:
> > > On Thursday, November 17, 2011 11:58:03 am Andriy Gapon wrote:
> > >> on 17/11/2011 18:37 John Baldwin said the following:
> > >>> On Thursday, November 17, 2011 4:47:42 am Andriy Gapon wrote:
> > on 17/11/2011 10:34 Andriy Gapon said the following:
> > > on 17/11/2011 10:15 Kostik Belousov said the following:
> > >> I have the following change for eons on my test boxes. Without it,
> > >> I simply cannot get _any_ dump.
> > >>
> > >> diff --git a/sys/cam/cam_xpt.c b/sys/cam/cam_xpt.c
> > >> index 10b89c7..a38e42f 100644
> > >> --- a/sys/cam/cam_xpt.c
> > >> +++ b/sys/cam/cam_xpt.c
> > >> @@ -4230,7 +4230,7 @@ xpt_done(union ccb *done_ccb)
> > >> TAILQ_INSERT_TAIL(&cam_simq, sim, links);
> > >> mtx_unlock(&cam_simq_lock);
> > >> sim->flags |= CAM_SIM_ON_DONEQ;
> > >> -if (first)
> > >> +if (first && panicstr == NULL)
> > >> swi_sched(cambio_ih, 0);
> > >> }
> > >> }
> > >
> > > I think that this (or similar) change should go into the patch and
> > > the tree.
> > >
> >
> > And, BTW, I still would like to do something like the following
> > (perhaps with
> > td_oncpu = NOCPU and td_flags &= ~TDF_NEEDRESCHED also moved to the
> > common code):
> >
> > Index: sys/kern/sched_ule.c
> > ===
> > --- sys/kern/sched_ule.c (revision 227608)
> > +++ sys/kern/sched_ule.c (working copy)
> > @@ -1790,7 +1790,6 @@ sched_switch(struct thread *td, struct thread
> > *new
> > td->td_oncpu = NOCPU;
> > if (!(flags & SW_PREEMPT))
> > td->td_flags &= ~TDF_NEEDRESCHED;
> > - td->td_owepreempt = 0;
> > tdq->tdq_switchcnt++;
> > /*
> > * The lock pointer in an idle thread should never change.
> > Reset it
> > Index: sys/kern/kern_synch.c
> > ===
> > --- sys/kern/kern_synch.c (revision 227608)
> > +++ sys/kern/kern_synch.c (working copy)
> > @@ -406,6 +406,8 @@ mi_switch(int flags, struct thread *newtd)
> > ("mi_switch: switch must be voluntary or involuntary"));
> > KASSERT(newtd != curthread, ("mi_switch: preempting back to
> > ourself"));
> >
> > + td->td_owepreempt = 0;
> > +
> > /*
> > * Don't perform context switches from the debugger.
> > */
> > Index: sys/kern/sched_4bsd.c
> > ===
> > --- sys/kern/sched_4bsd.c (revision 227608)
> > +++ sys/kern/sched_4bsd.c (working copy)
> > @@ -940,7 +940,6 @@ sched_switch(struct thread *td, struct thread *new
> > td->td_lastcpu = td->td_oncpu;
> > if (!(flags & SW_PREEMPT))
> > td->td_flags &= ~TDF_NEEDRESCHED;
> > - td->td_owepreempt = 0;
> > td->td_oncpu = NOCPU;
> >
> > /*
> >
> > Does anybody see any potential problems with such a change?
> > >>>
> > >>> Hmm, does this mean the preemption will be lost if you break into the
> > >>> debugger and continue in the non-panic case?
> > >>
> > >> Not sure which exact scenario you have in mind.
> > >> Please note that the above diff just moves resetting of td_owepreempt to
> > >> an
> > >> earlier place. As far as I can see there are no checks of td_owepreempt
> > >> value
> > >> between the new place and the old places.
> > >
> > > I'm worried that you are clearing td_owepreempt even in cases where a
> > > context
> > > switch is not performed. So say you enter DDB with td_owepreempt set and
> > > that
> > > DDB bails on a context switch. With your change it will now clear
> > > td_owepreempt
> > > and "lose" the preemption.
> > >
> >
> > And without the change we get the recursion and double-fault because of
> > kdb_switch -> thread_unlock -> spinlock_exit -> critical_exit ->
> > mi_switch in this case ?
> >
> > BTW, it is my opinion that we really should not let the debugger code call
> > mi_switch for any reason.
>
> Hmmm, you could also make critical_exit() not perform deferred preemptions
> if SCHEDULER_STOPPED? That would fix the recursion and still let the
> preemption "work" when resuming from the debugger?
Or you could let the debugger run in a permament critical section (though
perhaps that breaks too many other things like debugger routines that try
to use locks like the 'kill' command
Re: Stop scheduler on panic
On Thursday, November 17, 2011 2:42:51 pm Andriy Gapon wrote:
> on 17/11/2011 21:09 John Baldwin said the following:
> > On Thursday, November 17, 2011 11:58:03 am Andriy Gapon wrote:
> >> on 17/11/2011 18:37 John Baldwin said the following:
> >>> On Thursday, November 17, 2011 4:47:42 am Andriy Gapon wrote:
> on 17/11/2011 10:34 Andriy Gapon said the following:
> > on 17/11/2011 10:15 Kostik Belousov said the following:
> >> I have the following change for eons on my test boxes. Without it,
> >> I simply cannot get _any_ dump.
> >>
> >> diff --git a/sys/cam/cam_xpt.c b/sys/cam/cam_xpt.c
> >> index 10b89c7..a38e42f 100644
> >> --- a/sys/cam/cam_xpt.c
> >> +++ b/sys/cam/cam_xpt.c
> >> @@ -4230,7 +4230,7 @@ xpt_done(union ccb *done_ccb)
> >>TAILQ_INSERT_TAIL(&cam_simq, sim, links);
> >>mtx_unlock(&cam_simq_lock);
> >>sim->flags |= CAM_SIM_ON_DONEQ;
> >> - if (first)
> >> + if (first && panicstr == NULL)
> >>swi_sched(cambio_ih, 0);
> >>}
> >>}
> >
> > I think that this (or similar) change should go into the patch and the
> > tree.
> >
>
> And, BTW, I still would like to do something like the following (perhaps
> with
> td_oncpu = NOCPU and td_flags &= ~TDF_NEEDRESCHED also moved to the
> common code):
>
> Index: sys/kern/sched_ule.c
> ===
> --- sys/kern/sched_ule.c (revision 227608)
> +++ sys/kern/sched_ule.c (working copy)
> @@ -1790,7 +1790,6 @@ sched_switch(struct thread *td, struct thread *new
> td->td_oncpu = NOCPU;
> if (!(flags & SW_PREEMPT))
> td->td_flags &= ~TDF_NEEDRESCHED;
> -td->td_owepreempt = 0;
> tdq->tdq_switchcnt++;
> /*
> * The lock pointer in an idle thread should never change.
> Reset it
> Index: sys/kern/kern_synch.c
> ===
> --- sys/kern/kern_synch.c(revision 227608)
> +++ sys/kern/kern_synch.c(working copy)
> @@ -406,6 +406,8 @@ mi_switch(int flags, struct thread *newtd)
> ("mi_switch: switch must be voluntary or involuntary"));
> KASSERT(newtd != curthread, ("mi_switch: preempting back to
> ourself"));
>
> +td->td_owepreempt = 0;
> +
> /*
> * Don't perform context switches from the debugger.
> */
> Index: sys/kern/sched_4bsd.c
> ===
> --- sys/kern/sched_4bsd.c(revision 227608)
> +++ sys/kern/sched_4bsd.c(working copy)
> @@ -940,7 +940,6 @@ sched_switch(struct thread *td, struct thread *new
> td->td_lastcpu = td->td_oncpu;
> if (!(flags & SW_PREEMPT))
> td->td_flags &= ~TDF_NEEDRESCHED;
> -td->td_owepreempt = 0;
> td->td_oncpu = NOCPU;
>
> /*
>
> Does anybody see any potential problems with such a change?
> >>>
> >>> Hmm, does this mean the preemption will be lost if you break into the
> >>> debugger and continue in the non-panic case?
> >>
> >> Not sure which exact scenario you have in mind.
> >> Please note that the above diff just moves resetting of td_owepreempt to an
> >> earlier place. As far as I can see there are no checks of td_owepreempt
> >> value
> >> between the new place and the old places.
> >
> > I'm worried that you are clearing td_owepreempt even in cases where a
> > context
> > switch is not performed. So say you enter DDB with td_owepreempt set and
> > that
> > DDB bails on a context switch. With your change it will now clear
> > td_owepreempt
> > and "lose" the preemption.
> >
>
> And without the change we get the recursion and double-fault because of
> kdb_switch -> thread_unlock -> spinlock_exit -> critical_exit ->
> mi_switch in this case ?
>
> BTW, it is my opinion that we really should not let the debugger code call
> mi_switch for any reason.
Hmmm, you could also make critical_exit() not perform deferred preemptions
if SCHEDULER_STOPPED? That would fix the recursion and still let the
preemption "work" when resuming from the debugger?
--
John Baldwin
___
freebsd-current@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-current
To unsubscribe, send any mail to "freebsd-current-unsubscr...@freebsd.org"
Re: Stop scheduler on panic
On Thu, Nov 17, 2011 at 1:08 PM, Andriy Gapon wrote:
> on 17/11/2011 23:02 m...@freebsd.org said the following:
>> Another patch related to this area we have at $WORK:
>>
>> #if defined(SMP)
>> - /*
>> - * Bind us to CPU 0 so that all shutdown code runs there. Some
>> - * systems don't shutdown properly (i.e., ACPI power off) if we
>> - * run on another processor.
>> - */
>> - thread_lock(curthread);
>> - sched_bind(curthread, 0);
>> - thread_unlock(curthread);
>> - KASSERT(PCPU_GET(cpuid) == 0, ("%s: not running on cpu 0",
>> __func__));
>> + /*
>> + * sched_bind can't be done reliably inside of panic. cpu_reset()
>> will
>> + * rebind us in any case, more reliably.
>> + */
>> + if (panicstr == NULL) {
>> + /*
>> + * Bind us to CPU 0 so that all shutdown code runs there.
>> Some
>> + * systems don't shutdown properly (i.e., ACPI power off) if
>> we
>> + * run on another processor.
>> + */
>> + thread_lock(curthread);
>> + sched_bind(curthread, 0);
>> + thread_unlock(curthread);
>> + KASSERT(PCPU_GET(cpuid) == 0, ("boot: not running on cpu
>> 0"));
>> + }
>> #endif
>> /* We're in the process of rebooting. */
>> rebooting = 1;
>
> Yes, you have contributed this patch before and it is a part of the bigger
> stop-scheduler-on-panic patches. Have you had a chance to review those? :)
It's been so long I don't remember what I've sent where. I did look
over the patch but had no additional comments.
Cheers,
matthew
___
freebsd-current@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-current
To unsubscribe, send any mail to "freebsd-current-unsubscr...@freebsd.org"
Re: Stop scheduler on panic
on 17/11/2011 23:02 m...@freebsd.org said the following:
> Another patch related to this area we have at $WORK:
>
> #if defined(SMP)
> - /*
> -* Bind us to CPU 0 so that all shutdown code runs there. Some
> -* systems don't shutdown properly (i.e., ACPI power off) if we
> -* run on another processor.
> -*/
> - thread_lock(curthread);
> - sched_bind(curthread, 0);
> - thread_unlock(curthread);
> - KASSERT(PCPU_GET(cpuid) == 0, ("%s: not running on cpu 0", __func__));
> + /*
> +* sched_bind can't be done reliably inside of panic. cpu_reset()
> will
> +* rebind us in any case, more reliably.
> +*/
> + if (panicstr == NULL) {
> + /*
> +* Bind us to CPU 0 so that all shutdown code runs there.
> Some
> +* systems don't shutdown properly (i.e., ACPI power off) if
> we
> +* run on another processor.
> +*/
> + thread_lock(curthread);
> + sched_bind(curthread, 0);
> + thread_unlock(curthread);
> + KASSERT(PCPU_GET(cpuid) == 0, ("boot: not running on cpu 0"));
> + }
> #endif
> /* We're in the process of rebooting. */
> rebooting = 1;
Yes, you have contributed this patch before and it is a part of the bigger
stop-scheduler-on-panic patches. Have you had a chance to review those? :)
--
Andriy Gapon
___
freebsd-current@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-current
To unsubscribe, send any mail to "freebsd-current-unsubscr...@freebsd.org"
Re: Stop scheduler on panic
2011/11/17 :
> On Thu, Nov 17, 2011 at 12:54 PM, Attilio Rao wrote:
>> 2011/11/17 Andriy Gapon :
>>> BTW, it is my opinion that we really should not let the debugger code call
>>> mi_switch for any reason.
>>
>> Yes, I agree with this, this is why the sched_bind() in boot() is
>> broken (immagine calling things like doadump from KDB. KDB right now
>> can be thought as a first cut of this patch because it does disable
>> the CPUs when entering the context, thus, the bug here is that if you
>> stop all CPUs including CPU0 and later on you want bind on it you are
>> death).
>
> Another patch related to this area we have at $WORK:
>
> #if defined(SMP)
> - /*
> - * Bind us to CPU 0 so that all shutdown code runs there. Some
> - * systems don't shutdown properly (i.e., ACPI power off) if we
> - * run on another processor.
> - */
> - thread_lock(curthread);
> - sched_bind(curthread, 0);
> - thread_unlock(curthread);
> - KASSERT(PCPU_GET(cpuid) == 0, ("%s: not running on cpu 0", __func__));
> + /*
> + * sched_bind can't be done reliably inside of panic. cpu_reset()
> will
> + * rebind us in any case, more reliably.
> + */
> + if (panicstr == NULL) {
> + /*
> + * Bind us to CPU 0 so that all shutdown code runs there.
> Some
> + * systems don't shutdown properly (i.e., ACPI power off) if
> we
> + * run on another processor.
> + */
> + thread_lock(curthread);
> + sched_bind(curthread, 0);
> + thread_unlock(curthread);
> + KASSERT(PCPU_GET(cpuid) == 0, ("boot: not running on cpu 0"));
> + }
> #endif
> /* We're in the process of rebooting. */
> rebooting = 1;
This doesn't cover the KDB case which is the most broken here.
(I'm a bit unsure about the name of functions and I cannot check now,
but in short):
- you enter KDB via debug.kdb.enter=1 (for example)
- kdb_enter() stop CPUs and if it is on CPU1 it stops CPU0
- you call functions entering boot() from KDB prompt (IIRC "call
doadump" should do it)
- boot() wants to bind on CPU0 which is turned off
This case only take care of panic, which is not enough.
Attilio
--
Peace can only be achieved by understanding - A. Einstein
___
freebsd-current@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-current
To unsubscribe, send any mail to "freebsd-current-unsubscr...@freebsd.org"
Re: Stop scheduler on panic
On Thu, Nov 17, 2011 at 12:54 PM, Attilio Rao wrote:
> 2011/11/17 Andriy Gapon :
>> BTW, it is my opinion that we really should not let the debugger code call
>> mi_switch for any reason.
>
> Yes, I agree with this, this is why the sched_bind() in boot() is
> broken (immagine calling things like doadump from KDB. KDB right now
> can be thought as a first cut of this patch because it does disable
> the CPUs when entering the context, thus, the bug here is that if you
> stop all CPUs including CPU0 and later on you want bind on it you are
> death).
Another patch related to this area we have at $WORK:
#if defined(SMP)
- /*
-* Bind us to CPU 0 so that all shutdown code runs there. Some
-* systems don't shutdown properly (i.e., ACPI power off) if we
-* run on another processor.
-*/
- thread_lock(curthread);
- sched_bind(curthread, 0);
- thread_unlock(curthread);
- KASSERT(PCPU_GET(cpuid) == 0, ("%s: not running on cpu 0", __func__));
+ /*
+* sched_bind can't be done reliably inside of panic. cpu_reset() will
+* rebind us in any case, more reliably.
+*/
+ if (panicstr == NULL) {
+ /*
+* Bind us to CPU 0 so that all shutdown code runs there. Some
+* systems don't shutdown properly (i.e., ACPI power off) if we
+* run on another processor.
+*/
+ thread_lock(curthread);
+ sched_bind(curthread, 0);
+ thread_unlock(curthread);
+ KASSERT(PCPU_GET(cpuid) == 0, ("boot: not running on cpu 0"));
+ }
#endif
/* We're in the process of rebooting. */
rebooting = 1;
Cheers,
matthew
___
freebsd-current@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-current
To unsubscribe, send any mail to "freebsd-current-unsubscr...@freebsd.org"
Re: Stop scheduler on panic
2011/11/17 Andriy Gapon :
> on 17/11/2011 21:09 John Baldwin said the following:
>> On Thursday, November 17, 2011 11:58:03 am Andriy Gapon wrote:
>>> on 17/11/2011 18:37 John Baldwin said the following:
On Thursday, November 17, 2011 4:47:42 am Andriy Gapon wrote:
> on 17/11/2011 10:34 Andriy Gapon said the following:
>> on 17/11/2011 10:15 Kostik Belousov said the following:
>>> I have the following change for eons on my test boxes. Without it,
>>> I simply cannot get _any_ dump.
>>>
>>> diff --git a/sys/cam/cam_xpt.c b/sys/cam/cam_xpt.c
>>> index 10b89c7..a38e42f 100644
>>> --- a/sys/cam/cam_xpt.c
>>> +++ b/sys/cam/cam_xpt.c
>>> @@ -4230,7 +4230,7 @@ xpt_done(union ccb *done_ccb)
>>> TAILQ_INSERT_TAIL(&cam_simq, sim, links);
>>> mtx_unlock(&cam_simq_lock);
>>> sim->flags |= CAM_SIM_ON_DONEQ;
>>> - if (first)
>>> + if (first && panicstr == NULL)
>>> swi_sched(cambio_ih, 0);
>>> }
>>> }
>>
>> I think that this (or similar) change should go into the patch and the
>> tree.
>>
>
> And, BTW, I still would like to do something like the following (perhaps
> with
> td_oncpu = NOCPU and td_flags &= ~TDF_NEEDRESCHED also moved to the
> common code):
>
> Index: sys/kern/sched_ule.c
> ===
> --- sys/kern/sched_ule.c (revision 227608)
> +++ sys/kern/sched_ule.c (working copy)
> @@ -1790,7 +1790,6 @@ sched_switch(struct thread *td, struct thread *new
> td->td_oncpu = NOCPU;
> if (!(flags & SW_PREEMPT))
> td->td_flags &= ~TDF_NEEDRESCHED;
> - td->td_owepreempt = 0;
> tdq->tdq_switchcnt++;
> /*
> * The lock pointer in an idle thread should never change. Reset it
> Index: sys/kern/kern_synch.c
> ===
> --- sys/kern/kern_synch.c (revision 227608)
> +++ sys/kern/kern_synch.c (working copy)
> @@ -406,6 +406,8 @@ mi_switch(int flags, struct thread *newtd)
> ("mi_switch: switch must be voluntary or involuntary"));
> KASSERT(newtd != curthread, ("mi_switch: preempting back to ourself"));
>
> + td->td_owepreempt = 0;
> +
> /*
> * Don't perform context switches from the debugger.
> */
> Index: sys/kern/sched_4bsd.c
> ===
> --- sys/kern/sched_4bsd.c (revision 227608)
> +++ sys/kern/sched_4bsd.c (working copy)
> @@ -940,7 +940,6 @@ sched_switch(struct thread *td, struct thread *new
> td->td_lastcpu = td->td_oncpu;
> if (!(flags & SW_PREEMPT))
> td->td_flags &= ~TDF_NEEDRESCHED;
> - td->td_owepreempt = 0;
> td->td_oncpu = NOCPU;
>
> /*
>
> Does anybody see any potential problems with such a change?
Hmm, does this mean the preemption will be lost if you break into the
debugger and continue in the non-panic case?
>>>
>>> Not sure which exact scenario you have in mind.
>>> Please note that the above diff just moves resetting of td_owepreempt to an
>>> earlier place. As far as I can see there are no checks of td_owepreempt
>>> value
>>> between the new place and the old places.
>>
>> I'm worried that you are clearing td_owepreempt even in cases where a context
>> switch is not performed. So say you enter DDB with td_owepreempt set and
>> that
>> DDB bails on a context switch. With your change it will now clear
>> td_owepreempt
>> and "lose" the preemption.
>>
>
> And without the change we get the recursion and double-fault because of
> kdb_switch -> thread_unlock -> spinlock_exit -> critical_exit ->
> mi_switch in this case ?
>
> BTW, it is my opinion that we really should not let the debugger code call
> mi_switch for any reason.
Yes, I agree with this, this is why the sched_bind() in boot() is
broken (immagine calling things like doadump from KDB. KDB right now
can be thought as a first cut of this patch because it does disable
the CPUs when entering the context, thus, the bug here is that if you
stop all CPUs including CPU0 and later on you want bind on it you are
death).
We need to discuss this and the patch more extensively, I'm taking my
time and hopefully will do a full review during the weekend.
Attilio
--
Peace can only be achieved by understanding - A. Einstein
___
freebsd-current@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-current
To unsubscribe, send any mail to "freebsd-current-unsubscr...@freebsd.org"
Re: Stop scheduler on panic
on 17/11/2011 21:09 John Baldwin said the following:
> On Thursday, November 17, 2011 11:58:03 am Andriy Gapon wrote:
>> on 17/11/2011 18:37 John Baldwin said the following:
>>> On Thursday, November 17, 2011 4:47:42 am Andriy Gapon wrote:
on 17/11/2011 10:34 Andriy Gapon said the following:
> on 17/11/2011 10:15 Kostik Belousov said the following:
>> I have the following change for eons on my test boxes. Without it,
>> I simply cannot get _any_ dump.
>>
>> diff --git a/sys/cam/cam_xpt.c b/sys/cam/cam_xpt.c
>> index 10b89c7..a38e42f 100644
>> --- a/sys/cam/cam_xpt.c
>> +++ b/sys/cam/cam_xpt.c
>> @@ -4230,7 +4230,7 @@ xpt_done(union ccb *done_ccb)
>> TAILQ_INSERT_TAIL(&cam_simq, sim, links);
>> mtx_unlock(&cam_simq_lock);
>> sim->flags |= CAM_SIM_ON_DONEQ;
>> -if (first)
>> +if (first && panicstr == NULL)
>> swi_sched(cambio_ih, 0);
>> }
>> }
>
> I think that this (or similar) change should go into the patch and the
> tree.
>
And, BTW, I still would like to do something like the following (perhaps
with
td_oncpu = NOCPU and td_flags &= ~TDF_NEEDRESCHED also moved to the common
code):
Index: sys/kern/sched_ule.c
===
--- sys/kern/sched_ule.c (revision 227608)
+++ sys/kern/sched_ule.c (working copy)
@@ -1790,7 +1790,6 @@ sched_switch(struct thread *td, struct thread *new
td->td_oncpu = NOCPU;
if (!(flags & SW_PREEMPT))
td->td_flags &= ~TDF_NEEDRESCHED;
- td->td_owepreempt = 0;
tdq->tdq_switchcnt++;
/*
* The lock pointer in an idle thread should never change. Reset it
Index: sys/kern/kern_synch.c
===
--- sys/kern/kern_synch.c (revision 227608)
+++ sys/kern/kern_synch.c (working copy)
@@ -406,6 +406,8 @@ mi_switch(int flags, struct thread *newtd)
("mi_switch: switch must be voluntary or involuntary"));
KASSERT(newtd != curthread, ("mi_switch: preempting back to ourself"));
+ td->td_owepreempt = 0;
+
/*
* Don't perform context switches from the debugger.
*/
Index: sys/kern/sched_4bsd.c
===
--- sys/kern/sched_4bsd.c (revision 227608)
+++ sys/kern/sched_4bsd.c (working copy)
@@ -940,7 +940,6 @@ sched_switch(struct thread *td, struct thread *new
td->td_lastcpu = td->td_oncpu;
if (!(flags & SW_PREEMPT))
td->td_flags &= ~TDF_NEEDRESCHED;
- td->td_owepreempt = 0;
td->td_oncpu = NOCPU;
/*
Does anybody see any potential problems with such a change?
>>>
>>> Hmm, does this mean the preemption will be lost if you break into the
>>> debugger and continue in the non-panic case?
>>
>> Not sure which exact scenario you have in mind.
>> Please note that the above diff just moves resetting of td_owepreempt to an
>> earlier place. As far as I can see there are no checks of td_owepreempt
>> value
>> between the new place and the old places.
>
> I'm worried that you are clearing td_owepreempt even in cases where a context
> switch is not performed. So say you enter DDB with td_owepreempt set and that
> DDB bails on a context switch. With your change it will now clear
> td_owepreempt
> and "lose" the preemption.
>
And without the change we get the recursion and double-fault because of
kdb_switch -> thread_unlock -> spinlock_exit -> critical_exit ->
mi_switch in this case ?
BTW, it is my opinion that we really should not let the debugger code call
mi_switch for any reason.
--
Andriy Gapon
___
freebsd-current@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-current
To unsubscribe, send any mail to "freebsd-current-unsubscr...@freebsd.org"
Re: Stop scheduler on panic
On Thursday, November 17, 2011 11:58:03 am Andriy Gapon wrote:
> on 17/11/2011 18:37 John Baldwin said the following:
> > On Thursday, November 17, 2011 4:47:42 am Andriy Gapon wrote:
> >> on 17/11/2011 10:34 Andriy Gapon said the following:
> >>> on 17/11/2011 10:15 Kostik Belousov said the following:
> I have the following change for eons on my test boxes. Without it,
> I simply cannot get _any_ dump.
>
> diff --git a/sys/cam/cam_xpt.c b/sys/cam/cam_xpt.c
> index 10b89c7..a38e42f 100644
> --- a/sys/cam/cam_xpt.c
> +++ b/sys/cam/cam_xpt.c
> @@ -4230,7 +4230,7 @@ xpt_done(union ccb *done_ccb)
> TAILQ_INSERT_TAIL(&cam_simq, sim, links);
> mtx_unlock(&cam_simq_lock);
> sim->flags |= CAM_SIM_ON_DONEQ;
> -if (first)
> +if (first && panicstr == NULL)
> swi_sched(cambio_ih, 0);
> }
> }
> >>>
> >>> I think that this (or similar) change should go into the patch and the
> >>> tree.
> >>>
> >>
> >> And, BTW, I still would like to do something like the following (perhaps
> >> with
> >> td_oncpu = NOCPU and td_flags &= ~TDF_NEEDRESCHED also moved to the common
> >> code):
> >>
> >> Index: sys/kern/sched_ule.c
> >> ===
> >> --- sys/kern/sched_ule.c (revision 227608)
> >> +++ sys/kern/sched_ule.c (working copy)
> >> @@ -1790,7 +1790,6 @@ sched_switch(struct thread *td, struct thread *new
> >>td->td_oncpu = NOCPU;
> >>if (!(flags & SW_PREEMPT))
> >>td->td_flags &= ~TDF_NEEDRESCHED;
> >> - td->td_owepreempt = 0;
> >>tdq->tdq_switchcnt++;
> >>/*
> >> * The lock pointer in an idle thread should never change. Reset it
> >> Index: sys/kern/kern_synch.c
> >> ===
> >> --- sys/kern/kern_synch.c (revision 227608)
> >> +++ sys/kern/kern_synch.c (working copy)
> >> @@ -406,6 +406,8 @@ mi_switch(int flags, struct thread *newtd)
> >>("mi_switch: switch must be voluntary or involuntary"));
> >>KASSERT(newtd != curthread, ("mi_switch: preempting back to ourself"));
> >>
> >> + td->td_owepreempt = 0;
> >> +
> >>/*
> >> * Don't perform context switches from the debugger.
> >> */
> >> Index: sys/kern/sched_4bsd.c
> >> ===
> >> --- sys/kern/sched_4bsd.c (revision 227608)
> >> +++ sys/kern/sched_4bsd.c (working copy)
> >> @@ -940,7 +940,6 @@ sched_switch(struct thread *td, struct thread *new
> >>td->td_lastcpu = td->td_oncpu;
> >>if (!(flags & SW_PREEMPT))
> >>td->td_flags &= ~TDF_NEEDRESCHED;
> >> - td->td_owepreempt = 0;
> >>td->td_oncpu = NOCPU;
> >>
> >>/*
> >>
> >> Does anybody see any potential problems with such a change?
> >
> > Hmm, does this mean the preemption will be lost if you break into the
> > debugger and continue in the non-panic case?
>
> Not sure which exact scenario you have in mind.
> Please note that the above diff just moves resetting of td_owepreempt to an
> earlier place. As far as I can see there are no checks of td_owepreempt value
> between the new place and the old places.
I'm worried that you are clearing td_owepreempt even in cases where a context
switch is not performed. So say you enter DDB with td_owepreempt set and that
DDB bails on a context switch. With your change it will now clear td_owepreempt
and "lose" the preemption.
--
John Baldwin
___
freebsd-current@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-current
To unsubscribe, send any mail to "freebsd-current-unsubscr...@freebsd.org"
Re: Stop scheduler on panic
On Thu, Nov 17, 2011 at 02:40:45PM +0200, Alexander Motin wrote: > On 11/17/11 13:14, Kostik Belousov wrote: > > On Thu, Nov 17, 2011 at 11:29:02AM +0200, Alexander Motin wrote: > >> On 11/17/11 11:06, Kostik Belousov wrote: > >>> On Thu, Nov 17, 2011 at 10:40:58AM +0200, Alexander Motin wrote: > On 11/17/11 10:15, Kostik Belousov wrote: > > On Thu, Nov 17, 2011 at 01:07:38AM +0200, Alexander Motin wrote: > >> On 17.11.2011 00:21, Andriy Gapon wrote: > >>> on 16/11/2011 21:27 Fabian Keil said the following: > Kostik Belousov wrote: > > > I was tricked into finishing the work by Andrey Gapon, who developed > > the patch to reliably stop other processors on panic. The patch > > greatly improves the chances of getting dump on panic on SMP host. > > I tested the patch trying to get a dump (from the debugger) for > kern/162036, which currently results in the double fault reported in: > http://lists.freebsd.org/pipermail/freebsd-current/2011-September/027766.html > > It didn't help, but also didn't make anything worse. > > Fabian > >>> > >>> The mi_switch recursion looks very familiar to me: > >>> mi_switch() at mi_switch+0x270 > >>> critical_exit() at critical_exit+0x9b > >>> spinlock_exit() at spinlock_exit+0x17 > >>> mi_switch() at mi_switch+0x275 > >>> critical_exit() at critical_exit+0x9b > >>> spinlock_exit() at spinlock_exit+0x17 > >>> [several pages of the previous three lines skipped] > >>> mi_switch() at mi_switch+0x275 > >>> critical_exit() at critical_exit+0x9b > >>> spinlock_exit() at spinlock_exit+0x17 > >>> intr_even_schedule_thread() at intr_event_schedule_thread+0xbb > >>> ahci_end_transaction() at ahci_end_transaction+0x398 > >>> ahci_ch_intr() at ahci_ch_intr+0x2b5 > >>> ahcipoll() at ahcipoll+0x15 > >>> xpt_polled_action() at xpt_polled_action+0xf7 > >>> > >>> In fact I once discussed with jhb this recursion triggered from a > >>> different > >>> place. To quote myself: > >>> spinlock_exit -> critical_exit -> mi_switch -> kdb_switch > >>> -> > >>> thread_unlock -> spinlock_exit -> critical_exit -> mi_switch -> > >>> ... > >>> in the kdb context > >>> this issue seems to be triggered by td_owepreempt being true > >>> at > >>> the time > >>> kdb is entered > >>> and there of course has to be an initial spinlock_exit call > >>> somewhere > >>> in my case it's because of usb keyboard > >>> I wonder if it would make sense to clear td_owepreempt right > >>> before > >>> calling kdb_switch in mi_switch > >>> instead of in sched_switch() > >>> clearing td_owepreempt seems like a scheduler-independent > >>> operation to me > >>> or is it better to just skip locking in usb when kdb_active > >>> is set > >>> ? > >>> > >>> The workaround described above should work in this case. > >>> Another possibility is to pessimize mtx_unlock_spin() implementations > >>> to > >>> check > >>> SCHEDULER_STOPPED() and to bypass any further actions in that case. > >>> But > >>> that > >>> would add unnecessary overhead to the sunny day code paths. > >>> > >>> Going further up the stack one can come up with the following > >>> proposals: > >>> - check SCHEDULER_STOPPED() swi_sched() and return early > >>> - do not call swi_sched() from xpt_done() if we somehow know that we > >>> are > >>> in a > >>> polling mode > >> > >> There is no flag in CAM now to indicate polling mode, but if needed, > >> it > >> should not be difficult to add one and not call swi_sched(). > > > > I have the following change for eons on my test boxes. Without it, > > I simply cannot get _any_ dump. > > > > diff --git a/sys/cam/cam_xpt.c b/sys/cam/cam_xpt.c > > index 10b89c7..a38e42f 100644 > > --- a/sys/cam/cam_xpt.c > > +++ b/sys/cam/cam_xpt.c > > @@ -4230,7 +4230,7 @@ xpt_done(union ccb *done_ccb) > > TAILQ_INSERT_TAIL(&cam_simq, sim, links); > > mtx_unlock(&cam_simq_lock); > > sim->flags |= CAM_SIM_ON_DONEQ; > > - if (first) > > + if (first && panicstr == NULL) > > swi_sched(cambio_ih, 0); > > } > > } > > That should be OK for kernel dumping. I was thinking about CAM abusing > polling not only for dumping. But looking on cases where it does it now, > may be it is better to rewrite them instead. > >>> > >>> So, should I interpret your response as 'Reviewed by" ? > >> > >> It feels somehow dirty to me. I don't like these global variables. If > >> yo
Re: Stop scheduler on panic
On Nov 13, 2011, at 3:32 AM, Kostik Belousov wrote: > I was tricked into finishing the work by Andrey Gapon, who developed > the patch to reliably stop other processors on panic. The patch > greatly improves the chances of getting dump on panic on SMP host. > Several people already saw the patchset, and I remember that Andrey > posted it to some lists. > > The change stops other (*) processors early upon the panic. This way, > no parallel manipulation of the kernel memory is performed by CPUs. > In particular, the kernel memory map is static. Patch prevents the > panic thread from blocking and switching out. > > * - in the context of the description, other means not current. > > Since other threads are not run anymore, lock owner cannot release a > lock which is required by panic thread. Due to this, we need to fake > a lock acquisition after the panic, which adds minimal overhead to the > locking cost. The patch tries to not add any overhead on the fast path > of the lock acquire. The check for the after-panic condition was > reduced to single memory access, done only when the quick cas lock > attempt failed, and braced with __unlikely compiler hint. > > For now, the new mode of operation is disabled by default, since some > further USB changes are needed to make USB keyboard usable in that > environment. > > With the patch, getting a dump from the machine without debugger > compiled in is much more realistic. Please comment, I will commit the > change in 2 weeks unless strong reasons not to are given. > > http://people.freebsd.org/~kib/misc/stop_cpus_on_panic.1.patch > We have many systems running Andriy's latest version of the patch under 8.2. I also brought in the related USB patch; without it, the system hangs up while dumping almost every time. With both patches in place* it has worked flawlessly for us. -Andrew * - with one change: always do the critical_enter() / critical_exit(). Using spinlock_enter() blocks the software watchdog, which needs to still be active in case the dump hangs for other reasons. This is obviously not ideal but the best solution I have right now. We also stop all of the network interfaces at the beginning of boot(). -- Andrew Boyerabo...@averesystems.com ___ freebsd-current@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/freebsd-current To unsubscribe, send any mail to "freebsd-current-unsubscr...@freebsd.org"
Re: Stop scheduler on panic
on 17/11/2011 18:37 John Baldwin said the following:
> On Thursday, November 17, 2011 4:47:42 am Andriy Gapon wrote:
>> on 17/11/2011 10:34 Andriy Gapon said the following:
>>> on 17/11/2011 10:15 Kostik Belousov said the following:
I have the following change for eons on my test boxes. Without it,
I simply cannot get _any_ dump.
diff --git a/sys/cam/cam_xpt.c b/sys/cam/cam_xpt.c
index 10b89c7..a38e42f 100644
--- a/sys/cam/cam_xpt.c
+++ b/sys/cam/cam_xpt.c
@@ -4230,7 +4230,7 @@ xpt_done(union ccb *done_ccb)
TAILQ_INSERT_TAIL(&cam_simq, sim, links);
mtx_unlock(&cam_simq_lock);
sim->flags |= CAM_SIM_ON_DONEQ;
- if (first)
+ if (first && panicstr == NULL)
swi_sched(cambio_ih, 0);
}
}
>>>
>>> I think that this (or similar) change should go into the patch and the tree.
>>>
>>
>> And, BTW, I still would like to do something like the following (perhaps with
>> td_oncpu = NOCPU and td_flags &= ~TDF_NEEDRESCHED also moved to the common
>> code):
>>
>> Index: sys/kern/sched_ule.c
>> ===
>> --- sys/kern/sched_ule.c (revision 227608)
>> +++ sys/kern/sched_ule.c (working copy)
>> @@ -1790,7 +1790,6 @@ sched_switch(struct thread *td, struct thread *new
>> td->td_oncpu = NOCPU;
>> if (!(flags & SW_PREEMPT))
>> td->td_flags &= ~TDF_NEEDRESCHED;
>> -td->td_owepreempt = 0;
>> tdq->tdq_switchcnt++;
>> /*
>> * The lock pointer in an idle thread should never change. Reset it
>> Index: sys/kern/kern_synch.c
>> ===
>> --- sys/kern/kern_synch.c(revision 227608)
>> +++ sys/kern/kern_synch.c(working copy)
>> @@ -406,6 +406,8 @@ mi_switch(int flags, struct thread *newtd)
>> ("mi_switch: switch must be voluntary or involuntary"));
>> KASSERT(newtd != curthread, ("mi_switch: preempting back to ourself"));
>>
>> +td->td_owepreempt = 0;
>> +
>> /*
>> * Don't perform context switches from the debugger.
>> */
>> Index: sys/kern/sched_4bsd.c
>> ===
>> --- sys/kern/sched_4bsd.c(revision 227608)
>> +++ sys/kern/sched_4bsd.c(working copy)
>> @@ -940,7 +940,6 @@ sched_switch(struct thread *td, struct thread *new
>> td->td_lastcpu = td->td_oncpu;
>> if (!(flags & SW_PREEMPT))
>> td->td_flags &= ~TDF_NEEDRESCHED;
>> -td->td_owepreempt = 0;
>> td->td_oncpu = NOCPU;
>>
>> /*
>>
>> Does anybody see any potential problems with such a change?
>
> Hmm, does this mean the preemption will be lost if you break into the
> debugger and continue in the non-panic case?
Not sure which exact scenario you have in mind.
Please note that the above diff just moves resetting of td_owepreempt to an
earlier place. As far as I can see there are no checks of td_owepreempt value
between the new place and the old places.
--
Andriy Gapon
___
freebsd-current@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-current
To unsubscribe, send any mail to "freebsd-current-unsubscr...@freebsd.org"
Re: Stop scheduler on panic
On Thursday, November 17, 2011 4:47:42 am Andriy Gapon wrote:
> on 17/11/2011 10:34 Andriy Gapon said the following:
> > on 17/11/2011 10:15 Kostik Belousov said the following:
> >> I have the following change for eons on my test boxes. Without it,
> >> I simply cannot get _any_ dump.
> >>
> >> diff --git a/sys/cam/cam_xpt.c b/sys/cam/cam_xpt.c
> >> index 10b89c7..a38e42f 100644
> >> --- a/sys/cam/cam_xpt.c
> >> +++ b/sys/cam/cam_xpt.c
> >> @@ -4230,7 +4230,7 @@ xpt_done(union ccb *done_ccb)
> >>TAILQ_INSERT_TAIL(&cam_simq, sim, links);
> >>mtx_unlock(&cam_simq_lock);
> >>sim->flags |= CAM_SIM_ON_DONEQ;
> >> - if (first)
> >> + if (first && panicstr == NULL)
> >>swi_sched(cambio_ih, 0);
> >>}
> >>}
> >
> > I think that this (or similar) change should go into the patch and the tree.
> >
>
> And, BTW, I still would like to do something like the following (perhaps with
> td_oncpu = NOCPU and td_flags &= ~TDF_NEEDRESCHED also moved to the common
> code):
>
> Index: sys/kern/sched_ule.c
> ===
> --- sys/kern/sched_ule.c (revision 227608)
> +++ sys/kern/sched_ule.c (working copy)
> @@ -1790,7 +1790,6 @@ sched_switch(struct thread *td, struct thread *new
> td->td_oncpu = NOCPU;
> if (!(flags & SW_PREEMPT))
> td->td_flags &= ~TDF_NEEDRESCHED;
> - td->td_owepreempt = 0;
> tdq->tdq_switchcnt++;
> /*
>* The lock pointer in an idle thread should never change. Reset it
> Index: sys/kern/kern_synch.c
> ===
> --- sys/kern/kern_synch.c (revision 227608)
> +++ sys/kern/kern_synch.c (working copy)
> @@ -406,6 +406,8 @@ mi_switch(int flags, struct thread *newtd)
> ("mi_switch: switch must be voluntary or involuntary"));
> KASSERT(newtd != curthread, ("mi_switch: preempting back to ourself"));
>
> + td->td_owepreempt = 0;
> +
> /*
>* Don't perform context switches from the debugger.
>*/
> Index: sys/kern/sched_4bsd.c
> ===
> --- sys/kern/sched_4bsd.c (revision 227608)
> +++ sys/kern/sched_4bsd.c (working copy)
> @@ -940,7 +940,6 @@ sched_switch(struct thread *td, struct thread *new
> td->td_lastcpu = td->td_oncpu;
> if (!(flags & SW_PREEMPT))
> td->td_flags &= ~TDF_NEEDRESCHED;
> - td->td_owepreempt = 0;
> td->td_oncpu = NOCPU;
>
> /*
>
> Does anybody see any potential problems with such a change?
Hmm, does this mean the preemption will be lost if you break into the
debugger and continue in the non-panic case?
--
John Baldwin
___
freebsd-current@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-current
To unsubscribe, send any mail to "freebsd-current-unsubscr...@freebsd.org"
Re: Stop scheduler on panic
On 11/17/11 13:14, Kostik Belousov wrote: > On Thu, Nov 17, 2011 at 11:29:02AM +0200, Alexander Motin wrote: >> On 11/17/11 11:06, Kostik Belousov wrote: >>> On Thu, Nov 17, 2011 at 10:40:58AM +0200, Alexander Motin wrote: On 11/17/11 10:15, Kostik Belousov wrote: > On Thu, Nov 17, 2011 at 01:07:38AM +0200, Alexander Motin wrote: >> On 17.11.2011 00:21, Andriy Gapon wrote: >>> on 16/11/2011 21:27 Fabian Keil said the following: Kostik Belousov wrote: > I was tricked into finishing the work by Andrey Gapon, who developed > the patch to reliably stop other processors on panic. The patch > greatly improves the chances of getting dump on panic on SMP host. I tested the patch trying to get a dump (from the debugger) for kern/162036, which currently results in the double fault reported in: http://lists.freebsd.org/pipermail/freebsd-current/2011-September/027766.html It didn't help, but also didn't make anything worse. Fabian >>> >>> The mi_switch recursion looks very familiar to me: >>> mi_switch() at mi_switch+0x270 >>> critical_exit() at critical_exit+0x9b >>> spinlock_exit() at spinlock_exit+0x17 >>> mi_switch() at mi_switch+0x275 >>> critical_exit() at critical_exit+0x9b >>> spinlock_exit() at spinlock_exit+0x17 >>> [several pages of the previous three lines skipped] >>> mi_switch() at mi_switch+0x275 >>> critical_exit() at critical_exit+0x9b >>> spinlock_exit() at spinlock_exit+0x17 >>> intr_even_schedule_thread() at intr_event_schedule_thread+0xbb >>> ahci_end_transaction() at ahci_end_transaction+0x398 >>> ahci_ch_intr() at ahci_ch_intr+0x2b5 >>> ahcipoll() at ahcipoll+0x15 >>> xpt_polled_action() at xpt_polled_action+0xf7 >>> >>> In fact I once discussed with jhb this recursion triggered from a >>> different >>> place. To quote myself: >>> spinlock_exit -> critical_exit -> mi_switch -> kdb_switch -> >>> thread_unlock -> spinlock_exit -> critical_exit -> mi_switch -> ... >>> in the kdb context >>> this issue seems to be triggered by td_owepreempt being true >>> at >>> the time >>> kdb is entered >>> and there of course has to be an initial spinlock_exit call >>> somewhere >>> in my case it's because of usb keyboard >>> I wonder if it would make sense to clear td_owepreempt right >>> before >>> calling kdb_switch in mi_switch >>> instead of in sched_switch() >>> clearing td_owepreempt seems like a scheduler-independent >>> operation to me >>> or is it better to just skip locking in usb when kdb_active is >>> set >>> ? >>> >>> The workaround described above should work in this case. >>> Another possibility is to pessimize mtx_unlock_spin() implementations >>> to >>> check >>> SCHEDULER_STOPPED() and to bypass any further actions in that case. >>> But >>> that >>> would add unnecessary overhead to the sunny day code paths. >>> >>> Going further up the stack one can come up with the following proposals: >>> - check SCHEDULER_STOPPED() swi_sched() and return early >>> - do not call swi_sched() from xpt_done() if we somehow know that we >>> are >>> in a >>> polling mode >> >> There is no flag in CAM now to indicate polling mode, but if needed, it >> should not be difficult to add one and not call swi_sched(). > > I have the following change for eons on my test boxes. Without it, > I simply cannot get _any_ dump. > > diff --git a/sys/cam/cam_xpt.c b/sys/cam/cam_xpt.c > index 10b89c7..a38e42f 100644 > --- a/sys/cam/cam_xpt.c > +++ b/sys/cam/cam_xpt.c > @@ -4230,7 +4230,7 @@ xpt_done(union ccb *done_ccb) > TAILQ_INSERT_TAIL(&cam_simq, sim, links); > mtx_unlock(&cam_simq_lock); > sim->flags |= CAM_SIM_ON_DONEQ; > - if (first) > + if (first && panicstr == NULL) > swi_sched(cambio_ih, 0); > } > } That should be OK for kernel dumping. I was thinking about CAM abusing polling not only for dumping. But looking on cases where it does it now, may be it is better to rewrite them instead. >>> >>> So, should I interpret your response as 'Reviewed by" ? >> >> It feels somehow dirty to me. I don't like these global variables. If >> you consider it is fine, proceed, I see no much harm. But if not, I can >> add polling flag to the CAM. Flip a coin for me. :) > You promised to add the polling at summer' meeting in Kiev. Will you do > it now ? Sorry, I've probably forgot. The patch is attached. -- Alexander Motin Index: cam_sim.h ===
Re: Stop scheduler on panic
On Thu, Nov 17, 2011 at 11:29:02AM +0200, Alexander Motin wrote: > On 11/17/11 11:06, Kostik Belousov wrote: > > On Thu, Nov 17, 2011 at 10:40:58AM +0200, Alexander Motin wrote: > >> On 11/17/11 10:15, Kostik Belousov wrote: > >>> On Thu, Nov 17, 2011 at 01:07:38AM +0200, Alexander Motin wrote: > On 17.11.2011 00:21, Andriy Gapon wrote: > > on 16/11/2011 21:27 Fabian Keil said the following: > >> Kostik Belousov wrote: > >> > >>> I was tricked into finishing the work by Andrey Gapon, who developed > >>> the patch to reliably stop other processors on panic. The patch > >>> greatly improves the chances of getting dump on panic on SMP host. > >> > >> I tested the patch trying to get a dump (from the debugger) for > >> kern/162036, which currently results in the double fault reported in: > >> http://lists.freebsd.org/pipermail/freebsd-current/2011-September/027766.html > >> > >> It didn't help, but also didn't make anything worse. > >> > >> Fabian > > > > The mi_switch recursion looks very familiar to me: > > mi_switch() at mi_switch+0x270 > > critical_exit() at critical_exit+0x9b > > spinlock_exit() at spinlock_exit+0x17 > > mi_switch() at mi_switch+0x275 > > critical_exit() at critical_exit+0x9b > > spinlock_exit() at spinlock_exit+0x17 > > [several pages of the previous three lines skipped] > > mi_switch() at mi_switch+0x275 > > critical_exit() at critical_exit+0x9b > > spinlock_exit() at spinlock_exit+0x17 > > intr_even_schedule_thread() at intr_event_schedule_thread+0xbb > > ahci_end_transaction() at ahci_end_transaction+0x398 > > ahci_ch_intr() at ahci_ch_intr+0x2b5 > > ahcipoll() at ahcipoll+0x15 > > xpt_polled_action() at xpt_polled_action+0xf7 > > > > In fact I once discussed with jhb this recursion triggered from a > > different > > place. To quote myself: > > spinlock_exit -> critical_exit -> mi_switch -> kdb_switch -> > > thread_unlock -> spinlock_exit -> critical_exit -> mi_switch -> ... > > in the kdb context > > this issue seems to be triggered by td_owepreempt being true > > at > > the time > > kdb is entered > > and there of course has to be an initial spinlock_exit call > > somewhere > > in my case it's because of usb keyboard > > I wonder if it would make sense to clear td_owepreempt right > > before > > calling kdb_switch in mi_switch > > instead of in sched_switch() > > clearing td_owepreempt seems like a scheduler-independent > > operation to me > > or is it better to just skip locking in usb when kdb_active is > > set > > ? > > > > The workaround described above should work in this case. > > Another possibility is to pessimize mtx_unlock_spin() implementations > > to > > check > > SCHEDULER_STOPPED() and to bypass any further actions in that case. > > But > > that > > would add unnecessary overhead to the sunny day code paths. > > > > Going further up the stack one can come up with the following proposals: > > - check SCHEDULER_STOPPED() swi_sched() and return early > > - do not call swi_sched() from xpt_done() if we somehow know that we > > are > > in a > > polling mode > > There is no flag in CAM now to indicate polling mode, but if needed, it > should not be difficult to add one and not call swi_sched(). > >>> > >>> I have the following change for eons on my test boxes. Without it, > >>> I simply cannot get _any_ dump. > >>> > >>> diff --git a/sys/cam/cam_xpt.c b/sys/cam/cam_xpt.c > >>> index 10b89c7..a38e42f 100644 > >>> --- a/sys/cam/cam_xpt.c > >>> +++ b/sys/cam/cam_xpt.c > >>> @@ -4230,7 +4230,7 @@ xpt_done(union ccb *done_ccb) > >>> TAILQ_INSERT_TAIL(&cam_simq, sim, links); > >>> mtx_unlock(&cam_simq_lock); > >>> sim->flags |= CAM_SIM_ON_DONEQ; > >>> - if (first) > >>> + if (first && panicstr == NULL) > >>> swi_sched(cambio_ih, 0); > >>> } > >>> } > >> > >> That should be OK for kernel dumping. I was thinking about CAM abusing > >> polling not only for dumping. But looking on cases where it does it now, > >> may be it is better to rewrite them instead. > > > > So, should I interpret your response as 'Reviewed by" ? > > It feels somehow dirty to me. I don't like these global variables. If > you consider it is fine, proceed, I see no much harm. But if not, I can > add polling flag to the CAM. Flip a coin for me. :) You promised to add the polling at summer' meeting in Kiev. Will you do it now ? pgpEdTAw2tcfa.pgp Description: PGP signature
Re: Stop scheduler on panic
on 17/11/2011 10:34 Andriy Gapon said the following:
> on 17/11/2011 10:15 Kostik Belousov said the following:
>> I have the following change for eons on my test boxes. Without it,
>> I simply cannot get _any_ dump.
>>
>> diff --git a/sys/cam/cam_xpt.c b/sys/cam/cam_xpt.c
>> index 10b89c7..a38e42f 100644
>> --- a/sys/cam/cam_xpt.c
>> +++ b/sys/cam/cam_xpt.c
>> @@ -4230,7 +4230,7 @@ xpt_done(union ccb *done_ccb)
>> TAILQ_INSERT_TAIL(&cam_simq, sim, links);
>> mtx_unlock(&cam_simq_lock);
>> sim->flags |= CAM_SIM_ON_DONEQ;
>> -if (first)
>> +if (first && panicstr == NULL)
>> swi_sched(cambio_ih, 0);
>> }
>> }
>
> I think that this (or similar) change should go into the patch and the tree.
>
And, BTW, I still would like to do something like the following (perhaps with
td_oncpu = NOCPU and td_flags &= ~TDF_NEEDRESCHED also moved to the common
code):
Index: sys/kern/sched_ule.c
===
--- sys/kern/sched_ule.c(revision 227608)
+++ sys/kern/sched_ule.c(working copy)
@@ -1790,7 +1790,6 @@ sched_switch(struct thread *td, struct thread *new
td->td_oncpu = NOCPU;
if (!(flags & SW_PREEMPT))
td->td_flags &= ~TDF_NEEDRESCHED;
- td->td_owepreempt = 0;
tdq->tdq_switchcnt++;
/*
* The lock pointer in an idle thread should never change. Reset it
Index: sys/kern/kern_synch.c
===
--- sys/kern/kern_synch.c (revision 227608)
+++ sys/kern/kern_synch.c (working copy)
@@ -406,6 +406,8 @@ mi_switch(int flags, struct thread *newtd)
("mi_switch: switch must be voluntary or involuntary"));
KASSERT(newtd != curthread, ("mi_switch: preempting back to ourself"));
+ td->td_owepreempt = 0;
+
/*
* Don't perform context switches from the debugger.
*/
Index: sys/kern/sched_4bsd.c
===
--- sys/kern/sched_4bsd.c (revision 227608)
+++ sys/kern/sched_4bsd.c (working copy)
@@ -940,7 +940,6 @@ sched_switch(struct thread *td, struct thread *new
td->td_lastcpu = td->td_oncpu;
if (!(flags & SW_PREEMPT))
td->td_flags &= ~TDF_NEEDRESCHED;
- td->td_owepreempt = 0;
td->td_oncpu = NOCPU;
/*
Does anybody see any potential problems with such a change?
--
Andriy Gapon
___
freebsd-current@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-current
To unsubscribe, send any mail to "freebsd-current-unsubscr...@freebsd.org"
Re: Stop scheduler on panic
On 11/17/11 11:06, Kostik Belousov wrote: > On Thu, Nov 17, 2011 at 10:40:58AM +0200, Alexander Motin wrote: >> On 11/17/11 10:15, Kostik Belousov wrote: >>> On Thu, Nov 17, 2011 at 01:07:38AM +0200, Alexander Motin wrote: On 17.11.2011 00:21, Andriy Gapon wrote: > on 16/11/2011 21:27 Fabian Keil said the following: >> Kostik Belousov wrote: >> >>> I was tricked into finishing the work by Andrey Gapon, who developed >>> the patch to reliably stop other processors on panic. The patch >>> greatly improves the chances of getting dump on panic on SMP host. >> >> I tested the patch trying to get a dump (from the debugger) for >> kern/162036, which currently results in the double fault reported in: >> http://lists.freebsd.org/pipermail/freebsd-current/2011-September/027766.html >> >> It didn't help, but also didn't make anything worse. >> >> Fabian > > The mi_switch recursion looks very familiar to me: > mi_switch() at mi_switch+0x270 > critical_exit() at critical_exit+0x9b > spinlock_exit() at spinlock_exit+0x17 > mi_switch() at mi_switch+0x275 > critical_exit() at critical_exit+0x9b > spinlock_exit() at spinlock_exit+0x17 > [several pages of the previous three lines skipped] > mi_switch() at mi_switch+0x275 > critical_exit() at critical_exit+0x9b > spinlock_exit() at spinlock_exit+0x17 > intr_even_schedule_thread() at intr_event_schedule_thread+0xbb > ahci_end_transaction() at ahci_end_transaction+0x398 > ahci_ch_intr() at ahci_ch_intr+0x2b5 > ahcipoll() at ahcipoll+0x15 > xpt_polled_action() at xpt_polled_action+0xf7 > > In fact I once discussed with jhb this recursion triggered from a > different > place. To quote myself: > spinlock_exit -> critical_exit -> mi_switch -> kdb_switch -> > thread_unlock -> spinlock_exit -> critical_exit -> mi_switch -> ... > in the kdb context > this issue seems to be triggered by td_owepreempt being true at > the time > kdb is entered > and there of course has to be an initial spinlock_exit call > somewhere > in my case it's because of usb keyboard > I wonder if it would make sense to clear td_owepreempt right > before > calling kdb_switch in mi_switch > instead of in sched_switch() > clearing td_owepreempt seems like a scheduler-independent > operation to me > or is it better to just skip locking in usb when kdb_active is > set > ? > > The workaround described above should work in this case. > Another possibility is to pessimize mtx_unlock_spin() implementations to > check > SCHEDULER_STOPPED() and to bypass any further actions in that case. But > that > would add unnecessary overhead to the sunny day code paths. > > Going further up the stack one can come up with the following proposals: > - check SCHEDULER_STOPPED() swi_sched() and return early > - do not call swi_sched() from xpt_done() if we somehow know that we are > in a > polling mode There is no flag in CAM now to indicate polling mode, but if needed, it should not be difficult to add one and not call swi_sched(). >>> >>> I have the following change for eons on my test boxes. Without it, >>> I simply cannot get _any_ dump. >>> >>> diff --git a/sys/cam/cam_xpt.c b/sys/cam/cam_xpt.c >>> index 10b89c7..a38e42f 100644 >>> --- a/sys/cam/cam_xpt.c >>> +++ b/sys/cam/cam_xpt.c >>> @@ -4230,7 +4230,7 @@ xpt_done(union ccb *done_ccb) >>> TAILQ_INSERT_TAIL(&cam_simq, sim, links); >>> mtx_unlock(&cam_simq_lock); >>> sim->flags |= CAM_SIM_ON_DONEQ; >>> - if (first) >>> + if (first && panicstr == NULL) >>> swi_sched(cambio_ih, 0); >>> } >>> } >> >> That should be OK for kernel dumping. I was thinking about CAM abusing >> polling not only for dumping. But looking on cases where it does it now, >> may be it is better to rewrite them instead. > > So, should I interpret your response as 'Reviewed by" ? It feels somehow dirty to me. I don't like these global variables. If you consider it is fine, proceed, I see no much harm. But if not, I can add polling flag to the CAM. Flip a coin for me. :) -- Alexander Motin ___ freebsd-current@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/freebsd-current To unsubscribe, send any mail to "freebsd-current-unsubscr...@freebsd.org"
Re: Stop scheduler on panic
On Thu, Nov 17, 2011 at 10:40:58AM +0200, Alexander Motin wrote: > On 11/17/11 10:15, Kostik Belousov wrote: > > On Thu, Nov 17, 2011 at 01:07:38AM +0200, Alexander Motin wrote: > >> On 17.11.2011 00:21, Andriy Gapon wrote: > >>> on 16/11/2011 21:27 Fabian Keil said the following: > Kostik Belousov wrote: > > > I was tricked into finishing the work by Andrey Gapon, who developed > > the patch to reliably stop other processors on panic. The patch > > greatly improves the chances of getting dump on panic on SMP host. > > I tested the patch trying to get a dump (from the debugger) for > kern/162036, which currently results in the double fault reported in: > http://lists.freebsd.org/pipermail/freebsd-current/2011-September/027766.html > > It didn't help, but also didn't make anything worse. > > Fabian > >>> > >>> The mi_switch recursion looks very familiar to me: > >>> mi_switch() at mi_switch+0x270 > >>> critical_exit() at critical_exit+0x9b > >>> spinlock_exit() at spinlock_exit+0x17 > >>> mi_switch() at mi_switch+0x275 > >>> critical_exit() at critical_exit+0x9b > >>> spinlock_exit() at spinlock_exit+0x17 > >>> [several pages of the previous three lines skipped] > >>> mi_switch() at mi_switch+0x275 > >>> critical_exit() at critical_exit+0x9b > >>> spinlock_exit() at spinlock_exit+0x17 > >>> intr_even_schedule_thread() at intr_event_schedule_thread+0xbb > >>> ahci_end_transaction() at ahci_end_transaction+0x398 > >>> ahci_ch_intr() at ahci_ch_intr+0x2b5 > >>> ahcipoll() at ahcipoll+0x15 > >>> xpt_polled_action() at xpt_polled_action+0xf7 > >>> > >>> In fact I once discussed with jhb this recursion triggered from a > >>> different > >>> place. To quote myself: > >>> spinlock_exit -> critical_exit -> mi_switch -> kdb_switch -> > >>> thread_unlock -> spinlock_exit -> critical_exit -> mi_switch -> ... > >>> in the kdb context > >>> this issue seems to be triggered by td_owepreempt being true at > >>> the time > >>> kdb is entered > >>> and there of course has to be an initial spinlock_exit call > >>> somewhere > >>> in my case it's because of usb keyboard > >>> I wonder if it would make sense to clear td_owepreempt right > >>> before > >>> calling kdb_switch in mi_switch > >>> instead of in sched_switch() > >>> clearing td_owepreempt seems like a scheduler-independent > >>> operation to me > >>> or is it better to just skip locking in usb when kdb_active is > >>> set > >>> ? > >>> > >>> The workaround described above should work in this case. > >>> Another possibility is to pessimize mtx_unlock_spin() implementations to > >>> check > >>> SCHEDULER_STOPPED() and to bypass any further actions in that case. But > >>> that > >>> would add unnecessary overhead to the sunny day code paths. > >>> > >>> Going further up the stack one can come up with the following proposals: > >>> - check SCHEDULER_STOPPED() swi_sched() and return early > >>> - do not call swi_sched() from xpt_done() if we somehow know that we are > >>> in a > >>> polling mode > >> > >> There is no flag in CAM now to indicate polling mode, but if needed, it > >> should not be difficult to add one and not call swi_sched(). > > > > I have the following change for eons on my test boxes. Without it, > > I simply cannot get _any_ dump. > > > > diff --git a/sys/cam/cam_xpt.c b/sys/cam/cam_xpt.c > > index 10b89c7..a38e42f 100644 > > --- a/sys/cam/cam_xpt.c > > +++ b/sys/cam/cam_xpt.c > > @@ -4230,7 +4230,7 @@ xpt_done(union ccb *done_ccb) > > TAILQ_INSERT_TAIL(&cam_simq, sim, links); > > mtx_unlock(&cam_simq_lock); > > sim->flags |= CAM_SIM_ON_DONEQ; > > - if (first) > > + if (first && panicstr == NULL) > > swi_sched(cambio_ih, 0); > > } > > } > > That should be OK for kernel dumping. I was thinking about CAM abusing > polling not only for dumping. But looking on cases where it does it now, > may be it is better to rewrite them instead. So, should I interpret your response as 'Reviewed by" ? pgplu0xBQRD1h.pgp Description: PGP signature
Re: Stop scheduler on panic
On 11/17/11 10:15, Kostik Belousov wrote: > On Thu, Nov 17, 2011 at 01:07:38AM +0200, Alexander Motin wrote: >> On 17.11.2011 00:21, Andriy Gapon wrote: >>> on 16/11/2011 21:27 Fabian Keil said the following: Kostik Belousov wrote: > I was tricked into finishing the work by Andrey Gapon, who developed > the patch to reliably stop other processors on panic. The patch > greatly improves the chances of getting dump on panic on SMP host. I tested the patch trying to get a dump (from the debugger) for kern/162036, which currently results in the double fault reported in: http://lists.freebsd.org/pipermail/freebsd-current/2011-September/027766.html It didn't help, but also didn't make anything worse. Fabian >>> >>> The mi_switch recursion looks very familiar to me: >>> mi_switch() at mi_switch+0x270 >>> critical_exit() at critical_exit+0x9b >>> spinlock_exit() at spinlock_exit+0x17 >>> mi_switch() at mi_switch+0x275 >>> critical_exit() at critical_exit+0x9b >>> spinlock_exit() at spinlock_exit+0x17 >>> [several pages of the previous three lines skipped] >>> mi_switch() at mi_switch+0x275 >>> critical_exit() at critical_exit+0x9b >>> spinlock_exit() at spinlock_exit+0x17 >>> intr_even_schedule_thread() at intr_event_schedule_thread+0xbb >>> ahci_end_transaction() at ahci_end_transaction+0x398 >>> ahci_ch_intr() at ahci_ch_intr+0x2b5 >>> ahcipoll() at ahcipoll+0x15 >>> xpt_polled_action() at xpt_polled_action+0xf7 >>> >>> In fact I once discussed with jhb this recursion triggered from a different >>> place. To quote myself: >>> spinlock_exit -> critical_exit -> mi_switch -> kdb_switch -> >>> thread_unlock -> spinlock_exit -> critical_exit -> mi_switch -> ... >>> in the kdb context >>> this issue seems to be triggered by td_owepreempt being true at >>> the time >>> kdb is entered >>> and there of course has to be an initial spinlock_exit call >>> somewhere >>> in my case it's because of usb keyboard >>> I wonder if it would make sense to clear td_owepreempt right >>> before >>> calling kdb_switch in mi_switch >>> instead of in sched_switch() >>> clearing td_owepreempt seems like a scheduler-independent >>> operation to me >>> or is it better to just skip locking in usb when kdb_active is set >>> ? >>> >>> The workaround described above should work in this case. >>> Another possibility is to pessimize mtx_unlock_spin() implementations to >>> check >>> SCHEDULER_STOPPED() and to bypass any further actions in that case. But >>> that >>> would add unnecessary overhead to the sunny day code paths. >>> >>> Going further up the stack one can come up with the following proposals: >>> - check SCHEDULER_STOPPED() swi_sched() and return early >>> - do not call swi_sched() from xpt_done() if we somehow know that we are >>> in a >>> polling mode >> >> There is no flag in CAM now to indicate polling mode, but if needed, it >> should not be difficult to add one and not call swi_sched(). > > I have the following change for eons on my test boxes. Without it, > I simply cannot get _any_ dump. > > diff --git a/sys/cam/cam_xpt.c b/sys/cam/cam_xpt.c > index 10b89c7..a38e42f 100644 > --- a/sys/cam/cam_xpt.c > +++ b/sys/cam/cam_xpt.c > @@ -4230,7 +4230,7 @@ xpt_done(union ccb *done_ccb) > TAILQ_INSERT_TAIL(&cam_simq, sim, links); > mtx_unlock(&cam_simq_lock); > sim->flags |= CAM_SIM_ON_DONEQ; > - if (first) > + if (first && panicstr == NULL) > swi_sched(cambio_ih, 0); > } > } That should be OK for kernel dumping. I was thinking about CAM abusing polling not only for dumping. But looking on cases where it does it now, may be it is better to rewrite them instead. -- Alexander Motin ___ freebsd-current@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/freebsd-current To unsubscribe, send any mail to "freebsd-current-unsubscr...@freebsd.org"
Re: Stop scheduler on panic
on 17/11/2011 10:15 Kostik Belousov said the following: > On Thu, Nov 17, 2011 at 01:07:38AM +0200, Alexander Motin wrote: >> On 17.11.2011 00:21, Andriy Gapon wrote: >>> Going further up the stack one can come up with the following proposals: >>> - check SCHEDULER_STOPPED() swi_sched() and return early >>> - do not call swi_sched() from xpt_done() if we somehow know that we are >>> in a >>> polling mode >> >> There is no flag in CAM now to indicate polling mode, but if needed, it >> should not be difficult to add one and not call swi_sched(). > > I have the following change for eons on my test boxes. Without it, > I simply cannot get _any_ dump. > > diff --git a/sys/cam/cam_xpt.c b/sys/cam/cam_xpt.c > index 10b89c7..a38e42f 100644 > --- a/sys/cam/cam_xpt.c > +++ b/sys/cam/cam_xpt.c > @@ -4230,7 +4230,7 @@ xpt_done(union ccb *done_ccb) > TAILQ_INSERT_TAIL(&cam_simq, sim, links); > mtx_unlock(&cam_simq_lock); > sim->flags |= CAM_SIM_ON_DONEQ; > - if (first) > + if (first && panicstr == NULL) > swi_sched(cambio_ih, 0); > } > } I think that this (or similar) change should go into the patch and the tree. -- Andriy Gapon ___ freebsd-current@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/freebsd-current To unsubscribe, send any mail to "freebsd-current-unsubscr...@freebsd.org"
Re: Stop scheduler on panic
On Thu, Nov 17, 2011 at 01:07:38AM +0200, Alexander Motin wrote: > On 17.11.2011 00:21, Andriy Gapon wrote: > >on 16/11/2011 21:27 Fabian Keil said the following: > >>Kostik Belousov wrote: > >> > >>>I was tricked into finishing the work by Andrey Gapon, who developed > >>>the patch to reliably stop other processors on panic. The patch > >>>greatly improves the chances of getting dump on panic on SMP host. > >> > >>I tested the patch trying to get a dump (from the debugger) for > >>kern/162036, which currently results in the double fault reported in: > >>http://lists.freebsd.org/pipermail/freebsd-current/2011-September/027766.html > >> > >>It didn't help, but also didn't make anything worse. > >> > >>Fabian > > > >The mi_switch recursion looks very familiar to me: > >mi_switch() at mi_switch+0x270 > >critical_exit() at critical_exit+0x9b > >spinlock_exit() at spinlock_exit+0x17 > >mi_switch() at mi_switch+0x275 > >critical_exit() at critical_exit+0x9b > >spinlock_exit() at spinlock_exit+0x17 > >[several pages of the previous three lines skipped] > >mi_switch() at mi_switch+0x275 > >critical_exit() at critical_exit+0x9b > >spinlock_exit() at spinlock_exit+0x17 > >intr_even_schedule_thread() at intr_event_schedule_thread+0xbb > >ahci_end_transaction() at ahci_end_transaction+0x398 > >ahci_ch_intr() at ahci_ch_intr+0x2b5 > >ahcipoll() at ahcipoll+0x15 > >xpt_polled_action() at xpt_polled_action+0xf7 > > > >In fact I once discussed with jhb this recursion triggered from a different > >place. To quote myself: > >spinlock_exit -> critical_exit -> mi_switch -> kdb_switch -> > >thread_unlock -> spinlock_exit -> critical_exit -> mi_switch -> ... > >in the kdb context > >this issue seems to be triggered by td_owepreempt being true at > >the time > >kdb is entered > >and there of course has to be an initial spinlock_exit call > >somewhere > >in my case it's because of usb keyboard > >I wonder if it would make sense to clear td_owepreempt right > >before > >calling kdb_switch in mi_switch > >instead of in sched_switch() > >clearing td_owepreempt seems like a scheduler-independent > >operation to me > >or is it better to just skip locking in usb when kdb_active is set > >? > > > >The workaround described above should work in this case. > >Another possibility is to pessimize mtx_unlock_spin() implementations to > >check > >SCHEDULER_STOPPED() and to bypass any further actions in that case. But > >that > >would add unnecessary overhead to the sunny day code paths. > > > >Going further up the stack one can come up with the following proposals: > >- check SCHEDULER_STOPPED() swi_sched() and return early > >- do not call swi_sched() from xpt_done() if we somehow know that we are > >in a > >polling mode > > There is no flag in CAM now to indicate polling mode, but if needed, it > should not be difficult to add one and not call swi_sched(). I have the following change for eons on my test boxes. Without it, I simply cannot get _any_ dump. diff --git a/sys/cam/cam_xpt.c b/sys/cam/cam_xpt.c index 10b89c7..a38e42f 100644 --- a/sys/cam/cam_xpt.c +++ b/sys/cam/cam_xpt.c @@ -4230,7 +4230,7 @@ xpt_done(union ccb *done_ccb) TAILQ_INSERT_TAIL(&cam_simq, sim, links); mtx_unlock(&cam_simq_lock); sim->flags |= CAM_SIM_ON_DONEQ; - if (first) + if (first && panicstr == NULL) swi_sched(cambio_ih, 0); } } pgp3ESGBc0NMm.pgp Description: PGP signature
Re: Stop scheduler on panic
On 17.11.2011 00:21, Andriy Gapon wrote: on 16/11/2011 21:27 Fabian Keil said the following: Kostik Belousov wrote: I was tricked into finishing the work by Andrey Gapon, who developed the patch to reliably stop other processors on panic. The patch greatly improves the chances of getting dump on panic on SMP host. I tested the patch trying to get a dump (from the debugger) for kern/162036, which currently results in the double fault reported in: http://lists.freebsd.org/pipermail/freebsd-current/2011-September/027766.html It didn't help, but also didn't make anything worse. Fabian The mi_switch recursion looks very familiar to me: mi_switch() at mi_switch+0x270 critical_exit() at critical_exit+0x9b spinlock_exit() at spinlock_exit+0x17 mi_switch() at mi_switch+0x275 critical_exit() at critical_exit+0x9b spinlock_exit() at spinlock_exit+0x17 [several pages of the previous three lines skipped] mi_switch() at mi_switch+0x275 critical_exit() at critical_exit+0x9b spinlock_exit() at spinlock_exit+0x17 intr_even_schedule_thread() at intr_event_schedule_thread+0xbb ahci_end_transaction() at ahci_end_transaction+0x398 ahci_ch_intr() at ahci_ch_intr+0x2b5 ahcipoll() at ahcipoll+0x15 xpt_polled_action() at xpt_polled_action+0xf7 In fact I once discussed with jhb this recursion triggered from a different place. To quote myself: spinlock_exit -> critical_exit -> mi_switch -> kdb_switch -> thread_unlock -> spinlock_exit -> critical_exit -> mi_switch -> ... in the kdb context this issue seems to be triggered by td_owepreempt being true at the time kdb is entered and there of course has to be an initial spinlock_exit call somewhere in my case it's because of usb keyboard I wonder if it would make sense to clear td_owepreempt right before calling kdb_switch in mi_switch instead of in sched_switch() clearing td_owepreempt seems like a scheduler-independent operation to me or is it better to just skip locking in usb when kdb_active is set ? The workaround described above should work in this case. Another possibility is to pessimize mtx_unlock_spin() implementations to check SCHEDULER_STOPPED() and to bypass any further actions in that case. But that would add unnecessary overhead to the sunny day code paths. Going further up the stack one can come up with the following proposals: - check SCHEDULER_STOPPED() swi_sched() and return early - do not call swi_sched() from xpt_done() if we somehow know that we are in a polling mode There is no flag in CAM now to indicate polling mode, but if needed, it should not be difficult to add one and not call swi_sched(). -- Alexander Motin ___ freebsd-current@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/freebsd-current To unsubscribe, send any mail to "freebsd-current-unsubscr...@freebsd.org"
Re: Stop scheduler on panic
on 16/11/2011 21:27 Fabian Keil said the following: > Kostik Belousov wrote: > >> I was tricked into finishing the work by Andrey Gapon, who developed >> the patch to reliably stop other processors on panic. The patch >> greatly improves the chances of getting dump on panic on SMP host. > > I tested the patch trying to get a dump (from the debugger) for > kern/162036, which currently results in the double fault reported in: > http://lists.freebsd.org/pipermail/freebsd-current/2011-September/027766.html > > It didn't help, but also didn't make anything worse. > > Fabian The mi_switch recursion looks very familiar to me: mi_switch() at mi_switch+0x270 critical_exit() at critical_exit+0x9b spinlock_exit() at spinlock_exit+0x17 mi_switch() at mi_switch+0x275 critical_exit() at critical_exit+0x9b spinlock_exit() at spinlock_exit+0x17 [several pages of the previous three lines skipped] mi_switch() at mi_switch+0x275 critical_exit() at critical_exit+0x9b spinlock_exit() at spinlock_exit+0x17 intr_even_schedule_thread() at intr_event_schedule_thread+0xbb ahci_end_transaction() at ahci_end_transaction+0x398 ahci_ch_intr() at ahci_ch_intr+0x2b5 ahcipoll() at ahcipoll+0x15 xpt_polled_action() at xpt_polled_action+0xf7 In fact I once discussed with jhb this recursion triggered from a different place. To quote myself: spinlock_exit -> critical_exit -> mi_switch -> kdb_switch -> thread_unlock -> spinlock_exit -> critical_exit -> mi_switch -> ... in the kdb context this issue seems to be triggered by td_owepreempt being true at the time kdb is entered and there of course has to be an initial spinlock_exit call somewhere in my case it's because of usb keyboard I wonder if it would make sense to clear td_owepreempt right before calling kdb_switch in mi_switch instead of in sched_switch() clearing td_owepreempt seems like a scheduler-independent operation to me or is it better to just skip locking in usb when kdb_active is set ? The workaround described above should work in this case. Another possibility is to pessimize mtx_unlock_spin() implementations to check SCHEDULER_STOPPED() and to bypass any further actions in that case. But that would add unnecessary overhead to the sunny day code paths. Going further up the stack one can come up with the following proposals: - check SCHEDULER_STOPPED() swi_sched() and return early - do not call swi_sched() from xpt_done() if we somehow know that we are in a polling mode -- Andriy Gapon ___ freebsd-current@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/freebsd-current To unsubscribe, send any mail to "freebsd-current-unsubscr...@freebsd.org"
Re: Stop scheduler on panic
Kostik Belousov wrote: > I was tricked into finishing the work by Andrey Gapon, who developed > the patch to reliably stop other processors on panic. The patch > greatly improves the chances of getting dump on panic on SMP host. I tested the patch trying to get a dump (from the debugger) for kern/162036, which currently results in the double fault reported in: http://lists.freebsd.org/pipermail/freebsd-current/2011-September/027766.html It didn't help, but also didn't make anything worse. Fabian signature.asc Description: PGP signature
Re: Stop scheduler on panic
on 14/11/2011 15:08 Kostik Belousov said the following: > On Mon, Nov 14, 2011 at 12:06:48PM +0200, Andriy Gapon wrote: >> On a more serious note: >> - some code in my latest version of the patch was contributed by or was based >> on the code or ideas contributed by jhb and mdf (so that attributions are not >> lost) Oops, forgot the most recent and quite big contribution from Attilio. > Please provide me with proper attribution for contributors and testers. My memory has faded a bit, so let's make it simple. In cooperation with:attilio, avg, jhb, kib, mdf Tested by: avg, Eugene Grosbein , gnn, Steven Hartland , glebius, kib [strike out obvious/unnecessary] >> - there was a concern about how sync-on-panic would work >> >> About the latter, I have never really tested it. mdf has suggested to >> move the sync-on-panic code to a place after we ensure that there is >> only one CPU in panic(), but before we stop other CPUs. > sync_on_panic is incompatible with the patch. I argue that it provides > non-zero chance of damaging good filesystems even if panic was unrelated > to the fs/bio/device layer. As an example, consider the case when other > CPU was modifying in-memory representation of the metadata, and panic > happen on this CPU. If you write half-changed block back, you make more > damage to the filesystem then if you do not. The half-backed sync spoils > any journaling or SU consistency guarantees. Not sure how this is different from what we have right now (without the patch). Perhaps I misunderstand what you say, but, just in case, the proposal was to do sync-on-panic before stopping other CPUs. > The issues of multithreading nature of our storage subsystem are secondary. > > The user who sets either tunable shall know what he does. That has always been the case. and apparently there are people who still want this option to exist. >> I think that I've already sent you a list of the early testers for >> various WIP versions of the patch. > I do not have the list. Included above. > BTW, if you want, feel free to handle the commit youself. You definitely > spent much more efforts on the stuff and deserve the credit. > > I was promised in private that a review will be provided during this > weekend. Unfortunately too little time and spare mind capacity to do the commit now. I do not care much about the credit (or commit-o-meter) as long as we get functionality in. It was/is a collective effort in any case. Besides I won't be able to handle any potential regression reports in a sufficiently speedy manner. -- Andriy Gapon signature.asc Description: OpenPGP digital signature
Re: Stop scheduler on panic
On Mon, Nov 14, 2011 at 12:06:48PM +0200, Andriy Gapon wrote: > on 13/11/2011 10:32 Kostik Belousov said the following: > > I was tricked into finishing the work by Andrey Gapon, who developed the > > patch to reliably stop other processors on panic. The patch greatly > > improves the chances of getting dump on panic on SMP host. Several people > > already saw the patchset, and I remember that Andrey posted it to some > > lists. > > > > The change stops other (*) processors early upon the panic. This way, no > > parallel manipulation of the kernel memory is performed by CPUs. In > > particular, the kernel memory map is static. Patch prevents the panic > > thread from blocking and switching out. > > > > * - in the context of the description, other means not current. > > > > Since other threads are not run anymore, lock owner cannot release a lock > > which is required by panic thread. Due to this, we need to fake a lock > > acquisition after the panic, which adds minimal overhead to the locking > > cost. The patch tries to not add any overhead on the fast path of the lock > > acquire. The check for the after-panic condition was reduced to single > > memory access, done only when the quick cas lock attempt failed, and braced > > with __unlikely compiler hint. > > > > For now, the new mode of operation is disabled by default, since some > > further USB changes are needed to make USB keyboard usable in that > > environment. > > > > With the patch, getting a dump from the machine without debugger compiled > > in is much more realistic. Please comment, I will commit the change in 2 > > weeks unless strong reasons not to are given. > > > > http://people.freebsd.org/~kib/misc/stop_cpus_on_panic.1.patch > > > > > On a more serious note: > - some code in my latest version of the patch was contributed by or was based > on the code or ideas contributed by jhb and mdf (so that attributions are not > lost) Please provide me with proper attribution for contributors and testers. > - there was a concern about how sync-on-panic would work > > About the latter, I have never really tested it. mdf has suggested to > move the sync-on-panic code to a place after we ensure that there is > only one CPU in panic(), but before we stop other CPUs. sync_on_panic is incompatible with the patch. I argue that it provides non-zero chance of damaging good filesystems even if panic was unrelated to the fs/bio/device layer. As an example, consider the case when other CPU was modifying in-memory representation of the metadata, and panic happen on this CPU. If you write half-changed block back, you make more damage to the filesystem then if you do not. The half-backed sync spoils any journaling or SU consistency guarantees. The issues of multithreading nature of our storage subsystem are secondary. The user who sets either tunable shall know what he does. > > I think that I've already sent you a list of the early testers for > various WIP versions of the patch. I do not have the list. BTW, if you want, feel free to handle the commit youself. You definitely spent much more efforts on the stuff and deserve the credit. I was promised in private that a review will be provided during this weekend. pgpyIteKouhHY.pgp Description: PGP signature
Re: Stop scheduler on panic
on 13/11/2011 10:32 Kostik Belousov said the following: > I was tricked into finishing the work by Andrey Gapon, who developed the > patch to reliably stop other processors on panic. The patch greatly > improves the chances of getting dump on panic on SMP host. Several people > already saw the patchset, and I remember that Andrey posted it to some > lists. > > The change stops other (*) processors early upon the panic. This way, no > parallel manipulation of the kernel memory is performed by CPUs. In > particular, the kernel memory map is static. Patch prevents the panic > thread from blocking and switching out. > > * - in the context of the description, other means not current. > > Since other threads are not run anymore, lock owner cannot release a lock > which is required by panic thread. Due to this, we need to fake a lock > acquisition after the panic, which adds minimal overhead to the locking > cost. The patch tries to not add any overhead on the fast path of the lock > acquire. The check for the after-panic condition was reduced to single > memory access, done only when the quick cas lock attempt failed, and braced > with __unlikely compiler hint. > > For now, the new mode of operation is disabled by default, since some > further USB changes are needed to make USB keyboard usable in that > environment. > > With the patch, getting a dump from the machine without debugger compiled > in is much more realistic. Please comment, I will commit the change in 2 > weeks unless strong reasons not to are given. > > http://people.freebsd.org/~kib/misc/stop_cpus_on_panic.1.patch > On a more serious note: - some code in my latest version of the patch was contributed by or was based on the code or ideas contributed by jhb and mdf (so that attributions are not lost) - there was a concern about how sync-on-panic would work About the latter, I have never really tested it. mdf has suggested to move the sync-on-panic code to a place after we ensure that there is only one CPU in panic(), but before we stop other CPUs. I think that I've already sent you a list of the early testers for various WIP versions of the patch. And thanks a lot again. -- Andriy Gapon ___ freebsd-current@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/freebsd-current To unsubscribe, send any mail to "freebsd-current-unsubscr...@freebsd.org"
Re: Stop scheduler on panic
-BEGIN PGP SIGNED MESSAGE- Hash: SHA1 Kostik, thanks a lot! I am not really sorry for taking part in tricking you into doing this :) I guess I now own that cognac back :) on 13/11/2011 10:32 Kostik Belousov said the following: > I was tricked into finishing the work by Andrey Gapon, who developed the > patch to reliably stop other processors on panic. The patch greatly > improves the chances of getting dump on panic on SMP host. Several people > already saw the patchset, and I remember that Andrey posted it to some > lists. > > The change stops other (*) processors early upon the panic. This way, no > parallel manipulation of the kernel memory is performed by CPUs. In > particular, the kernel memory map is static. Patch prevents the panic > thread from blocking and switching out. > > * - in the context of the description, other means not current. > > Since other threads are not run anymore, lock owner cannot release a lock > which is required by panic thread. Due to this, we need to fake a lock > acquisition after the panic, which adds minimal overhead to the locking > cost. The patch tries to not add any overhead on the fast path of the lock > acquire. The check for the after-panic condition was reduced to single > memory access, done only when the quick cas lock attempt failed, and > braced with __unlikely compiler hint. > > For now, the new mode of operation is disabled by default, since some > further USB changes are needed to make USB keyboard usable in that > environment. > > With the patch, getting a dump from the machine without debugger compiled > in is much more realistic. Please comment, I will commit the change in 2 > weeks unless strong reasons not to are given. > > http://people.freebsd.org/~kib/misc/stop_cpus_on_panic.1.patch > - -- Andriy Gapon -BEGIN PGP SIGNATURE- Version: GnuPG v2.0.18 (FreeBSD) Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/ iQEcBAEBAgAGBQJOwOYqAAoJEHSlLSemUf4v8VYH/13H/MXhxOc6Vz6r+M6x4yDn 03/TwS+QO7+689KEWvr5NQjPcV7cI3Ku3UIS3QlSUQIZYhhepJjK1UgQHM7ra5yr qzXwpEEealJc7GKSjMTIAsjpfcPgRdT66nwymPZWS3ojZJVXYPiGmc4p3uDVrAgv MnuKOdOVUaeI4tOqYa4wWoCSz++tTpbqIGQrlLTWn8yzhGDvNdj2YTEPoETbrTWO E/t8r+BPqeE5pkJjEoDMOpdZqzBB/45x5krocVVCWZL8gt5hp7c5CutGe6lsJ3on Agv5CGyCQb50AmyapDhc/miECtf4qEekFxO3wiqNBTgHxoN2uxvkIMvUhNlxqGg= =yxdc -END PGP SIGNATURE- ___ freebsd-current@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/freebsd-current To unsubscribe, send any mail to "freebsd-current-unsubscr...@freebsd.org"
