Re: Stop scheduler on panic

2011-12-08 Thread John Baldwin

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 Gapona...@freebsd.org  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-07 Thread Attilio Rao
2011/12/7 Andriy Gapon a...@freebsd.org:
 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

2011-12-06 Thread Attilio Rao
2011/12/2 Andriy Gapon a...@freebsd.org:
 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 Baldwinj...@freebsd.org:
 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

2011-12-06 Thread Attilio Rao
2011/12/4 Andriy Gapon a...@freebsd.org:
 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-06 Thread Attilio Rao
2011/12/4 Andriy Gapon a...@freebsd.org:
 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-06 Thread Attilio Rao
2011/11/13 Kostik Belousov kostik...@gmail.com:
 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 for KDB
with this 

Re: Stop scheduler on panic

2011-12-06 Thread 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.

 @@ -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-12-06 Thread Attilio Rao
2011/12/6 Andriy Gapon a...@freebsd.org:
 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

2011-12-06 Thread 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.

-- 
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-04 Thread Andriy Gapon
on 02/12/2011 17:30 m...@freebsd.org said the following:
 On Fri, Dec 2, 2011 at 2:05 AM, Andriy Gapon a...@freebsd.org 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

2011-12-04 Thread 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!

-- 
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-04 Thread 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.

 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

2011-12-02 Thread Andriy Gapon
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

2011-12-02 Thread mdf
On Fri, Dec 2, 2011 at 2:05 AM, Andriy Gapon a...@freebsd.org 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

2011-12-02 Thread 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).


--
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-02 Thread Attilio Rao
2011/12/2 John Baldwin j...@freebsd.org:
 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

2011-12-02 Thread John Baldwin

On 12/2/11 12:18 PM, Attilio Rao wrote:

2011/12/2 John Baldwinj...@freebsd.org:

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-02 Thread Attilio Rao
2011/12/2 John Baldwin j...@freebsd.org:
 On 12/2/11 12:18 PM, Attilio Rao wrote:

 2011/12/2 John Baldwinj...@freebsd.org:

 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

2011-12-02 Thread 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 Baldwinj...@freebsd.org:
 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-01 Thread Andriy Gapon

[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-12-01 Thread John Baldwin
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

2011-12-01 Thread Andriy Gapon
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 a...@icyb.net.ua
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

2011-12-01 Thread John Baldwin
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 a...@icyb.net.ua
 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

2011-12-01 Thread Andriy Gapon
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 a...@icyb.net.ua
 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

2011-12-01 Thread John Baldwin

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 Gapona...@icyb.net.ua
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

2011-11-21 Thread 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).

 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

2011-11-21 Thread Attilio Rao
2011/11/21 John Baldwin j...@freebsd.org:
 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

2011-11-18 Thread Fabian Keil
Alexander Motin m...@freebsd.org 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 Belousovkostik...@gmail.com  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:
  avgspinlock_exit -  critical_exit -  mi_switch -  kdb_switch 
  -
  thread_unlock -  spinlock_exit -  critical_exit -  mi_switch -  
  ...
  avgin the kdb context
  avgthis issue seems to be triggered by td_owepreempt being true 
  at 
  the time
  kdb is entered
  avgand there of course has to be an initial spinlock_exit call 
  somewhere
  avgin my case it's because of usb keyboard
  avgI wonder if it would make sense to clear td_owepreempt right 
  before
  calling kdb_switch in mi_switch
  avginstead of in sched_switch()
  avgclearing td_owepreempt seems like a scheduler-independent 
  operation to me
  avgor is it better to just skip locking in usb when kdb_active 
  is set
  avg?
 
  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

2011-11-18 Thread Andriy Gapon
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

2011-11-17 Thread Kostik Belousov
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 Belousovkostik...@gmail.com  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:
 avgspinlock_exit -  critical_exit -  mi_switch -  kdb_switch -
 thread_unlock -  spinlock_exit -  critical_exit -  mi_switch -  ...
 avgin the kdb context
 avgthis issue seems to be triggered by td_owepreempt being true at 
 the time
 kdb is entered
 avgand there of course has to be an initial spinlock_exit call 
 somewhere
 avgin my case it's because of usb keyboard
 avgI wonder if it would make sense to clear td_owepreempt right 
 before
 calling kdb_switch in mi_switch
 avginstead of in sched_switch()
 avgclearing td_owepreempt seems like a scheduler-independent 
 operation to me
 avgor is it better to just skip locking in usb when kdb_active is set
 avg?
 
 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

2011-11-17 Thread Andriy Gapon
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

2011-11-17 Thread Alexander Motin
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 Belousovkostik...@gmail.com  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:
 avgspinlock_exit -  critical_exit -  mi_switch -  kdb_switch -
 thread_unlock -  spinlock_exit -  critical_exit -  mi_switch -  ...
 avgin the kdb context
 avgthis issue seems to be triggered by td_owepreempt being true at 
 the time
 kdb is entered
 avgand there of course has to be an initial spinlock_exit call 
 somewhere
 avgin my case it's because of usb keyboard
 avgI wonder if it would make sense to clear td_owepreempt right 
 before
 calling kdb_switch in mi_switch
 avginstead of in sched_switch()
 avgclearing td_owepreempt seems like a scheduler-independent 
 operation to me
 avgor is it better to just skip locking in usb when kdb_active is set
 avg?

 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

2011-11-17 Thread Kostik Belousov
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 Belousovkostik...@gmail.com  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:
  avgspinlock_exit -  critical_exit -  mi_switch -  kdb_switch -
  thread_unlock -  spinlock_exit -  critical_exit -  mi_switch -  ...
  avgin the kdb context
  avgthis issue seems to be triggered by td_owepreempt being true at 
  the time
  kdb is entered
  avgand there of course has to be an initial spinlock_exit call 
  somewhere
  avgin my case it's because of usb keyboard
  avgI wonder if it would make sense to clear td_owepreempt right 
  before
  calling kdb_switch in mi_switch
  avginstead of in sched_switch()
  avgclearing td_owepreempt seems like a scheduler-independent 
  operation to me
  avgor is it better to just skip locking in usb when kdb_active is 
  set
  avg?
 
  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

2011-11-17 Thread Alexander Motin
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 Belousovkostik...@gmail.com  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:
 avgspinlock_exit -  critical_exit -  mi_switch -  kdb_switch -
 thread_unlock -  spinlock_exit -  critical_exit -  mi_switch -  ...
 avgin the kdb context
 avgthis issue seems to be triggered by td_owepreempt being true at 
 the time
 kdb is entered
 avgand there of course has to be an initial spinlock_exit call 
 somewhere
 avgin my case it's because of usb keyboard
 avgI wonder if it would make sense to clear td_owepreempt right 
 before
 calling kdb_switch in mi_switch
 avginstead of in sched_switch()
 avgclearing td_owepreempt seems like a scheduler-independent 
 operation to me
 avgor is it better to just skip locking in usb when kdb_active is 
 set
 avg?

 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

2011-11-17 Thread Andriy Gapon
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

2011-11-17 Thread Kostik Belousov
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 Belousovkostik...@gmail.com  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:
  avgspinlock_exit -  critical_exit -  mi_switch -  kdb_switch -
  thread_unlock -  spinlock_exit -  critical_exit -  mi_switch -  ...
  avgin the kdb context
  avgthis issue seems to be triggered by td_owepreempt being true 
  at 
  the time
  kdb is entered
  avgand there of course has to be an initial spinlock_exit call 
  somewhere
  avgin my case it's because of usb keyboard
  avgI wonder if it would make sense to clear td_owepreempt right 
  before
  calling kdb_switch in mi_switch
  avginstead of in sched_switch()
  avgclearing td_owepreempt seems like a scheduler-independent 
  operation to me
  avgor is it better to just skip locking in usb when kdb_active is 
  set
  avg?
 
  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

2011-11-17 Thread Alexander Motin
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 Belousovkostik...@gmail.com  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:
 avgspinlock_exit -  critical_exit -  mi_switch -  kdb_switch -
 thread_unlock -  spinlock_exit -  critical_exit -  mi_switch -  ...
 avgin the kdb context
 avgthis issue seems to be triggered by td_owepreempt being true 
 at 
 the time
 kdb is entered
 avgand there of course has to be an initial spinlock_exit call 
 somewhere
 avgin my case it's because of usb keyboard
 avgI wonder if it would make sense to clear td_owepreempt right 
 before
 calling kdb_switch in mi_switch
 avginstead of in sched_switch()
 avgclearing td_owepreempt seems like a scheduler-independent 
 operation to me
 avgor is it better to just skip locking in usb when kdb_active is 
 set
 avg?

 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
===
--- cam_sim.h   (revision 227580)
+++ cam_sim.h   (working copy)
@@ -104,7 +104,8 @@
u_int32_t   flags;
 #defineCAM_SIM_REL_TIMEOUT_PENDING 0x01
 #defineCAM_SIM_MPSAFE  0x02
-#define CAM_SIM_ON_DONEQ   0x04
+#defineCAM_SIM_ON_DONEQ0x04
+#defineCAM_SIM_POLLED  0x08
struct callout  callout;
struct cam_devq *devq;  /* Device Queue to use for this SIM */
int refcount; /* References to the SIM. */
Index: cam_xpt.c

Re: Stop scheduler on panic

2011-11-17 Thread John Baldwin
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

2011-11-17 Thread Andriy Gapon
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

2011-11-17 Thread Andrew Boyer

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

2011-11-17 Thread Kostik Belousov
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 Belousovkostik...@gmail.com  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:
  avgspinlock_exit -  critical_exit -  mi_switch -  kdb_switch 
  -
  thread_unlock -  spinlock_exit -  critical_exit -  mi_switch -  
  ...
  avgin the kdb context
  avgthis issue seems to be triggered by td_owepreempt being true 
  at 
  the time
  kdb is entered
  avgand there of course has to be an initial spinlock_exit call 
  somewhere
  avgin my case it's because of usb keyboard
  avgI wonder if it would make sense to clear td_owepreempt right 
  before
  calling kdb_switch in mi_switch
  avginstead of in sched_switch()
  avgclearing td_owepreempt seems like a scheduler-independent 
  operation to me
  avgor is it better to just skip locking in usb when kdb_active 
  is set
  avg?
 
  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
 ===
 --- cam_sim.h (revision 227580)
 +++ cam_sim.h (working copy)
 @@ -104,7 +104,8 @@
   u_int32_t   flags;
  #define  CAM_SIM_REL_TIMEOUT_PENDING 0x01
  #define  CAM_SIM_MPSAFE  0x02
 -#define CAM_SIM_ON_DONEQ 0x04
 +#define  CAM_SIM_ON_DONEQ0x04
 +#define  CAM_SIM_POLLED  0x08
   

Re: Stop scheduler on panic

2011-11-17 Thread John Baldwin
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

2011-11-17 Thread 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.

-- 
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 Thread Attilio Rao
2011/11/17 Andriy Gapon a...@freebsd.org:
 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

2011-11-17 Thread mdf
On Thu, Nov 17, 2011 at 12:54 PM, Attilio Rao atti...@freebsd.org wrote:
 2011/11/17 Andriy Gapon a...@freebsd.org:
 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 Thread Attilio Rao
2011/11/17  m...@freebsd.org:
 On Thu, Nov 17, 2011 at 12:54 PM, Attilio Rao atti...@freebsd.org wrote:
 2011/11/17 Andriy Gapon a...@freebsd.org:
 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

2011-11-17 Thread Andriy Gapon
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 Thread mdf
On Thu, Nov 17, 2011 at 1:08 PM, Andriy Gapon a...@freebsd.org 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

2011-11-17 Thread John Baldwin
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

2011-11-17 Thread John Baldwin
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 (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.

-- 
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-11-16 Thread Fabian Keil
Kostik Belousov kostik...@gmail.com 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

2011-11-16 Thread Andriy Gapon
on 16/11/2011 21:27 Fabian Keil said the following:
 Kostik Belousov kostik...@gmail.com 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:
avg   spinlock_exit - critical_exit - mi_switch - kdb_switch -
thread_unlock - spinlock_exit - critical_exit - mi_switch - ...
avg   in the kdb context
avg   this issue seems to be triggered by td_owepreempt being true at the time
kdb is entered
avg   and there of course has to be an initial spinlock_exit call somewhere
avg   in my case it's because of usb keyboard
avg   I wonder if it would make sense to clear td_owepreempt right before
calling kdb_switch in mi_switch
avg   instead of in sched_switch()
avg   clearing td_owepreempt seems like a scheduler-independent operation to 
me
avg   or is it better to just skip locking in usb when kdb_active is set
avg   ?

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

2011-11-16 Thread Alexander Motin

On 17.11.2011 00:21, Andriy Gapon wrote:

on 16/11/2011 21:27 Fabian Keil said the following:

Kostik Belousovkostik...@gmail.com  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:
avgspinlock_exit -  critical_exit -  mi_switch -  kdb_switch -
thread_unlock -  spinlock_exit -  critical_exit -  mi_switch -  ...
avgin the kdb context
avgthis issue seems to be triggered by td_owepreempt being true at the 
time
kdb is entered
avgand there of course has to be an initial spinlock_exit call somewhere
avgin my case it's because of usb keyboard
avgI wonder if it would make sense to clear td_owepreempt right before
calling kdb_switch in mi_switch
avginstead of in sched_switch()
avgclearing td_owepreempt seems like a scheduler-independent operation to 
me
avgor is it better to just skip locking in usb when kdb_active is set
avg?

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

2011-11-14 Thread Andriy Gapon
-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


Re: Stop scheduler on panic

2011-11-14 Thread Andriy Gapon
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

2011-11-14 Thread Kostik Belousov
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

2011-11-14 Thread Andriy Gapon
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 eu...@grosbein.net,
gnn,
Steven Hartland kill...@multiplay.co.uk,
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